Skip to content

Conversation

@gavinking
Copy link
Member

@gavinking gavinking commented Dec 20, 2024

Supersedes #9379

[Please describe here what your change is about]


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-15848

@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Dec 20, 2024

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: [4c26596, 5bd3e6b, a6203b8, 77ae2cf, 3501cfd, 7b2b52e]

› This message was automatically generated.

Comment on lines +371 to +372
// TODO: why do we not check !lazinessInterceptor.hasWrittenFieldNames()
// as we do below in isNonDirtyViaTracker() ?
Copy link
Member Author

Choose a reason for hiding this comment

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

@dreab8 This is a question for you.

Copy link
Member

Choose a reason for hiding this comment

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

@gavinking with dirty checking setting a property will not trigger the initialization, while without dirty checking setting a property triggers the enhamced proxy initilaization and then the interceptor would not be of type EnhancementAsProxyLazinessInterceptor

Copy link
Member

Choose a reason for hiding this comment

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

basically writtenFieldNames is populated only when dirty checking is enabled

Copy link
Member Author

Choose a reason for hiding this comment

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

@dreab8 I guess I don't understand what is the difference between the two branches. They both do:

if ( interceptor instanceof EnhancementAsProxyLazinessInterceptor ) {

and then one of them does the additional check, and one doesn't.

Copy link
Member

Choose a reason for hiding this comment

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

@gavinking one branch deals wit the case dirty checking is not enabled, in this case it's enough interceptor instanceof EnhancementAsProxyLazinessInterceptor to be sure it's not dirty, the other branch deals with the case dirty checking is enabled and if interceptor instanceof EnhancementAsProxyLazinessInterceptor we need also to be sure it has not written field names to say that it's not dirty

this is definitely not perfect yet, but it's certainly a much
better foundation than the ancient implementation which was bad
and side-effecty in all sorts of ways
and add a TODO (a question for @dreab8)
&& !collection.isDirty() //optimization
&& loadedPersister != null
&& loadedPersister.isMutable() //optimization
&& ( collection.isDirectlyAccessible() || loadedPersister.getElementType().isMutable() ) //optimization

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
CollectionPersister.getElementType
should be avoided because it has been deprecated.
private static boolean isCollectionDirty(PersistentCollection<?> collection, CollectionPersister loadedPersister) {
return collection.isDirty()
|| collection.wasInitialized()
&& loadedPersister != null

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
CollectionPersister.getElementType
should be avoided because it has been deprecated.
}
else {
assert uniqueKeyPropertyName != null;
final Type keyType = persister.getPropertyType( uniqueKeyPropertyName );

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
EntityPersister.getPropertyType
should be avoided because it has been deprecated.
// We now have the value of the property-ref we reference. However,
// we need to dig a little deeper, as that property might also be
// an entity type, in which case we need to resolve its identifier
final Type type = entityPersister.getPropertyType( uniqueKeyPropertyName );

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
EntityPersister.getPropertyType
should be avoided because it has been deprecated.
Comment on lines +1489 to +1490
log.debugf("Flushing to force deletion of re-saved object: "
+ infoString( key.getPersister(), key.getIdentifier(), getFactory() ) );

Check failure

Code scanning / CodeQL

Insertion of sensitive information into log files High

This
potentially sensitive information
is written to a log file.
@gavinking gavinking merged commit ca58bbe into hibernate:main Dec 20, 2024
23 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants