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

Update core and location services to not throw exceptions when passed null arguments

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.1.2
    • Component/s: Development
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Rice Module:
      Rice Core
    • Application Requirement:
      KFS
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required

      Description

      I don't know if it was an intentional change, but a number of core and location services now (in Rice 2.x) throw exceptions if you pass in a null or blank component of the object's primary key.

      E.g., getNamespace(), getPostalCode(), etc...

      Given that many of these are called in an automated fashion, based on what data the user enters, this is causing problems. Sometimes it is in code which we have no control over (EBO resolution from within the KNS) or complicates code when we would be perfectly fine with receiving a null as a result. (Since we still have to check for it anyway, since the non-null code passed in may not exist.

      Anyway, if it will not cause problems, could we please have these methods just treat blanks as valid values but return a null result. It would make programming against the interfaces easier. Thanks.

        Attachments

          Issue Links

            Activity

            Hide
            tschneeb Travis Schneeberger added a comment -

            This was a decision made to make all rice services behave consistently when possible. We wanted service consumers to to be able to anticipate how services are going to behave throughout rice. Also having a consistent design provides guidelines for service authors. Prior to rice 2.0 the services provided by rice were a hodgepodge of design philosophies based on whatever the author of the service decided to do. Given this, null values are generally considered bad input and it was decided to throw IllegalArgumentException. The other equally important piece to this was the service interface should be documented preconditions and postconditions.

            Show
            tschneeb Travis Schneeberger added a comment - This was a decision made to make all rice services behave consistently when possible. We wanted service consumers to to be able to anticipate how services are going to behave throughout rice. Also having a consistent design provides guidelines for service authors. Prior to rice 2.0 the services provided by rice were a hodgepodge of design philosophies based on whatever the author of the service decided to do. Given this, null values are generally considered bad input and it was decided to throw IllegalArgumentException. The other equally important piece to this was the service interface should be documented preconditions and postconditions.
            Hide
            jkeller Jonathan Keller added a comment -

            Unfortunately, this was a contract change from 1.x on services which are used internally by the KNS and KIM documents. So, we now have code which wants to validate data and simply calls the getters to validate the data. That is now blowing up instead of returning null.

            So, if the service contracts are not going to change, then this needs to turn into a request to ensure that any place in which service APIs are called by the KNS/KRAD (E.g., from a module service for EBOs), that they check the parameters for "blankness" before passing them in. The throwing of exceptions is breaking other code.

            Show
            jkeller Jonathan Keller added a comment - Unfortunately, this was a contract change from 1.x on services which are used internally by the KNS and KIM documents. So, we now have code which wants to validate data and simply calls the getters to validate the data. That is now blowing up instead of returning null. So, if the service contracts are not going to change, then this needs to turn into a request to ensure that any place in which service APIs are called by the KNS/KRAD (E.g., from a module service for EBOs), that they check the parameters for "blankness" before passing them in. The throwing of exceptions is breaking other code.
            Hide
            tschneeb Travis Schneeberger added a comment -

            Yeah I feel your pain. We knew this was going to be impacting but thought consistency was in the best interest of the rice project as a whole. Unfortunately part of this change should have been fixing all of the KRAD and KNS logic to adapt to the service contract change. These checks are probably a good thing anyway because it is generally accepted that user input should be validated upfront before being passed along to the service layer (which has the potential to be an unnecessary remote call).

            Show
            tschneeb Travis Schneeberger added a comment - Yeah I feel your pain. We knew this was going to be impacting but thought consistency was in the best interest of the rice project as a whole. Unfortunately part of this change should have been fixing all of the KRAD and KNS logic to adapt to the service contract change. These checks are probably a good thing anyway because it is generally accepted that user input should be validated upfront before being passed along to the service layer (which has the potential to be an unnecessary remote call).
            Hide
            jcoltrin Jessica Coltrin (Inactive) added a comment -

            moving to 2.1.2

            Show
            jcoltrin Jessica Coltrin (Inactive) added a comment - moving to 2.1.2
            Hide
            gilesp Peter Giles (Inactive) added a comment -

            Hi Eric, I need your guidance on this one. I'm assigning it to you for the time being.

            Show
            gilesp Peter Giles (Inactive) added a comment - Hi Eric, I need your guidance on this one. I'm assigning it to you for the time being.
            Hide
            jkeller Jonathan Keller added a comment -

            FWIW - I'm OK with the service contracts being altered from their Rice 1.x versions as long as the internal use of EBOs within KNS/KRAD short-circuits the call to the service if the value is blank. (Or, since EBOs work on compound keys as well...if any of the PK fields are blank.) I think it's fair to skip the remote call if there is no valid value in the field and simply return null.

            It's that internal code which I can't control. So we need to make sure that Rice itself never calls those services without checking the parameters. (It could also just be the module services which the EBO framework uses to make the service calls. I don't care where the check is as long as a default existence check or dynamic property resolution does not result in an exception.

            Show
            jkeller Jonathan Keller added a comment - FWIW - I'm OK with the service contracts being altered from their Rice 1.x versions as long as the internal use of EBOs within KNS/KRAD short-circuits the call to the service if the value is blank. (Or, since EBOs work on compound keys as well...if any of the PK fields are blank.) I think it's fair to skip the remote call if there is no valid value in the field and simply return null. It's that internal code which I can't control. So we need to make sure that Rice itself never calls those services without checking the parameters. (It could also just be the module services which the EBO framework uses to make the service calls. I don't care where the check is as long as a default existence check or dynamic property resolution does not result in an exception.
            Hide
            gilesp Peter Giles (Inactive) added a comment -
            • Fixed the bug described in the linked issue by adding a null check before calling the service.
            • proofed these module services' getExternalizableBusinessObject methods against calling core/location services with blank inputs.
            • Also Searched for usages of LocationApiServiceLocator CoreServiceApiServiceLocator to look for suspect invocations, but didn't find any others.

            Resolving. Please reopen if you find that I've missed something.

            Show
            gilesp Peter Giles (Inactive) added a comment - Fixed the bug described in the linked issue by adding a null check before calling the service. proofed these module services' getExternalizableBusinessObject methods against calling core/location services with blank inputs. Also Searched for usages of LocationApiServiceLocator CoreServiceApiServiceLocator to look for suspect invocations, but didn't find any others. Resolving. Please reopen if you find that I've missed something.

              People

              • Assignee:
                gilesp Peter Giles (Inactive)
                Reporter:
                jkeller Jonathan Keller
              • Votes:
                0 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: