Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix id type wrong due to no sorted methods from Class.getMethods #620

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hugogu
Copy link

@hugogu hugogu commented Jan 31, 2018

The Class.getMethods returns undetermined ordering in OpenJDK which may break the idType.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

Copy link
Member

@aklish aklish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. The change seems reasonable. Can you add a unit test case so we don't have a regression? Thanks!

@@ -188,6 +188,11 @@ private void bindEntityId(Class<?> cls, String type, AccessibleObject fieldOrMet
String fieldName = getFieldName(fieldOrMethod);
Class<?> fieldType = getFieldType(fieldOrMethod);

if (idType != null && fieldType.isAssignableFrom(idType)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ordering is undefined in the OpenJDK, how can we be sure that we always want the first idType?

This looks like it will cause us to only bind the first field or method annotated with @Id we find and not necessarily the @Id that is closest to the class we are binding.

Also, what does it mean for a JPA annotated bean to have more than 1 field annotated with @Id?

Copy link
Collaborator

@DennisMcWherter DennisMcWherter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewing the JPA spec (warning: Oracle PDF link), it may be worth considering the information in Section 2.4. Id's are actually a bit complicated and we have not implemented the full spec here (mainly due to lack of practical use rather than lack of will). Namely, behavior related to @IdClass is likely not spec compliant today. However, this seems mostly related to information about derived entities (section 2.4.1).

@hugogu can you give an example of what exactly this is trying to solve?

If you are extending and using a @MappedSuperclass (this is distinct from a section 2.4.1 derived class), section 2.11 demonstrates it is not explicitly supported to override attributes through typical Java inheritance (and in fact, may be disallowed):

When applied to the subclasses, the inherited mappings will apply in the context of the subclass tables. Mapping information can be overridden in such subclasses by using the AttributeOverride and AssociationOverride annotations or corresponding XML elements.

In short, it looks like you would want to use @AttributeOverride to make this stick.

Now, to be clear, I am pretty sure we do not support this today. However, implementing the override in the EntityBinding should be a straightforward task.

@hugogu
Copy link
Author

hugogu commented Feb 1, 2018

@clayreimann @DennisMcWherter ,

Thank you all for the review comments. It sounds like there is a more appropriate way to achieve the same purpose. I'll do some more investigation as per your suggestions and get back with more details.

Hugo

@DennisMcWherter
Copy link
Collaborator

Thanks! Looking forward to seeing the update! :)

@zhangzhenhuajack
Copy link

1:When MyEntity.class extend AbstractPersistable . This problem while reshow.
2:Because EntityBinding source code cls.getMethods() while get two getId() Methods,have the some ’AnnotationPresent(Id.class)‘,but One is
the interface Persistable‘s Method and return Type is Obejct。
3:May be have better method ?

@clayreimann
Copy link
Contributor

clayreimann commented Feb 3, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants