JSON Schema: Ignore fields via comment

[rodaine]

This patch enables having fields ignored in the JSON Schema generation via a comment containing jsonschema:ignore (in either a leading or trailing comment).

A comment here is expedient, but not ideal as a long-term strategy. Would be much better as a custom option. This also has no impact on PubSub generation, though could presumably be extended to support that as well.

Part of this required changes to the golden tests as source location info (read: comments) aren't preserved in the reflection descriptors by the Go protobuf library (or any of the official ones for that matter). As a result, comments have now appeared in the input.json and impacted golden tests.

Holding off documenting until this strategy is approved.

[Alfus]

should the fields really be ignored? Or somehow less prominent in the json schema. I am pretty sure it will show you an error if you try to use a field that is not listed in the json schema.

[mfridman]

@Alfus are you thinking of something more explicit like https://json-schema.org/understanding-json-schema/reference/combining#not where you can exclude specific keys?

but also, this seems like a nice property in general where you want the jsonschema to be a subset of the protos?

[Alfus]

I am suggesting the field get listed in the schema, but not have docs, or maybe json schema has a 'hid' option so it doen't suggest the field, but will still validate it, if present.

[rodaine]

The goal here is for a config file that supports some internal-only (mostly testing or experimental) properties, but hides them from end users. Some options:

• Set additionalProperties to allow extra undefined fields. This aligns well with unknown proto fields, fwiw.

• We could generate a wildcard property that will apply to any unknown fields. I'm not familiar enough with JSON Schema or IDE plugins to say if they're good at handling specialization/specificity in this context.

• Move these rules out of the comments (or field options) and up to the protoc plugin as options there. That way the caller can choose what to generate by usecase (as opposed to having it fixed in the proto source itself).

• Leave this as-is and allow the JSON schema to be an explicit subset of the actual proto definition.

[mfridman]

My vote would be option 4 (current implementation), followed closely by a plugin option to get this out of the proto definitions, something like ignore_type. I personally don't mind the extra annotations, but I can see how that's a slippery slope towards littering proto files with unrelated things.

In terms of additionalProperties or simply dropping, not sure which is better. I'd defer to the person who is actually generating and working with this (I think ignoring it makes sense to start). It also sounds like we could support both in due time and they don't have to be mutually exclusive?

My only other suggestion is let's give it a try, get some hands-on experience, report feedback and tweak it as we go.

[rodaine]

Kept it as-is for now. We can revisit with a more robust solution after we see where this all lands.

[mfridman]

I'd suggest removing the readme changes, and keeping this as an undocumented "internal feature". Otherwise lgtm as we buy us time to iterate on this.