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

Embed Value Types #21

Closed
wants to merge 2 commits into from
Closed

Conversation

sbward
Copy link

@sbward sbward commented Mar 25, 2022

Adds support for using embedded non-struct type fields.

Example:

type CustomStringType string

type EmbedValueTypeTest struct {
	CustomStringType
}

Output for EmbedValueTypeTest:

{
  "$schema": "http://json-schema.org/draft/2020-12/schema",
  "$id": "https://github.com/invopop/jsonschema/embed-value-type-test",
  "$ref": "#/$defs/EmbedValueTypeTest",
  "$defs": {
    "EmbedValueTypeTest": {
      "properties": {
        "CustomStringType": {
          "type": "string"
        }
      },
      "additionalProperties": false,
      "type": "object",
      "required": [
        "CustomStringType"
      ]
    }
  }
}

Notes

  • The tests already have an embedded MapType field on several examples, which now appears in the schema output.
  • I removed a line that lowercased embedded field names for consistency. If a JSON tag is present on an embedded value type, it will be used for the name. Otherwise the name of the field will be the name of the type.

Copy link
Contributor

@samlown samlown left a comment

Choose a reason for hiding this comment

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

The MapType was explicitly left out in this PR: alecthomas/jsonschema#35, which makes me wonder what the correct strategy is here. The original tests and PR are not clear as to the objective, or what was failing before.

@@ -951,11 +951,9 @@ func (r *Reflector) reflectFieldName(f reflect.StructField) (string, bool, bool,

// field anonymous but without json tag should be inherited by current type
if f.Anonymous && !exist {
if !r.YAMLEmbeddedStructs {
if !r.YAMLEmbeddedStructs && f.Type.Kind() == reflect.Struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what to think about this.

The YAMLEmbeddedStructs is something I haven't managed to understand. Even reading the original PR I still don't get it: alecthomas/jsonschema#74

I do think however the struct check should be on it's own line, even if that results in duplicated code for the sake of clarity.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

It seems to me that the YAMLEmbeddedStructs feature is mostly about disabling embedding, and the association with YAML is probably unnecessary.

Embedding semantics should be customizable through options to accommodate everyone's needs. In my opinion some more work is needed to improve the configuration API in this area.

I would be happy to make additional changes along those lines.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I get your point now and agree. The YAML part and naming in general is very confusing, in effect, it does the reverse of what it suggests.

I'd suggest an alternative approach:

  • Remove the YAMLEmbeddedStructs option.
  • Embed Anonymous structs by default, i.e. the same as what the current YAMLEmbeddedStructs = false does.
  • Continue to ignore any other anonymous type, like string, map, etc.
  • If the Anonymous type (string, map, struct, etc.) has a json:"name" tag, then we treat it as a property.

Would that work for your use-case?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I just left a comment that was wrong so I deleted it. Here is my updated reply:

Yes, that would work for me, but it doesn't seem like the best solution because it doesn't follow Go's conventions for serialization. I would tweak your suggestions such that all anonymous types are embedded by default, and, since this is a breaking change, we would release a new major version (by changing the module declaration to module github.com/invopop/jsonschema/v2 v2.0.0 which will require developers to go get the package and opt-in to the changes). This will not disrupt anyone's projects and it gives us the ideal outcome :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbward sorry, I thought my suggestion was sticking to Go conventions, I totally agree, by default we want to generate JSON Schema that matches the Golang output! I've just tested, and sure enough, anonymous fields are serialised with embedded type names.

I'm not sure we need to worry to much about versions here. This repo was forked quite recently, and does not have a v1 tag, breakage should be expected, within reason!

So to rehash the points:

  • Remove the YAMLEmbeddedStructs option.
  • Merge Anonymous structs with no tag (as per Go)
  • Embed all other anonymous types with or without tags and anonymous structs with tags as properties.

I do not think we should reject options that change this (like YAMLEmbeddedStructs). If you want "property-like" functionality of a struct, tag it, so that the JSON Encoders will be compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbward I need to fix the handling of root non-struct objects, so managed to make a proposal for this. Please see #23 and let me know if you think that would help for your use-case. Cheers.

@samlown
Copy link
Contributor

samlown commented Jul 6, 2022

Closing this one. Hopefully the latest release (v0.5.0) will solve the original issue.

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