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

Provide API to allow JSON converters/types to describe their associated JSON schema #105769

Open
captainsafia opened this issue Jul 31, 2024 · 13 comments

Comments

@captainsafia
Copy link
Member

The current implementation of the JsonSchemaExporter generates schemas for types based on its on semantics. There's currently not strategy for types/converters to advertise their associated JSON schemas.

This gap has presented a blocker for a couple of scenarios in ASP.NET Core's OpenAPI support around JSON schema.

  • gRPC's JSON transcoding layer needs to represent schemas for types based on the underlying protobuf representation instead of the generated C# implementation.
  • Types that implement a custom converter are not able to define a schema and are represented by the catch-all true schema.

While both System.Text.Json and ASP.NET Core expose APIs for modifying schemas at different layers of the stack, both approaches are reactive and allow mutating the schema that is generated instead of proactively dictating the schema that should be set.

Solutions that have been discussed in the past includ exposing a new IJsonSchemaResolver interface for type owners to implement:

public interface IJsonSchemaResolver
{
    JsonSchema GetJsonSchema(JsonTypeInfo typeInfo); 
    JsonSchema GetJsonSchema(JsonPropertyInfo propertyInfo); 
}

Or alternatively including a new virtual method on the JsonConverter to define associated schemas:

public class JsonConverter
{
    public virtual JsonSchema? GetSchema(JsonSerializerOptions options) => ...
}

cc: @JamesNK @eiriktsarpalis

@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 31, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

@stephentoub
Copy link
Member

This gap has presented a blocker

@captainsafia, as in we need to address this in .NET 9?

@captainsafia
Copy link
Member Author

@captainsafia, as in we need to address this in .NET 9?

Ah, should've been clearer about this.

Not a blocker for .NET 9, but definitely would be good to slot in for .NET 10.

@stephentoub stephentoub added this to the 10.0.0 milestone Jul 31, 2024
@gregsdennis
Copy link
Contributor

public interface IJsonSchemaResolver
{
    JsonSchema GetJsonSchema(JsonTypeInfo typeInfo); 
    JsonSchema GetJsonSchema(JsonPropertyInfo propertyInfo); 
}

There is no JsonSchema type shipping with .Net 9, and I'm not aware of one planned for 10. The current implementation for generation simply generates a JsonNode.

@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Jul 31, 2024
@JamesNK
Copy link
Member

JamesNK commented Jul 31, 2024

The problem with the proposed API is JSON schemas exist in the context of a document.

For example, schemas in a document reference each other via $ref, and schemas are commonly stored together in $defs (I don't know if JsonSchemaMapper has a feature to output generated schemas together in $defs yet. If you don't, I guarantee that there will be a lot of people asking for this feature. It's the idiomatic way to organize schemas in JSON schema).

These things happen beyond the scope of an individual JsonSchema instance. Customizations to schema generation for a JsonTypeInfo need to flow out and impact the rest of the document. If JsonTypeInfo needs to generate its own subschemas based on types in its properties, people will want:

  • Those schemas to be output in $defs
  • Other places that have the same schema, e.g. a schema for Person type, to all reference the common schema in $defs with $ref.

I think customization must happen before the document is generated and work at the Type level. Swashbuckle does this successfully using ISerializerDataContractResolver.

Using Swashbuckle's contract resolver, I can instruct it what the schema output for a type should be at the Type level. For example:

The Any type has a JsonConverter that customizes its output. I can then make its schema match the custom output by saying its schema is an object, with one known property of System.String, and additional properties of type Google.Protobuf.Value. The Swashbuckle schema generated then uses this custom contract to generate the desired JSON schema. Google.Protobuf.Value. is stored in the idiomatic shared location, and other places in the doc that also use Google.Protobuf.Value have a $ref to that shared location.

@gregsdennis
Copy link
Contributor

The current implementation does not support $refs, despite my arguing for them (see comments in the relevant PRs). What is currently provided is a stepping stone; just enough to get asp.net over the line.

@JamesNK
Copy link
Member

JamesNK commented Aug 1, 2024

Yes, what JsonSchemaMapper currently does is very rudimentary in .NET 9.

$ref and $def are needed. They're the idiomatic way to organize schemas in JSON. Without those features, a large amount of work is required to make good output from the type.

As evidence of this, see everything that ASP.NET Core needs to do to post-process the output of the mapper to make it usable. The mapper should be the one doing the work to place schemas in definitions and reference them. Some post-processing in ASP.NET Core will still be required to modify the JSON schema syntax to be compatible with OpenAPI, but it will be much less, and simpler, than what is currently done.

As soon as you start adding support for these features, you realize that an API to customize schemas individually doesn't work.

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Aug 2, 2024

For context, the reason why such an API wasn't added in .NET 9 is due to the lack of a built-in JsonSchema type. Building these abstractions around JsonNode is plainly not a good idea in the long run. Once a JsonSchema type is available building this feature should be relatively straightforward.

JsonSchemaExporter currently does not use $def (or $id) OOTB in the schemas that it generates. $ref schemas using JSON pointer are only generated in case of recursive schemas. The reason is naming: for a general-purpose naming scheme we would need to use identifiers that either encode the FQN of a given type or generate UUIDs neither of which felt like an acceptable solution. If we do add support for $def or $id in the future this should involve exposing an abstraction for users to define their own naming scheme.

@Peter-B-
Copy link

I try to generate a Json schema for some types, which contain simple value objects generated with Vogen. This makes me run into the issue described here.

It would be great to have an interface for describing my schema. For the simple use-case it might be sufficient to override JsonConverter.GetSchema, even though I definitely see the value of generating and reusing $ref.

Another issue related to this is, that GetJsonSchemaAsNode returns true for any type with a custom JsonConverter defined.

{
  "type": "object",
  "properties": {
    "propertyWithCustomConverter": true
  }
}

I think this violates the JsonSchema specification, which states

The value of "properties" MUST be an object. Each value of this object MUST be a valid JSON Schema.

I'd prefer to receive an NotSupportedException, preferably with a flag to ignore that in JsonSchemaExporterOptions.

Does it make sense to raise an issue for that? Or is it too late anyway, with .Net 9 just being released?

@gregsdennis
Copy link
Contributor

gregsdennis commented Nov 15, 2024

I think this violates the JsonSchema specification

true is a valid schema. It's equivalent to {}, which allows all JSON instances. This was introduced with draft 6.

@eiriktsarpalis
Copy link
Member

I think this violates the JsonSchema specification, which states

The value of "properties" MUST be an object. Each value of this object MUST be a valid JSON Schema.

The value of "properties" is an object in your example. It's just that the schemas of individual properties can boolean per the spec.

@Peter-B-
Copy link

Ah. True. Sorry, I didn't get that.

Besides that, I'm not sure if it is a very helpful property description.

@gregsdennis
Copy link
Contributor

gregsdennis commented Nov 15, 2024

@Peter-B- if you think the spec needs to be clarified, please feel free to open an issue in the JSON Schema spec repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants