Fix caching of OneToOneType from second level cache.

[deAtog]

The changes here are based on the current implementation in the ManyToOneType. These changes fix issue #2552 by storing the ID of the related one-to-one object when disassembling the parent/owner. When assembling the parent/owner the ID of the one-to-one is no longer assumed to be that of the owner and when it is null, it is reliably so. This eliminates the cache-miss and subsequent database query when the related one-to-one object was null.

Fixes #2552

[hazzik]

Need to have tests.

[bahusoid]

Maybe we should keep old logic when it's safe to use owner id (when IsReferenceToPrimaryKey && !IsNullable)...

[deAtog]

Maybe we should keep old logic when it's safe to use owner id (when IsReferenceToPrimaryKey && !IsNullable)...

I think adding the check for that particular case would only add additional unnecessary complexity.

[bahusoid]

Maybe we should keep old logic when it's safe to use owner

Or maybe not - as it seems proper handling would also require some ugly code we have in Hydrate

[bahusoid]

The changes here are based on the current implementation in the ManyToOneType

It seems we need to introduce base type ToOneType with common functionality (no need to do it in this PR)

[hazzik]

It seems we need to introduce base type ToOneType with common functionality (no need to do it in this PR)

EntitiyType is that ToOneType

[deAtog]

This seems to cause a regression when committing a transaction that adds a related one-to-one object to an existing entity. Nhibernate does not currently have a test case for this, but Hibernate ORM does. I'll try to resolve this issue next week and add the related test.

[bahusoid]

Hibernate Issue: https://hibernate.atlassian.net/browse/HB-992

Trouble is that no SQL UPDATE is needed, so Hibernate does not consider the owning object to be dirty (at least as far as the db is concerned). However, it /is/ dirty as far as the cache is concerned.

.

[deAtog]

Right. That's the problem that I've confirmed.. The related entry gets persisted to the database, but the cached entry of the owning entity is not updated. If you have an idea of how to fix this, that would be helpful.

[deAtog]

I added the basic one-to-one persistence test that was failing in Hibernate ORM. This test fails after changing the disassemble and assemble methods, but passes once the isDirty method is implemented. The isDirty check causes an update to the owner to occur, but since the one-to-one property is not updatable, it only hits the cache and not the database. In the case that was failing, this causes the null id of the one-to-one object to be updated with the correct id in the cache.

[bahusoid]

but since the one-to-one property is not updatable, it only hits the cache and not the database

Are you sure? I don't think it works this way. If object is considered dirty NHibernate will trigger update for all properties unless dynamic-update is specified. So I believe now unnecessary update statement is generated. And I'm not even sure that dynamic-update case is handled properly big chance that invalid SQL is generated for this case.

[deAtog]

@bahusoid Yes, I am 100% certain of this. An Update action is added to the ActionQueue with the only change being the ID of the related one-to-one property. Since that value is not undateable on the owner, it is not considered as part of the database update. Since nothing else is modified in this case, the database update is skipped but the change is still persisted in the cache.

[deAtog]

To be more specific, the flow is as follows:

1. An EntityUpdateAction is added to the ActionQueue for the owner.
2. All actions in the queue are executed via a call to ActionQueue.ExecuteActions() which calls EntityUpdateAction.Execute().
3. EntityUpdateAction.Execute() obtains the entity persister which in this case is a SingleTableEntityPersister which is derived from the AbstractEntityPersister.
4. Once the persister is obtained Execute() calls persister.Update() with the list of dirty fields. (In this case only the OneToOneType field is dirty.)
5. AbstractEntityPersister.Update() calls GetTableUpdateNeeded which iterates over the list of dirty fields and marks the tables that need updated based on which fields are dirty and updatable.
6. GetTableUpdateNeeded returns false for all tables as the OneToOneType field is dirty but not updatable.
7. AbstractEntityPersister.Update() iterates over the list of tables requiring updates and performs the update if needed. (No updates are needed and the function returns.)
8. Finally, EntityUpdateAction.Execute() updates the cache with the current state of the entity.

[deAtog]

I updated the tests for this based on feedback from the Hibernate community. The tests no longer make use of the query cache nor rely on the BatchableCache. I also pushed just the tests prior to the fixes so that you can observe the failure case prior to any changes.

[deAtog]

@hazzik In my opinion, this is ready to be integrated after review. These changes mirror the ones I've proposed for Hibernate

[hazzik]

There are some comments for consideration and suggestions to code.

[hazzik]

Because this code is now identical for OneToOneType and ManyToOneType it would be better to move it to EntityType (the base of both types).

For discussion:

• do we want to implement IsDirty(object old, object current, ISessionImplementor session) in EntityType or;
• create a new method IsDirtyCore... and call it from overrides? /cc @bahusoid @fredericDelaporte @maca88

[deAtog]

