JamesNK / Newtonsoft.Json.Schema

Json.NET Schema is a powerful, complete and easy to use JSON Schema framework for .NET
http://www.newtonsoft.com/jsonschema
Other
247 stars 107 forks source link

Nullability of `JSchemaGenerationProvider.GetSchema` return is incorrect #341

Closed JR-Morgan closed 4 months ago

JR-Morgan commented 5 months ago

The Newtonsoft.Json.Schema.Generation.JSchemaGenerationProvider.GetSchema(JSchemaTypeGenerationContext) return type is incorrectly typed as a non-nullable JSchema

The XML comment for the function clearly says that null is a valid return value, so the return type should be JSchema?

i.e.

  public abstract JSchema? GetSchema(JSchemaTypeGenerationContext context);

This is a problem for users implementing GetSchema and wanting to return null, as they will either see a CS8603 - Possible null reference return or a CS8764 - Nullability of return type doesn't match overridden member (possibly because of nullability attributes depending on nullability syntax.

JamesNK commented 5 months ago

The XML doc comment is probably the incorrect one here.

The intent of the type is CanGenerateSchema is called to check that the provider can generate for a type, and then GetSchema always returns a value.

Non-null is checked for here: https://github.com/JamesNK/Newtonsoft.Json.Schema/blob/05524f9972c75648c7af40e99240f2280c03dc16/Src/Newtonsoft.Json.Schema/Generation/JSchemaGeneratorInternal.cs#L247-L252 But not checked for here: https://github.com/JamesNK/Newtonsoft.Json.Schema/blob/05524f9972c75648c7af40e99240f2280c03dc16/Src/Newtonsoft.Json.Schema/Generation/JSchemaGeneratorInternal.cs#L272-L273

I think the change to make is to improve the XML doc and check that GetSchema doesn't return null in an extra place.

What do you think?

cdriesler commented 5 months ago

For a bit more context (unsure either way) the example here also suggests using null as a way to fall back to using default generation.

JamesNK commented 4 months ago

Thanks. I looked into it more and you're right that this should allow null. Fixing here: https://github.com/JamesNK/Newtonsoft.Json.Schema/pull/342

It might be some time before the next release so you'll need to suppress the warning for now.

JR-Morgan commented 4 months ago

Aha, looks perfect, Thanks 🙌