HHH-19829 - Deprecate MultiIdentifierLoadAccess and byMultipleIds #11061

jrenaat · 2025-10-03T21:49:18Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

https://hibernate.atlassian.net/browse/HHH-19829

gavinking

As commented, the records would be better as enums.

I've left a couple of comments about minor stuff.

hibernate-core/src/test/java/org/hibernate/orm/test/loading/multiLoad/MultiLoadTest.java

gavinking · 2025-10-06T16:43:36Z

hibernate-core/src/main/java/org/hibernate/internal/SessionImpl.java

 				loadAccess.withReadOnly( option == ReadOnlyMode.READ_ONLY );
 			}
+			else if ( option instanceof MultiFindOption multiFindOption ) {
+				throw new IllegalArgumentException( "MultiFindOption " + multiFindOption.toString() + " can only be used in multi-find operations" );


Suggested change

throw new IllegalArgumentException( "MultiFindOption " + multiFindOption.toString() + " can only be used in multi-find operations" );

throw new IllegalArgumentException( "Option '" + multiFindOption + "' can only be used in 'findMultiple()'" );

@jrenaat Why did you mark this suggestion as resolved?

Because I adopted it as suggested (it just wasn't pushed to my PR yet)

gavinking · 2025-10-06T16:49:01Z

hibernate-core/src/main/java/org/hibernate/MultiFindOption.java

+/**
+ * Simple marker interface for FindOptions which can be applied to multiple id loading.
+ */
+public interface MultiFindOption extends FindOption {


One more thing, since MultiFindOption:

doesn't actually add any type safety (it extends MultiFindOption)

has a quite ugly name

can be added into the hierarchy at any time if needed

I would just remove it, and make these new options implement FindOption directly.

It's not adding anything.

It was steve's suggestion to do it like this.
It does make the restriction in session.find(..., FindOption ... options) somewhat cleaner having it around

It does make the restriction in session.find(..., FindOption ... options) somewhat cleaner having it around

I don't know what you mean by this.

private <T> void setLoadAccessOptions(FindOption[] options, IdentifierLoadAccessImpl<T> loadAccess) { for ( FindOption option : options ) { if ( option instance of MultiFindOption ) { throw ... } } }

is quite nicer than individually checking for each type.

It's not adding anything.

I disagree. Find usages of MultiFindOption has value.

has a quite ugly name

Your opinion.

is quite nicer than individually checking for each type.

Which is what I meant

So if you really insist on retaining it (I would not, since this is the root package org.hibernate) then how about FindMultipleOption, to reflect that it is an option for the findMultiple() method?

sebersole

I was going to make a few suggestions about Javadoc but I'll just adjust those later.

sebersole · 2025-10-06T17:36:31Z

hibernate-core/src/main/java/org/hibernate/MultiFindOption.java

+/**
+ * Simple marker interface for FindOptions which can be applied to multiple id loading.
+ */
+public interface MultiFindOption extends FindOption {


private <T> void setLoadAccessOptions(FindOption[] options, IdentifierLoadAccessImpl<T> loadAccess) { for ( FindOption option : options ) { if ( option instance of MultiFindOption ) { throw ... } } }

is quite nicer than individually checking for each type.

sebersole · 2025-10-06T17:42:20Z

As commented, the records would be better as enums.

I went back and forth on this. Better in terms of comparison for sure.

jrenaat · 2025-10-06T17:54:02Z

As commented, the records would be better as enums.

I went back and forth on this. Better in terms of comparison for sure.

I think this is somewhat a matter of personal taste?
is loadAccess.withReadOnly( option == ReadOnlyMode.READ_ONLY ); really so much better or more readable than loadAccess.enableSessionCheck( sessionChecking.enabled() ) ?

gavinking · 2025-10-06T18:06:02Z

I think this is somewhat a matter of personal taste?

loadAccess.withReadOnly( option == ReadOnlyMode.READ_ONLY )

really so much better or more readable than

loadAccess.enableSessionCheck( sessionChecking.enabled() )

Neither of those code fragments show usage of ReadOnlyMode as an API, so I don't care at all.

The difference with an enum is that the compiler can do coverage analysis of the enumerated values, and it simply can't do that with public static finals.

Otherwise, the two options are equivalent.

If you really want an enabled() method (and, to be clear, it's not necessary) then your choices are:

public enum ReadOnlyMode implements FindOption {
	READ_ONLY, READ_WRITE;
	
	public boolean enabled() {
		return this != READ_WRITE;
	}
}

vs:

public record ReadOnlyMode(boolean enabled) implements FindOption {
	public static final READ_ONLY = new ReadOnlyMode(true);
	public static final READ_WRITE = new ReadOnlyMode(false);
}

With the first option, I can use ReadOnlyMode in a switch expression without needing to add a default clause. In the second option I can't do that.
Also, the second option lets me write new ReadOnlyMode(true) to instantiate a new ReadOnlyMode which is equal but non-identical to READ_ONLY. Which is weird and useless and a potential source of bugs.

Folks, this is what enums are supposed to be used for. Let's not do weird things here, please.

sebersole · 2025-10-06T18:10:06Z

Folks, this is what enums are supposed to be used for. Let's not do weird things here, please.

To model booleans... Got it...

sebersole · 2025-10-06T18:13:15Z

I think this is somewhat a matter of personal taste?
is loadAccess.withReadOnly( option == ReadOnlyMode.READ_ONLY ); really so much better or more readable than loadAccess.enableSessionCheck( sessionChecking.enabled() ) ?

MultiIdentifierLoadAccess is being deprecated. That's what you are doing. So that's not really relevant. We will likely leave it as an internal contract, but the deprecation is about removing it as an API.

gavinking · 2025-10-06T18:16:32Z

To model booleans

Boolean is a perfect example of an enumerated type!

In languages like Ceylon or Haskell where boolean isn't a primitive type, it's typically even declared as an enumerated type (more precisely, as a sum type).

e.g. Haskell: data Bool = True | False

sebersole · 2025-10-06T18:20:39Z

The point is about enums wrapping booleans, not booleans-as-enum

gavinking · 2025-10-06T18:29:59Z

The point is about enums wrapping booleans

I understand. We are defining types which are isomorphic to boolean.

What I'm saying is that if Java didn't have a primitive boolean type, then the conventional and by far the most natural way to add it to the language would be to make it an enum, as it is in Ceylon. (Again, strictly speaking, languages like Ceylon and Haskell have sum types, not enums, but "enum" is just a special sort of sum type.)

Because that way the compiler knows there are only two values of the type. The compiler has no way of reasoning about the possible values of a record, so you cannot implement Boolean as a record.

Therefore, if we're asking what's the natural way to implement a type which is isomorphic to boolean, the straightforward answer is: as an enum.

I'm not saying it makes a massive difference in practice; it doesn't. It's just a more natural use of the Java language.

gavinking · 2025-10-06T18:38:05Z

Anyway, look, if you don't find those arguments convincing, that's fine.

But we already have the precedent of:

jakarta.persistence.CacheRetrieveMode,
jakarta.persistence.PessimisticLockScope, and
org.hibernate.ReadOnlyMode

which are all enums isomorphic to boolean.

Unless ya'll can think of some particular reason why users should be allowed to write new OrderedReturn(true), can we please just be consistent with the stuff that already exists?

sebersole · 2025-10-06T18:40:28Z

I think I already hinted I was going that way. You just felt the need to go off on a tangent.

sebersole · 2025-10-06T18:41:00Z

As commented, the records would be better as enums.

I went back and forth on this. Better in terms of comparison for sure.

I think clearly indicates a leaning ;)

gavinking · 2025-10-06T18:41:42Z

As commented, the records would be better as enums.

I went back and forth on this. Better in terms of comparison for sure.

I think clearly indicates a leaning ;)

OK, great. I read it as skepticism.

jrenaat · 2025-10-06T18:54:50Z

This is the actual first time i can clearly indicate the trigger of one of my migraines ... :-|

…e in findMultiple() Signed-off-by: Jan Schatteman <[email protected]>

…dMultiple instead of byMultipleIds, thus covering the newly introduced MultiFindOptions Signed-off-by: Jan Schatteman <[email protected]>

…tipleIds() methods Signed-off-by: Jan Schatteman <[email protected]>

…ations Signed-off-by: Jan Schatteman <[email protected]>

jrenaat force-pushed the HHH-19829_multifindoptions branch from 7938bbd to 6eea4ed Compare October 6, 2025 16:14

gavinking requested changes Oct 6, 2025

View reviewed changes

gavinking reviewed Oct 6, 2025

View reviewed changes

sebersole reviewed Oct 6, 2025

View reviewed changes

jrenaat added 4 commits October 6, 2025 21:04

HHH-19829 - Introduce MultiFindOptions and its implementations for us…

5937abd

…e in findMultiple() Signed-off-by: Jan Schatteman <[email protected]>

HHH-19829 - replicate some existing tests in MultiLoadTest to use fin…

e87b641

…dMultiple instead of byMultipleIds, thus covering the newly introduced MultiFindOptions Signed-off-by: Jan Schatteman <[email protected]>

HHH-19829 - Deprecate MultiIdentifierLoadAccess and the Session.byMul…

acceac6

…tipleIds() methods Signed-off-by: Jan Schatteman <[email protected]>

HHH-19829 - Restrict use of MultiFindOptions to the findMultiple oper…

204da85

…ations Signed-off-by: Jan Schatteman <[email protected]>

jrenaat force-pushed the HHH-19829_multifindoptions branch from 6eea4ed to 204da85 Compare October 6, 2025 19:07

	throw new IllegalArgumentException( "MultiFindOption " + multiFindOption.toString() + " can only be used in multi-find operations" );
	throw new IllegalArgumentException( "Option '" + multiFindOption + "' can only be used in 'findMultiple()'" );

Uh oh!

HHH-19829 - Deprecate MultiIdentifierLoadAccess and byMultipleIds #11061

Are you sure you want to change the base?

HHH-19829 - Deprecate MultiIdentifierLoadAccess and byMultipleIds #11061

Conversation

jrenaat commented Oct 3, 2025 • edited by hibernate-github-bot bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gavinking left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebersole left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

jrenaat commented Oct 6, 2025

Uh oh!

gavinking commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

gavinking commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

gavinking commented Oct 6, 2025

Uh oh!

gavinking commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

sebersole commented Oct 6, 2025

Uh oh!

gavinking commented Oct 6, 2025

Uh oh!

jrenaat commented Oct 6, 2025

Uh oh!

Uh oh!

jrenaat commented Oct 3, 2025 •

edited by hibernate-github-bot bot

Loading

gavinking commented Oct 6, 2025 •

edited

Loading

gavinking commented Oct 6, 2025 •

edited

Loading

gavinking commented Oct 6, 2025 •

edited

Loading