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

ognl 3.4.5 - Issue with using custom class loader in getValue #350

Closed
davsclaus opened this issue Jan 8, 2025 · 8 comments · Fixed by #353
Closed

ognl 3.4.5 - Issue with using custom class loader in getValue #350

davsclaus opened this issue Jan 8, 2025 · 8 comments · Fixed by #353
Milestone

Comments

@davsclaus
Copy link

At Apache Camel we have a camel-ognl module
https://github.com/apache/camel/tree/main/components/camel-ognl

When we upgrade from 3.4.4 to 3.4.5 we had an unit test error in one of our tests.
https://github.com/apache/camel/blob/main/components/camel-ognl/src/test/java/org/apache/camel/language/ognl/OgnlTest.java#L48

At first sight I suspect its this change
#337

in the getValue method, that somehow the custom classloader that Camel will provide in the unit test is not being in use.

The custom class loader is part of the test
https://github.com/apache/camel/blob/main/components/camel-ognl/src/test/java/org/apache/camel/language/ognl/MyClassResolver.java#L30

where the classloader will now correctly fix the classname and load the class (notice the Animal1 vs Animal) in the name.

The camel code that creates the ognl context and invokes ognl from camel in that test is executing this code
https://github.com/apache/camel/blob/main/components/camel-ognl/src/main/java/org/apache/camel/language/ognl/OgnlExpression.java#L53

@davsclaus davsclaus changed the title ognl 3.4.5 - Issue with using custom class loader in OgetValue ognl 3.4.5 - Issue with using custom class loader in getValue Jan 8, 2025
@davsclaus
Copy link
Author

We made a workaround so we can use 3.4.5 but have it work with the test
apache/camel@6f7d168

@lukaszlenart
Copy link
Collaborator

Thanks for reporting this, yet I'm not sure if I understand the problem. In your code

exchange.getContext().setClassResolver(new MyClassResolver(context));

should getContext() and context be the same? And they are changed because of #337 ?

@davsclaus
Copy link
Author

Thanks for reporting this, yet I'm not sure if I understand the problem. In your code

exchange.getContext().setClassResolver(new MyClassResolver(context));

should getContext() and context be the same? And they are changed because of #337 ?

This is pure Apache Camel code (context is not Ognl context here). This code is what setup Camel to use that custom hacked classloader we use in the unit test.

@lukaszlenart
Copy link
Collaborator

Hm... so why does it fail? I don't see a connection with the change and OGNL :(

@davsclaus
Copy link
Author

Hm... so why does it fail? I don't see a connection with the change and OGNL :(

I think its this code change in src/main/java/ognl/Ognl.java (link to PR in top) see the code changes in getValue.

I would assume that ognl when attempting to load the class Animal1 does not call the custom class loader provided by Camel but maybe mistakenly uses the default classloader from Ognl due to changes in getValue.

@davsclaus
Copy link
Author

So its maybe due to this change / wrapping

OgnlContext ognlContext = addDefaultContext(root, context);

@davsclaus
Copy link
Author

My gut feeling is that addDefaultContext(root, context); somehow causes ognl to not use my custom classloader but instead the default from ognl instead. So maybe that code should somehow take into account if the custom context instance have custom classloader (and maybe other settings) then they should be used first.

@lukaszlenart
Copy link
Collaborator

@davsclaus please review the linked PR, it should solve the problem.

@lukaszlenart lukaszlenart added this to the 3.4.6 milestone Jan 23, 2025
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 a pull request may close this issue.

2 participants