From d84b86b21b64fa486c5319077680616d3f71cddd Mon Sep 17 00:00:00 2001 From: Mike Gerwitz Date: Wed, 2 May 2012 13:26:47 -0400 Subject: [PATCH] Added `proxy' keyword support The concept of proxy methods will become an important, core concept in ease.js that will provide strong benefits for creating decorators and proxies, removing boilerplate code and providing useful metadata to the system. Consider the following example: Class( 'Foo', { // ... 'public performOperation': function( bar ) { this._doSomethingWith( bar ); return this; }, } ); Class( 'FooDecorator', { 'private _foo': null, // ... 'public performOperation': function( bar ) { return this._foo.performOperation( bar ); }, } ); In the above example, `FooDecorator` is a decorator for `Foo`. Assume that the `getValueOf()` method is undecorated and simply needs to be proxied to its component --- an instance of `Foo`. (It is not uncommon that a decorator, proxy, or related class will alter certain functionality while leaving much of it unchanged.) In order to do so, we can use this generic, boilerplate code return this.obj.func.apply( this.obj, arguments ); which would need to be repeated again and again for *each method that needs to be proxied*. We also have another problem --- `Foo.getValueOf()` returns *itself*, which `FooDecorator` *also* returns. This breaks encapsulation, so we instead need to return ourself: 'public performOperation': function( bar ) { this._foo.performOperation( bar ); return this; }, Our boilerplate code then becomes: var ret = this.obj.func.apply( this.obj, arguments ); return ( ret === this.obj ) ? this : ret; Alternatively, we could use the `proxy' keyword: Class( 'FooDecorator2', { 'private _foo': null, // ... 'public proxy performOperation': '_foo', } ); `FooDecorator2.getValueOf()` and `FooDecorator.getValueOf()` both perform the exact same task --- proxy the entire call to another object and return its result, unless the result is the component, in which case the decorator itself is returned. Proxies, as of this commit, accomplish the following: - All arguments are forwarded to the destination - The return value is forwarded to the caller - If the destination returns a reference to itself, it will be replaced with a reference to the caller's context (`this`). - If the call is expected to fail, either because the destination is not an object or because the requested method is not a function, a useful error will be immediately thrown (rather than the potentially cryptic one that would otherwise result, requiring analysis of the stack trace). N.B. As of this commit, static proxies do not yet function properly. --- lib/MemberBuilder.js | 37 ++++- lib/MemberBuilderValidator.js | 19 +++ lib/MethodWrapperFactory.js | 7 +- lib/MethodWrappers.js | 35 +++++ lib/class.js | 1 + lib/interface.js | 1 + lib/prop_parser.js | 1 + lib/util.js | 2 +- test/MemberBuilder/GetterSetterTest.js | 2 +- test/MemberBuilder/MethodTest.js | 58 +++++++- test/MemberBuilder/PropTest.js | 2 +- test/MemberBuilder/VisibilityTest.js | 2 +- test/MemberBuilderValidator/MethodTest.js | 49 +++++++ test/MemberBuilderValidator/inc-common.js | 5 +- test/MethodWrapperFactoryTest.js | 11 +- test/MethodWrappersTest.js | 162 ++++++++++++++++++++++ test/test-class-abstract.js | 3 - test/test-class_builder-const.js | 1 + test/test-class_builder-static.js | 1 + test/test-class_builder-visibility.js | 1 + test/test-util-prop-parse.js | 25 ++++ 21 files changed, 409 insertions(+), 16 deletions(-) diff --git a/lib/MemberBuilder.js b/lib/MemberBuilder.js index 3eaa117..b5f9d28 100644 --- a/lib/MemberBuilder.js +++ b/lib/MemberBuilder.js @@ -42,20 +42,26 @@ var util = require( __dirname + '/util' ), * * @param {Function} wrap_method method wrapper * @param {Function} wrap_override method override wrapper + * @param {Function} wrap_proxy method proxy wrapper * @param {MemberBuilderValidator} validate member validator * * @constructor */ -module.exports = function MemberBuilder( wrap_method, wrap_override, validate ) +module.exports = function MemberBuilder( + wrap_method, wrap_override, wrap_proxy, validate +) { // permit omitting 'new' keyword if ( !( this instanceof module.exports ) ) { - return new module.exports( wrap_method, wrap_override, validate ); + return new module.exports( + wrap_method, wrap_override, wrap_proxy, validate + ); } this._wrapMethod = wrap_method; this._wrapOverride = wrap_override; + this._wrapProxy = wrap_proxy; this._validate = validate; }; @@ -125,7 +131,11 @@ exports.buildMethod = function( ); // we might be overriding an existing method - if ( prev ) + if ( keywords[ 'proxy' ] ) + { + dest[ name ] = this._createProxy( value, instCallback, cid, name ); + } + else if ( prev ) { if ( keywords[ 'override' ] || prev_keywords[ 'abstract' ] ) @@ -355,6 +365,27 @@ function hideMethod( super_method, new_method, instCallback, cid ) } +/** + * Create a method that proxies to the method of another object + * + * @param {string} proxy_to name of property (of instance) to proxy to + * + * @param {Function} instCallback function to call in order to retrieve + * object to bind 'this' keyword to + * + * @param {number} cid class id + * @param {string} mname name of method to invoke on destination object + * + * @return {Function} proxy method + */ +exports._createProxy = function( proxy_to, instCallback, cid, mname ) +{ + return this._wrapProxy.wrapMethod( + proxy_to, null, cid, instCallback, mname + ); +}; + + /** * Generates a method override function * diff --git a/lib/MemberBuilderValidator.js b/lib/MemberBuilderValidator.js index bb5799e..5b53759 100644 --- a/lib/MemberBuilderValidator.js +++ b/lib/MemberBuilderValidator.js @@ -94,6 +94,25 @@ exports.prototype.validateMethod = function( ); } + if ( keywords[ 'proxy' ] ) + { + // proxies are expected to provide the name of the destination object + if ( typeof value !== 'string' ) + { + throw TypeError( + "Cannot declare proxy method '" + name + "'; string value " + + "expected" + ); + } + else if ( keywords[ 'abstract' ] ) + { + // proxies are always concrete + throw TypeError( + "Proxy method '" + name + "' cannot be abstract" + ); + } + } + // search for any previous instances of this member if ( prev ) { diff --git a/lib/MethodWrapperFactory.js b/lib/MethodWrapperFactory.js index 2c29c41..c1c6a80 100644 --- a/lib/MethodWrapperFactory.js +++ b/lib/MethodWrapperFactory.js @@ -55,9 +55,12 @@ module.exports = exports = function MethodWrapperFactory( factory ) * @param {number} cid class id that method is associated with * @param {function()} getInst function to determine instance and return * associated visibility object + * @param {string=} name name of method */ -exports.prototype.wrapMethod = function( method, super_method, cid, getInst ) +exports.prototype.wrapMethod = function( + method, super_method, cid, getInst, name +) { - return this._factory( method, super_method, cid, getInst ); + return this._factory( method, super_method, cid, getInst, name ); }; diff --git a/lib/MethodWrappers.js b/lib/MethodWrappers.js index bd2734d..4d3ecd1 100644 --- a/lib/MethodWrappers.js +++ b/lib/MethodWrappers.js @@ -81,5 +81,40 @@ exports.standard = { return retval; }; }, + + + wrapProxy: function( proxy_to, _, cid, getInst, name ) + { + return function() + { + var context = getInst( this, cid ) || this, + retval = undefined, + dest = context[ proxy_to ] + ; + + // rather than allowing a cryptic error to be thrown, attempt to + // detect when the proxy call will fail and provide a useful error + // message + if ( ( typeof dest !== 'object' ) + || ( typeof dest[ name ] !== 'function' ) + ) + { + throw TypeError( + "Unable to proxy " + name + "() call to '" + proxy_to + + "'; '" + proxy_to + "' is undefined or '" + name + + "' is not a function." + ); + } + + retval = dest[ name ].apply( dest, arguments ); + + // if the object we are proxying to returns itself, then instead + // return a reference to *ourself* (so as not to break encapsulation + // and to provide a more consistent and sensible API) + return ( retval === dest ) + ? this + : retval; + }; + }, }; diff --git a/lib/class.js b/lib/class.js index d5bd987..31701e1 100644 --- a/lib/class.js +++ b/lib/class.js @@ -34,6 +34,7 @@ var util = require( __dirname + '/util' ), require( __dirname + '/MemberBuilder' )( MethodWrapperFactory( wrappers.wrapNew ), MethodWrapperFactory( wrappers.wrapOverride ), + MethodWrapperFactory( wrappers.wrapProxy ), require( __dirname + '/MemberBuilderValidator' )( function( warning ) { diff --git a/lib/interface.js b/lib/interface.js index 40738c8..f918a37 100644 --- a/lib/interface.js +++ b/lib/interface.js @@ -29,6 +29,7 @@ var util = require( __dirname + '/util' ), member_builder = require( __dirname + '/MemberBuilder' )( MethodWrapperFactory( wrappers.wrapNew ), MethodWrapperFactory( wrappers.wrapOverride ), + MethodWrapperFactory( wrappers.wrapProxy ), require( __dirname + '/MemberBuilderValidator' )() ), diff --git a/lib/prop_parser.js b/lib/prop_parser.js index 88be1d0..9a27845 100644 --- a/lib/prop_parser.js +++ b/lib/prop_parser.js @@ -34,6 +34,7 @@ var _keywords = { 'const': true, 'virtual': true, 'override': true, + 'proxy': true, }; diff --git a/lib/util.js b/lib/util.js index 3f1b6ba..34c0fb6 100644 --- a/lib/util.js +++ b/lib/util.js @@ -333,7 +333,7 @@ exports.propParse = function( data, options ) ); } // method - else if ( typeof value === 'function' ) + else if ( ( typeof value === 'function' ) || ( keywords[ 'proxy' ] ) ) { callbackMethod.call( callbackMethod, diff --git a/test/MemberBuilder/GetterSetterTest.js b/test/MemberBuilder/GetterSetterTest.js index b9941dc..8dc5217 100644 --- a/test/MemberBuilder/GetterSetterTest.js +++ b/test/MemberBuilder/GetterSetterTest.js @@ -68,7 +68,7 @@ require( 'common' ).testCase( ); this.sut = this.require( 'MemberBuilder' )( - stubFactory, stubFactory, + stubFactory, stubFactory, stubFactory, this.mockValidate = this.getMock( 'MemberBuilderValidator' ) ); diff --git a/test/MemberBuilder/MethodTest.js b/test/MemberBuilder/MethodTest.js index aa1fe77..ecaee7f 100644 --- a/test/MemberBuilder/MethodTest.js +++ b/test/MemberBuilder/MethodTest.js @@ -59,13 +59,27 @@ require( 'common' ).testCase( setUp: function() { + var _self = this; + // stub factories used for testing var stubFactory = this.require( 'MethodWrapperFactory' )( function( func ) { return func; } ); + // used for testing proxies explicitly + var stubProxyFactory = this.require( 'MethodWrapperFactory' )( + function() + { + _self.proxyFactoryCall = arguments; + return _self.proxyReturnValue; + } + ); + + this.proxyFactoryCall = null; + this.proxyReturnValue = function() {}; + this.sut = this.require( 'MemberBuilder' )( - stubFactory, stubFactory, + stubFactory, stubFactory, stubProxyFactory, this.mockValidate = this.getMock( 'MemberBuilderValidator' ) ); @@ -127,4 +141,46 @@ require( 'common' ).testCase( this.assertEqual( true, called, 'validateMethod() was not called' ); }, + + + /** + * The `proxy' keyword should result in a method that proxies to the given + * object's method (both identified by name). + */ + "Creates proxy when `proxy' keyword is given": function() + { + var _self = this, + called = false, + + cid = 1, + name = 'foo', + value = 'bar', + keywords = { 'proxy': true }, + + instCallback = function() {} + ; + + // build the proxy + this.sut.buildMethod( + this.members, {}, name, value, keywords, instCallback, cid, {} + ); + + this.assertNotEqual( null, this.proxyFactoryCall, + "Proxy factory should be used when `proxy' keyword is provided" + ); + + this.assertDeepEqual( + [ value, null, cid, instCallback, name ], + this.proxyFactoryCall, + "Proxy factory should be called with proper arguments" + ); + + // ensure it was properly generated (use a strict check to ensure the + // *proper* value is returned) + this.assertStrictEqual( + this.proxyReturnValue, + this.members[ 'public' ][ name ], + "Generated proxy method should be properly assigned to members" + ); + }, } ); diff --git a/test/MemberBuilder/PropTest.js b/test/MemberBuilder/PropTest.js index 97631e2..47b5e26 100644 --- a/test/MemberBuilder/PropTest.js +++ b/test/MemberBuilder/PropTest.js @@ -65,7 +65,7 @@ require( 'common' ).testCase( ); this.sut = this.require( 'MemberBuilder' )( - stubFactory, stubFactory, + stubFactory, stubFactory, stubFactory, this.mockValidate = this.getMock( 'MemberBuilderValidator' ) ); diff --git a/test/MemberBuilder/VisibilityTest.js b/test/MemberBuilder/VisibilityTest.js index 7b457a7..fa05f99 100644 --- a/test/MemberBuilder/VisibilityTest.js +++ b/test/MemberBuilder/VisibilityTest.js @@ -214,7 +214,7 @@ require( 'common' ).testCase( ); this.sut = this.require( 'MemberBuilder' )( - stubFactory, stubFactory, + stubFactory, stubFactory, stubFactory, this.getMock( 'MemberBuilderValidator' ) ); diff --git a/test/MemberBuilderValidator/MethodTest.js b/test/MemberBuilderValidator/MethodTest.js index 781705d..750a312 100644 --- a/test/MemberBuilderValidator/MethodTest.js +++ b/test/MemberBuilderValidator/MethodTest.js @@ -378,5 +378,54 @@ require( 'common' ).testCase( _self.quickVisChangeTest( cur[ 0 ], cur[ 1 ], true, 'conflict' ); } ); }, + + + /** + * Proxies forward calls to other properties of a given instance. The only + * way to represent those properties is by name, which we will use a string + * to accomplish. Therefore, the value of a proxy method must be the name of + * the property to proxy to (as a string). + */ + "`proxy' keyword must provide string value": function() + { + var name = 'foo', + _self = this; + + this.quickFailureTest( name, 'string value expected', function() + { + // provide function instead of string + _self.sut.validateMethod( + name, function() {}, { 'proxy': true }, {}, {} + ); + } ); + }, + + + /** + * Similar to the above test, but asserts that string values are permitted. + */ + "`proxy' keyword can provide string value": function() + { + var _self = this; + + this.assertDoesNotThrow( function() + { + _self.sut.validateMethod( + 'foo', 'dest', { 'proxy': true }, {}, {} + ); + }, TypeError ); + }, + + + /** + * It does not make sense for a proxy to be abstract; proxies are concrete + * by definition (in ease.js' context, at least). + */ + 'Method proxy cannot be abstract': function() + { + this.quickKeywordMethodTest( [ 'proxy', 'abstract' ], + 'cannot be abstract' + ); + }, } ); diff --git a/test/MemberBuilderValidator/inc-common.js b/test/MemberBuilderValidator/inc-common.js index 0b39701..2f65cf2 100644 --- a/test/MemberBuilderValidator/inc-common.js +++ b/test/MemberBuilderValidator/inc-common.js @@ -113,8 +113,11 @@ exports.quickKeywordTest = function( var testfunc = function() { + // proxies use strings, while all others use functions + var val = ( keyword_obj[ 'proxy' ] ) ? 'proxyDest': function() {}; + _self.sut[ type ]( - name, function() {}, keyword_obj, prev_data, prev_obj + name, val, keyword_obj, prev_data, prev_obj ); }; diff --git a/test/MethodWrapperFactoryTest.js b/test/MethodWrapperFactoryTest.js index ed9e302..01d1da8 100644 --- a/test/MethodWrapperFactoryTest.js +++ b/test/MethodWrapperFactoryTest.js @@ -64,10 +64,13 @@ var common = require( './common' ), super_method = function() {}, cid = 55, getInst = function() {}, + name = 'someMethod', retval = 'foobar'; var result = Sut( - function( given_method, given_super, given_cid, givenGetInst ) + function( + given_method, given_super, given_cid, givenGetInst, givenName + ) { called = true; @@ -87,9 +90,13 @@ var common = require( './common' ), "Factory method should be provided with proper inst function" ); + assert.equal( givenName, name, + "Factory method should be provided with proper method name" + ); + return retval; } - ).wrapMethod( method, super_method, cid, getInst ); + ).wrapMethod( method, super_method, cid, getInst, name ); // we'll include this in addition to the following assertion (which is // redundant) to make debugging more clear diff --git a/test/MethodWrappersTest.js b/test/MethodWrappersTest.js index 868659e..a5a1564 100644 --- a/test/MethodWrappersTest.js +++ b/test/MethodWrappersTest.js @@ -180,3 +180,165 @@ var common = require( './common' ), ); } )(); + +/** + * The proxy wrapper should forward all arguments to the provided object's + * appropriate method. The return value should also be proxied back to the + * caller. + */ +( function testProxyWillProperlyForwardCallToDestinationObject() +{ + var name = 'someMethod', + propname = 'dest', + + args = [ 1, {}, 'three' ], + args_given = [], + + getInst = function() + { + return inst; + }, + + method_retval = {}, + dest = { + someMethod: function() + { + args_given = Array.prototype.slice.call( arguments ); + return method_retval; + }, + }, + + // acts like a class instance + inst = { dest: dest }, + + proxy = sut.standard.wrapProxy( propname, null, 0, getInst, name ) + ; + + assert.strictEqual( method_retval, proxy.apply( inst, args ), + "Proxy call should return the value from the destination" + ); + + assert.deepEqual( args, args_given, + "All arguments should be properly forwarded to the destination" + ); +} )(); + + +/** + * If the destination object returns itself, then we should return the context + * in which the proxy was called; this ensures that we do not break + * encapsulation. Consequently, it also provides a more consistent and sensical + * API and permits method chaining. + * + * If this is not the desired result, then the user is free to forefit the proxy + * wrapper and instead use a normal method, manually proxying the call. + */ +( function testProxyReturnValueIsReplacedWithContextIfDestinationReturnsSelf() +{ + var propname = 'foo', + method = 'bar', + + foo = { + bar: function() + { + // return "self" + return foo; + } + }, + + inst = { foo: foo }, + + ret = sut.standard.wrapProxy( + propname, null, 0, + function() + { + return inst; + }, + method + ).call( inst ) + ; + + assert.strictEqual( inst, ret, + "Proxy should return instance in place of destination, if returned" + ); +} )(); + + +// common assertions between a couple of proxy tests +function proxyErrorAssertCommon( e, prop, method ) +{ + assert.ok( + e.message.search( 'Unable to proxy' ) > -1, + "Unexpected error received: " + e.message + ); + + assert.ok( + ( ( e.message.search( prop ) > -1 ) + && ( e.message.search( method ) > -1 ) + ), + "Error should contain property and method names" + ); +} + + +/** + * Rather than allowing a cryptic error to be thrown by the engine, take some + * initiative and attempt to detect when a call will fail due to the destination + * not being an object. + */ +( function testProxyThrowsErrorIfCallWillFailDueToNonObject() +{ + var prop = 'noexist', + method = 'foo'; + + try + { + // should fail because 'noexist' does not exist on the object + sut.standard.wrapProxy( + prop, null, 0, + function() { return {}; }, + method + )(); + } + catch ( e ) + { + proxyErrorAssertCommon( e, prop, method ); + return; + } + + assert.fail( + "Error should be thrown if proxy would fail due to a non-object" + ); +} )(); + + +/** + * Rather than allowing a cryptic error to be thrown by the engine, take some + * initiative and attempt to detect when a call will fail due to the destination + * method not being a function. + */ +( function testProxyThrowsErrorIfCallWillFailDueToNonObject() +{ + var prop = 'dest', + method = 'foo'; + + try + { + // should fail because 'noexist' does not exist on the object + sut.standard.wrapProxy( + prop, null, 0, + function() { return { dest: { foo: 'notafunc' } }; }, + method + )(); + } + catch ( e ) + { + proxyErrorAssertCommon( e, prop, method ); + return; + } + + assert.fail( + "Error should be thrown if proxy would fail due to a non-function" + ); +} )(); + diff --git a/test/test-class-abstract.js b/test/test-class-abstract.js index 7bc40c2..9ca321a 100644 --- a/test/test-class-abstract.js +++ b/test/test-class-abstract.js @@ -461,13 +461,10 @@ var ConcreteFoo = Class.extend( AbstractFoo, */ ( function testImplementingInterfacesWillPreserveAbstractClassDeclaration() { - assert.doesNotThrow( function() - { // if not considered abstract, extend() will fail, as it will contain // abstract member foo AbstractClass( 'TestImplExtend' ) .implement( Interface( { foo: [] } ) ) .extend( {} ); - }, Error, 'Class should still be abstract after implement().extend()' ); } )() diff --git a/test/test-class_builder-const.js b/test/test-class_builder-const.js index 6668b44..5279d72 100644 --- a/test/test-class_builder-const.js +++ b/test/test-class_builder-const.js @@ -38,6 +38,7 @@ require( 'common' ).testCase( this.require( '/MemberBuilder' )( MethodWrapperFactory( wrappers.wrapNew ), MethodWrapperFactory( wrappers.wrapOverride ), + MethodWrapperFactory( wrappers.wrapProxy ), this.getMock( 'MemberBuilderValidator' ) ), this.require( '/VisibilityObjectFactoryFactory' ).fromEnvironment() diff --git a/test/test-class_builder-static.js b/test/test-class_builder-static.js index 33d6669..0557af1 100644 --- a/test/test-class_builder-static.js +++ b/test/test-class_builder-static.js @@ -40,6 +40,7 @@ require( 'common' ).testCase( common.require( '/MemberBuilder' )( MethodWrapperFactory( wrappers.wrapNew ), MethodWrapperFactory( wrappers.wrapOverride ), + MethodWrapperFactory( wrappers.wrapProxy ), this.getMock( 'MemberBuilderValidator' ) ), common.require( '/VisibilityObjectFactoryFactory' ).fromEnvironment() diff --git a/test/test-class_builder-visibility.js b/test/test-class_builder-visibility.js index 9c5d559..f2bc98f 100644 --- a/test/test-class_builder-visibility.js +++ b/test/test-class_builder-visibility.js @@ -43,6 +43,7 @@ require( 'common' ).testCase( this.require( '/MemberBuilder' )( MethodWrapperFactory( wrappers.wrapNew ), MethodWrapperFactory( wrappers.wrapOverride ), + MethodWrapperFactory( wrappers.wrapProxy ), this.getMock( 'MemberBuilderValidator' ) ), this.require( '/VisibilityObjectFactoryFactory' ).fromEnvironment() diff --git a/test/test-util-prop-parse.js b/test/test-util-prop-parse.js index b3d3935..6ca81df 100644 --- a/test/test-util-prop-parse.js +++ b/test/test-util-prop-parse.js @@ -46,6 +46,9 @@ var data = { // abstract method abstractMethod: util.createAbstractMethod(), + + // proxy + 'proxy someProxy': 'dest', }; // only add getter/setter if it's supported by our engine @@ -82,6 +85,13 @@ util.propParse( data, { { delete chk_each[ name ]; } + + // TODO: Odd case. Perhaps this doesn't belong here or we can rewrite + // this test. + if ( name === 'someProxy' ) + { + delete chk_each[ 'proxy someProxy' ]; + } }, property: function( name, value ) @@ -209,3 +219,18 @@ assert.equal( }, SyntaxError, 'Valid var names as args should not throw exceptions' ); } )(); + +/** + * Proxies, since their values are strings, would conventionally be considered + * properties. Therefore, we must ensure that the `proxy' keyword is properly + * applied to return a method rather than a property. + */ +( function testProxiesAreConsideredMethodsDespiteTheirStringValues() +{ + assert.equal( + methods.someProxy, + data[ 'proxy someProxy' ], + "Properties with `proxy' keyword should be considered to be methods" + ); +} )(); +