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

MethodInvokingFactoryBean fails to invoke publicly exported methods overridden by internal classes when using JPMS #34028

Open
anandkarandikar opened this issue Dec 4, 2024 · 4 comments
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: waiting-for-triage An issue we've not yet triaged or decided on

Comments

@anandkarandikar
Copy link

anandkarandikar commented Dec 4, 2024

Description

We encountered an issue while using Spring Framework (v5.3.29) in a Java 17 modular application. The problem arises when trying to invoke a public method from an exported class that is overridden by another class (within an internal package) using Spring's MethodInvokingFactoryBean. The method is not accessible due to Java Module System restrictions. This issue is reproducible in Java 17 and does not occur in Java 8.

(Tested with: 5.3.29, 5.3.39, 6.2.0)


Steps to reproduce

Start with a simple maven project and choose Java 17 as the preferred JDK

1. Dependencies (pom.xml):

<dependencies>
    <dependency>
        <groupId>org.springframework</groupId>
        <artifactId>spring-context</artifactId>
        <version>5.3.29</version>
    </dependency>
    <dependency>
        <groupId>org.apache.logging.log4j</groupId>
        <artifactId>log4j-core</artifactId>
        <version>2.17.2</version>
    </dependency>
</dependencies>

2. Custom Implementation

Extending org.apache.logging.log4j.core.LoggerContext to create a custom logger context class:

package com.example.impl;

import org.apache.logging.log4j.core.LoggerContext;

public class MyLoggerContext extends LoggerContext {
    public MyLoggerContext(String name) {
        super(name);
        System.out.println("Initializing LoggerContext " + name);
    }

    @Override
    public void reconfigure() {
        super.reconfigure();
        System.out.println("Called reconfigure with " + getName());
    }
}

3. Factory Class

A factory class LoggerContextFactory that provides an instance of MyLoggerContext:

package com.example.api;

import com.example.impl.MyLoggerContext;
import org.apache.logging.log4j.core.LoggerContext;

public class LoggerContextFactory {
    public static LoggerContext createLoggerContext() {
        return new MyLoggerContext("TestLoggerContext");
    }
}

4. Module Configuration

The module-info.java file exports only the com.example.api package, keeping com.example.impl internal:

module CustomLoggerContext {
    requires org.apache.logging.log4j.core;
    requires spring.context;
    exports com.example.api;
}

5. Spring Configuration

Defined in springApplicationContext.xml:

<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans"
       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xsi:schemaLocation="http://www.springframework.org/schema/beans
       http://www.springframework.org/schema/beans/spring-beans.xsd">

    <bean id="loggerContextFactory" class="com.example.api.LoggerContextFactory"/>

    <bean id="loggerContextInstance" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean">
        <property name="targetObject" ref="loggerContextFactory"/>
        <property name="targetMethod" value="createLoggerContext"/>
    </bean>

    <bean id="invokeReconfigureMethod" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean">
        <property name="targetObject" ref="loggerContextInstance"/>
        <property name="targetMethod" value="reconfigure"/>
    </bean>
</beans>

6. MainClass

Used to initialize the Spring ApplicationContext:

package com.example.runner;

import org.springframework.context.support.ClassPathXmlApplicationContext;

public class MainClass {
    public static void main(String[] args) {
        ClassPathXmlApplicationContext ctx = new ClassPathXmlApplicationContext("springApplicationContext.xml");
        System.out.println("Successful");
    }
}

Execution Output / Stacktrace

