Embed Value Types#21
Conversation
samlown
left a comment
There was a problem hiding this comment.
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.
| // 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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
YAMLEmbeddedStructsoption. - Embed Anonymous structs by default, i.e. the same as what the current
YAMLEmbeddedStructs = falsedoes. - 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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
@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.
|
Closing this one. Hopefully the latest release ( |
Adds support for using embedded non-struct type fields.
Example:
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
MapTypefield on several examples, which now appears in the schema output.