Do not ignore multiple types when serializing to 3.0 - #2960
Conversation
165e0eb to
a663f6f
Compare
There was a problem hiding this comment.
Pull request overview
This pull request updates OpenApiSchema JSON serialization to properly represent schemas with multiple JsonSchemaType flags when targeting OpenAPI 3.0, emitting anyOf/oneOf (when possible) instead of silently ignoring additional types, and adds coverage to validate the new behavior.
Changes:
- Split type serialization logic between OpenAPI 2.0 and OpenAPI 3.0+ paths, adding 3.0-specific handling for multi-type schemas via
anyOf/oneOf. - Add comprehensive unit tests covering multi-type combinations (with/without
null, and with existingoneOf/anyOfpresent). - Simplify
ToSingleIdentifierby using a lookup and providing a clearer exception for unexpected values.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/Microsoft.OpenApi.Tests/Models/OpenApiSchemaTests.cs | Adds tests validating multi-type serialization behavior for OpenAPI 3.0 across multiple composition scenarios. |
| src/Microsoft.OpenApi/Models/OpenApiSchema.cs | Refactors and extends type serialization to support multi-type handling in 3.0 using anyOf/oneOf, keeping 2.0 behavior unchanged. |
| src/Microsoft.OpenApi/Extensions/OpenApiTypeMapper.cs | Updates ToSingleIdentifier to use a lookup and throw a clearer exception for unexpected inputs. |
Comments suppressed due to low confidence (1)
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1091
- In the v3.1+ path, TrySerializeTypePropertyForVersion3AndLater also assumes Enum.GetValues() will match at least one flag in the provided Type. If Type is 0 or contains only unknown bits, array will be empty and array[0] will throw IndexOutOfRangeException. Add an empty-array guard and return false when no valid flags are present.
var array = (from JsonSchemaType flag in jsonSchemaTypeValues
where type.HasFlag(flag)
select flag).ToArray();
if (array.Length > 1)
{
writer.WriteOptionalCollection(OpenApiConstants.Type, array, (w, s) => w.WriteValue(s.ToSingleIdentifier()));
}
else
{
writer.WriteProperty(OpenApiConstants.Type, array[0].ToSingleIdentifier());
}
Given the investigations I did for #2967, I think it's fine for us to continue ensuring, as much as possible, that we produce semantically equivalent document for all versions. The cases presented in #2967 fall into two groups:
I don't believe there is a need for a new public API to do the semantic transformations, and I don't think all transformations we might want to do in future will be representable in the object model. So we will end up with logic scattered between dedicated walkers and in the serialization itself. We shouldn't need to pay additional performance cost for deep cloning the whole document as well. In all cases, we can always decide to move the logic to a walker in the future, if really necessary. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1088
- In the OpenAPI 3.1+ branch,
array[0]is used whenarray.Length <= 1. IfTypeis 0 or contains no known flags,arraywill be empty and this will throwIndexOutOfRangeException.
}
else
{
writer.WriteProperty(OpenApiConstants.Type, array[0].ToSingleIdentifier());
}
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1073
arrayWithoutNull[0]is accessed without guarding forarrayWithoutNull.Length == 0. IfTypeis set to an unexpected value (e.g., 0 / no flags, or only unknown bits), this will throwIndexOutOfRangeExceptionduring OpenAPI 3.0 serialization.
else
{
writer.WriteProperty(OpenApiConstants.Type, arrayWithoutNull[0].ToSingleIdentifier());
return;
}
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1053
- For OpenAPI 3.0, when Type includes
nulland multiple non-null types, this path represents null by adding a child schema withType = Nullinto anyOf/oneOf. HoweverSerializeNullablewill still emit top-levelnullable: truewheneverHasNullTypeis true, even though there is no top-leveltypein that case. This produces redundant/ineffective output and contradicts the new tests that expect nonullablewhen null is represented via anyOf/oneOf.
This issue also appears in the following locations of the same file:
- line 1069
- line 1084
// - If we have more than one type (excluding null), we have to use anyOf/oneOf.
// - If we have exactly one type alone (without null), we emit the type property.
// - If we have exactly one non-null type and also we have the null type, we emit the type property and nullable: true (handled in SerializeNullable)
if (arrayWithoutNull.Length > 1)
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1029
- This XML doc uses a
<returns>block, but the method returnsvoid. Either remove the<returns>documentation or change the method signature to return a value (and use it at the call site).
/// <returns>
/// true if the Type was serializable using "type" property, and false if
/// it serialized using anyOf/oneOf or if it couldn't be serialized at all.
/// </returns>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (5)
test/Microsoft.OpenApi.Tests/Models/OpenApiSchemaV30CompatibilityTests.cs:147
- The test name says it "OmitsType", but the updated expected JSON now writes multiple types via anyOf (and also keeps nullable). Renaming the test would better match the behavior being asserted.
public async Task SerializeMultipleNonNullTypesWithNullAsV3OmitsTypeButKeepsNullable()
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1029
- XML documentation has a section describing a boolean return value, but SerializeTypePropertyForVersion3AndLater returns void. This makes the docs misleading for future maintainers.
/// <summary>
/// Tries to serialize the "type" property for OpenAPI v3 and later versions.
/// </summary>
/// <returns>
/// true if the Type was serializable using "type" property, and false if
/// it serialized using anyOf/oneOf or if it couldn't be serialized at all.
/// </returns>
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1048
- For OpenAPI 3.0, if Type is set to an empty flags value (e.g., (JsonSchemaType)0), typeWithoutNull will produce no flags and arrayWithoutNull will be empty. The current code then falls into the single-type branch and indexes arrayWithoutNull[0], which will throw IndexOutOfRangeException.
var typeWithoutNull = type & ~JsonSchemaType.Null;
var hasNull = typeWithoutNull != type;
var arrayWithoutNull = (from JsonSchemaType flag in jsonSchemaTypeValues
where typeWithoutNull.HasFlag(flag)
select flag).ToArray();
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1079
- For OpenAPI versions other than 3.0, if Type is an empty flags value ((JsonSchemaType)0), the computed array will be empty and the code will index array[0], throwing IndexOutOfRangeException.
var array = (from JsonSchemaType flag in jsonSchemaTypeValues
where type.HasFlag(flag)
select flag).ToArray();
test/Microsoft.OpenApi.Tests/Models/OpenApiSchemaV30CompatibilityTests.cs:121
- The test name says it "OmitsType", but the updated expected JSON now writes multiple types via anyOf. Renaming the test would keep intent clear and avoid future confusion.
This issue also appears on line 147 of the same file.
public async Task SerializeMultipleNonNullTypesAsV3OmitsType()
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:429
- Folding anyOf/oneOf into
schema.Typeshould guard against empty collections;All(...)returns true for an empty list, which would produceType = 0(invalid) and drop the anyOf/oneOf. Also, the lambda pattern variableschemashadows the outerschemavariable, reducing readability.
if (schema.AnyOf is not null &&
schema.AnyOf.All(childSchema => childSchema is OpenApiSchema schema && DoesSchemaRepresentSingleType(schema)))
{
JsonSchemaType types = GetAllTypes(schema.AnyOf);
schema.AnyOf = null;
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:430
- The anyOf/oneOf folding into
schema.Typeonly checks that each child has a singleType. If child schemas include additional constraints (e.g.,format, bounds,enum, etc.), folding will drop those constraints by nulling out anyOf/oneOf and will change the schema semantics.
schema.AnyOf.All(childSchema => childSchema is OpenApiSchema schema && DoesSchemaRepresentSingleType(schema)))
{
JsonSchemaType types = GetAllTypes(schema.AnyOf);
schema.AnyOf = null;
schema.Type = types;
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1029
- The XML doc for
SerializeTypePropertyForVersion3AndLaterclaims it returns abooland documents return semantics, but the method returnsvoid. This will generate incorrect/intellisense documentation.
/// <summary>
/// Tries to serialize the "type" property for OpenAPI v3 and later versions.
/// </summary>
/// <returns>
/// true if the Type was serializable using "type" property, and false if
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:427
nullable: trueis tracked in metadata whenTypeisn't available yet, and later applied in the post-parse metadata step. With the new anyOf/oneOf folding (which may inferTypeonly at the end), nullable can be lost because the metadata key is removed/applied before the folding occurs.
This issue also appears in the following locations of the same file:
- line 425
- line 426
if (schema.Type is null)
{
if (schema.AnyOf is not null &&
schema.AnyOf.All(childSchema => childSchema is OpenApiSchema schema && DoesSchemaRepresentSingleType(schema)))
{
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1088
- In the OpenAPI 3.1+ path,
array[0]is accessed whenarray.Length <= 1, butarraycan be empty ifTypeis0(no flags). This can throw an IndexOutOfRangeException during serialization.
if (array.Length > 1)
{
writer.WriteOptionalCollection(OpenApiConstants.Type, array, (w, s) => w.WriteValue(s.ToSingleIdentifier()));
}
else
{
writer.WriteProperty(OpenApiConstants.Type, array[0].ToSingleIdentifier());
}
src/Microsoft.OpenApi/Models/OpenApiSchema.cs:1029
- The XML doc comment for
SerializeTypePropertyForVersion3AndLaterdescribes a boolean return value, but the method returnsvoid. This makes the generated docs misleading.
/// <summary>
/// Tries to serialize the "type" property for OpenAPI v3 and later versions.
/// </summary>
/// <returns>
/// true if the Type was serializable using "type" property, and false if
/// it serialized using anyOf/oneOf or if it couldn't be serialized at all.
/// </returns>
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:438
- Collapsing
anyOf/oneOfintoschema.Typebased only on each child having a singleTyperisks dropping constraints. For example,anyOf: [{ type: "string", format: "uuid" }, { type: "integer", minimum: 0 }]would match the current predicate, but clearingAnyOf/OneOfwould lose those per-branch constraints.
if (schema.Type is null)
{
if (schema.AnyOf is not null &&
schema.AnyOf.All(childSchema => childSchema is OpenApiSchema schema && DoesSchemaRepresentSingleType(schema)))
{
JsonSchemaType types = GetAllTypes(schema.AnyOf);
schema.AnyOf = null;
schema.Type = types;
}
else if (schema.OneOf is not null &&
schema.OneOf.All(childSchema => childSchema is OpenApiSchema schema && DoesSchemaRepresentSingleType(schema)))
{
JsonSchemaType types = GetAllTypes(schema.OneOf);
schema.OneOf = null;
schema.Type = types;
}
Gavin Barron (gavinbarron)
left a comment
There was a problem hiding this comment.
a few small nits, and one additional test case we need to ensure works as intended
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:529
- The folding guard in DoesSchemaRepresentSingleType() doesn’t consider some OpenApiSchema properties (e.g., Const / WasConstExplicitlySet, Example, Examples). That means a child schema like {"type":"string","example":"x"} can be treated as “type-only” and those extra keywords would be lost when anyOf/oneOf is collapsed back into Type.
schema.ExternalDocs is null &&
!schema.Deprecated &&
schema.Xml is null &&
schema.Extensions is null &&
schema.UnrecognizedKeywords is null;
src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs:438
- The folding logic for anyOf/oneOf doesn’t guard against empty arrays. If an input schema contains
"anyOf": [](or"oneOf": []),All(...)is vacuously true and GetAllTypes() will setschema.Typeto 0 and clear the composition keyword, which is a surprising/destructive transformation. It would be safer to only fold when the array has at least one element.
if (schema.AnyOf is not null &&
schema.AnyOf.All(s => s is OpenApiSchema child && DoesSchemaRepresentSingleType(child)))
{
JsonSchemaType types = GetAllTypes(schema.AnyOf);
schema.AnyOf = null;
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Youssef1313 <31348972+Youssef1313@users.noreply.github.com>
@baywet is oof
Fixes #2939
ToSingleIdentifieris only a simplification to make it easier to read.Typehad multiple values (e.g, String and Integer). This is now handled usingoneOforanyOf(whichever isn't used by the current schema already). If both are used, we ignore as it we used to in the past.Type.Nullfrom it (if it exists).For context: https://spec.openapis.org/oas/v3.0.4.html#json-schema-keywords