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

Invoke init/destroy/SpEL methods via public declaring type whenever possible #33216

Closed
sbrannen opened this issue Jul 14, 2024 · 3 comments
Closed
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Jul 14, 2024

Overview

The original scope of this issue was limited to method invocations in SpEL expressions (see below); however, after further investigation we have decided to apply the same enhancement for init and destroy methods in spring-beans.


For reflective method invocation in the Spring Expression Language (SpEL), we currently use ClassUtils.getInterfaceMethodIfPossible(...) to attempt to invoke the method via an interface, based on the rationale stated in the Javadoc for that utility.

Determine a corresponding interface method for the given method handle, if possible.

This is particularly useful for arriving at a public exported type on Jigsaw which can be reflectively invoked without an illegal access warning.

This works well for methods which are actually defined in an interface, but it does not work for methods defined in a superclass.

For example, in a SpEL expression it is currently impossible to invoke toString() on a non-public type in a different module. This can be seen when attempting to invoke toString() on an unmodifiable list created by Collections.unmodifiableList(...). Doing so results in an exception similar to the following.

Caused by: java.lang.reflect.InaccessibleObjectException: Unable to make public java.lang.String java.util.Collections$UnmodifiableCollection.toString() accessible: module java.base does not "opens java.util" to unnamed module @5f80e27e
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:354)
	at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
	at java.base/java.lang.reflect.Method.checkCanSetAccessible(Method.java:199)
	at java.base/java.lang.reflect.Method.setAccessible(Method.java:193)
	at org.springframework.util.ReflectionUtils.makeAccessible(ReflectionUtils.java:566)
	at org.springframework.expression.spel.support.ReflectiveMethodExecutor.execute(ReflectiveMethodExecutor.java:125)
	... 9 more

Users can address this by adding an appropriate --add-opens declaration, such as --add-opens java.base/java.util=ALL-UNNAMED.

However, it would be nice if applications did not have to add such an --add-opens declaration for such use cases in SpEL.

For compiled expressions, we added support in #29857 to invoke the method via a public declaring type, since that is an actual requirement for compiled Java code. So, we should consider doing the same for methods invoked via reflection.

Related Issues

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Jul 14, 2024
@sbrannen sbrannen added this to the 6.2.x milestone Jul 14, 2024
@sbrannen
Copy link
Member Author

A failing test case and proof of concept can be viewed in the following feature branch.

main...sbrannen:spring-framework:issues/gh-33216-spel-invoke-method-via-public-type

Note, however, that MethodInvocationTests.methodOfJavaLangObjectDeclaredInNonPublicType() will only fail prior to the fix if the test is run without an --add-opens java.base/java.util=ALL-UNNAMED declaration.

@snicoll snicoll added the type: enhancement A general enhancement label Jul 14, 2024
@snicoll

This comment was marked as outdated.

@sbrannen

This comment was marked as outdated.

@sbrannen sbrannen self-assigned this Aug 17, 2024
@sbrannen sbrannen removed the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Aug 19, 2024
@sbrannen sbrannen modified the milestones: 6.2.x, 6.2.0-RC1 Aug 19, 2024
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 19, 2024
@sbrannen sbrannen changed the title Always invoke method via public declaring type in SpEL Always invoke methods via public declaring type in SpEL Aug 19, 2024
sbrannen added a commit to sbrannen/spring-framework that referenced this issue Aug 21, 2024
@sbrannen sbrannen changed the title Always invoke methods via public declaring type in SpEL Always invoke init/destroy/SpEL methods via public declaring type Aug 22, 2024
@sbrannen sbrannen changed the title Always invoke init/destroy/SpEL methods via public declaring type Invoke init/destroy/SpEL methods via public declaring type whenever possible Aug 22, 2024
sbrannen added a commit that referenced this issue Aug 27, 2024
Commit 47f88e1 introduced support for invoking init/destroy/SpEL
methods via public types whenever possible. To achieve that,
getInterfaceMethodIfPossible() was modified so that it only returned
interface methods from public interfaces; however, we have learned that
third parties relied on the previous behavior which found any interface
method (even in non-public interfaces).

In light of the above, this commit partially reverts commit 47f88e1
in order to reinstate getInterfaceMethodIfPossible() in non-deprecated
form with its previous behavior.

See gh-33216
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants