[jira] [Comment Edited] (VELOCITY-952) Velocity is calling the "wrong" method

["Robert Scholte (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/VELOCITY-952?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17504829#comment-17504829 ] 

Robert Scholte edited comment on VELOCITY-952 at 3/11/22, 9:46 AM:
-------------------------------------------------------------------

{code}
public class ModuleVisibilityTest extends BaseTestCase {

	public ModuleVisibilityTest(String name) {
		super(name);
	}
	
	@Override
	protected void setUpContext(VelocityContext context) {
		context.put("timeZone", TimeZone.getTimeZone("UTC"));
	}
	
	public void testModuleVisibility() {
		assertEvalEquals("0","$timeZone.getOffset(0)");
	}

}
{code}

A small testcase to reproduce it. And I think the assumption is right.
In that case, what should be improved is respecting the method return type instead of the class of the returned instance.


was (Author: rfscholte):
{code}
public class ModuleVisibilityTest extends BaseTestCase {

	public ModuleVisibilityTest(String name) {
		super(name);
	}
	
	@Override
	protected void setUpContext(VelocityContext context) {
		context.put("timeZone", TimeZone.getTimeZone("UTC"));
	}
	
	public void testModuleVisibilityTest() {
		assertEvalEquals("0","$timeZone.getOffset(0)");
	}

}
{code}

A small testcase to reproduce it. And I think the assumption is right.
In that case, what should be improved is respecting the method return type instead of the class of the returned instance.

> Velocity is calling the "wrong" method
> --------------------------------------
>
>                 Key: VELOCITY-952
>                 URL: https://issues.apache.org/jira/browse/VELOCITY-952
>             Project: Velocity
>          Issue Type: Bug
>          Components: Engine
>    Affects Versions: 2.3
>            Reporter: Thomas Mortagne
>            Priority: Major
>
> OK, the title is maybe a bit harsh, but it catches the eyes :)
> At XWiki we recently started testing on Java 17 to see if there is any problem which leaded us to add things like {{--add-opens java.base/java.util=ALL-UNNAMED}} but Velocity happen to be the source of another problem related to the new module world.
> When doing something like {{$datetool.timeZone.getOffset(0)}} ($datetool being the org.apache.velocity.tools.generic.DateTool) we get the following error:
> {noformat}
> ...
> Caused by: java.lang.IllegalAccessException: class org.apache.velocity.util.introspection.UberspectImpl$VelMethodImpl cannot access class sun.util.calendar.ZoneInfo (in module java.base) because module java.base does not export sun.util.calendar to unnamed module @7ca16adc
>  at java.base/jdk.internal.reflect.Reflection.newIllegalAccessException(Reflection.java:392)
>  at java.base/java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:674)
>  at java.base/java.lang.reflect.Method.invoke(Method.java:560)
>  at org.apache.velocity.util.introspection.UberspectImpl$VelMethodImpl.doInvoke(UberspectImpl.java:571)
>  at org.apache.velocity.util.introspection.UberspectImpl$VelMethodImpl.invoke(UberspectImpl.java:554)
>  at org.apache.velocity.runtime.parser.node.ASTMethod.execute(ASTMethod.java:221)
>  ... 199 more
> {noformat}
> The reason is that while the developer intent/expectation was to call "java.util.TimeZone#getOffset(0)" what Velocity really called from JVM point of view is "sun.util.calendar.ZoneInfo.getOffset(0)" directly.
> That's because Velocity is doing (I assume, since I did not check the exact code) the equivalent of:
> {{noformat}}
> java.util.TimeZone.getDefault().getClass().getMethod("getOffset", long.class).invoke(TimeZone.getDefault(), 0);
> {{noformat}}
> which is itself the equivalent of:
> {{noformat}}
> sun.util.calendar.ZoneInfo.class.getMethod("getOffset", long.class).invoke(TimeZone.getDefault(), 0);
> {{noformat}}
> I guess the only way to fix this would be to track down the lowest overridden Method (I agree, it might not always be easy to choose between two interfaces declaring a method with the same signature, but choosing the first one we find from the same hierarchy level is still better than nothing) and call that one instead. With the use case used as example in this issue, that would mean ending up doing the equivalent of:
> {{noformat}}
> java.util.TimeZone.class.getMethod("getOffset", long.class).invoke(TimeZone.getDefault(), 0);
> {{noformat}}
> which, from JVM point of view, is covered by the {{--add-opens java.base/java.util=ALL-UNNAMED}}.
> It would be a big change, but at least can't think of any retro-compatibility problem it might cause.
> One might be tempted to answer "just add {{--add-opens java.base/sun.util.calendar=ALL-UNNAMED}}" but it does not seem fair as this is not what the API that the script uses was exposing at all, you might need to document a different one for each JVM implementation (even if it's probably unlikely for this specific example) but more importantly you will potentially need quite a lot of those and will only discover it at runtime since it's not so easy to guess from an API in which you only know the public JVM classes since that's what you manipulate.
> I would be happy to work on this, but I wanted first ask what others think about this problem in general and the possible solutions I may have missed.



--
This message was sent by Atlassian Jira
(v8.20.1#820001)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@velocity.apache.org
For additional commands, e-mail: dev-help@velocity.apache.org