[
https://issues.apache.org/jira/browse/VELOCITY-952?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17710993#comment-17710993
]
Thomas Mortagne commented on VELOCITY-952:
------------------------------------------
{quote}
bq. respecting the method return type
Carrying this information around with the object sounds like a complex thing to
change (but maybe not, I don't know the Velocity AST very well), but I agree
it's probably the most accurate.
{quote}
So I finally did some digging on what it would take to implement this idea, and
it's indeed not trivial. From what I understood, the problem is that here is no
way to associate metadata to a value in the context right now, so all the APIs
which manipulate the value of a variable don't have any way to pass/return more
info about this value (thinking especially about associating a Type with the
value in the Context and how to return a Type along with the value returned by
ASTMethod#execute.
I started by refactoring a bit {{org.apache.velocity.context.Context}} to
support associating metadata to an entry without breaking existing API, but
then the number of things to change in pretty critical places to take this into
account is a bit overwhelming for me and a huge challenge in terms of retro
compatibly without really knowing if that's really the direction that you guys
want to take.
Problem is that this bug makes pretty much impossible to say that XWiki
supports Java 17 because of the many unforeseen IllegalAccessException failures
you can end up with depending on which apparently standard Java API you are
manipulating in a script. So for now my plan is to workaround this with an
uberspector on XWiki side doing what I initially proposed: try to find the
lowest overwritten Method (which, I agree, is not the best and not something I
would recommend on standard Velocity side compared to the "carry the return
type around with the value" idea).
> 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.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org
For additional commands, e-mail: dev-h...@velocity.apache.org
