Skip initialization of lazy properties when setting one

[maca88]

The aim of this PR is to have the ability to update uninitialized lazy properties without triggering their initialization, which is required by #1922 in order to update the fetched lazy properties.

This PR also modifies the behavior for properties mapped as lazy="no-proxy" to not be initialized when set, which solves the issue where a LazyInitializationException occurred when setting the property value while the session is disconnected (#1267 first point).

[fredericDelaporte]

In its current state, the PR will break the lazy property feature if some custom implementations of entity persisters or tuplizers, or of field interceptors, are used. I do not think it is acceptable for a minor version.
And for a major version, then better directly change the interfaces for forcing everyone to implement the required new methods.

So either working fallback, at worst assuming the old behavior (everything lazy either initialized or uninitialized), should be put in place, or the change should be set for the 6.0 milestone, with modified interfaces rather than new extensions method not supporting every cases.

Either ways, some possible breaking changes will be there and should be mentioned:
Possible breaking changes:

• Setting the value of an unitialized lazy property will no more trigger loading of all the lazy properties of the entity.
• If an unitialized lazy property has got its value set, without any other subsequent lazy property load on the entity, a dynamic update will occur on flush, even if the entity has dynamic updates disabled. This update will occur even if the set value is identical to the currently persisted property value.

[maca88]

Thanks @fredericDelaporte, I've used your fallback suggestion and solved all mentioned issues.

[fredericDelaporte]

Are you planning to do some more changes for keeping it WIP?

[maca88]

I've analized how Hibernate field interceptor works today and I am planning to align some things with it.
Hibernate has a applyInterception method in PojoInstantiator and it is overriden in PojoEntityInstantiator (does not exist in NHibernate) where the field interceptor is injected. The applyInterception is called when the proxy is created PojoInstantiator.instantiate which means that the field interceptor is injected from the begining without a session and afterwards in PojoEntityTuplizer.afterInitialize the sesion is set and $$_hibernate_clearDirtyAttributes is called (they have a more elaborate dirty check which is separated into two interfaces: SelfDirtinessTracker and ExtendedSelfDirtinessTracker). In NHibernate the interceptor is injected in PocoEntityTuplizer.AfterInitialize when all properties are already set. Their interceptor also does not trigger the initialization of lazy properties when one of them is set, it only populate the initializedLazyFields field when one is set. They have also a separate interface InterceptorImplementor

interface InterceptorImplementor {
  Set<String> getInitializedLazyAttributeNames();
  void attributeInitialized(String name);
}

for getting initialized lazy fields and attributeInitialized which is used to flag a field name as initialized. I did not understood exactly why attributeInitialized is called when a property is initialized as it will be flagged as initialized by the interceptor when set anyway. They also have a BytecodeEnhancementMetadata interface stored in EntityMetamodel which also contains the information about lazy properties for a type. Currently in NHibernate we have to calculate them in PocoEntityTuplizer instead.

So my current plan is to:

1. Port ApplyInterception method
2. Port PocoEntityInstantiator class
3. Port the BytecodeEnhancementMetadata interface
4. Modify the added GetUninitializedLazyProperties method into GetInitializedLazyFieldNames

About InterceptorImplementor, I am not sure if we should create a seaprate interface for GetInitializedLazyFieldNames.

[fredericDelaporte]

they have a more elaborate dirty check which is separated into two interfaces

Likely because they support custom dirtiness mechanisms, allowing an user to use something else than the default "compare with loaded state" mechanism. Usually, that "something else" is change tracking data like a dirty property per entity's property, set by some interceptor mechanism when a property is set.

Modify the added GetUninitializedLazyProperties method into GetInitializedLazyFieldNames

But it seems that what our code actually needs is to know about the uninitialized properties, not the initialized ones. They can be computed from each others of course, but better have directly what we need. Aligning on Hibernate helps porting of course, but I do not think it should be done at the cost of getting less optimal code.

The first three points are not strongly related to the change done by this PR. I would rather see them done as another PR, something like "Port Hibernate's current field interceptor mechanism".

[maca88]

I do not think it should be done at the cost of getting less optimal code.

You are right, will leave as it is, I only renamed the method name in IFieldInterceptor to be more consistent with other members.

I would rather see them done as another PR

Agreed, better to have them separated.

[hazzik]

@maca88 can you please clarify in the description what does it mean "related"?

[maca88]

Description updated.

[fredericDelaporte]

I am seeing this a bit late, but I think it should have been removed. It causes the built proxy to use an obsoleted method for getter implementations.

[maca88]

I didn't changed that on puprose as I tried to minimize the performance inpact because of the cast in the extension method. To be honest I didn't test how much the cast will actaully inpact on the performance, so if you know that it is irrelevant we should remove it.

[fredericDelaporte]

Well, I have mixed feelings on this subject, because the obsolete code in this IL generation case is nearer to what we should have in 6.0 than the non-obsoleted code.
But it feels as a bug that it compiles without failing due to referencing an obsolete symbol. Rider shows it as an error, but the compiler is fine with it. That is maybe a compiler bug for the nameof case when it refers an obsolete property. In such case the compilation may break on a tooling update.

[maca88]

I did a quick test and it seems that for calling a no-op Intercept method is twice faster without the cast for a million calls, which is not a big deal as casting a million times takes approx. 9ms. So we could remove it in order to avoid showning as an error in Rider. It seems that the compiler is not smart enough to detect that the only reference that the nameof references is an obsolete member.

[fredericDelaporte]

Or you can suppress the warning with directives around that line.