Fix deletion of component with null property

[fredericDelaporte]

Fixes #1170 - Incorrect query when items removed from a collection
of components contain null values

NH-3634 has added support of null valued component properties in select queries comparisons, but they are indeed still not properly supported since this still fails when deleting.

[fredericDelaporte]

All commits should be squashed.

The last commit may be dropped if preferred: it could be a possible breaking change for external users of the impacted utility methods, if they use it on array of a type implementing IEquatable without overriding object.Equals or with an object.Equals override having a different semantic than the one from IEquatable. The previous implementation was always delegating equality to object.Equals.

[fredericDelaporte]

This "forced to not-nullable" thing seems to me a bit undue, as this impacts only ddl generation, forbidding to test such cases (unless tweaking the generated schema afterward), but not forbidding users not using schema generation to map such things, and insert null. Then these cases will have the delete issue too, and also an issue on update. But no one seems to ask for nullable support on those cases, so I have not handled them.

[fredericDelaporte]

Not overriding it causes the set to always delete all its elements, causing the update test to be even more moot. (It is anyway somewhat moot, as with set, updates are handled by delete/insert.)

[hazzik]

I think this behaviour needs to be changed (in a separate PR).

[fredericDelaporte]

Do you mean, if the component reference has not changed, update it instead of deleting/inserting it? But the persistence context loaded state does not keep track of component references and so, does not allow to check for that condition.
Or do you mean we should somewhat "recycle" components to delete by updating them with data of components to add in order to replace as most as possible deletes/inserts by single updates? That would cause actual removals combined with adds to be replaced by updates, which could be quite unexpected for users, and breaking for custom auditing logic (like trigger based auditing).

[hazzik]

I'm talking about this behaviour: "Not overriding it causes the set to always delete all its elements" it shouldn't be like this.

[fredericDelaporte]

From here:

Note: if you define an ISet of composite elements, it is very important to implement Equals() and GetHashCode() correctly.

It is in fact an error to map a set of components without a proper equal semantic. The set cannot enforce its semantic in such case: better use a bag instead.

Of course the side effect this error has about flushes, causing the whole set to be purged and recreated, could still be fixed. But that would be fixing a case resulting anyway from a bad mapping/implementation on the user side. Why not if this can be done in a simple way with very few impacts on normal uses, but otherwise, I do not think it should be fixed.

Fixing it will likely require to extract the set content as component values arrays, then searching for each array of values if we can find the same array in the loaded state. This looks to me as a non-negligible overhead, compared to just using the set Contains method. Moreover it would then ignore equals implementations having another semantic, which will be a possible breaking change.

[fredericDelaporte]

Not replaced yet for avoiding a possible breaking change for those having overridden it.

[fredericDelaporte]

Rebased for also updating the documentation. (Fixup commit already squashed.)

[hazzik]

Turns out this magic already exists in the .NET Fx:

Array implements IStructuralEquatable interface:

var a = new[] {1};
var b = new[] {1};
IStructuralEquatable aa = a;

var @equals = aa.Equals(b, EqualityComparer<int>.Default); //true

[fredericDelaporte]

But that magic works by comparing everything as objects as it uses the non generic IEqualityComparer interface, causing value types to be boxed.
The ArrayEquals(byte[] a, byte[] b) overload would be likely less efficient if re-implemented with this.
The way I have re-implemented this (with the last commit) also allows ArrayEquals<T> to avoid boxing.

[israelaece]

I'm using the NH 5.3.5 and I continue dealing with this issue (removing itens from <set> collection), but, in my case, the composite-element has three properties, two of them are many-to-one.

Note that my DocumentPrefix property is not-null="true". When I set this property to not-null="false" the problem goes away, but in terms of my business domain, this property is not null. Anyway, I can live with this behavior; I'm just reporting the issue to listen your opinion if this is by design or not. Thanks a lot!

<set name="Itens"
     cascade="all-delete-orphan"
     lazy="true"
     access="field.camelcase">
  <key column="ParentId"/>
  <composite-element class="Avalista">
    <property name="DocumentPrefix"
              type="AnsiString"
              not-null="true">
      <column name="DocumentPrefix"
              not-null="true"
              sql-type="varchar(11)" />
    </property>
    <many-to-one name="Person"
                 column="PersonId"
                 not-null="false"
                 lazy="false"
                 class="Person" />
    <many-to-one name="Company"
                 column="CompanyId"
                 not-null="false"
                 lazy="false"
                 class="Company" />
  </composite-element>
</set>

[fredericDelaporte]

It is better open a new issue for this, with a more complete example. (With which data does it fails, ...)