From 4f086322d0b5e9e9792860a90c8765a08530041f Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sat, 25 Jul 2026 16:05:14 +0300 Subject: [PATCH] Do not reuse cached ConstructorExecutor without ConstructorResolvers Prior to this commit, a SpEL ConstructorReference could reuse a cached ConstructorExecutor even when the current EvaluationContext no longer had any registered ConstructorResolvers, leading to inconsistent behavior if the EvaluationContext changes between evaluations of the same expression. This commit aligns ConstructorReference with the analogous logic in PropertyOrFieldReference by discarding the cached ConstructorExecutor whenever there are no ConstructorResolvers registered in the current EvaluationContext, ensuring that constructor resolution consistently fails with a CONSTRUCTOR_NOT_FOUND exception in that scenario. Closes gh-36985 --- .../spel/ast/ConstructorReference.java | 65 ++++++++++--------- .../spel/ConstructorInvocationTests.java | 28 ++++++++ 2 files changed, 64 insertions(+), 29 deletions(-) diff --git a/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java b/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java index b2a0457e05d..e7b66721972 100644 --- a/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java +++ b/spring-expression/src/main/java/org/springframework/expression/spel/ast/ConstructorReference.java @@ -136,37 +136,44 @@ public class ConstructorReference extends SpelNodeImpl { ConstructorExecutor executorToUse = this.cachedExecutor; if (executorToUse != null) { - try { - return executorToUse.execute(state.getEvaluationContext(), arguments); - } - catch (AccessException ex) { - // Two reasons this can occur: - // 1. the constructor invoked actually threw a real exception - // 2. the constructor invoked was not passed the arguments it expected and has become 'stale' - - // In the first case we should not retry, in the second case we should see if there is a - // better suited constructor. - - // To determine which situation it is, the AccessException will contain a cause. - // If the cause is an InvocationTargetException, a user exception was thrown inside the constructor. - // Otherwise, the constructor could not be invoked. - if (ex.getCause() instanceof InvocationTargetException cause) { - // User exception was the root cause - exit now - Throwable rootCause = cause.getCause(); - if (rootCause instanceof RuntimeException runtimeException) { - throw runtimeException; - } - else { - String typeName = (String) this.children[0].getValueInternal(state).getValue(); - throw new SpelEvaluationException(getStartPosition(), rootCause, - SpelMessage.CONSTRUCTOR_INVOCATION_PROBLEM, typeName, - FormatHelper.formatMethodForMessage("", argumentTypes)); - } - } - - // At this point we know it wasn't a user problem so worth a retry if a better candidate can be found + if (state.getEvaluationContext().getConstructorResolvers().isEmpty()) { + // Constructor resolution is not supported in the current context, + // so we discard the cached executor. this.cachedExecutor = null; } + else { + try { + return executorToUse.execute(state.getEvaluationContext(), arguments); + } + catch (AccessException ex) { + // Two reasons this can occur: + // 1. the constructor invoked actually threw a real exception + // 2. the constructor invoked was not passed the arguments it expected and has become 'stale' + + // In the first case we should not retry, in the second case we should see if there is a + // better suited constructor. + + // To determine which situation it is, the AccessException will contain a cause. + // If the cause is an InvocationTargetException, a user exception was thrown inside the constructor. + // Otherwise, the constructor could not be invoked. + if (ex.getCause() instanceof InvocationTargetException cause) { + // User exception was the root cause - exit now + Throwable rootCause = cause.getCause(); + if (rootCause instanceof RuntimeException runtimeException) { + throw runtimeException; + } + else { + String typeName = (String) this.children[0].getValueInternal(state).getValue(); + throw new SpelEvaluationException(getStartPosition(), rootCause, + SpelMessage.CONSTRUCTOR_INVOCATION_PROBLEM, typeName, + FormatHelper.formatMethodForMessage("", argumentTypes)); + } + } + + // At this point we know it wasn't a user problem so worth a retry if a better candidate can be found + this.cachedExecutor = null; + } + } } // Either there was no ConstructorExecutor or it no longer exists diff --git a/spring-expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java b/spring-expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java index 89dfe246372..4911acae28e 100644 --- a/spring-expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java +++ b/spring-expression/src/test/java/org/springframework/expression/spel/ConstructorInvocationTests.java @@ -19,6 +19,7 @@ package org.springframework.expression.spel; import java.util.ArrayList; import java.util.List; +import org.assertj.core.api.ThrowableTypeAssert; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -32,6 +33,7 @@ import org.springframework.expression.spel.testresources.PlaceOfBirth; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatException; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; /** * Tests invocation of constructors. @@ -142,6 +144,28 @@ class ConstructorInvocationTests extends AbstractExpressionTests { assertThat(ctx.getConstructorResolvers()).hasSize(2); } + @Test // gh-36985 + void cachedConstructorExecutorIsNotUsedWithoutRegisteredConstructorResolvers() { + ((StandardTypeLocator) super.context.getTypeLocator()).registerImport(Fruit.class.getPackageName()); + + // reflective constructor accessor is the only one by default + assertThat(super.context.getConstructorResolvers()).hasSize(1); + + String expression = "new Fruit('apple', T(java.awt.Color).RED, 'red').name"; + Expression expr = parser.parseExpression(expression); + assertThat(expr.getValue(super.context)).isEqualTo("apple"); + // Ensure the same expression can be evaluated again. + assertThat(expr.getValue(super.context)).isEqualTo("apple"); + + super.context.setConstructorResolvers(List.of()); + assertThat(super.context.getConstructorResolvers()).isEmpty(); + + // Evaluation of the same expression should no longer work. + assertThatSpelEvaluationException() + .isThrownBy(() -> expr.getValue(super.context)) + .extracting(SpelEvaluationException::getMessageCode).isEqualTo(SpelMessage.CONSTRUCTOR_NOT_FOUND); + } + @Test void varargsConstructors() { ((StandardTypeLocator) super.context.getTypeLocator()).registerImport(Fruit.class.getPackageName()); @@ -184,6 +208,10 @@ class ConstructorInvocationTests extends AbstractExpressionTests { evaluate("new String(3.0d)", "3.0", String.class); } + private ThrowableTypeAssert assertThatSpelEvaluationException() { + return assertThatExceptionOfType(SpelEvaluationException.class); + } + @SuppressWarnings("serial") static class TestException extends Exception {