The implementation of IsDirty in this case is identical to the implementation of the ManyToOneType. Maybe the common functionality can be combined in common ToOneType in a future enhancement. The purpose of this change set is only to correct the above mentioned issue.

[bahusoid]

@hazzik Either way works for me. Simply moving IsDirty to base looks as less code...

It seems IsModified should also be modified accordingly for OneToOneType. But have no idea when it's really needed...

[deAtog]

@bahusoid IsModified is inherited from AbstractType which returns true/false based on IsDirty.

[hazzik]

I made a typo. It should be code is now identical not code is no*t* identical. Fixed.

[bahusoid]

IsModified may not handle the case when old is an instance of a OneToOneType and not an identifier

I don't get why you think it's unrelated... It's related.
Old logic: handles both identifier and entity instance in old parameter (because it simply returns null)
If you make IsModified call IsDirty (as you thought it's been working) - your new logic will fail when IsModified is called with identifier. Because now GetIdentifier might be called on old instance (and it will surely fail if old is already identifier)

[deAtog]

You are correct, IsModified always returns false. This is consistent with the original behavior of the type that you cannot modify owner of the owner of the OneToOneType once assigned. I'm not against changing this behavior but I think that doing so would be the topic of another issue. IsDirty determines whether or not the parent should be considered dirty or not, while IsModified determines whether or not an update is required to the identifier of the OneToOneType. Since updates are not currently permitted, IsModified always returns false.

[bahusoid]

Maybe I just don't understand something. But from what I see DefaultFlushEntityEventListener.DirtyCheck can call both methods for obtaining dirty properties depending on current session state:

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

Line 452 in 6ab71d0

dirtyProperties = persister.FindDirty(values, loadedState, entity, session);

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

Line 480 in 6ab71d0

dirtyProperties = persister.FindModified(databaseSnapshot, values, entity, session);

With only difference that IsModified is called on entities added to session via Update call and using databaseSnapshot (and identifier related code was added to many-to-one to handle such db snapshots). So to me it looks like those method should behave the same as both of them can be called for dirty check.

So regarding

IsDirty determines whether or not the parent should be considered dirty or not, while IsModified determines whether or not an update is required to the identifier of the OneToOneType

Can you clarify where did you get this difference and how it plays with DefaultFlushEntityEventListener.DirtyCheck? And I don't really understand what determines whether or not an update is required to the identifier of the OneToOneType means considering identifier of the entity is not updateable. And if you mean in parent object - it's still can be changed from null to something and vice-versa.

[deAtog]

IsDirty has the following comment:

/// <summary>
/// Should the parent be considered dirty, given both the old and current 
/// field or element value?
/// </summary>

IsModified has the following comment

/// <summary>
/// Says whether the value has been modified
/// </summary>

Pseudo code for DefaultFlushEntityEventListener.DirtyCheck the following:

L428-434 Get entity, current property values, loaded property values, id, and persiseter.
L436 Check if the Interceptor handled the Dirty check.
L443 If the Interceptor did not handle the dirty check, then attempt to dirty check the entity.
L448 If the loaded state of the entity is null, then the entity is transient and cannot be dirty checked.
L449-453 If the entity was loaded then compare current property values against loaded property values to determine if the entity is dirty.
L454 If the entity was deleted, compare the current property values against the deleted state.
L474-484 Get the current values from the database if select before update is required or check the id against the unsaved value. If a database entry exists or the id is not equal to the unsaved value, check for modified property values.

Based on the above and the fact that the OneToOneType is not updatable, I see no reason why IsModified should ever return anything but false for the OneToOneType.

[bahusoid]

L448 If the loaded state of the entity is null, then the entity is transient and cannot be dirty checked.

loadedState can also be null if entity is added to session via session.Update . It actually has explicit comment about it... So I expect with current PR state we still have issue with cache when owner is added to session via session.Update and then updated (owner also should be mapped with select-before-update="true"). Can you prove me wrong with test case?

[bahusoid]

For many-to-one it's true only for not-found="ignore". Why it's always true here? Shouldn't it be here similarly true only for constrained="false" associations (so basically return IsNullable;)

[deAtog]

There are two use cases for a one-to-one with constrained="false" on the parent side and constrained="true" on the child side. From my understanding of IsAlwaysDirtyChecked only determines whether or not the relations should be checked to see if it is dirty. When constrained="false" we definitely want to always perform the dirty check. When constrained="true", the id of the child should match the id of the parent and so presumably it can never be dirty. I think returning IsNullable here would be acceptable.

[bahusoid]

So to finalize this PR the following needs to be done:

1. IsModified needs the same handling as in many-to-one (see OneToOnePersistedOnOwnerUpdateForSessionUpdate);
2. IsAlwaysDirtyChecked should also be adjusted;
3. Move common logic to EntityType. I'm actually fine to make this refactoring as separate task later.. So I see it as optional point

[fredericDelaporte]

Possible breaking change: now one-to-one changes trigger a version increment. #3216 is required for reverting to the old behavior.