Fix conflicting required+nullable schema when only NonNullableReferen…#3912
Fix conflicting required+nullable schema when only NonNullableReferen…#3912KitKeen wants to merge 2 commits intodomaindrivendev:masterfrom
Conversation
…ceTypesAsRequired is enabled
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3912 +/- ##
=======================================
Coverage 95.04% 95.04%
=======================================
Files 111 111
Lines 3958 3958
Branches 801 801
=======================================
Hits 3762 3762
Misses 196 196
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I need to think about this more before doing a proper review on this PR. Members can still be required yet allow For example: public class SomeType
{
public required string? A { get; set; }
public required string? C { get; set; }
}// Bad payload, c is missing
{
"a": "b"
}// Good payload, c is specified as explicitly null
{
"a": "b",
"c": null
}// Good payload, c is specified with a value
{
"a": "b",
"c": "d"
}Any changes here need to support different scenarios people may have when nullable reference types are and aren't enabled and when members are required or not. The defaults should where possible match expectations, but at the same type we don't want to introduce any breaking changes or make different opinions users may have on how to treat required and/or nullable reference types together. |
…renceTypesAsRequired
|
Martin, I appreciate your review! You're right that required + nullable: true is a valid OpenAPI combination public class SomeType With NonNullableReferenceTypesAsRequired enabled: property required nullable correctness Your required string? example is also covered: public class SomeType Here IsNonNullableReferenceType() returns false for string?, so nullable The fix only clears nullable when both conditions are true: I've also added a test |
|
take your time reviewing it, I'd rather listen to all of the conserns before merging. |
Fix conflicting
required+nullableschema when onlyNonNullableReferenceTypesAsRequiredis enabledFixes #3297
Problem
When
NonNullableReferenceTypesAsRequired()is enabled withoutSupportNonNullableReferenceTypes(), non-nullable reference-type properties end up with both"required": trueand"nullable": truein the generated schema.Root cause
IsNullableonly checksSupportNonNullableReferenceTypes:So when only
-AsRequiredis set, the property gets added torequired(viaGenerateSchemaForMembers) butnullableis never cleared.Fix
Before / after
builder.Services.AddSwaggerGen(c => c.NonNullableReferenceTypesAsRequired());Before:
After:
Tests
Added
GenerateSchema_NonNullableReferenceTypesAsRequired_DoesNotMarkPropertyAsNullablefor bothTypeWithNullableContextAnnotatedandTypeWithNullableContextNotAnnotated. Fails onmain, passes after the fix. Full suite 720/720 onnet10.0, clean build onnet8.0/net9.0/net10.0.