Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception selecting overloaded method in 3.1.4 #23

Closed
danielfernandez opened this issue May 6, 2016 · 12 comments
Closed

Exception selecting overloaded method in 3.1.4 #23

danielfernandez opened this issue May 6, 2016 · 12 comments

Comments

@danielfernandez
Copy link
Contributor

In Thymeleaf there is an Aggregates class which contains an overloaded avg(...) method. See the specific class here.

All the different variants of this method look like this:

    public BigDecimal avg(final Iterable<? extends Number> target) {...}
    public BigDecimal avg(final Number[] target) {...}
    public BigDecimal avg(final byte[] target) {...}
    public BigDecimal avg(final short[] target) {...}
    public BigDecimal avg(final int[] target) {...}
    public BigDecimal avg(final long[] target) {...}    
    public BigDecimal avg(final float[] target) {...}
    public BigDecimal avg(final double[] target) {...}

In one of my tests, I create an object using an OGNL expression, like:

nums01 = { 5, 5 }

Which creates a java.util.ArrayList<java.lang.Integer> for the nums01 variable.

Then I use this variable as an argument to an avg call in an OGNL expression such as:

#aggregates.avg(nums01)

This worked perfectly until OGNL 3.1.3, and the first version of the overloaded method (the one receiving an Iterable<? extends Number>) was executed. However, in OGNL 3.1.4 I receive this exception:

java.lang.IllegalArgumentException: Can't decide wich method to use: "public java.math.BigDecimal org.thymeleaf.expression.Aggregates.avg(float[])" or "public java.math.BigDecimal org.thymeleaf.expression.Aggregates.avg(long[])"
    at ognl.OgnlRuntime.findBestMethod(OgnlRuntime.java:1427)
    at ognl.OgnlRuntime.getAppropriateMethod(OgnlRuntime.java:1283)
    at ognl.OgnlRuntime.callAppropriateMethod(OgnlRuntime.java:1452)
    at ognl.ObjectMethodAccessor.callMethod(ObjectMethodAccessor.java:68)
    at ognl.OgnlRuntime.callMethod(OgnlRuntime.java:1598)
    at ognl.ASTMethod.getValueBody(ASTMethod.java:91)
    at ognl.SimpleNode.evaluateGetValueBody(SimpleNode.java:212)
    at ognl.SimpleNode.getValue(SimpleNode.java:258)
    at ognl.ASTChain.getValueBody(ASTChain.java:141)
    at ognl.SimpleNode.evaluateGetValueBody(SimpleNode.java:212)
    at ognl.SimpleNode.getValue(SimpleNode.java:258)
    at ognl.Ognl.getValue(Ognl.java:494)
    at ognl.Ognl.getValue(Ognl.java:458)
@lukaszlenart
Copy link
Collaborator

It can be only related to #19

@marvkis
Copy link
Contributor

marvkis commented May 17, 2016

Hi.

#19 was originally made by me. I'll take a look into that issue...

Sorry for the late reply it seems somehow my github email notification's dont work and I just disovered this by chance...

@lukaszlenart
Copy link
Collaborator

@marvkis great, thanks a lot!

@marvkis
Copy link
Contributor

marvkis commented May 17, 2016

Okay, I have an idea what's going wrong:
Initially in AstMethod.getChildrenClasses() implementation for ASTList I already had a parametrized array as returning type that hold's the information about the appropriate carried class types. But as this didn't reflect ASTList's real behavior - returning an ArrayList - this wasn't the right way so I changed it to return a list. List itself is a generic type, but as far as I know a 'Class' can't be parametrized easily so the 'inner' type information got lost and so the method weighting can't make a clean decision...

Have to dig into that... but it must work somehow, when refactoring into a class the returning types are parametrized...

marvin-enthus added a commit to secadm/ognl that referenced this issue May 17, 2016
marvin-enthus added a commit to secadm/ognl that referenced this issue May 17, 2016
marvin-enthus added a commit to secadm/ognl that referenced this issue May 17, 2016
….1.4

- Improved method matching: Lists default to List<Object> because of "Java Generics Type Erasure"
- Don't bail out (with exception) immedeately if we get two methods with same score - a method with a
  better score might follow. Just remember the Exception and if we really don't get an better option
  throw the exception
marvin-enthus added a commit to secadm/ognl that referenced this issue May 17, 2016
marvin-enthus added a commit to secadm/ognl that referenced this issue May 17, 2016
….1.4

- Improved method matching: Lists default to List<Object> because of "Java Generics Type Erasure"
- Don't bail out (with exception) immedeately if we get two methods with same score - a method with a
  better score might follow. Just remember the Exception and if we really don't get an better option
  throw the exception
marvin-enthus pushed a commit to secadm/ognl that referenced this issue May 17, 2016
marvin-enthus pushed a commit to secadm/ognl that referenced this issue May 17, 2016
@marvin-enthus marvin-enthus mentioned this issue May 17, 2016
@marvkis
Copy link
Contributor

marvkis commented May 17, 2016

After some investigation:

The "Generics" approach is the wrong way. I started an approach to use a "typed class carrier" but it was the wrong way. It worked quite well for "compiled" OGNL commands, but it failed during interpretation - Interpretation is done on live objects and not on classes, and due to "Generics Type Erasure" generics type information is only available during compilation and not any more during run time. So i abandoned this approach - it's still here for whom it might interest: Issue-23-Generics

So, as always if you ran into the wrong direction, change your perspective: What does java itself when it has to call avg() with a java.util.List argument? And indeed, it chooses the avg(Iterable...) method.

And it didn't work because the OgnlRuntime.isTypeCompatible() bailed out with an exception to early instead of continuing if there is a better matching method.
I also had to change the method weighting and matching a bit with the 'Generics Type Erasure' in mind: A java.util.List only will match to an Object[] array during runtime. With this in mind all Unit-Tests worked again.

Btw: sa-ChristianNiessner is my 'work' account and marvkis my personal / private account. I did this during work hours so the credit is for my company ;)

@lukaszlenart
Copy link
Collaborator

@marvkis aka sa-ChristianNiessner please provide your company name and I will used in Version Notes :)

@marvin-enthus
Copy link
Contributor

@lukaszlenart it is 'secadm GmbH' - http://www.secadm.de/ . thanks a lot.

@danielfernandez Was this discovered in an unit test of thymeleaf or during runtime? Can you do further 'real life' tests?

@lukaszlenart can you test it within struts to see if the changes perform fine or bring up another issues?

thanks!

@danielfernandez
Copy link
Contributor Author

Do we already have a snapshot for this? if so, what's OGNL's Maven snapshots repository?

@lukaszlenart
Copy link
Collaborator

@danielfernandez
Copy link
Contributor Author

I can confirm all Thymeleaf tests pass with 3.1.5-SNAPSHOT, including the dozen tests that fail with 3.1.4

@lukaszlenart
Copy link
Collaborator

Fixed with #24

@lukaszlenart
Copy link
Collaborator

3.1.5 is under way to the Central, thanks @sa-ChristianNiessner and @danielfernandez !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants