Enable one-to-one optimistic lock handling in mapping

[bahusoid]

Regression from #2576
Before 5.4.0 one-to-one changes are always ignored for version update (as if optimistic-lock is set to false in mapping). And you can't control it by mapping.
In 5.4.0 version is always updated (and you can't control it by mapping too). Thus it's a possible breaking change.

This PR keeps new behavior as it's consistent with other optimistic-lock mappings but allows to change it in mapping (by setting optimistic-lock to false for one-to-one mapping)

See #3204

[bahusoid]

Though it never worked it seems original intention was to always update version. So maybe it should be true by default (as with all other properties). So as an alternative we can document it as possible breaking change with ability to restore old behavior in mapping

[fredericDelaporte]

So, it looks to me the old behavior was a bug. As such we should not restore it, and it is not even a possible breaking change, but an undocumented bug fix.

[bahusoid]

It looks to me the old behavior was a bug

Version update depends in first place on owner object dirtiness. And IsDirty returning false for OneToOne seems was also intentional behavior:

nhibernate-core/src/NHibernate/Type/OneToOneType.cs

Lines 60 to 62 in 37b5020

	public override bool IsDirty(object old, object current, ISessionImplementor session)
	{
	return false;

So it's hard to say where the problem is considering that intentional behavior is not documented and not changeable and old behavior was always there.

So my thought on this - yes 5.4.0 default behavior looks legit (behaves consistently with other mappings). But then we should allow to restore back to old behavior.

[fredericDelaporte]

I thought it could still be changed through mappings. If that is the case, it seems enough to me. Introducing some setting to restore a previous behavior akin to a bug (by principle of consistency at least) looks overkill to me.

I understand it means affected applications will have to fix their mapping, which can be huge. But when code relies on a behavior undocumented and inconsistent with similar features, I think that is acceptable to offer no better than instructions for fixing the code when said behavior is fixed.

[bahusoid]

I thought it could still be changed through mappings.

Nope. This PR implements ability to control it through mappings.

Ok so to conclude: we keep new behavior as it's consistent with other mappings. But this PR allows to restore old behavior in mapping (by setting optimistic-lock to false for one-to-one mapping)

[bahusoid]

Hm. #2576 was also ported to hibernate so I checked what's going on there. They reverted this change completely hibernate/hibernate-orm#5330

And the reason seems to be similar to this issue @deAtog Can you shed light on it? Should we be worried too?

[deAtog]

In my opinion, the OptimisticLock property should always return false. Prior to the changes I made, the IsDirty property of the one-to-one always returned false. As such the version property was never incremented if the value of the one-to-one was modified. The changes I made to the IsDirty property caused a regression with versioning. Whenever a transient child was associated with a persistent parent it caused the version of the parent to be incremented. None of my original tests included versioning, so this issue was not discovered at that time.

Due the regression, the nhibernate-orm developers decided to revert my changes to address the caching issue. The reason given was that the IsDirty property should only be used for database updates and not cache related updates. Since a one-to-one does not have a backing field in the database, they felt it should never be dirty. Needless to say, there is no way to currently differentiate between a cache or database related update as the two are currently combined into a single update event based upon the IsDirty property. As such, I disagree with their analysis that the IsDirty property should be reserved solely for database related changes. Likewise I cannot find any utility in incrementing the version number whenever a one-to-one is modified. As such it is my opinion that the OptimisticLock property should always return false for a one-to-one as stated above.

For example, modifying a constrained one-to-one property of a child with a foreign key identifier currently does not change the identifier of the child. Likewise, adding or removing a child from a persistent parent does not modify the parent. As such, there is little utility in optimistic locking in either of these cases as they would only increment the version number.

[bahusoid]

The reason given was that the IsDirty property should only be used for database updates and not cache related updates

I don't quite understand what "database update" means. Does it mean some columns from owner table need to be modified? But it doesn't work this way for collection mappings. If collection is modified owner is considered dirty (though no owner columns are modified by this update) In this regard I don't see much difference between collection mapping and one-to-one mapping.

[bahusoid]

Likewise I cannot find any utility in incrementing the version number whenever a one-to-one is modified.

Same reasons as with any other version updates - to avoid concurrent owner modifications even indirect (like with collections) So I'm not convinced I still think current 5.4 behavior is consistent with other mappings and with this PR can be configured to restore old behavior.

[deAtog]

A "database update" refers to a modification to existing data in the database. E.g. when you add a child to a persistent parent, the child creates an insert into a related table but ordinarily does not modify the existing data of the parent. Thus the one-to-one on the Parent though dirty does not normally cause a database update. As I stated above I disagree with their assessment of the IsDirty property.

I'm okay with these changes as the default for OptimisticLock is set to false. Personally, I would never set it to true, but I guess you have an argument for where it might be useful despite the inherent performance implications.