DWR

Regression : DWR method overloading with param type handled by custom converter

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 3.0.RC2
  • Fix Version/s: 3.0.RC4
  • Component/s: Core
  • Documentation Required:
    No
  • Description:
    Hide
    We have a custom converter that takes a JSON encoded string and inflates it into an object "GridConfig". Our service has 2 methods:
    1) getUsers(class GridConfig)
    2) getUsers(class java.lang.String, int, int)

    You can see the first method has 1 parameter, the second has 3 parameters. From JavaScript I am passing 1 parameter that is type "string". This string is the JSON encoded form of the "GridConfig" that the converter is expecting.

    The code in org.directwebremoting.extend.Call.findMethod is causing a regression. The simplest fix is to change line 165... the "Remove non-varargs methods which declare less params than were passed" to remove non-vararg methods which declare less params AND MORE params. Because, lets face it, if I pass in a single parameter method #2 is not going to be getting called!

    Since it doesn't do this. The code will end up removing the method getUsers(class GridConfig) from its list of choices because of the block starting on line 204: "if (allMethods.size() > 1) {"
    This block of code will make the determination that the "string" parameter is not assignable to "GridConfig" (even though there is a perfectly capable converter). Had the code removed method #2, this block would not have triggered, and we would have successfully called method #1 resulting in the behavior DWR used to have.
    Show
    We have a custom converter that takes a JSON encoded string and inflates it into an object "GridConfig". Our service has 2 methods: 1) getUsers(class GridConfig) 2) getUsers(class java.lang.String, int, int) You can see the first method has 1 parameter, the second has 3 parameters. From JavaScript I am passing 1 parameter that is type "string". This string is the JSON encoded form of the "GridConfig" that the converter is expecting. The code in org.directwebremoting.extend.Call.findMethod is causing a regression. The simplest fix is to change line 165... the "Remove non-varargs methods which declare less params than were passed" to remove non-vararg methods which declare less params AND MORE params. Because, lets face it, if I pass in a single parameter method #2 is not going to be getting called! Since it doesn't do this. The code will end up removing the method getUsers(class GridConfig) from its list of choices because of the block starting on line 204: "if (allMethods.size() > 1) {" This block of code will make the determination that the "string" parameter is not assignable to "GridConfig" (even though there is a perfectly capable converter). Had the code removed method #2, this block would not have triggered, and we would have successfully called method #1 resulting in the behavior DWR used to have.

Activity

Hide
David Marginian added a comment - 17/Oct/11 3:22 PM - edited

Dealing with overloaded methods is tricky and has always been difficult. I agree with you about the code on 165 but I need to check with Mike first. It should probably just be looking for a match on the args length if vararg methods aren't involved (remove if exact match not found). I am hesitant to modify it without more thought as it was added by Joe quite awhile back, I am assuming for a good reason.

In the meantime, have you tried using class mappings? If DWR knows what type your Object is than it should find the correct method in LocalUtil.isJavaScriptTypeAssignableTo. Without class mapping DWR knows it has an Object but it doesn't know what type that Object is. Why the complexity? Think about the following overloaded case:

public void overloaded(TypeA a);
public void overloaded(TypeB b);

Try class mapping and let me know.
http://directwebremoting.org/dwr/documentation/server/configuration/dwrxml/converters/bean.html#mappingJavaToJavaScript

Show
David Marginian added a comment - 17/Oct/11 3:22 PM - edited Dealing with overloaded methods is tricky and has always been difficult. I agree with you about the code on 165 but I need to check with Mike first. It should probably just be looking for a match on the args length if vararg methods aren't involved (remove if exact match not found). I am hesitant to modify it without more thought as it was added by Joe quite awhile back, I am assuming for a good reason. In the meantime, have you tried using class mappings? If DWR knows what type your Object is than it should find the correct method in LocalUtil.isJavaScriptTypeAssignableTo. Without class mapping DWR knows it has an Object but it doesn't know what type that Object is. Why the complexity? Think about the following overloaded case: public void overloaded(TypeA a); public void overloaded(TypeB b); Try class mapping and let me know. http://directwebremoting.org/dwr/documentation/server/configuration/dwrxml/converters/bean.html#mappingJavaToJavaScript
Hide
David Marginian added a comment - 17/Oct/11 3:56 PM

The change that is causing the problem was checked in on September 10th, 2008 (SVN rev. 2348).

Here is some related discussion from the list:
http://dwr.2114559.n2.nabble.com/Converter-patch-td5406918.html#none

Show
David Marginian added a comment - 17/Oct/11 3:56 PM The change that is causing the problem was checked in on September 10th, 2008 (SVN rev. 2348). Here is some related discussion from the list: http://dwr.2114559.n2.nabble.com/Converter-patch-td5406918.html#none
Hide
Jason Clawson added a comment - 17/Oct/11 5:12 PM

