Skip to content

Fix conflicting required+nullable schema when only NonNullableReferen…#3912

Open
KitKeen wants to merge 2 commits intodomaindrivendev:masterfrom
KitKeen:fix/nullable-conflict-non-nullable-as-required
Open

Fix conflicting required+nullable schema when only NonNullableReferen…#3912
KitKeen wants to merge 2 commits intodomaindrivendev:masterfrom
KitKeen:fix/nullable-conflict-non-nullable-as-required

Conversation

@KitKeen
Copy link
Copy Markdown

@KitKeen KitKeen commented Apr 18, 2026

Fix conflicting required + nullable schema when only NonNullableReferenceTypesAsRequired is enabled

Fixes #3297

Problem

When NonNullableReferenceTypesAsRequired() is enabled without SupportNonNullableReferenceTypes(), non-nullable reference-type properties end up with both "required": true and "nullable": true in the generated schema.

Root cause

IsNullable only checks SupportNonNullableReferenceTypes:

if (_generatorOptions.SupportNonNullableReferenceTypes)
{
    nullable &= !memberInfo.IsNonNullableReferenceType();
}

So when only -AsRequired is set, the property gets added to required (via GenerateSchemaForMembers) but nullable is never cleared.

Fix

if (_generatorOptions.SupportNonNullableReferenceTypes || _generatorOptions.NonNullableReferenceTypesAsRequired)
{
    nullable &= !memberInfo.IsNonNullableReferenceType();
}

Before / after

builder.Services.AddSwaggerGen(c => c.NonNullableReferenceTypesAsRequired());

public class WeatherForecast
{
    public string Summary { get; set; }
}

Before:

"summary": { "type": "string", "nullable": true }

After:

"summary": { "type": "string" }

Tests

Added GenerateSchema_NonNullableReferenceTypesAsRequired_DoesNotMarkPropertyAsNullable for both TypeWithNullableContextAnnotated and TypeWithNullableContextNotAnnotated. Fails on main, passes after the fix. Full suite 720/720 on net10.0, clean build on net8.0/net9.0/net10.0.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.04%. Comparing base (d22870e) to head (6254b15).

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           
Flag Coverage Δ
Linux 95.04% <100.00%> (ø)
Windows 95.04% <100.00%> (ø)
macOS 95.04% <100.00%> (ø)

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:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@martincostello
Copy link
Copy Markdown
Collaborator

martincostello commented Apr 18, 2026

I need to think about this more before doing a proper review on this PR.

Members can still be required yet allow nulls (i.e. property must be present, may not have a value).

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.

@KitKeen
Copy link
Copy Markdown
Author

KitKeen commented Apr 18, 2026

Martin, I appreciate your review!

You're right that required + nullable: true is a valid OpenAPI combination
and should be preserved. The fix handles this correctly because the guard
memberInfo.IsNonNullableReferenceType() distinguishes between the two cases:

public class SomeType
{
public string A { get; set; } // non-nullable → IsNonNullableReferenceType() = true
public string? B { get; set; } // nullable → IsNonNullableReferenceType() = false
}

With NonNullableReferenceTypesAsRequired enabled:

property required nullable correctness
string A true false ✅ non-nullable, must be present and have a value
string? B false true ✅ nullable, not required

Your required string? example is also covered:

public class SomeType
{
public required string? A { get; set; }
}

Here IsNonNullableReferenceType() returns false for string?, so nullable
is not cleared- the schema correctly reflects required: true, nullable: true.

The fix only clears nullable when both conditions are true:
а) NonNullableReferenceTypesAsRequired is enabled
b) The member is genuinely non-nullable (IsNonNullableReferenceType() = true)

I've also added a test
GenerateSchema_NonNullableReferenceTypesAsRequired_PreservesNullable_IfPropertyHasRequiredKeywordAndIsNullable covering exactly this scenario required string? stays required + nullable, required string stays required + not nullable.

@KitKeen
Copy link
Copy Markdown
Author

KitKeen commented Apr 18, 2026

take your time reviewing it, I'd rather listen to all of the conserns before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: When using NonNullableReferenceTypesAsRequired, both required and nullable exist

2 participants