jsr338-experts@jpa-spec.java.net

[jsr338-experts] Re: updated spec draft (converters)

From: Linda DeMichiel <linda.demichiel_at_oracle.com>
Date: Wed, 14 Mar 2012 12:14:04 -0700

On 3/14/2012 12:09 PM, michael keith wrote:
> That would certainly be consistent.
> Not as convenient, but definitely consistent :-)
>
> Another approach would be to go back to defining the annotation on an entity (i.e. like named queries, id generators and
> many other global things that we already define) and point to the converter class.
>

That seems to me much less convenient. Unlike named queries (which are just strings,
rather than objects), we don't need a class to attach converters to in order to
define them outside of XML.


> Example:
>
> @Converter(class=MyConverter.class autoApply=true)
> @Entity
> public class Employee { ... }
>
>
> On 14/03/2012 2:36 PM, Linda DeMichiel wrote:
>> Currently @Converter has only one attribute, autoApply, although
>> we have open issues around scoping (extending to subclasses, etc).
>>
>> It occurs to me that we could approach autoApply converters as we
>> do default entity listeners, and require them to be listed in the
>> orm.xml. If we did this, we could dispense with the @Converter
>> annotation until such time as we needed it to further refine the
>> semantics.
>>
>> Opinions? Fire away :-)
>>
>> -Linda
>>
>>
>> On 3/14/2012 10:37 AM, Gordon Yorke wrote:
>>>
>>>
>>> On 14/03/2012 1:49 PM, michael keith wrote:
>>>>
>>>> On 14/03/2012 12:18 PM, Gordon Yorke wrote:
>>>>> On 14/03/2012 1:11 PM, michael keith wrote:
>>>>>>
>>>>>>>>>>> I've uploaded a draft of the spec with the attribute converter additions to the document
>>>>>>>>>>> downloads area, http://java.net/projects/jpa-spec/downloads.
>>>>>>>>>>>
>>>>>>>>>>> The converter changes can be found in sections 3.7, 10.5, and 11.1.10-11.
>>>>>>>>>>>
>>>>>>>>>>> The following open issues are pending:
>>>>>>>>>>>
>>>>>>>>>>> * Conversion of @Id and @Version.
>>>>>>>>>>>
>>>>>>>>>>> * Explicit listing of converters in persistence.xml file. I'm not sure I understand what
>>>>>>>>>>> was being proposed here.
>>>>>>>>>>
>>>>>>>>>> When the archive is not scanned (it's an option in persistence.xml), the provider has no way of finding the list
>>>>>>>>>> of converters unless the user explicitly list them in persistence.xml like it does list classes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> OK, thanks -- so would you propose that they simply be listed using the class element?
>>>>>>>>
>>>>>>>> These are O/R mapping classes. Why would they be listed in persistence.xml? The flag to not scan for annotations
>>>>>>>> (the xml-mapping-metadata-complete option) is in the orm.xml mapping file, that is where I would expect to specify
>>>>>>>> these when not using annotations, right?
>>>>>>> but the flag that controls scanning for persistence related classes is in the persistence.xml file as is the list
>>>>>>> of persistence related classes. By default the provider is not allowed to scan for classes. Many deployments occur
>>>>>>> without any orm.xml.
>>>>>> There is an exclude-unlisted-classes setting in the persistence.xml file that *disables* scanning for entities,
>>>>>> embeddables and mapped superclasses, but by default the provider is supposed to scan (when running in a container -
>>>>>> outside the container it is not defined). This option was only added to enable a persistence unit to disable some
>>>>>> classes from being considered even though they were in the same JAR.
>>>>>> In any case, my point was that this is an object-relational mapping class and by design we have always tried to keep
>>>>>> the O/R mapping concepts in the orm.xml file (when we are talking about not relying upon the annotation sensing,
>>>>>> which I believe was the point).
>>>>> exclude-unlisted-classes is true by default
>>>>
>>>> No, by default it is false (but I hope that is what you mean?)
>>> No, the default is true : "<xsd:element name="exclude-unlisted-classes" type="xsd:boolean" default="true"
>>> minOccurs="0">" and portable Java SE applications must list all managed persistence classes (section 8.2.1.6.4) .
>>>>
>>>>> and so by default scanning by the provider must be *enabled*.
>>>>
>>>> Correct.
>>>>
>>>>> The scanning the container performs is defined elsewhere.
>>>>
>>>> We're not talking about container scanning, just provider scanning.
>>>>
>>>>> The only thing being discussed here is the auto scanning and how the provider should be notified of the converter
>>>>> classes when scanning is not enabled.
>>>>
>>>> Yes. So far you have mostly just restated what I said above, so we're good up to this point :-)
>>> No, we disagree on whether @Converter classes will always be discovered. Emmanuel's point and my point is that by
>>> default in Java SE there is no way for a provider to find the *annotated* auto-apply Converter classes. Users should
>>> have the option of listing the annotated Converter classes within the list of managed classes in the persistence.xml
>>> file.
>>>>
>>>>> Requiring an orm.xml file just to list the converter classes seems out of place.
>>>>
>>>> Putting mapping information in our mapping file is exactly the right place, and is the way we designed it from the
>>>> beginning. Listing them in with the entities/embeddables and mapped superclasses is clearly more convenient, but not
>>>> architecturally the correct place for them to be.
>>> I do not believe anyone is proposing listing converter classes within other managed classes (ie .entities/embeddables
>>> and mapped superclasses). That seems out of place to me.
>>>> I guess we need to decide whether we want to go with architectural consistency or convenience. Both have merits and
>>>> are valid arguments.
>>> If it is architecturally inconsistent then why have the list of annotated managed classes at all in the persistence.xml
>>> file ? There is nothing inconsistent about continuing a configuration pattern that is already defined in the
>>> specification.
>>>
>>> For clarity, an element for converters should still be added to the persistence-unit element in orm.xml for un-annotated
>>> classes but I assumed that would be added later when the xsds are updated.
>>