Skip to content

Avoid checking for null on non-nullable property for projection#6706

Merged
glen-84 merged 25 commits into
ChilliCream:mainfrom
glen-84:ga/ef-complex-types
Mar 2, 2026
Merged

Avoid checking for null on non-nullable property for projection#6706
glen-84 merged 25 commits into
ChilliCream:mainfrom
glen-84:ga/ef-complex-types

Conversation

@glen-84

@glen-84 glen-84 commented Nov 17, 2023

Copy link
Copy Markdown
Member

Summary of the changes (Less than 80 chars)

  • Check the nullability of properties before doing a null check.

Closes #6604.

@glen-84

glen-84 commented Dec 6, 2023

Copy link
Copy Markdown
Member Author

@PascalSenn Is this feasible?

@PascalSenn

Copy link
Copy Markdown
Member

@glen-84

We did remove it once for ef core and then added it again:
#6604 (comment)

So i am not sure what to do now. I believe if you do not do the nullcheck then it will include all the fields from the child or something like this.

@glen-84

glen-84 commented Dec 6, 2023

Copy link
Copy Markdown
Member Author

You removed it completely, or conditionally based on the nullability of the property?

Should I be seeing something significant in the test snapshots, or is there no test that would highlight the previous issue? If not, then I think that we need that test to exist in order to have some more confidence here.

@PascalSenn

Copy link
Copy Markdown
Member

This is the context:
28ed6ae

So you only apply it when the property is nullable and ef does not complain?

@glen-84

glen-84 commented Dec 6, 2023

Copy link
Copy Markdown
Member Author

So you only apply it when the property is nullable and ef does not complain?

By "does not complain", you mean that the tests in HotChocolate.Data.Projections.SqlServer.Tests pass? If so, yes, they all pass.

@glen-84

glen-84 commented Feb 4, 2024

Copy link
Copy Markdown
Member Author

@michaelstaib michaelstaib force-pushed the main branch 2 times, most recently from 8a97a79 to e2d2300 Compare June 17, 2024 17:49
@codecov

codecov Bot commented Oct 6, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (6a3b7a3) to head (2a5d875).
Report is 79 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #6706       +/-   ##
==========================================
- Coverage   74.16%       0   -74.17%     
==========================================
  Files        2677       0     -2677     
  Lines      140790       0   -140790     
  Branches    16371       0    -16371     
==========================================
- Hits       104421       0   -104421     
+ Misses      30774       0    -30774     
+ Partials     5595       0     -5595     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@glen-84 glen-84 marked this pull request as ready for review February 27, 2026 16:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 39 out of 39 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 40 out of 40 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@glen-84 glen-84 merged commit ad5b0c6 into ChilliCream:main Mar 2, 2026
117 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Projection middleware checks for null on non-nullable field, resulting in exception for complex type in EF Core 8

4 participants