StringIndexOutOfBoundsException in AbstractErrors for class-level JSR-303 validator [SPR-11374]

[spring-projects-issues]

Michael Simons opened SPR-11374 and commented

The performance improvement of #15928 is nice, but has one flaw. If a field name is "" (which is the case of a class level validator like the following (http://stackoverflow.com/a/2155576)

import javax.validation.ConstraintValidator;
import javax.validation.ConstraintValidatorContext;

import org.apache.commons.beanutils.PropertyUtils;

public class FieldsMatchValidator implements ConstraintValidator<FieldsMatch, Object> {
	private String firstFieldName;
	private String secondFieldName;

	@Override
	public void initialize(final FieldsMatch constraintAnnotation) {
		firstFieldName = constraintAnnotation.first();
		secondFieldName = constraintAnnotation.second();
	}

	@Override
	public boolean isValid(final Object value, final ConstraintValidatorContext context) {
		boolean rv = false;
		try {
			final Object firstObj = PropertyUtils.getProperty(value, firstFieldName);
			final Object secondObj = PropertyUtils.getProperty(value, secondFieldName);

			rv = firstObj == null && secondObj == null || firstObj != null && firstObj.equals(secondObj);
		}
		catch (final Exception ignore) {
		}
		return rv;
	}
}

added to a class like so

@FieldsMatch(first = "newPassword", second = "newPasswordConfirmation")
public class UpdateUserPasswordCmd {
	@NotBlank
	private String oldPassword;
	
	@NotBlank	
	@Length(min=6, max=40)	
	private String newPassword;
	
	@NotBlank
	@Length(min=6, max=40)
	private String newPasswordConfirmation;
...

it breaks with:

java.lang.StringIndexOutOfBoundsException: String index out of range: -1
	at java.lang.String.charAt(String.java:658)
	at org.springframework.validation.AbstractErrors.isMatchingFieldError(AbstractErrors.java:214)
	at org.springframework.validation.AbstractBindingResult.getFieldError(AbstractBindingResult.java:207)
	at org.springframework.validation.beanvalidation.SpringValidatorAdapter.processConstraintViolations(SpringValidatorAdapter.java:115)
	at org.springframework.validation.beanvalidation.SpringValidatorAdapter.validate(SpringValidatorAdapter.java:102)
	at org.springframework.validation.DataBinder.validate(DataBinder.java:772)
	at org.springframework.web.method.annotation.ModelAttributeMethodProcessor.validateIfApplicable(ModelAttributeMethodProcessor.java:159)
	at org.springframework.web.method.annotation.ModelAttributeMethodProcessor.resolveArgument(ModelAttributeMethodProcessor.java:107)
	at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:77)
	at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:162)
	at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:123)
	at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:104)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandleMethod(RequestMappingHandlerAdapter.java:745)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:686)
	at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:80)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:925)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:856)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:953)
	at org.springframework.web.servlet.FrameworkServlet.doPost(FrameworkServlet.java:855)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:647)
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:829)

I have the impression it happens only in combination with other validations and with the default violations in place.

I'm adding now

                     context			
.buildConstraintViolationWithTemplate(context.getDefaultConstraintMessageTemplate())
                                                                                                                                    .addNode(secondFieldName)				
                                                                                                                                    .addConstraintViolation()				
                                                                                                                                                                                                                                                                                                                                                                                        .disableDefaultConstraintViolation();								
;

to my validator which is better on it's one, but i think the slightly different semantics should be fixed.

Take care and thank you.

---

Affects: 3.2.7, 4.0.1

Issue Links:

• Optimize lookup of field errors in AbstractBindingResult [SPR-11304] #15928 Optimize lookup of field errors in AbstractBindingResult

Backported to: 3.2.8

[spring-projects-issues]

Juergen Hoeller commented

I'm not entirely sure why this is being called with an empty field name at the top level (i.e. with no nested path set either). In any case, I have restored the original empty field name behavior for 4.0.2 and 3.2.8 now.

Juergen

[spring-projects-issues]

Michael Simons commented

Thanks, Jürgen.

You're right, i'm wondering too. The custom validator is only in place at class level.

FYI:

Validation is called in a @Controller

@RequestMapping(value="/updatePassword", method=RequestMethod.POST)
	public String updatePassword (
		final @AuthenticationPrincipal UserDetailsImpl userDetails,
		final @ModelAttribute(value="updatePasswordCmd") @Valid UpdateUserPasswordCmd updatePasswordCmd,			
		final BindingResult bindingResult,
		final HttpServletRequest request, 
		final Locale locale,
		final Model model,
		final RedirectAttributes redirectAttributes
	) {

happens with Spring 3.2.7/ Validator 4.3.1.Final and Spring 4.0.1 / Validator 5.0.3.Final

[spring-projects-issues]

Juergen Hoeller commented

It's interesting that the empty field name case only happens with default constraint validation being active there...

We'll simply have to define an empty property path as valid in a JSR-303 ConstraintValidation that we're introspecting. This simply wasn't properly defined before. The algorithm never found a FieldError for such a getFieldError("") call anyway, always considering the violation as to-be-registered. I suppose it should actually check for a top-level ObjectError in such a case, in order to not register an additional error for a pre-registered one, just like for regular fields. I'll have a look at that case.

For the time being, the basic behavior is restored for the latest 4.0.2 and 3.2.8 snapshots, as a remedy for the immediate issue.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

Since the check for an existing FieldError was primarily meant for binding failures, SpringValidatorAdapter's algorithm seems to be alright as-is: Global ObjectErrors can't result from binding failures to begin with. So simply accepting empty field names for getFieldError checks (again) seems to be all that's needed here.

I've revised the fix in the meantime, making field error access with wildcards more efficient than before. String's substring is an expensive operation after some recent changes in the JDK 7 and 8 lines where it always creates a copy of the underlying content, so it's better to use regionMatches instead of startsWith + substring.

If there is anything else that you run into here - with any combination of constraints processed by any version of Hibernate Validator -, please let me know.

Juergen

[spring-projects-issues]

Michael Simons commented

Thank you very much, this kind of support is highly appreciated and great work.

I'll keep this in mind if we run into trouble again.