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

change '#id' to 'id(this)' #587

Merged
merged 1 commit into from
Mar 25, 2024
Merged

change '#id' to 'id(this)' #587

merged 1 commit into from
Mar 25, 2024

Conversation

gavinking
Copy link
Contributor

Since Lukas just merged my proposed change to JPQL which introduced id() and version() functions.

since Lukas just merged my proposed change to JPQL
which introduced id() and version() functions
@gavinking gavinking marked this pull request as ready for review March 25, 2024 15:14
@gavinking
Copy link
Contributor Author

A question I have for you guys is: should we add id(this) and version(this) as legal expressions to JPQL?

*/
String ID = "#id";
String ID = "id(this)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have this function included before?

I mean, if the goal is to have safely contained specifications, or in this case, it is OK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we have this function included before?

WDYM?

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking the Note:

Note that {@code id(this)} is the expression in JPQL for the
* unique identifier of an entity with an implicit identification
* variable.

It is a JPQL function and not a Jakarta Data Query Language function.

I remember that you taught us that the specification should be self-contained. That is, the vendor should read only this specification and understand the behavior of Jakarta Data.

We have included this JPQL function right now by note.

Should we also include this function in our query instead of referring to this JPQL query? Or is it acceptable in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah well that's why I asked:

A question I have for you guys is: should we add id(this) and version(this) as legal expressions to JPQL?

It's not critical in this case (or at least not yet) because right now this is just a magical constant string value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

at least not yet

Obviously I'm thinking toward the future here.

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.

This seems like a good forward thinking thing to do. We don't currently specify that functions can be supplied to Sort or OrderBy, but should we ever decide to add that, this would be consistent with it.

@njr-11
Copy link
Contributor

njr-11 commented Mar 25, 2024

A question I have for you guys is: should we add id(this) and version(this) as legal expressions to JPQL?

It seems like that could be useful, but it should be fine to wait on that until next version.

@gavinking gavinking merged commit fe5a2d8 into jakartaee:main Mar 25, 2024
3 checks passed
@njr-11
Copy link
Contributor

njr-11 commented Mar 26, 2024

I thought of another use for id(this). Assuming it can be used as follows, it could allow us to bring back BasicRepository.findByIdIn,

@Query("WHERE id(this) IN ?1 ORDER BY id(this) ASC")
List<T> findByIdIn(Iterable<K> ids);

@gavinking
Copy link
Contributor Author

gavinking commented Mar 26, 2024

Assuming it can be used as follows, it could allow us to bring back BasicRepository.findByIdIn

That is correct.

Indeed, as far as I recall, this was the real reason I originally wanted it.

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

3 participants