YAML tag support

[samlown]

In this PR #23 we took steps to remove support for YAML tags. Previous versions of jsonschema did have support for using yaml if json was not available, or if a configuration option was enabled forcing priority of the yaml tag.

Ultimately, the decision to remove support for YAML was based on the potential for confusion. JSON Schema isn't really optimised for the YAML use-case, which has a more complex syntax. If you ever tried to parse a JSON file created based on a JSON Schema generated using YAML tags, you would get unexpected results.

If you need to deal with YAML, our recommended approach is to first convert to JSON and then parse using the json tags. The invopop/yaml repo will handle this for you.

[adrienaury]

Hello,

I really don't know what to think about this. My use-case is to generate a jsonschema that will be used to validate YAML file in VSCode (via this plugin).

I just added the missing json tags in my code, and it produces the exact same jsonchema as before (https://github.com/CGI-FR/PIMO/pull/142/files#diff-79ca10368c383e5ed5c7d8885fe1f4c2758b5ae4afd3feaf629e3a348b69b054) so I think it's fine. I don't see why you should keep the yaml tags support, for now. I hope i'm right ! :)

[samlown]

Glad to hear it's working! I haven't managed to get YAML Schema loading to work in VSCode yet. Works great with JSON files though.

Do let us know if you reach a different conclusion in the end!

[cupcakearmy]

Samesies here. Adding json is fine i guess :)

[samlown]

haha I appreciate its a bit frustrating if you have yaml:"var" everywhere already, but I do think in the long run it'll keep things cleaner.

[balki]

In my usecase, I need to parse yaml config to go-struct. No need for json tags or json conversion at all. Used previous version of jsonschema to generate schema.json which is used by IDE/editors for intellisense/autocompletion. With the new version, json tags need to be added even though the application itself does not need them. It would be nice to have yaml tags support back.

[samlown]

Hi Balki, sorry to hear you're not happy. I appreciate its annoying to have to change tags. Have a look through this (old, but still valid) blog post and let me know if you still feel the same: https://web.archive.org/web/20150812020634/http://ghodss.com/2014/the-right-way-to-handle-yaml-in-golang/. The https://github.com/invopop/yaml project is a great alternative for configuration files.

I'm still against re-adding support for yaml tags as for the majority of users it can become confusing, and just a one-off pain for anyone with yaml: tags.

[mathewTH]

My main argument applicable to our code would rest on that idea that code should be self documenting as much as possible. Using json tags in the code when you really mean yaml makes the code less readable.
In our code base we have many structs with different tags depending on the uses, eg bson for database, json for api, and yaml for config. These different struct types are usually in different packages depending on the purpose, but the tags only reinforce the distinction.
It really is a shame to make our code more confusing just to make use of this library.

I do see the point around the use case where you are parsing (or writing) both yaml and json for the same struct. If you have that use case, then it does make sense to use one set of any customisations for both.
From a purely semantic point of view though, given that yaml is a superset of json (as the blog rightly points out), doesn't it make more sense to use a yaml unmarshaller for both, since the json will be valid yaml too? (the yaml may not be valid json) this avoids any yaml/json conversion. For marshalling, some direct conversion between yaml/json does seem best.

Its a shame to see some of the bugs and behaviours pointed out with go-yaml. Given the blog was written in 2014, and gopkg.in/yaml.v3 has come out much more recently, have the issues been pointed out to the maintainers so they could defend or change them with the v3 breaking changes?

I want to offer 1 critique of the examples shown in the blog. They do illustrate the json/yaml behaviour differences, but in my opinion, code that avoids any issues from the behaviour, would be better regardless of whether yaml/json unmarshalling had that discrepancy or not.
If you use a pointer in a go struct you should be prepared to handle the fact its going to be nil by default. This ties into this quote from https://go.dev/doc/effective_go#allocation_new "it's helpful to arrange when designing your data structures that the zero value of each type can be used without further initialization" (I think this statement stands the test of time, even if that document as a whole doesn't)
Given this struct:

type DateOfBirth struct {
     Time *time.Time  // Pointer to a time.Time struct.
}
func (dob *DateOfBirth) UnmarshalJSON(b []byte) error { ... }

I would be asking why do you want time to be nil, and is there likely to ever be any other fields in the struct?
without good answers I would be changing it to

type DateOfBirth time.Time 
func (dob *DateOfBirth) UnmarshalJSON(b []byte) error { ... }

which incidentally, won't have any problems unmarshalling null from yaml.

[samlown]

Mathew, thanks for raising your point of view! I'm not convinced, but while reading your post I had an idea.

While I disagree with explicit support for YAML given the potential for confusion, I can understand the need for flexibility. My proposal would be to make the "field name tag" configurable. i.e., if you want to use a different tag name instead of json, you could configure the reflector to use yaml instead. Essentially, its a question of changing this line https://github.com/invopop/jsonschema/blob/main/reflect.go#L992 from:

func (r *Reflector) reflectFieldName(f reflect.StructField) (string, bool, bool, bool) {
	jsonTagString, _ := f.Tag.Lookup("json")

to:

func (r *Reflector) reflectFieldName(f reflect.StructField) (string, bool, bool, bool) {
	jsonTagString, _ := f.Tag.Lookup(r.FieldNameTag)

Would that work for you? Of course, with this simple approach you'd loose backwards compatibility with regular json tags, but I really do hope you're not mixing and matching, that would be incredibly confusing.

[mathewTH]

I believe that would work for me. I'm looking at whether we want to switch to github.com/invopop/yaml, but this should allow me to use this library without requiring other changes.
There shouldn't be any cases where both json and yaml tags are used on the same struct.

[mathewTH]

I have raised a PR to do what you suggest #64

[Duologic]

I've been using this library to generate JSON Schemas to eventually create Jsonnet libraries. However running this against Prometheus didn't give me the usual result, turns out it only supports the YAML tags and don't seem willing to change it. One of the arguments is that they support YAML anchors.

I'm not involved in the prometheus project and figure I might run into similar walls elsewhere so dropping a message here. Perhaps you can suggest a workaround for me?

[Duologic]

After a bit of fiddling I now have a somewhat useable output, added these three lines:

# diff reflect.go
992a993,995
>     if jsonTagString == "" {
>         jsonTagString, _ = f.Tag.Lookup("yaml")
>     }

[Duologic]

I guess #64 covers this workaround too.

[Duologic]

I'm using the workaround in #64 now.

[samlown]

Glad to hear that's working! We just need some tests on #64 then we can merge it.

[balki]

@samlown Added tests. Please take a look. #75