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

KimTypeServIveBase class has protected method which can return null but calling method is assuming non-null value

    Details

    • Type: Bug Fix Bug Fix
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0
    • Component/s: Development
    • Labels:
      None
    • Similar issues:
      KULRICE-1284Protect call to get property type in PojoPropertyUtilsBean from throwing exceptions
      KULRICE-10110Cleanup on copy methods
      KULRICE-7594Provide input field instance to key value finder method getKeyValues
      KULRICE-4047UIDocumentServiceImpl.getMember() can return null but is not handled
      KULRICE-3299The UiDocumentServiceImpl.getAttributeDefnId() has a code typo which results in a class cast exception
      KULRICE-5135fix ojb proxies for classes with protected setters
      KULRICE-7616the KEW ActionSet class is mutable and I assume it should not be
      KULRICE-3643Make private methods in document-related classes protected to improve overridability
      KULRICE-7655WorkflowDocumentService.getActionsTaken has the wrong method name
      KULRICE-1414Select Lists (call to ValuesFinder class) not updated frequently enough
    • Rice Module:
      KIM
    • Application Requirement:
      KFS

      Description

      The class KimTypeServiceBase has a method getAttributeDefinitions() which makes a call into the method getDataDictionaryAttributeDefinition(). The getAttributeDefinitions() method assumes the returned value is a non-null value but if the method getDataDictionaryAttributeDefinition() can't find a valid DD AttributeDefinition in the system, an exception is caught and logged and a null object is returned (currently on line 548 of KimTypeServiceBase).

      It seems bad that when an exception is thrown a null value is returned and then the calling method will, in most cases, throw a NullPointerException. Probably the getAttributeDefintiions() should just check for a valid non-null AttributeDefintiion object and throw an exception if one isn't found. Then again i'm now a KIM expert so there may be a better solution.

        Issue Links

          Activity

          Hide
          Jonathan Keller added a comment -

          My take (I haven't looked at the code at the moment) is that it should simply return an empty list so that clients don't need to handle the null.

          Show
          Jonathan Keller added a comment - My take (I haven't looked at the code at the moment) is that it should simply return an empty list so that clients don't need to handle the null.
          Ailish Byrne made changes -
          Field Original Value New Value
          Link This issue duplicates KFSMI-4212 [ KFSMI-4212 ]
          Ailish Byrne made changes -
          Fix Version/s 1.0.1 [ 15300 ]
          Fix Version/s KFS Release 3.0 [ 12780 ]
          Fix Version/s 1.0 [ 13481 ]
          Reporter David Elyea [ delyea ] Ailish Byrne [ abyrne ]
          Application Requirement [KRICE] [KFS]
          Priority Major [ 3 ] Critical [ 2 ]
          Responsible Team [Rice Team] [KFS Team]
          Peter Giles (Inactive) made changes -
          Assignee Peter Giles [ gilesp ]
          Hide
          Peter Giles (Inactive) added a comment -

          Looking at KimTypeServiceBase.getDataDictionaryAttributeDefinition(...), what is happening is that if a base AttributeDefinition can't be loaded then an uninitialized KimDataDictionaryAttributeDefinition instance is being returned. That leaves it to the caller to inspect the returned value and determine that its fields are empty. It would be better to return null, since then it would be explicit that no AttributeDefinition was found. I'm going to go with that, and javadoc that behavior. I'll then update getAttributeDefinitions(...) to do the appropriate null check on the returned value.

          Show
          Peter Giles (Inactive) added a comment - Looking at KimTypeServiceBase.getDataDictionaryAttributeDefinition(...), what is happening is that if a base AttributeDefinition can't be loaded then an uninitialized KimDataDictionaryAttributeDefinition instance is being returned. That leaves it to the caller to inspect the returned value and determine that its fields are empty. It would be better to return null, since then it would be explicit that no AttributeDefinition was found. I'm going to go with that, and javadoc that behavior. I'll then update getAttributeDefinitions(...) to do the appropriate null check on the returned value.
          Hide
          Peter Giles (Inactive) added a comment -

          KimTypeServiceBase.java:

          • modified getDataDictionaryAttributeDefinition(...) to return null instead of an uninitialized object if it doesn't get a base AttributeDefinition back from the DD.
          • modified getAttribteDefinitions(...) do to a null check on the returned value from getDataDictionaryAttributeDefinition(...).
          • did some FindBugs cleanup, including removal of a potential null dereference.
          Show
          Peter Giles (Inactive) added a comment - KimTypeServiceBase.java: modified getDataDictionaryAttributeDefinition(...) to return null instead of an uninitialized object if it doesn't get a base AttributeDefinition back from the DD. modified getAttribteDefinitions(...) do to a null check on the returned value from getDataDictionaryAttributeDefinition(...). did some FindBugs cleanup, including removal of a potential null dereference.
          Peter Giles (Inactive) made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Peter Giles (Inactive) added a comment -

          Committed to 1.0, reopening to correct the fix version

          Show
          Peter Giles (Inactive) added a comment - Committed to 1.0, reopening to correct the fix version
          Peter Giles (Inactive) made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          Peter Giles (Inactive) made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.0 [ 13481 ]
          Fix Version/s KFS Release 3.0 [ 12780 ]
          Fix Version/s 1.0.1 [ 15300 ]
          Resolution Fixed [ 1 ]
          Hide
          Eric Westfall added a comment -

          Bulk change of all Rice 1.0 issues to closed after public release.

          Show
          Eric Westfall added a comment - Bulk change of all Rice 1.0 issues to closed after public release.
          Eric Westfall made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Shem Patterson (Inactive) made changes -
          Workflow custom [ 73482 ] Copy of custom for rice [ 211209 ]
          Shem Patterson (Inactive) made changes -
          Workflow Copy of custom for rice [ 211209 ] custom [ 220957 ]
          Shem Patterson (Inactive) made changes -
          Workflow custom [ 220957 ] Rice Workflow [ 230705 ]

            People

            • Assignee:
              Peter Giles (Inactive)
              Reporter:
              Ailish Byrne
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Structure Helper Panel