Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set additionalProperties when [JsonExtensionData] is present #56767

Closed
wants to merge 1 commit into from

Conversation

captainsafia
Copy link
Member

Reacting to feedback in #56318 (comment) and discussion in #56707.

The additionalProperties keyword in a schema is used to indicate that a schema can match against properties explicitly defined in the schema.properties list and additional unspecified properties.

The JSON Schema specification that STJ's JsonSchemaExporter targets assumes that additionalProperties is true by default for all schemas, meaning that a schema for a type will always be able to capture properties that are not explicitly defined in the properties list (see here).

The OpenAPI specification (as of v3 that we target) assumes the opposite. additionalProperties is assumed false and must be explicitly enabled in order to indicate that a schema can capture properties not explicitly defined. Furthermore, OpenAPI doesn't support a value true for the schema to support catch-alls. It requires that you implement an empty object to indicate that additional property values can match any type.

These two problems come together like Voltron in our handling of [JsonExtensionData] attributes on property. JsonSchemaExporter assumes that additionalProperties is implicitly true and doesn't set it when it generates a schema. We have to patch this mismatch in our TransformSchemaNode implementation to comply with what OpenAPI expects.

Since [JsonExtensionData] can only ever be applied on Dictionary<string, object> and Dictionary<string, JsonElement>, we don't have to worry about specifying concrete schemas for the values of additional properties. The empty object will do.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc labels Jul 13, 2024
@captainsafia captainsafia requested a review from mikekistler July 13, 2024 00:09
@captainsafia captainsafia requested a review from a team as a code owner July 13, 2024 00:09
@captainsafia captainsafia force-pushed the safia/addl-props-extension-data branch from 0c19713 to 6227ab4 Compare July 13, 2024 05:30
// match the OpenAPI v3 interpretation here by checking to see if any properties on a type map to
// extension data and explicitly setting the `additionalProperties` keyword to an empty object.
// Since `[ExtensionData]` can only be applied on dictionaries with `object` or `JsonElement` values
// there is no need to encode a concrete schema for thm and the catch-all empty schema is sufficient.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// there is no need to encode a concrete schema for thm and the catch-all empty schema is sufficient.
// there is no need to encode a concrete schema for them and the catch-all empty schema is sufficient.

public string Name => name;

[JsonExtensionData]
public IDictionary<string, object> ExtensionData { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth having a test for the JsonElement case also mentioned in the description? Possibly also a class that derives from something with the base having the extension property?

@mikekistler
Copy link
Contributor

The OpenAPI specification (as of v3 that we target) assumes the opposite. additionalProperties is assumed false ...

I don't think this is true. In the Schema Object section of the OpenAPI 3.0.3 spec it says:

additionalProperties - Value can be boolean or object. Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema. Consistent with JSON Schema, additionalProperties defaults to true.

@dotnet-policy-service dotnet-policy-service bot added this to the 9.0-preview7 milestone Jul 15, 2024
@captainsafia captainsafia deleted the safia/addl-props-extension-data branch July 22, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants