This Bugzilla instance is deprecated, and most Eclipse projects now use GitHub or Eclipse GitLab. Please see the deprecation plan for details.
Bug 211300 - Add Transformation mapping support to the EclipseLink-ORM.XML Schema
Summary: Add Transformation mapping support to the EclipseLink-ORM.XML Schema
Status: RESOLVED FIXED
Alias: None
Product: z_Archived
Classification: Eclipse Foundation
Component: Eclipselink (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: Nobody - feel free to take it CLA
QA Contact:
URL:
Whiteboard: Fixed in 1.0M6
Keywords:
Depends on:
Blocks: 200040
  Show dependency tree
 
Reported: 2007-11-28 14:22 EST by Guy Pelletier CLA
Modified: 2022-06-09 10:26 EDT (History)
8 users (show)

See Also:


Attachments
The proposed patch. (105.98 KB, patch)
2008-03-07 10:15 EST, Andrei Ilitchev CLA
no flags Details | Diff
The proposed patch, please disregard the prev. one. (107.24 KB, patch)
2008-03-07 15:51 EST, Andrei Ilitchev CLA
no flags Details | Diff
the patch implements Guy's comments. (38.55 KB, text/plain)
2008-03-11 17:52 EDT, Andrei Ilitchev CLA
no flags Details
the patch implements Guy's comments. (38.55 KB, patch)
2008-03-11 17:52 EDT, Andrei Ilitchev CLA
no flags Details | Diff
the patch implementing Guy's comments - take 2. (39.65 KB, patch)
2008-03-12 11:22 EDT, Andrei Ilitchev CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guy Pelletier CLA 2007-11-28 14:22:56 EST
 
Comment 1 Andrei Ilitchev CLA 2008-01-25 16:06:58 EST
Overview.
  Defined types:
  properties - may be owned by any mapping (not just transformation mapping);
  column-transformer - corresponds to be field-transformer in 11, renamed for consistency (may have a return-written-value element - for ReturningPolicy support). Base abstract element.
  method-based-column-transformer; class-based-column-transformer; constant-column-transformer - different types of column transformers.

  transformer - corresponds to abstract-transformer-mapping in 11.

    <xsd:complexType name="property">
        <xsd:sequence>
            <xsd:element name="value" type="xsd:anyType"/>
        </xsd:sequence>
        <xsd:attribute name="name" type="xsd:string"/>
    </xsd:complexType>

    <xsd:complex-type name="properties">
        <xsd:complexType>
            <xsd:sequence>
                <xsd:element name="property" type="opm:property" minOccurs="0" maxOccurs="unbounded"/>
            </xsd:sequence>
        </xsd:complexType>
    </xsd:element>

    <xsd:complexType name="column-transformer" abstract="true">
      <xsd:sequence>
        <xsd:element name="column" type="orm:column"/>    
        <xsd:element name="return" type="orm:return-written-value" minOccurs="0"/>
      </xsd:sequence>
    </xsd:complexType>

    <xsd:complexType name="method-based-column-transformer">
        <xsd:extension base="orm:column-transformer">
            <xsd:complexContent>
                <xsd:attribute name="method-name" type="xsd:string" use="required"/>
            </xsd:complexContent>
        </xsd:extension>
    </xsd:complexType>

    <xsd:complexType name="class-based-column-transformer">
        <xsd:complexContent>
            <xsd:extension base="orm:column-transformer">
		        <xsd:complexContent>
		            <xsd:attribute name="class-name" type="xsd:string" use="required"/>
		        </xsd:complexContent>
            </xsd:extension>
        </xsd:complexContent>
    </xsd:complexType>

    <xsd:complexType name="constant-column-transformer">
        <xsd:complexContent>
            <xsd:extension base="orm:column-transformer">
               <xsd:sequence>
                    <xsd:element name="value" type="xsd:anyType"/>
                </xsd:sequence>
            </xsd:extension>
        </xsd:complexContent>
    </xsd:complexType>

  <xsd:complex-type name="column-transformers">
      <xsd:complexType>
          <xsd:sequence>
              <xsd:element name="column-transformer" type="opm:column-transformer" minOccurs="0" maxOccurs="unbounded"/>
          </xsd:sequence>
      </xsd:complexType>
  </xsd:element>

  <xsd:complexType name="transformation">
    <xsd:annotation>
      <xsd:documentation>

        @Target({METHOD, FIELD, CLASS}) @Retention(RUNTIME)
        public @interface Transformation {
          String name() default "";
          boolean readOnly() default false;
          String getMethod() default "";
          String setMethod() default "";
          FetchType fetch() default EAGER;
          boolean optional() default true;
          boolean mutable() default true;
          String attributeMethod default "";
          String attributeTransformer default "";
          ColumnTransformer[] default {};
          Property[] default {};
        }

      </xsd:documentation>
    </xsd:annotation>
    <xsd:sequence>
      <xsd:element name="attribute-method" type="xsd:string" minOccurs="0">
          <xsd:annotation>
              <xsd:documentation>The name of the attribute transformation defined in the domain class.</xsd:documentation>
          </xsd:annotation>
      </xsd:element>
      <xsd:element name="attribute-transformer" type="xsd:string" minOccurs="0">
          <xsd:annotation>
              <xsd:documentation>The class name of the attribute transformer. Used in place of attribute-transformation.</xsd:documentation>
          </xsd:annotation>
      </xsd:element>
      <xsd:element name="column-transformers" minOccurs="0">
      <xsd:element name="properties" type="orm:properties" minOccurs="0"/>
    </xsd:sequence>
    <xsd:attribute name="name" type="xsd:string"/>
    <xsd:attribute name="read-only" type="xsd:boolean"/>
    <xsd:attribute name="get-method" type="xsd:string"/>
    <xsd:attribute name="set-method" type="xsd:string"/>
    <xsd:attribute name="fetch" type="orm:fetch-type"/>
    <xsd:attribute name="optional" type="xsd:boolean"/>
    <xsd:attribute name="mutable" type="xsd:boolean">
  </xsd:complexType>
Comment 2 Shaun Smith CLA 2008-01-28 09:58:38 EST
My initial reaction is that some of the element and attibute names are very long, e.g. "method-based-column-transformer".  Among the goals for the eclipselink-orm.xml are brevity and readability/clarity.  I know there is a tension between the two goals but it's important to consider them when defining element and attribute names.  Developers who are writing these files by hand will appreciate clear short names.  Context can be used to shorten names.  For example, in "method-based-column-transformer" the "method-name" attribute could simply be "name".  We know by context we're talking about methods.

It would help if you could provide an example document excerpt so the schema can be evaluated easier.

--Shaun

Comment 3 Andrei Ilitchev CLA 2008-01-28 14:38:10 EST
Changes.
1. As per Shaun's suggestion, dropping "based" from "method-based-column-transformer" and "class-based-column-transformer" types' names:
"method-column-transformer" and "class-column-transformer".
Also changing attribute names in these types from "method-name" and "class-name" to just "name".
2. Attribute "read-only" in transformation type should be removed as unnecessary. To make the mapping read-only don't provide any column-transformers. Alternatively specify columns (fields) in column-transformers as not-insertable and not-updatable.

Document example:
    <orm:transformation name="normalHours" fetch="LAZY" optional="false">
       <orm:attribute-method>buildNormalHours</orm:attribute-method>
       <orm:column-transformers>
	  <orm:method-column-transformer name="getStartTime">
	     <orm:column table="EMPLOYEE" name="START_TIME" updatable="false"/>
	     <orm:return type="java.sql.Time"/>
	  </orm:method-column-transformer>
	  <orm:method-column-transformer  name="getEndTime">
	     <orm:field table="EMPLOYEE" name="END_TIME" updatable="false"/>
	     <orm:return type="java.sql.Time"/>
	  </orm:method-column-transformer>
       </orm:column-transformations>
    </opm:transformation>

This transformation mapping is stolen from Employee example.
Some additions were made to illustrate the possibilities:
1) it's lazily fetched;
2) doesn't allow null as attribute value;
3) mapped to two fields - allowing only inserts (not updates);
4) returning policy specified for both fields, note that it requires the type (that's because there's no way to find the type with which the fields should be read back - can't get those from the mapping).
Comment 4 Doug Clarke CLA 2008-01-28 14:58:09 EST
I think its important to discuss this in terms of a minimal configuration to show the potential brevity possible. here is what I would hope a transformation mapping might look like in XML:

    <transformation name="normalHours">
       <attribute-method>buildNormalHours</attribute-method>
       <columns>
          <column name="START_TIME" method-name="getStartTime"/>
          <column name="END_TIME" method-name="getEndTime">
       </columns>
    </transformation>
Comment 5 Doug Clarke CLA 2008-01-28 15:03:45 EST
I don't care much for attribute-method either. Maybe something like:

    <transformation name="normalHours">
       <method>buildNormalHours</method>
       <columns>
          <column name="START_TIME" method="getStartTime"/>
          <column name="END_TIME" method="getEndTime">
       </columns>
    </transformation>

In the original example i am confused by updateable and am unsure of what the return-type semantics are. 
Comment 6 Andrei Ilitchev CLA 2008-01-28 15:37:58 EST
1. There could be either attribute-method or attribute-class.
May be make them attributes? Then we won't need to define <method> and <class> elements (those would be very general in name and very trivial in substance - just a single string)

    <transformation name="normalHours" method="buildNormalHours">
or
    <transformation name="normalHours" class="NormalHoursBuilder">

2. <columns> implies a collection of <column> - which is spec.-defined annotation (and therefore xml element, too).
That's why <columns> are too general and we can't re-define <column>, so I think we should stick with <column-tarsformers> and <column-transformer>.

We could have:
   <orm:column-transformers>
      <orm:column-transformer method-name="getStartTime">
         <orm:column table="EMPLOYEE" name="START_TIME"/>
      </orm:column-transformer>
but is it really better?

3. updatable is a field on original spec.-defined Column annotation (and therefore column element in xml). Column annotation provided together with basic may specify not only column name but also its "read-only" status (as a combination of insertable=true/false and updatable=true/false).
Natural extention of this to transformation mapping (where we write into several fields) is obvious.

4. ReturningPolicy requires the type to use for returned values. Usually this type is obtained from the mapping, TransformationMapping is exception - its fields don't have a java type set - so it should be explicitly specified by the user.

Comment 7 Shaun Smith CLA 2008-01-28 16:09:06 EST
1. I like the idea of moving the selection of transformation method or class into an attribute.

    <transformation name="normalHours" method="buildNormalHours">
or
    <transformation name="normalHours" class="NormalHoursBuilder">

2. Doug was just leaving out optional attributes to see what it looke like "most of the time".  Anyway, we don't have to include elements to group things like column transforms.  We can just flatten things to be:

    <transformation name="normalHours" method="buildNormalHours">
  	<column-transform name="START_TIME" method="getStartTime" type="java.sql.Time/>
    </transformation

Although the name "column-transform" doesn't really describe what this element does--I don't have a better idea right now.

>3. updatable is a field on original spec.-defined Column annotation (and
>therefore column element in xml). Column annotation provided together with
>basic may specify not only column name but also its "read-only" status (as a
>combination of insertable=true/false and updatable=true/false).
>Natural extention of this to transformation mapping (where we write into
>several fields) is obvious.

What's the point of a column-transform specification that can't update the column?  I think you're saying that by including data schema info here we could use it in DDL gen.  That makes sense except that it makes no sense to mark it read-only in this "write-only" element.

>4. ReturningPolicy requires the type to use for returned values. Usually this
>type is obtained from the mapping, TransformationMapping is exception - its
>fields don't have a java type set - so it should be explicitly specified by the
>user.

Understood.  I added included it:

  	<column-transform name="START_TIME" method="getStartTime" type="java.sql.Time/>
Comment 8 Andrei Ilitchev CLA 2008-01-28 16:36:14 EST
2. ... we don't have to include elements to group things
like column transforms.  We can just flatten things to be...

It's not possible to have several annotations with the same name accociated with the same attribute (or method or class), the following doesn't work:
@MyAnnotation(value=1)
@MyAnnotation(value=2)
void myMethod()

Therefore if we want to be consistent with annotations we'll need <column-transformers> element.

3.What's the point of a column-transform specification that can't update the
column? 

The transformation is allowed to insert values (when the new row is inserted) but not allowed to update the existing ones.

4. ...Understood.  I added included it:
        <column-transform name="START_TIME" method="getStartTime"
type="java.sql.Time/>

The problem with this solution is that the type required only in case ReturningPolicy is used - and ReturningPolicy may have more attributes:
    <xsd:complexType name="return-written-field">
      <xsd:attribute name="insert" type="xsd:boolean"/>
      <xsd:attribute name="update" type="xsd:boolean"/>
      <xsd:attribute name="insert-return-only" type="xsd:boolean"/>
      <xsd:attribute name="type" type="xsd:string"/>
    </xsd:complexType>
By default insert=true; update=false; insert-return-only=false - that's why sometimes a simple <return/> will appear, but it could be as well:
<return insert="true" update="true" insert-return-only="true" type="java.sql.Time"/>

Also <return> may be applied with basic mapping, too.

That's why <return> should be a separate type - not part of <column-transformer>
Comment 9 Doug Clarke CLA 2008-01-28 21:55:02 EST
Yes, the pattern we are following form the JPA specification is to allow the single use of the annotation where applicable (@NamedQuery) but then when you require multiple you must use the container(@NamedQueries). While XML does not require this we should follow the JPA defined convention for XML grouping.
Comment 10 Shaun Smith CLA 2008-01-29 10:09:58 EST
The orm.xml schema does not always use a "container element" when there are repeated elements.   That is, the structure below is used sometimes and not others:

<elements>
  <element>
...
<elements>

The complex-type <entity> is full of elements that have maxOccurs="unbounded", meaning that you just have any number of occurrences of a given element--no container.  For example, in the definition below you can see that <entity> can have any number of <name-query> elements without a containing <named-queries> containing element.  However, other elements are grouped, e.g., <attributes> is a container element.  The bottom line is that there is no single rule about using containing elements or not in the orm.xml schema.  In the eclipselink-orm.xml We should strive for brevity and clarity and decide on a case by case basis whether a container element will help or hinder achieving either of these two goals.

    <xsd:sequence>
      <xsd:element name="description" type="xsd:string" minOccurs="0"/>
      <xsd:element name="table" type="orm:table" 
                   minOccurs="0"/>
      <xsd:element name="secondary-table" type="orm:secondary-table" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="primary-key-join-column" 
                   type="orm:primary-key-join-column" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="id-class" type="orm:id-class" minOccurs="0"/>
      <xsd:element name="inheritance" type="orm:inheritance" minOccurs="0"/>
      <xsd:element name="discriminator-value" type="orm:discriminator-value" 
                   minOccurs="0"/>
      <xsd:element name="discriminator-column" 
                   type="orm:discriminator-column" 
                   minOccurs="0"/>
      <xsd:element name="sequence-generator" type="orm:sequence-generator" 
                   minOccurs="0"/>
      <xsd:element name="table-generator" type="orm:table-generator" 
                   minOccurs="0"/>
      <xsd:element name="named-query" type="orm:named-query" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="named-native-query" type="orm:named-native-query" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="sql-result-set-mapping" 
                   type="orm:sql-result-set-mapping" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="exclude-default-listeners" type="orm:emptyType" 
                   minOccurs="0"/>
      <xsd:element name="exclude-superclass-listeners" type="orm:emptyType" 
                   minOccurs="0"/>
      <xsd:element name="entity-listeners" type="orm:entity-listeners" 
                   minOccurs="0"/>
      <xsd:element name="pre-persist" type="orm:pre-persist" minOccurs="0"/>
      <xsd:element name="post-persist" type="orm:post-persist" 
                   minOccurs="0"/>
      <xsd:element name="pre-remove" type="orm:pre-remove" minOccurs="0"/>
      <xsd:element name="post-remove" type="orm:post-remove" minOccurs="0"/>
      <xsd:element name="pre-update" type="orm:pre-update" minOccurs="0"/>
      <xsd:element name="post-update" type="orm:post-update" minOccurs="0"/>
      <xsd:element name="post-load" type="orm:post-load" minOccurs="0"/>
      <xsd:element name="attribute-override" type="orm:attribute-override" 
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="association-override" 
                   type="orm:association-override"
                   minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="attributes" type="orm:attributes" minOccurs="0"/>
    </xsd:sequence>


Comment 11 Andrei Ilitchev CLA 2008-01-29 16:28:05 EST
1. Ok, that means we can do without container for column transformations.

2. After talking with Blaise I realized that 
    <transformation name="normalHours" method="buildNormalHours">
or
    <transformation name="normalHours" class="NormalHoursBuilder">
is a bad idea: the user *must* specify one (and only one) of these two attributes - but it's not reflected in the schema.

So, taking it all into account, the mapping from Employee example (using a lot of defaults) will look like:
    <orm:transformation name="normalHours">
       <orm:attribute-method>buildNormalHours</orm:attribute-method>
       <orm:column-method name="getStartTime">
          <orm:column name="START_TIME"/>
       </orm:column-method>
       <orm:column-method  name="getEndTime">
          <orm:column name="END_TIME"/>
       </orm:column-method>
    </opm:transformation>

Note the names: attribute-method and column-method are looking uniform
(other possible elements: attribute-class for agttribute;
column-class and column-const for columns).

To make the mapping read-only - drop column transformers:
    <orm:transformation name="normalHours">
       <orm:attribute-method>buildNormalHours</orm:attribute-method>
    </opm:transformation>

However attribute trabsformer can't be dropped.
Comment 12 Andrei Ilitchev CLA 2008-01-30 10:47:51 EST
Updated to follow the write-method naming style of http://wiki.eclipse.org/EclipseLink/Development/200040/employee-map.xml
<transformation name="normalHours">
	<read-method name="buildNormalHours">
		<column name="START_TIME"/>
	</read-method>
	<write-method name="getStartTime">
		<column name="START_TIME"/>
	</write-method>
	<write-method name="getEndTime">
		<column name="END_TIME"/>
	</write-method>
</transformation>

Seems logical and simple to use "read-method" or "read-class" element for attribute transformer; "write-method", "write-class", "write-const" for column transformer.
Comment 13 Andrei Ilitchev CLA 2008-01-30 10:52:16 EST
I meant:
<transformation name="normalHours">
        <read-method name="buildNormalHours"/>
        <write-method name="getStartTime">
                <column name="START_TIME"/>
        </write-method>
        <write-method name="getEndTime">
                <column name="END_TIME"/>
        </write-method>
</transformation>
Comment 14 Andrei Ilitchev CLA 2008-01-31 15:59:39 EST
Have to change xml - can't define matching annotation.
The culprit is lack of inheritance in annotation world.

The best we can do with the current xml is:

        @Target({}) @Retention(RUNTIME)
        public @interface WriteMethod {
          String name;
          Column column;
          Return return default Return(NONE);
        }

        @Target({}) @Retention(RUNTIME)
        public @interface WriteClass {
          String name;
          Column column;
          Return return default Return(NONE);
        }

        @Target({METHOD, FIELD, CLASS}) @Retention(RUNTIME)
        public @interface Transformation {
          ...
          String readMethod default "";
          String readClass default "";
          WriteMethod[] writeMethods default {};
          WriteClass[] writeClasses default {};
        }

Note that the user can't specify both (or none of) readMethod and readClass (though the annotation allows that).

******************************************************

Here's the new proposed solution:

        public enum TransformerType {
          CLASS,
          METHOD
        }

        @Target({}) @Retention(RUNTIME)
        public @interface ReadTransformer {
          TransformerType type;
          String name;
        }

        @Target({}) @Retention(RUNTIME)
        public @interface WriteTransformer {
          TransformerType type;
          String name;
          Column column;
          Return return default Return(NONE);
        }

        @Target({METHOD, FIELD, CLASS}) @Retention(RUNTIME)
        public @interface Transformation {
          ...
          ReadTransformer readTransformer;
          WriteTransformer[] writeTransformers default {};
        }

        The annotation would look like:
        @Transformation {
          ...
          readTransformer=ReadTransformer(type=TransformerType.METHOD, name=buildNormalHours),
          writeTransformers={
            WriteTransformer(type=TransformerType.METHOD, name="getStartTime", column=Column(name=START_TIME)),
            WriteTransformer(type=TransformerType.METHOD, name="getEndTime", column=Column(name=END_TIME))
          }
        }

Xml will look like:
<transformation name="normalHours">
        <read-transformer type=TransformerType.METHOD name="buildNormalHours"/>
        <write-transformer type=TransformerType.METHOD name="getStartTime">
                <column name="START_TIME"/>
        </write-transformer>
        <write-transformer type=TransformerType.METHOD name="getEndTime">
                <column name="END_TIME"/>
        </write-transformer>
</transformation>

It look very big - the unfortunate drawback of annotations being defined "globally" - I would've used "@Write instaed of @WriteTransformer if that would've been confined to @Transformation context.
Comment 15 Doug Clarke CLA 2008-02-01 15:53:07 EST
I don't care much for the METHOD/CLASS enum approach as it complicates things and the resulting XML is far form clear. The annotation example is wrong with the ... and embedded annotations lacking the @. Please make additional examples complete.

I was not aware of the option to use AttributeTransformer on thre read and FieldTranformer on the write. I believe these cases should each be their own annotations and XML elements instead of qualifying them with the enum.

Comment 16 Michael Keith CLA 2008-02-04 12:04:34 EST
Some annotation suggestions from Mike & Doug:

Basic Employee Example using methods:

@ReadTransformer(method="buildNormalHours")
@WriteTranformers({
     @WriteTranformer(column=@Column(name="START_TIME"), method="getStartTime"),
     @WriteTranformer(column=@Column(name="END_TIME"), method="getEndTime")
}

USING AttributeTranfortmer and FieldTranformer classes:

@ReadTransformer(class=package.MyNormalHoursTransformer.class)
@WriteTranformers({
   @WriteTranformer(column=@Column(name="START_TIME"), 
      class=package.MyTimeTransformer.class),
   @WriteTranformer(column=@Column(name="END_TIME"), 
      class=package.MyTimeTransformer.class)
})

COMBINED EXAMPLE:

@ReadTransformer(transformerClass=package.MyNormalHoursTransformer.class)
@WriteTranformers({
   @WriteTranformer(column=@Column(name="START_TIME"), 
      method="getStartDate"),
   @WriteTranformer(column=@Column(name="END_TIME"), 
      class=package.MyTimeTransformer.class)
})
Comment 17 Doug Clarke CLA 2008-02-04 12:15:58 EST
Now to convert these same annotations in the corresponding XML I would suggest:

<transformation name="normalHours">
        <read method="buildNormalHours"/>
        <write method="getStartTime">
                <column name="START_TIME"/>
        </write>
        <write method="getEndTime">
                <column name="END_TIME"/>
        </write>
</transformation>

USING AttributeTranfortmer and FieldTranformer classes:

<transformation name="normalHours">
        <read method="buildNormalHours"/>
        <write class="package.MyTimeTransformer">
                <column name="START_TIME"/>
        </write>
        <write class="package.MyTimeTransformer">
                <column name="END_TIME"/>
        </write>
</transformation>

COMBINED EXAMPLE:

<transformation name="normalHours">
        <read method="buildNormalHours"/>
        <write method="getStartTime">
                <column name="START_TIME"/>
        </write>
        <write class="package.MyTimeTransformer">
                <column name="END_TIME"/>
        </write>
</transformation>
Comment 18 Andrei Ilitchev CLA 2008-02-04 13:13:56 EST
Two comments:

1. The suggested annotation:
@Target({}) @Retention(RUNTIME)
public @interface ReadTransformer {
  Class transformerClass default void.class;
  String method default "";
}
allows to specify both or none of the values - however only annotation with the single value specified is correct:
@ReadTransformer(method="buildNormalHours")
@ReadTransformer(class=package.MyNormalHoursTransformer.class)

The following will throw exception:
@ReadTransformer()
@ReadTransformer(class=package.MyNormalHoursTransformer.class, method="buildNormalHours")

Are we willing to live with that?

2. The suggested xml elements names "write" and "read" don't follow the naming pattern which (I think) we used to follow: annotation named "thisAndThat" corresponds to xml element named "this-and-that" (with the same going for attributes, too).

I like the simple xml elements' names a lot, but wouldn't it be confusing for the user:
ReadTransformer corresponding to <read>;
transformerClass corresponding to attribute "class"?

In case we decide to break the namimg pattern - what would be the rules?
Could we say that xml elements may have simpler names then their annotation counterparts in case those elements are defined internally in their master elements?
Comment 19 Andrei Ilitchev CLA 2008-02-05 12:52:04 EST
I was wrong in 2. in my prev. posting: the naming pattern for annotation methods / xml elements having similar names is still preserved:

@Transformation(
read=@ReadTransformer(method="buildNormalHours"),
write={
     @WriteTranformer(column=@Column(name="START_TIME"),
method="getStartTime", type="java.sql.Time"),
     @WriteTranformer(column=@Column(name="END_TIME"), 
method="getEndTime", type="java.sql.Time")
},
return=INSERT,
)

<transformation name="normalHours">
     <read method="buildNormalHours"/>
     <write method="getStartTime">
             <column name="START_TIME"/>
     </write>
     <write method="getEndTime">
             <column name="END_TIME"/>
     </write>
</transformation>

Annotation names should correspond to xml type names:
@Transformation <-> <transformation>
@WriteTransformer <-> <write-transformer>

Annotation's methods' names should correspond to element names:
"read" <-> <read>
"write" <-> <write>

Therefore
"ReadTransformer" doesn't correspond to <read> (but to <read-transformer>).
However "transformerClass" corresponding to attribute "class" - so would be nice to use the same name for both. However we may have a rule that allows name difference in case the name from one space ("class" from xml) can't be use in another (can't use "class: as a method name in annotation).
Comment 20 Doug Clarke CLA 2008-02-06 11:13:09 EST
Andrei,

As with JPA we are trying to avoid using nested annotations whenever possible. I prefer the proposed solution with @ReadTransform and @WriterTransformers/@WriteTransformer both being annotations on the field/property.

While we want an obvious link between the XML and the annotation we do not need to have the same hierarchy in annotations that we would have in XML.
Comment 21 Andrei Ilitchev CLA 2008-02-06 16:37:14 EST
Current state of transformation mapping in the xsd on my machine:

  <xsd:complexType name="read-transformer">
    <xsd:annotation>
      <xsd:documentation>

        @Target({}) @Retention(RUNTIME)
        public @interface ReadTransformer {
          Class transformerClass() default void.class;
          String method default "";
        }

      </xsd:documentation>
    </xsd:annotation>
    <xsd:attribute name="class" type="xsd:string" minOccurs="0"/>
    <xsd:attribute name="method" type="xsd:string" minOccurs="0"/>
  </xsd:complexType>

  <xsd:complexType name="write-transformer">
    <xsd:annotation>
      <xsd:documentation>

        @Target({}) @Retention(RUNTIME)
        public @interface WriteTransformer {
          Class transformerClass() default void.class;
          String method() default "";
          Column column();
          Class columnClass() default void.class; 
        }

      </xsd:documentation>
    </xsd:annotation>
    <xsd:sequence>
      <xsd:element name="column" type="orm:column/>
    </xsd:sequence>
    <xsd:attribute name="class" type="xsd:string" minOccurs="0"/>
    <xsd:attribute name="method" type="xsd:string" minOccurs="0"/>
    <xsd:attribute name="column-class" type="xsd:string" minOccurs="0"/>
  </xsd:complexType>

  <xsd:complexType name="transformation">
    <xsd:annotation>
      <xsd:documentation>

        @Target({METHOD, FIELD, CLASS}) @Retention(RUNTIME)
        public @interface Transformation {
          String name() default "";
          FetchType fetch() default EAGER;
          boolean optional() default true;
          boolean mutable() default true;
        }

      </xsd:documentation>
    </xsd:annotation>
    <xsd:sequence>
      <xsd:element name="read" type="orm:read-transformer"/>
      <xsd:element name="write" type="orm:write-transformer" minOccurs="0" maxOccurs="unbounded"/>
      <xsd:element name="property" type="orm:property" minOccurs="0" maxOccurs="unbounded"/> 
    </xsd:sequence>
    <xsd:attribute name="name" type="xsd:string"/>
    <xsd:attribute name="get-method" type="xsd:string"/>
    <xsd:attribute name="set-method" type="xsd:string"/>
    <xsd:attribute name="fetch" type="orm:fetch-type"/>
    <xsd:attribute name="optional" type="xsd:boolean"/>
    <xsd:attribute name="mutable" type="xsd:boolean"/>
    <xsd:attribute name="return" type="orm:return-mode" default="NONE"/>
  </xsd:complexType>


Open Issues.

1. Spec. has a pattern of using similar names for annotations and xml
("ThisAndThat" in annotations corresponds to <this-and-that> in xml).

Annotations naming suggested by Mike:

@ReadTransformer(transformerClass=package.MyNormalHoursTransformer.class)
@WriteTranformers({
   @WriteTranformer(column=@Column(name="START_TIME"), 
      method="getStartDate"),
   @WriteTranformer(column=@Column(name="END_TIME"), 
      class=package.MyTimeTransformer.class)
})

don't correspond in this way to xml names suggested by Doug:

<transformation name="normalHours">
     <read method="buildNormalHours"/>
     <write method="getStartTime">
             <column name="START_TIME"/>
     </write>
     <write method="getEndTime">
             <column name="END_TIME"/>
     </write>
</transformation>

That may be confusing for the users switching back and forth between annotations and xml.

Suggested solution: Change xml names (<read> to <read-transformer>).
Alternatively define some simple rules, like: 
Annotation's name should correspond to xml type name:
@WriteTransformer <-> <write-transformer>;
Annotation method's name should correspond to xml attribute name:
transformerClass() <-> transformer-class.


2. Does transformation mapping require the methods found in @Basic?
          FetchType fetch() default EAGER;
          boolean optional() default true;

3. Is @Transformation annotation required?
Pro: that's where we can put all the methods:
          FetchType fetch() default EAGER;
          boolean optional() default true;
          boolean mutable() default true;
          ReturnMode return() default NONE;
Contra: it's doesn't define anything unless (at least one) Transformer is specified, too.

Suggested solution: Don't need @Transformation, put the methods either into ReadTransformer (fetch) or WriteTransformers (optional, mutable, return).

4. ReturningPolicy requires java type to be specified by the user (in case of DirectToField mapping TopLink sets the type into DatabaseField, not so with transformation mapping). How to do it?
4.1 Add to WriteTransformer:
String columnType() default "" 
4.2 Define a comprehensive ColumnType annotation:
@ColumnType{
  String javaType() default "";
  int sqlType() default 0;
  ? String plSqlType() default "";
}
Comment 22 Andrei Ilitchev CLA 2008-02-08 16:47:59 EST
Here are the two examples (annotations and xml) that Doug wrote during the review meeting.

@Transformation(fetch=FecthType.LAZY, optional="true")
@ReadTransformer(class=package.MyNormalHoursTransformer.class)
@WriteTranformers({
   @WriteTranformer(column=@Column(name="START_TIME"), 
      method="getStartDate"),
   @WriteTranformer(column=@Column(name="END_TIME"), 
      class=package.MyTimeTransformer.class)
})
@Mutable
@ReturnUpdate
@Access(AccessType.PROPERTY)
@AccessMethods(get="getNormalHours", set="setNormalHours")
@Properties({
   @Property(name="x", value="y")
})

- @Id? 
- @Version?  
- @MappedSuperclass? yes


<transformation name="normalHours" fetch="LAZY" optional="true">
     <read-transformer method="buildNormalHours"/>
     <write-transformer method="getStartTime">
             <column name="START_TIME"/>
     </write-transformer>
     <write-transformer class="package.MyTimeTransformer">
             <column name="END_TIME"/>
     </write-transformer>
     <mutable/>
     <return-update/>
     <access type="PROPERTY"/>
     <access-methods get="getNormalHours" set="setNormalHours"/>
     <properties>
        <property name="x" value="y"/>
     </properties>
</transformation>

- Wait for JPA 2.0's solution for @Id & @Version approach
- <!-- Bug 217164 -->

*********************************************

The questions in the previous posting were answered as follows:
1. Spec. has a pattern of using similar names for annotations and xml
("ThisAndThat" in annotations corresponds to <this-and-that> in xml).

Keep the pattern, change xml elemnts' names
(<read> to <read-transformer>). 

2. Does transformation mapping require the methods found in @Basic?

Yes (see Doug's annotation example above).

3. Is @Transformation annotation required?

Yes (see Doug's annotation example above).

4. ReturningPolicy requires java type to be specified by the user (in case of
DirectToField mapping TopLink sets the type into DatabaseField, not so with
transformation mapping). How to do it?

Don't specify the types - will try to figure them out.
The user may set types in customizer if required.

************************************************

I would suggest not to apply ReturningPolicy to TransformationMapping at all because unless we can figure out how to make it work in Employee example.

************************************************

Defaults (suggested by Doug):
1. optional = true 
2. fetch=EAGER 
3. Write transformer using method will default to assumed column name if not provided (only in case there's a single WriteTransformer specified).
4. No read transformer defaults to do nothing during read 
5. No write transformers means read-only 
6. Returning policy column type defaults to that returned from accessor method of field transformer. 

I don't understand the last one (6.) - buildFieldValue of FieldTransformer returns object, so it's not going to help. We could *try* to use attribute type,  but will fail in Employee example (it's Time[]).

************************************************

Validation (suggested by Doug):
1. In both read and write transformer you can only have a method or a class;
2. When specifying a class in either read or write transformer it must be accessible and must implement the appropriate interface;
3. Read transformer method must implement correct signature.

There is a temptation to add a check for field uniqueness (can't have two WriteTransformers for a single field), but James says that this should be caught by TopLink's mapping validation.

Comment 23 Andrei Ilitchev CLA 2008-02-11 15:35:52 EST
Finally I understood Doug's position on ReturningPolicy with TransformationMapping (see 6 in Defaults in the prev. posting): in case ColumnTransformer is method based, ReturningPolicy can (and should) obtain the field type(s) automatically (logged https://bugs.eclipse.org/bugs/show_bug.cgi?id=218554)

Also add to Validation:
4. Can't have returning policy if it's a read-only mapping (no ColumnWriters specified).
Comment 24 Andrei Ilitchev CLA 2008-03-07 10:15:48 EST
Created attachment 91880 [details]
The proposed patch.

The proposed patch doesn't include ReturningPolicy, access methods, properties - those items also relevant for other mappings and should be dealt with separately.
Comment 25 Andrei Ilitchev CLA 2008-03-07 15:51:27 EST
Created attachment 91935 [details]
The proposed patch, please disregard the prev. one.

Had to make a new patch after attempted to merge the previous one - some essential changes happened in the repository (some classes changed packages).
Comment 26 Andrei Ilitchev CLA 2008-03-07 16:01:59 EST
Checked in the patch.
Now TransformationMapping could be specified in eclipselink jpa using either annotations or xml.
Tests:
  testMethodBasedTransformationMapping and testClassBasedTransformationMapping;
for annotations found in:
  AdvancedJPAJunitTest;
for xml:
  EntityMappingsAdvancedJUnitTestCase
Comment 27 Andrei Ilitchev CLA 2008-03-11 17:52:06 EDT
Created attachment 92239 [details]
the patch implements Guy's comments.

The previous patch has been already checked in. This one should be applied on top of it. This patch addresses the following feedback from Guy:
Andrei,
 
A few things:
 
1 - You need to change the Contributor comment for new files (including testing files). They should look something like this
 
  Contributors:
    Andrei Ilitchev (Oracle), March 10, 2008
      - New file introduced for bug 221658
 
2 - Also update any @since tags as well. New files should not be
  @since Toplink EJB 3.0 Reference Implementation 
   but rather
  @since EclipseLink 1.0 
 
3 - You need to document where your new annotations can be placed. That is, valid within an @Entity, @MappedSuperclass or @Embeddable?
 
4 - WriteTransformerMetadata and ReadTransformerMetadata need more commenting, I didn't seen any for those classes.
 
5 - I would move the isTransformation check from ClassAccessor - buildAccessor before the isOneToMany and isOneToOne checks since they can default. Avoid the off-case where a transformation mapping could accidently default into one of those mappings.
 
6 - I think but not sure (double checking with Matt McIvor) but mappings in the XMLEntityMappingsMappingProject must be added in the same order they are defined in the schema. Meaning your transformationsMapping should be added after the transientsMapping. Although I think in this case it would make more sense to define the mapping in the schema before the transient and leave the mapping file as is.
 
7 - You need to use reflective calls when access metadata from annotations. Meaning you will need a new MetadataHelper file (just copy one from some other package) and use the invoke method to extract info from the Annotation. 
 
Example: (see the other accessors for more info)
 
// Set the mutable value if one is present.
Annotation mutable = getAnnotation(Mutable.class);
if (mutable != null) {
  m_mutable = (Boolean) MetadataHelper.invokeMethod("value", mutable);
}
 
8 - XMLEntityMappingsMappingProject:
 
Line 1490 is incorrect: transformerClassNameMapping.setAttributeName("m_TransformerClassName");
should be: transformerClassNameMapping.setAttributeName("m_transformerClassName"); (notice the lower case T)
 
Line 1876 is also incorrect: transformerClassNameMapping.setAttributeName("m_TransformerClassName");
should be: transformerClassNameMapping.setAttributeName("m_transformerClassName");
 
Better still, you should also make these mappings (from both Read and Write transformer) into getMethods and call them when building the descriptors
 
XMLDirectMapping transformerClassNameMapping = new XMLDirectMapping();
transformerClassNameMapping.setAttributeName("m_TransformerClassName");
transformerClassNameMapping.setGetMethodName("getTransformerClassName");
transformerClassNameMapping.setSetMethodName("setTransformerClassName");
transformerClassNameMapping.setXPath("@transformer-class");
descriptor.addMapping(transformerClassNameMapping);
 
XMLDirectMapping methodMapping = new XMLDirectMapping();
methodMapping.setAttributeName("m_method");
methodMapping.setGetMethodName("getMethod");
methodMapping.setSetMethodName("setMethod");
methodMapping.setXPath("@method");
descriptor.addMapping(methodMapping);
 
9 - The documentation that is included in the annotation should also be included in the schema in the <xsd:documentation> tag.
 
Other than that I think it's fine.
 
Thanks,
Guy

----------------
Sorry one more thing:
 
m_transformerClassName will need to be initialized through its owning XMLEntityMappings object so that it may take a <package> specification into consideration.
 
I can show how to do this when you are making the changes.
 
Cheers,
Guy
Comment 28 Andrei Ilitchev CLA 2008-03-11 17:52:15 EDT
Created attachment 92240 [details]
the patch implements Guy's comments.

The previous patch has been already checked in. This one should be applied on top of it. This patch addresses the following feedback from Guy:
Andrei,
 
A few things:
 
1 - You need to change the Contributor comment for new files (including testing files). They should look something like this
 
  Contributors:
    Andrei Ilitchev (Oracle), March 10, 2008
      - New file introduced for bug 221658
 
2 - Also update any @since tags as well. New files should not be
  @since Toplink EJB 3.0 Reference Implementation 
   but rather
  @since EclipseLink 1.0 
 
3 - You need to document where your new annotations can be placed. That is, valid within an @Entity, @MappedSuperclass or @Embeddable?
 
4 - WriteTransformerMetadata and ReadTransformerMetadata need more commenting, I didn't seen any for those classes.
 
5 - I would move the isTransformation check from ClassAccessor - buildAccessor before the isOneToMany and isOneToOne checks since they can default. Avoid the off-case where a transformation mapping could accidently default into one of those mappings.
 
6 - I think but not sure (double checking with Matt McIvor) but mappings in the XMLEntityMappingsMappingProject must be added in the same order they are defined in the schema. Meaning your transformationsMapping should be added after the transientsMapping. Although I think in this case it would make more sense to define the mapping in the schema before the transient and leave the mapping file as is.
 
7 - You need to use reflective calls when access metadata from annotations. Meaning you will need a new MetadataHelper file (just copy one from some other package) and use the invoke method to extract info from the Annotation. 
 
Example: (see the other accessors for more info)
 
// Set the mutable value if one is present.
Annotation mutable = getAnnotation(Mutable.class);
if (mutable != null) {
  m_mutable = (Boolean) MetadataHelper.invokeMethod("value", mutable);
}
 
8 - XMLEntityMappingsMappingProject:
 
Line 1490 is incorrect: transformerClassNameMapping.setAttributeName("m_TransformerClassName");
should be: transformerClassNameMapping.setAttributeName("m_transformerClassName"); (notice the lower case T)
 
Line 1876 is also incorrect: transformerClassNameMapping.setAttributeName("m_TransformerClassName");
should be: transformerClassNameMapping.setAttributeName("m_transformerClassName");
 
Better still, you should also make these mappings (from both Read and Write transformer) into getMethods and call them when building the descriptors
 
XMLDirectMapping transformerClassNameMapping = new XMLDirectMapping();
transformerClassNameMapping.setAttributeName("m_TransformerClassName");
transformerClassNameMapping.setGetMethodName("getTransformerClassName");
transformerClassNameMapping.setSetMethodName("setTransformerClassName");
transformerClassNameMapping.setXPath("@transformer-class");
descriptor.addMapping(transformerClassNameMapping);
 
XMLDirectMapping methodMapping = new XMLDirectMapping();
methodMapping.setAttributeName("m_method");
methodMapping.setGetMethodName("getMethod");
methodMapping.setSetMethodName("setMethod");
methodMapping.setXPath("@method");
descriptor.addMapping(methodMapping);
 
9 - The documentation that is included in the annotation should also be included in the schema in the <xsd:documentation> tag.
 
Other than that I think it's fine.
 
Thanks,
Guy

----------------
Sorry one more thing:
 
m_transformerClassName will need to be initialized through its owning XMLEntityMappings object so that it may take a <package> specification into consideration.
 
I can show how to do this when you are making the changes.
 
Cheers,
Guy
Comment 29 Andrei Ilitchev CLA 2008-03-12 11:22:23 EDT
Created attachment 92326 [details]
the patch implementing Guy's comments - take 2.

Recieved feedback from Guy:

Because it's a security sensitive method that we do not want to expose to users.
----- Original Message ----- 
From: Andrei Ilitchev 
To: Guy Pelletier 
Sent: Wednesday, March 12, 2008 9:32 AM
Subject: Re: Code for package qualification


>You can not change the visibility of the invokeMethod from MetadataHelper in accessors.mappings
Why?
----- Original Message ----- 
From: Guy Pelletier 
To: Andrei Ilitchev 
Sent: Wednesday, March 12, 2008 9:20 AM
Subject: Re: Code for package qualification


Looks good, only one problem.
 
You can not change the visibility of the invokeMethod from MetadataHelper in accessors.mappings. It must remain package. You need to create a new MetadataHelper with the same method in the metadata.transformers package and use it instead.
 
Cheers,
Guy
Comment 30 Andrei Ilitchev CLA 2008-03-12 11:54:09 EDT
The latest patch was blessed by Guy and I checked it in.
Comment 31 Eclipse Webmaster CLA 2022-06-09 10:26:52 EDT
The Eclipselink project has moved to Github: https://github.com/eclipse-ee4j/eclipselink