Velocity
  1. Velocity
  2. VELOCITY-132

IllegalArgumentException while calling an overloaded method

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Invalid
    • Affects Version/s: 1.3-rc1
    • Fix Version/s: 1.5
    • Component/s: Engine
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      If there are two methods with the same name and different parameters like:
      public class myClass
      {
      public String foo ( Integer intObj );
      public String foo ( String str );
      }

      and a velocity template like this

      $myObj.foo( $someObj.getNull() )
      $myObj.foo( $str )

      while $someObj.getNull() returns null and $str is a java.lang.String object
      with a String like 'test'. Because velocity caches the first method with the
      Integer argument on calling with the parameter null (which isn't of course of
      any type/class) the call fails with the String argument, because velocity tries
      to call the foo( Integer intObj )!

      In the velocity log appears a IllegalArgumentException which is right, but
      velocity should call the right method!

      mike

      1. VelocityCachingBug.java
        0.5 kB
        mike
      2. velocitybug.vm
        0.1 kB
        mike
      3. velocity_patch.patch
        11 kB
        Llewellyn Falco
      4. includecall.vm
        0.0 kB
        mike

        Issue Links

          Activity

          Hide
          Henning Schmiedehausen added a comment -

          Close all resolved issues for Engine 1.5 release.

          Show
          Henning Schmiedehausen added a comment - Close all resolved issues for Engine 1.5 release.
          Hide
          Will Glass-Husain added a comment -

          Hi, thanks for reporting this and commenting.

          I'm marketing this issue as invalid. There's two different issues here-- one has been previously fixed and one is "as desired".

          The first issue concerns the IllegalArgumentException when the provided example is included with #parse. I was unable to reproduce this. I've added unit tests to verify this is working properly. I believe it was fixed between 1.4 and 1.5. Mike comments above that in the nightly source his example returns:

          $testobj.foo( $param )
          String

          this is the correct response. Calling $testobj.foo(null) could return either of two possible method calls. Since it's impossible to tell which to call, Velocity returns neither. (there's an error message in the log noting the ambiguity).

          The second issue is when Llewellyn argues above that this is incorrect behavior, that if a method call is ambiguous, Velocity should pick one. I disagree. It's hard to know which one to pick. There's a clear message in the log listing the method call and stating it's ambiguous. Thus it's the developer's responsibility to provide a context object with methods that are not ambiguous.

          Resolved - marking as invalid, but adding unit tests to confirm. Thanks again.

          Show
          Will Glass-Husain added a comment - Hi, thanks for reporting this and commenting. I'm marketing this issue as invalid. There's two different issues here-- one has been previously fixed and one is "as desired". The first issue concerns the IllegalArgumentException when the provided example is included with #parse. I was unable to reproduce this. I've added unit tests to verify this is working properly. I believe it was fixed between 1.4 and 1.5. Mike comments above that in the nightly source his example returns: $testobj.foo( $param ) String this is the correct response. Calling $testobj.foo(null) could return either of two possible method calls. Since it's impossible to tell which to call, Velocity returns neither. (there's an error message in the log noting the ambiguity). The second issue is when Llewellyn argues above that this is incorrect behavior, that if a method call is ambiguous, Velocity should pick one. I disagree. It's hard to know which one to pick. There's a clear message in the log listing the method call and stating it's ambiguous. Thus it's the developer's responsibility to provide a context object with methods that are not ambiguous. Resolved - marking as invalid, but adding unit tests to confirm. Thanks again.
          Hide
          Nathan Bubna added a comment -

          Llewellyn, were you able to update your patch to fully fix this?

          Show
          Nathan Bubna added a comment - Llewellyn, were you able to update your patch to fully fix this?
          Hide
          Will Glass-Husain added a comment -

          Ok, thanks. Llewellyn - want to take a crack at updating your patch? If the test passes and all the other tests pass, I'll commit.

          Show
          Will Glass-Husain added a comment - Ok, thanks. Llewellyn - want to take a crack at updating your patch? If the test passes and all the other tests pass, I'll commit.
          Hide
          mike added a comment -

          With this file the bug can be reproduced.
          The correct output should be

          Integer
          String

          Show
          mike added a comment - With this file the bug can be reproduced. The correct output should be Integer String
          Hide
          mike added a comment -

          I just downloaded the actual nightly src and tested the bug with the files which I included above. The fix dowsn't seem to solve the issue: In the old version (1.4) the output of the test is

          Integer
          $testobj.foo( $param )

          in the patched new version it is
          $testobj.foo( $param )
          String

          which isn't correct either. The correct output should be
          Integer
          String

          I don't think this is a minor bug so it should be fixed in 1.5 if in any case possible. If you need further information about the bug please contact me.

          Show
          mike added a comment - I just downloaded the actual nightly src and tested the bug with the files which I included above. The fix dowsn't seem to solve the issue: In the old version (1.4) the output of the test is Integer $testobj.foo( $param ) in the patched new version it is $testobj.foo( $param ) String which isn't correct either. The correct output should be Integer String I don't think this is a minor bug so it should be fixed in 1.5 if in any case possible. If you need further information about the bug please contact me.
          Hide
          Llewellyn Falco added a comment -

          This resolves the issue at hand by selecting the first method it the case that either 2 will do equally well.
          it is not great in that it would be nice to know the expected type of a null, but that isn't possible under the current structure.
          If you are in a situtation where you really want abiguous methods to fail, then it provides the ability also, seemly call
          MethodMap.setFailOnAmbiguity(true);

          a test is included

          note about the test...
          I realize that the test isn't the normal way that test are made in velocity, but I would really like to encourage this method, as it is much simplier to
          write
          read
          change
          and is self contained in 1 file. i will print out the actual test so all can see.

          package org.apache.velocity.test;

          import junit.framework.TestCase;
          import org.apache.velocity.app.event.ContextAware;
          import org.apache.velocity.context.Context;
          import org.apache.velocity.util.VelocityParser;

          public class VelocityNullArgumentTestCase extends TestCase implements ContextAware
          {
          /***********************************************************************/
          public void testOverloadedMethodFound() throws Exception

          { assertEquals("you got null", VelocityParser.parseString("$object.getClass($nullValue)", this)); }

          /***********************************************************************/
          public void setContext(Context context)

          { context.put("object", this); context.put("nullValue", null); }

          /***********************************************************************/
          public static String getClass(Class c)

          { return c == null ? "you got null" : c.getName(); }

          /***********************************************************************/
          public static String getClass(String c)

          { return c == null ? "you got null" : c.getClass().getName(); }

          /***********************************************************************/
          /***********************************************************************/
          }

          Show
          Llewellyn Falco added a comment - This resolves the issue at hand by selecting the first method it the case that either 2 will do equally well. it is not great in that it would be nice to know the expected type of a null, but that isn't possible under the current structure. If you are in a situtation where you really want abiguous methods to fail, then it provides the ability also, seemly call MethodMap.setFailOnAmbiguity(true); a test is included note about the test... I realize that the test isn't the normal way that test are made in velocity, but I would really like to encourage this method, as it is much simplier to write read change and is self contained in 1 file. i will print out the actual test so all can see. package org.apache.velocity.test; import junit.framework.TestCase; import org.apache.velocity.app.event.ContextAware; import org.apache.velocity.context.Context; import org.apache.velocity.util.VelocityParser; public class VelocityNullArgumentTestCase extends TestCase implements ContextAware { /***********************************************************************/ public void testOverloadedMethodFound() throws Exception { assertEquals("you got null", VelocityParser.parseString("$object.getClass($nullValue)", this)); } /***********************************************************************/ public void setContext(Context context) { context.put("object", this); context.put("nullValue", null); } /***********************************************************************/ public static String getClass(Class c) { return c == null ? "you got null" : c.getName(); } /***********************************************************************/ public static String getClass(String c) { return c == null ? "you got null" : c.getClass().getName(); } /***********************************************************************/ /***********************************************************************/ }
          Hide
          Llewellyn Falco added a comment -

          the following test breaks.

          package com.spun.util.velocity.tests;

          import junit.framework.TestCase;
          import org.apache.velocity.context.Context;
          import com.spun.util.velocity.ContextAware;
          import com.spun.util.velocity.VelocityParser;

          public class VelocityNullArgumentTest extends TestCase implements ContextAware
          {
          /***********************************************************************/
          public void testOverloadedMethodFound() throws Exception

          { assertEquals("you got null", VelocityParser.parseString("$object.getClass($nullValue)", this)); }

          /***********************************************************************/
          public void setupContext(Context context)

          { context.put("object", this); context.put("nullValue", null); }

          /***********************************************************************/
          public static String getClass(Class c)

          { return c == null ? "you got null" : c.getName(); }

          /***********************************************************************/
          public static String getClass(String c)

          { return c == null ? "you got null" : c.getClass().getName(); }

          /***********************************************************************/
          /***********************************************************************/
          }

          Show
          Llewellyn Falco added a comment - the following test breaks. package com.spun.util.velocity.tests; import junit.framework.TestCase; import org.apache.velocity.context.Context; import com.spun.util.velocity.ContextAware; import com.spun.util.velocity.VelocityParser; public class VelocityNullArgumentTest extends TestCase implements ContextAware { /***********************************************************************/ public void testOverloadedMethodFound() throws Exception { assertEquals("you got null", VelocityParser.parseString("$object.getClass($nullValue)", this)); } /***********************************************************************/ public void setupContext(Context context) { context.put("object", this); context.put("nullValue", null); } /***********************************************************************/ public static String getClass(Class c) { return c == null ? "you got null" : c.getName(); } /***********************************************************************/ public static String getClass(String c) { return c == null ? "you got null" : c.getClass().getName(); } /***********************************************************************/ /***********************************************************************/ }
          Hide
          Will Glass-Husain added a comment -

          same issue, same poster. will resolve second issue as invalid (no new discussion)

          Show
          Will Glass-Husain added a comment - same issue, same poster. will resolve second issue as invalid (no new discussion)
          Hide
          MySign added a comment -

          the bug appears only, if the call of foo is in a file
          which is included by #parse.

          You can reproduce the bug with following files:

          main template file:
          #set ( $param = $someobj.getNull() )
          #parse ( "includecall.vm" )<br>
          #set ( $param = "a string" )
          #parse ( "includecall.vm" )

          includecall.vm:
          $testobj.foo( $param )

          class with the name someobj in the context:

          public class VelocityCachingBug
          {
          public VelocityCachingBug()
          {
          }

          public String foo ( Integer s )

          { return "Integer"; }

          public String foo ( String i )

          { return "String"; }

          public Object getNull()

          { return null; }

          }

          if you render the main template, the second call can't be found by velocity
          because the parsed file is cached only once (as one single node).

          I don't think the solution to this is to cache every call in a parsed file (if
          it's parsed more than once), because that can lead to much more memory use if
          you have many parsed files (like we have...), which are mostly the same.

          Show
          MySign added a comment - the bug appears only, if the call of foo is in a file which is included by #parse. You can reproduce the bug with following files: main template file: #set ( $param = $someobj.getNull() ) #parse ( "includecall.vm" )<br> #set ( $param = "a string" ) #parse ( "includecall.vm" ) includecall.vm: $testobj.foo( $param ) class with the name someobj in the context: public class VelocityCachingBug { public VelocityCachingBug() { } public String foo ( Integer s ) { return "Integer"; } public String foo ( String i ) { return "String"; } public Object getNull() { return null; } } if you render the main template, the second call can't be found by velocity because the parsed file is cached only once (as one single node). I don't think the solution to this is to cache every call in a parsed file (if it's parsed more than once), because that can lead to much more memory use if you have many parsed files (like we have...), which are mostly the same.
          Hide
          Geir Magnusson Jr added a comment -

          I think the bug is more subtle than stated, because I think the problem really is if you have

          public class Foo
          {
          public void a(Integer s) {}
          public void a(String i) {}
          }

          and your template has

          $foo.a($value)

          and if $value is a null the first time and an String the second time, is that when it happens?

          Because if you have

          $foo.a($valueisnull)
          $foo.a($string)

          it shouldn't be a problem because the two $foo.XXXX are both separate nodes in the template's
          syntax tree, and thus are cached independently.

          Can you clarify?

          Show
          Geir Magnusson Jr added a comment - I think the bug is more subtle than stated, because I think the problem really is if you have public class Foo { public void a(Integer s) {} public void a(String i) {} } and your template has $foo.a($value) and if $value is a null the first time and an String the second time, is that when it happens? Because if you have $foo.a($valueisnull) $foo.a($string) it shouldn't be a problem because the two $foo.XXXX are both separate nodes in the template's syntax tree, and thus are cached independently. Can you clarify?

            People

            • Assignee:
              Unassigned
              Reporter:
              MySign
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development