Initializing LoggerContext TestLoggerContext
Exception in thread "main" org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'invokeReconfigureMethod' defined in class path resource [springApplicationContext.xml]: Invocation of init method failed; nested exception is java.lang.IllegalAccessException: class org.springframework.util.MethodInvoker cannot access class com.example.impl.MyLoggerContext (in module CustomLoggerContext) because module CustomLoggerContext does not export com.example.impl to unnamed module @277c0f21
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1804)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean(AbstractAutowireCapableBeanFactory.java:620)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean(AbstractAutowireCapableBeanFactory.java:542)
    at org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$0(AbstractBeanFactory.java:335)
    at org.springframework.beans.factory.support.DefaultSingletonBeanRegistry.getSingleton(DefaultSingletonBeanRegistry.java:234)
    at org.springframework.beans.factory.support.AbstractBeanFactory.doGetBean(AbstractBeanFactory.java:333)
    at org.springframework.beans.factory.support.AbstractBeanFactory.getBean(AbstractBeanFactory.java:208)
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.preInstantiateSingletons(DefaultListableBeanFactory.java:936)
    at [email protected]/org.springframework.context.support.AbstractApplicationContext.finishBeanFactoryInitialization(AbstractApplicationContext.java:921)
    at [email protected]/org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:583)
    at [email protected]/org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:144)
    at [email protected]/org.springframework.context.support.ClassPathXmlApplicationContext.<init>(ClassPathXmlApplicationContext.java:85)
    at CustomLoggerContext/com.example.runner.MainClass.main(MainClass.java:7)
Caused by: java.lang.IllegalAccessException: class org.springframework.util.MethodInvoker cannot access class com.example.impl.MyLoggerContext (in module CustomLoggerContext) because module CustomLoggerContext does not export com.example.impl to unnamed module @277c0f21
    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:561)
    at org.springframework.util.MethodInvoker.invoke(MethodInvoker.java:283)
    at org.springframework.beans.factory.config.MethodInvokingBean.invokeWithTargetException(MethodInvokingBean.java:123)
    at org.springframework.beans.factory.config.MethodInvokingFactoryBean.afterPropertiesSet(MethodInvokingFactoryBean.java:108)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods(AbstractAutowireCapableBeanFactory.java:1863)
    at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean(AbstractAutowireCapableBeanFactory.java:1800)
    ... 12 more

Process finished with exit code 1


Observations

  1. This issue does not occur in Java 8 or when not using JPMS.
  2. The default behavior of MethodInvokingFactoryBean fails because the MethodInvoker attempts to access a method in MyLoggerContext via reflection, which is restricted due to Java Module System encapsulation.
  3. A workaround exists, but it requires an undesirable modification:

Original Bean Definition:

<bean id="invokeReconfigureMethod" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean">
    <property name="targetObject" ref="loggerContextInstance"/>
    <property name="targetMethod" value="reconfigure"/>
</bean>

Modified Bean Definition with staticMethod:

<bean id="invokeReconfigureMethod" class="org.springframework.beans.factory.config.MethodInvokingFactoryBean">
    <property name="targetObject" ref="loggerContextInstance"/>
    <property name="staticMethod" value="org.apache.logging.log4j.core.LoggerContext.reconfigure"/>
</bean>

This workaround works because we are specifying the class name to take the method from to be the exported one (LoggerContext) instead of the internal one. The MethodInvoker#prepare() method handles property staticMethod and invokes the resolveClassName() method that seems to be helping in this case.

Execution Output/Stacktrace with the workaround

Initializing LoggerContext TestLoggerContext
Called reconfigure with TestLoggerContext
Successful

The output line Called reconfigure with TestLoggerContext signifies that overridden reconfigure() method was called from MyLoggerContext class.

Ideally, staticMethod should not be used to invoke a non-static method.

  1. The inability to invoke public methods in parent classes/interfaces just because they are overridden by internal classes creates a limitation. We have customers that have been using this kind of Spring configurations for a long time, and now they are facing a regression when they upgrade their Java version to one that supports JPMS. Even though we have the option to provide the workaround, this is coming as an unexpected breaking change for them.

Proposed Fix:

