Partially port Hibernate's current field interceptor mechanism

[maca88]

As already discussed, the Hibernate field interceptor works differently that the current NHibernate's one and this PR is aimed to shrink the differences between the two. The most important difference that this PR ported is when the field interceptor is injected. In NHibernate the interceptor is injected after the entity is hydrated and in Hibernate it is injected right after the entity is instantiated. By injecting the interceptor at the entity creation the interceptor will track all uninitialized properties and we don't need to calculate them after the entity is initialized, which is what #1922 currently does. Another mechanism that this PR aligns is the ability to retrieve an initialized unwrapped proxy after the session is closed:

using (var s = OpenSession())
{
  order = s.Get<Order>(1);
  payment = order.Payment; // Initialize Payment
}
Assert.That(order.Payment, Is.EqualTo(payment));

The current code will throw as the payment was not set upon initialization.

Also, another thing that was done in this PR and does not work in Hibernate is to update the state of the initialized lazy="no-proxy" properties when they are set, for example:

using (var s = OpenSession())
{
  var order = s.Get<Order>(1);
  Assert.That(order.Payment is WireTransfer, Is.True); // Load property
  Assert.That(NHibernateUtil.IsPropertyInitialized(order, "Payment"), Is.True);
  order.Payment = s.Load<Payment>(2); // Load a different payment
  Assert.That(NHibernateUtil.IsPropertyInitialized(order, "Payment"), Is.False); // The initialized state of the Payment was updated
}

I think it is more correct to update the state as otherwise we are not able to detect if the Payment is really initialized or not.

There is still some differences that this PR does not address:

1. In Hibernate properties with lazy="no-proxy" are treated the same way as lazy properties, which means that the property will not be present in the query when the entity is retrieved from the database:

doInHibernate( this::sessionFactory, s -> {
  a = s.get( A.class, 1L );
  b = a.b; // Initialize b (b is a many-to-one relaton mapped as lazy="no-proxy")
  assertTrue(Hibernate.isPropertyInitialized(a, "b"));
} );

The above code will produce 3 queries:

• getting the entity a without containg the b property
• getting the b property
• getting the b entity

2. In Hibernate uninitialized lazy properties can optionally be accessed when the session is closed, a read only session is created by the interceptor to retrieve the value and then disposed.

I think that NHibernate behavior for lazy="no-proxy" is more practical/usefull as I don't see the point of not selecting the id of b as the main point of lazy propeties is to avoid expensive formulas or columns with large data.

The breaking change of this PR is the changed behavior of NHibernateUtil.IsPropertyInitialized which now returns the actual initialization state for the property.

EDIT:

