Support for @ModelAttribute interdependency [SPR-6299]

[spring-projects-issues]

John Glynn opened SPR-6299 and commented

A common requirement is that a @ModelAttribute annotated method be dependent upon another.

ie.

@ModelAttribute("foo")
public Object getFoo() {
    ...
}

@ModelAttribute("bar")
public Object getBar(@ModelAttribute("foo") Object foo) {
  if( some condition of foo ){
    do stuff
  }
}

---

Affects: 2.5.6

Reference URL: http://forum.springsource.org/showthread.php?p=263603#post263603

Issue Links:

• @ModelAttribute usage, can prevent @ModelAttribute initialization of dependent. [SPR-9922] #14555 @ModelAttribute usage, can prevent @ModelAttribute initialization of dependent. ("is duplicated by")
• @ModelAttribute parameter dependency issue. [SPR-11315] #15939 @ModelAttribute parameter dependency issue. ("is duplicated by")
• @ModelAttribute allowed on attribute in @ModelAttribute annoted method [SPR-10913] #15541 @ModelAttribute allowed on attribute in @ModelAttribute annoted method ("is duplicated by")
• Allow an @ModelAttribute method to be invoked after the @RequestMapping method [SPR-5695] #10365 Allow an @ModelAttribute method to be invoked after the @RequestMapping method
• Portlet MVC @ModelAttribute interdependency is still not supported [SPR-13694] #18269 Portlet MVC @ModelAttribute interdependency is still not supported

Referenced from: commits 56a82c1

27 votes, 32 watchers

[spring-projects-issues]

Pavel Marinchev commented

Basically, the situation when the "foo" model attribute is being bind into getBar(...) method is quite possible according to existed source code (3.0.0-RELEASE).

Please, see invokeHandlerMethod(...) method in HandlerMethodInvoker class. Inside this method the invocation of resolveHandlerArguments(...) method uses the the same model being populated with @ModelAttribute methods results.

The problem there is that we don't have any guarantee of proper dependencies resolution. Sometimes we get luck and it works, but sometimes not. It depends of how the reflection works.

[spring-projects-issues]

Kevin Pearcey commented

I have just hit this problem. We have been using this as a 'clean' way to provide typed data into the @RequestHandler methods, but looks like that's all about to go - Everything is fine on out test systems, but we have one class where the ordering is incorrect when pushed onto a production server.

[spring-projects-issues]

Rossen Stoyanchev commented

Couldn't the same be done as follows?

@ModelAttribute
public void populateModel(Model model) { 
  Foo foo = ...;
  model.addAttribute(foo);
  if( some condition of foo ){ 
    Bar bar = ...
    model.addAttribute(bar);
  }  
}

This is equivalent to the referenceData() method mentioned in the forum thread.

[spring-projects-issues]

Patras Vlad Sebastian commented

They are not equivalent in all cases.
When a handler method is about to be called, only the model attributes that are specified as parameters are loaded/created and bound.
For example:

@ModelAttribute("a")
public Object createModelA() {
 ...
}

@ModelAttribute
public Object createModelB(@ModelAttribute("a") Object a) {
 ... //use "a" here
}

@RequestMapping(...)
public String methodThatNeedsA(@ModelAttribute("a") Object a) {
 ...
}

@RequestMapping(...)
public String methodThatNeedsB(@ModelAttribute("b") Object b) {
 ...
}

When methodThatNeedsA is called, createModelB is not called, but when methodThatNeedsB is called, both createModelA and createModelB will be called. Suppose the code in createModelB should only be called for methodThatNeedsB, this would solve the problem. If we were to use a single @ModelAttribute method to create both models then we would need to know in advance what handler method is going to be called so we would know what models to create.

Also your populateModel method is not at all equivalent to the old referenceData. It is closer to formBackingObject that was previously used to create the command and was called before the handler method. referenceData was used to add related data to the model and was called after the handler method (it may actually depend on what the handler method does).

[spring-projects-issues]

Rossen Stoyanchev commented

I see. In that case the original description only partially covers this scenario. What you're looking for is for @ModelAttribute methods to be invoked more selectively similar to what is requested in #11901 except in your use case the decision is implicit.

[spring-projects-issues]

