@PathVariable and @ModelAttribute incompatibility prevent me from having a nice "update" handler [SPR-7608]

[spring-projects-issues]

Nicolas Romanetti opened SPR-7608 and commented

Spring MVC examples often refer to the creation of an entity using @ModelAttribute.

A common pattern is to update an existing entity.
Generally you want to load the existing entity and then apply on it the allowed fields.

In my controller I have an handler for showing an entity, one for creating a new one...
here are their signature:

@RequestMapping("show/{pk}")
public String show(@PathVariable("pk") Account account, Model model)

@RequestMapping(value = "create", method = RequestMethod.POST)
public String create(@Valid Account account, BindingResult bindingResult, Model model)

Now, of course I need a handler, as nice as the one above to update the entity.
Here it is:

@RequestMapping(value = "update/{pk}", method = { RequestMethod.PUT, RequestMethod.POST })
public String update(@Valid @PathVariable("pk") Account account, BindingResult bindingResult, Model model);

The handler above is not accepted by SPring MVC, however I think it should! Here is what I would expect:
1/ convert the pk to the Account entity using the corresponding Parser (it works for "show" handler above...)
2/ Once converted, since the account is placed just before the bindingResult, it should be used to bind the request parameters that is to update directly the entity.
3/Once updated, it should perform the validation.

Note: I am confident in updating directly the entity as I know I can restrict the allowed fields thanks to an InitBinder.

The code above throw an exception:
org.springframework.web.bind.annotation.support.HandlerMethodInvocationException: Failed to invoke handler method [public java.lang.String fr.nnn.web.controller.AccountController.update(fr.nnn.domain.Account,org.springframework.validation.BindingResult,java.lang.Boolean,java.lang.Boolean,org.springframework.ui.Model)]; nested exception is java.lang.IllegalStateException: Errors/BindingResult argument declared without preceding model attribute. Check your handler method signature!

I tried to add the @ModelAttribute("account") annotation after @PathVariable("pk") but then another exception is thrown... stating that I cannot use both annotations at the same time.

Am I missing something or do you agree it would be a nice feature?

---

Affects: 3.0.3

Attachments:

• HandlerMethodInvoker.java (41.13 kB)

Issue Links:

• Authorize @ModelAttribute to play along with @RequestParam/@RequestHeader/... [SPR-6801] #11467 Authorize @ModelAttribute to play along with @RequestParam/@RequestHeader/...

2 votes, 4 watchers

[spring-projects-issues]

Árpád Tamási commented

I think it can be simply corrected here by dissolving annotation exclusivity and using the previously possibly created args[i] in modelattribute resolution:
org.springframework.web.bind.annotation.support.HandlerMethodInvoker.resolveHandlerArguments(Method, Object, NativeWebRequest, ExtendedModelMap)
However, I cannot judge side-effects so do not try it right now.
There is a different solution for the problem here: http://forum.springsource.org/showthread.php?p=256020

[spring-projects-issues]

Árpád Tamási commented

A proposed solution

[spring-projects-issues]

Rossen Stoyanchev commented

Allowing two annotations won't solve the problem. For @PathVariable a path segment is parsed to derive the method argument value. For @ModelAttribute, first the object is created in one of 3 ways (by invoking one or more @ModelAttribute methods, by retrieving it from the session, or through a default constructor) and second the object is populated by applying request parameter values. These are two rather different mechanisms and mixing them would not be a good solution without making further assumptions.

