HHH-19605 Session.isDirty might return true when batch fetching entity with CacheConcurrencyStrategy.READ_WRITE

[mbellade]

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

---

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.

---

[mbellade]

Not sure why we weren't checking for non-enhanced proxies, but this seems more correct (and in line with isNonDirtyViaTracker).

[gavinking]

I forget, but @mbellade look at the git history for this:

• In commit eb1e19f I added this comment:

    		// TODO: why do we not check !lazinessInterceptor.hasWrittenFieldNames()
    		//       as we do below in isNonDirtyViaTracker() ?
  

• In commit 5b4a6df I deliberately removed the TODO, which suggests that I figured out it's actually correct?

I don't remember any of this, and I stupidly didn't document my reasoning for removing the TODO in the commit message, but probably there's more here than meets the eye.

[mbellade]

I'm just checking !interceptor.isInitialized() though, not interceptor.hasWrittenFieldNames(): I get not relying on the internal written flags when using a custom dirtiness strategy, but how can a non-initialized proxy ever be considered "dirty"?

I was also wondering if there were reasons it was explicitly omitted, and we just considered any EnhancementAsProxyLazinessInterceptor to be non-initialized, but I couldn't come up with anything.

[gavinking]

Did you manage to come up with a test to demonstrate it?

[mbellade]

To demonstrate what? That an uninitialized proxy can be encountered here?

[gavinking]

I guess ...

[mbellade]

So I added one, and that revealed a NPE caused by holder.getEntityEntry() returning null. I've now switched the implementation in DefaultDirtyCheckEventListener to use PersistenceContext#reentrantSafeEntityEntries, which to me sounds more correct. This also removes the need to handle non-enhanced proxies, lmk what you think.

[gavinking]

Ah good.

PersistenceContext#reentrantSafeEntityEntries

This one instantiates a many instances of EntityEntryCrossRefImpl though. Is there no way to avoid that?

This also removes the need to handle non-enhanced proxies, lmk what you think.

Yeah, nice.