Community
Participate
Working Groups
Java Docs for CopyPolicy: http://www.eclipse.org/eclipselink/api/1.0/org/eclipse/persistence/descriptors/copying/CopyPolicy.html Note: the JavaDocs around the various copy policies are a little weak. Basically we have support for a customer specifying their own implementation of a CopyPolicy @CopyPolicy("mypackage.MyPolicy") In addition to this we have an InstantiationCopyPolicy which is the default and the options of a CloneCopyPolicy. I would like to see us come up with one annotation that supports the 3 approaches. public class CloneCopyPolicy extends AbstractCopyPolicy { protected String methodName; protected String workingCopyMethodName;
Proposed design follows the way things are done for @Converter There are basically 3 types of copy policy 1. InstantiationCopyPolicy (default) - copies by using default constructor and setting fields or properties 2. CopyPolicy - user specifies a class that defines how the object is copied by implementing CopyPolicy 3. CloneCopyPolicy - a specified clone method is used to copy the method, a potentially different method is used for unit of work cloning Here are suggestions 1. Annotation: @InstantiationCopyPolicy XML <instantiation-copy-policy/> 2. Annotation @CopyPolicy("foo.Bar") // foo.Bar represents the class the user defines their copy policy in XML <copy-policy policy-class="foo.Bar"/> 3. Annotation @CloneCopyPolicy(method="myClone") or @CloneCopyPolicy(method="myClone", workingCopyMethod="myWorkingCopyClone") or @CloneCopyPolicy(workingCopyMethod="myWorkingCopyClone") XML <clone-copy-policy type="copy" method="myClone" workingCopyMethod="myWorkingCopyClone"/> <clone-copy-policy type="copy" workingCopyMethod="myWorkingCopyClone"/> <clone-copy-policy type="copy" method="myClone"/>
Here is the XSD update: <!-- **************************************************** --> <xsd:complexType name="copy-policy"> <xsd:annotation> <xsd:documentation> /** * A CopyPolicy is used to set a org.eclipse.persistence.descriptors.copying.CopyPolicy on an Entity. * It is required that a class that implements org.eclipse.persistence.descriptors.copying.CopyPolicy * be specified as the argument. * * A CopyPolicy should be specified on an Entity or MappedSuperclass. * * For instance: * @Entity * @CopyPolicy("example.MyCopyPolicy") */ public @interface CopyPolicy { /* * (Required) * This defines the class of the copy policy. It must specify a class that implements * org.eclipse.persistence.descriptors.copying.CopyPolicy */ Class value(); } </xsd:documentation> </xsd:annotation> <xsd:attribute name="class" type="xsd:string" use="required"/> </xsd:complexType> <!-- **************************************************** --> <xsd:complexType name="instantiation-copy-policy"> <xsd:annotation> <xsd:documentation> /** * An InstantiationCopyPolicy is used to set a org.eclipse.persistence.descriptors.copying.InstantiationCopyPolicy on an Entity. * InstantiationCopyPolicy is the default CopyPolicy in EclipseLink and therefore this configuration option is only used to override * other types of copy policies * * An InstantiationCopyPolicy should be specified on an Entity or MappedSuperclass. * * Example: * @Entity * @InstantiationCopyPolicy */ public @interface InstantiationCopyPolicy { } </xsd:documentation> </xsd:annotation> </xsd:complexType> <!-- **************************************************** --> <xsd:complexType name="clone-copy-policy"> <xsd:annotation> <xsd:documentation> /** * A CloneCopyPolicy is used to set a org.eclipse.persistence.descriptors.copying.CloneCopyPolicy on an Entity. * A CloneCopyPolicy must specify at one or both of the "method" or "workingCopyMethod". "workingCopyMethod" is used * to clone objects that will be returned to the user as they are registered in EclipseLink's transactional mechanism, * the UnitOfWork. "method" will be used for the clone that is used for comparison in conjunction with EclipseLink's * DeferredChangeDetectionPolicy * * A CloneCopyPolicy should be specified on an Entity or MappedSuperclass. * * Example: * @Entity * @CloneCopyPolicy(method="myCloneMethod") * * or: * * @Entity * @CloneCopyPolicy(method="myCloneMethod", workingCopyMethod="myWorkingCopyCloneMethod") * * or: * @Entity * @CloneCopyPolicy(workingCopyMethodName="myWorkingCopyClone") */ public @interface CloneCopyPolicy { /** * (Optional) * Either method or workingCopyMethod must be specified * this defines a method that will be used to create a clone that will be used for comparison by * EclipseLink's DeferredChangeDetectionPolicy * @return */ String method(); /** * (Optional) * Either method or workingCopyMethod must be specified * this defines a method that will be used to create a clone that will be used to create the * object returned when registering an Object in an EclipseLink UnitOfWork * @return */ String workingCopyMethod(); } </xsd:documentation> </xsd:annotation> <xsd:attribute name="method" type="xsd:string"/> <xsd:attribute name="working-copy-method" type="xsd:string"/> </xsd:complexType> <!-- **************************************************** -->
Created attachment 95203 [details] proposed fix
Created attachment 95263 [details] updated patch based on review
Reviewed by Guy Pelletier Added the following tests in both the annotations and xml test models: RelationshipModelJUnitTestSuite.testCopyPolicy RelationshipModelJUnitTestSuite.testInstantiationCopyPOlicy RelationshipModelJUnitTestSuite.testCloneCopyPolicy
My concern with 3 separate annotations and xml schema elements is that it is not as clearly and usable to the user. With the 3 different annotation/elements which are mutually exclusive we need to add in test and exceptions for the case where multiple ones are provided. If there are strong technical reasons why a single annotation and XML element cannot be used based on validation logic then we just need to capture the reasons here and document the additional usage restrictions and troubleshooting for the new new exceptions. Here are my rough thoughts on what a single config might look like: 1. Instantiation Copy Policy Annotation: @CopyPolicy(type="instantiation") XML <copy-policy type="instantiation"/> 2. Custom policy class Annotation @CopyPolicy(type="custom", class="foo.Bar") XML <copy-policy type="custom" class="foo.Bar"/> 3. Clone methods Annotation @CopyPolicy(type="clone", method="myClone") or @CopyPolicy(type="clone", method="myClone", workingCopyMethod="myWorkingCopyClone") or @CopyPolicy(type="clone", workingCopyMethod="myWorkingCopyClone") XML <copy-policy type="clone" method="myClone" workingCopyMethod="myWorkingCopyClone"/> <copy-policy type="clone" workingCopyMethod="myWorkingCopyClone"/> <copy-policy type="clone" method="myClone"/>
Your comments would have been quite useful on March 13, when the initial request for feedback was sent to the mailing list, or in the nearly 4 weeks between then and when the feature was checked in. As it stands, it is a bit late to change the design of the feature. If you'd like to enter a separate change request, and get it prioritized appropriately we can certainly discuss making some changes. The design you are proposing was considered, but the alternate design was chosen for several reasons. 1. It follows an already existing pattern in our ORM processing. i.e. This is how Converters are designed. 2. Doing things this way provides superior compile time validation and code completion for, in my opinion, the least intuitive part of using CopyPolicies - i.e. the attributes that can be used with a particular CopyPolicy. In that way, I believe it makes CopyPolicies more usable than the alternate design.
Agreed - this feedback should have been provided much earlier. My initial comments and suggestions at the top of the bug were not addressed and my previous comments are basically re-iterating those thoughts. If feedback is provided in a bug and rejected it is valuable to capture the reasons for the benefit of all parties involved. This is a rarely used feature so having a single annotation and element would have been simpler in my opinion. Addressing the user experience through code completion and simplified validation are also concerns which I can see leading to alternate solutions. I am not voting for doing it my way as much as I want to make sure each of these new configuration elements is well reviewed and discussed. At this point I agree that we should not change anything but do need to carefully review this in conjunction with all of the other descriptor policy types for simplicity and consistency across the policies. I would only recommend coming back to this feature if we agreed that it is necessary to align all of the descriptor policies in a consistent fashion and this one was different. In cases where multiple independent annotations or XML elements must be used in a mutually exclusive fashion we need to carefully capture this in the JavaDocs, XSD, and documentation (wiki) to make it clear to users how these new configuration elements can be used and when they should be used.
Just to be clear the solution implemented is described in comments #2 and #3. I am copying these examples into the functional specification available at the URL link in this bug.
The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink
