Fix transient detection for proxy #2045

bahusoid · 2019-03-08T19:17:53Z

Fixes #2043

Fixes nhibernate#2043

fredericDelaporte · 2019-03-08T19:32:49Z

The change suspected in #2043 dates back to 5.0. Is this a regression of 5.0?

bahusoid · 2019-03-08T20:06:43Z

Is this a regression of 5.0?

Yes. By adding call to method ForeignKeys.IsTransientFast -> IEntityPersister.IsTransient -> IEntityTuplizer.GetIdentifier which doesn't work with proxies.

So in fix I've added support for proxies (1st place as shortcut to ForeignKeys.IsTransientFast, and 2nd place to support proxies in AbstractEntityTuplizer.GetIdentifier)

src/NHibernate/Engine/ForeignKeys.cs

fredericDelaporte · 2019-03-11T19:22:47Z

This one will have to be back-ported down to 5.0.

hazzik · 2019-03-12T21:42:14Z

src/NHibernate/Tuple/Entity/AbstractEntityTuplizer.cs

@@ -114,6 +114,11 @@ public object Instantiate(object id)

 		public object GetIdentifier(object entity)
 		{
+			if (entity is INHibernateProxy proxy)


In other places the caller checks for proxy before calling this method.

Indeed, in NHibernate.Type.EntityType, the checks done there are no more needed with this PR changes.

There are other cases where the proxy is unproxied before hand, but that looks to me as a requirement of the implemented features there rather than due to GetIdentifier not supporting proxies.

In other places the caller checks for proxy before calling this method.

And your point? You prefer to keep it that way? To me it looks better to support it here.
Or at least move this check to AbstractEntityPersister.GetIdentifier.

If it's about clean up other places - I think we can't remove checks in other places in this PR to avoid some custom implementations relying on it. I can add 6.0 TODO here for clean up.

I would prefer to keep this check outside of this method as in other places.

So how about moving it to AbstractEntityPersister.GetIdentifier? As I don't see why persister.GetIdentifier shouldn't handle proxies.

I am seeing more benefits in having the checks inside that method rather than outside. I do not really see why this method should not do the check itself. It does look very lightweight to me, so having the check done for cases where it can never be a proxy should not have a significant impact. What am I missing?

@fredericDelaporte I agree with you. But if you are OK with my latest changes I think this part is out of scope of this PR as now IsTransient logic never calls GetIdentifier for proxies

bahusoid · 2019-03-13T18:24:22Z

src/NHibernate/Async/Engine/ForeignKeys.cs

-				{
-					return Task.FromResult<bool?>(false);
-				}
+			if (session.PersistenceContext.IsEntryFor(entity))


Actually as shows NH607 this check is wrong. PersistenceContext can contain entries for transient objects.
And hack was added to support wrong implementation of IsNotTransientSlow:

nhibernate-core/src/NHibernate/Engine/ForeignKeys.cs

Lines 249 to 255 in ac39173

/***********************************************/

// TODO NH verify the behavior of NH607 test

// these lines are only to pass test NH607 during PersistenceContext porting

// i'm not secure that NH607 is a test for a right behavior

EntityEntry entry = session.PersistenceContext.GetEntry(entity);

if (entry != null)

return entry.Id;

So in this PR I'm going to move this check back to IsNotTransientSlow and in new PR for 5.3 will try to remove it completly.

fredericDelaporte · 2019-03-13T21:10:19Z

src/NHibernate/Engine/ForeignKeys.cs

@@ -171,6 +171,9 @@ public static bool IsNotTransientSlow(string entityName, object entity, ISession
 		/// </remarks>
 		public static bool? IsTransientFast(string entityName, object entity, ISessionImplementor session)
 		{
+			if (entity.IsProxy())
+				return false;


So, despite some hesitations on that subject, now proxies are considered as never transient.

What happens if we load a proxy of an entity with an assigned identifier, initialize it, delete it in session, flush, and save it again? In such case we would have a transient proxy, and it should be possible to save it, I guess. If that case does work with currently released NHibernate, then the logic from "not transient" should not be applied to "transient" too. (And we should consider removing it from the "not" case.)

This scenario still works (both direct and cascace saving) - as entity is unproxied along the way to IsTransientFast. I will commit tests for such cases later.

But from correctness standpoint you are right - my initial implementation is more correct. It correctly detects transient objects from any context. So I think I simply need to drop all my commits to the state you've approved initially. Do you agree? That means that GetIdentifier fix is also required here.

Your initial implementation has my preference.

@hazzik,

I would prefer to keep this check outside of this method as in other places.

Is-it because this is a patch of 5.2.x? So you consider that this kind of (even small) "semantic" change should be avoided?
(I do not actually see it as a semantic change. I see it more like fixing a bug in GetIdentifier, since its documentation does not state it does not support proxies.)

Sorry, I did not have time to clarify my position on this subject. While this check is ok here, I think it should be performed outside of this method. First of all, AbstractEntityPersister/AbstractEntityTuplizer does not deal with any kind of instantiaded proxies currently (except a delegated method to instantiate one). Secondly, ISessionImplementor.GetEntityPersister(...) is a very heavy method and really should be left as a last resort. So, check for the proxy should happen outside of the persister to avoid its lookup.

ISessionImplementor.GetEntityPersister(...) is a very heavy method and really should be left as a last resort.

~~Why single dictionary lookup is considered heavy?~~ And in our case it's persister.IsTransient method that calls GetIdentifier. So we really save nothing.
It's either put this check in persister.IsTransient vs GetIdentifier (persister or tupilizer). To me the deeper the better - to cover all possible future cases (so in tupilizer).

AbstractEntityPersister/AbstractEntityTuplizer does not deal with any kind of instantiaded proxies currently

I take it you meant - non-instantiated. As I already tell EntityPersister must be able to work with such proxy. For now it's only for IsTransient method.

Why single dictionary lookup is considered heavy?

You are confusing it with another method that is of ISessionFactory

Ok. It could be heavy for subclasses. But that wasn't really the point. My main point for proper IsTransient detection we need to support proxies right inside persister. And it's better to do right in GetIdentifier method

I've pushed changes to the point I think is the right fix. Feel free to adjust it your way

Obsolete

fredericDelaporte

What about this last way of handling proxies? It avoids having the persister dealing with proxies for now. We could still go back on that subject in 5.3, while doing this minimal fix for 5.2.x.

src/NHibernate/Engine/ForeignKeys.cs

Fixes nhibernate#2043 in v5.1.x

Fixes nhibernate#2043 in v5.0.x

Fixes #2043 in v5.1.x

Fixes #2043 in v5.0.x

bahusoid added 3 commits March 8, 2019 15:25

Fix transient detection for proxy

0def815

Fixes nhibernate#2043

Async regen

2f694e8

Fixed test

238cb37

fredericDelaporte added the t: Fix label Mar 8, 2019

small clean up

baf1031

bahusoid commented Mar 8, 2019

View reviewed changes

src/NHibernate/Engine/ForeignKeys.cs Outdated Show resolved Hide resolved

Fix long table name and formatting

986fa50

fredericDelaporte reviewed Mar 10, 2019

View reviewed changes

src/NHibernate/Engine/ForeignKeys.cs Outdated Show resolved Hide resolved

src/NHibernate/Engine/ForeignKeys.cs Outdated Show resolved Hide resolved

fredericDelaporte previously approved these changes Mar 11, 2019

View reviewed changes

fredericDelaporte added this to the 5.2.5 milestone Mar 11, 2019

hazzik reviewed Mar 12, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

bahusoid commented Mar 13, 2019

View reviewed changes

fredericDelaporte reviewed Mar 13, 2019

View reviewed changes

fredericDelaporte self-requested a review March 16, 2019 19:42

bahusoid added 2 commits March 16, 2019 22:41

Move proxy check before interceptor

7969cf3

Tests for transient proxy

660dcd3

bahusoid force-pushed the gh2043 branch from 8f9e995 to 660dcd3 Compare March 16, 2019 21:09

Avoid dealing with proxies inside the persister

a1d631d

fredericDelaporte force-pushed the gh2043 branch from e038843 to a1d631d Compare March 17, 2019 10:27

fredericDelaporte reviewed Mar 17, 2019

View reviewed changes

bahusoid commented Mar 17, 2019

View reviewed changes

src/NHibernate/Engine/ForeignKeys.cs Show resolved Hide resolved

hazzik approved these changes Mar 17, 2019

View reviewed changes

fredericDelaporte merged commit 656a88b into nhibernate:5.2.x Mar 18, 2019

fredericDelaporte added the r: Fixed label Mar 18, 2019

fredericDelaporte pushed a commit to fredericDelaporte/nhibernate-core that referenced this pull request Mar 18, 2019

Fix transient detection for proxy (nhibernate#2045)

838a465

Fixes nhibernate#2043 in v5.1.x

fredericDelaporte pushed a commit to fredericDelaporte/nhibernate-core that referenced this pull request Mar 19, 2019

Fix transient detection for proxy (nhibernate#2045)

385af75

Fixes nhibernate#2043 in v5.0.x

fredericDelaporte pushed a commit that referenced this pull request Mar 20, 2019

Fix transient detection for proxy (#2045)

df7d189

Fixes #2043 in v5.1.x

fredericDelaporte pushed a commit that referenced this pull request Mar 20, 2019

Fix transient detection for proxy (#2045)

7a59132

Fixes #2043 in v5.0.x

	/***********************************************/
	// TODO NH verify the behavior of NH607 test
	// these lines are only to pass test NH607 during PersistenceContext porting
	// i'm not secure that NH607 is a test for a right behavior
	EntityEntry entry = session.PersistenceContext.GetEntry(entity);
	if (entry != null)
	return entry.Id;

Fix transient detection for proxy #2045

Fix transient detection for proxy #2045

Uh oh!

Conversation

bahusoid commented Mar 8, 2019

Uh oh!

fredericDelaporte commented Mar 8, 2019

Uh oh!

bahusoid commented Mar 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fredericDelaporte commented Mar 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bahusoid Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

bahusoid Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bahusoid Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fredericDelaporte Mar 12, 2019 •

edited

Loading

bahusoid Mar 13, 2019 •

edited

Loading

fredericDelaporte Mar 13, 2019 •

edited

Loading

bahusoid Mar 15, 2019 •

edited

Loading

bahusoid Mar 15, 2019 •

edited

Loading

fredericDelaporte left a comment •

edited

Loading