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

OCM:The UUID of the collection elements changes on update.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: 1.5
    • Component/s: jackrabbit-ocm
    • Labels:
      None

      Description

      On ocm.update transaction, the Current implementation of DefaultCollectionConverterImpl recreates the colleciton-element nodes if there is no id field specificaiton. This is completely valid for majority of the cases. But I came across a case where the colleciton element has a uuid field. In this case also what is happening with the current implementation is that it drops all the elements from the old collection-elements and recreates the new ones. The major flip side is that now I am left with brand new UUIDs. I think we should address the uniqueness characteristics specified through UUID also while mapping colleciton elements.

      I have a patch and a TestCase to verify the same. I have implemented it only for the digester. If people feel the approach is right I will work out an annotation based testcase as well. I do not think it is going to fail even with annotations.

      1. JCR-1784.bonigopalan.patch
        19 kB
        Boni Gopalan
      2. JCR-1784.additional.bonigopalan
        6 kB
        Boni Gopalan

        Activity

        Hide
        Boni Gopalan added a comment -

        Fixed with the supplied patch

        Show
        Boni Gopalan added a comment - Fixed with the supplied patch
        Hide
        Christophe Lombart added a comment - - edited

        Your patch seems good but I'm wondering why you have modified the class AbstractMapperImpl ( see [1]).
        It works without this modification and we can leave the code as it is (throw an exception is there is no descriptor).
        Are you agree if we don't modify this class ?

        [1] Index: src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java
        ===================================================================
        — src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java (revision 701632)
        +++ src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java (working copy)
        @@ -200,7 +200,7 @@
        public ClassDescriptor getClassDescriptorByClass(Class clazz) {
        ClassDescriptor descriptor = mappingDescriptor.getClassDescriptorByName(clazz.getName());
        if (descriptor==null)

        { - throw new IncorrectPersistentClassException("Class of type: " + clazz.getName() + " has no descriptor."); + //throw new IncorrectPersistentClassException("Class of type: " + clazz.getName() + " has no descriptor."); }

        return descriptor ;
        }

        Show
        Christophe Lombart added a comment - - edited Your patch seems good but I'm wondering why you have modified the class AbstractMapperImpl ( see [1] ). It works without this modification and we can leave the code as it is (throw an exception is there is no descriptor). Are you agree if we don't modify this class ? [1] Index: src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java =================================================================== — src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java (revision 701632) +++ src/main/java/org/apache/jackrabbit/ocm/mapper/impl/AbstractMapperImpl.java (working copy) @@ -200,7 +200,7 @@ public ClassDescriptor getClassDescriptorByClass(Class clazz) { ClassDescriptor descriptor = mappingDescriptor.getClassDescriptorByName(clazz.getName()); if (descriptor==null) { - throw new IncorrectPersistentClassException("Class of type: " + clazz.getName() + " has no descriptor."); + //throw new IncorrectPersistentClassException("Class of type: " + clazz.getName() + " has no descriptor."); } return descriptor ; }
        Hide
        Christophe Lombart added a comment -

        The patch has been applied. thanks and well done ! it was a nice bug
        I didn't modify the class AbstractMapperImpl. For me, this modification is not necessary.
        I also added unit tests for the annotation support.

        Let me know if something is wrong.

        Show
        Christophe Lombart added a comment - The patch has been applied. thanks and well done ! it was a nice bug I didn't modify the class AbstractMapperImpl. For me, this modification is not necessary. I also added unit tests for the annotation support. Let me know if something is wrong.
        Hide
        Boni Gopalan added a comment -

        We seem to be going back and forth with this AbstractMapperImpl change I made. I had a problem with that throws clause and unfortunately I need to spend some time to recreate the test scenario that required me to change the implementation. If I remove the throws clause I know that a few of my application testcases will fail. I will work out a testcase that I hope will convince you the need to remove that throws. Till then I completely am with you on keeping AbstractMapperImpl as is.

        I came across a similar issue with update elsewhere. Consider same name sibling nodes with UUID specified. Though these are not part of a collection it very well act as those are collection elements. However since the code handles the mapping from directly within ObjectConverterImpl's update method we need to make that update as well intelligent enough to decipher uniqueness through UUID. I have a patch and I will add it to this discussion.

        Show
        Boni Gopalan added a comment - We seem to be going back and forth with this AbstractMapperImpl change I made. I had a problem with that throws clause and unfortunately I need to spend some time to recreate the test scenario that required me to change the implementation. If I remove the throws clause I know that a few of my application testcases will fail. I will work out a testcase that I hope will convince you the need to remove that throws. Till then I completely am with you on keeping AbstractMapperImpl as is. I came across a similar issue with update elsewhere. Consider same name sibling nodes with UUID specified. Though these are not part of a collection it very well act as those are collection elements. However since the code handles the mapping from directly within ObjectConverterImpl's update method we need to make that update as well intelligent enough to decipher uniqueness through UUID. I have a patch and I will add it to this discussion.
        Hide
        Boni Gopalan added a comment -

        I also did a bit of code cleanup to remove some repeating code. Basically I saw the approach to update a Node to be :

        Locate the node to update and then update it. So I introduce a method that takes in the session, Node to Update and the Object to Update to as parameters and does the updating. This methos is called from the other facade methods that already existed for doing the updates. Eliminated the code duplication.

        All the tests are passing.

        Show
        Boni Gopalan added a comment - I also did a bit of code cleanup to remove some repeating code. Basically I saw the approach to update a Node to be : Locate the node to update and then update it. So I introduce a method that takes in the session, Node to Update and the Object to Update to as parameters and does the updating. This methos is called from the other facade methods that already existed for doing the updates. Eliminated the code duplication. All the tests are passing.
        Hide
        Christophe Lombart added a comment -

        Done. Thanks for your contribution.

        Do you want to see the latest path in the branch 1.5 ?
        I'm not sure that is critical for the release 1.5.

        Show
        Christophe Lombart added a comment - Done. Thanks for your contribution. Do you want to see the latest path in the branch 1.5 ? I'm not sure that is critical for the release 1.5.
        Hide
        Jukka Zitting added a comment -

        Both changes are now also in the 1.5 branch. I guess this is everything we need here, so resolving as Fixed.

        Show
        Jukka Zitting added a comment - Both changes are now also in the 1.5 branch. I guess this is everything we need here, so resolving as Fixed.

          People

          • Assignee:
            Christophe Lombart
            Reporter:
            Boni Gopalan
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 3h
              3h
              Remaining:
              Remaining Estimate - 3h
              3h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development