PDA

View Full Version : DirectJNgine + other Ext.Direct routers: do we need call validation at the client?



pagullo
21 Jul 2009, 5:21 AM
We are in the process of polishing DirectJNgine (http://code.google.com/p/directjngine/) 1.0 final, a Java-based Ext Direct router, and I would like to get feedback on some issues with remote calls.

I think the issues I'm going to explore are relevant for other if not all router implementations, be them for PHP, .NET, Java..., and that's why I'm opening a new thread.

Problems with remote calls -should we implement remote call checks at the client?

Let's take a look at the following Java code,


double sum( double d1, double d2) {
return d1 + d2;
}
If you are PHP or .NET guy, think of you language's version of a two-args function.

Case 1:

Now, let's call our example method with the following javascript code:


MyAction.sum(3, undefined, 5)This returns 8, even though the method is called with three parameters, but can handle only two. From my point of view, the client should not even be allowed to call the server.

As far as I know, this happens because ExtJs skips undefineds when serializing to json. Anyway, the request just sends 5 and 3 as parameters, and therefore what the server sees is the equivalent of the following client call:


MyAction.sum(3,5)Therefore, the server can't handle the case because it can't know what happened. Feels like checking must be done at the client.

This will be an issue for all direct routers.

(BTW, passing a function instead of undefined is problematic too)

Case 2:

Take a look at the following javascript call:


MyAction.sum( 3, 5, 7522 )While the router can choose whether to ignore the "additional" parameters, I feel like the client code should flag an error, because Ext Direct has a 'len' argument, which I think it should honour.

We might think about this as a 'feature' that allows us to pass as many arguments as you want. But I think you can get the functionality just by passing as the las parameter an array, they work nicely in DirectJNgine.

BTW, this javascript code has a similar problem, just that now we are calling the method with not enough parameters:


MyAction.sum( 3 )Case 3:
This is case 1 on steroids.

When you pass an undefined as an argument, the check is trivial. However, if you pass as an argument an object with a field that is explicitly set to undefined, or an array that has an undefined value, we have the same problem as in case 1: Ext Direct will skip undefineds.

When you pass an argument that contains and object which has an array whose second value is an object which has a field explictily set to undefined...well, you don't want to debug that!


...

Now, I would *love* it if the client checked all these issues, even if only while in debug mode, to avoid checking overhead.

And I would love it too if somebody clarified what the 'len' in an action method really stands/should stand for.

Maybe some Ext Direct guy could comment/clarify/fix these issues?

...

P.S: a proposal
I have implemented Djn.RemoteCallSupport for DirectJNgine, a call checker that you install and checks for all those issues, checking parameters and their referenced objects and arrays recursively, to check all these situations.

First you install a call checker that handles the 'beforecall' event, as follows:



var remotingProvider = Ext.Direct.addProvider( Djn.test.REMOTING_API);
Djn.RemoteCallSupport.addCallValidation(remotingProvider);
Djn.RemoteCallSupport.validateCalls = true;
The validation code installed by addCallValidation is as follows:


Djn = Ext.namespace( "Djn");

Djn.RemoteCallSupport = {
DIRECT_CALL_ERROR_PREFIX : 'Direct call error',
validateCalls: false,
addCallValidation : function( provider ) {
/****************************************************************************
*
* Checking method parameters is very complicated.
*
* We rely on 'transaction.args' containing what the server will receive,
* minus callback functions at the end.
* Besides, we assume that, once we have found a funtion, this is
* not part of the arguments to send to the server -which, on the
* other hand, can't possibly handle a javascript function!
*
****************************************************************************/

provider.on( "beforecall", function(provider, transaction) {
if( !Djn.RemoteCallSupport.validateCalls ) {
return true;
}
////////////////////
var arguments = transaction.args;
var fullMethodName = transaction.action + "." + transaction.method;

// Do not check form methods, they receive pairs paramName-value, which need-not/cannot be checked
var isFormMethod = transaction.isForm !== undefined;
if( isFormMethod ) {
return true;
}

/////////////////// Utilities
function typeOf(value){
var s = typeof value;
if (s === 'object') {
if (value) {
if (typeof value.length === 'number' &&
!(value.propertyIsEnumerable('length')) &&
typeof value.splice === 'function') {
s = 'array';
}
}
else {
s = 'null';
}
}
return s;
}

function isFunction(v) {
return typeOf(v) === 'function';
}

function isArray( v ) {
return typeOf(v) === 'array';
}

function isObject( v ) {
return typeOf(v) === 'object';
}

function arrayHasUndefinedValue( value ) {
// Ejn.Assert.isTrue( isArray(value));

for( i = 0; i < value.length; i++ ) {
var item = value[i];
if( item === undefined ) {
return true;
}
else if( isObject(item)) {
return objectHasArrayWithUndefinedValue(item);
}
else if( isArray(item)) {
return arrayHasUndefinedValue(item);
}
}
return false;
}

function objectHasArrayWithUndefinedValue(value) {
// Ejn.Assert.isTrue( isObject(value));

for( memberName in value ) {
var memberValue = value[memberName];
if( isArray(memberValue)) {
return arrayHasUndefinedValue(memberValue)
}
else if( isObject(memberValue)) {
return objectHasArrayWithUndefinedValue(memberValue);
}
}
return false;
}

function arrayToString( a, count ) {
var result = "[";
for( i = 0; i < count; i++ ) {
if( a[i] === undefined ) {
result += 'undefined';
}
else if( a[i] === null ) {
result += "null";
}
else if ( typeof(a[i]) === 'string') {
result += "'" + a[i] + "'";
}
else {
result += a[i].toString();
}
var isLastItem = i === count -1;
if( !isLastItem ) {
result += ",";
}
}
result += "]";
return result;
};

/////////////////// Check parameters count
function getArgumentCountUpToFirstFunction( array ) {
var result = 0;
var i = 0;
for(; i < array.length; i++ ) {
if( typeOf( array[i]) === 'function') {
return i;
}
}
return i;
}

function getExpectedMethod( provider, transaction ) {
var action = provider.actions[transaction.action];

for( i = 0; i < action.length; i++ ) {
var methodName = transaction.method;
var method = action[i];
if( method.name === methodName ) {
return method;
}
}

// Ejn.Assert.isTrue(false, "We should have found the action and method specified in the transaction, '" + fullMethodName + "'");
return;
}

var argsArgumentsCount = getArgumentCountUpToFirstFunction( transaction.args );
var expectedArgumentsCount = getExpectedMethod( provider, transaction ).len;
if( argsArgumentsCount != expectedArgumentsCount ) {
var error = Djn.RemoteCallSupport.DIRECT_CALL_ERROR_PREFIX + ", '" + fullMethodName + "' received '" + argsArgumentsCount + "' parameters, but the call requires '" + expectedArgumentsCount + "' parameters.";
throw error;
// return false
}

/////////////////////// Check parameters do not have invalid data
if( arguments === null || arguments.length === 0) {
// No need to check parameter validity!
return
}

function callHasIllegalDataInParameters( parameters, parameterCount ) {
// Ejn.Assert.isTrue( isArray(parameters));

for( i = 0; i < parameterCount; i++ ) {
var value = parameters[i];
if( value === undefined) {
return true;
}
else {
if( isFunction(value)) {
return true;
}
else if( isArray(value)) {
return arrayHasUndefinedValue(value);
}
else if( isObject(value) ) {
return objectHasArrayWithUndefinedValue(value);
}
}
}
return false;
}

if( callHasIllegalDataInParameters(arguments, expectedArgumentsCount ) ) {
var error = Djn.RemoteCallSupport.DIRECT_CALL_ERROR_PREFIX + ", '" + fullMethodName + "' must not receive undefined parameters. Call parameters: " + arrayToString( arguments, expectedArgumentsCount);
// Ext.log( "ERROR: " + error );
throw error;
// return false
}

}
);
return true;
}
}
So far, this code passes a dozen tests -but I have to confess that I'm fairly new to javascript, so it looks a bit "javaish".

That said, if it is already not part of DirectJNgine is because there are several cons:

a) We check arguments recursively, and this can be slow. What's worse, if we have object a pointing to b, and b to a, we get infinite recursion -maybe somebody else can enhance the code to solve this "infinite recursion" issue?

b) I have the feeling that the code is fragile: if ExtJs guys change some little detail, all DJN users might end up with broken code if they are using Djn.RemoteCallSupport, without them knowing until something happens at runtime.

c) I am not completely sure about the 'len' semantics for a Direct method. Is it a mandatory size? Is it a minimum size? What to do depens on this, too. Don't want to make an unchecked assumption be part of a final release...

I would like to know ExtJs guys opinion before committing to add this to DirectJNgine.

Thanks,

Pedro