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

feat: Add NullableFromJSONSchemaTags option #106

Closed
wants to merge 9 commits into from

Conversation

candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented Oct 3, 2023

This changes default behavior to look for the jsonschema:"nullable" tag to mark field optional only Reflector.NullableFromJSONSchemaTags is set to true.

In Go it's perfectly fine to unmarshal the following types from null:

  • pointers
  • slices
  • maps
  • interfaces

This PR also includes the changes from #107, so it might be better to merge that one first.

@candiduslynx
Copy link
Contributor Author

@samlown this changes the default behavior to be closer to default encoding/json implementation.

The other option could be make the default behavior to look for tags & only explicitly use field type for nullability (when some option is set).

@samlown
Copy link
Contributor

samlown commented Oct 4, 2023

@candiduslynx thanks for this, but I'm trying to understand what exact problem you're trying to solve with this PR. For me, the nullable type is implied when there is no value, I'm really not sure why you'd want to define it explicitly.

@candiduslynx
Copy link
Contributor Author

candiduslynx commented Oct 4, 2023

@samlown consider json {"a":null}.
If schema defines property a having "type": "object", the validation will fail.
But the {} might validate successfully, if a isn't a required property.
So, for structs that don't have json:",omitempty" a perfectly valid marshaled representation with explicit null value will fail.

Consider the example added in 26eed64:

type TestNullableField struct {
	A *string `json:"a"`
}
Old behavior (`NullableFromJSONSchemaTags: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": {
          "type": "string"
        }
      },
      "additionalProperties": false,
      "type": "object",
      "required": [
        "a"
      ]
    }
  }
}

New behavior (`NullableFromJSONSchemaTags: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": {
          "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
Copy link
Contributor Author

@samlown This PR also includes the changes from #107, so it might be better to merge that one first.

@candiduslynx
Copy link
Contributor Author

I'm going to reopen a fresh version that preserves default behavior + add README.md doc for this

@candiduslynx candiduslynx deleted the fix/field-nullability branch October 6, 2023 09:39
@candiduslynx
Copy link
Contributor Author

Replaced by #110

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.

2 participants