Uploaded image for project: 'Spring Framework'
  1. Spring Framework
  2. SPR-6299

Support for @ModelAttribute interdependency

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Complete
    • Affects Version/s: 2.5.6
    • Fix Version/s: 4.1 RC1
    • Component/s: Web
    • Labels:
      None

      Description

      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
        }
      }

        Issue Links

          Activity

          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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 SPR-5695 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 SPR-5695 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!

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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 SPR-5695 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 SPR-5695 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!
          Hide
          twhitmore.nz@gmail.com Thomas Whitmore added a comment -

          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!

          Show
          twhitmore.nz@gmail.com Thomas Whitmore added a comment - 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!
          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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 SPR-5695 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 SPR-5695 open for now and also the resolution of SPR-10859 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.

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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 SPR-5695 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 SPR-5695 open for now and also the resolution of SPR-10859 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.
          Hide
          rstoya05-aop Rossen Stoyanchev added a comment -

          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.

          Show
          rstoya05-aop Rossen Stoyanchev added a comment - 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.
          Hide
          mdeinum Marten Deinum added a comment -

          This also solves SPR-10913 (as that is basically a duplicate).

          Show
          mdeinum Marten Deinum added a comment - This also solves SPR-10913 (as that is basically a duplicate).

            People

            • Assignee:
              rstoya05-aop Rossen Stoyanchev
              Reporter:
              jglynn John Glynn
              Last updater:
              Juergen Hoeller
            • Votes:
              27 Vote for this issue
              Watchers:
              32 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Days since last comment:
                2 years, 41 weeks, 5 days ago