Revised isInstanceOf/isAssignable message concatenation

Issue: SPR-15196

spring-core/src/main/java/org/springframework/util/Assert.java

@@ -56,6 +56,51 @@ import java.util.function.Supplier;
  */
 public abstract class Assert {
 
+	/**
+	 * Assert a boolean expression, throwing an {@code IllegalStateException}
+	 * if the expression evaluates to {@code false}.
+	 * <p>Call {@link #isTrue} if you wish to throw an {@code IllegalArgumentException}
+	 * on an assertion failure.
+	 * <pre class="code">
+	 * Assert.state(id == null,
+	 *     () -&gt; "ID for " + entity.getName() + " must not already be initialized");
+	 * </pre>
+	 * @param expression a boolean expression
+	 * @param messageSupplier a supplier for the exception message to use if the
+	 * assertion fails
+	 * @throws IllegalStateException if {@code expression} is {@code false}
+	 * @since 5.0
+	 */
+	public static void state(boolean expression, Supplier<String> messageSupplier) {
+		if (!expression) {
+			throw new IllegalStateException(nullSafeGet(messageSupplier));
+		}
+	}
+
+	/**
+	 * Assert a boolean expression, throwing an {@code IllegalStateException}
+	 * if the expression evaluates to {@code false}.
+	 * <p>Call {@link #isTrue} if you wish to throw an {@code IllegalArgumentException}
+	 * on an assertion failure.
+	 * <pre class="code">Assert.state(id == null, "The id property must not already be initialized");</pre>
+	 * @param expression a boolean expression
+	 * @param message the exception message to use if the assertion fails
+	 * @throws IllegalStateException if {@code expression} is {@code false}
+	 */
+	public static void state(boolean expression, String message) {
+		if (!expression) {
+			throw new IllegalStateException(message);
+		}
+	}
+
+	/**
+	 * @deprecated as of 4.3.7, in favor of {@link #state(boolean, String)}
+	 */
+	@Deprecated
+	public static void state(boolean expression) {
+		state(expression, "[Assertion failed] - this state invariant must be true");
+	}
+
 	/**
 	 * Assert a boolean expression, throwing an {@code IllegalArgumentException}
 	 * if the expression evaluates to {@code false}.
@@ -480,11 +525,8 @@ public abstract class Assert {
 	 * @param type the type to check against
 	 * @param obj the object to check
 	 * @param messageSupplier a supplier for the exception message to use if the
-	 * assertion fails; the message will be prepended to the message generated
+	 * assertion fails. See {@link #isInstanceOf(Class, Object, String)} for details.
-	 * by this method in order to provide further context. It should normally end
-	 * in ":" or "." so that the generated message looks OK when appended to it.
 	 * @throws IllegalArgumentException if the object is not an instance of type
-	 * @see Class#isInstance
 	 * @since 5.0
 	 */
 	public static void isInstanceOf(Class<?> type, Object obj, Supplier<String> messageSupplier) {
@@ -496,14 +538,15 @@ public abstract class Assert {
 
 	/**
 	 * Assert that the provided object is an instance of the provided class.
-	 * <pre class="code">Assert.instanceOf(Foo.class, foo, "Processing Foo:");</pre>
+	 * <pre class="code">Assert.instanceOf(Foo.class, foo, "Foo expected");</pre>
 	 * @param type the type to check against
 	 * @param obj the object to check
-	 * @param message a message which will be prepended to the message generated
+	 * @param message a message which will be prepended to provide further context.
-	 * by this method in order to provide further context. It should normally end
+	 * If it is empty or ends in ":" or ";" or "," or ".", a full exception message
-	 * in ":" or "." so that the generated message looks OK when appended to it.
+	 * will be appended. If it ends in a space, the name of the offending object's
+	 * type will be appended. In any other case, a ":" with a space and the name
+	 * of the offending object's type will be appended.
 	 * @throws IllegalArgumentException if the object is not an instance of type
-	 * @see Class#isInstance
 	 */
 	public static void isInstanceOf(Class<?> type, Object obj, String message) {
 		notNull(type, "Type to check against must not be null");
@@ -518,29 +561,20 @@ public abstract class Assert {
 	 * @param type the type to check against
 	 * @param obj the object to check
 	 * @throws IllegalArgumentException if the object is not an instance of type
-	 * @see Class#isInstance
 	 */
 	public static void isInstanceOf(Class<?> type, Object obj) {
 		isInstanceOf(type, obj, "");
 	}
 
-	private static void instanceCheckFailed(Class<?> type, Object obj, String message) {
-		String className = (obj != null ? obj.getClass().getName() : "null");
-		throw new IllegalArgumentException(StringUtils.hasLength(message) ? message + ": " + className :
-				"Object of class [" + className + "] must be an instance of " + type);
-	}
-
 	/**
 	 * Assert that {@code superType.isAssignableFrom(subType)} is {@code true}.
 	 * <pre class="code">
-	 * Assert.isAssignable(Number.class, myClass, () -&gt; "Processing " + myClass.getSimpleName() + ":");
+	 * Assert.isAssignable(Number.class, myClass, () -&gt; "Processing " + myAttributeName + ":");
 	 * </pre>
 	 * @param superType the super type to check against
 	 * @param subType the sub type to check
 	 * @param messageSupplier a supplier for the exception message to use if the
-	 * assertion fails; the message will be prepended to the message generated
+	 * assertion fails. See {@link #isAssignable(Class, Class, String)} for details.
-	 * by this method in order to provide further context. It should normally end
-	 * in ":" or "." so that the generated message looks OK when appended to it.
 	 * @throws IllegalArgumentException if the classes are not assignable
 	 * @since 5.0
 	 */
@@ -553,12 +587,14 @@ public abstract class Assert {
 
 	/**
 	 * Assert that {@code superType.isAssignableFrom(subType)} is {@code true}.
-	 * <pre class="code">Assert.isAssignable(Number.class, myClass);</pre>
+	 * <pre class="code">Assert.isAssignable(Number.class, myClass, "Number expected");</pre>
 	 * @param superType the super type to check against
 	 * @param subType the sub type to check
-	 * @param message a message which will be prepended to the message generated
+	 * @param message a message which will be prepended to provide further context.
-	 * by this method in order to provide further context. It should normally end
+	 * If it is empty or ends in ":" or ";" or "," or ".", a full exception message
-	 * in ":" or "." so that the generated message looks OK when appended to it.
+	 * will be appended. If it ends in a space, the name of the offending sub type
+	 * will be appended. In any other case, a ":" with a space and the name of the
+	 * offending sub type will be appended.
 	 * @throws IllegalArgumentException if the classes are not assignable
 	 */
 	public static void isAssignable(Class<?> superType, Class<?> subType, String message) {
@@ -579,54 +615,50 @@ public abstract class Assert {
 		isAssignable(superType, subType, "");
 	}
 
-	private static void assignableCheckFailed(Class<?> superType, Class<?> subType, String message) {
-		throw new IllegalArgumentException(StringUtils.hasLength(message) ? message + ": " + subType :
-				subType + " is not assignable to " + superType);
-	}
 
-	/**
+	private static void instanceCheckFailed(Class<?> type, Object obj, String msg) {
-	 * Assert a boolean expression, throwing an {@code IllegalStateException}
+		String className = (obj != null ? obj.getClass().getName() : "null");
-	 * if the expression evaluates to {@code false}.
+		String result = "";
-	 * <p>Call {@link #isTrue} if you wish to throw an {@code IllegalArgumentException}
+		boolean defaultMessage = true;
-	 * on an assertion failure.
+		if (StringUtils.hasLength(msg)) {
-	 * <pre class="code">
+			if (endsWithSeparator(msg)) {
-	 * Assert.state(id == null,
+				result = msg + " ";
-	 *     () -&gt; "ID for " + entity.getName() + " must not already be initialized");
+			}
-	 * </pre>
+			else {
-	 * @param expression a boolean expression
+				result = messageWithTypeName(msg, className);
-	 * @param messageSupplier a supplier for the exception message to use if the
+				defaultMessage = false;
-	 * assertion fails
+			}
-	 * @throws IllegalStateException if {@code expression} is {@code false}
-	 * @since 5.0
-	 */
-	public static void state(boolean expression, Supplier<String> messageSupplier) {
-		if (!expression) {
-			throw new IllegalStateException(nullSafeGet(messageSupplier));
 		}
-	}
+		if (defaultMessage) {
-
+			result = result + ("Object of class [" + className + "] must be an instance of " + type);
-	/**
-	 * Assert a boolean expression, throwing an {@code IllegalStateException}
-	 * if the expression evaluates to {@code false}.
-	 * <p>Call {@link #isTrue} if you wish to throw an {@code IllegalArgumentException}
-	 * on an assertion failure.
-	 * <pre class="code">Assert.state(id == null, "The id property must not already be initialized");</pre>
-	 * @param expression a boolean expression
-	 * @param message the exception message to use if the assertion fails
-	 * @throws IllegalStateException if {@code expression} is {@code false}
-	 */
-	public static void state(boolean expression, String message) {
-		if (!expression) {
-			throw new IllegalStateException(message);
 		}
+		throw new IllegalArgumentException(result);
 	}
 
-	/**
+	private static void assignableCheckFailed(Class<?> superType, Class<?> subType, String msg) {
-	 * @deprecated as of 4.3.7, in favor of {@link #state(boolean, String)}
+		String result = "";
-	 */
+		boolean defaultMessage = true;
-	@Deprecated
+		if (StringUtils.hasLength(msg)) {
-	public static void state(boolean expression) {
+			if (endsWithSeparator(msg)) {
-		state(expression, "[Assertion failed] - this state invariant must be true");
+				result = msg + " ";
+			}
+			else {
+				result = messageWithTypeName(msg, subType);
+				defaultMessage = false;
+			}
+		}
+		if (defaultMessage) {
+			result = result + (subType + " is not assignable to " + superType);
+		}
+		throw new IllegalArgumentException(result);
+	}
+
+	private static boolean endsWithSeparator(String msg) {
+		return (msg.endsWith(":") || msg.endsWith(";") || msg.endsWith(",") || msg.endsWith("."));
+	}
+
+	private static String messageWithTypeName(String msg, Object typeName) {
+		return msg + (msg.endsWith(" ") ? "" : ": ") + typeName;
 	}
 
 	private static String nullSafeGet(Supplier<String> messageSupplier) {

spring-core/src/test/java/org/springframework/util/AssertTests.java

@@ -497,6 +497,21 @@ public class AssertTests {
 		Assert.isInstanceOf(String.class, 42L, "Custom message");
 	}
 
+	@Test
+	public void isInstanceOfWithTypeMismatchAndCustomMessageWithSeparator() {
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage(
+				"Custom message: Object of class [java.lang.Long] must be an instance of class java.lang.String");
+		Assert.isInstanceOf(String.class, 42L, "Custom message:");
+	}
+
+	@Test
+	public void isInstanceOfWithTypeMismatchAndCustomMessageWithSpace() {
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Custom message for java.lang.Long");
+		Assert.isInstanceOf(String.class, 42L, "Custom message for ");
+	}
+
 	@Test
 	public void isInstanceOfWithMessageSupplier() {
 		Assert.isInstanceOf(String.class, "foo", () -> "enigma");
@@ -559,8 +574,22 @@ public class AssertTests {
 	@Test
 	public void isAssignableWithTypeMismatchAndCustomMessage() {
 		thrown.expect(IllegalArgumentException.class);
-		thrown.expectMessage("enigma: class java.lang.Integer");
+		thrown.expectMessage("Custom message: class java.lang.Integer");
-		Assert.isAssignable(String.class, Integer.class, "enigma");
+		Assert.isAssignable(String.class, Integer.class, "Custom message");
+	}
+
+	@Test
+	public void isAssignableWithTypeMismatchAndCustomMessageWithSeparator() {
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Custom message: class java.lang.Integer is not assignable to class java.lang.String");
+		Assert.isAssignable(String.class, Integer.class, "Custom message:");
+	}
+
+	@Test
+	public void isAssignableWithTypeMismatchAndCustomMessageWithSpace() {
+		thrown.expect(IllegalArgumentException.class);
+		thrown.expectMessage("Custom message for class java.lang.Integer");
+		Assert.isAssignable(String.class, Integer.class, "Custom message for ");
 	}
 
 	@Test