Patras Vlad Sebastian commented

I'm not sure that is what the writer of the original description intended. However I only see it useful to have more @ModelAttribute methods when different handler methods need different attributes.
If you need more than one attribute (dependent on each other or not), but you are ok with creating all of them for all handler methods, then your populateModel method will work just fine.
The only difference is that @ModelAttribute is an annotation and it's easier too see what attributes are created and it can also be scanned with reflection, whereas populateModel adds attributes by code.

Obviously you could write a method similar to populateModel that will do selective model creation:

@ModelAttribute
public void populateModelSelective(Model model) { 
  model.addAttribute("a", new Object());
  if( determine if methodThatNeedsB is going to be called ){ 
    model.addAttribute("b", new Object());
  }  
}

@RequestMapping(...)
public String methodThatNeedsA(@ModelAttribute("a") Object a) {
 ...
}

@RequestMapping(...)
public String methodThatNeedsB(@ModelAttribute("b") Object b) {
 ...
}

However you have to duplicate the meaning of @RequestMapping for methodThatNeedsB at "determine if methodThatNeedsB is going to be called". Also, at a later time you might add anotherMethodThatNeedsB and you have to consider that also.

I don't want to hijack this improvement ticket, this should be just about allowing @ModelAttribute methods be dependent on each other, regardless of how you are going to use that. I will continue the discussion about "selective model creation" on #11901 and link to this comment.

[spring-projects-issues]

Thomas Whitmore commented

Hi, I've just run into the same/ related @ModelAttribute "reference data" dependency problem.

In my case, which is very simple & presumably common:

@ModelAttribute("job")
protected Job loadOrCreate (@RequestParam(value="id", required=false) Integer id);

@ModelAttribute("jobDisplay")
protected JobDisplay buildJobUI (@ModelAttribute("job") Job job);

What I'm finding, is that by quirk of reflection 'buildJobUI' is being found before 'loadOrCreate'; data binding is running, by default, with a newly created instance of Job; 'loadOrCreate' is then skipped, since it's been added to the implicit model by the automatic argument resolution.

So, Job is never loaded from the database, 'loadOrCreate' is being skipped, and fields are bound into the wrong object.

PS: I wouldn't mind being able to make a "Reference Data" method which didn't have to return any named attribute, but could just populate the implicit model. Can I do this?

BTW: I'm not in favour of the idea of "selective model creation" in the slightest, sounds like a complex solution in search of a problem.

Thanks guys!

[spring-projects-issues]

Thomas Whitmore commented

See also:
https://jira.springsource.org/browse/SPR-9922

[spring-projects-issues]

John Glynn commented

@Thomas

PS: I wouldn't mind being able to make a "Reference Data" method which didn't have to return any named attribute, but could just populate the implicit model. Can I do this?

Indeed, that is one available workaround for the problem. See example posted by Rossen above.

[spring-projects-issues]

Ricardo Oliveira commented

Having the same problem, this should be classified as a bug. Running the code on Oracle Java 7 will complain about missing default constructor, but running on openjdk 7 works fine. Not sure why different jvms will instantiate session attributes in different order, but this is something that should be fixed.

[spring-projects-issues]

Chris Beams commented

This sounds related to #14417, where we dealt with similar non-determinism in the ordering of methods from Class#getDeclaredMethods when running against JDK7. Rossen Stoyanchev, you may want to take a look at this.

[spring-projects-issues]

Rossen Stoyanchev commented

The order in which @ModelAttribute methods are invoked has always depended on reflection, and although not guaranteed in practice it isn't completely random. Apparently in JDK 7, the ordering from Class#getDeclaredMethods is quite random. That's still not a bug with regards to @ModelAttribute methods, which are not guaranteed to be invoked in any specific order.

It is true however that a method marked with @ModelAttribute("foo") will not be invoked if "foo" is already in the model but the intention there is to avoid overriding attributes added from the session (via @SessionAttributes or flash attributes), not as an inter-dependency mechanism across @ModelAttribute methods.

We may still want to introduce some predictable order of invocation for the same reasons JUnit 4.11 introduced predictability even though the order of JUnit tests shouldn't matter.

[spring-projects-issues]

Patras Vlad Sebastian commented

