Rossen Stoyanchev opened SPR-5500 and commented

A controller with a @ModelAttribute method accepting a @PathVariable cannot have methods without the path variable. This is problematic because it would make sense to be able to support the following using a single controller:

/carts/new /carts/{cartId}

This issue was raised in a forum thread: http://forum.springframework.org/showthread.php?t=67385

Affects: 3.0 M1

Reference URL: http://forum.springframework.org/showthread.php?t=67385

Issue Links: - #19212 Allow @PathVariable to be optional

1 votes, 7 watchers

Comment From: spring-projects-issues

Arjen Poutsma commented

I am afraid there is very little we can do here, any possibly solution will be very fragile. There are two workarounds, however:

  • write two separate controllers, one with @ModelAttribute, one without
  • do the data binding manually in the handler methods, using ServletRequestDataBinder.

Comment From: spring-projects-issues

Roald Bankras commented

Can you explain why the ModelAttribute annotated method is called for every request? It seems to me like overhead. The beauty of the new Controller architected is that you can make one Controller class responsible for a particular resource. This constraint through the @ModelAttribute is destroying the architecture and thus sounds like a design flaw.

Comment From: spring-projects-issues

Arjen Poutsma commented

The underlying problem becomes clearer when you consider the following @Controller:

@Controller
public class BasicController {

   @ModelAttribute("cart")
   public Cart prepareCart(@PathVariable long cartId) {}

   @RequestMapping("/carts/{cartId}")
   public Cart handleCart1(@ModelAttribute Cart cart) {}

   @RequestMapping("/other/{cartId}")
   public Cart handleCart2(@ModelAttribute Cart cart) {}

   @RequestMapping("/carts")
   public Cart handleCarts() {}

}

While it certainly possible to deduce the value of cartId in prepareCart() for the /carts/1 and /other/1 cases, you must also remember that @ModelAttribute also indicates reference data. So when /carts is requested, handleCarts() is also invoked, and the cart returned from prepareCart() is to be placed in the model as reference data.

What we could do is invoke @ModelAttribute methods conditionally, only when the @PathVariable or @RequestParam(required=true) can be bound. But - as the sample above indicates - it is not that simple. Alternatively, we can add the possibility of using @RequestParam on @ModelAttribute methods. That way, it is quite clear where - for instance - the path variables come from.

I have to think about this a bit more.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

It would make sense to specify this information on the @ModelAttribute method itself but IMO the meaning of @ModelAttribute is too overloaded given you can place it both on a type and also on a method. It's not very intuitive and perhaps stretches the meaning of the annotation.

Perhaps we need a new annotation to more formally designate a method for invocation before or after request handling methods (e.g. @Interceptor). This annotation would provide more transference and control over when it is invoked. I imagine it could have many of the same attributes as @RequestMapping.

@Interceptor(value="/*/{cartId}") public Cart prepareCart(@PathVariable long cartId) {}

Comment From: spring-projects-issues

Roald Bankras commented

I think the sample of Arjen shows exactly the problem. Is it an option to give the @PathVariable a required flag? Just like the @RequestParam.

Comment From: spring-projects-issues

Arjen Poutsma commented

Adding a required flag would not help us. URI templates are always required, otherwise you shouldn't put them in the @RequestMapping value. For instance, which requests should be handled by the following method?

@RequestMapping("/hotels/{hotelId}/bookings")
public void bookings(@PathVariable(value="hotelId", required=false) int hotelId) {
...
}

Obviously, this would match /hotels/1/bookings, but also /hotels/bookings? Overall, I think this only complicates matters more, and doesn't get us further.

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Another example of where @ModelAttribute poses challenges is if you need to bind different command objects. For example an AccountsController would have methods for editing an account but may also have methods for searching accounts, which may require binding to a different object (e.g. AccountSearchCriteria). As soon as you add a @ModelAttribute method for preloading an account it becomes necessary to create a separate controller for searching accounts.

Comment From: spring-projects-issues

Ken Sipe commented

I agree with Rossen that the @ModelAttribute is too overloaded. Consider this configuration for @ModelAttribute annotation for a method; it would be nice if the annotation supported the ability to support a request method such as:

@ModelAttribute(method=RequestMethod.GET) public List\ getStates() {...}

@RequestMapping(method=RequestMethod.GET) public void setupForm(@ModelAttribute Book book) {...}

@RequestMapping(method=RequestMethod.POST) public void processForm(@ModelAttribute Book book) {...}

The ability to restrict the @ModelAttribute for the method to just the GET removes the unnecessary call for the POST. This also provides more support for two annotations as this request method configuration would make no sense on a parameter of a request mapping.

Comment From: spring-projects-issues

Leomar Uztariz commented

I partially agree with Ken, but I think that the @ModelAttribute should specify the information of the RequestMappings it applies for also, for example a configuration could be:

\


@ModelAttribute(method=RequestMethod.GET, requestmappings="/customers/list.htm,/customers/manage.htm")
public List<AnyObject> getAnyObjects(){...}

@ModelAttribute(requestmappings="/customers/edit.htm")
public Customer bindCustomer(@RequestParam(required = false, value = "customerId")Long customerId){...}

@RequestMapping("/customers/list.htm")
public ModelAndView listCustomers(){...}

@RequestMapping(method = RequestMethod.GET, value="/customers/manage.htm")
public ModelAndView manageCustomer(){...}

@RequestMapping(method = RequestMethod.POST, value="/customers/manage.htm")
public ModelAndView saveCustomer(@ModelAttribute("customer")Customer customer....){...}

\ That way we could specify for which requestmapping and methods the @ModelAttribute should apply thus giving more flexibility and avoiding the current overhead, just my opinion

Comment From: spring-projects-issues

Rob Monie commented

I'd also love to see and elegant solution to this one - see my forum post for my scenario - http://forum.springsource.org/showthread.php?t=73432

On Arjen's advice, i'm using the ServletRequestDataBinder directly in my controller for PUT requests but this seems like something I shouldn't need to do.

Comment From: spring-projects-issues

Arjen Poutsma commented

After talking this over with Jürgen, we decided not to fix this, mainly because any solution would be worse (more complex) than the problem we're trying to solve in the first place.

Methods annotated with @ModelAttribute are designed to apply to all methods in the controller, to remove repetitive code. If your binding logic does not apply to all methods, you can simply use a ServletRequestDataBinder to bind the request to your form object. You can even refactor this in another method, if you care about repetitiveness.

As an alternative to the data binder, you can use the new binding API which is currently worked on. We will try to provide a binding API that is as convenient as possible in such scenarios, but it will be an API to be used as part of a handler method impl in any case.

Comment From: spring-projects-issues

Adrian Ber commented

What about using @RequestMapping on @ModelAttribute methods?

Comment From: spring-projects-issues

Rossen Stoyanchev commented

Also remember that @PathVariable is supported with Map<String, String> as the method argument which gives you all available path variables. You can then check if the one you want is present.

Comment From: adrianboimvaser

Wouldn't it be possible to only invoke the @ModelAttribute methods if the handler method has the appropriate parameters? I.e:

@RestController
@RequestMapping(path = "units")
public class UnitController {

    @ModelAttribute
    public Unit getModelAttribute(@PathVariable Long id) {
        return unitService.findById(id);
    }

    @GetMapping
    public List<Unit> search() {
        ...
    }

    @GetMapping("{id}")
    public Unit findById(@ModelAttribute Unit unit) {
        return unit;
    }

getModelAttribute would be invoked for method findById because it returns a Unit, which is the type of one of findById's parameter, but it would not be invoked for method search, which doesn't have an argument of type Unit.