jakartaee / persistence

https://jakartaee.github.io/persistence/
Other
206 stars 59 forks source link

Define default AccessType for XML configuration #332

Open dazey3 opened 2 years ago

dazey3 commented 2 years ago

Section 2.3.1 (Default Access Type) goes into great detail to define the default access type for mapping annotations. However, it does not define a default access type strategy for XML descriptor configurations. This causes issues for persistence providers as they are open to default to whatever strategy they want for XML configurations and overall reduces portability.

Possible fixes:

1) We pick one. Define either AccessType.FIELD or AccessType.PROPERTY as the default type for XML configuration.

2) Specifically state that access attribute is required in orm.xml schema.

Current orm.xml schema fragment:

    <xsd:complexType name="entity">
        <xsd:sequence>
            ...
            <xsd:attribute name="class" type="xsd:string" use="required"/>
            <xsd:attribute name="access" type="orm:access-type"/>

This seems pretty clean, but creates issues with backwards compatibility. If the spec can't define a default access type (because there is no annotation placement to use), this would force users to make that determination.

3) Just say that the behavior is provider dependent and address portability. This has precedent as the spec already addresses portability at several places. This would be the easiest solution as well if no one agrees we should make any changes.

dazey3 commented 2 years ago

One other thing to note is that Section 2.3.2 does contain the following, but I don't think it is very clear what it means:

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.

Who is in error? Is this saying that # 2 is correct; that an error should be thrown by the provider for XML configurations because 'a default type cannot be determined' and 'an access type is not explicitly specified'? Perhaps this could be cleaned up to make its point clearer

lukasj commented 2 years ago
  1. is not an option - access in an orm.xml can be defined on multiple places - there can be a persistence-unit-default, a global setting or per-entity/embeddable/... setting - which one should be the one required to be set? What impact would such setting have on the overriding rules?

I think that portable behavior in this case is covered by the last paragraph of 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.

which, IMHO, clearly says, that the only correct (=portable) behavior in case of missing access type strategy is to fail - access type is not explicitly specified by means of annotations or the XML descriptor => it cannot be determined.

dazey3 commented 2 years ago

@lukasj

which, IMHO, clearly says, that the only correct (=portable) behavior in case of missing access type strategy is to fail - access type is not explicitly specified by means of annotations or the XML descriptor => it cannot be determined.

Interesting. I think I would agree with this interpretation. If I understand correctly, this means that if XML descriptor configuration is used, and no explicit access type is defined for an entity, then the provider "cannot determine" a default access type and should error, correct?

What I find interesting about this interpretation is that OpenJPA/EclipseLink/Hibernate all pick a default access type strategy, even in the event of XML descriptor without a strategy declaration. Perhaps the issue is that the specification is not clear enough on the expected behavior (all three providers got it wrong)? Perhaps the specification should define the error type and be reworded?

Also, this would mean the providers I listed above would need to change behavior to become compliant. Not exactly a decision I prefer to start throwing exceptions when they currently work. I thought maybe it might be better to work with these three providers current behavior to redefine the spec better, but I do agree with your interpretation.

lukasj commented 2 years ago

Can current interpretation be: If there is no xml, then defaults defined for annotations apply and settings from XML are taken into account if and only if they are present?

dazey3 commented 2 years ago

If there is no xml, then defaults defined for annotations apply and settings from XML are taken into account if and only if they are present?

Yes, that is how it should/does work. If you use annotations, the location of the annotations denote the default access type. If XML declares the access type, then that is used.

The issue isn't about mixing XML/Annotation configurations. The issue is with XML only configurations (no annotations) and no explicit access declaration.

public class MyEntity {
    private int id;
    private int intVal;
    private int transientInt;

    public int getId() {
        return id;
    }

    public int getIntVal() {
        return intVal + transientInt;
    }
<entity-mappings ...
    <entity class="simple.test.MyEntity">
        <attributes>
            <id name="id"/>
            <basic name="intVal"/>
            <transient name="transientInt"/>
        </attributes>
    </entity>
</entity-mappings>

What should happen in this case? There is no explicit access type declared. Users moving from one provider to another may not realize that the default (provider specific currently) changed. For instance, Hibernate defaults to PROPERTY and EclipseLink defaults to FIELD. Both are viable since the spec doesnt define a default, nor say it's provider dependent, nor say explicitly they should error

gavinking commented 1 year ago

@dazey I agree that the default access type for a pure-XML config is undefined, and it's very unfortunate that the leading implementations disagree on the default.

But this is hardly a disaster: pure-XML config is not so common, and if you use it, and need portability, it's really very easy to stick a single <access>FIELD</access> or <access>PROPERTY</access> at the top or your orm.xml file.

Now, I'm not in principle against specifying that the default is FIELD, but that would be a breaking change for our users, and so I'm against doing it in JPA 3.2. IMO, it's a change we could consider for JPA 4.0.

For now, I don't think we need to do anything at all.