OpenJPA
  1. OpenJPA
  2. OPENJPA-1613

Exception thrown when enhancing a (property access) class that has an abstract @MappedSuperclass with no annotated properties

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta2, 2.0.0-beta3
    • Fix Version/s: 2.1.0
    • Component/s: kernel
    • Labels:
      None

      Description

      If you have a class (using property access) that has an abstract @MappedSuperclass that happens to have no annotated methods, you get the following exception when enhancing:

      org.apache.openjpa.util.MetaDataException: "implicit property access" for class "org.apache.openjpa.persistence.simple.SubclassPerson" is not consistent with "implicit field access" used by its persistent superclass "org.apache.openjpa.persistence.simple.AbstractSuperclass". All persistent classes in an inheritance hierarchy must use a single implicit field or property based access style or explicitly declare an access style.

      Presumably the enhancer is deciding incorrectly that the superclass is using field access. A workaround is to annotate the superclass with @Access(AccessType.PROPERTY) so the enhancer doesn't make this assumption, but that is not JPA 1.0 backwards compatible.

      This did not occur in any of the OpenJPA 1.* versions

      1. OPENJPA-1613-tests.diff
        9 kB
        Donald Woods
      2. OPENJPA-1613-failing-code-changes.diff
        6 kB
        Donald Woods
      3. abstract-subclass.patch
        6 kB
        Simon Droscher

        Activity

        Hide
        Michael Dick added a comment -

        Last code change was in April. If there's more to be done we'll open another issue.

        Show
        Michael Dick added a comment - Last code change was in April. If there's more to be done we'll open another issue.
        Hide
        Jeremy Bauer added a comment -

        Forgot to mention - I added code to log a trace level message when the access type cannot be determined and default access is used. I had this as a warning, but due to the sheer quantity of entities in the jUnit bucket that use default access, the added logging was slowing down the test bucket and bloating the logs.

        Show
        Jeremy Bauer added a comment - Forgot to mention - I added code to log a trace level message when the access type cannot be determined and default access is used. I had this as a warning, but due to the sheer quantity of entities in the jUnit bucket that use default access, the added logging was slowing down the test bucket and bloating the logs.
        Hide
        Jeremy Bauer added a comment -

        Committed a fix under rev 936930 which included a slightly modified version of Simon's testcase.

        As of this commit, OpenJPA will now take @javax.persistence.Transient fields and properties into consideration when making a default access determination. This is consistent with the JPA specifications and OpenJPA 1.x behavior. However, OpenJPA 2.x does not completely mimic 1.x behavior regarding persistent properties. The provided test application exposed another inconsistency between OpenJPA 1.x and 2.x. OpenJPA 2.x does more stringent verification on persistent properties. Per spec,

        <jpa 2.0 spec>
        In this case, for every persistent property property of type T of the entity, there is a getter method, get-
        Property, and setter method setProperty. For boolean properties, isProperty may be used as an alternative
        name for the getter method.
        For single-valued persistent properties, these method signatures are:
        • T getProperty()
        • void setProperty(T t)
        </jpa 2.0 spec>

        The supplied test application patch did not have a matching setter for the getter. OpenJPA 1.x took the @Transient getter into account when making the default access determination - but dumped this message regarding the property:

        <OpenJPA 1.x>
        No setter was found for method someProperty in type test.AbstractSuperclass while searching for persistent properties. This method will be ignored. If you intended for this to be persistent, please add a corresponding setter, or switch to field access for this type hierarchy.
        </OpenJPA 1.x>

        This is an inconsistency in the 1.x code base. I think a property should only be considered in the access calculation only if the property is/can be persistent. As of this commit, OpenJPA 2.0 will only take the @Transient property into the default access calculation if the property is persistent (has a matching getter/setter). Thus, I made a small change to the test code to ensure the @Transient property is in the format defined by the spec.

        Show
        Jeremy Bauer added a comment - Committed a fix under rev 936930 which included a slightly modified version of Simon's testcase. As of this commit, OpenJPA will now take @javax.persistence.Transient fields and properties into consideration when making a default access determination. This is consistent with the JPA specifications and OpenJPA 1.x behavior. However, OpenJPA 2.x does not completely mimic 1.x behavior regarding persistent properties. The provided test application exposed another inconsistency between OpenJPA 1.x and 2.x. OpenJPA 2.x does more stringent verification on persistent properties. Per spec, <jpa 2.0 spec> In this case, for every persistent property property of type T of the entity, there is a getter method, get- Property, and setter method setProperty. For boolean properties, isProperty may be used as an alternative name for the getter method. For single-valued persistent properties, these method signatures are: • T getProperty() • void setProperty(T t) </jpa 2.0 spec> The supplied test application patch did not have a matching setter for the getter. OpenJPA 1.x took the @Transient getter into account when making the default access determination - but dumped this message regarding the property: <OpenJPA 1.x> No setter was found for method someProperty in type test.AbstractSuperclass while searching for persistent properties. This method will be ignored. If you intended for this to be persistent, please add a corresponding setter, or switch to field access for this type hierarchy. </OpenJPA 1.x> This is an inconsistency in the 1.x code base. I think a property should only be considered in the access calculation only if the property is/can be persistent. As of this commit, OpenJPA 2.0 will only take the @Transient property into the default access calculation if the property is persistent (has a matching getter/setter). Thus, I made a small change to the test code to ensure the @Transient property is in the format defined by the spec.
        Hide
        Jeremy Bauer added a comment -

        Thanks, Donald. I appreciate the feedback. Adding warnings for these conditions is a great idea.

        Show
        Jeremy Bauer added a comment - Thanks, Donald. I appreciate the feedback. Adding warnings for these conditions is a great idea.
        Hide
        Donald Woods added a comment -

        Agree. In this case where AbstractSuperclass.java only has the @Transient on a getter and no parent classes to examine, then we'd use the old behavior and set the access type as property.
        If we do this, then I'd also like to see a WARN message logged, letting the user know we're setting the access type based on @Transient and they should really set an explicit type instead (as this behavior may not be portable and could change in future Specs.) Also, we should add a WARN when getDefaultAcessType() is called and we end up defaulting to ACCESS_FIELD, with the same type of warning message.

        Show
        Donald Woods added a comment - Agree. In this case where AbstractSuperclass.java only has the @Transient on a getter and no parent classes to examine, then we'd use the old behavior and set the access type as property. If we do this, then I'd also like to see a WARN message logged, letting the user know we're setting the access type based on @Transient and they should really set an explicit type instead (as this behavior may not be portable and could change in future Specs.) Also, we should add a WARN when getDefaultAcessType() is called and we end up defaulting to ACCESS_FIELD, with the same type of warning message.
        Hide
        Jeremy Bauer added a comment -

        I ran some comparisons between OpenJPA 1.2.2 and 2.0.x. Version 1.2.2 is picking up the @javax.persistence.Transient annotation on the property in AbstractSuperclass and using this annotation to trigger property access by default. Based on the JPA 1.0 spec (and carried forward to the 2.0 spec):

        <jpa 1.0 - section 2.1.1>
        When annotations are used, the placement of the mapping annotations on either the persistent fields or persistent properties of the entity class specifies the access type as being either field- or property-based access respectively.
        </jpa 1.0>

        As you can see the spec uses "annotations" freely in this context. Can this be inferred to include all javax.persistence annotations, including Transient? If so, it would follow that this mapped superclass definition would/should result in an ambiguous access type exception: (which it does in OpenJPA 1.2.2 - not currently in OpenJPA 2.0.x - 2.0.x defaults to field access)

        @MappedSuperclass
        class MySuper {

        @Transient
        private String name;

        @Transient
        private Address getAddress()

        { ... }
        private void setAddress(Address a) { ... }

        }

        As Donald mentioned, OpenJPA 2.x.x currently ignores fields and methods tagged with the @Transient annotation when calculating the default access type. Should OpenJPA take @Transient into account when deciding which access type to choose? It is a very odd case where this annotation alone would be the deciding factor. Regarding @Transient the 1.0 spec simply states:

        <jpa 1.0 - section 2.1.1>
        All non-transient instance variables that are not annotated with the Transient annotation are persistent.
        and
        All properties not annotated with the Transient annotation are persistent.
        </jpa 1.0 -section 2.1.1>

        As Donald also points out, the 2.0 spec adds:

        <jpa 2.0 - section 2.3.1>
        It is an error if a default access type cannot be determined and an access type is not explicitly specified by means of annotations or the XML descriptor. The behavior of applications that mix the placement of annotations on fields and properties within an entity hierarchy without explicitly specifying the Access annotation is undefined.
        </jpa 2.0 - section 2.3.1>

        Looking at this specific case - it seems odd that @MappedSuperclass is used on an class with no persistent state. If that is true, you could eliminate @MappedSuperclass from AbstractSuperclass. Extending a non-persistent Java class is possible with OpenJPA.

        Regardless, based on the verbiage in the spec and given the behavior of prior releases of OpenJPA, it would seem that using @Transient to detect the default access type is what OpenJPA should do. Opinions, please!

        Show
        Jeremy Bauer added a comment - I ran some comparisons between OpenJPA 1.2.2 and 2.0.x. Version 1.2.2 is picking up the @javax.persistence.Transient annotation on the property in AbstractSuperclass and using this annotation to trigger property access by default. Based on the JPA 1.0 spec (and carried forward to the 2.0 spec): <jpa 1.0 - section 2.1.1> When annotations are used, the placement of the mapping annotations on either the persistent fields or persistent properties of the entity class specifies the access type as being either field- or property-based access respectively. </jpa 1.0> As you can see the spec uses "annotations" freely in this context. Can this be inferred to include all javax.persistence annotations, including Transient? If so, it would follow that this mapped superclass definition would/should result in an ambiguous access type exception: (which it does in OpenJPA 1.2.2 - not currently in OpenJPA 2.0.x - 2.0.x defaults to field access) @MappedSuperclass class MySuper { @Transient private String name; @Transient private Address getAddress() { ... } private void setAddress(Address a) { ... } } As Donald mentioned, OpenJPA 2.x.x currently ignores fields and methods tagged with the @Transient annotation when calculating the default access type. Should OpenJPA take @Transient into account when deciding which access type to choose? It is a very odd case where this annotation alone would be the deciding factor. Regarding @Transient the 1.0 spec simply states: <jpa 1.0 - section 2.1.1> All non-transient instance variables that are not annotated with the Transient annotation are persistent. and All properties not annotated with the Transient annotation are persistent. </jpa 1.0 -section 2.1.1> As Donald also points out, the 2.0 spec adds: <jpa 2.0 - section 2.3.1> It is an error if a default access type cannot be determined and an access type is not explicitly specified by means of annotations or the XML descriptor. The behavior of applications that mix the placement of annotations on fields and properties within an entity hierarchy without explicitly specifying the Access annotation is undefined. </jpa 2.0 - section 2.3.1> Looking at this specific case - it seems odd that @MappedSuperclass is used on an class with no persistent state. If that is true, you could eliminate @MappedSuperclass from AbstractSuperclass. Extending a non-persistent Java class is possible with OpenJPA. Regardless, based on the verbiage in the spec and given the behavior of prior releases of OpenJPA, it would seem that using @Transient to detect the default access type is what OpenJPA should do. Opinions, please!
        Hide
        Donald Woods added a comment -

        Some code changes that I tried to make, which broke some existing junits, so DO NOT commit this AS-IS.

        Show
        Donald Woods added a comment - Some code changes that I tried to make, which broke some existing junits, so DO NOT commit this AS-IS.
        Hide
        Donald Woods added a comment -

        updated tests

        Show
        Donald Woods added a comment - updated tests
        Hide
        Donald Woods added a comment -

        From sect. 2.3.1 of the JPA 2.0 spec -
        ...
        All such classes in the entity hierarchy whose access type is defaulted in this way must be consistent in their placement of annotations on either fields or properties, such that a single, consistent default access type applies within the hierarchy. Any embeddable classes used by such classes will have the same access type as the default access type of the hierarchy unless the Access annotation is specified as defined below.

        It is an error if a default access type cannot be determined and an access type is not explicitly specified by means of annotations or the XML descriptor. The behavior of applications that mix the placement of annotations on fields and properties within an entity hierarchy without explicitly specifying the Access annotation is undefined.

        So, in your case, the method is marked as Transient, so it is ignored. The variable is private, which will not be persistance in the 2.0 spec, so it is ignored. Therefore, the above "It is an error if a default access type cannot be determined" comes into play here, since the MappedSuperclass doesn't provide an explicit AccessType.

        One could argue though, if we should have thrown an exception, instead of defaulting to FIELD (which is the old OpenJPA 1.x behavior, as Private variables would be persisted.)

        Show
        Donald Woods added a comment - From sect. 2.3.1 of the JPA 2.0 spec - ... All such classes in the entity hierarchy whose access type is defaulted in this way must be consistent in their placement of annotations on either fields or properties, such that a single, consistent default access type applies within the hierarchy. Any embeddable classes used by such classes will have the same access type as the default access type of the hierarchy unless the Access annotation is specified as defined below. It is an error if a default access type cannot be determined and an access type is not explicitly specified by means of annotations or the XML descriptor. The behavior of applications that mix the placement of annotations on fields and properties within an entity hierarchy without explicitly specifying the Access annotation is undefined. So, in your case, the method is marked as Transient, so it is ignored. The variable is private, which will not be persistance in the 2.0 spec, so it is ignored. Therefore, the above "It is an error if a default access type cannot be determined" comes into play here, since the MappedSuperclass doesn't provide an explicit AccessType. One could argue though, if we should have thrown an exception, instead of defaulting to FIELD (which is the old OpenJPA 1.x behavior, as Private variables would be persisted.)
        Hide
        Donald Woods added a comment -

        Well, since you marked the methods with @Transient, the PersistenceMetaDataDeafults.determineImplicitAccessType() will return ACCESS_UNKNOWN, as those methods are removed from consideration, which causes PersistenceMetaDataDeafults.determineAccessType() to call getDeafultAccessType() which returns the default of ACCESS_FIELD.

        Show
        Donald Woods added a comment - Well, since you marked the methods with @Transient, the PersistenceMetaDataDeafults.determineImplicitAccessType() will return ACCESS_UNKNOWN, as those methods are removed from consideration, which causes PersistenceMetaDataDeafults.determineAccessType() to call getDeafultAccessType() which returns the default of ACCESS_FIELD.
        Hide
        Donald Woods added a comment -

        For 1.0 (and 2.0) apps, you can use a orm.xml to specify the access-type as an attribute on the entity -
        <xsd:simpleType name="access-type"> <xsd:annotation>
        <xsd:documentation>
        This element determines how the persistence provider accesses the state of an entity or embedded object.
        </xsd:documentation> </xsd:annotation> <xsd:restriction base="xsd:token">
        <xsd:enumeration value="PROPERTY"/>
        <xsd:enumeration value="FIELD"/> </xsd:restriction>
        </xsd:simpleType>

        Show
        Donald Woods added a comment - For 1.0 (and 2.0) apps, you can use a orm.xml to specify the access-type as an attribute on the entity - <xsd:simpleType name="access-type"> <xsd:annotation> <xsd:documentation> This element determines how the persistence provider accesses the state of an entity or embedded object. </xsd:documentation> </xsd:annotation> <xsd:restriction base="xsd:token"> <xsd:enumeration value="PROPERTY"/> <xsd:enumeration value="FIELD"/> </xsd:restriction> </xsd:simpleType>
        Hide
        Simon Droscher added a comment -

        Attached a test case that demonstrates the problem

        Show
        Simon Droscher added a comment - Attached a test case that demonstrates the problem

          People

          • Assignee:
            Jeremy Bauer
            Reporter:
            Simon Droscher
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development