Change nullability handling for schemas with anyOf, allOf, oneOf or type = null#3976
Change nullability handling for schemas with anyOf, allOf, oneOf or type = null#3976ldeluigi wants to merge 4 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3976 +/- ##
=======================================
Coverage 95.04% 95.05%
=======================================
Files 111 111
Lines 3958 3965 +7
Branches 801 806 +5
=======================================
+ Hits 3762 3769 +7
Misses 196 196
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@martincostello could you enable the CI again please? |
|
@ldeluigi What would be the most straightforward way for me to check if your PR fixes the issue for me? |
Either clone it and add your case as a unit test for swagger gen and see the result, or debug from your side where is the problem in the current version of the library and then look at the changed lines and see if I changed those lines related to your issue |
|
Sorry for the delay in reviewing - been busy with other things. As this area is quite sensitive in terms of potential breaking changes (we've accidentally broken things in this area in the past) I asked Claude Code to do a review of the changes for possible issues or conflicts with the specification. Please review the output below and see if any of the findings are relevant to these changes. The "Concerns" section is the most relevant, but I've included all the output for context. Review of PR #3976 — Nullability handling for
|
| Case | Old 3.1 output | New 3.1 output | New 3.0 output |
|---|---|---|---|
object? (no type) |
type: "null" ❌ (only null allowed — the bug) |
type: ["null","boolean","integer",…,"array"] ✅ |
nullable: true ✅ |
nullable anyOf |
type: "null" + anyOf |
anyOf: [..., {type:null}] ✅ |
nullable: true (+ flattened anyOf) ✅ |
nullable oneOf |
same as anyOf | oneOf: [..., {type:null}] ✅ |
✅ |
int? etc. |
integer+null |
integer+null (unchanged) ✅ |
integer+nullable ✅ |
So the headline bug is genuinely fixed, and the union approach matches exactly the "Any Type" definition the issue author cited from the OpenAPI spec. Importantly, the default serialization version in Swashbuckle is OpenAPI 3.0 (SwaggerOptions.OpenApiVersion = OpenApi3_0), and I confirmed the verbose union cleanly downconverts to nullable: true with no type — so 3.0 documents are unaffected/correct.
The normal primitive path (int? → integer | null) is provably unchanged: those types skip all the anyOf/oneOf/allOf branches, fall into the else (??= is a no-op since Type is already set), then |= Null. ✅
Concerns
-
allOfbranch produces a contradictory 3.1 schema.schema.Type ??= JsonSchemaType.Nullfollowed by|= Nullyieldstype: "null"alongsideallOf: [...]— i.e. "must be null AND must satisfy the subschemas," which is unsatisfiable in 3.1. (In 3.0 it downconverts harmlessly toallOf+nullable: true.) The PR explicitly says this preserves pre-existing behavior for a niche case, and I confirmed the old code produced the same thing — so it's not a regression, but it's not actually "fixed" either. The correct 3.1 form would wrap inanyOf: [{allOf:[…]}, {type:null}]. Acceptable to defer, but worth calling out. -
The
allOfbranch has no dedicated test. The two new[Fact]tests coveranyOfandoneOf;DictionaryWrappersDatacovers theobject?union. There is no test that drivesSetNullableon a schema withAllOf.Count > 0. Codecov reports patch coverage green, so an existing test likely grazes it, but there's no explicit assertion of the (admittedly questionable)type:null + allOfoutput. Minor: theallOfpattern binds{ Count: > 0 } allOfbut never usesallOf(theanyOf/oneOfbindings are used). -
AnnotationsSchemaFilterwas only half-aligned. It got the union-type fix but not theanyOf/oneOf/allOfhandling, so[SwaggerSchema(Nullable = true)]on ananyOfcustom schema would OR the union intoTyperather than adding{type:null}to theanyOflist — inconsistent withSchemaGenerator. No test covers this path either. Edge case, but the two code paths now diverge. -
No test asserts the serialized document. All tests assert in-memory
JsonSchemaTypeflags /anyOfcounts. Given the whole point is spec-valid output, a snapshot test of the emitted 3.0 and 3.1 JSON would be more convincing. (I verified this manually above; it's fine — just not locked in by a test.)
Breaking changes
No API breaks. There are document-shape changes for 3.1 consumers: object?, nullable anyOf, and nullable oneOf members now emit different (correct) schemas than before. These are bug fixes replacing invalid/misleading output, and 3.0 output is unchanged — so I'd classify this as a non-breaking correctness fix, though anyone snapshotting 3.1 docs will see diffs.
The test refactor (moving ModelOfA/B/C, TypeWithRequiredProperties, etc. into TestSupport/Fixtures) is clean — no other references to the old nested types and no name collisions (CI passes).
Pull Request
The issue or feature being addressed
Fixes #3936
Details on the issue fix or feature implementation
In order to fix #3936 I needed to check whethere the nullability was being applied to a schema of one of these kinds:
allOfproperty configured: I restored the previous behaviour of having type: Null instead of type: null. This is a niche scenario.anyOforoneOfproperty configured: in this case, according to the open api specification, nullability should be expressed with an additional anyOf/oneOf type with type: Null