#363 Wrong overloaded method selected when null value is passed

[Anonymous]

Steps to recreate:

1) add an instance of the following class to the context of the Freemarker templated defined at #2:

public class TestClass {
public String testMethod(Map<String, ? extends Object> fields, List listField) {
return "Executed: testMethod(Map<String, ? extends Object> fields, List listField) on input: fields=" + fields + " and listField=" + listField;
}
public String testMethod(Object ... fields) {
return "Executed: testMethod(Object ... fields) on input: fields=" + fields;
}
}

2) run the following Freemarker template:

<#assign aMap = {"name":"Billy", "lastName", "Pilgrim"} />
<#assign testResult = testClass.testMethod(theMap, [])>
TEST RESULT: ${testResult}
<#assign testResult = testClass.testMethod(theMap, null)>
TEST RESULT: ${testResult}

3) the output is:

TEST RESULT: Executed: testMethod(Map fields, List listField) on input: fields={name=Billy, lastName=Pilgrim} and listField=[]
TEST RESULT: Executed: testMethod(Object ... fields) on input: fields=[Ljava.lang.Object;@566b24a

4) I believe that the output of the second line is wrong because the expected behavior is that when a Map and a null (for the List) is passed then the method executed should be:
testMethod(Map<String, ? extends Object> fields, List listField)

[Adam Heath]

I'm the ofbiz developer who originally discovered this bug. I've got a patch against the 2.3 branch on sf that seems to fix it. The test cases run both before and after, with no changes in output.

[Adam Heath]

I don't see how to attach a file to an issue that I did not create; I have a stand-alone single java file that doesn't work, then I have a patch that fixes it, and then my java file does work. But I can't upload them.

[Anonymous]

File created by Adam Heat that can be used to recreate the issue

[Anonymous]

and this is a change that seems to fix the issue

[Dániel Dékány]

The problem with your patch is that this can't be backward compatible. Unless... varargs support was rewritten in 2.3.19 (before that it was utterly broken). I will have to check what happened in this situation in 2.3.18. If we are lucky, it was 2.3.19 where the priorities broke, and so it can be fixed without violating the version policy.

[Adam Heath]

It's broken with 2.3.18 as well. I've got a git svn clone of the old repo.

I've expanded my tests, and freemarker is really doing the wrong specificity testing on arg types.

template="${obj.method(string, null)}"

String method(String s, List l) {}
String method(String s, Map m) {}
String method(Object... args) {}
==

In this case, freemarker *should* throw an error, multiple methods that metch. With 2.3.18, it picks the varargs method. With my patch, it fixes the original problem, and now also throws a correct exception for the issue.

Additionally, if you have 2 methods, one that takes an 'int', and another that takes a 'float', then in freemarker you have a '0' or '0.0' numeric literal, it picks the wrong method too; in my case, it always picks the 'int' variant, in both 2.3.18 and 2.3.19. My patch doesn't fix this.

In 2.3.18 and .19, if I have a method that takes Object[], but *not* Object..., then freemarker can't find any matching methods. With my patch against .19, it finds the 2 methods(List and Map variants). My fix seems to have no bearing whatsoever on the varargs change in .19. It seems to just be a normal problem with method resolution.

If none of this makes sense, then I apologize. It's very hard to deal with this issue tracker, I can't actually upload any files, so can't show test cases. I might end up having to do this thru an ofbiz bug filed in jira, just so that I can upload files.

I also understand the issue with backwards compatibility. And even if these problems *are* fixed in freemarker, and ofbiz's uses of it is correct, we might have downstream users(of ofbiz) that require the old behavior. Maybe there should be some configuration parameter that can be configured per template, or a <@config> type thing that is specified at the top, that allows for configuring how the template handles method resolution.

[Dániel Dékány]

I think that for method(Object[] args) it shouldn't find any match for two arguments, so the behavior is correct.

Regarding 0 VS 0.0, the bigger problem is that even for 0.5 it prefers the method for int argument over the float argument. Worse, if I have *only* m(double x) in Java, and issue m(0.5) in the template, the method gets dobule 0 (like if it was first converted to int, then back to double), unless I write m(0.5?double). The method also gets 0 for m(0.5?float), instead of doing a float to double casting. The only time it works as expected is when the m is like m(BigDecimal x), since 0.5 in FreeMarker is by default stored as BigDecimal. It's puzzling why it wasn't reported for a decade... seems people don't use decimal literals in templates. It's also something that can't just be an overlook... I will ask the original author about this things.

