Handle nullability more accurately by Youssef1313 · Pull Request #2933 · microsoft/OpenAPI.NET

Youssef1313 · 2026-07-09T10:30:33Z

My previous PR was trying to handle nullability for enums specifically. This PR is more generalized to handle nulls more correctly as it was still buggy for non-enums.

In addition, the previous fix for enums didn't also fix all enum scenarios. It only fixed it when we are given "oneOf (null, schemaWithEnum)". But if we are given a schema reference instead, we are not going to add "null" to the reference and we will produce the wrong thing.

I'll leave comments on some specific updated tests to explain.

Youssef1313 · 2026-07-09T10:31:32Z

+            // NOTE: nullable property has no effect on the schema if type is not specified, so it is omitted in the serialized output.
            // Arrange
-            var expected = @"nullable: true";
+            var expected = @"{ }";


Both behaviors are technically okay. But it's better to avoid emitting nullable when the spec says that it has no effect. This is less misleading for people reading the document and no familiar with the spec details.

Youssef1313 · 2026-07-09T10:36:37Z

                {
                  "type": "string",
                  "oneOf": [
+                    {


The behavior here was previously buggy.

Even though "nullable" was set to true. A null value would have been still rejected because we won't match any of the "oneOf" schemas.

nullable only widens the "type" when present, but if the schema is still constrained to not allow null, a null would be rejected.

https://spec.openapis.org/oas/v3.0.4.html#fixed-fields-20

This keyword only takes effect if type is explicitly defined within the same Schema Object. A true value indicates that both null values and values of the type specified by type are allowed. Other Schema Object constraints retain their defined behavior, and therefore may disallow the use of null as a value. A false value leaves the specified or default type unmodified. The default value is false.

The new behavior is more aligned with the object model given by the user.

The user specified a oneOf child schema with Type = null, and we serialize the semantically equivalent schema that's valid for OpenAPI 3.0 (which doesn't use draft 2020-12 and so can't use the straightforward { "type": "null" } approach.

Youssef1313 · 2026-07-09T10:48:13Z

-            Extensions.TryGetValue(OpenApiConstants.NullableExtension, out var nullExtRawValue) &&
-            nullExtRawValue is JsonNodeExtension { Node: JsonNode jsonNode } &&
-            jsonNode.GetValueKind() is JsonValueKind.True;
+        internal bool IsNullable


When we are parsing a 3.0 document, we might encounter "nullable" property.

We might not know (yet) if this nullable property should have any effect or not (because we don't know if a future type property will be encountered or not).

As a result, we shouldn't be modifying OpenApiSchema.Type right away as soon as we see "nullable": true when deserializing.

To address this, IsNullable is made settable internally.

We will set it without modifying the type to indicate that "nullable": true was encountered while deserializing a document.

Then, at the end of the deserialization when we got all the information, we add "null" to OpenApiSchema.Type if nullable was set and we have a non-null type.

I think this is confusing because of the "upgrade" path. Let's acknowledged that conversions are lossy by nature, and that we do everything we can to enable roundtrips with as good fidelity as possible.

on roundtrips and upgrades

nullable: true type: string

should roundtrip in 3.0, but should also lead to

type: ["string", "null"]

In 3.1/3.2

on the presence of nullable without type

the OpenAPI 3.0 specification states

This keyword only takes effect if type is explicitly defined within the same Schema Object

It is NOT incorrect to have nullable: true with no type, it only means that keyword/field is meaningless.

While the library ensures the document is structured properly, it does not enforce any specific semantics (other than the logical conversions).

on v2 serialization

v2 is a 12 years old standard, v3: 9 years old. While those still have a large user base due to some tooling never upgrading (some of it becoming abandonware), I don't believe we should over optimize for those. Especially if that comes at the potential expense of the more modern versions.

We do provide the nullable as an extension to provide more context to the consumer when something was downgraded. But honestly, I doubt that a tool that has not taken the time to upgrade in 9 years at this point will take the time to implement that extension. Yes we should roundtrip that extension when possible to avoid losses, but that'd a much lower priority.

on v3.1/3.2

the specification clearly states that having just type: null is perfectly fine.

conclusion

With all that in mind. I don't want to see "multiple states" on the schema object that need to be kept in sync. Those schema objects can be initialized from deserialization, but also from the object model, and can be mutated afterwards. It'd be too easy for the state to become "out of sync" on one of the paths because the caller does not (and shouldn't know) the implementation details.

For deserialization:

When we encounter nullable: true in v2, or x-nullable: true in v3, we should set the type to "add null" (preserving any previous type value) as one of the values. (and drop the extension in v2 to avoid any confusion)

When deserializing the type, it should preserve the type null value that might have been set before, this way we guard against any order of those properties.

(for v3.1, 3.2, we should deserialize the type only)

For serialization:

If the type contains null in v2, emit the x-nullable: true extension.

If the type contains null in v3, emit the nullable: true extension.

For any other version, just emit the type.

For the object model:

we should only expose the object model as defined in 3.2, and have fields (as in state) for the things defined in 3.2.

Doing anything else with any/one/all of is a slippery slope, especially when we talk about upgrades etc. We should not be doing any of that.
This also assumes that we're serializing a "null" value for enum correctly, and not silently dropping it.

I hope this clarifies the cleanup at hand. Let me know what you think.

@baywet Does it look better after the last refactoring?

When we encounter nullable: true in v2, or x-nullable: true in v3, we should set the type to "add null" (preserving any previous type value) as one of the values. (and drop the extension in v2 to avoid any confusion)

I'm assuming you swapped the version for where nullable and x-nullable belong. But when we encounter it, I do not think we should add "null" to the type right away. We might not know yet if the user has "type" defined in the first place. What I want to make sure is, if I have the following two schemas in 3.0:

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

and

{ "nullable": true }

In the first schema, I expect the OpenApiSchema.Type to be JsonSchemaType.String | JsonSchemaType.Null, but for the second, OpenApiSchema.Type should remain null.

So, we don't always just add JsonSchemaType.Null to the Type, as it's incorrect, because nullable only has a meaning when type is explicitly defined, and so deserialization should take that into account.

If we want to avoid the additional state/field in OpenApiSchema object model, we can instead use the extensions dictionary during deserialization to have an internal extension that we "encountered" nullable. And in FinalizeDeserialization, we always remove the internal extension, and at that point we know whether or not we should add JsonSchemaType.Null.

Youssef1313 · 2026-07-09T10:49:27Z

-            IList<IOpenApiSchema>? effectiveOneOf = OneOf;
-            IList<IOpenApiSchema>? effectiveAnyOf = AnyOf;


For correctness, we are no longer altering/modifying the oneOf/anyOf.
If we find oneOf/anyOf that has a child schema with null type, we will serialize it as enum with a single null element, which is valid for OpenAPI 3.0.

baywet

Thanks for the contribution!

baywet

Getting closer (I'm only focusing my review on schema object field, and deserialization so far)

baywet · 2026-07-09T13:56:45Z

-
-                schema.Extensions.Remove(OpenApiConstants.NullableExtension);
-            }
+            schema.FinalizeDeserialization();


why are we implementing reading the extension at all in this version? it should only read nullable. The extension should only apply to v2

@baywet This is pre-existing behavior, I think. I'm not sure why it existed.

yes, and I think it was a mistake to begin with. Added here #1820

There is a test asserting the current behavior: LoadSchemaWithNullableExtensionAsV31Works.
I can change the test and behavior though.

baywet · 2026-07-09T13:57:54Z

-                            o.Type |= JsonSchemaType.Null;
-                        else
-                            o.Type = JsonSchemaType.Null;
+                        o.IsNullableFromDeserialization = true;


we shouldn't maintain a state on the object for that. Revert this change. (original code before the PR)

I'm not following how I should approach fixing the current bugs in the area then.

Basically, we cannot just say "we have nullable set to true, we we are going to expand the Type with JsonSchemaType.Null". That in itself was a bug.

baywet · 2026-07-09T13:58:35Z

+        /// <summary>
+        /// This property is internal and is only used to mark the "nullable" state when doing deserialization.
+        /// It's only set during deserialization to flag whether we encountered "nullable" or not.
+        /// And it's read:
+        /// 1. During FinalizeDeserialization to determine whether the Type should be changed to include null or not.
+        /// 2. When serializing nullable, if IsNullable was false and IsNullableFromDeserialization was true, we will still serialize "nullable".
+        /// </summary>
+        internal bool IsNullableFromDeserialization { get; set; }


we shouldn't have this field/state

baywet · 2026-07-09T13:59:41Z

-                        else
-                            o.Type = JsonSchemaType.Null;
-                    }
+                    o.IsNullableFromDeserialization = bool.Parse(value);


revert this change (code from before the PR)

baywet · 2026-07-09T14:00:32Z

-
-            schema.Extensions.Remove(OpenApiConstants.NullableExtension);
-        }
+        schema.FinalizeDeserialization();


we shouldn't parse the extension in 3.1

baywet · 2026-07-09T14:00:45Z

-                        else
-                            o.Type = JsonSchemaType.Null;
-                    }
+                    o.IsNullableFromDeserialization = bool.Parse(value);


revert this change (before the PR)

baywet · 2026-07-09T14:00:49Z

-
-            schema.Extensions.Remove(OpenApiConstants.NullableExtension);
-        }
+        schema.FinalizeDeserialization();


we shouldn't parse the extension in 3.1

@baywet This is a pre-existing behavior, I think, right?

yes, and I think it was an error to have it in the first place.

Youssef1313 requested a review from a team as a code owner July 9, 2026 10:30

Youssef1313 commented Jul 9, 2026

View reviewed changes

Youssef1313 force-pushed the dev/ygerges/nullable branch from 67ef969 to 937b8eb Compare July 9, 2026 10:38

Handle nullability more accurately

a70df43

Youssef1313 force-pushed the dev/ygerges/nullable branch from 937b8eb to a70df43 Compare July 9, 2026 10:39

Youssef1313 commented Jul 9, 2026

View reviewed changes

Comment thread src/Microsoft.OpenApi/Reader/V2/OpenApiSchemaDeserializer.cs

Copilot AI mentioned this pull request Jul 9, 2026

Use locally-built Microsoft.OpenApi packages from OpenAPI.NET PR #2933 dotnet/aspnetcore#67697

Draft

4 tasks

baywet requested changes Jul 9, 2026

View reviewed changes

Comment thread src/Microsoft.OpenApi/Models/OpenApiSchema.cs Outdated

Comment thread src/Microsoft.OpenApi/Models/OpenApiSchema.cs Outdated

Comment thread src/Microsoft.OpenApi/Models/OpenApiSchema.cs Outdated

Comment thread src/Microsoft.OpenApi/Models/OpenApiSchema.cs Outdated

Youssef1313 added 3 commits July 9, 2026 14:56

Cleanup

f66b887

Rename SerializeTypeProperty to TrySerializeTypeProperty

157397e

Separate rp

eb2baed

baywet reviewed Jul 9, 2026

View reviewed changes

Youssef1313 added 3 commits July 9, 2026 16:13

Cleanup

8db16f9

Progress

01105d1

Cleanp

7ce25f7

Youssef1313 requested a review from baywet July 9, 2026 20:31

Youssef1313 added 4 commits July 9, 2026 23:01

Refactor

9b24b17

Use cached json extension

e754250

AddExtension only if Type is still unknown

4b74290

Cleanup

fd32d81

		IList<IOpenApiSchema>? effectiveOneOf = OneOf;
		IList<IOpenApiSchema>? effectiveAnyOf = AnyOf;

Uh oh!

Conversation

Youssef1313 commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

on roundtrips and upgrades

on the presence of nullable without type

on v2 serialization

on v3.1/3.2

conclusion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

baywet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

baywet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Youssef1313 commented Jul 9, 2026 •

edited

Loading