Make ISession.IsDirty() method immutable operation

[bahusoid]

ISession.IsDirty() method description:

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

So expected behavior: method should not persist any entities (work in read-only mode)

And current behavior: It's very unexpected that this call can lead to data modifications in DB:
all cascaded entities are saved (at least all with Identity ID lead to DB inserts - very unexpected and dangerous behavior)
Edit: that point is fixed in 5.4 by #2752, but the session state can still be mutated by IsDirty: this needs still to be fixed.

My use case for using this ISession.IsDirty() is the following:

var entity = LoadEntity();
//Do some modifications to entity like entity.Children.Add(child); 
//which is mapped with Cascade.All; Also child Id is Identity generated by DB
entity.Children.Add(CreateNewChild());

//... And at the end of the request:
if(session.IsDirty())
{
	//do some resource consuming stuff like open DTC transaction or something else that needs to be done only if database modifications are requried
	using(var dtcTransaction = OpenDTCTransaction())
	{
		session.Flush();
		//...do some stuff with legacy VB objects
	}
}

With current implementation I can't use IsDirty() as it will save child entity.Children elements without transaction context - very dangerous and unexpected.
See test case #1414

[fredericDelaporte]

Seems related to NH-2727 and NH-2136 which were closed as "Not an issue" without much explanations.
About NH-2136, the case happens with identity generator. Maybe there is some good technical constraint mandating this behavior with identity, but it should then be documented on IsDirty (and added to the list of reasons it is not recommended to use identity for generating ids).

[hazzik]

Maybe some information is in the mailing lists (I’m away sleeping, so could not check now)

[hazzik]

Read the linked issues. I don’t know why this is an “expected behavior”, @fabiomaulo. Could you please elaborate? I agree that the behavior should be as described by @bahusoid.

[fabiomaulo]

I can't remember which was the discussion around this matter... let me check...

[fabiomaulo]

Ok... IIRC things, in these cases, are wired with flush mode. In NH-2727 Ricardo said he is changing the entity during saveorupdate (probably with a listener) so the session becomes dirty. IMO as he have implemented a custom-listener to do something during saveorupdate he should change even DefaultDirtyCheckEventListener.
The second issue is easier... the usage of Identity strategy for the POID is the cause... the only way to know an ID is going to DB. hmmm, I think I wrote a post some years ago, maybe more than one:
http://fabiomaulo.blogspot.com.ar/2008/12/identity-never-ending-story.html
http://fabiomaulo.blogspot.com.ar/2009/02/nh210-generators-behavior-explained.html
There should be some others... Identity is a very ugly beast.
BTW: Perhaps the behavior of the default listener can be changed, fixing Ricardo's issue, even if we have to have extremely careful because somebody may change something, in the graph of the loaded objects, using events wired to the session lifecycle so we may have a fake state (the IsDirty throw a false but something will be saved on flush).

[fredericDelaporte]

The point with identity is, why should IsDirty knows the id? If it is mandatory for checking dirtiness, so be it, but otherwise it should avoid triggering the id generation (which in the case of identity triggers the whole insert).

About NH-2727, it looks to me as boiling down to this question: is it legit for IsDirty to raise SaveOrUpdate events?

By the way this issue 1413 is indeed two issues: the many-to-one failure and the id generation triggered by IsDirty (which is maybe bound to the fact it raises SaveOrUpdate, I have not checked anything in code yet about all that).

[fabiomaulo]

Yes Federic, in general terms something called IsDirty should not call something called SaveOrUpdate... probably there is a short way to check and should be re-wrote.
The dirty check is not so easy when you work with collections of objects graphs where you have to deal with the cascade behavior...anyway a dirty collection become a dirty session.

[hazzik]

So, it seems that this is the same as NH-2136, except it happens to anything regardless of the type of id generator.

[hazzik]

This is actually p: Major due the fact if there is no transaction it will perform a real flush to the database, which would be considered as a manual flush by the core.

[trivalik]

Does this happen also in older NHibernate 3.x?

I am right now struggling with a huge performance gap after migrate NHibernate from 3.1 to 5.3.16/5.4.2 on calling IsDirty, Since the call is near after every change in program, it lead to a less fluent work flow.

[fredericDelaporte]

I am not sure whether this bug applies to such an old version or not, but is sounds likely to me. IsDirty is implemented almost like a Flush theoretically without the updates sent to the database. That is the reason for the bug in some special cases. That implementation has not changed much as far as I know.

About the performance loss for your application, if you could pinpoint what causes it (like profiling sql queries and seeing if there are more of them issued on IsDirty calls, or other things), that would help seeing if that is something needing a fix, and then please report it as a new issue with detailed information of how to reproduce it.

About the principle of calling frequently IsDirty, that is anyway a huge source of performance loss. The basic mechanism for dirty checking in NHibernate has not changed since its first releases: at worst the session iterates all entities it holds and compares their current state with the state they were having at load time. That is a quite heavy mechanism.
(Hibernate (Java counterpart) has some extension points allowing to use different dirty checking mechanisms, but they have not yet been ported to NHibernate.)
With such a mechanism, it is better to use dirty checking the least possible. (Actually I had never used it in applications I had worked on.)
Especially, there is no point in coding something like if (session.IsDrity()) session.Flush();: flush anyway performs a dirty check itself, so such a code just triggers a double dirty check when the session is dirty, without improving the performances of the "not dirty case". A variation of such an anti-pattern can also be if (session.IsDrity()) session.Commit(); else session.Rollback();: just commit instead. There is nothing to rollback anyway if the session is not dirty, so committing instead is equivalent.

[trivalik]

I will keep that in mind. That I try to make an example in my spare time. I will post at least the profiling result in a new issue.

[fredericDelaporte]

I forgot about #2752: is the current description of the issue still relevant? I mean, may we still have entities inserted into the DB on dirty checking? Normally, with #2752, we only have at worst new entities added into the session, but not sent to the database.

[bahusoid]

I forgot about #2752: is the current description of the issue still relevant?

No DB modifications but session state is still modified (see #2752 (comment))