Bug 336006

Comment 1 Nan Li 2011-02-11 16:04:39 EST

Created attachment 188817 [details]
patch

Patch includes the validation for version mapping besides the ones for basic, id, and element collection.

Comment 2 Nan Li 2011-02-16 09:41:12 EST

Created attachment 189097 [details]
patch

1. Moved all the validation of the version mapping to the patch of bug 336135
2. Changed a bit of the validation code
3. Removed formatting changes

Comment 3 Neil Hauge 2011-02-16 10:32:04 EST

Review:

- The validation logic that has been added to the specific java and orm mappings in this patch can be fully delegated to GenericJavaTemporalConverter and GenericOrmTemporalConverter.  Most of these mappings are already calling this.converter.validate(...), which is what we should be tying into here.  The temporal converter implementations would then contain this behavior, removing the need to use an instanceof to determine whether we have the right type of converter.  They would need to override validate(...), which is where you would then call the validation logic that has been delegated to the same class. This will result in less duplicated code, and better general factoring. 

- The TEMPORAL_MAPPING_SUPPORTED_TYPES static string should be defined on and only on the TemporalConverter interface where it can be referenced from the necessary locations.

- Error Messages:  "To specify temporal type, the persistent field or property must be of type java.util.Date or java.util.Calendar". This isn't bad, but try to be a bit more direct for error messages.   Perhaps - "The persistent field or property for temporal type must be of type java.util.Date or java.util.Calendar".

Comment 4 Nan Li 2011-02-17 09:07:39 EST

Created attachment 189184 [details]
patch

Integrated the review comments

Comment 5 Neil Hauge 2011-02-18 11:55:12 EST

In the element collection mappings, is there a reason you moved the converter.validate() call from the validateValue() method?  Seems it would be most appropriate to leave it where it was before.  

Also, Karen mentioned that EclipseLink (and it seems all the other providers) support java.util.GregorianCalendar for Temporal as well, so that should be added to the list of supported types.

Comment 6 Nan Li 2011-02-21 11:29:33 EST

(In reply to comment #5)
> In the element collection mappings, is there a reason you moved the
> converter.validate() call from the validateValue() method?  Seems it would be
> most appropriate to leave it where it was before.  

I was thinking the validation applies to both value types of basic and embeddable so I extracted it out; otherwise, the converter.validate() would need to be invoked at two places. I can do that way if it's more appropriate.
 
> Also, Karen mentioned that EclipseLink (and it seems all the other providers)
> support java.util.GregorianCalendar for Temporal as well, so that should be
> added to the list of supported types.

Added

Comment 7 Neil Hauge 2011-02-21 11:49:43 EST

(In reply to comment #6)
> (In reply to comment #5)
> > In the element collection mappings, is there a reason you moved the
> > converter.validate() call from the validateValue() method?  Seems it would be
> > most appropriate to leave it where it was before.  
> 
> I was thinking the validation applies to both value types of basic and
> embeddable so I extracted it out; otherwise, the converter.validate() would
> need to be invoked at two places. I can do that way if it's more appropriate.

If it applies to both then you would just move that call outside of the switch statement in the validateValue() method.  I would just make that call first, before the switch.  The reason for this is that the converter validation is still a part of the value validation, and as such should be kept in that method.  

That said, I see that Karen had this specifically in the Basic type validation, and there is also that TODO there.  Karen, can you comment on this issue?

Comment 8 Karen Butzke 2011-02-21 14:14:14 EST

As far as I can tell my comment in AbstractJavaElementCollectionMapping2_0 is unrelated and I'm not really sure what I was trying to say.

I think Nan is right that we should be validating the converter whether the type is basic or embeddable for an element collection. I don't know why I didn't do this in the first place, but I must have for some reason thought you wouldn't use converters in this case. Looking at the validation for all the converters I think this makes sense and the call can be placed outside of the switch statement in validateValue(), as Neil has suggested.

I've tested around some with EclipseLink's validation and this matches what they are doing as well.

Comment 9 Nan Li 2011-02-21 16:43:24 EST

Created attachment 189450 [details]
patch

Integrated the comments from Neil and Karen. Thanks for the input!

Comment 10 Karen Butzke 2011-02-24 17:37:11 EST

I have committed this patch with a few minor changes including removing the printlns.

Comment 11 Jolene Moffitt 2011-04-26 12:02:51 EDT

Verified in Build I-3.3.0-20110414085808

Verified error message appears when you select temporal for id, basic and element collection mappings and the type is not date, calendar or gregorian calendar.  See the link to view test steps for verification. http://wiki.eclipse.org/Dali_3.0_M6