Address review comments.

Author: mp911de

src/main/antora/modules/ROOT/pages/value-expressions.adoc

@@ -1,8 +1,8 @@
 [[valueexpressions.fundamentals]]
 = Value Expressions Fundamentals
 
-Value Expressions a composition of {spring-framework-docs}/core/expressions.html[Spring Expression Language (SpEL)] and {spring-framework-docs}/core/beans/environment.html#beans-placeholder-resolution-in-statements[Property Placeholder Resolution].
-Value Expressions a combine powerful evaluation of programmatic expressions with the simplicity to resort to property-placeholder resolution to obtain values from the `Environment` such as configuration properties.
+Value Expressions are a combination of {spring-framework-docs}/core/expressions.html[Spring Expression Language (SpEL)] and {spring-framework-docs}/core/beans/environment.html#beans-placeholder-resolution-in-statements[Property Placeholder Resolution].
+They combine powerful evaluation of programmatic expressions with the simplicity to resort to property-placeholder resolution to obtain values from the `Environment` such as configuration properties.
 
 Expressions are expected to be defined by a trusted input such as an annotation value and not to be determined from user input.
 
@@ -35,12 +35,12 @@ orders-${tenant-config.suffix}                                 <4>
 <1> Value Expression using a single SpEL Expression.
 <2> Value Expression using a static SpEL Expression evaluating to `2-hello-world`.
 <3> Value Expression using a single Property Placeholder.
-<4> Composite expression comprised from the literal `orders-` and the Property Placeholder `${tenant-config.suffix}`.
+<4> Composite expression comprised of the literal `orders-` and the Property Placeholder `${tenant-config.suffix}`.
 <5> Composite expression using SpEL, Property Placeholders and literals.
 ====
 
-Using value expressions introduces a lot of flexibility to your code.
-Keep in mind that using value expressions requires evaluation of the expression on each usage.
+NOTE: Using value expressions introduces a lot of flexibility to your code.
+Doing so requires evaluation of the expression on each usage and, therefore, value expression evaluation has an impact on the performance profile.
 
 [[valueexpressions.api]]
 == Parsing and Evaluation

src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java

@@ -35,7 +35,10 @@
 import org.apache.commons.logging.LogFactory;
 import org.springframework.beans.BeanUtils;
 import org.springframework.beans.BeansException;
+import org.springframework.beans.factory.BeanFactory;
+import org.springframework.beans.factory.BeanFactoryAware;
 import org.springframework.beans.factory.InitializingBean;
+import org.springframework.beans.factory.ListableBeanFactory;
 import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.context.ApplicationEventPublisher;
@@ -91,8 +94,8 @@
  * @author Christoph Strobl
  */
 public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?, P>, P extends PersistentProperty<P>>
