Uploaded image for project: 'Kuali Rice Development'
  1. Kuali Rice Development
  2. KULRICE-10889

Look into how base controller functionality is provided

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Critical
    • Resolution: Complete
    • Affects Version/s: None
    • Fix Version/s: 2.5
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • 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

        Attachments

          Issue Links

            Activity

            Hide
            tschneeb 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
            tschneeb 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
            tschneeb 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
            tschneeb 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
            tschneeb 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
            tschneeb 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
            dpace 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
            dpace 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
            jkneal 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
            jkneal 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
            dpace 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
            dpace 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
            shahess 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
            shahess 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:
                jkneal Jerry Neal (Inactive)
                Reporter:
                jkneal 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