Skip to content

Conversation

@captainsafia
Copy link
Member

Closes #57041.

This PR is an extension of the changes that was introduced in 4c8b5fe to replace the use of true schemas with empty object schemas in the OpenAPI implementation.

JsonSchemaExporter uses the true schemas as catch-all schema for types. OpenAPI schema diverges from JSON schema and expects that empty schemas { } be used to represent catch-all values. The previous logic special cased fixing up these mappings for object types. This PR updates the implementation to do the mapping if the underlying schema is a true schema, regardless of the type. This makes the implementation resilient to other cases where a true schema is returned by STJ, like when a user defined converter is associated with a type.

With these changes, we'd expect the following diff in the generated schema.

public class ContainerType
{
	public string Name { get; }
	public TypeWithUserDefinedConverter ChildType { get; }
}
{
	"type": "object",
	"properties": {
		"name": {
			"type": "string"
		},
-		"childType": true,
+		"childType": { },
	}
}

@captainsafia captainsafia requested a review from a team as a code owner July 29, 2024 19:38
@ghost ghost added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jul 29, 2024
Copy link
Contributor

@mikekistler mikekistler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 👍

// expected in OpenAPI documents.
if (type == typeof(object))
// anything (like the `object` type) or types with user-defined converters. We override
// this default behavior here to match the style traditionally expected in OpenAPI documents.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "traditionally" mean here? Is there a newer style of OpenAPI documents?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, OpenAPI v3.1 is an upcoming version that has full parity with JSON schema and would permit true schemas being used as catch-all schemas. I'll update this comment to be more explicit.

if (type == typeof(object))
// anything (like the `object` type) or types with user-defined converters. We override
// this default behavior here to match the style traditionally expected in OpenAPI documents.
if (schema.GetValueKind() == JsonValueKind.True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idle thought: what happens for dynamic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question! Looks like it gets handled similarily to object foo. I added test coverage for this in fa39cfc.

@captainsafia captainsafia enabled auto-merge (squash) July 29, 2024 23:09
@captainsafia captainsafia merged commit 27f2a01 into main Jul 30, 2024
@captainsafia captainsafia deleted the safia/57041 branch July 30, 2024 01:43
@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-rc1 milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Custom types with converters cause schema generation to fail

4 participants