Polish SpEL-related tests

spring-expression/src/test/java/org/springframework/expression/spel/SetValueTests.java

@@ -17,8 +17,11 @@
 package org.springframework.expression.spel;
 
 import java.util.Collection;
+import java.util.List;
 import java.util.Set;
 
+import org.assertj.core.api.ThrowableTypeAssert;
+import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.Test;
 
 import org.springframework.expression.EvaluationException;
@@ -35,6 +38,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
  *
  * @author Keith Donald
  * @author Andy Clement
+ * @author Sam Brannen
  */
 class SetValueTests extends AbstractExpressionTests {
 
@@ -42,178 +46,200 @@ class SetValueTests extends AbstractExpressionTests {
 
 
 	@Test
-	void testSetProperty() {
-		setValue("wonNobelPrize", true);
+	void setValueFailsWhenLeftOperandIsNotAssignable() {
+		setValueAndExpectError("3=4", "enigma");
 	}
 
 	@Test
-	void testSetNestedProperty() {
-		setValue("placeOfBirth.city", "Wien");
+	void setProperty() {
+		setValue("wonNobelPrize", true);
 	}
 
 	@Test
-	void testSetArrayElementValue() {
-		setValue("inventions[0]", "Just the telephone");
+	void setNestedProperty() {
+		setValue("placeOfBirth.city", "Wien");
 	}
 
 	@Test
-	void testErrorCase() {
-		setValueExpectError("3=4", null);
+	void setArrayElementValueToStringFromString() {
+		setValue("inventions[0]", "Just the telephone");
 	}
 
 	@Test
-	void testSetElementOfNull() {
-		setValueExpectError("new org.springframework.expression.spel.testresources.Inventor().inventions[1]",
-				SpelMessage.CANNOT_INDEX_INTO_NULL_VALUE);
+	void setArrayElementInNonexistentIndex() {
+		setValueAndExpectError(
+			"new org.springframework.expression.spel.testresources.Inventor().inventions[1]", "my invention");
 	}
 
 	@Test
-	void testSetArrayElementValueAllPrimitiveTypes() {
+	void setArrayElementValueToPrimitiveTypeFromWrapperType() {
+		// All primitive values below are auto-boxed into their wrapper types.
+		setValue("arrayContainer.booleans[1]", false);
+		setValue("arrayContainer.chars[1]", (char) 3);
+		setValue("arrayContainer.shorts[1]", (short) 3);
+		setValue("arrayContainer.bytes[1]", (byte) 3);
 		setValue("arrayContainer.ints[1]", 3);
+		setValue("arrayContainer.longs[1]", 3L);
 		setValue("arrayContainer.floats[1]", 3.0f);
-		setValue("arrayContainer.booleans[1]", false);
 		setValue("arrayContainer.doubles[1]", 3.4d);
-		setValue("arrayContainer.shorts[1]", (short)3);
-		setValue("arrayContainer.longs[1]", 3L);
-		setValue("arrayContainer.bytes[1]", (byte) 3);
-		setValue("arrayContainer.chars[1]", (char) 3);
 	}
 
 	@Test
-	void testIsWritableForInvalidExpressions_SPR10610() {
-		StandardEvaluationContext lContext = TestScenarioCreator.getTestEvaluationContext();
+	void setArrayElementValueToPrimitiveTypeFromStringResultsInError() {
+		String notPrimitiveOrWrapper = "not primitive or wrapper";
+		setValueAndExpectError("arrayContainer.booleans[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.chars[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.shorts[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.bytes[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.ints[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.longs[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.floats[1]", notPrimitiveOrWrapper);
+		setValueAndExpectError("arrayContainer.doubles[1]", notPrimitiveOrWrapper);
+	}
+
+	@Test
+	void setArrayElementValueToPrimitiveTypeFromSingleElementList() {
+		setValue("arrayContainer.booleans[1]", List.of(false), false);
+		setValue("arrayContainer.chars[1]", List.of('a'), 'a');
+		setValue("arrayContainer.shorts[1]", List.of((short) 3), (short) 3);
+		setValue("arrayContainer.bytes[1]", List.of((byte) 3), (byte) 3);
+		setValue("arrayContainer.ints[1]", List.of(42), 42);
+		setValue("arrayContainer.longs[1]", List.of(42L), 42L);
+		setValue("arrayContainer.floats[1]", List.of(42F), 42F);
+		setValue("arrayContainer.doubles[1]", List.of(42D), 42D);
+	}
+
+	@Disabled("Disabled due to bug in Indexer.setArrayElement() regarding primitive/wrapper types")
+	@Test
+	void setArrayElementValueToPrimitiveTypeFromEmptyListResultsInError() {
+		List<Object> emptyList = List.of();
+		// TODO These fail because CollectionToObjectConverter returns null.
+		// It currently throws: java.lang.IllegalStateException: Null conversion result for index [[]].
+		// Whereas, it should throw a SpelEvaluationException.
+		setValueAndExpectError("arrayContainer.booleans[1]", emptyList);
+		setValueAndExpectError("arrayContainer.chars[1]", emptyList);
+		setValueAndExpectError("arrayContainer.shorts[1]", emptyList);
+		setValueAndExpectError("arrayContainer.bytes[1]", emptyList);
+		setValueAndExpectError("arrayContainer.ints[1]", emptyList);
+		setValueAndExpectError("arrayContainer.longs[1]", emptyList);
+		setValueAndExpectError("arrayContainer.floats[1]", emptyList);
+		setValueAndExpectError("arrayContainer.doubles[1]", emptyList);
+	}
+
+	@Test  // gh-15239
+	void isWritableForInvalidExpressions() {
+		// Do NOT reuse super.context!
+		StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
 
 		// PROPERTYORFIELDREFERENCE
 		// Non-existent field (or property):
 		Expression e1 = parser.parseExpression("arrayContainer.wibble");
-		assertThat(e1.isWritable(lContext)).as("Should not be writable!").isFalse();
+		assertThat(e1.isWritable(localContext)).as("Should not be writable!").isFalse();
 
 		Expression e2 = parser.parseExpression("arrayContainer.wibble.foo");
-		assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() ->
-				e2.isWritable(lContext));
-//			org.springframework.expression.spel.SpelEvaluationException: EL1008E:(pos 15): Property or field 'wibble' cannot be found on object of type 'org.springframework.expression.spel.testresources.ArrayContainer' - maybe not public?
-//					at org.springframework.expression.spel.ast.PropertyOrFieldReference.readProperty(PropertyOrFieldReference.java:225)
+		assertThatSpelEvaluationException().isThrownBy(() -> e2.isWritable(localContext));
 
 		// VARIABLE
 		// the variable does not exist (but that is OK, we should be writable)
 		Expression e3 = parser.parseExpression("#madeup1");
-		assertThat(e3.isWritable(lContext)).as("Should be writable!").isTrue();
+		assertThat(e3.isWritable(localContext)).as("Should be writable!").isTrue();
 
 		Expression e4 = parser.parseExpression("#madeup2.bar"); // compound expression
-		assertThat(e4.isWritable(lContext)).as("Should not be writable!").isFalse();
+		assertThat(e4.isWritable(localContext)).as("Should not be writable!").isFalse();
 
 		// INDEXER
 		// non-existent indexer (wibble made up)
 		Expression e5 = parser.parseExpression("arrayContainer.wibble[99]");
-		assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() ->
-				e5.isWritable(lContext));
+		assertThatSpelEvaluationException().isThrownBy(() -> e5.isWritable(localContext));
 
 		// non-existent indexer (index via a string)
 		Expression e6 = parser.parseExpression("arrayContainer.ints['abc']");
-		assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() ->
-				e6.isWritable(lContext));
+		assertThatSpelEvaluationException().isThrownBy(() -> e6.isWritable(localContext));
 	}
 
 	@Test
-	void testSetArrayElementValueAllPrimitiveTypesErrors() {
-		// none of these sets are possible due to (expected) conversion problems
-		setValueExpectError("arrayContainer.ints[1]", "wibble");
-		setValueExpectError("arrayContainer.floats[1]", "dribble");
-		setValueExpectError("arrayContainer.booleans[1]", "nein");
-		// TODO -- this fails with NPE due to ArrayToObject converter - discuss with Andy
-		//setValueExpectError("arrayContainer.doubles[1]", new ArrayList<String>());
-		//setValueExpectError("arrayContainer.shorts[1]", new ArrayList<String>());
-		//setValueExpectError("arrayContainer.longs[1]", new ArrayList<String>());
-		setValueExpectError("arrayContainer.bytes[1]", "NaB");
-		setValueExpectError("arrayContainer.chars[1]", "NaC");
-	}
-
-	@Test
-	void testSetArrayElementNestedValue() {
+	void setArrayElementNestedValue() {
 		setValue("placesLived[0].city", "Wien");
 	}
 
 	@Test
-	void testSetListElementValue() {
+	void setListElementValue() {
 		setValue("placesLivedList[0]", new PlaceOfBirth("Wien"));
 	}
 
 	@Test
-	void testSetGenericListElementValueTypeCoercion() {
-		// TODO currently failing since setValue does a getValue and "Wien" string != PlaceOfBirth - check with andy
+	void setGenericListElementValueTypeCoercion() {
 		setValue("placesLivedList[0]", "Wien");
 	}
 
 	@Test
-	void testSetGenericListElementValueTypeCoercionOK() {
+	void setGenericListElementValueTypeCoercionOK() {
 		setValue("booleanList[0]", "true", Boolean.TRUE);
 	}
 
 	@Test
-	void testSetListElementNestedValue() {
+	void setListElementNestedValue() {
 		setValue("placesLived[0].city", "Wien");
 	}
 
 	@Test
-	void testSetArrayElementInvalidIndex() {
-		setValueExpectError("placesLived[23]", "Wien");
-		setValueExpectError("placesLivedList[23]", "Wien");
+	void setArrayElementInvalidIndex() {
+		setValueAndExpectError("placesLived[23]", "Wien");
+		setValueAndExpectError("placesLivedList[23]", "Wien");
 	}
 
 	@Test
-	void testSetMapElements() {
+	void setMapElements() {
 		setValue("testMap['montag']","lundi");
 	}
 
 	@Test
-	void testIndexingIntoUnsupportedType() {
-		setValueExpectError("'hello'[3]", 'p');
+	void indexingIntoUnsupportedType() {
+		setValueAndExpectError("'hello'[3]", 'p');
 	}
 
 	@Test
-	void testSetPropertyTypeCoercion() {
+	void setPropertyTypeCoercion() {
 		setValue("publicBoolean", "true", Boolean.TRUE);
 	}
 
 	@Test
-	void testSetPropertyTypeCoercionThroughSetter() {
+	void setPropertyTypeCoercionThroughSetter() {
 		setValue("SomeProperty", "true", Boolean.TRUE);
 	}
 
 	@Test
-	void testAssign() {
-		StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext();
+	void assign() {
+		// Do NOT reuse super.context!
+		StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
 		Expression e = parse("publicName='Andy'");
-		assertThat(e.isWritable(eContext)).isFalse();
-		assertThat(e.getValue(eContext)).isEqualTo("Andy");
+		assertThat(e.isWritable(localContext)).isFalse();
+		assertThat(e.getValue(localContext)).isEqualTo("Andy");
 	}
 
-	/*
-	 * Testing the coercion of both the keys and the values to the correct type
+	/**
+	 * Testing the coercion of both the keys and the values to the correct type.
 	 */
 	@Test
-	void testSetGenericMapElementRequiresCoercion() {
-		StandardEvaluationContext eContext = TestScenarioCreator.getTestEvaluationContext();
+	void setGenericMapElementRequiresCoercion() {
+		// Do NOT reuse super.context!
+		StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
 		Expression e = parse("mapOfStringToBoolean[42]");
-		assertThat(e.getValue(eContext)).isNull();
+		assertThat(e.getValue(localContext)).isNull();
 
 		// Key should be coerced to string representation of 42
-		e.setValue(eContext, "true");
+		e.setValue(localContext, "true");
 
 		// All keys should be strings
-		Set<?> ks = parse("mapOfStringToBoolean.keySet()").getValue(eContext, Set.class);
-		for (Object o: ks) {
-			assertThat(o.getClass()).isEqualTo(String.class);
-		}
+		Set<?> keys = parse("mapOfStringToBoolean.keySet()").getValue(localContext, Set.class);
+		assertThat(keys).allSatisfy(key -> assertThat(key).isExactlyInstanceOf(String.class));
 
 		// All values should be booleans
-		Collection<?> vs = parse("mapOfStringToBoolean.values()").getValue(eContext, Collection.class);
-		for (Object o: vs) {
-			assertThat(o.getClass()).isEqualTo(Boolean.class);
-		}
+		Collection<?> values = parse("mapOfStringToBoolean.values()").getValue(localContext, Collection.class);
+		assertThat(values).allSatisfy(key -> assertThat(key).isExactlyInstanceOf(Boolean.class));
 
 		// One final test check coercion on the key for a map lookup
-		Object o = e.getValue(eContext);
+		Object o = e.getValue(localContext);
 		assertThat(o).isEqualTo(Boolean.TRUE);
 	}
 
@@ -225,28 +251,30 @@ class SetValueTests extends AbstractExpressionTests {
 	/**
 	 * Call setValue() but expect it to fail.
 	 */
-	protected void setValueExpectError(String expression, Object value) {
+	private void setValueAndExpectError(String expression, Object value) {
 		Expression e = parser.parseExpression(expression);
 		assertThat(e).isNotNull();
 		if (DEBUG) {
 			SpelUtilities.printAbstractSyntaxTree(System.out, e);
 		}
-		StandardEvaluationContext lContext = TestScenarioCreator.getTestEvaluationContext();
-		assertThatExceptionOfType(EvaluationException.class).isThrownBy(() ->
-				e.setValue(lContext, value));
+		// Do NOT reuse super.context!
+		StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
+		assertThatSpelEvaluationException().isThrownBy(() -> e.setValue(localContext, value));
 	}
 
-	protected void setValue(String expression, Object value) {
+	private void setValue(String expression, Object value) {
+		Class<?> expectedType = value.getClass();
 		try {
 			Expression e = parser.parseExpression(expression);
 			assertThat(e).isNotNull();
 			if (DEBUG) {
 				SpelUtilities.printAbstractSyntaxTree(System.out, e);
 			}
-			StandardEvaluationContext lContext = TestScenarioCreator.getTestEvaluationContext();
-			assertThat(e.isWritable(lContext)).as("Expression is not writeable but should be").isTrue();
-			e.setValue(lContext, value);
-			assertThat(e.getValue(lContext,value.getClass())).as("Retrieved value was not equal to set value").isEqualTo(value);
+			// Do NOT reuse super.context!
+			StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
+			assertThat(e.isWritable(localContext)).as("Expression is not writeable but should be").isTrue();
+			e.setValue(localContext, value);
+			assertThat(e.getValue(localContext, expectedType)).as("Retrieved value was not equal to set value").isEqualTo(value);
 		}
 		catch (EvaluationException | ParseException ex) {
 			throw new AssertionError("Unexpected Exception: " + ex.getMessage(), ex);
@@ -254,26 +282,29 @@ class SetValueTests extends AbstractExpressionTests {
 	}
 
 	/**
-	 * For use when coercion is happening during a setValue().  The expectedValue should be
+	 * For use when coercion is happening during setValue(). The expectedValue should be
 	 * the coerced form of the value.
 	 */
-	protected void setValue(String expression, Object value, Object expectedValue) {
+	private void setValue(String expression, Object value, Object expectedValue) {
 		try {
 			Expression e = parser.parseExpression(expression);
 			assertThat(e).isNotNull();
 			if (DEBUG) {
 				SpelUtilities.printAbstractSyntaxTree(System.out, e);
 			}
-			StandardEvaluationContext lContext = TestScenarioCreator.getTestEvaluationContext();
-			assertThat(e.isWritable(lContext)).as("Expression is not writeable but should be").isTrue();
-			e.setValue(lContext, value);
-			Object a = expectedValue;
-			Object b = e.getValue(lContext);
-			assertThat(a).isEqualTo(b);
+			// Do NOT reuse super.context!
+			StandardEvaluationContext localContext = TestScenarioCreator.getTestEvaluationContext();
+			assertThat(e.isWritable(localContext)).as("Expression is not writeable but should be").isTrue();
+			e.setValue(localContext, value);
+			assertThat(expectedValue).isEqualTo(e.getValue(localContext));
 		}
 		catch (EvaluationException | ParseException ex) {
 			throw new AssertionError("Unexpected Exception: " + ex.getMessage(), ex);
 		}
 	}
 
+	private static ThrowableTypeAssert<SpelEvaluationException> assertThatSpelEvaluationException() {
+		return assertThatExceptionOfType(SpelEvaluationException.class);
+	}
+
 }

spring-expression/src/test/java/org/springframework/expression/spel/SpelDocumentationTests.java

@@ -277,6 +277,7 @@ class SpelDocumentationTests extends AbstractExpressionTests {
 		assertThat(falseValue).isFalse();
 
 		// -- AND and NOT --
+
 		expression = "isMember('Nikola Tesla') and !isMember('Mihajlo Pupin')";
 		falseValue = parser.parseExpression(expression).getValue(societyContext, Boolean.class);
 		assertThat(falseValue).isFalse();
@@ -284,15 +285,21 @@ class SpelDocumentationTests extends AbstractExpressionTests {
 
 	@Test
 	void stringOperators() {
-		// Concatenation
+		// -- Concatenation --
+
+		// evaluates to "hello world"
 		String helloWorld = parser.parseExpression("'hello' + ' ' + 'world'").getValue(String.class);
 		assertThat(helloWorld).isEqualTo("hello world");
 
-		// Character Subtraction
+		// -- Subtraction --
+
+		// evaluates to 'a'
 		char ch = parser.parseExpression("'d' - 3").getValue(char.class);
 		assertThat(ch).isEqualTo('a');
 
-		// Repeat
+		// -- Repeat --
+
+		// evaluates to "abcabc"
 		String repeated = parser.parseExpression("'abc' * 2").getValue(String.class);
 		assertThat(repeated).isEqualTo("abcabc");
 	}

spring-expression/src/test/java/org/springframework/expression/spel/TestScenarioCreator.java

@@ -28,7 +28,8 @@ import org.springframework.expression.spel.testresources.PlaceOfBirth;
 
 /**
  * Builds an evaluation context for test expressions.
- * Features of the test evaluation context are:
+ *
+ * <p>Features of the test evaluation context are:
  * <ul>
  * <li>The root context object is an Inventor instance {@link Inventor}
  * </ul>
@@ -112,9 +113,9 @@ class TestScenarioCreator {
 	/**
 	 * Create the root context object, an Inventor instance. Non-qualified property
 	 * and method references will be resolved against this context object.
-	 * @param testContext the evaluation context in which to set the root object
+	 * @param context the evaluation context in which to set the root object
 	 */
-	private static void setupRootContextObject(StandardEvaluationContext testContext) {
+	private static void setupRootContextObject(StandardEvaluationContext context) {
 		GregorianCalendar c = new GregorianCalendar();
 		c.set(1856, 7, 9);
 		Inventor tesla = new Inventor("Nikola Tesla", c.getTime(), "Serbian");
@@ -122,7 +123,7 @@ class TestScenarioCreator {
 		tesla.setInventions("Telephone repeater", "Rotating magnetic field principle",
 				"Polyphase alternating-current system", "Induction motor", "Alternating-current power transmission",
 				"Tesla coil transformer", "Wireless communication", "Radio", "Fluorescent lights");
-		testContext.setRootObject(tesla);
+		context.setRootObject(tesla);
 	}
 
 
@@ -130,10 +131,7 @@ class TestScenarioCreator {
 	// in test expressions
 
 	public static String isEven(int i) {
-		if ((i % 2) == 0) {
-			return "y";
-		}
-		return "n";
+		return ((i % 2) == 0 ? "y" : "n");
 	}
 
 	public static int[] reverseInt(int i, int j, int k) {
@@ -141,11 +139,7 @@ class TestScenarioCreator {
 	}
 
 	public static String reverseString(String input) {
-		StringBuilder backwards = new StringBuilder();
-		for (int i = 0; i < input.length(); i++) {
-			backwards.append(input.charAt(input.length() - 1 - i));
-		}
-		return backwards.toString();
+		return new StringBuilder(input).reverse().toString();
 	}
 
 	public static String varargsFunction(String... strings) {