-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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.
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
Correct nullability for set operations #18419
Conversation
d3738d3
to
1fcecd3
Compare
EntityProjection is marked as nullable only if it is mapped to a table. The source table can project out nulls so when generating ColumnExpression make them nullable. If the entityProjection is not mapped to a table then nullable flag is unnecessary. The backing propertyExpressions should already be nullable if required. May be we could decorate TableExpressionBase as nullable but breaking change.
|
1fcecd3
to
752820c
Compare
Yeah, I see that - just a bit uncomfortable about having EntityProjection lying about its nullability, in case we start looking at it in some place in the future etc. |
EntityProjection does not lie about anything because it does not tell you its nullability. |
Right, but it contains an incorrect setting about itself, in case we ever need to rebind a property or similar. I understand it's irrelevant in the current code/design though. |
752820c
to
69cc698
Compare
What is rebinding a property? If you pass a table then tell if it can be null. Purely metadata to create columnExpression if not created already. There is no setting which is incorrect. |
Never mind, am just leaving it as-is. |
If you think there is a bug or design issue somewhere then please explain in detail what scenario is not working or why it is an issue. cc: @ajcvickers |
Apologies, was not suggesting there was a bug in the design or anything... There's no bug here, just a possible matter of future-proofing it by making sure that EntityProjections always has the correct |
test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs
Show resolved
Hide resolved
Make async and simplify. Continues #18419
Fixes #18135
@smitpatel some minor comments: