Community
Participate
Working Groups
Build Identifier: I20101028-1441 The Temporal annotation/element must be specified for persistent fields or properties of type java.util.Date and java.util.Calendar. It may only be specified for fields or properties of these types. When the Temporal annotation/element is used in conjunction with the ElementCollection annotation/element, the element collection value should be of such a temporal type. If the element collection is a Map, this applies to the map value. Validation error should be reported if the mapped persistent fields/properties or the element collection values is not of such a temporal type when the Temporal annotation/element applies. Reproducible: Always Steps to Reproduce: Specifying @Temporal or <temporal> for a basic, id, or element collection mapping
Created attachment 188817 [details] patch Patch includes the validation for version mapping besides the ones for basic, id, and element collection.
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
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".
Created attachment 189184 [details] patch Integrated the review comments
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.
(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
(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?
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.
Created attachment 189450 [details] patch Integrated the comments from Neil and Karen. Thanks for the input!
I have committed this patch with a few minor changes including removing the printlns.
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
