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

Add support for nullable types #124

Conversation

JMPSequeira
Copy link
Contributor

Support Nullable Types in Schema Generation

@gregsdennis
Copy link
Collaborator

This should already be supported through generating Nullable<T>. I'll verify tonight.

@JMPSequeira
Copy link
Contributor Author

This should already be supported through generating Nullable<T>. I'll verify tonight.

Got carried away. Nice lib btw.
I intend to use it on a personal project and noticed that nullable types where being generated as objects.

Copy link
Collaborator

@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.

How was the editor config generated? Does it match with my R# rules?

/// <summary>
/// Indicates if the CLR type is originally a Nullable`1.
/// </summary>
public bool IsNullable { get; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the idea of putting this on the context. It's a separate type. I'd like to see how it behaves currently.

Furthermore, a nullable int should render as "type": ["integer", "null"]. And if that's supported, should reference types have the same multi-type support? Maybe there needs to be an option that enables this kind of support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the idea of putting this on the context. It's a separate type. I'd like to see how it behaves currently.

It was the minimal disruption route.

Furthermore, a nullable int should render as "type": ["integer", "null"].

Agreed. And shouldn't an int be marked required by default?

And if that's supported, should reference types have the same multi-type support? Maybe there needs to be an option that enables this kind of support.

Reference types should render as "type":[{referenceType}, "null"] unless stated as Required, or, if Nullable reference types are enabled, not marked as nullable.

Copy link
Collaborator

@gregsdennis gregsdennis Jun 15, 2021

Choose a reason for hiding this comment

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

And shouldn't an int be marked required by default?

I want explicitly required props via [Required]. Deserializing JSON data that's missing a given prop would just result in the default value, and this is usually fine.

Reference types should render as "type":[{referenceType}, "null"]

Here's where .Net's and JSON Schema's concepts of "required" interfere. .Net's interprets absent and null (or default, rather) as the same. JSON Schema separates these ideas so that I can require a value to be present but optionally specify that it could be null. Example:

[Required]
public string Foo { get; set; }

could generate

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

but this schema would allow { "foo": null } which would fail .Net's validation.

I can see the logic behind having either required or including type: null from the C# perspective, but that leaves some open questions. For this table, I've listed what seems to be the the proper behavior, but some (❓s) I'm unsure on.

value type nullable value type reference type
required exclude null ✔️
list in required ✔️
include null
list in required ✔️
exclude null
list in required ✔️
optional exclude null ✔️
exclude from required ✔️
include null ✔️
exclude from required ✔️
include null ✔️
exclude from required ✔️

... if Nullable reference types are enabled, not marked as nullable.

I can't tell from this library if the consumer has nullable reference types enabled. I can check for the attribute, but if it's not there, I can't make assumptions because either they are using NRT and it's not nullable (don't allow nulls), or they're not using NRT (do allow nulls).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Regarding that last bit, there's a lot of chatter on recognizing the attribute for when NRT are enabled, but nothing that says what happens in C#8 if NRT are disabled. I'll have to run an experiment.

https://stackoverflow.com/questions/58453972/how-to-use-net-reflection-to-check-for-nullable-reference-type
https://github.com/RicoSuter/Namotion.Reflection
dotnet/docs#9662
https://codeblog.jonskeet.uk/2019/02/10/nullableattribute-and-c-8/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still captures the null intent,

From what I could gather from NJsonSchema and Newtonsoft the null handling is configurable.

IMHO it should be about capturing user intent, from a C#(going out on a limb here assuming most generation is to provide clients with validation logic) and zero-config perspective, making it as close to standard validation in .NET. This does not mean it can't be configurable, just that I'd make the defaults reflect this line of thought.

Either way, having it configurable would solve your doubts on how to handle it.

Copy link
Collaborator

@gregsdennis gregsdennis Jun 15, 2021

Choose a reason for hiding this comment

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

making it as close to standard validation in .NET

This is where my philosophy departs from those libraries. I want to capture JSON Schema as purely as possible. I'm not so concerned about how standard validation works. My concern is how JSON Schema wants validation to work.

The purpose of this generation library is to create a JSON Schema starting point that can be further refined by humans. It's definitely not intended to be used to generate production-ready schemas. As such, I'd like to see this entire idea covered under a configuration option.

I think it's a good idea, though. I'd just like to take a different approach to solve it.

@JMPSequeira
Copy link
Contributor Author

How was the editor config generated? Does it match with my R# rules?

Inferred from code analysis by VS

@gregsdennis
Copy link
Collaborator

I think I'd like to see what an implementation of a generator that covers Nullable<T> would look. This is the approach I'd default to.

@JMPSequeira
Copy link
Contributor Author

Implemented with a nullable Generator here. Check it out:
https://github.com/gregsdennis/json-everything/compare/master...JMPSequeira:Support_Nullable_Value_Types?expand=1

@gregsdennis
Copy link
Collaborator

This has been good stepping stone to #127 and leading on to #128. Thanks for your work on this. Closing this PR for now.

@JMPSequeira JMPSequeira deleted the Add_Support_For_Nullable_Types branch June 18, 2021 09:58
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