Test ISession.IsDirty() should not trigger cascade saving

[bahusoid]

Added failing test cases for #1413.

[fredericDelaporte]

Thanks for the test case.

[fredericDelaporte]

Should be GH1413 instead of GHTODO, here and in other files.

[fredericDelaporte]

For new tests, we favor NUnit constraint model over its classic model, see two models.

[bahusoid]

@fredericDelaporte Done. I'm not very comfortable with this "constraint model" - so let me know if anything is not right.
Note: Exception handling seems better to me in classic model. At least Assert.DoesNotThrow looks more concise/clear to me than Assert.That(.., Throws.Nothing,...)

[fredericDelaporte]

Thanks for the change.

[bahusoid]

PR updated (I cleaned up all not related stuff)

[hazzik]

Could you please modify the test to check that there were no inserts performed (with SqlLogSpy)? It seems that with the current state it does generate IDs but does not actually saves anything to the database.

[bahusoid]

@hazzik Sorry I won't be able to do any coding in the following week or two.
And yes - current test doesn't generate any SQL inserts. But it surely will If I change EnityChild.Id mapping from Generators.GuidComb to Generators.Identity.

The main point of the test - IsDirty() should not modify nor entities in memory nor DB state. It should be immutable operation. Don't you agree?

As @fredericDelaporte already pointed out here:

is it legit for IsDirty to raise SaveOrUpdate events?

So this test checks this part - and I think there is no need to do additional checks if SQL is really generated. Let me know if you disagree - I will update tests as you think is right.

[fredericDelaporte]

I have added statistics instead, with native generator. See commit details.

[hazzik]

@fredericDelaporte this way this issue becomes a duplicate of NH-2136. Which is closed "By design". The problem is that if we want to fix this issue - we need completely rewrite IsDirty.

[fredericDelaporte]

The point is, was it really reasonable to close this as "by design" without more actions? I do not think so. The bare minimum would be to document the possible side effects of IsDirty, at least in its xml comments.

Currently it just states:

Does this ISession contain any changes which must be synchronized with the database? Would any SQL be executed if we flushed this session?

So a method explicitly documented as allowing to check if some SQL may be triggered by another operation may itself triggers some SQL without giving any clue about that... Quite unexpected. And it causes IsDirty to be somewhat moot for those falling in the cases triggering SQL. (identity generator => insert + all generator db-sided => id generation triggered.)

[fredericDelaporte]

I have checked the Hibernate implementation, it is still the same than NHibernate on this point.
About reported issues on Hibernate side, none have been rejected as "by design", they are closed by lack of test case or information instead. It seems they have never been seriously studied:

• HHH-404 - issue with a "please help" quality level, it was not really deserving more than a close.
• HHH-3963 - it was providing a test case, but it was not handled until a new major version of Hibernate, then closed because the opener did not confirm the issue was still there with the new version. Hum, interesting way of handling issues: applying it to NHibernate bugs would likely close a good bunch of them!
• HHH-6611 - closed because not providing a test case.
• Forum post with an invalid suggestion. At least in NHibernate case, changing the flush mode does not dodge the issue. There are other forum posts on the subject, without any useful answer.

we need completely rewrite IsDirty

If it was about rewriting all the IsDirty methods, it would be a bit overwhelming. But it looks to me we "only" need to rewrite the DefaultDirtyCheckEventListener class, which seems to be only used by SessionImpl.IsDirty. So it does not look to me as a too big deal, although it would likely imply to somewhat rewrite a good deal of AbstractFlushingEventListener into DefaultDirtyCheckEventListener but in a way without side effects. It could have two drawbacks: de-synchronization risk between the IsDirty checking logic and the actual flush changes ; loss of the optimization reducing the number of dirty check to be done after an IsDirty call (the ClearFromFlushNeededCheck call currently done in DefaultDirtyCheckEventListener). But the IsDirty could also gain some speed through early exit as soon as it finds something dirty.

[fredericDelaporte]

Having looked a bit more into it, it would require a lot of changes in the cascading logic for allowing a "cascade simulation" just telling whether a save would be done or not. So I think we should likely just document the potential side effects of ISession.IsDirty.

[bahusoid]

If I try to go this way:

we need completely rewrite IsDirty

Will you consider it (I suspect it will end up in copy-paste from flush routine)? Current implementation seems like not very useful as identity generators are commonly used (at least in projects where I was involved). And processing all the objects in session - doesn't look as effective solution.

[fredericDelaporte]

The dirty checking mechanism is injectable at many levels, including for the global dirty check on the session. You can change the default isDirty event handler for the session by setting another one, implementing IDirtyCheckEventListener, and set by using Configuration.SetListener. (It can be done in xml configuration too, see here.) Its default implementation is DefaultDirtyCheckEventListener.
A custom one not triggering the save should likely not inherit from AbstractFlushingEventListener but directly implements IDirtyCheckEventListener on its own.
Now for reducing code duplication it would likely instead require to adapt AbstractFlushingEventListener and the cascade mechanism, but doing so without breaking changes is challenging (otherwise it would mandate a new major release).
The advantage of the custom dirty check event listener would be to be doable as a separated project, usable with current NHibernate v5.

Changes made

[hazzik]

This can "easily" be fixed by enabling this branch of code:

nhibernate-core/src/NHibernate/Event/Default/AbstractSaveEventListener.cs

Lines 256 to 266 in 0b66742

	if (!shouldDelayIdentityInserts)
	{
	log.Debug("executing identity-insert immediately");
	source.ActionQueue.Execute(insert);
	id = insert.GeneratedId;
	//now done in EntityIdentityInsertAction
	//persister.setIdentifier( entity, id, source.getEntityMode() );
	key = source.GenerateEntityKey(id, persister);
	source.PersistenceContext.CheckUniqueness(key, entity);
	//source.getBatcher().executeBatch(); //found another way to ensure that all batched joined inserts have been executed
	}

Unfortunately due to "NH Different behavior (shouldDelayIdentityInserts=false anyway)" it never executes.

Hope this helps.

[bahusoid]

I think that proper dirty check shouldn't call PerformReplicateOrSave at all. But it looks like a huge refactoring I'm not ready to start right now. And if #1754 fixes the main issue (unexpected db inserts) maybe it's not worth implementing the "proper" way.

[hazzik]

And if #1754 fixes the main issue (unexpected db inserts)

It does not. We need the other means to enable delayed inserts for the dirty checks.

[hazzik]

Rebased, squashed, commit reworded, test is marked as known bug.