Fix #1419 - ISession.IsDirty() shouldn't throw exception for transient many-to-one object in a session

[bahusoid]

Fixes #1419

[fredericDelaporte]

Need to use IsTransientSlow, otherwise the bug will still be there for entities having an assigned id already assigned or having an unsaved-value set as undefined. IsTransientSlow hits the db only if IsTransientFast was not able of answering, which occurs in aforementioned cases.

[hazzik]

Need to have a test for this case.

[hazzik]

@fredericDelaporte, the GetEntityIdentifierIfNotUnsaved calls IsTransientFast as well.

[hazzik]

Ok, what will happen in case of an entity with assigned ID - the "IsDirty" method will not fail, but it will return false.

[bahusoid]

@fredericDelaporte

Need to use IsTransientSlow, otherwise the bug will still be there

Are you sure? I simply reused check that lead to exception:

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

Lines 250 to 265 in a6f97d0

	if (IsTransientFast(entityName, entity, session).GetValueOrDefault())
	{
	/***********************************************/
	// 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;
	// the check was put here to have les possible impact
	/**********************************************/
	entityName = entityName ?? session.GuessEntityName(entity);
	string entityString = entity.ToString();
	throw new TransientObjectException(
	string.Format("object references an unsaved transient instance - save the transient instance before flushing or set cascade action for the property to something that would make it autosave. Type: {0}, Entity: {1}", entityName, entityString));

So in your case exception won't be thrown by GetIdentifier(current, session); method. And result will be given to identifierType.IsDirty method.

@hazzik

the "IsDirty" method will not fail, but it will return false

Why false? What IsDirty returns depends on identifierType.IsDirty call. And from my understanding it should work fine - we will simply check if identifier value is changed. And if it's new object - it should definitely change.

[bahusoid]

Just in case I made a quick test with assigned id and it works OK. Should I squash it in to test commit? Or separate commit is OK?

[fredericDelaporte]

Yes, better add it to test commit. You may first add it as a "fixup! Test case for GH1419 (ISession.IsDirty() failed for transient many-to-one" commit and later squash it if you prefer.

Well, already added, never mind, can still be squashed later.

Ok, what will happen in case of an entity with assigned ID - the "IsDirty" method will not fail, but it will return false.

I believe IsTransientFast yields null instead in that case, but @bahusoid test seems to show I were wrong in assuming the whole case would fail. Adding tests for assigned id and unsaved-value "undefined" is definitely the way to go. (Maybe can we disregard the undefined case, using that option looks a bit like calling for troubles, I do not know why it even exist.)

[hazzik]

I was checking with Assigned Guids. Not sure why it works with integers.

[bahusoid]

@hazzik I changed int to Guid in my test and it still works...
Note: My first attempt with assigned id was also failing until I realized that I forgot to map new property in parent object )

[hazzik]

Maybe I had the same problem ;)

[bahusoid]

Added new test as separate commit. Let me know if it needs to be rebased and squashed instead.

[hazzik]

Thanks. Nice work