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

Support nullable value types #127

Conversation

gregsdennis
Copy link
Collaborator

Created as an alternative approach to #124.

@gregsdennis gregsdennis changed the base branch from master to schemagen-vnext June 16, 2021 09:12
@gregsdennis
Copy link
Collaborator Author

I like this as a starting point.

It doesn't add null as a valid type (e.g. "type": [ "integer", "null" ]), which I think it probably should for nullable value types. However this is also something that I think needs to be covered in an option. I think as you have it currently should be the default behavior (not adding null).

Further thinking (and perhaps a secondary option) is needed around doing this for nullable ref types. I'll ask around the JSON Schema circles to see if people have feelings about this. Feel free to join me their Slack (it's on their website).

@gregsdennis
Copy link
Collaborator Author

@JMPSequeira what do you think of adding a [NotNull] attribute? It would only have an effect on reference types (your generator can handle value types), and in its absence null is added to the type keyword.

I would like your generator to add null to the type keyword when applicable.

@JMPSequeira
Copy link
Contributor

I'll add type null to nullable value types.

I think the NotNullAttribute should also cover Nullable, specially after adding null to the type array.

@gregsdennis
Copy link
Collaborator Author

So here's what I'm thinking:

  • Add an option to support nullability in type. This would be a [Flags] enum:
    • Disabled (default) would never put nullintype`.
    • AllowForNullableValueTypes = 1 would add a null for Nullable<T> types.
    • AllowForReferenceTypes = 2 would add a null for reference types (still can't determine if NRT is enabled in client code)
    • AllowForAllTypes = AllowForNullableValueTypes | AllowForReferenceTypes
  • Add a [NotNull] that can override the behavior of the option to disallow null in type for the property on which it's used.
  • Keep [Required] working as is: it would only affect the required keyword.

This means that you can have something like

{
  "properties": {
    "foo": { "type": [ "integer", "null" ]
  },
  "required": [ "foo" ]
}

which would allow an explicitly included null value such as { "foo": null }. This means that the client (you) can express the full gamut of JSON Schema but would need to be extra careful to declare things exactly. I'll have to document this as well.

@JMPSequeira
Copy link
Contributor

Ok I can see that working. I suggest also having a NullAttribute to override when nullability == Disabled.

@JMPSequeira
Copy link
Contributor

JMPSequeira commented Jun 17, 2021

Regarding all the changes being implemented I have one question.

This poses an issue as there is no way to control the nullability of the root schema, Attributes are only read from Members and even if that wasn't the case there's also the issue with types from assemblies where one cannot apply attributes.

For example:

var builder = new JsonSchemaBuilder();

var schema = builder.FromType<List<string>>(configuration: new()
{
	Nullability = Nullability.AllowForAllTypes
});

I propose having an extra param on FromType where one explicitly indicate the behavior for the root schema.

Something in the lines of:

public static JsonSchemaBuilder FromType<T>(this JsonSchemaBuilder builder, SchemaGeneratorConfiguration? configuration = null, bool? rootAsNullable = null)
{
	return FromType(builder, typeof(T), configuration, rootAsNullable);
}

Let me know your thoughts on this.

gregsdennis added a commit that referenced this pull request Jun 18, 2021
gregsdennis added a commit that referenced this pull request Jun 18, 2021
Copy link
Collaborator Author

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

I think this got us a lot closer. There are a few concerns, which I've commented on. I'm not asking for you to update this, though. Please have a look at #128. I've pulled in a lot of your work. Thanks for the help.

{
var valueType = Type;

if ((context.Configuration.Nullability & Nullability.AllowForReferenceTypes) != 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a .HasFlag() method on enums that should help here.


namespace Json.Schema.Generation.Generators
{
internal abstract class BaseReferenceTypeGenerator : ISchemaGenerator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Baking this into the generator class hierarchy doesn't really help with extensibility. If someone wants to create their own generator, they'd have to inherit from this class (but they can't because it's internal) or handle this logic on their own.

/// <summary>
/// Default value. Type `null` will not be applied to schema.
/// </summary>
Disabled = 1 << 0,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should be the default value, so just 0.

/// <summary>
/// Gets or sets the application of type `null` to schema.
/// </summary>
public Nullability Nullability { get; set; } = Nullability.Disabled;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That allows you to not set this.

@gregsdennis
Copy link
Collaborator Author

Closing this one, too, as I'm going to merge #128. Thanks for your work on this.

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

Successfully merging this pull request may close these issues.

None yet

2 participants