A similar issue was addressed previously in Spring EL (SpEL) by enhancing the ReflectivePropertyAccessor to locate accessor methods in public interfaces and public (super-)classes, defaulting to current logic if not found. (See #21385).

We suggest implementing a comparable fix for Spring's MethodInvokingFactoryBean in the bean creation process to handle such cases.


Related Issues

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 4, 2024
@sbrannen sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Dec 5, 2024
@sbrannen sbrannen changed the title MethodInvokingFactoryBean fails to invoke publicly exported methods overridden by internal classes when using JPMS MethodInvokingFactoryBean fails to invoke publicly exported methods overridden by internal classes when using JPMS Dec 5, 2024
@sbrannen
Copy link
Member

sbrannen commented Dec 5, 2024

Hi @anandkarandikar,

Congratulations on submitting your first issue for the Spring Framework!

And thanks for the very detailed write up. Much appreciated.

Ideally, staticMethod should not be used to invoke a non-static method.

I agree that's not ideal, but I must admit: that's a very clever way around the problem. 👍

That staticMethod property in MethodInvoker could just as well be called fullyQualifiedMethod. In light of that, I think it's a reasonable workaround for the time being.

A similar issue was addressed previously in Spring EL (SpEL) by enhancing the ReflectivePropertyAccessor to locate accessor methods in public interfaces and public (super-)classes, defaulting to current logic if not found. We suggest implementing a comparable fix for Spring's MethodInvokingFactoryBean in the bean creation process to handle such cases.

That's correct. We have been making use of ClassUtils.getInterfaceMethodIfPossible() for quite some time in various places, and in Spring Framework 6.2 we introduced ClassUtils.getPubliclyAccessibleMethodIfPossible().

However, neither of those utilities take into account visibility restrictions imposed by the Java Module System.

For example, in your use case MyLoggerContext is a public class, and reconfigure() is a public method. Thus, neither ClassUtils.getInterfaceMethodIfPossible() nor ClassUtils.getPubliclyAccessibleMethodIfPossible() would be able to determine that the method should instead be invoked via the LoggerContext declaration of the reconfigure() method.

As a side note, if MyLoggerContext were not declared as public, then ClassUtils.getPubliclyAccessibleMethodIfPossible() would be able to determine that the method should be invoked via LoggerContext#reconfigure.

In summary, I don't believe we have any method resolution logic in Spring Framework that takes module visibility constraints into account when determining which method to invoke.

@jhoeller, thoughts?

@anandkarandikar
Copy link
Author

@sbrannen
Thank you for the feedback regarding the workaround we've been using.

On other note

However, neither of those utilities take into account visibility restrictions imposed by the Java Module System.

I believe this is reflected in the intent of the following comment within the getPubliclyAccessibleMethodIfPossible method:

// If the method is not public, we can abort the search immediately; or if the method's
// declaring class is public, the method is already publicly accessible.

However, this assumption appears to be invalid in cases where the class is public but resides in a package that is not exported. In such scenarios, even a public method within a public class cannot be invoked due to module visibility restrictions.

Would it be possible to enhance ClassUtils to account for these module system restrictions? Additionally, since MethodInvokingFactoryBean does not currently utilize this flow, could this be considered another area for improvement?

@sbrannen
Copy link
Member

Would it be possible to enhance ClassUtils to account for these module system restrictions?

It believe it should be possible, but we will need to discuss this topic within the team.

Additionally, since MethodInvokingFactoryBean does not currently utilize this flow, could this be considered another area for improvement?

The method resolution algorithm used in MethodInvokingFactoryBean is actually inherited from MethodInvoker.

So, we would first need to decide if we want to modify that general algorithm. However, another option would be to override getPreparedMethod() in MethodInvokingFactoryBean (or perhaps in MethodInvokingBean) so that it can apply ClassUtils.getPubliclyAccessibleMethodIfPossible() to the method resolved by the algorithm in MethodInvoker.

In any case feel free to open a separate ticket to suggest that MethodInvokingBean/MethodInvokingFactoryBean make an attempt to invoke the method via a publicly accessible interface or superclass. Though, the change in semantics should perhaps be enabled via a boolean flag, since traditional users of these invokers may expect that exactly the configured method is invoked.

@anandkarandikar
Copy link
Author

@sbrannen Appreciate the feedback and a peek into the internals.
We will discuss the possibility of creating a separate ticket within our team and if required, will surely create one.

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) status: waiting-for-triage An issue we've not yet triaged or decided on
Projects
None yet
Development

No branches or pull requests

3 participants