HHH-19829 - Deprecate MultiIdentifierLoadAccess and the Session.byMultipleIds() methods #11070
Conversation
…tipleIds() methods Signed-off-by: Jan Schatteman <[email protected]>
…tipleIds() methods
|
Follow-up on #11061 |
| * `ReadOnlyMode` - whether the entities loaded should be marked as read-only. | ||
| * `LockMode` (`LockModeType`) - a lock mode to be applied | ||
| * `Timeout` - if a pessimistic lock mode is used, a timeout to allow | ||
| * `Locking.Scope` (PessimisticLockScope`) - if a pessimistic lock mode is used, what scope should it be applied |
There was a problem hiding this comment.
what scope should it be applied to ?
There was a problem hiding this comment.
This entire section needs a re-write. Feel free to tackle it :)
I'm just trying to improve it by removing out dated information - addition by subtraction.
There was a problem hiding this comment.
No, I was just indicating that perhaps the 'to' was missing, that's all
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| List<Person> persons = session.findMultiple( | ||
| Person.class, | ||
| List.of(1,2,3), | ||
| PESSIMISTIC_WRITE, | ||
| ORDERED | ||
| ); |
Check notice
Code scanning / CodeQL
Unread local variable Note test
| * <p/> | ||
| * When combined with {@linkplain OrderedReturn#UNORDERED}, the entity is | ||
| * simply excluded from the result. | ||
| * <p/> |
There was a problem hiding this comment.
I keep tripping over this.
If by "excluded from the load result" you mean that no nulls are present in the load result for removed instances when sessionchecking == enabled, includeremovals == exclude and ordering == unordered, then I can't get this one to work as expected:
e.g. running MultiLoadTest.testUnorderedMultiLoadFrom2ndLevelCachePendingDeleteReturnRemoved, with these settings returns a List of size 3, where one element is null, which seems to contradict what follows the IF above.
If what follows the IF above is NOT what you meant then my assumption is wrong.
There was a problem hiding this comment.
I think EXCLUDE + UNORDERED is really the only thing confusing you?
My memory is just wrong for that case. It does appear that in this case the null for the removed entity is still included in the result. That's not what I remember and I have no reasonable explanation why that is the case. I mean, I can see maybe wanting to avoid needing to resize the list later or something like that, but from a user perspective I think that seems wrong.
Given that, I almost think the naming is wrong.
There was a problem hiding this comment.
I think EXCLUDE + UNORDERED is really the only thing confusing you?
this.
There was a problem hiding this comment.
My memory is just wrong for that case. It does appear that in this case the null for the removed entity is still included in the result. That's not what I remember and I have no reasonable explanation why that is the case. I mean, I can see maybe wanting to avoid needing to resize the list later or something like that, but from a user perspective I think that seems wrong.
Given that, I almost think the naming is wrong.
So, change name, reword doc some more, or create a jira if you consider it's a bug?
There was a problem hiding this comment.
Its not that I consider it a bug per-se. I just consider it less than obvious behavior. But I assume its probably always been this way. Perhaps a third option makes sense, though this third option would depend on the value for OrderedReturn.
enum IncludeRemovals {
/** Legacy false option */
INCLUDE,
/** Legacy true option - replace with null */
REPLACE,
/** New option. */
EXCLUDE
}This new option would trigger the more (imo) appropriate behavior -
- With ordered results, we replace with nulls. This is ofc needed to maintain ordering.
- With unordered results, we'd not have any entry in the result for removed entities.
There was a problem hiding this comment.
Don't we have a backward compatibility problem with that?
There was a problem hiding this comment.
With adding a new option? Why would that be a problem?
There was a problem hiding this comment.
Misinterpreted the reformulated options, my bad
…tipleIds() methods
HHH-19829 - Deprecate MultiIdentifierLoadAccess and the Session.byMultipleIds() methods
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