Validation issue in a Spring MVC controller using JSR 303 in order to deal with empty values

Question

I have a problem with Spring MVC which is as follows: I use JSR 303 validation in order to make sure properties of a bean (see PasswordInfo below) are neither empty nor null.

I am also checking a business logic rule i.e. that two passwords are identical.

The issue is that if one of the string fields (currentPassword and newPassword) is empty, it is still passed to the service layer by the controller in order to check the business logic rule and of course an IllegalArgumentException is raised!

Here is the PasswordInfo bean:

@RooEquals
@RooJavaBean
public class PasswordInfo {

    @NotNull(groups = { ValidationGroups.PasswordModification.class })
    @NotEmpty(groups = { ValidationGroups.PasswordModification.class })
    private String currentPassword;

    @NotNull(groups = { ValidationGroups.PasswordModification.class, ValidationGroups.PasswordReset.class })
    @NotEmpty(groups = { ValidationGroups.PasswordModification.class, ValidationGroups.PasswordReset.class })       
    @Size(min = 6, groups = { ValidationGroups.PasswordModification.class, ValidationGroups.PasswordReset.class })
    private String newPassword;
...

Here is the relevant controller method:

@RequestMapping(value = "/preference/password", method = RequestMethod.POST, produces = "text/html")
    public String modifyPassword(@ModelAttribute @Validated({ ValidationGroups.PasswordModification.class }) PasswordInfo passwordInfo,
            BindingResult bindingResult, Model model, @CurrentMember Member member) {
        if (!preferenceService.passwordsMatch(member.getPassword(), passwordInfo.getCurrentPassword())) {
            bindingResult.rejectValue("currentPassword", "controller.preference.wrong_current_password");
        }

        if (bindingResult.hasErrors()) {
            model.addAttribute("passwordInfo", passwordInfo);
            return "preference/password";
        }

        preferenceService.modifyPassword(member, passwordInfo.getNewPassword());
        return "redirect:/preference/password";
    }

Here is the relevant service-layer method:

@Override
    public boolean passwordsMatch(String encrypted, String plain) {
        if (encrypted == null || encrypted.isEmpty() || plain == null || plain.isEmpty()) {
            throw new IllegalArgumentException("One argument is null or empty");
        }
        return passwordEncoder.matches(plain, encrypted);
    }

My concern is to avoid placing another bindingResults.hasErrors block such as:

if (bindingResult.hasErrors()) {
        model.addAttribute("passwordInfo", passwordInfo);
        return "preference/password";
    }

...before the business logic check in order to avoid repeating myself...

Can anyone please suggest a clean solution to this problem?

score 2 · Answer 1 · answered Oct 08 '13 at 01:27

2

The issue here is that since you are providing a BindingResult as a parameter, Spring MVC is expecting you to handle your validation issues in the controller method. It will not outright reject the request (i.e. prevent the call to your controller method and throw an exception.) You probably just need to restructure your method to get the desired result:

@RequestMapping(value = "/preference/password", method = RequestMethod.POST, produces = "text/html")
public String modifyPassword(@ModelAttribute @Validated({ ValidationGroups.PasswordModification.class }) PasswordInfo passwordInfo,
                             BindingResult bindingResult, Model model, @CurrentMember Member member) {
    String view = "preference/password";
    if (!bindingResult.hasErrors()) {
        if (preferenceService.passwordsMatch(member.getPassword(), passwordInfo.getCurrentPassword())) {
            preferenceService.modifyPassword(member, passwordInfo.getNewPassword());
            view = "redirect:/preference/password";
        } else {
            bindingResult.rejectValue("currentPassword", "controller.preference.wrong_current_password");
        }
    }
    // NOTE: I have removed the call to model.addAttribute("passwordInfo", passwordInfo) because it should already exist in the model, no?
    return view;
}

answered Oct 08 '13 at 01:27

MattSenter

3,060
18
18

thanks. The code you provided is a pretty good idea actually. I am really wondering what is the best (and established) practice with Spring MVC though: use code similar to yours (which works great but is a slightly more difficult to read and test - nested ifs) or go ahead and relax the service layer checks: keep the "null" checks but remove the "isEmpty" checks... – balteo Oct 08 '13 at 02:29
What is your opinion on my last comment? – balteo Oct 08 '13 at 02:30
I don't think there is really a "best" or "established" way to structure your logic...at least not from a Spring MVC perspective. I would keep the validation you have in place, for sure, because that's what it's there for: to validate. – MattSenter Oct 08 '13 at 14:30
Also, in regards to the example I gave, I still have a habit of using the Single-Entry-Single-Exit paradigm. In my opinion, it actually creates cleaner code, but it is largely up to personal preference (or the preference of your team.) If you are interested in the topic, here's a discussion on it: http://stackoverflow.com/questions/4838828/why-should-a-function-have-only-one-exit-point – MattSenter Oct 08 '13 at 14:36
1

Sorry to keep commenting, but you might also be interested in Spring's support for custom validators to move your passwordsMatch() method to its own 303 validator: http://docs.spring.io/spring/docs/current/spring-framework-reference/html/validation.html#validation-beanvalidation-spring-constraints =) – MattSenter Oct 08 '13 at 14:50
The link you kindly provided about custom JSR 303 bean validation is very interesting although I want to keep the flexibility of doing a one-off check manually. Now as far as "single exit point" is concerned, I still think it is a bit contrived especially insofar as it leads to nested ifs in my case. I am still wondering whether throwing an `IllegalArgumentException` if an empty string is passed is not a bit too harsh... – balteo Oct 09 '13 at 20:18
We're probably getting slightly off topic, but if you are interested in an alternate approach to handling nulls, check out this post: http://elegantcode.com/2010/05/01/say-no-to-null/ EDIT: Wait, my bad, you said "empty string" not "null." I guess that's a business rule you have to figure out. Is an empty string a valid value for password? Probably not. If not, I'd reject it. – MattSenter Oct 09 '13 at 20:51
You're right: we're off topic now. I have opened another post [see here](http://stackoverflow.com/questions/19282117/pros-and-cons-of-throwing-an-illegalargumentexception-if-a-string-argument-is-em) – balteo Oct 09 '13 at 21:03

score 0 · Answer 2 · answered Oct 07 '13 at 09:40

0

I am not following your whole example and to be honest I don't fully understand your problem, but one of the things I am wondering about is why you are not using Bean Validation as well for the password match constraint? You could write a custom class constraint for PasswordInfo.

answered Oct 07 '13 at 09:40

Hardy

18,659
3
49
65

Hello Hardy! Actually, the issue is more related to Spring MVC than it is to JSR 303 - although I did add the "bean-validation" tag to my post. Basically, I am looking for a clean way to avoid repeating the `if (bindingResult.hasErrors())` block when I mix custom validation with JSR 303 validation. Do you see the point? – balteo Oct 07 '13 at 09:59

Validation issue in a Spring MVC controller using JSR 303 in order to deal with empty values

2 Answers2