OpenJPA
  1. OpenJPA
  2. OPENJPA-1061

Entities extending from a Mapped Superclass that defines the ID fields share the same ObjectID type parameter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 1.2.1
    • Fix Version/s: 1.0.4, 1.2.2, 1.3.0, 2.0.0-M3
    • Component/s: jpa
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      When a mapped superclass (MSC) defines @Id fields, it appears that entities extending the MSC use the MSC's type in the generated ObjectID's type field. This can result in unexpected primary key collissions between entities that are not intended to be related in an entity inheritance hierarchy. Attached to the JIRA is a junit test case that demonstrates the problem.

      1. OpenJPA-1061_1.2.x.patch
        212 kB
        Jody Grassel
      2. OpenJPA-JIRA1061_1166-packagerefactor-1.2.x.patch
        345 kB
        Jody Grassel
      3. OpenJPA-JIRA1061-packagerefactor-1.2.x.patch
        288 kB
        Jody Grassel
      4. OpenJPA-JIRA1061-packagerefactor-trunk.patch
        289 kB
        Jody Grassel

        Issue Links

          Activity

          Jody Grassel created issue -
          Hide
          Jody Grassel added a comment -

          Unit test that demonstrates the problem.

          Show
          Jody Grassel added a comment - Unit test that demonstrates the problem.
          Jody Grassel made changes -
          Field Original Value New Value
          Attachment unitttest.patch [ 12407270 ]
          Hide
          Jody Grassel added a comment -

          What seems to be happening is that during entity enhancement, the MSC type acquires an ObjectIDType. When the enhancer reaches the entity types extending the MSC, the if-block:

          if (_meta.getIdentityType() == ClassMetaData.ID_APPLICATION
          && (_meta.getPCSuperclass() == null || getCreateSubclass() ||
          _meta.getObjectIdType() !=
          _meta.getPCSuperclassMetaData().getObjectIdType())) {

          compares the entity type's ObjectIDType with its MSC, finds they are equivalent, so the entity enhancement does not add the app-id methods; the leaf entity types defer to the MSC's app-id methods. So when a new entity is being inserted into the persistence context, its initial ObjectID has the MSC in its type field, instead of the leaf entity type. If another entity extending from the same MSC has the same PK-value, OpenJPAId's implentation of equals() will think it is equivalent, failing the checkForDuplicateId() check.

          I've included a potential fix for the above issue. It adds an additional check, looking for PKFields that are defined by a MSC. If it finds such PKFields, it walks up the inheritance tree, stopping at either the MSC defining the PKField, or at an entity type (to avoid breaking entity inheritance mechanisms). Stopping at the MSC defining the PKField qualifies the entity type for app-id method inclusion during enhancement. This way, the leaf entities use their own pcNewObjectIdInstance() methods (with the appropriate type coded into the method) instead of relying on the mapped superclass's version of the method.

          However, there is a degree of uncertainty I have with the fix. While it fixes the described problem, and it does not fail any of the existing JUnit tests, I rely on the ClassMetaData.isEmbeddedOnly() method to determine if a type is a MSC or a genuine entity. From what I could tell, the ClassMetaData.setEmbeddedOnly() method is called only during annotation and ORL XML metadata processing (it is set explicitly false for MappedSuperClass types). However, the methods ClassMetaData.isEmbeddedOnly() and ClassMetaData.resolveMeta() are capable of mutating the ClassMetaData._embedded field; so there needs to be verification that relying on the _embedded field is the correct method of identifying a MSC type for this processing.

          Show
          Jody Grassel added a comment - What seems to be happening is that during entity enhancement, the MSC type acquires an ObjectIDType. When the enhancer reaches the entity types extending the MSC, the if-block: if (_meta.getIdentityType() == ClassMetaData.ID_APPLICATION && (_meta.getPCSuperclass() == null || getCreateSubclass() || _meta.getObjectIdType() != _meta.getPCSuperclassMetaData().getObjectIdType())) { compares the entity type's ObjectIDType with its MSC, finds they are equivalent, so the entity enhancement does not add the app-id methods; the leaf entity types defer to the MSC's app-id methods. So when a new entity is being inserted into the persistence context, its initial ObjectID has the MSC in its type field, instead of the leaf entity type. If another entity extending from the same MSC has the same PK-value, OpenJPAId's implentation of equals() will think it is equivalent, failing the checkForDuplicateId() check. I've included a potential fix for the above issue. It adds an additional check, looking for PKFields that are defined by a MSC. If it finds such PKFields, it walks up the inheritance tree, stopping at either the MSC defining the PKField, or at an entity type (to avoid breaking entity inheritance mechanisms). Stopping at the MSC defining the PKField qualifies the entity type for app-id method inclusion during enhancement. This way, the leaf entities use their own pcNewObjectIdInstance() methods (with the appropriate type coded into the method) instead of relying on the mapped superclass's version of the method. However, there is a degree of uncertainty I have with the fix. While it fixes the described problem, and it does not fail any of the existing JUnit tests, I rely on the ClassMetaData.isEmbeddedOnly() method to determine if a type is a MSC or a genuine entity. From what I could tell, the ClassMetaData.setEmbeddedOnly() method is called only during annotation and ORL XML metadata processing (it is set explicitly false for MappedSuperClass types). However, the methods ClassMetaData.isEmbeddedOnly() and ClassMetaData.resolveMeta() are capable of mutating the ClassMetaData._embedded field; so there needs to be verification that relying on the _embedded field is the correct method of identifying a MSC type for this processing.
          Hide
          Jody Grassel added a comment -

          Proposed fix, pending verification that _embedded is a suitable check for MSC.

          Show
          Jody Grassel added a comment - Proposed fix, pending verification that _embedded is a suitable check for MSC.
          Jody Grassel made changes -
          Attachment PCEnhancer.patch [ 12407271 ]
          Hide
          Pinaki Poddar added a comment -

          I have added a new method ClassMetaData.isAbstract() which affirms (in JPA context) for MappedSuperclass. This method may help to identify whether a type is MSC.

          Show
          Pinaki Poddar added a comment - I have added a new method ClassMetaData.isAbstract() which affirms (in JPA context) for MappedSuperclass. This method may help to identify whether a type is MSC.
          Hide
          Fay Wang added a comment -

          hi Jody, the patch looks fine to me. ClassMetaData.resolveMeta can potentially modify _embedded field on the condition that the class meta data being resolved is actually "embedded" inside some other entity/embeddable. In other words, it has "owner". In the MSC case, one can not at the same time inherit from MSC and also embed an MSC when this MSC declares an Id field. As to the method isEmbeddableOnly, the _embedded field will not be set again in this method if it is already set. As you pointed out, in the case of MSC, this field is already set during the annotation/xml parsing time.

          Show
          Fay Wang added a comment - hi Jody, the patch looks fine to me. ClassMetaData.resolveMeta can potentially modify _embedded field on the condition that the class meta data being resolved is actually "embedded" inside some other entity/embeddable. In other words, it has "owner". In the MSC case, one can not at the same time inherit from MSC and also embed an MSC when this MSC declares an Id field. As to the method isEmbeddableOnly, the _embedded field will not be set again in this method if it is already set. As you pointed out, in the case of MSC, this field is already set during the annotation/xml parsing time.
          Hide
          Jody Grassel added a comment -

          Thanks for the advise. I migrated the ClassMetaData.isAbstract()/setAbstract() methods from trunk to the 1.2.x release, and modified my patch to use those methods instead of isEmbeddable(); I'm much more content with that approach as it removes any chance that the added logic to addPCMethods() will trigger on anything else other then a MappedSuperclass.

          I also found it curious to hear that the unit tests pass with the trunk build – perhaps there has been a change in processing given all the additional embeddable support provided by JPA 2.0. It would still be good to bring the unit tests to trunk, extra testing never hurts.

          I've attached a new patch which migrates and utilizes the ClassMetaData.[is|set]Abstract() methods to this JIRA.

          Show
          Jody Grassel added a comment - Thanks for the advise. I migrated the ClassMetaData.isAbstract()/setAbstract() methods from trunk to the 1.2.x release, and modified my patch to use those methods instead of isEmbeddable(); I'm much more content with that approach as it removes any chance that the added logic to addPCMethods() will trigger on anything else other then a MappedSuperclass. I also found it curious to hear that the unit tests pass with the trunk build – perhaps there has been a change in processing given all the additional embeddable support provided by JPA 2.0. It would still be good to bring the unit tests to trunk, extra testing never hurts. I've attached a new patch which migrates and utilizes the ClassMetaData. [is|set] Abstract() methods to this JIRA.
          Jody Grassel made changes -
          Attachment OpenJPA-1061_1.2.x.patch [ 12407376 ]
          Donald Woods made changes -
          Patch Info [Patch Available]
          Michael Dick made changes -
          Assignee Jody Grassel [ fyrewyld ] Michael Dick [ mikedd ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_1.2.x.patch [ 12407376 ]
          Jody Grassel made changes -
          Attachment PCEnhancer.patch [ 12407271 ]
          Jody Grassel made changes -
          Attachment unitttest.patch [ 12407270 ]
          Hide
          Jody Grassel added a comment -

          Updated patch. The previous patch failed when the MappedSuperclass had the id fields set to private-access.

          Show
          Jody Grassel added a comment - Updated patch. The previous patch failed when the MappedSuperclass had the id fields set to private-access.
          Jody Grassel made changes -
          Attachment OPENJPA-1061_1.2.x.patch [ 12408829 ]
          Jody Grassel made changes -
          Attachment OPENJPA-1061_1.2.x.patch [ 12408829 ]
          Hide
          Jody Grassel added a comment -

          Updated product code changes to resolve mapped superclass object id creation issue, and supporting JUnit test cases.

          Show
          Jody Grassel added a comment - Updated product code changes to resolve mapped superclass object id creation issue, and supporting JUnit test cases.
          Jody Grassel made changes -
          Attachment OpenJPA-1061_main_1.2.x.patch [ 12410012 ]
          Attachment OpenJPA-1061_test_1.2.x.patch [ 12410013 ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_test_1.2.x.patch [ 12410013 ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_test_1.2.x.patch [ 12410026 ]
          Hide
          Jody Grassel added a comment -

          Combined fix and junit together.

          Show
          Jody Grassel added a comment - Combined fix and junit together.
          Jody Grassel made changes -
          Attachment OpenJPA-1061_1.2.x.patch [ 12411454 ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_1.2.x.patch [ 12411454 ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_main_1.2.x.patch [ 12410012 ]
          Jody Grassel made changes -
          Attachment OpenJPA-1061_test_1.2.x.patch [ 12410026 ]
          Hide
          Jody Grassel added a comment -

          Formatting cleanup.

          Show
          Jody Grassel added a comment - Formatting cleanup.
          Jody Grassel made changes -
          Attachment OpenJPA-1061_1.2.x.patch [ 12411854 ]
          Hide
          Michael Dick added a comment -

          Thanks for the patch Jody

          Show
          Michael Dick added a comment - Thanks for the patch Jody
          Michael Dick made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.2.2 [ 12313681 ]
          Fix Version/s 1.3.0 [ 12313326 ]
          Fix Version/s 2.0.0 [ 12314019 ]
          Resolution Fixed [ 1 ]
          Michael Dick made changes -
          Link This issue incorporates OPENJPA-1156 [ OPENJPA-1156 ]
          Hide
          Jody Grassel added a comment -

          Shortened the package names and tuned the entity class names used by the test case for OPENJPA-1061. This patch is intended for trunk. A patch for 1.2.x is coming shortly.

          Show
          Jody Grassel added a comment - Shortened the package names and tuned the entity class names used by the test case for OPENJPA-1061 . This patch is intended for trunk. A patch for 1.2.x is coming shortly.
          Jody Grassel made changes -
          Hide
          Jody Grassel added a comment -

          Refactored packaging for the tests for the 1.2.x branch.

          Show
          Jody Grassel added a comment - Refactored packaging for the tests for the 1.2.x branch.
          Jody Grassel made changes -
          Hide
          Jody Grassel added a comment -

          Another pass at refactoring the package and class names.

          Show
          Jody Grassel added a comment - Another pass at refactoring the package and class names.
          Jody Grassel made changes -
          Donald Woods made changes -
          Fix Version/s 2.0.0-M3 [ 12314148 ]
          Fix Version/s 2.0.0 [ 12314019 ]
          Michael Dick made changes -
          Fix Version/s 1.0.4 [ 12313301 ]
          Donald Woods made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              Michael Dick
              Reporter:
              Jody Grassel
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development