Jackrabbit Oak
  1. Jackrabbit Oak
  2. OAK-978

AssertionError thrown for invalid paths

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9
    • Component/s: jcr
    • Labels:
      None

      Description

      If the Oak is run with assertions enabled (-ea VM flag) then it throws AssertionError instead of javax.jcr.RepositoryException

          @Test
          public void testExceptionThrownForInvalidPath() throws RepositoryException {
              Session session = getAdminSession();
      
              session.itemExists("//jcr:content");
          }
      

      Above code has following behaviour

      • In JR2 - javax.jcr.RepositoryException: Invalid path://jcr:content
      • In Oak with -ea - java.lang.AssertionError
      • In Oak without -ea - Returns false.

      For compatibility and also as per spec it should throw RepositoryException

        Activity

        Hide
        Michael Dürig added a comment -

        This is not a compatibility issue but a bug. It should actually throw a RepositoryException and never a AssertionError. I'll move the issue to a top level one.

        Show
        Michael Dürig added a comment - This is not a compatibility issue but a bug. It should actually throw a RepositoryException and never a AssertionError . I'll move the issue to a top level one.
        Hide
        Michael Dürig added a comment -

        The issues is caused by the (premature) optimisation in NamePathMapperImpl#getOakPath. I'll remove this for the time being. We can optimise again when it turns out to be necessary.

        Show
        Michael Dürig added a comment - The issues is caused by the (premature) optimisation in NamePathMapperImpl#getOakPath . I'll remove this for the time being. We can optimise again when it turns out to be necessary.
        Hide
        Michael Dürig added a comment -
        Show
        Michael Dürig added a comment - Fixed at http://svn.apache.org/r1517773
        Hide
        Jukka Zitting added a comment -

        (premature) optimization

        Are we sure it's premature? Name/path mapping used to be on a critical path for many of the micro-benchmarks I ran before, and even though we managed to refactor away most of the unnecessary mappings, I wouldn't be surprised if this remained a performance-critical piece of code, since it gets invoked by almost all JCR method calls, including reads.

        caused

        The way I see it, the NamePathMapper should be responsible just for applying namespace mappings, not verifying whether a path is valid or matches actual content. The validation and resolution of a path (the latter implies the former, so no extra check would commonly be needed) is better handled once namespace mappings have already been done. Thus I disagree that the cause of this issue is in NamePathMapperImpl.

        Show
        Jukka Zitting added a comment - (premature) optimization Are we sure it's premature? Name/path mapping used to be on a critical path for many of the micro-benchmarks I ran before, and even though we managed to refactor away most of the unnecessary mappings, I wouldn't be surprised if this remained a performance-critical piece of code, since it gets invoked by almost all JCR method calls, including reads. caused The way I see it, the NamePathMapper should be responsible just for applying namespace mappings, not verifying whether a path is valid or matches actual content. The validation and resolution of a path (the latter implies the former, so no extra check would commonly be needed) is better handled once namespace mappings have already been done. Thus I disagree that the cause of this issue is in NamePathMapperImpl .
        Hide
        Michael Dürig added a comment -

        Are we sure it's premature?

        In the way that it was broken: yes.

        The way I see it, the NamePathMapper should be responsible...

        We had that discussion on several occasions but so far no one found out how to cleanly separate those concerns: in order to validate a path, you need to fully parse it. The code I removed tried otherwise and it failed, even though it has been 'fixed' several times. An alternative solution would be to replace the removed code with a call to JcrPathParser.validate but I think that wouldn't aid performance and AFAIR we even had something along these lines earlier.

        the thing you are for calling here is making paths proper classes so we can parse the path string into an intermediate representation with thunks for name mappings.

        Show
        Michael Dürig added a comment - Are we sure it's premature? In the way that it was broken: yes. The way I see it, the NamePathMapper should be responsible... We had that discussion on several occasions but so far no one found out how to cleanly separate those concerns : in order to validate a path, you need to fully parse it. The code I removed tried otherwise and it failed, even though it has been 'fixed' several times. An alternative solution would be to replace the removed code with a call to JcrPathParser.validate but I think that wouldn't aid performance and AFAIR we even had something along these lines earlier. the thing you are for calling here is making paths proper classes so we can parse the path string into an intermediate representation with thunks for name mappings.
        Hide
        Alex Parvulescu added a comment -

        Bulk close for the 0.9 release

        Show
        Alex Parvulescu added a comment - Bulk close for the 0.9 release

          People

          • Assignee:
            Michael Dürig
            Reporter:
            Chetan Mehrotra
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development