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

Allow JsonNodeELResolver to invoke methods #3996

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rssap
Copy link

@rssap rssap commented Dec 7, 2024

This PR proposes a change to the Util class to fix a problem in the BeanELResolver, when invoking the get method on an ArrayNode.

In this commit, the logic of the BeanELResolver was enhanced to better deal with method overloading.

We encountered some inconsistencies in the Behavior of the BeanELResolver compared to an older flowable release (in our case 6.6.0)
The ArrayNode has two methods with the name "get":

  • get(int index)
  • get(String fieldName)

Before the mentioned commit, it was possible to use the BeanELResolver to invoke ArrayNode::get and pass a Long as parameter, which is no longer possible. Even though none of the two methods apply to a Long (without casting it to an int), I think that there is still a benefit in supporting it:

  • JUEL expressions in Flowable will treat the "0" in ${array.get(0)} as a Long. Therefore, the BeanELResolver will try to invoke "get" with a Long as parameter.
  • As it was possible in the past to use such expressions, it would be great to have backwards compatibility.

I have added a unit test to demonstrate the problem. With the modification to the Util class, the test passes. Without it, it fails. When the test is executed on the state of flowable-6.6.0 (some release before the logic of the BeanELResolver was changed), it passes as well.

Without the modification, the test fails with the following exception: org.flowable.common.engine.impl.javax.el.MethodNotFoundException: Unable to find unambiguous method: class com.fasterxml.jackson.databind.node.ArrayNode.get(java.lang.Long)

Check List:

  • Unit tests: YES
  • Documentation: NO

@filiphr
Copy link
Contributor

filiphr commented Dec 9, 2024

Thanks for the analysis and PR @rssap. That line is a bit dangerous as JUEL can coerce anything into a String. We've had mistakes before where someone would call a method accepting a string with some object and the objects toString() would be called, which then leads to some strange and unexpected behaviour.

I would be more keen on adjusting the JsonNodeELResolver instead of adjusting this on the BeanELResolver.

We also try to avoid using mocks like in the test you have. A better test would be in ExpressionManagerTest or in JsonTest

@rssap
Copy link
Author

rssap commented Dec 9, 2024

I would be more keen on adjusting the JsonNodeELResolver instead of adjusting this on the BeanELResolver.

Sounds good. I have adjusted the JsonNodeELResolver now. To remain flexible and prevent code duplication, I reused the BeanELResolver::invoke internally. Let me know if you like/dislike that.

Small note: The expressions ${array.get(0)} and ${array[0]} return different objects, but I hope that is acceptable:

  • ${array.get(0)} returns a JsonNode (e.g. a TextNode)
  • ${array[0]} returns the actual (e.g. string) value. No more .textValue() needed.

@rssap rssap changed the title Allow BeanELResolver to invoke ArrayNode::get with a Long Allow JsonNodeELResolver to invoke methods Dec 10, 2024
@rssap
Copy link
Author

rssap commented Dec 11, 2024

@filiphr please have another look at my changes.

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

Successfully merging this pull request may close these issues.

2 participants