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

Update Jakarta Persistence Section annotation #461

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

otaviojava
Copy link
Contributor

Change

  • Update the Jakarta Persistence section, explaining that all the annotations from Jakarta persistence should be available once combined with Jakarta Data.

⚠️ This one might be a mistake that I made. There is no Jakarta Persistence Lite. Thus, as user I expected once using Jakarta Persistence I want to have access to all of those as well; for example, Inheritance or MappedSuperclass

Copy link
Contributor

@njr-11 njr-11 left a comment

Choose a reason for hiding this comment

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

I'm pretty sure we have had this conversation before. We cannot require support for all annotations in Jakarta Persistence in combination with Jakarta Data without defining what the integration between the two specs actually means for each annotation. This list that is currently in the spec is there because the integration for those is clear. As others become well-defined in the spec, they can be added. It is wrong to simply state that all are supported. Jakarta Data does not define what it means to support them.

@gavinking
Copy link
Contributor

We cannot require support for all annotations in Jakarta Persistence in combination with Jakarta Data without defining what the integration between the two specs actually means for each annotation.

@njr-11 I'm not sure what you mean here. For the vast majority of JPA annotation, I don't see why there would be any specific integration concerns. A Jakarta Data supporting JPA is going to be based on a full JPA implementation, and so all the annotations should "just work", IMO.

What am I missing?

@njr-11
Copy link
Contributor

njr-11 commented Feb 20, 2024

What am I missing?

I'm trying to remember some of the examples that came up in the previous discussion. Just browsing through some of the Jakarta Persistence annotations that aren't listed there, one example might be @EmbeddedId. You could write a repository method findByIdBetween(id1, id2), but I don't know of anything in the spec that would define the behavior of how to compare EmbeddedId to determine whether one is between two others, or even whether these can be compared at all. Is it ordered for comparison by the components that make it up, and if so, how is it determined which components are given precedence over the others (order of declaration, alphabetically by field name, something else?). Or is it ordered in some other way entirely? You might actually know the answer to this - it's possible Jakarta Persistence might define all this and I didn't look closely enough. And if it does clearly define it, then we could add that Jakarta Data requires this annotation be supported when used with Jakarta Persistence. But absent that, it doesn't seem like we should be declaring that it must be supported with Jakarta Data.

@gavinking
Copy link
Contributor

OK so the issue is specifically with respect to querying? That's not really clear in the text of the spec. It sounded like it was saying you can only use specific JPA annotations on your entities, which would be weird.

So, OK, yeah, with respect to querying I guess it's a good point that I suppose I need to think about a little bit further. We don't currently have any sort of straight well-defined dictionary from concepts in Jakarta Data querying to concepts in JPQL, so one can indeed be left wondering what something like "between" means for some specific type.

I don't know of anything in the spec that would define the behavior of how to compare EmbeddedId to determine whether one is between two others, or even whether these can be compared at all.

But on the other hand, we do already have the notion of attributes which are "sortable" and attributes which aren't. That is, we have SortableAttribute in the metamodel.

Now, we should probably draw out that distinction in the spec, and say something about why an attribute might not be sortable.

@gavinking
Copy link
Contributor

We don't currently have any sort of straight well-defined dictionary from concepts in Jakarta Data querying to concepts in JPQL,

On the other hand, if we do #458, then such a dictionary is easy to write down.

@njr-11
Copy link
Contributor

njr-11 commented Feb 20, 2024

Now, we should probably draw out that distinction in the spec, and say something about why an attribute might not be sortable.

Yes, if we add that information to the spec, I think we could add EmbeddedId and probably a number of the relation attributes as required.

@gavinking
Copy link
Contributor

I think we could add EmbeddedId

If I understand correctly, NoSQL is adding support for embeddables, and so it makes sense to talk about them a bit more explicitly in the the Data spec.

@otaviojava
Copy link
Contributor Author

I think we could add EmbeddedId

If I understand correctly, NoSQL is adding support for embeddables, and so it makes sense to talk about them a bit more explicitly in the the Data spec.

Thus, we need to improve.
Can we approve this one and fix this point at another PR?

#463

@otaviojava
Copy link
Contributor Author

I think we could add EmbeddedId

If I understand correctly, NoSQL is adding support for embeddables, and so it makes sense to talk about them a bit more explicitly in the the Data spec.

Thus, we need to improve. Can we approve this one and fix this point at another PR?

#463

@gavinking @njr-11, so can we move forward and fix this aspect in other PRs?

@gavinking
Copy link
Contributor

gavinking commented Feb 21, 2024

Look, why not just remove the bit that says "and requires support for all annotations in the Jakarta Persistence specification" which I intuit is what makes @njr-11 uncomfortable here.

Jakarta Data doesn't need to mandate that JPA implementations implement JPA. The JPA spec already does that.

@otaviojava otaviojava merged commit 97b01dd into main Feb 21, 2024
3 checks passed
@otaviojava otaviojava deleted the update-jakarta-persistence-annotation branch February 21, 2024 19:04
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.

3 participants