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

An action without parameters results in NoSuchMethodException starting with Spring Framework 6.2.0-M1 #1802

Closed
transentia opened this issue Mar 27, 2024 · 6 comments · May be fixed by #1819
Closed
Assignees
Labels
status: superseded An issue that has been superseded by another type: enhancement
Milestone

Comments

@transentia
Copy link

Long-established app. uses Webflow 3.0.0.
Thought I'd try the upgrade to Framework 6.2.0-SNAPSHOT...see what breaks.

This code broke:

public class ExistingReportActions extends FormAction {
    ...

    public Event evaluateExistingReport() {
        ...
    }

Rectified by supplying the 'missing' RequestContext parameter explicitly:

public class ExistingReportActions extends FormAction {
    ...

    public Event evaluateExistingReport(RequestContext requestContext) {
        ...
    }

The flow xml remains as:

    <!-- *** existingReport MUST BE THE FIRST STATE in the flow *** -->
    <action-state id="existingReport">
        <evaluate expression="existingReportActions.evaluateExistingReport"/>
        ....
    </action-state>

Eliding the RequestContext parameter used to be allowable. It appears that it no longer is.

A fairly trivial difference, but a breaking one nonetheless.

@rstoyanchev
Copy link
Contributor

Thanks for the report. Could you extract a small reproducer?

@rstoyanchev rstoyanchev self-assigned this Sep 18, 2024
@rstoyanchev rstoyanchev added this to the 3.0.1 milestone Sep 18, 2024
snicoll added a commit that referenced this issue Oct 10, 2024
This commit adds a GHA workflow that builds the project against a list
of configurable Spring Framework versions. The build action has been
updated to accept an additional input that tunes the Spring Framework
version the build is using

As a result, the default Spring Framework version of the project is now
configured in gradle.properties.

Closes gh-1802
@rstoyanchev
Copy link
Contributor

In 6.0, the method is called successfully via SpEL, but in 6.2 the same expression does not return a value, and Web Flow tries the overloaded method signature with a RequestControl parameter, and fails if that doesn't exist.

It does work with parenthesis, i.e. "existingReportActions.evaluateExistingReport()".

@rstoyanchev rstoyanchev changed the title Running with Framework 6.2.0-SNAPSHOT gives NoSuchMethodException for parameterless action An action with parameters results in NoSuchMethodException starting with Spring Framework 6.2.0-M1 Nov 1, 2024
@rstoyanchev
Copy link
Contributor

This changed in 6.2.0-M1. I cannot reproduce it in 6.1.x.

I have not yet narrowed it down to a specific change, but suspect it is expected, and we may just have to document it as an upgrade note.

@sbrannen
Copy link
Member

sbrannen commented Nov 5, 2024

I'm currently investigating whether we can support the previous use case in Spring Web Flow, but in the interim I can provide my initial findings here.

Basically, there are two different SpEL features that come into play here.

