-
Notifications
You must be signed in to change notification settings - Fork 3k
Add findByIds API to Panache #49754
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
base: main
Are you sure you want to change the base?
Add findByIds API to Panache #49754
Conversation
@gsmet can you maybe point me to how to achieve the same for Mongo, so the API is consistently available? Or maybe point me to somebody else or some resource? Also, this PR is still a DRAFT as I definitely should also add some tests... |
@geoand thank you - I found these files, unfortunately I have zero clue about Mongo, and I could not find an API to do a |
It's possible that the Mongo driver does not have such a capability, but I don't remember off hand |
b58f5ef
to
696c7bc
Compare
0c6d5c7
to
e5c8f46
Compare
...ime/src/main/java/io/quarkus/hibernate/orm/panache/common/runtime/AbstractJpaOperations.java
Outdated
Show resolved
Hide resolved
c1e48d4
to
efa2654
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, thanks for the PR :)
...te-orm-panache/runtime/src/main/java/io/quarkus/hibernate/orm/panache/PanacheEntityBase.java
Show resolved
Hide resolved
...mongodb-panache/runtime/src/main/java/io/quarkus/mongodb/panache/PanacheMongoEntityBase.java
Outdated
Show resolved
Hide resolved
...-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/defaultpu/TestEndpoint.java
Outdated
Show resolved
Hide resolved
.../hibernate-orm-panache/src/test/java/io/quarkus/it/panache/defaultpu/PanacheMockingTest.java
Outdated
Show resolved
Hide resolved
...s/mongodb-panache/src/test/java/io/quarkus/it/mongodb/panache/MongodbPanacheMockingTest.java
Outdated
Show resolved
Hide resolved
@FroMage thanks for all the feedback, I hope I didn't miss anything. One more thing: I also found out that extensions/spring-data-jpa/runtime/src/main/java/io/quarkus/spring/data/runtime/RepositorySupport.java was already offering the same functionality by finding entities one by one based on their ID. I migrated it to use the new API and marked the second API that doesn't seem to bring any benefit to me anymore deprecated. |
This comment has been minimized.
This comment has been minimized.
b7e536c
to
afd8bcb
Compare
afd8bcb
to
e59afad
Compare
LGTM, do you mind merging your commits before we merge this? |
This comment has been minimized.
This comment has been minimized.
b318067
to
3b466ae
Compare
This comment has been minimized.
This comment has been minimized.
3b466ae
to
7e83cc4
Compare
Do you mind rebasing on the latest |
7e83cc4
to
31a286d
Compare
Thanks |
This comment has been minimized.
This comment has been minimized.
I think the test failures are related :-/ |
Hmmm, they were green some days ago :-( will look at it in the evening. |
31a286d
to
b76799e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b76799e
to
4595945
Compare
This comment has been minimized.
This comment has been minimized.
4595945
to
53dcddf
Compare
This comment has been minimized.
This comment has been minimized.
53dcddf
to
4fd5bb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you ready to take this out of draft?
@@ -24,13 +24,21 @@ public static List<?> findByIds(AbstractJpaOperations<PanacheQuery<?>> operation | |||
result.add(byId); | |||
} | |||
} | |||
return result; | |||
return operations.findByIds(entityClass, result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this one? It appears we do findById
on each ID, and then call findByIds
with the results (which should be a list of entities, not ids).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FroMage this one in particular of course is incorrect and some leftover of pushing the current state of affairs before I reverted this PR to a draft. Unfortunately, there are a few problems, mainly caused by the vararg-API (some places use Iterable
s and calls end up in the wrong API). Also, when I last looked at it last Friday, I think I remember that on some places we have inconsistent and maybe confusing namings. I am quite busy at the moment, can only offer to continue with it in the evenings (but am absolutely willing to). But since we talk: I think it would be best to remove the vararg-API again (or give it another name) to move forward here.
Hibernate 7 introduced a new API to find multiple Entities by their IDs, which is not yet available in Hibernate ORM with Panache. To allow making use of it, this PR introduces a new API called
findByIds
(returnsList<Entity>
).It also adds a similar
findByIds
API for MongoDB with Panache, albeit only performing acollection.find(Filters.in("_id", ids))
, to avoid falling short.