Fix TryGetMappedNullability for CompositeCustomType

[PleasantD]

Fixes #2937

[fredericDelaporte]

Thanks for your suggestion and time invested in this trouble,

As this seems to be a regression (although I have not located what has triggered it in 5.3.10), can this PR be retargeted on 5.3.x?

An async regen is also needed and missing. (I can add it if you wish.)

[fredericDelaporte]

To give some context, this matches your suggestion here:

I think what needs to happen there is that if componentType is CompositeCustomType then we can "inherit" the nullability from the nullability of the "parent" property. This could be accomplished by removing the last element of the memberPath and letting the logic continue with the entityPersister property logic.

I do not think this is enough, because there are other cases than CompositeCustomType which are an IAbstractComponentType but are not yielding an instance for their PropertyNullability property, like AnyType.

For me, the minimal correction should be to just ignore them, so rewrite the if (componentType != null) as if (componentType?.PropertyNullability != null).

About inferring the nullability from the overall component nullability when it is a custom component, why is it valid? Sure, if the component is nullable, then the property must be nullable too. But a component property can be nullable while other properties of the component are not, causing the component to be not nullable. So we cannot know if the property is nullable or not when all we know is that the component is not nullable.

If think it would be safer to just change the test as I propose without testing the parent nullability as you propose.

[bahusoid]

the minimal correction should be to just ignore them, so rewrite the if (componentType != null) as if (componentType?.PropertyNullability != null).

But it would lead for further processing as no component. It should just quit returning false. Something like:

 if (componentType != null)
{
	if (componentType.PropertyNullability == null)
		return nullable = false;
...

[PleasantD]

@bahusoid Your suggestion leads to a failure in the test case.
The AssertResult helper method asserts that if TryGetMappedType returns true (which it does) then TryGetMappedNullability should also return true.

In response to @fredericDelaporte
If we take your suggestion then we get a similar failure in the test case.
This time it is because entityPersister.EntityMetamodel.GetPropertyIndexOrNull(memberPath) will return null if the memberPath contains the mapped property (eg Multiple.count).
Since the entityPersister.EntityMetamodel contains no mapping for this composite property we again return false for TryGetMappedNullability

So either the test is wrong and needs to be changed, or the logic I provided is correct.

With regards to the possibility that AnyType might cause problems: I believe that this may not be possible because the prior call to TryGetMappedType should would return false if trying to access any unmapped property.

---

There is one other possibility, which is that my logic is half correct.
The nullability of a property of a nullable composite user type might correctly be (pseudo code)
nullable = parentNullability || propertyNullability
Where for something like o.CustomType.MappedProp the nullability of MappedProp is true if o.CustomType is mapped as nullable OR MappedProp (as defined by the custom type) is itself nullable.

What my proposed solution does is only the first half of the logic.
I don't know what would be the correct way to resolve the second half, but I think it's reasonable in most cases to ignore it.
The only time it would be incorrect would be if o.CustomType is mapped as non-nullable but MappedProp is a nullable type (or column).
I don't know how this nullability is being consumed, so I don't know if that slight incorrectness would cause incorrect behaviour or expectations elsewhere.

[fredericDelaporte]

I think the test is wrong for things mapped as AnyType or component user types. But I have not had a look at it yet to check.

What my proposed solution does is only the first half of the logic.

Yes, that is precisely what my paragraph starting with "About inferring the nullability from the overall component nullability when it is a custom component, why is it valid?" is about.

I don't know how this nullability is being consumed, so I don't know if that slight incorrectness would cause incorrect behaviour or expectations elsewhere.

I have not checked but I think it would cause Linq null comparisons to fail. In NHibernate, we attempt to mimic Linq-to-object behavior about null comparison, which requires additional or SQL clauses to introduce is [not] null checks, since in SQL semantic, (=|<>) null yields always "unknown". But these clauses come at a performance cost, so NHibernate avoids them when a property is deemed not nullable. If the property is actually nullable, then querying for cases where it is null will fail to find matching data.

[PleasantD]

I see your point, and agree.
I'll admit I'm out of my depth on this one and I don't think I'll be able to commit any more time to working on a solution right now.

My "naive" solution is good enough for my company's purposes since the way we map custom composite type properties is always non-nullable. Since we've been waiting months for a viable 5.3.x version and can't wait any longer, we'll depend on an internal build for now and hopefully this gets fixed correctly sometime soon.

[fredericDelaporte]

About AnyType, where does this comes it would be used for unmapped properties? It is about the any property mapping, so, something mapped.

[PleasantD]

I fixed the PR to branch from and merge to the 5.3.x branch.

After some messing about I finally got the Generate async code option from the build menu to work, but it only generated a handful of whitespace changes, so I didn't commit them.

The appveyor build appears to have failed some tests... but I don't think I caused those.
(might be because 5.3.x doesn't include #2921)

[fredericDelaporte]

Replaced by #2941.