Skip to content

Fix TryGetMappedNullability for CompositeCustomType #2938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
+17 −1
Closed

Fix TryGetMappedNullability for CompositeCustomType #2938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/NHibernate.Test/Linq/TryGetMappedTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,18 @@ public void NestedComponentPropertyCastTest()
o => o?.Name == "component[OtherProperty1]");
}

[Test]
public void CompositePropertyTest()
{
var query = session.Query<Glarch>().Select(o => o.Multiple.count);
AssertSupported(
query,
typeof(Glarch).FullName,
"Multiple.count",
o => o is Int32Type,
o => o?.Name == typeof(MultiplicityType).FullName);
}

[Test]
public void ManyToOneTest()
{
Expand Down
6 changes: 5 additions & 1 deletion src/NHibernate/Util/ExpressionsHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,11 @@ internal static bool TryGetMappedNullability(
}

int index;
if (componentType != null)
if (componentType is CompositeCustomType cct)
{
memberPath = memberPath.Remove(memberPath.LastIndexOf('.'));
}
else if (componentType != null)
Comment on lines +161 to +165
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
index = Array.IndexOf(
componentType.PropertyNames,
Expand Down