The problem is that the value coming from DWR (javascript layer) is of type "String" not "Object". So, the call to isJavaScriptTypeAssignableTo will return false even though there is a Converter that will decode the JSON string into a GridConfig object. Generally we don't rely on overloading at all, just number of parameters differentiation. (it wasn't supported previously)

The mapping thing is a good suggestion but unfortunately wont work in this case since my value is a string. It just needs to be decoded by my converter.

It seems obvious that if I provide 1 parameter in my call, and have a choice of several methods of which only one has a single param-- it should try and call that one regardless of any type checking.

By the way, we just upgraded from DWR RC2.

Show
Jason Clawson added a comment - 17/Oct/11 5:12 PM The problem is that the value coming from DWR (javascript layer) is of type "String" not "Object". So, the call to isJavaScriptTypeAssignableTo will return false even though there is a Converter that will decode the JSON string into a GridConfig object. Generally we don't rely on overloading at all, just number of parameters differentiation. (it wasn't supported previously) The mapping thing is a good suggestion but unfortunately wont work in this case since my value is a string. It just needs to be decoded by my converter. It seems obvious that if I provide 1 parameter in my call, and have a choice of several methods of which only one has a single param-- it should try and call that one regardless of any type checking. By the way, we just upgraded from DWR RC2.
Hide
David Marginian added a comment - 17/Oct/11 5:29 PM

