Jackrabbit Oak
  1. Jackrabbit Oak
  2. OAK-48

MicroKernel.getNodes() should return null for not existing nodes instead of throwing an exception

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.2.1
    • Component/s: mk
    • Labels:
      None

      Description

      As discussed on the list [1, 2] exceptions should only be used for exceptional cases. Requesting a not existing node through Microkernel.getNodes() should therefore rather return null instead of throwing an exception.

      [1] http://markmail.org/thread/agibgcdjv756m53u
      [2] http://markmail.org/message/gfbmogwr6mrhe2pm

      1. OAK-48.patch
        6 kB
        Michael Dürig

        Issue Links

          Activity

          Hide
          Stefan Guggisberg added a comment -

          > Requesting a not existing node through Microkernel.getNodes() should therefore rather return null instead of throwing an exception.

          i am not convinced. IMO requesting a non-existing node is rather an exception.
          there's the nodeExists() method if you want to check whether a node at a given
          path exists.

          Show
          Stefan Guggisberg added a comment - > Requesting a not existing node through Microkernel.getNodes() should therefore rather return null instead of throwing an exception. i am not convinced. IMO requesting a non-existing node is rather an exception. there's the nodeExists() method if you want to check whether a node at a given path exists.
          Hide
          Michael Dürig added a comment -

          there's the nodeExists() method if you want to check whether a node at a given path exists.

          Which is potentially expensive since it causes an additional round trip to a possibly remote API.

          Show
          Michael Dürig added a comment - there's the nodeExists() method if you want to check whether a node at a given path exists. Which is potentially expensive since it causes an additional round trip to a possibly remote API.
          Hide
          Christian Stocker added a comment -

          I'd prefer null instead of an Exception as well, exactly for the remote API reason.

          Show
          Christian Stocker added a comment - I'd prefer null instead of an Exception as well, exactly for the remote API reason.
          Hide
          Jukka Zitting added a comment -

          +1 for null return value.

          As seen in KernelNodeState, especially with a large list of child nodes the client doesn't usually know in advance whether a particular node exists or not. Making two calls (nodeExists() followed by getNodes()) or catching and interpreting an exception is inferior to the return value of a single method call covering also the case of a non-existing node.

          Show
          Jukka Zitting added a comment - +1 for null return value. As seen in KernelNodeState, especially with a large list of child nodes the client doesn't usually know in advance whether a particular node exists or not. Making two calls (nodeExists() followed by getNodes()) or catching and interpreting an exception is inferior to the return value of a single method call covering also the case of a non-existing node.
          Hide
          Thomas Mueller added a comment -

          +1 return null if the node doesn't exist

          In addition to what Jukka wrote, this would avoid potential concurrency problems in case
          the MicroKernel implementation doesn't or only halfheartedly supports the concept of revisions
          (the node could be deleted after nodeExists() and getNodes()).

          In theory, we could remove nodeExists() from the API
          (testing for existence could be achieved using a special variant of getNodes()).

          Show
          Thomas Mueller added a comment - +1 return null if the node doesn't exist In addition to what Jukka wrote, this would avoid potential concurrency problems in case the MicroKernel implementation doesn't or only halfheartedly supports the concept of revisions (the node could be deleted after nodeExists() and getNodes()). In theory, we could remove nodeExists() from the API (testing for existence could be achieved using a special variant of getNodes()).
          Hide
          Michael Dürig added a comment -

          Proposed patch

          Show
          Michael Dürig added a comment - Proposed patch
          Hide
          Stefan Guggisberg added a comment -

          fixed in svn r1325827

          Show
          Stefan Guggisberg added a comment - fixed in svn r1325827
          Hide
          Thomas Mueller added a comment -

          The various wrappers don't support this yet.
          Also, it seems there is not test case yet.

          Show
          Thomas Mueller added a comment - The various wrappers don't support this yet. Also, it seems there is not test case yet.
          Show
          Stefan Guggisberg added a comment - > Also, it seems there is not test case yet. yes, there is: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-it/mk/src/main/java/org/apache/jackrabbit/mk/test/MicroKernelIT.java?r1=1325827&r2=1325826&view=diff&pathrev=1325827
          Hide
          Jukka Zitting added a comment -

          Adjusted the wrapper layer and fixed the failing integration tests in revision 1325998.

          Show
          Jukka Zitting added a comment - Adjusted the wrapper layer and fixed the failing integration tests in revision 1325998.
          Hide
          Thomas Mueller added a comment -

          Implemented the feature in the wrappers and added test cases.

          Show
          Thomas Mueller added a comment - Implemented the feature in the wrappers and added test cases.

            People

            • Assignee:
              Thomas Mueller
              Reporter:
              Michael Dürig
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development