feat: Add NullableFromType option

[candiduslynx]

Instead of #106.

Allows to define property nullability based on the field type instead of the jsonschema:"nullable" tag.
Useful when the types included are referenced from 3rd party packages.

Consider the following example:

type TestNullableField struct {
	A *string `json:"a"`
}

The following JSON is a perfectly valid value for Go:

{
  "a": null
}

The schemas produced for the TestNullableField struct are:

Old behavior (`NullableFromType:false`)

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "https://github.com/invopop/jsonschema/test-nullable-field",
  "$ref": "#/$defs/TestNullableField",
  "$defs": {
    "TestNullableField": {
      "properties": {
        "a": {
          "type": "string"
        }
      },
      "additionalProperties": false,
      "type": "object",
      "required": [
        "a"
      ]
    }
  }
}

New behavior (`NullableFromType:true`)

{
  "$schema": "https://json-schema.org/draft/2020-12/schema",
  "$id": "https://github.com/invopop/jsonschema/test-nullable-field",
  "$ref": "#/$defs/TestNullableField",
  "$defs": {
    "TestNullableField": {
      "properties": {
        "a": {
          "oneOf": [
            {
              "type": "string"
            },
            {
              "type": "null"
            }
          ]
        }
      },
      "additionalProperties": false,
      "type": "object",
      "required": [
        "a"
      ]
    }
  }
}

In both cases a is required, but the old behavior will fail for {"a":null} whereas the new one will succeed.

[candiduslynx]

@samlown this is a less intrusive implementation for #106.
It will kick in the new logic only if the NullableFromType option is used.

[candiduslynx]

@samlown I hope you're doing well in these uneasy days.

I've implemented the changes in #109 & #110 & tested them via a fork at github.com/cloudquery/jsonschema@cqmain.

The results of using the updated code along with NullableFromType: true, RequiredFromJSONSchemaTags: true is something I'm quite satisfied with.
You can find an example in schema.json file. It includes both the nullable properties based on the field type & the clashing types (you can easily find them searching for -1" suffix)).

Could you please give an estimate when these PRs (#109 & #110) could be merged? It'll be far easier to pin an upstream version instead of maintaining a fork for these 2 changes.

[kop]

Highly interested in this PR as well. Is there anything I can help with to get this merged?

[candiduslynx]

Highly interested in this PR as well. Is there anything I can help with to get this merged?

@kop I really hope @samlown has time to review/merge that.
In the meantime, we use a fork at github.com/cloudquery/jsonschema. It has cqmain default branch, that has the following PRs merged:

• fix: Proper handling of clashing types case #109
• feat: Add NullableFromType option #110
• fix: Don't limit the explicit format #111
• fix: Key construction in ExtractGoComments #112

[samlown]

Sorry for the delay there! This looks good to me, thanks! I do think there should be some extra tests for those non-pointer reflection types though.

[candiduslynx]

@samlown thanks for the review!

I have an additional question here: currently the library uses oneOf to make schema nullable.
In my opinion, a better way would be anyOf:
If the schema is already nullable (e.g., it already is {"oneOf":[{...}, {"type":"null"}]}) then adding another oneOf layer will effectively make null value to fail validation, as the oneOf requires one & only one variant to be valid:

{
  "oneOf": [
    {
      "oneOf": [
        {},
        {
          "type": "null"
        }
      ]
    },
    {
      "type": "null"
    }
  ]
}

If we use anyOf this is lifted to at least one of the variants.

I can raise this as a separate issue & address it in a follow-up PR.
WDYT?

[elise-prequel]

Hi! Echoing interest in this PR -- happy to help if there's anything outstanding to get it in.

[Rosswell]

@samlown would you be able to take another look when you have a chance?

[candiduslynx]

Hi @samlown!

Could you please advice if you need any assistance in maintenance of this library?

I see that the last commit here was 4 months ago, & I wonder whether this library is actually actively maintained or not.

[samlown]

Sorry for the delay, I really don't have much time at the moment. There are quite a few style things here that I'd be worried about, especially regarding public methods which are not tested (just make them private!)

[samlown]

These methods are inconsistent with the rest of the package, I don't think they should be in the schema.go file, nor should they be exposed publicly. Any public public method should be tested.

Also, Make in Go is typically reserved for constructors that return a new instance of a struct.

To avoid the reflect file getting too big, my suggestion would be to create a reflect_nullable.go file with these as private methods defined there.

[samlown]

It feels like this logic should be inside its own method.

[samlown]

What is the objective here? I can't see where unwrapped is consumed, making this section much harder to read as I'd guess everything is happening on the pointer.

[candiduslynx]

https://github.com/invopop/jsonschema/pull/110/files#diff-672d1dd2b9193c8818108f625c29ff25feaea4c1c50cc36cfda18108cba3e1b2R540
We need this to work with pointer fields as well

[candiduslynx]

@samlown I'm closing this PR for 2 reasons:

1. I no longer have access to the fork the PR was opened from
2. It seems to me that you just don't have the capacity to get this reviewed and merged

If you'll be available for review & merging please tag me here & I'll reopen from my own fork.