It does seem obvious, but I just want to make sure as the code was added for a reason (mistake, some type of weird corner case we aren't thinking of).

RC3 isn't out yet. Are you using a dev build or did you mean you upgraded from RC1 to RC2? Are you saying this worked in RC1/RC2? The code that is causing you grief has existed since 9/2008, so this doesn't make much sense to me.

Show
David Marginian added a comment - 17/Oct/11 5:29 PM It does seem obvious, but I just want to make sure as the code was added for a reason (mistake, some type of weird corner case we aren't thinking of). RC3 isn't out yet. Are you using a dev build or did you mean you upgraded from RC1 to RC2? Are you saying this worked in RC1/RC2? The code that is causing you grief has existed since 9/2008, so this doesn't make much sense to me.
Hide
Jason Clawson added a comment - 17/Oct/11 6:23 PM

We are using RC3 now. We have configured our bamboo server to package up jars and add it to our nexus based on your svn. We had trouble previously in finding DWR in any public maven location. We are looking to take advantage of some of the new features and bugfixes.

2008 seems pretty old. The previous version is labeled as RC2 in our nexus but this was uploaded by someone by hand. It could be mislabled. The source jars weren't uploaded either :-/. I will decompile the class tomorrow and see what it looks like. Perhaps this will shed some more light.

All I know is that service methods that used to be called are no longer called because it can't find the method after the DWR upgrade. This is the one thing I am sure about.

Show
Jason Clawson added a comment - 17/Oct/11 6:23 PM We are using RC3 now. We have configured our bamboo server to package up jars and add it to our nexus based on your svn. We had trouble previously in finding DWR in any public maven location. We are looking to take advantage of some of the new features and bugfixes. 2008 seems pretty old. The previous version is labeled as RC2 in our nexus but this was uploaded by someone by hand. It could be mislabled. The source jars weren't uploaded either :-/. I will decompile the class tomorrow and see what it looks like. Perhaps this will shed some more light. All I know is that service methods that used to be called are no longer called because it can't find the method after the DWR upgrade. This is the one thing I am sure about.
Hide
David Marginian added a comment - 17/Oct/11 7:25 PM
Show
David Marginian added a comment - 17/Oct/11 7:25 PM https://oss.sonatype.org
Hide
Mike Wilson added a comment - 19/Oct/11 1:09 AM

The reason for these problems is a chicken-and-egg problem wrt to finding the right converter for incoming data. DWR only uses type information from the method signatures to know what converter to use on the data (unless class-mapping is used). As converters don't register what type they expect as input, we have the following assumptions in the code to allow method overloading without resorting to having to try running several converters for a single piece of incoming data:

  • JS basic types (numbers, string etc) hardwired to corresponding type in Java
  • JS object types may have custom converters subclassed from BasicObjectConverter

This is what you see in lines 204-220 ("Added to increase our ability to call overloaded methods accurately"), checked in 2010 by David, where we call isJavaScriptTypeAssignableTo() to allow us to use overloaded methods with the same number of parameters but different basic JS types, f ex:

js:myMethod("") -> java:myMethod(String)
  js:myMethod({}) -> java:myMethod(MyClass)

This is the part which is conflicting with your code that relies on the DWR2 behaviour.

You are also mentioning line 164-169 (Remove non-varargs methods which declare less params than were passed), checked in 2008 by Joe. The reason more parameters are allowed, is to offer JavaScript semantics on unspecified parameters with nullable types, f ex:

js:myMethod("") -> java:myMethod(String, String /*null*/, String /*null*/)

This is involved more by fluke in your case, and would not be significant in a somewhat different setup.

To sum up, my view is that we should support your use case where one can register a converter to deal with any JS type. Unfortunately, this conflicts with type-based method overloading which is also something that has been requested by other DWR users. There are two fixes:

  • refactor converter registration to allow specifying input type
  • add a DWR configuration parameter ("strictMethodOverloading=false") that provides possibility to use DWR2-like behaviour (disabling the isJavaScriptTypeAssignableTo() check)

My opinion is that it is too late for refactoring converters in DWR3, and it should be done in the next release instead. This leaves us with the configuration parameter solution.
What are your thoughts?

Show
Mike Wilson added a comment - 19/Oct/11 1:09 AM The reason for these problems is a chicken-and-egg problem wrt to finding the right converter for incoming data. DWR only uses type information from the method signatures to know what converter to use on the data (unless class-mapping is used). As converters don't register what type they expect as input, we have the following assumptions in the code to allow method overloading without resorting to having to try running several converters for a single piece of incoming data:
  • JS basic types (numbers, string etc) hardwired to corresponding type in Java
  • JS object types may have custom converters subclassed from BasicObjectConverter
This is what you see in lines 204-220 ("Added to increase our ability to call overloaded methods accurately"), checked in 2010 by David, where we call isJavaScriptTypeAssignableTo() to allow us to use overloaded methods with the same number of parameters but different basic JS types, f ex:
js:myMethod("") -> java:myMethod(String)
  js:myMethod({}) -> java:myMethod(MyClass)
This is the part which is conflicting with your code that relies on the DWR2 behaviour. You are also mentioning line 164-169 (Remove non-varargs methods which declare less params than were passed), checked in 2008 by Joe. The reason more parameters are allowed, is to offer JavaScript semantics on unspecified parameters with nullable types, f ex:
js:myMethod("") -> java:myMethod(String, String /*null*/, String /*null*/)
This is involved more by fluke in your case, and would not be significant in a somewhat different setup. To sum up, my view is that we should support your use case where one can register a converter to deal with any JS type. Unfortunately, this conflicts with type-based method overloading which is also something that has been requested by other DWR users. There are two fixes:
  • refactor converter registration to allow specifying input type
  • add a DWR configuration parameter ("strictMethodOverloading=false") that provides possibility to use DWR2-like behaviour (disabling the isJavaScriptTypeAssignableTo() check)
My opinion is that it is too late for refactoring converters in DWR3, and it should be done in the next release instead. This leaves us with the configuration parameter solution. What are your thoughts?
Hide
David Marginian added a comment - 19/Oct/11 4:27 AM

"You are also mentioning line 164-169 (Remove non-varargs methods which declare less params than were passed), checked in 2008 by Joe. The reason more parameters are allowed, is to offer JavaScript semantics on unspecified parameters with nullable types, f ex:"
Thanks for the explanation Mike.

Show
David Marginian added a comment - 19/Oct/11 4:27 AM "You are also mentioning line 164-169 (Remove non-varargs methods which declare less params than were passed), checked in 2008 by Joe. The reason more parameters are allowed, is to offer JavaScript semantics on unspecified parameters with nullable types, f ex:" Thanks for the explanation Mike.
Hide
Jason Clawson added a comment - 19/Oct/11 2:25 PM

I understand the nullable parameters. It makes sense.

I think your suggested fix is perfect. The configuration parameter would be fine for now.

Just a few thoughts on the Converter refactor:
It would be nice if a converter could accept multiple types. For instance, maybe our converter would like to accept a string OR an object.

We override the DefaultConverterManager with our own to provide converter priorities, allow for annotation based converter bindings, and guice multi-binder semantics for converter registration. I assume the code that will check accepted input type will live in the ConverterManager? I just want to be sure that we can programatically set this. Our internal application framework has a standard "convention over configuration" architecture so its nice to be able to step in and set everything up programatically. You guys have done a really great job of making that easy.

Show
Jason Clawson added a comment - 19/Oct/11 2:25 PM I understand the nullable parameters. It makes sense. I think your suggested fix is perfect. The configuration parameter would be fine for now. Just a few thoughts on the Converter refactor: It would be nice if a converter could accept multiple types. For instance, maybe our converter would like to accept a string OR an object. We override the DefaultConverterManager with our own to provide converter priorities, allow for annotation based converter bindings, and guice multi-binder semantics for converter registration. I assume the code that will check accepted input type will live in the ConverterManager? I just want to be sure that we can programatically set this. Our internal application framework has a standard "convention over configuration" architecture so its nice to be able to step in and set everything up programatically. You guys have done a really great job of making that easy.
Hide
Mike Wilson added a comment - 05/Dec/11 11:28 AM

Sorry about the delay. I just need to verify a few things and set up a few test cases before fixing this.

Show
Mike Wilson added a comment - 05/Dec/11 11:28 AM Sorry about the delay. I just need to verify a few things and set up a few test cases before fixing this.

People

Dates

  • Created:
    17/Oct/11 2:06 PM
    Updated:
    29/Nov/12 3:54 PM