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 Improvement
    • Status: Closed Closed
    • Priority: Blocker 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
    • Similar issues:
      KULRICE-7706KRAD role type services passing nulls to location/core service methods
      KULRICE-7768RoleService.updateDelegateMember() throws exception if you pass in an existing delegate member
      KULRICE-1777Prevent exception when parameter value is null in UrlFactory.parameterizeUrl
      KULRICE-830Create proxies/wrappers for commonly implemented services and check for null/bad return vals and throw exceptions if bad vals are returned
      KULRICE-6980KIM update API not allowing updates unless version number is passed
      KULRICE-4419KIM Role Service fails when member ID is null
      KULRICE-244resolve core service dependency issues
      KULRICE-13272The “add line” button in Stacked List demo is in the wrong location and throws null pointer exception when selected
      KULRICE-6800ParamaterRepositoryService throws exception due to null ParameterType
      KULRICE-5506All of our kim services need to have proper exception handling, return immutable collections, do not return null
    • 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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Jessica Coltrin (Inactive) added a comment -

          moving to 2.1.2

          Show
          Jessica Coltrin (Inactive) added a comment - moving to 2.1.2
          Hide
          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
          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
          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
          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
          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
          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:
              Peter Giles (Inactive)
              Reporter:
              Jonathan Keller
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Structure Helper Panel