By injecting the interceptor at the entity creation the issue of fetching a property mapped as lazy="no-proxy" for an entity that is not yet loaded is now working (#1267 second test).

Possible breaking change: assigning an uninitialized proxy to a no-proxy property will no more trigger the proxy initialization. Moreover, reading the property afterwards will no more unwrap the assigned proxy, but will yield it.

[maca88]

But putting an uninitialized proxy in a no-proxy property looks to me as quite a special case anyway

Not anymore as the interceptor is now injected from the entity creation so the interceptor will receive a proxy for a no-proxy relation in TwoPhaseLoad.InitializeEntity if the relation was not fetched. The interceptor has to be smarter in order to avoid loading all no-proxy relations in 2-phase load.
About updating the state there is one scenario that would be helpful which is serialization. In order to avoid serializing a not loaded relation a mechanism is needed that is able to check whether the property is loaded or not and if the mechanism does not have the actual state an uninitialized relation could be loaded which could cause performance issues. Indeed, normally if the relation exists we would want to include it in the serialization result, but when using a client side ORM like BreezeJS or JayData which are aware of entity relations and their loaded state we don't want the uninitialized relation to be loaded as it can be loaded from the client side ORM when needed.

[maca88]

On second thought the IsPropertyInitialized method does not work correctly today for no-proxy properties as the method should answer the following question: Was the property (column) retrieved from the database?
Currently, no-proxy properties in NHibernate are always retrieved so the method should always return true from them. But if we change that the following test would fail:

[Test]
public void WillNotLoadGhostPropertyByDefault()
{
  using (ISession s = OpenSession())
  {
    var order = s.Get<Order>(1);
    Assert.IsFalse(NHibernateUtil.IsPropertyInitialized(order, "Payment"));
  }
}

In order to correct that we would have to port the Hibernate behavior for no-proxy described in the first post so that the property is not retrieved when retrieving the entity, which IMO is not a good idea.
The other solution would be to leave the breaking change and provide a separate static method that would extract the no-proxy property without unwrapping it, which could be then tested with IsInitialized method.

[fredericDelaporte]

The fact that the proxy id is retrieved immediately is an implementation detail. I do not think IsPropertyInitialized on a no-proxy association has to care about it.

The normal behavior of a no-proxy association is to yield a hydrated, un-proxied entity. For doing this, it may use a proxy under the hood, but that is just a hidden implementation detail. IsPropertyInitialized has only to tell whether accessing the property will yield its value without triggering a db access, or not.

When only the association id is loaded, it will cause a db access: the property has to be considered non-initialized.
When the hidden proxy inner entity state has already been loaded, it will just yield that entity state without hitting the db: the property can be considered initialized.

But putting an uninitialized proxy in a no-proxy property looks to me as quite a special case anyway

Not anymore as the interceptor is now injected from the entity creation so the interceptor will receive a proxy for a no-proxy relation in TwoPhaseLoad.InitializeEntity if the relation was not fetched.

I hope this is still a hidden implementation detail: I mean, when reading the property, we still get an unproxied entity. Otherwise the purpose of no-proxy is defeated by the change here.

In such case, when an user put an uninitialized proxy in a no-proxy property, this is still an edge and a bit weird case. I have not check the behavior of the property in such case by the way, but if, when reading it afterwards, it yields an hydrated un-proxied entity instead of the proxy the user has assigned to it, that would be a debatable behavior. It should yield back what the user has assigned to it instead, for not breaking the unicity of the entity when working with a single session.

[fredericDelaporte]

I have "resolved" my first comment because it was based on a false assumption.

I think it is more correct to update the state as otherwise we are not able to detect if the Payment is really initialized or not.

I am not convinced of this, because we have two methods: IsPropertyInitialized, about the property state, and IsInitialized, about a proxy state, whatever the property it comes from (if any).

All this was about the case where the user assigns a proxy to a no-proxy property. I was assuming accessing the property afterwards would have yielded back the proxy, but this is not the case. It yields the unwrapped entity.

In such case, I agree the IsPropertyInitialized must yield false if the proxy was not initialized, because accessing the property will trigger a db-access.

About whether the no-proxy behavior should be changed for yielding back what the user has assigned to it, or not, better handle that in another PR. (But well, although I think this behavior debatable, it can be argued that it allows to have no-proxy somewhat strictly no-proxy. Why not, I do not think I would open this another PR myself.)

[maca88]

The fact that the proxy id is retrieved immediately is an implementation detail. I do not think IsPropertyInitialized on a no-proxy association has to care about it.

Fine by me.

IsPropertyInitialized has only to tell whether accessing the property will yield its value without triggering a db access, or not.

This is exactly how the current behavior of this PR works.

I hope this is still a hidden implementation detail: I mean, when reading the property, we still get an unproxied entity

Of course, this is the whole purpose of no-proxy.

I have not check the behavior of the property in such case by the way

Currently, NHibernate will initialize the entity on set as it does not return earlier when the setter argument of the Intercept method is true like this PR does.

It should yield back what the user has assigned to it instead, for not breaking the unicity of the entity when working with a single session.

I am ok with this behavior but we would still need to list this as a breaking change as currently the proxy will be initialized on set. This behavior could be achieved by checking if the session is set on the interceptor or not as the session is set in the AfterInitialize method which is after the 2-phase load.

In Hibernate when setting a proxy on a initialized no-proxy property works the same as you described that it should be working:

doInHibernate( this::sessionFactory, s -> {
  child = s.get( Child.class, 1L );
  assertThat( child, notNullValue() );

  assertFalse( isPropertyInitialized( child, "parent" ) );
  // Initialize parent no-proxy property
  assertThat(child.parent, instanceOf(Parent.class));
  assertTrue( isPropertyInitialized( child, "parent" ) );

  Parent p = s.load(Parent.class, 2L);
  assertFalse(Hibernate.isInitialized(p));
  // Set a proxy to the no-proxy property
  child.parent = p;
  assertFalse(Hibernate.isInitialized(p)); // set will not trigger the proxy to be initialized
  assertTrue( isPropertyInitialized( child, "parent" ) );

  assertTrue(Hibernate.isInitialized(child.parent)); //  fails as an uninitialized proxy is returned

} );

but in the following scenario the behavior is quite unexpected (at least for me):

doInHibernate( this::sessionFactory, s -> {
  child = (Child)s.createQuery("from Child fetch all properties where id = :id") // This is the same as s.Get<Child>(1) in NHibernate as no-proxy properties are always retrieved
      .setParameter("id", 1L)
      .uniqueResult();

  assertTrue( isPropertyInitialized( child, "parent" ) );
  assertTrue(Hibernate.isInitialized(child.parent)); // fails as an uninitialized proxy is returned
} );

I think that is not how it should work as the no-proxy property was not set by the user.

[fredericDelaporte]

It should yield back what the user has assigned to it instead, for not breaking the unicity of the entity when working with a single session.

I am ok with this behavior but we would still need to list this as a breaking change as currently the proxy will be initialized on set. This behavior could be achieved by checking if the session is set on the interceptor or not as the session is set in the AfterInitialize method which is after the 2-phase load.

As explained in this comment, I do not think this PR should change the property behavior of yielding the unwrapped entity instead of the proxy assigned by the user to the property.

Currently, NHibernate will initialize the entity on set as it does not return earlier when the setter argument of the Intercept method is true like this PR does.

So, as you write, small breaking change: assigning an uninitialized proxy to a no-proxy property will no more trigger the proxy initialization. It should not be really a trouble because the main reason for doing such an assignment is to try avoiding loading the entity.

[maca88]

I do not think this PR should change the property behavior of yielding the unwrapped entity instead of the proxy assigned by the user to the property.

With the latest commit the interceptor was modified to yield a proxy instead of unwrapping it when it is set by the user.

[fredericDelaporte]

For some reasons, dd9204b has broken the field-interceptor proxy serialization. I have not investigated why.

[maca88]

For some reasons, dd9204b has broken the field-interceptor proxy serialization

Actually this commit somehow broke the following test: StaticProxyFixture.SessionSerializationWithLazyProperty. The strange thing is that on my local machine this test passes with both net461 and netcoreapp2.0, so I am currently not able to reproduce it in order to see what is wrong.