Improve MessageSourceMessageInterpolator when using MessageSource.setUseCodeAsDefaultMessage(true) and Bean Validation attributes

[nosan]

When I was doing research for #42773, I came across two potential improvements for
MessageSourceMessageInterpolator:

• The logic around MessageSource.setUseCodeAsDefaultMessage(true) Hibernate validation messages broken in spring boot 2.6.1 when setUseCodeAsDefaultMessage set to true #28930 could be
  improved a little bit:

//Current test in MessageSourceMessageInterpolatorTests

@Test
void interpolateWhenParametersAreUnknownUsingCodeAsDefaultShouldLeaveThemUnchanged() {
	this.messageSource.setUseCodeAsDefaultMessage(true);
	this.messageSource.addMessage("top", Locale.getDefault(), "{child}+{child}");
	assertThat(this.interpolator.interpolate("{foo}{top}{bar}", this.context))
			.isEqualTo("{foo}{child}+{child}{bar}");
}

// Changed a little bit
@Test
void interpolateWhenParametersAreUnknownUsingCodeAsDefaultShouldLeaveThemUnchanged() {
	this.messageSource.setUseCodeAsDefaultMessage(true);
	this.messageSource.addMessage("top", Locale.getDefault(), "{child}+{child}");
	this.messageSource.addMessage("foo", Locale.getDefault(), "foo");
	this.messageSource.addMessage("bar", Locale.getDefault(), "bar");
	// Actually, I expected the result to be "foo{child}+{child}bar" 
	// since I explicitly specified these values in the MessageSource.
	// However, the test fails! 
	assertThat(this.interpolator.interpolate("{foo}{top}{bar}", this.context))
			.isEqualTo("foo{child}+{child}bar");
}

If MessageSource contains the searched parameter, it should be used, even if the value is identical to the parameter itself.

The solution for this is quite straightforward

private static final String DEFAULT_MESSAGE = MessageSourceMessageInterpolator.class.getName();

private String replaceParameter(String parameter, Locale locale, Set<String> visitedParameters) {
	parameter = replaceParameters(parameter, locale, visitedParameters);
	String value = this.messageSource.getMessage(parameter, null, DEFAULT_MESSAGE, locale);
	if (value == null || value.equals(DEFAULT_MESSAGE)) {
		return null;
	}
	return replaceParameters(value, locale, visitedParameters);
}

• The message lookup for EL expressions like ${validatedValue} and for Bean Validation
  attributes such as {max} could be skipped. For example:size.person.name=${validatedValue} must be between {min} and {max}. Currently, MessageSourceMessageInterpolator tries to resolve {validatedValue}, {min} and {max} via MessageSource. Clearly, the first one is an EL expression, while the latter two are Bean Validation attributes.

EL expression can be handled by:

  // EL Expression
  if (buf.charAt(i) == '$' && next(buf, i, '{')) {
  i++;
  continue;
  }

Bean validation attributes:

private boolean isBeanValidationAttribute(String parameter, Context context) {
    ConstraintDescriptor<?> constraintDescriptor = context.getConstraintDescriptor();
    Map<String, Object> attributes = constraintDescriptor.getAttributes();
    return attributes.containsKey(parameter);
}

UPDATE: I changed the initial description to make it clearer.

[nosan]

main...nosan:spring-boot:gh-42782 contains a potential fix.

[nosan]

The message lookup for EL expressions like ${validatedValue} and for Bean Validation
attributes such as {max} could be skipped. For example:size.person.name=${validatedValue} must be between {min} and {max}. Currently, MessageSourceMessageInterpolator tries to resolve {validatedValue}, {min} and {max} via MessageSource. Clearly, the first one is an EL expression, while the latter two are Bean Validation attributes.

This could introduce a breaking change. For example, if someone has a property like key=100 and they are currently using @NotBlank(message=${key}), they might expect the result to be $100. However, I don't think this being a significant issue, as such usage is quite uncommon in validation messages.

[wilkinsona]

Thanks for your patience while we found time to look at this, @nosan.

As far as I can remember, we haven't seen any issues about this not quite working correctly. As such, I think we should be cautious in what we change. My feeling is that it makes sense to fix {foo} being left as {foo} when the message is the same as its code. I think we should leave the rest as it currently is as that change feels a little riskier.

If you agree, would you like to open a PR?

[nosan]

Thanks @wilkinsona

I think we should leave the rest as it currently is as that change feels a little riskier.

Indeed, it is risky.

If you agree, would you like to open a PR?

#45212

[wilkinsona]

Thanks very much, @nosan. Closing in favor of #45212.