JDO
  1. JDO
  2. JDO-709

Standardize field/property converters

    Details

      Description

      This request is to standardize a user's ability to specify conversions of fields or properties of persistence-capable classes. Currently, this is left to vendor extensions.

      1. JDO-709-4.patch
        5 kB
        Andy Jefferson
      2. JDO-709-3.patch
        7 kB
        Andy Jefferson
      3. JDO-709-01.patch
        7 kB
        Matthew T. Adams

        Activity

        Hide
        Craig L Russell added a comment -

        > @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter."
        > I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion".
        > Which is the confusion between annotation default and default behavior.
        Indeed. It is misleading. The "converter" attribute default says use the "default conversion", whilst the boolean "useDefaultConversion" says DON'T use the "default conversion".

        I'm willing to live with the potential confusion since no one will ever explicitly use converter(UseDefault.class).

        The code at https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/JDOTypeConverterUtils.java
        works fine for cases I've used.

        Thanks for the example. I don't see anything wrong with this approach.

        Show
        Craig L Russell added a comment - > @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter." > I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion". > Which is the confusion between annotation default and default behavior. Indeed. It is misleading. The "converter" attribute default says use the "default conversion", whilst the boolean "useDefaultConversion" says DON'T use the "default conversion". I'm willing to live with the potential confusion since no one will ever explicitly use converter(UseDefault.class). The code at https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/JDOTypeConverterUtils.java works fine for cases I've used. Thanks for the example. I don't see anything wrong with this approach.
        Hide
        Andy Jefferson added a comment -

        > @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter."
        > I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion".
        > Which is the confusion between annotation default and default behavior.

        Indeed. It is misleading. The "converter" attribute default says use the "default conversion", whilst the boolean "useDefaultConversion" says DON'T use the "default conversion".

        > At runtime, I don't think the template types are available, as they have been erased.
        > How does the implementation figure out the A and D of AttributeConverter<A, D>? Do we need to add ...

        The code at https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/JDOTypeConverterUtils.java
        works fine for cases I've used. For example if I have a converter

        public class CurrencyAttrConverter implements AttributeConverter<Currency, String>
        {
        public Currency convertToAttribute(String str)
        {
        if (str == null)

        { return null; }

        return java.util.Currency.getInstance(str.substring(5));
        }

        public String convertToDatastore(Currency curr)

        { return curr != null ? ("CURR:" + curr.toString()) : null; }

        }

        then use of those methods returns the attribute type as Currency, and the dbType as String.

        Show
        Andy Jefferson added a comment - > @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter." > I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion". > Which is the confusion between annotation default and default behavior. Indeed. It is misleading. The "converter" attribute default says use the "default conversion", whilst the boolean "useDefaultConversion" says DON'T use the "default conversion". > At runtime, I don't think the template types are available, as they have been erased. > How does the implementation figure out the A and D of AttributeConverter<A, D>? Do we need to add ... The code at https://github.com/datanucleus/datanucleus-api-jdo/blob/master/src/main/java/org/datanucleus/api/jdo/JDOTypeConverterUtils.java works fine for cases I've used. For example if I have a converter public class CurrencyAttrConverter implements AttributeConverter<Currency, String> { public Currency convertToAttribute(String str) { if (str == null) { return null; } return java.util.Currency.getInstance(str.substring(5)); } public String convertToDatastore(Currency curr) { return curr != null ? ("CURR:" + curr.toString()) : null; } } then use of those methods returns the attribute type as Currency, and the dbType as String.
        Hide
        Craig L Russell added a comment -

        I'm having trouble understanding how the implementation can process the Converters annotation. The value is a list of Convert annotations and the implementation is supposed to figure out to which domain type the values (AttributeConverter) apply.

        At runtime, I don't think the template types are available, as they have been erased. How does the implementation figure out the A and D of AttributeConverter<A, D>? Do we need to add

        Class<A> getAttributeClass() and
        Class<D> getDatabaseClass()

        to the interface requirements of AttributeConverter?

        Show
        Craig L Russell added a comment - I'm having trouble understanding how the implementation can process the Converters annotation. The value is a list of Convert annotations and the implementation is supposed to figure out to which domain type the values (AttributeConverter) apply. At runtime, I don't think the template types are available, as they have been erased. How does the implementation figure out the A and D of AttributeConverter<A, D>? Do we need to add Class<A> getAttributeClass() and Class<D> getDatabaseClass() to the interface requirements of AttributeConverter?
        Hide
        Craig L Russell added a comment -

        Some of the confusion is the use of the term "default" for annotation defaults and behavior defaults.

        Maybe an example will help me understand better, using the annotation default value for converter(UseDefault.class) and useDefaultConversion(false):

        Case 1:
        @Persistent(column("ss_name")),
        or what is seen by the implementation with default values for annotation:
        @Persistent(column("ss_name"), converter(UseDefault.class), useDefaultConversion(false))
        Standard behavior: in order, use the class converter for the type, or the PMF converter, or the default conversion

        Case 2:
        @Persistent(column("ss_name"), useDefaultConversion(true)),
        or what is seen by the implementation with default values for annotation:
        @Persistent(column("ss_name"), converter(UseDefault.class), useDefaultConversion(true))
        use the default conversion regardless of whether a converter is specified at the class or PMF

        Case 3:
        @Persistent(column("ss_name"), converter(SpecialConverter.class)),
        or what is seen by the implementation with default values for annotation:
        @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(false))
        use the special converter specified

        Case 4:
        @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(true)),
        or what is seen by the implementation with default values for annotation:
        @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(true))
        error at the time this class is loaded

        @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter."
        I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion". Which is the confusion between annotation default and default behavior.

        Show
        Craig L Russell added a comment - Some of the confusion is the use of the term "default" for annotation defaults and behavior defaults. Maybe an example will help me understand better, using the annotation default value for converter(UseDefault.class) and useDefaultConversion(false): Case 1: @Persistent(column("ss_name")), or what is seen by the implementation with default values for annotation: @Persistent(column("ss_name"), converter(UseDefault.class), useDefaultConversion(false)) Standard behavior: in order, use the class converter for the type, or the PMF converter, or the default conversion Case 2: @Persistent(column("ss_name"), useDefaultConversion(true)), or what is seen by the implementation with default values for annotation: @Persistent(column("ss_name"), converter(UseDefault.class), useDefaultConversion(true)) use the default conversion regardless of whether a converter is specified at the class or PMF Case 3: @Persistent(column("ss_name"), converter(SpecialConverter.class)), or what is seen by the implementation with default values for annotation: @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(false)) use the special converter specified Case 4: @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(true)), or what is seen by the implementation with default values for annotation: @Persistent(column("ss_name"), converter(SpecialConverter.class), useDefaultConversion(true)) error at the time this class is loaded @Andy: "If we [default the useDefaultConversion to] "false" then that implies it will need a provided converter." I think the only confusion is in case 1 where it may sound like both "use default converter class" and "don't use default conversion". Which is the confusion between annotation default and default behavior.
        Hide
        Andy Jefferson added a comment - - edited

        +1 on the rename of the default annotation "converter".

        Yes the boolean means what you say (and no problem with the name you proposed), the only thing is that in an annotation it needs a default value. If we put "false" then that implies it will need a provided converter. If we put "true" then that implies it will have to be set to "false" if the user wants to specify a converter of their own (as well as specifying the converter attribute). An alternate would be to NOT have as a boolean (like we did for all other annotations), and instead have as String ... then the default would be "", and your rules would work.

        Show
        Andy Jefferson added a comment - - edited +1 on the rename of the default annotation "converter". Yes the boolean means what you say (and no problem with the name you proposed), the only thing is that in an annotation it needs a default value. If we put "false" then that implies it will need a provided converter. If we put "true" then that implies it will have to be set to "false" if the user wants to specify a converter of their own (as well as specifying the converter attribute). An alternate would be to NOT have as a boolean (like we did for all other annotations), and instead have as String ... then the default would be "", and your rules would work.
        Hide
        Craig L Russell added a comment -

        Apologies for the null annotation issue. I'll fall on my sword.

        My biggest concern is the naming of disableConversion. It really means "use Implementation Conversion" since there is always a conversion. It might help if we talk about "default" to mean only the JDO specified implementation default. Everything else is an override, which can be specified at the PMF level, the class level, and the field (key and value as well) level.

        I guess it would be clearer for me to have "useDefaultConversion" in the annotation. You can override the default conversion either at the PMF, the class, or the field/key/value level.

        1. If neither is specified on the the annotation: -> use class-level converter if available, else use PMF converter if available, otherwise fallback to default implementation handling.
        2. If only the useDefaultConversion is specified (true): -> use default implementation handling
        3. If only the converter is specified as converter(SpecialConverter.class): -> use converter
        4. If both the converter is specified as converter(SpecialConverter.class), useDefaultConversion(true): -> this is an error and throws an exception during annotation processing.

        Along the same lines, how about changing the name of the "marker" converter:

        + public static class UseDefault implements AttributeConverter<Object, Object>
        + {
        + public Object convertToDatastore(Object attributeValue)

        { + throw new JDOUserException("This converter is not usable."); + }
        +
        + public Object convertToAttribute(Object datastoreValue) { + throw new JDOUserException("This converter is not usable."); + }

        + }

        Show
        Craig L Russell added a comment - Apologies for the null annotation issue. I'll fall on my sword. My biggest concern is the naming of disableConversion. It really means "use Implementation Conversion" since there is always a conversion. It might help if we talk about "default" to mean only the JDO specified implementation default. Everything else is an override, which can be specified at the PMF level, the class level, and the field (key and value as well) level. I guess it would be clearer for me to have "useDefaultConversion" in the annotation. You can override the default conversion either at the PMF, the class, or the field/key/value level. 1. If neither is specified on the the annotation: -> use class-level converter if available, else use PMF converter if available, otherwise fallback to default implementation handling. 2. If only the useDefaultConversion is specified (true): -> use default implementation handling 3. If only the converter is specified as converter(SpecialConverter.class): -> use converter 4. If both the converter is specified as converter(SpecialConverter.class), useDefaultConversion(true): -> this is an error and throws an exception during annotation processing. Along the same lines, how about changing the name of the "marker" converter: + public static class UseDefault implements AttributeConverter<Object, Object> + { + public Object convertToDatastore(Object attributeValue) { + throw new JDOUserException("This converter is not usable."); + } + + public Object convertToAttribute(Object datastoreValue) { + throw new JDOUserException("This converter is not usable."); + } + }
        Hide
        Andy Jefferson added a comment - - edited

        As said above, you cannot do "default null" (my comment "crapness of Java annotation defaulting"). Maybe get those Java language spec people at Oracle to allow it? As also said above, an option is just to have the "converter" attribute as type Class and throw away generics, and you can then have "default void.class".

        Regarding "disableConversion" and "converter", yes it is not perfect how they interact.
        We have the following ways of handling a member (or key/value/element) :

        • default JDO implementation (internal) handling (what people get with JDO 3.1 or earlier)
        • PMF default converter for the type
        • optional class-level default converter for the type
        • optional member/element/key/value converter for the type

        One way we could interpret it all
        1. converter not specified, disableConversion = false -> use class-level converter if available, else use PMF converter if available, otherwise fallback to default implementation handling.
        2. converter not specified, disableConversion = true -> use default implementation handling
        3. converter specified, disableConversion = false -> use converter
        4. converter specified, disableConversion = true -> use default implementation handling

        Better ideas?

        Show
        Andy Jefferson added a comment - - edited As said above, you cannot do "default null" (my comment "crapness of Java annotation defaulting"). Maybe get those Java language spec people at Oracle to allow it? As also said above, an option is just to have the "converter" attribute as type Class and throw away generics, and you can then have "default void.class". Regarding "disableConversion" and "converter", yes it is not perfect how they interact. We have the following ways of handling a member (or key/value/element) : default JDO implementation (internal) handling (what people get with JDO 3.1 or earlier) PMF default converter for the type optional class-level default converter for the type optional member/element/key/value converter for the type One way we could interpret it all 1. converter not specified, disableConversion = false -> use class-level converter if available, else use PMF converter if available, otherwise fallback to default implementation handling. 2. converter not specified, disableConversion = true -> use default implementation handling 3. converter specified, disableConversion = false -> use converter 4. converter specified, disableConversion = true -> use default implementation handling Better ideas?
        Hide
        Craig L Russell added a comment -

        Do we really need NullAttributeConverter? It seems pointless. Was this perhaps supposed to be default null?

        The Persistent disableConversion seems awkward. If you specify a Converter and then disableConversion what happens? Seems like an error.

        + * Whether we should disable any conversion specified at the PMF level (where converter is not specified).
        + * @return Whether PMF attribute conversion is to be disabled.

        What does "PMF level (where converter is not specified)" mean?

        Show
        Craig L Russell added a comment - Do we really need NullAttributeConverter? It seems pointless. Was this perhaps supposed to be default null? The Persistent disableConversion seems awkward. If you specify a Converter and then disableConversion what happens? Seems like an error. + * Whether we should disable any conversion specified at the PMF level (where converter is not specified). + * @return Whether PMF attribute conversion is to be disabled. What does "PMF level (where converter is not specified)" mean?
        Hide
        Andy Jefferson added a comment -

        Patch for allowing specification of converter (and disable-converter) for field, interface, key, value, element for Metadata API

        Show
        Andy Jefferson added a comment - Patch for allowing specification of converter (and disable-converter) for field, interface, key, value, element for Metadata API
        Hide
        Andy Jefferson added a comment -

        Patch adding "converter" attribute to @Persistent, @Element, @Key, @Value annotations so we can specify the converter more easily via annotations (for Map keys/values, Collection elements, and embedded fields) without having to resort to the previous "name" (which is removed in this patch since not necessary)

        Show
        Andy Jefferson added a comment - Patch adding "converter" attribute to @Persistent, @Element, @Key, @Value annotations so we can specify the converter more easily via annotations (for Map keys/values, Collection elements, and embedded fields) without having to resort to the previous "name" (which is removed in this patch since not necessary)
        Hide
        Andy Jefferson added a comment -

        XML schema updates now in SVN.

        Something that would make the annotation route much better for handling embedded fields is to be able to specify the converter in @Persistent, @Element, @Key, @Value (so we can get rid of the "name" attribute from @Convert - seems this was copied from JPA where they have no concept of key or value metadata). The problem with this is the crapness of Java annotation defaulting, where we can't have a default of null. That is, it would be nice to add this to @Persistent etc

        Class<? extends AttributeConverter> converter() default null;

        we can't do that, and we can't use "void.class" either due to the generics usage on the Class.

        My suggestion is that we create an arbitrary NullAttributeConverter class that is then specified as the default (and an implementation can check for that and ignore it if so). Opinions?

        Show
        Andy Jefferson added a comment - XML schema updates now in SVN. Something that would make the annotation route much better for handling embedded fields is to be able to specify the converter in @Persistent, @Element, @Key, @Value (so we can get rid of the "name" attribute from @Convert - seems this was copied from JPA where they have no concept of key or value metadata). The problem with this is the crapness of Java annotation defaulting, where we can't have a default of null. That is, it would be nice to add this to @Persistent etc Class<? extends AttributeConverter> converter() default null; we can't do that, and we can't use "void.class" either due to the generics usage on the Class. My suggestion is that we create an arbitrary NullAttributeConverter class that is then specified as the default (and an implementation can check for that and ignore it if so). Opinions?
        Hide
        Andy Jefferson added a comment -

        Patch that extends/replaces the previous xml patch and adds
        "disable-converter" on <field>, <property>, <key>, <value>, <element>
        "disable-all-converters" on <class>, <interface>

        Show
        Andy Jefferson added a comment - Patch that extends/replaces the previous xml patch and adds "disable-converter" on <field>, <property>, <key>, <value>, <element> "disable-all-converters" on <class>, <interface>
        Hide
        Craig L Russell added a comment -

        Three cases to consider for disable converter:

        1. Use a specific converter not the default. This is easy. Specify a Convert annotation that overrides the default.

        2. Don't use the pmf converter for an element. Use the standard converter. Here we need a "disable pmf converter" on the element.

        3. Don't use the pmf converters for any elements in a class or interface. Here we need a "disable pmf converter" on the class or interface. Any elements that cannot use the default converter need to have a specific converter on the element. For example, if there is a converter that converts database strings to dates, if all pmf converters are disabled on a class, the field needs to have its own converter specified.

        Show
        Craig L Russell added a comment - Three cases to consider for disable converter: 1. Use a specific converter not the default. This is easy. Specify a Convert annotation that overrides the default. 2. Don't use the pmf converter for an element. Use the standard converter. Here we need a "disable pmf converter" on the element. 3. Don't use the pmf converters for any elements in a class or interface. Here we need a "disable pmf converter" on the class or interface. Any elements that cannot use the default converter need to have a specific converter on the element. For example, if there is a converter that converts database strings to dates, if all pmf converters are disabled on a class, the field needs to have its own converter specified.
        Hide
        Andy Jefferson added a comment -

        Initial patch for updating DTD/XSD. Note only includes jdo.dtd/jdo.xsd since what is in there will be copied to orm.dtd/orm.xsd when accepted.

        Current idea is that we have "converter" attribute on <field>, <property>, <key>, <value> and <element> allowing user to specify a converter for the type of that member (or container member). Likely also need an attribute "disable-converter" (true|false) for cases where the PMF has a converter default defined for a java type and in the particular case of some class/field we don't want to use the default.

        Show
        Andy Jefferson added a comment - Initial patch for updating DTD/XSD. Note only includes jdo.dtd/jdo.xsd since what is in there will be copied to orm.dtd/orm.xsd when accepted. Current idea is that we have "converter" attribute on <field>, <property>, <key>, <value> and <element> allowing user to specify a converter for the type of that member (or container member). Likely also need an attribute "disable-converter" (true|false) for cases where the PMF has a converter default defined for a java type and in the particular case of some class/field we don't want to use the default.
        Hide
        Craig L Russell added a comment -

        @Andy: Yes, if a global change to a type is needed, it can't simply be an annotation but has to be defined at the PMF level.

        If the global converter isn't appropriate for a specific class then that class can be annotated with a different converter.

        Show
        Craig L Russell added a comment - @Andy: Yes, if a global change to a type is needed, it can't simply be an annotation but has to be defined at the PMF level. If the global converter isn't appropriate for a specific class then that class can be annotated with a different converter.
        Hide
        Andy Jefferson added a comment -

        Re: Converter, that was simply to allow some form of defaulting mechanism for handling fields of a type (from JPA) hence avoiding the need to specify @Convert on every field of a type. Actually there is nothing in JPA to prevent the user from (incorrectly) defining multiple (autoApply) @Converter for a particular java type ... hence a better way of achieving this is desirable, i'm open to ideas.

        Maybe (instead of @Converter) we could have properties you can specify on PMF startup like "javax.jdo.option.typeconverter.

        {XXX}

        " (where XXX is the fully-qualified type name of the java type) and set this to the AttributeConverter class name that is the default for that type (and is used if present in the CLASSPATH and no @Convert is specified against a field of that type).

        Show
        Andy Jefferson added a comment - Re: Converter, that was simply to allow some form of defaulting mechanism for handling fields of a type (from JPA) hence avoiding the need to specify @Convert on every field of a type. Actually there is nothing in JPA to prevent the user from (incorrectly) defining multiple (autoApply) @Converter for a particular java type ... hence a better way of achieving this is desirable, i'm open to ideas. Maybe (instead of @Converter) we could have properties you can specify on PMF startup like "javax.jdo.option.typeconverter. {XXX} " (where XXX is the fully-qualified type name of the java type) and set this to the AttributeConverter class name that is the default for that type (and is used if present in the CLASSPATH and no @Convert is specified against a field of that type).
        Hide
        Craig L Russell added a comment -

        I think we need an example of how the Converter annotation would be used.

        Show
        Craig L Russell added a comment - I think we need an example of how the Converter annotation would be used.
        Hide
        Andy Jefferson added a comment -

        Majority of the patch was applied, with some changes
        1. Apache copyright added
        2. Omitted the package-level capabilities since we need to get something for 95% of the use-cases first (of which package is not). We can always extend to that in the future.
        3. Renamed "Converters" to "Converts" and added Converter annotation to allow for "autoApply" setting a default of whether a converter should default to being applied (following JPA converters).

        Show
        Andy Jefferson added a comment - Majority of the patch was applied, with some changes 1. Apache copyright added 2. Omitted the package-level capabilities since we need to get something for 95% of the use-cases first (of which package is not). We can always extend to that in the future. 3. Renamed "Converters" to "Converts" and added Converter annotation to allow for "autoApply" setting a default of whether a converter should default to being applied (following JPA converters).
        Hide
        Andy Jefferson added a comment -

        Patch looks fine to me.
        RI already supports the basic idea so will be trivial to enable this.
        Clearly we also need the XML updating, but that can come next.

        Show
        Andy Jefferson added a comment - Patch looks fine to me. RI already supports the basic idea so will be trivial to enable this. Clearly we also need the XML updating, but that can come next.
        Hide
        Matthew T. Adams added a comment -

        Initial thoughts. Including revision number in patch filename, as I expect we'll iterate on this.

        Show
        Matthew T. Adams added a comment - Initial thoughts. Including revision number in patch filename, as I expect we'll iterate on this.

          People

          • Assignee:
            Matthew T. Adams
            Reporter:
            Matthew T. Adams
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development