-		implements MappingContext<E, P>, ApplicationEventPublisherAware, ApplicationContextAware, InitializingBean,
-		EnvironmentAware {
+		implements MappingContext<E, P>, ApplicationEventPublisherAware, ApplicationContextAware, BeanFactoryAware,
+		EnvironmentAware, InitializingBean {
 
 	private static final Log LOGGER = LogFactory.getLog(MappingContext.class);
 
@@ -132,18 +135,44 @@ public void setApplicationEventPublisher(ApplicationEventPublisher applicationEv
 		this.applicationEventPublisher = applicationEventPublisher;
 	}
 
+	/**
+	 * Sets the application context. Sets also {@link ApplicationEventPublisher} and {@link Environment} if these weren't
+	 * already set.
+	 *
+	 * @param applicationContext the ApplicationContext object to be used by this object.
+	 * @throws BeansException
+	 */
 	@Override
 	public void setApplicationContext(ApplicationContext applicationContext) throws BeansException {
 
-		this.evaluationContextProvider = new ExtensionAwareEvaluationContextProvider(applicationContext);
+		setBeanFactory(applicationContext);
 
-		if (applicationEventPublisher == null) {
+		if (this.applicationEventPublisher == null) {
 			this.applicationEventPublisher = applicationContext;
 		}
 
-		this.environment = applicationContext.getEnvironment();
+		if (this.environment == null) {
+			this.environment = applicationContext.getEnvironment();
+		}
+	}
+
+	/**
+	 * @param beanFactory owning BeanFactory.
+	 * @throws BeansException
+	 * @since 3.3
+	 */
+	@Override
+	public void setBeanFactory(BeanFactory beanFactory) throws BeansException {
+
+		if (beanFactory instanceof ListableBeanFactory lbf) {
+			this.evaluationContextProvider = new ExtensionAwareEvaluationContextProvider(lbf);
+		}
 	}
 
+	/**
+	 * @param environment the {@code Environment} that this component runs in.
+	 * @since 3.3
+	 */
 	@Override
 	public void setEnvironment(Environment environment) {
 		this.environment = environment;
@@ -210,6 +239,7 @@ public Collection<E> getPersistentEntities() {
 		}
 	}
 
+	@Override
 	@Nullable
 	public E getPersistentEntity(Class<?> type) {
 		return getPersistentEntity(TypeInformation.of(type));

src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java

@@ -229,6 +229,10 @@ public void setEvaluationContextProvider(EvaluationContextProvider provider) {
 		this.evaluationContextProvider = provider;
 	}
 
+	/**
+	 * @param environment the {@code Environment} that this component runs in.
+	 * @since 3.3
+	 */
 	@Override
 	public void setEnvironment(Environment environment) {
 		this.environment = environment;

src/main/java/org/springframework/data/spel/DefaultValueExpressionParser.java

@@ -82,7 +82,8 @@ private CompositeValueExpression parseComposite(String expression, int placerhol
 			int endIndex = findPlaceholderEndIndex(expression, startIndex);
 			if (endIndex != -1) {
 
-				String part = expression.substring(startIndex, endIndex + 1);
+				int afterClosingParenthesisIndex = endIndex + 1;
+				String part = expression.substring(startIndex, afterClosingParenthesisIndex);
 
 				if (part.startsWith(PLACEHOLDER_PREFIX)) {
 					expressions.add(createPlaceholder(part));
@@ -96,9 +97,11 @@ private CompositeValueExpression parseComposite(String expression, int placerhol
 				startIndex = getStartIndex(placerholderIndex, expressionIndex);
 
 				if (startIndex == -1) {
-					expressions.add(new LiteralValueExpression(expression.substring(endIndex + 1)));
+					// no next expression but we're capturing everything after the expression as literal.
+					expressions.add(new LiteralValueExpression(expression.substring(afterClosingParenthesisIndex)));
 				} else {
-					expressions.add(new LiteralValueExpression(expression.substring(endIndex + 1, startIndex)));
+					// capture literal after the expression ends and before the next starts.
+					expressions.add(new LiteralValueExpression(expression.substring(afterClosingParenthesisIndex, startIndex)));
 				}
 			} else {
 				expressions.add(new LiteralValueExpression(expression.substring(startIndex)));
@@ -127,6 +130,7 @@ private static int findPlaceholderEndIndex(CharSequence buf, int startIndex) {
 
 		int index = startIndex + PLACEHOLDER_PREFIX_LENGTH;
 		char quotationChar = 0;
+		char nestingLevel = 0;
 		boolean skipEscape = false;
 
 		while (index < buf.length()) {
@@ -149,11 +153,18 @@ private static int findPlaceholderEndIndex(CharSequence buf, int startIndex) {
 				quotationChar = 0;
 			}
 
-			if (!skipEscape && quotationChar == 0 && c == SUFFIX) {
-				return index;
-			} else {
-				index++;
+			if (!skipEscape && quotationChar == 0) {
+
+				if (nestingLevel != 0 && c == SUFFIX) {
+					nestingLevel--;
+				} else if (c == '{') {
+					nestingLevel++;
+				} else if (nestingLevel == 0 && c == SUFFIX) {
+					return index;
+				}
 			}
+
+			index++;
 		}
 
 		return -1;

src/test/java/org/springframework/data/util/ValueEvaluationUnitTests.java renamed to src/test/java/org/springframework/data/spel/ValueEvaluationUnitTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2023 the original author or authors.
+ * Copyright 2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-package org.springframework.data.util;
+package org.springframework.data.spel;
 
 import static org.assertj.core.api.Assertions.*;
 
@@ -25,10 +25,9 @@
 import org.springframework.core.env.Environment;
 import org.springframework.core.env.MapPropertySource;
 import org.springframework.core.env.StandardEnvironment;
-import org.springframework.data.spel.ValueEvaluationContext;
-import org.springframework.data.spel.ValueExpressionParser;
-import org.springframework.data.spel.ValueParserConfiguration;
 import org.springframework.expression.EvaluationContext;
+import org.springframework.expression.EvaluationException;
+import org.springframework.expression.ParseException;
 import org.springframework.expression.spel.standard.SpelExpressionParser;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
 
@@ -45,7 +44,7 @@ public class ValueEvaluationUnitTests {
 	@BeforeEach
 	void setUp() {
 
-		MapPropertySource propertySource = new MapPropertySource("map", Map.of("key.one", "value"));
+		MapPropertySource propertySource = new MapPropertySource("map", Map.of("env.key.one", "value", "env.key.two", 42L));
 		StandardEnvironment environment = new StandardEnvironment();
 		environment.getPropertySources().addFirst(propertySource);
 
@@ -58,29 +57,34 @@ public Environment getEnvironment() {
 			@Override
 			public EvaluationContext getEvaluationContext() {
 				StandardEvaluationContext context = new StandardEvaluationContext();
-				context.setVariable("foo", "bar");
+				context.setVariable("contextVar", "contextVal");
 
 				return context;
 			}
 		};
 	}
 
-	@Test
+	@Test // GH-2369
 	void shouldParseAndEvaluateExpressions() {
 
 		assertThat(eval("foo")).isEqualTo("foo");
-		assertThat(eval("${key.one}")).isEqualTo("value");
-		assertThat(eval("${key.one")).isEqualTo("${key.one");
-		assertThat(eval("#{'foo'}-${key.one")).isEqualTo("foo-${key.one");
+		assertThat(eval("${env.key.one}")).isEqualTo("value");
+		assertThat(eval("${env.key.two}")).isEqualTo("42");
+		assertThat(eval("${env.key.one")).isEqualTo("${env.key.one");
+		assertThat(eval("#{'foo'}-${env.key.one")).isEqualTo("foo-${env.key.one");
 		assertThat(eval("#{'foo'}-")).isEqualTo("foo-");
 		assertThat(eval("#{'fo\"o'}-")).isEqualTo("fo\"o-");
 		assertThat(eval("#{\"foo\"}-")).isEqualTo("foo-");
 		assertThat(eval("#{\"fo'o\"}-")).isEqualTo("fo'o-");
-		assertThat(eval("${key.one}-")).isEqualTo("value-");
-		assertThat(eval("#{#foo}")).isEqualTo("bar");
+		assertThat(eval("${env.key.one}-")).isEqualTo("value-");
+		assertThat(eval("#{#contextVar}")).isEqualTo("contextVal");
+		assertThat(eval("${env.does.not.exist:some-default}-")).isEqualTo("some-default-");
+
+		assertThatExceptionOfType(EvaluationException.class).isThrownBy(() -> eval("${env.does.not.exist}-"))
+				.withMessageContaining("Could not resolve placeholder 'env.does.not.exist'");
 	}
 
-	@Test
+	@Test // GH-2369
 	void shouldParseLiteral() {
 
 		ValueParserConfiguration parserContext = () -> new SpelExpressionParser();
@@ -92,19 +96,22 @@ void shouldParseLiteral() {
 		assertThat(parser.parse("${key.one}").isLiteral()).isFalse();
 	}
 
-	@Test
+	@Test // GH-2369
 	void shouldParseCompoundExpression() {
 
 		assertThat(eval("#{'foo'}-${key.one")).isEqualTo("foo-${key.one");
 		assertThat(eval("#{'foo'}-")).isEqualTo("foo-");
 		assertThat(eval("#{'fo\"o'}-")).isEqualTo("fo\"o-");
 		assertThat(eval("#{\"foo\"}-")).isEqualTo("foo-");
 		assertThat(eval("#{\"fo'o\"}-")).isEqualTo("fo'o-");
-		assertThat(eval("${key.one}-")).isEqualTo("value-");
-		assertThat(eval("${key.one}-and-some-#{'foo'}-#{#foo}")).isEqualTo("value-and-some-foo-bar");
+		assertThat(eval("${env.key.one}-")).isEqualTo("value-");
+		assertThat(eval("${env.key.one}-and-some-#{'foo'}-#{#contextVar}")).isEqualTo("value-and-some-foo-contextVal");
+
+		assertThatExceptionOfType(ParseException.class).isThrownBy(() -> eval("#{#foo - ${numeric.key}}"))
+				.withMessageContaining("[#foo - ${numeric.key}]");
 	}
 
-	@Test
+	@Test // GH-2369
 	void shouldParseQuoted() {
 
 		assertThat(eval("#{(1+1) + '-foo'+\"-bar\"}")).isEqualTo("2-foo-bar");