Bugzilla – Full Text Bug Listing
| Summary: | [Validation] Need validation for embeddable id class | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [WebTools] Dali JPA Tools | Reporter: | Nan Li <nan.n.li> | ||||||||
| Component: | JPA | Assignee: | Leslie Davis <les.davis> | ||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||
| Severity: | enhancement | ||||||||||
| Priority: | P3 | CC: | karenfbutzke, les.davis, neil.hauge | ||||||||
| Version: | unspecified | Keywords: | plan | ||||||||
| Target Milestone: | 3.2 M6 | Flags: | karenfbutzke:
review+
|
||||||||
| Hardware: | PC | ||||||||||
| OS: | Windows 7 | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Nan Li
For item 1: This will be a warning as they are not explicitly mentioned in the spec and EclipseLink does not validate on this. The warning message should read "Embedded id class should include method definitions for equals(Object) and hashcode()" I will add methods to JDTTools for determining if the given type name define particular method signatures and create validation methods on GenericJavaEmbeddedIdMapping and GenericOrmEmbeddedIdMapping which use the JDTTools methods to determine whether the target embeddable type defines these methods and add the warning message if not. For item 2: I feel this is may be too restrictive as mentioned, might be a "nice to have" warning however? For item 3: This, like item 1, is more a general rule for primary key classes and would be a warning as well. The warning message should read "Embedded id class should be public, implement Serializable, and include a zero argument constructor" JDTTools already contains a method to determine if the class implements Serializable typeNamedImplementsInterfaceNamed(), and will contain classHasPublicZeroArgConstructor when a previous patch from Nan is checked in. Validation methods on GenericJavaEmbeddedIdMapping and GenericOrmEmbeddedIdMapping will uses these methods to determine whether the target embeddable type meets these requirements and add the warning message if not. For item 4: This is specified in the spec and would be an error. The error message should read "Id or id class is defined on another attribute/property on the parent entity, only one or the other may be defined on an entity" Validation methods added to GenericOrmEmbeddedIdMapping and GenericOrmEmbeddedIdMapping would be added to look at all other persistent attributes and properties on the entity for id or id class definitions. For Item 5: Validation already exists For Item 6: Page 280 of Pro JPA 2 the second paragraph under the heading "Embedded Id Class" appears to state the opposite, "Like other embedded objects, the embedded id class must be annotated with @Embeddable, but the access type might differ from that of the entity that uses it." I would also suggest adding validation for this from the spec: "Relationship mappings defined within an embedded id class are not supported." (In reply to comment #1) > For item 1: > > This will be a warning as they are not explicitly mentioned in the spec and > EclipseLink does not validate on this. The warning message should read > "Embedded id class should include method definitions for equals(Object) and > hashcode()" > > I will add methods to JDTTools for determining if the given type name define > particular method signatures and create validation methods on > GenericJavaEmbeddedIdMapping and GenericOrmEmbeddedIdMapping which use the > JDTTools methods to determine whether the target embeddable type defines these > methods and add the warning message if not. I think this can be an error message based on JPA 2.0 spec section 2.4. For the error message, use "... equals() and hashcode()". (In reply to comment #2) > For item 2: > > I feel this is may be too restrictive as mentioned, might be a "nice to have" > warning however? We don't yet do this validation for basic id's yet, so this can wait for now. > For item 3: > > This, like item 1, is more a general rule for primary key classes and would be > a warning as well. The warning message should read "Embedded id class should > be public, implement Serializable, and include a zero argument constructor" > This needs to be split up a bit. Public and zero argument constructor validation can be combined in one message. This should be an error based on section 2.4 of the spec. Implements Serializable should also be an error based on section 2.4 of the spec. > JDTTools already contains a method to determine if the class implements > Serializable typeNamedImplementsInterfaceNamed(), and will contain > classHasPublicZeroArgConstructor when a previous patch from Nan is checked in. > Validation methods on GenericJavaEmbeddedIdMapping and > GenericOrmEmbeddedIdMapping will uses these methods to determine whether the > target embeddable type meets these requirements and add the warning message if > not. (In reply to comment #3) > For item 4: > > This is specified in the spec and would be an error. The error message should > read "Id or id class is defined on another attribute/property on the parent > entity, only one or the other may be defined on an entity" > > Validation methods added to GenericOrmEmbeddedIdMapping and > GenericOrmEmbeddedIdMapping would be added to look at all other persistent > attributes and properties on the entity for id or id class definitions. This probably needs to be type level validation that results in an error when it's collection of mappings contain a simple Id and an Embedded Id. You will need to look at what AbstractPrimaryKeyValidator is doing (and its subclasses) to get an idea of what is taking place related to Id validation. Much care is needed when adding validation in this area and differences between Generic and EclipseLink are important, as EclipseLink is less restrictive in many cases. As for this particular error, I don't think EclipseLink is less restrictive. (In reply to comment #4) > For Item 5: > > Validation already exists > > For Item 6: > > Page 280 of Pro JPA 2 the second paragraph under the heading "Embedded Id > Class" appears to state the opposite, "Like other embedded objects, the > embedded id class must be annotated with @Embeddable, but the > access type might differ from that of the entity that uses it." This is also backed up by the spec, section 2.4. (In reply to comment #5) > I would also suggest adding validation for this from the spec: > > "Relationship mappings defined within an embedded id class are not > supported." Yes, please add this to the Embedded Id validation. Created attachment 192311 [details]
proposed bug fix patch
Deferring this to 3.1. Slightly too much risk to add this validation to the 3.0 release at this point. *** Bug 341821 has been marked as a duplicate of this bug. *** Moving JPA specific bugs to new JPA component in bugzilla. validation preferences are needed for these new messages (see JpaProblemSeveritiesPage) For Item 3: the zero arg constructor validation was added with bug 132216, so now that message can just be for making sure the class is public. Currently we end up with 2 validation messages about needing a zero-arg constructor minor, but this is misspelled - EMBEDDED_ID_CLASS_SHOULD_IMPLMENT_EQUALS_HASHCODE In JDTTools.typeImplementsMethod(IJavaProject, IType, String, String[]) no need to pass in the IJavaProject, it is not used. In GenericJavaEmbeddedIdMapping - validateTargeEmbeddableImplementsEqualsAndHashcode - typo - missing the t on the end of Target The method JDTTools.classIsPublic(IJavaProject, String) is unnecessary. I just added API TypeMapping.getJavaResourceType() and JavaResourceType already has the API isPublic(), so that can be used instead of this new method in JDTTools. I am going to look at removing JDTTools.classHasPublicZeroArgConstructor() as well, since that is somewhat redundant with API on JavaResourceType, just need to figure out which one is more correct. Created attachment 203372 [details]
patch with additional recommended changes
Fixed compilation problems and implemented suggested changes.
Les, I have to send this back at you again because it doesn't take into account virtual attributes in an orm.xml file. The validation messages do not have enough information to understand the context if an entity is added to the orm.xml file that contains a virtual embeddedId with any of these validation errors. And a couple minor things I was changing in the patch, so I don't forget with the next patch: TYPE_MAPPING_ID_AND_EMBEDDED_ID_BOTH_USED=This class must use either an ID or **and** embedded ID to specify its primary key, not both **and** should be *an* Also remove the '.' from the end of all the validation messages. That will match our other validation messages and the JDT messages. Created attachment 211005 [details]
proposed patch
Added validation for virtual attributes.
checked patch in for M6 |