As of the backward compatibility issue, such fixes should be added as new BeansWrapper settings. The defaults of those setting should be the backward-compatible ones, and then maybe over time, with 2nd version number increases, the defaults can change, then even more later the old default value of the settings can become unsupported. To work on this, you had to sign a CLA however (I will email it to you if you want to sign). Then you can even commit into the SVN directly.

[Adam Heath]

I personally have no problem with a CLA, and I'm certain work wouldn't either. I've already signed one for Apache, when I work on OfBiz. I've been involved with that project since 2004.

Yeah, I'm rather surprised that there are all these issues with method resolution. I've seen such issues occur with janino(diskless java compiler). I have *not* seen them with asm. I've thought that ideally there should be a library that tools others can then use. I'm surprised that there isn't one built-in to java. Looking at the javadoc for Class, the method lookup routines don't defined what happens when a class parameter type is null, nor *exactly* what happens when conversions need to occur.

[Adam Heath]

I've got a better patch now, that makes it configurable. Since I can't attach files, I'm having Jacopo do it for me.

[Adam Heath]

http://ofbiz.135035.n4.nabble.com/attachment/4631983/0/freemarker-nullArgs-fix.patch

[Anonymous]

Improved fix, that makes the change configurable. By Adam Heath

[Anonymous]

Last patch from Adam is now attached

[Dániel Dékány]

OK, I have to you (Adam) the CLA.

As of allowing attaching files to this tracker, I don't see any solutions for it here on the admin interface. So just email them to me, and I will attach them.

[Dániel Dékány]

(I meant, I have mailed the CLA to you...)

[Jacques Le Roux]

Hi,

Just curious, what is the situation here?

Thanks

Jacques (Apache OFBiz committer)

[Dániel Dékány]

It's on the TODO list for 2.3.20, so it won't be forgotten. But I don't know when 2.3.20 will come. As far as the patch implements this option correctly, you most certainly won't need to re-patch after the next release.

[Jacques Le Roux]

Thanks, it's quite clear

[Dániel Dékány]

I'm looking into the overloaded method related parts (I don't mean the patch) and... there are other issues as well. Certainly I will have to *try* redesign/rewrite this stuff pretty much (it's tricky because of FM object wrapping), and of course, I will have to add switch where you can chose between old and the new implementation, globally or per template. Now this 100% won't be finished in two days, so I guess 2.3.20 will be release without this, but then I continue working on this, and release 2.3.21 when this and some other smaller things are done...

[Dániel Dékány]

OK, this particular issue with null-s is fixed for a while in the Git repo. Now I have also fixed the issue with numbers (unnecessary truncation). Many many other anomalies were fixed too. While I don't see much chance of applications breaking due to these fixes, these fixes/reworkings are not active by default for backward compatibility. To activate all these improvements, you need to create the BeansWrapper (or the DefaultObjectWrapper) like new BeansWrapper(new Version(2, 3, 21)).

This will be released with 2.3.21, which I hope will make it in a few weeks. Until that, it would be good if you can test them. For that, check out the 2.3-gae branch from https://github.com/freemarker/freemarker/, and build it with ant (should just work, if you have Ivy installed on Ant, but see "Building" in the README.txt).

[Dániel Dékány]

Guys (from OfBiz), do you intend to test this? See previous comment. Do you need any help for that? (After ant manual you can check the details of the changes in the Version History chapter.) 2.3.21 is due in this summer, and it can be quite awkward to adjust anything after that (because, backward compatibility).

[Jacques Le Roux]

We need to check this. We still use our own freemarker-2.3.19-null-wildcards.jar

Thanks Dániel!

[Jacopo Cappellato]

Daniel,

thanks for your work on this.
I have downloaded the 2.3.21 ga from git; then I have created an instance with:

BeansWrapper theWrapper = new BeansWrapper(Version(2, 3, 21));

Then I have executed the test (see steps #1 and #2) as described in the summary of this ticket but unfortunately the test is still failing: I am getting the same output as described in the ticket.

PS: I have noticed that there is a typo in the step #2 of the test; the name of the variable "aMap" should be "theMap" instead.
Please replace:
<#assign aMap = {"name":"Billy", "lastName", "Pilgrim"} />
with
<#assign theMap = {"name":"Billy", "lastName", "Pilgrim"} />

in order to run the test properly.

We are very interested in seeing this issue resolved so please let me know if there is something I can do to help.

Thanks

Jacopo