spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.19k stars 40.69k forks source link

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

Open nosan opened 3 weeks ago

nosan commented 3 weeks ago

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


//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);
}

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 commented 3 weeks ago

https://github.com/spring-projects/spring-boot/compare/main...nosan:spring-boot:gh-42782 contains a potential fix.

nosan commented 3 weeks ago

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.