  • property reference
  • method reference

existingReportActions.evaluateExistingReport is a property reference that is supported by a registered PropertyAccessor.

existingReportActions.evaluateExistingReport() and existingReportActions.evaluateExistingReport(#requestContext) are both method references.

Thus, adding the () to existingReportActions.evaluateExistingReport converts it to a method reference which is handled by org.springframework.expression.spel.ast.MethodReference instead of org.springframework.expression.spel.ast.PropertyOrFieldReference.


Without the (), a registered PropertyAccessor is used to invoke the property accessor method.

If the org.springframework.expression.spel.support.ReflectivePropertyAccessor is used (which appears to be the case prior to Spring Framework 6.2), the invocation works.

However, if the org.springframework.webflow.expression.spel.ActionPropertyAccessor is used (which appears to be the case starting with Spring Framework 6.2 M1), the invocation fails since the method does not declare a RequestContext argument which is required by org.springframework.webflow.action.MultiAction.doExecute(RequestContext).

I believe the change in behavior (switch from ReflectivePropertyAccessor to ActionPropertyAccessor) is due to the following improvements for PropertyAccessor sorting.

In other words, I believe the behavior with Spring Framework 6.2 is the expected behavior in terms of SpEL contracts, and I believe those fixes/improvements in SpEL highlight something that maybe never should have worked in Spring Web Flow with the current implementations of ActionPropertyAccessor and MultiAction.

I will attempt to confirm that last statement and report back here.

@sbrannen
Copy link
Member

sbrannen commented Nov 5, 2024

In other words, I believe the behavior with Spring Framework 6.2 is the expected behavior in terms of SpEL contracts, and I believe those fixes/improvements in SpEL highlight something that maybe never should have worked in Spring Web Flow with the current implementations of ActionPropertyAccessor and MultiAction.

I will attempt to confirm that last statement and report back here.

I confirmed that.


Cause of the Error

The current implementation of ActionPropertyAccessor does not align with the requirements of MultiAction.

  • ActionPropertyAccessor claims it supports any Action method, disregarding the formal parameter list of the named method.
  • MultiAction requires that the Action method accepts a single RequestContext argument.

Thus, ActionPropertyAccessor incorrectly claims that it supports an Action method that accepts zero arguments, when it in fact should not. The result is that MultiAction fails to invoke the method since it does not accept the single RequestContext argument, resulting in the NoSuchMethodException.

Explanation

Prior to Spring Framework 6.2, SpEL incorrectly sorted the applicable PropertyAccessor implementations, giving the ReflectivePropertyAccessor priority over the ActionPropertyAccessor.

The effect of that is that an expression such as existingReportActions.evaluateExistingReport was handled by ReflectivePropertyAccessor which always invokes the evaluateExistingReport() method without any arguments (like a JavaBeans "getter" method).

Beginning with Spring Framework 6.2, SpEL correctly sorts the applicable PropertyAccessor implementations, by taking into account their getSpecificTargetClasses() methods. This gives priority to the ActionPropertyAccessor, effectively overriding the ReflectivePropertyAccessor.

The effect of that is that an expression such as existingReportActions.evaluateExistingReport is now handled by ActionPropertyAccessor since existingReportActions is in instance of Action. An AnnotatedAction is created which tracks the Action target and method name: existingReportActions and evaluateExistingReport in this scenario. MultiAction.doExecute(RequestContext) then throws an exception, because MethodKey.resolveMethod() fails to resolve the nonexistent evaluateExistingReport(RequestContext) method.

PropertyAccessor Execution Order

When evaluating the existingReportActions.evaluateExistingReport SpEL expression...

With Spring Framework 6.1.x, the registered property accessors are evaluated in this order:

  • ReflectivePropertyAccessor
  • FlowVariablePropertyAccessor
  • BeanFactoryPropertyAccessor
  • ActionPropertyAccessor

With Spring Framework 6.2, the registered property accessors are evaluated in this order:

  • ActionPropertyAccessor
  • ReflectivePropertyAccessor
  • FlowVariablePropertyAccessor
  • BeanFactoryPropertyAccessor

In summary, ActionPropertyAccessor is now correctly queried before ReflectivePropertyAccessor with Spring Framework 6.2.

Proposed Fix

The following change to ActionPropertyAccessor allows the reported use case to work.

@Override
public boolean canRead(EvaluationContext context, Object target, String name) {
	// Ensure the target is an Action.
	if (!(target instanceof Action)) {
		return false;
	}
	// Ensure the method adheres to the signature required by:
	// Action: execute(RequestContext)
	// or
	// MultiAction: <method name>(RequestContext)
	Method method = ReflectionUtils.findMethod(target.getClass(), name, RequestContext.class);
	return (method != null);
}

@rstoyanchev rstoyanchev changed the title An action with parameters results in NoSuchMethodException starting with Spring Framework 6.2.0-M1 An action without parameters results in NoSuchMethodException starting with Spring Framework 6.2.0-M1 Nov 6, 2024
sbrannen added a commit to sbrannen/spring-webflow that referenced this issue Nov 9, 2024
Prior to this commit, if a Spring Expression Language (SpEL) expression
did not include parentheses for a MultiAction method that does not
accept a RequestContext argument, the flow would pass on Spring
Framework versions prior to 6.2, but the same flow would fail on Spring
Framework 6.2 M1 or later. The following is such an expression and is
effectively a property reference which must be handled by a registered
PropertyAccessor: "reportActions.evaluateReport".

The reason for the change in behavior is that Spring Framework 6.2 M1
includes a bug fix for ordering SpEL PropertyAccessors. That bug fix
correctly results in Spring Web Flow's ActionPropertyAccessor being
evaluated before SpEL's ReflectivePropertyAccessor. Consequently, an
expression such as "reportActions.evaluateReport" is now handled by
ActionPropertyAccessor which indirectly requires that the action method
accept a RequestContext argument. When the method does not accept a
RequestContext argument, the flow fails with a NoSuchMethodException.

To address that, this commit revises the implementation of canRead(...)
in ActionPropertyAccessor so that it only returns `true` if the action
method accepts a RequestContext argument. When ActionPropertyAccessor's
canRead(...) method returns `false`, the generic
ReflectivePropertyAccessor will be used instead, which restores the
pre-6.2 behavior for such expressions.

The test suite passes for the following Spring Framework versions.

- ./gradlew -PspringFrameworkVersion=6.0.23 --rerun-tasks clean check
- ./gradlew -PspringFrameworkVersion=6.1.14 --rerun-tasks clean check
- ./gradlew -PspringFrameworkVersion=6.2.0-RC3 --rerun-tasks clean check

See spring-projects/spring-framework#33861
See spring-projects/spring-framework#33862
Closes spring-projectsgh-1802
@rstoyanchev rstoyanchev linked a pull request Nov 13, 2024 that will close this issue
@rstoyanchev
Copy link
Contributor

Superseded by #1819.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2024
@rstoyanchev rstoyanchev added the status: superseded An issue that has been superseded by another label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another type: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants