Fix NRE in linq processing of custom components #2941
Conversation
| from x2 in session.Query<NullableOwner>() | ||
| where x == x2 && x.ManyToOne == null && x.OneToOne.Name == null | ||
| select x2).ToListAsync()); | ||
| from x2 in session.Query<NullableOwner>() | ||
| where x == x2 && x.ManyToOne == null && x.OneToOne.Name == null | ||
| select x2).ToListAsync()); |
There was a problem hiding this comment.
I am not sure why those changes occur. Let's get rid of them, if that is not an unstable generation artifact.
There was a problem hiding this comment.
I think I just made some whitespace changes for #2772 and forgot to regenerate async code.
| @@ -807,7 +819,7 @@ public void NotNhQueryableTest() | |||
| Assert.That(() => expectedMemberType(memberType), $"Invalid member type: {memberType?.Name ?? "null"}"); | |||
| Assert.That(() => expectedComponentType(componentType), $"Invalid component type: {componentType?.Name ?? "null"}"); | |||
|
|
|||
| if (found) | |||
| if (found && (componentType == null || componentType.PropertyNullability != null)) | |||
There was a problem hiding this comment.
Maybe instead make nullability parameter nullable. And check that _tryGetMappedNullability returns false when it's null.
There was a problem hiding this comment.
I am adding a commit doing what I think you are meaning, but I do not see this as an improvement. It looks to me as a less understandable way of testing, and maybe more brittle, subject to cause the test to fail if we start implementing that nullability property for custom composite type.
There was a problem hiding this comment.
But with your initial change TryGetMappedNullability call is not tested at all for problematic case. Isn't it? So your changes were not covered.
subject to cause the test to fail if we start implementing that nullability property for custom composite type.
I see no problem with adjusting existing test cases when behavior is changed.
nit: I would also add optional nullability parameter to AssertResult method and keep AssertResult call it test case
There was a problem hiding this comment.
Ok, I thought the test parsing the query would also cause the method to be called, but I had not checked. I have added your nit.
| from x2 in session.Query<NullableOwner>() | ||
| where x == x2 && x.ManyToOne == null && x.OneToOne.Name == null | ||
| select x2).ToListAsync()); | ||
| from x2 in session.Query<NullableOwner>() | ||
| where x == x2 && x.ManyToOne == null && x.OneToOne.Name == null | ||
| select x2).ToListAsync()); |
There was a problem hiding this comment.
I think I just made some whitespace changes for #2772 and forgot to regenerate async code.
To be dropped if not considered actually adequate.
fix #2937
Replace #2938.