That is why this is an improvement and not a defect.
This ticket is about calling @ModelAttribute methods based on how they refer each other.

Having a consistent order is a bit different, it is more like a defect. Even though it's the developer's fault, it's better to fail consistently.

Still, there's values in implementing either of those.

[spring-projects-issues]

Dominic Clifton commented

This is related to a bug I raised here: https://jira.springsource.org/browse/SPR-11315

[spring-projects-issues]

Rossen Stoyanchev commented

My plan is to check @ModelAttribute method dependencies and invoke them in an appropriate order (vs the current order determined by reflection), or raise an exception (e.g. circular dependency). I find this an intuitive extension of the present mechanism.

Going a step further, what if an @ModelAttribute method depends on an @RequestMapping method, i.e. a command object bound and validated by the @RequestMapping method? We could treat those differently and invoke them after, not before. I've re-opened an old ticket #10365 and will explore that as an option.

Note that I don't intend to make @ModelAttribute methods conditional on whether they're an upstream dependency of the current @RequestMapping method as suggested by Patras Vlad Sebastian since that would change a long-standing (and by now expected) behavior.

That said if #10365 works out, such methods invoked after may need to be invoked conditionally, i.e. only after @RequestMapping methods that do have the command object in their signature. This is really a natural extension of what is being proposed as a solution and would be a new feature in any case so shouldn't surprise anyone.

Comments welcome!

[spring-projects-issues]

Thomas Whitmore commented

Thanks Rossen, I like your proposed plan.

While it would add a significant degree of complexity & sophistication internally, at the API level this should provide an “it just works” intuitive extension of the present mechanism. Since the JDK 7 reflection changes, this is really required for non-trivial use of @ModelAttribute to be useful.

My main concern really is diagnosability -- that the selection & ordering of methods can be logged effectively for debugging.

With regard to @ModelAttribute methods for supplemental rendering of the Model after request-handling -- this "kind of" makes sense as an extension of existing semantics, but I definitely miss having a clear distinction between “referenceData”() before the handler-method, and “renderModel()” after the handler-method. These are completely different phases of the request-handling lifecycle!

I am not sure exactly what the best answer is, in this case. But I feel that making pre & post phases of the handler-method into "all the same annotation" & automagically ordering them by dependency graph, may not help developer understanding as to there being an actual & very concrete lifecycle. I don't like whitewash :)

Keep up the great work & I am eager to see the results!

[spring-projects-issues]

Rossen Stoyanchev commented

Thanks for the comments Thomas. We'll make sure the logging information is useful !

Indeed invoking @ModelAttribute after @RequestMapping methods based on dependencies alone might be too subtle. At first I thought it aligned well with the motivation for #10365 which is to add attributes based on the command object. However as I think more through it I can see it becoming quite tricky for once leading to conditional invocation but more worrisome is the possibility to be invoked before and not after as a result of some relatively innocent looking change, e.g. adding another @MA method that pre-creates the command object. So it might be best to stick to the current predictable behavior that @ModelAttribute methods are invoked before. That said it would be good to hear more on cases for updating the model after the @RequestMapping method, so I'll keep #10365 open for now and also the resolution of #15486 may also provide new options in this regard.

There is a related clarification to be made in this, which is what should happen if an @ModelAttirbute method has a dependency on another model attribute that is not yet in the model? A lot of the reports on this and related issues seem to result in a model attribute silently created but not as intended due to incorrect ordering. Perhaps flagging this as an error would be most intuitive.

[spring-projects-issues]

Rossen Stoyanchev commented

There is a commit in master for the resolution of this issue. See the message for commit 56a82c for details.

Note that the solution makes a best effort to find a way to satisfy all inter-dependencies but if there are methods with unresolved dependencies still, those will be invoked in whatever order they happen to be. In other words it remains legal to have an @ModelAttribute input argument that is not yet in the model. That said the solution work for all examples I've seen and the kinds of examples I anticipate this will be useful for. As long as there is a path starting with a method with no dependencies, it should work fine.

The commit includes a dedicated test class for this issue with a few samples controllers. If there are specific samples you'd like to see or try, please comment here, or send a PR.

[spring-projects-issues]

Marten Deinum commented

This also solves #15541 (as that is basically a duplicate).