Jackrabbit Content Repository
  1. Jackrabbit Content Repository
  2. JCR-3128

Problem with formerly escaped JCR node names when upgrading to Jackrabbit 2.2.9

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2.9
    • Fix Version/s: 2.2.10, 2.3.2
    • Component/s: jackrabbit-core
    • Labels:
      None

      Description

      The following unit test fails:

      import static org.junit.Assert.*;
      
      import org.apache.jackrabbit.util.Text;
      import org.junit.Test;
      
      public class TestEscaping
      {
         @Test
         public void testEscaping() throws Exception
         {
            // expect this as an escaped string (e.g. formerly escaped with jackrabbit 1.6)
            String escaped = "nam%27e";
            String unescaped = Text.unescapeIllegalJcrChars(escaped);
            assertEquals(escaped, Text.escapeIllegalJcrChars(unescaped));
         }
      }
      

      This is a major problem when upgrading from 1.6.x to 2.2.9. The node names that were escaped in jackrabbit 1.6 are not longer escaped and that breaks the backward compatibility. I think the problem comes in with JCR-2198.

        Activity

        Hide
        Claus Köll added a comment -

        Hi,
        Single and double quote are valid since JCR 2.0

        Show
        Claus Köll added a comment - Hi, Single and double quote are valid since JCR 2.0
        Hide
        Sascha Theves added a comment -

        I know but the provided test case shouldn`t fail. I think I need backward compatible escaping/unescaping methods in the Text util and the provided test case should also work.

        Show
        Sascha Theves added a comment - I know but the provided test case shouldn`t fail. I think I need backward compatible escaping/unescaping methods in the Text util and the provided test case should also work.
        Hide
        Claus Köll added a comment -

        Yes you are right it is not backward compatible,
        but the Method works as designed. Please look into the TextTest.testEscapeIllegalJcrChars() Method.

        Show
        Claus Köll added a comment - Yes you are right it is not backward compatible, but the Method works as designed. Please look into the TextTest.testEscapeIllegalJcrChars() Method.
        Hide
        Sascha Theves added a comment -

        Then the design/contract of the method is wrong because unescaping and then escaping should produce the origin string...

        Show
        Sascha Theves added a comment - Then the design/contract of the method is wrong because unescaping and then escaping should produce the origin string...
        Hide
        Julian Reschke added a comment -

        (1) What was it escaped with?

        (2) No, the contract would be unescape(escape) == x, not the other way around.

        Show
        Julian Reschke added a comment - (1) What was it escaped with? (2) No, the contract would be unescape(escape ) == x, not the other way around.
        Hide
        Sascha Theves added a comment -

        1) It was escaped with the Text util from Jackrabbit 1.6: Text.escapeIllegalJcrChars.

        In my opinion you really have a problem here when migrating from Jackrabbit 1.6. But anyway I solved my problem with copying the jackrabbit 1.6 code of escaping/unescaping to my app and now I am using that code instead of Text util. That works in this case.

        Show
        Sascha Theves added a comment - 1) It was escaped with the Text util from Jackrabbit 1.6: Text.escapeIllegalJcrChars. In my opinion you really have a problem here when migrating from Jackrabbit 1.6. But anyway I solved my problem with copying the jackrabbit 1.6 code of escaping/unescaping to my app and now I am using that code instead of Text util. That works in this case.
        Hide
        Jukka Zitting added a comment -

        Yep, this is a clear backwards compatibility issue if you're using escapeIllegalJcrChars() for example to map arbitrary strings to JCR names. An upgrade shouldn't break that mapping.

        Unfortunately changing the mapping again now would just create another backwards compatibility issue. So my recommendation would be to either resolve this just as Won't Fix with a recommendation to use Sascha's workaround from above, or to add a new escapeIllegalJcr10Chars() method for use by clients that rely on the older mapping. In either case a client code change is needed for the 1.x to 2.x upgrade in cases where the client relies on this mapping.

        Show
        Jukka Zitting added a comment - Yep, this is a clear backwards compatibility issue if you're using escapeIllegalJcrChars() for example to map arbitrary strings to JCR names. An upgrade shouldn't break that mapping. Unfortunately changing the mapping again now would just create another backwards compatibility issue. So my recommendation would be to either resolve this just as Won't Fix with a recommendation to use Sascha's workaround from above, or to add a new escapeIllegalJcr10Chars() method for use by clients that rely on the older mapping. In either case a client code change is needed for the 1.x to 2.x upgrade in cases where the client relies on this mapping.
        Hide
        Sascha Theves added a comment -

        Totally agree with Jukka and for me the best solution would be to have a escapeIllegalJcr10Chars() method so I don`t need to copy the old jackrabbit code.

        Show
        Sascha Theves added a comment - Totally agree with Jukka and for me the best solution would be to have a escapeIllegalJcr10Chars() method so I don`t need to copy the old jackrabbit code.
        Hide
        Jukka Zitting added a comment -

        I committed such a escapeIllegalJcr10Chars() method in revision 1188590 and merged it to the 2.2 branch in revision 1188595.

        Introducing new method signature in a patch release like 2.2.10 is a bit troublesome (ideally there'd be no API changes), but since this is a workaround for a backwards compatibility issue I consider the benefits to outweight the potential drawbacks.

        Show
        Jukka Zitting added a comment - I committed such a escapeIllegalJcr10Chars() method in revision 1188590 and merged it to the 2.2 branch in revision 1188595. Introducing new method signature in a patch release like 2.2.10 is a bit troublesome (ideally there'd be no API changes), but since this is a workaround for a backwards compatibility issue I consider the benefits to outweight the potential drawbacks.

          People

          • Assignee:
            Jukka Zitting
            Reporter:
            Sascha Theves
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development