Kuali Rice Development
  1. Kuali Rice Development
  2. KULRICE-10889

Look into how base controller functionality is provided

    Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Complete
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Similar issues:
      KULRICE-10386Provide support for Bootstrap form classes
      KULRICE-9832Look at Bootstrap validation/error display
      KULRICE-5430Control still required when ControlField is provided
      KULRICE-9518Document how to develop custom providers using the Provider SPI
      KULRICE-12917Look into how header groups are formed
      KULRICE-3570Look into suggestions for document instantiation improvements
      KULRICE-8126Collection control does not honor disable if disable is disabled by an expression for collection refresh
      KULRICE-4715Provide support for Tree Controls
      KULRICE-10135Document how to register providers with ModuleConfiguration
      KULRICE-9517Document how to use JpaPersistenceProvider
    • Epic Link:
    • Rice Module:
      KRAD
    • Application Requirement:
      KC, KS
    • Sprint:
      2.5.0-m2 Sprint 3, 2.5.0-m3 Sprint 1, 2.5.0-m3 Sprint 2, 2.5.0-m4 Sprint 1, UXI 2.5.0-m4 Sprint 2
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      Consider a delegation model (as opposed to inheritance) for the UIF provided controller functionality

        Issue Links

          Activity

          Hide
          Travis Schneeberger added a comment - - edited

          In KC we have very large transactional documents. This means that we breakup controllers for a single document into multiple controllers. This allows us to have a better design and group similar functions together (as opposed to a 10,000 line mega controller for a large document).

          For example: we would have multiple controllers with request mappings for '@RequestMapping(value = "/proposalDevelopment")' If those controllers inherit from the base KRAD controller then we have two controllers for '@RequestMapping(value = "/proposalDevelopment")' with mappings for '@RequestMapping(params = "methodToCall=docHandler")' as an example. This is a duplicate mapping an will cause spring to have startup errors.

          So Rice having base controllers makes a rigid design for us and makes it so we cannot break our Docoument controllers up.

          Once option is to have rice not place RequestMapping on any methods in there base controllers. This isn't a good solution in that you would have to override all this methods just to include the annotation.

          Also constraining to design, KC has went to a DI only model but do to the design of rice controllers, the base controllers generally uses the service locator model. This makes the controllers harder to test and less flexible.

          Show
          Travis Schneeberger added a comment - - edited In KC we have very large transactional documents. This means that we breakup controllers for a single document into multiple controllers. This allows us to have a better design and group similar functions together (as opposed to a 10,000 line mega controller for a large document). For example: we would have multiple controllers with request mappings for '@RequestMapping(value = "/proposalDevelopment")' If those controllers inherit from the base KRAD controller then we have two controllers for '@RequestMapping(value = "/proposalDevelopment")' with mappings for '@RequestMapping(params = "methodToCall=docHandler")' as an example. This is a duplicate mapping an will cause spring to have startup errors. So Rice having base controllers makes a rigid design for us and makes it so we cannot break our Docoument controllers up. Once option is to have rice not place RequestMapping on any methods in there base controllers. This isn't a good solution in that you would have to override all this methods just to include the annotation. Also constraining to design, KC has went to a DI only model but do to the design of rice controllers, the base controllers generally uses the service locator model. This makes the controllers harder to test and less flexible.
          Hide
          Travis Schneeberger added a comment -

          In addition to the rice controllers constraining our design, this model is also constraining to rice as well. Each rice base controller is around 1000 lines long. They continue to grow as krad supports more functions. These controllers have methods for completely unrelated things like workflow routing, saving, or collection management.

          For those that want to use base controllers we suggest those controllers to remain in rice. We would like those base controllers to simply delegate to "ControllerServices". These services would be designed with one function in mind and the KC team can pick and choose which "ControllerServices" we want to use. For example: WorkflowControllerService (approve, disapprove, etc), CollectionControllerService (addLine, deleteLine, etc), NavigationControllerService(back, returnToXxx, etc). TableToFileControllerService (tableCsvRetrieval, tableXlsRetrieval, tableXmlRetrieval).

          Given that this is a delegation model using Spring DI to inject "ControllerServices" into our controllers, kc and kc implementers can more easily customize the ControllerServices using normal spring overrides.

          Show
          Travis Schneeberger added a comment - In addition to the rice controllers constraining our design, this model is also constraining to rice as well. Each rice base controller is around 1000 lines long. They continue to grow as krad supports more functions. These controllers have methods for completely unrelated things like workflow routing, saving, or collection management. For those that want to use base controllers we suggest those controllers to remain in rice. We would like those base controllers to simply delegate to "ControllerServices". These services would be designed with one function in mind and the KC team can pick and choose which "ControllerServices" we want to use. For example: WorkflowControllerService (approve, disapprove, etc), CollectionControllerService (addLine, deleteLine, etc), NavigationControllerService(back, returnToXxx, etc). TableToFileControllerService (tableCsvRetrieval, tableXlsRetrieval, tableXmlRetrieval). Given that this is a delegation model using Spring DI to inject "ControllerServices" into our controllers, kc and kc implementers can more easily customize the ControllerServices using normal spring overrides.
          Hide
          Travis Schneeberger added a comment - - edited

          with the above design rice's DocumentControllerBase would look like:

          public abstract class DocumentControllerBase extends UifControllerBase {
          
              private WorkflowControllerService workflowControllerService;
          
              @RequestMapping(params = "methodToCall=approve")
              public ModelAndView approve(@ModelAttribute("KualiForm") DocumentFormBase form, BindingResult result, HttpServletRequest request, HttpServletResponse response) throws Exception {
                  return workflowControllerService.approve(form, result, request, response);        
              }
          }
          

          A KC proposal development controller would look like:

          @Controller
          public abstract class PropDevPersonnelController  {
          
              @Autowired
              @Qualifier("workflowControllerService")
              private WorkflowControllerService workflowControllerService;
              @Autowired
              @Qualifier("persistenceControllerService")
              private PersistenceControllerService persistenceControllerService;
              @Autowired
              @Qualifier("collectionControllerService")
              private CollectionControllerService collectionControllerService;
              @Autowired
              @Qualifier("navigateControllerService")
              private NavigateControllerService navigateControllerService;
          
              @RequestMapping(value = "/proposalDevelopment", params={"methodToCall=navigate", "actionParameters[navigateToPageId]=PropDev-PersonnelPage"})
              public ModelAndView navigateToPersonnel(@ModelAttribute("KualiForm") DocumentFormBase form, BindingResult result, HttpServletRequest request, HttpServletResponse response) {
                  return navigateControllerService.navigate(form, result, request, response);
              }
          
           @RequestMapping(value = "/proposalDevelopment", params="methodToCall=addPerson")
             public ModelAndView addPerson(@ModelAttribute("KualiForm") ProposalDevelopmentDocumentForm form, BindingResult result,
                     HttpServletRequest request, HttpServletResponse response) throws Exception {
                  return collectionControllerService.addLine(form, result, request, response);
              }
          }
          
          Show
          Travis Schneeberger added a comment - - edited with the above design rice's DocumentControllerBase would look like: public abstract class DocumentControllerBase extends UifControllerBase { private WorkflowControllerService workflowControllerService; @RequestMapping(params = "methodToCall=approve" ) public ModelAndView approve(@ModelAttribute( "KualiForm" ) DocumentFormBase form, BindingResult result, HttpServletRequest request, HttpServletResponse response) throws Exception { return workflowControllerService.approve(form, result, request, response); } } A KC proposal development controller would look like: @Controller public abstract class PropDevPersonnelController { @Autowired @Qualifier( "workflowControllerService" ) private WorkflowControllerService workflowControllerService; @Autowired @Qualifier( "persistenceControllerService" ) private PersistenceControllerService persistenceControllerService; @Autowired @Qualifier( "collectionControllerService" ) private CollectionControllerService collectionControllerService; @Autowired @Qualifier( "navigateControllerService" ) private NavigateControllerService navigateControllerService; @RequestMapping(value = "/proposalDevelopment" , params={ "methodToCall=navigate" , "actionParameters[navigateToPageId]=PropDev-PersonnelPage" }) public ModelAndView navigateToPersonnel(@ModelAttribute( "KualiForm" ) DocumentFormBase form, BindingResult result, HttpServletRequest request, HttpServletResponse response) { return navigateControllerService.navigate(form, result, request, response); } @RequestMapping(value = "/proposalDevelopment" , params= "methodToCall=addPerson" ) public ModelAndView addPerson(@ModelAttribute( "KualiForm" ) ProposalDevelopmentDocumentForm form, BindingResult result, HttpServletRequest request, HttpServletResponse response) throws Exception { return collectionControllerService.addLine(form, result, request, response); } }
          Hide
          Douglas Pace added a comment -

          I agree with Travis. Our primary issue originally was the duplicate mappings when using multiple controllers per url. I understand there are likely other alternatives for that specific issue, but this style of delegation also provides us significant flexibility in other areas; testability of controllers, DI for all services and controllers, and flexibility in designing our own per-document controller OO-hierarchy. Travis suggested more radical changes than KC would need to continue using this model, but in general the separation of responsibility does make sense and would benefit us moving forward as well. At the lowest level we'd just like to see some support for this model so a redesign doesn't make it impossible in the future and some slight refactoring of protected methods so they can be more easily accessed in a service architecture.

          Show
          Douglas Pace added a comment - I agree with Travis. Our primary issue originally was the duplicate mappings when using multiple controllers per url. I understand there are likely other alternatives for that specific issue, but this style of delegation also provides us significant flexibility in other areas; testability of controllers, DI for all services and controllers, and flexibility in designing our own per-document controller OO-hierarchy. Travis suggested more radical changes than KC would need to continue using this model, but in general the separation of responsibility does make sense and would benefit us moving forward as well. At the lowest level we'd just like to see some support for this model so a redesign doesn't make it impossible in the future and some slight refactoring of protected methods so they can be more easily accessed in a service architecture.
          Hide
          Jerry Neal (Inactive) added a comment -

          Thanks for the information. When you have multiple controllers for the same document, how do you handle the workflow actions that can be taken from each, or the generic collection methods? Do you end up duplicating these methods on each controller?

          One thing I think should be noted as well is the maintenance of this. When upgrades occur, any methods we have added in the framework controllers will need to be to included in all your controllers, this is of course one of the main benefits of inheritance.

          Regardless of the delegation model, we will look to fix the request mapping issue in some way. In addition, we want to change our controllers to use dependency injection for services, and work to thin out the controllers as much as possible. I am not a fan of having a lot of logic in the controllers. We have started moving related logic in services, so the controller is more of a pass through.

          I understand the delegation provides more flexibility, but I just want to make sure the additional work this will cause us really does provide options (that would not be available with the current model with improvements above) that are taken advantage of.

          Can you give more detail on 'designing our own per-document controller OO-hierarchy' and also 'slight refactoring of protected methods so they can be more easily accessed in a service architecture.'.

          Show
          Jerry Neal (Inactive) added a comment - Thanks for the information. When you have multiple controllers for the same document, how do you handle the workflow actions that can be taken from each, or the generic collection methods? Do you end up duplicating these methods on each controller? One thing I think should be noted as well is the maintenance of this. When upgrades occur, any methods we have added in the framework controllers will need to be to included in all your controllers, this is of course one of the main benefits of inheritance. Regardless of the delegation model, we will look to fix the request mapping issue in some way. In addition, we want to change our controllers to use dependency injection for services, and work to thin out the controllers as much as possible. I am not a fan of having a lot of logic in the controllers. We have started moving related logic in services, so the controller is more of a pass through. I understand the delegation provides more flexibility, but I just want to make sure the additional work this will cause us really does provide options (that would not be available with the current model with improvements above) that are taken advantage of. Can you give more detail on 'designing our own per-document controller OO-hierarchy' and also 'slight refactoring of protected methods so they can be more easily accessed in a service architecture.'.
          Hide
          Douglas Pace added a comment -

          No we don't duplicate. In fact we can't as those methods would have the same requestmapping signature.

          Our current design has one abstract shared parent, ProposalControllerBase for instance. This base provides shared functionality that isn't mapping in SpringMVC. Things like general services and generic methods, like preSave that all controllers in Proposal are likely to utilize. Below that we have the actual controllers with mappings.

          We are likely going to create a ProposalGenericController(name?) that will contain the mapped methods that are generic. Things like the generic save and back buttons, generic navigation, refresh etc. Then each page that needs special functionality will have its own controller, like ProposalPersonnelController. This will contain things specific to this page. Methods for adding personnel through the wizard, a special case navigation method for navigating to the personnel page, etc. If we have a situation where it makes sense we might create another level. Where an abstract ProposalPersonnelControllerBase contains unmapped shared functionality and child controllers below that with specific functions for adding personnel and credit split for instance. And we will likely have ProposalWorkflowController for specific approval methods that may not even need to inherit from the ProposalControllerBase as it is unlikely to share any functionality with it or any of the other controllers.

          The only alternative I've heard related to providing some kind of controller flexibility is to have each page use a separate mapping. And while this may solve it, we will end up in a situation similar to struts where functionality for each page has to be shared across controllers, particularly functionality related to navigating from one page to another has to be placed in a shared common parent so all pages use the same functionality when navigating to a specific page. A model I never liked and often causes confusion when attempting to explain in-document navigation to people.

          As far as the refactoring being asked for, it relates to the 'createInitialForm' and 'initForm' methods. 'createInitialForm' is a protected method intended to be overridden by non-abstract controllers. In a delegation model this obviously doesn't work as easily. So to get our delegation model to work we've had to copy the functionality in initForm and provide a separate method that takes a form in instead of calling a shared method.

          While I understand there is more boilerplate code involved in the delegation model and some maintenance required to keep up to date with changes, the flexibility provided far outweighs the downsides for KC. Our large documents never fit easily into the inheritance model used in the KNS and there are a fair number of ugly workarounds in KC to make it work.

          Show
          Douglas Pace added a comment - No we don't duplicate. In fact we can't as those methods would have the same requestmapping signature. Our current design has one abstract shared parent, ProposalControllerBase for instance. This base provides shared functionality that isn't mapping in SpringMVC. Things like general services and generic methods, like preSave that all controllers in Proposal are likely to utilize. Below that we have the actual controllers with mappings. We are likely going to create a ProposalGenericController(name?) that will contain the mapped methods that are generic. Things like the generic save and back buttons, generic navigation, refresh etc. Then each page that needs special functionality will have its own controller, like ProposalPersonnelController. This will contain things specific to this page. Methods for adding personnel through the wizard, a special case navigation method for navigating to the personnel page, etc. If we have a situation where it makes sense we might create another level. Where an abstract ProposalPersonnelControllerBase contains unmapped shared functionality and child controllers below that with specific functions for adding personnel and credit split for instance. And we will likely have ProposalWorkflowController for specific approval methods that may not even need to inherit from the ProposalControllerBase as it is unlikely to share any functionality with it or any of the other controllers. The only alternative I've heard related to providing some kind of controller flexibility is to have each page use a separate mapping. And while this may solve it, we will end up in a situation similar to struts where functionality for each page has to be shared across controllers, particularly functionality related to navigating from one page to another has to be placed in a shared common parent so all pages use the same functionality when navigating to a specific page. A model I never liked and often causes confusion when attempting to explain in-document navigation to people. As far as the refactoring being asked for, it relates to the 'createInitialForm' and 'initForm' methods. 'createInitialForm' is a protected method intended to be overridden by non-abstract controllers. In a delegation model this obviously doesn't work as easily. So to get our delegation model to work we've had to copy the functionality in initForm and provide a separate method that takes a form in instead of calling a shared method. While I understand there is more boilerplate code involved in the delegation model and some maintenance required to keep up to date with changes, the flexibility provided far outweighs the downsides for KC. Our large documents never fit easily into the inheritance model used in the KNS and there are a fair number of ugly workarounds in KC to make it work.
          Hide
          Shannon Hess added a comment -

          Jerry,

          I reverted the change made to ObjectPropertyUtilsTest.testCustomEditor so that it still calls UifControllerHelper.invokeViewLifecycle. I did a search and ViewLifecycleTest is also still using UifControllerHelper.invokeViewLifecycle. Without the call to invokeViewLifecycle, the test fails. Is it crucial that UifControllerHelper.invokeViewLifecycle is not called?

          Thanks,
          Shannon

          Show
          Shannon Hess added a comment - Jerry, I reverted the change made to ObjectPropertyUtilsTest.testCustomEditor so that it still calls UifControllerHelper.invokeViewLifecycle. I did a search and ViewLifecycleTest is also still using UifControllerHelper.invokeViewLifecycle. Without the call to invokeViewLifecycle, the test fails. Is it crucial that UifControllerHelper.invokeViewLifecycle is not called? Thanks, Shannon

            People

            • Assignee:
              Jerry Neal (Inactive)
              Reporter:
              Jerry Neal (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 2 days
                2d
                Remaining:
                Remaining Estimate - 2 days
                2d
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Agile

                    Structure Helper Panel