I would recommend using a @ModelAttribute method to preload the required object (note that you do not need to add the account to the model explicitly, see #12200):

@ModelAttribute
public void loadAccount(@PathVariable("account") Account acccount)

That will work with both the show and the update methods both of which have an "account" path variable:

@RequestMapping("show/{account}")
public String show(@PathVariable("account") Account account, Model model)

@RequestMapping(value = "update/{account}", method = { RequestMethod.PUT, RequestMethod.POST })
public String update(@Valid @ModelAttribute Account account, BindingResult bindingResult, Model model);

Unfortunately that won't work well with the create method since its URL does not have the required path variable. Consider moving the create operation to a separate class. Or alternatively use AntPathMatcher to extract the URI template variable manually in the loadAccount() @ModelAttribute method.

[spring-projects-issues]

Árpád Tamási commented

My attached solution works well for months. You are right, the original resolveModelAttribute created a new object in the ways you explained but i created a fourth way, which uses the one parsed by @PathVariable:

if (boundedObject != null) {
            bindObject = boundedObject;
} else ... 

[spring-projects-issues]

Rossen Stoyanchev commented

Yes I'm sure it can be made to work that way for some scenarios. My concern is about ambiguity. Given:

public String update(@PathVariable("pk") Account account) { }

To resolve the argument via @PathVariable and then apply data binding ignores any Account instance that may have been added already through an @ModelAttribute method or as a session attribute. Or if the session attribute was used first and then data binding applied, the @PathVariable would then have no effect.

[spring-projects-issues]

Árpád Tamási commented

I'm sure you're right about ambiguity. However, we would like to encapsulate all CRUD methods of an entity into a single controller. I just needed a quick solution that helps and my little patch does in the way we use it perfectly.

@RequestMapping(value = "update/{account}", method = {RequestMethod.POST })
public String update(@PathVariable @Valid @ModelAttribute Account account, BindingResult bindingResult) {
   // check bindingresult
   ...
   try {
       sAccount.update(account);
       return new ModelAndView("redirect:name_of_details_view");
   } 
   // handle exceptions
   ...
}

However, I would gladly see an unambigous solution built into Spring that minimize update code without breaking our encapsulation rules. We love Spring and this problem is the only barrier it left in our way.

[spring-projects-issues]

Rossen Stoyanchev commented

However, we would like to encapsulate all CRUD methods of an entity into a single controller.

I think there may be a way to resolve this cleanly with the new RequestMappingHandlerAdapter in Spring 3.1, which resolves each method argument with a separate HandlerMethodArgumentResolver. We could enhance the PathVariableMethodArgumentResolver to apply data binding if (a) the argument is not a simple type (int, String, etc) and (b) the next argument is BindingResult.

Note also that in 3.1 an @PathVariable is automatically added to the model (#12200).

[spring-projects-issues]

Árpád Tamási commented

I think there may be a way to resolve this cleanly ...
Great. I gladly contribute in any way.

Note also that in 3.1 an @PathVariable is automatically added to the model (#12200).
It's comfortable usually but sometimes we don't need it in the model. E.g. POST requests usually end up with redirection. I'm sure there are other situations also so it would be nice to have a way to disable this functionality.

[spring-projects-issues]

Rossen Stoyanchev commented

It's comfortable usually but sometimes we don't need it in the model. E.g. POST requests usually end up with redirection. I'm sure there are other situations also so it would be nice to have a way to disable this functionality.

@PathVariables are never appended automatically to the query string (#13094).

[spring-projects-issues]

Árpád Tamási commented

That's great but sometimes we do not need them in the returned model. Neither in the query string nor in JSTL expressions. Redirection was just an example of these cases. Anyway, it's not a big issue.

[spring-projects-issues]

Rossen Stoyanchev commented

I've checked in the proposed solution. The approach is based on the @ModelAttribute annotation. Essentially it becomes one of the model attribute instantiation strategies. The order of instantiation becomes:

1. Is it in the session (via @SessionAttributes)?
2. Is it in the model (via @ModelAttribute method)?
3. Does the model attribute match to a URI template var by name and is type conversion possible?
4. Default constructor instantiation

When created from a URI template variable, an attempt is made to locate and use a registered Converter or PropertyEditor. This also includes checking and using a single String-argument constructor if present on the target class. That's just a feature of Spring MVC's type conversion.

If you do give this a try, which would be great, please keep in mind you need to use the new RequestMappingHandlerMapping and RequestMappingHandlerAdapter. With the MVC namespace that will be the case, or otherwise just replace DefaultAnnotationHandlerMapping and AnnotationMethodHandlerAdapter with the above.