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 two CrudRepository method names that do not follow the spec rules #268

Conversation

njr-11
Copy link
Contributor

@njr-11 njr-11 commented Sep 20, 2023

CrudRepository has two methods that don't follow the rules in the spec:

    findAllById(Iterable<K> ids);
    deleteAllById(Iterable<K> ids);

If a user added these methods to their own repository interface (which is something we explicitly state in the spec can be done), per the rules of the spec, it would mean an equality comparison where the id is equal to the Iterable, which isn't what is wanted. What is really wanted here is provided by the In keyword, which matches any item in the collection.

Names that are consistent with the spec rules would be:

    findAllByIdIn(Iterable<K> ids);
    deleteAllByIdIn(Iterable<K> ids);

or

    findByIdIn(Iterable<K> ids);
    deleteByIdIn(Iterable<K> ids);

This pull updates to the latter.

@njr-11 njr-11 added the bug Something isn't working label Sep 20, 2023
@njr-11 njr-11 added this to the Jakarta Data 1.0 milestone Sep 20, 2023
@otaviojava
Copy link
Contributor

Hey Nathan, I did not follow. Could you explain more?
Sorry, those are not dynamic and are in the built-in repositories; thus, why should they follow the dynamic method queries?
We already have the precedence of using the method default.

Another point is that I can find multiple keys; however, I will not need to use them "in" a query, such as in a relational database, for example, the key-value database. I can get all the value from the key using the "get" command.

@njr-11
Copy link
Contributor Author

njr-11 commented Sep 21, 2023

Hey Nathan, I did not follow. Could you explain more?
Sorry, those are not dynamic and are in the built-in repositories; thus, why should they follow the dynamic method queries?

The following is currently documented about the built-in repository methods:

You can copy individual method signatures from the

  • built-in repository methods onto your own, which is possible
  • because the built-in repository methods are consistent with the
  • same set of conventions that you use to write custom repository methods.

That's true for all of the other CrudRepository and PageableRepository methods, but it's not true for these,
findAllById(Iterable)
deleteAllById(Iterable)
because when copy those methods to your own repository interface and try to use them, it performs an equality comparison: where Id is equal to the Iterable.

To correctly write them on your own repository interface to match against any of the Ids in the Iterable, you need to use the In keyword,

findAllByIdIn(Iterable)
deleteAllByIdIn(Iterable)

or

findByIdIn(Iterable)
deleteByIdIn(Iterable)

This pull switches to the above so that a user can copy these from CrudRepository to their own repository and they will actually work the same way, as the documentation claims they would.

I will also point out that ensuring that we correctly adhere to our own documentation isn't the only benefit to making this change. It will also help new users learn how to properly compose repository methods if all of the built-in ones are good examples that follow the rules. Leaving them in violation of the rules would confuse our new users.

Another point is that I can find multiple keys; however, I will not need to use them "in" a query, such as in a relational database, for example, the key-value database. I can get all the value from the key using the "get" command.

Nowhere in our spec documentation does the In keyword ever state that it must translate into a SQL IN. Jakarta Data defines its In keyword only in terms of behavior that you get:

Find results where the property is one of the values that are contained within the given list

Requires that the entity's attribute value be within the list

This language does not in any way prevent key-value databases from implementing it with a "get" command.

@njr-11 njr-11 force-pushed the two-CrudRepository-method-names-do-not-follow-the-spec-rules branch from 283073c to 4a5955d Compare September 26, 2023 11:38
… and resolve merge conflict

Signed-off-by: Nathan Rauh <[email protected]>
@njr-11 njr-11 force-pushed the two-CrudRepository-method-names-do-not-follow-the-spec-rules branch from 4a5955d to 6f68807 Compare September 26, 2023 11:40
@otaviojava
Copy link
Contributor

I am fine with this.

@njr-11 njr-11 merged commit 35d0264 into jakartaee:main Sep 26, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants