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

HHH-17948 add Session.findAll(), StatelessSession.getAll() #8166

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

gavinking
Copy link
Member

@gavinking gavinking commented Apr 12, 2024

Signed-off-by: Gavin King <gavin@hibernate.org>
@sebersole
Copy link
Member

Minor comment, but looks reasonable otherwise

*
* @since 6.5
*/
<E> List<E> findAll(Class<E> entityType, Object... ids);
Copy link
Contributor

Choose a reason for hiding this comment

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

So just a convenience for byMultipleIds(Class<?>).multiLoad(Object...)? Maybe it's just me, but I don't think we should start "duplicating" API for convenience.

Copy link
Member

Choose a reason for hiding this comment

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

So just a convenience for byMultipleIds(Class<?>).multiLoad(Object...)? Maybe it's just me, but I don't think we should start "duplicating" API for convenience.

I generally very much agree with that. But I think a single convenience method for the common case is perfectly fine. Now if/when this starts getting overloaded, then I have a big problem with this

Copy link
Member Author

Choose a reason for hiding this comment

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

I also usually agree, but the thing is that JPA 3.2 has gone in a quite different direction with FindOptions.

I initially proposed something more like our XxxxLoadAccess interfaces, but the XxxOptions objects were the direction Lukas wanted to take, and he talked me into it. Ultimately I think this was the right choice.

Therefore the concept here, as described in the issue, is that in H7 we would allow findAll() to accept FindOptions. Yes, it's still "duplication" in some sense, but justifiable duplication, I would say.

Signed-off-by: Gavin King <gavin@hibernate.org>
@hibernate-github-bot
Copy link

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+
    ↳ Offending commits: [ff43020]

› This message was automatically generated.

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