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

encoding/protojson: Add Marshal option to add YAML !!int tags #1280

Closed
Capstan opened this issue Feb 23, 2021 · 3 comments
Closed

encoding/protojson: Add Marshal option to add YAML !!int tags #1280

Capstan opened this issue Feb 23, 2021 · 3 comments

Comments

@Capstan
Copy link

Capstan commented Feb 23, 2021

Is your feature request related to a problem? Please describe.
The int64 type doesn't fit into standard JSON and is rendered as a JSON string. However, if JSON is interpreted by a YAML interpreter, it will treat the string as a string and not as a number, so it is type-lossy to go from proto to YAML via the JSON marshaler.

Describe the solution you'd like
Like is a strong term. A minimal impact solution would be to add a EmitYAMLIntTags field to protojson.MarshalOptions that would, for the cases where an int64 type is emitted that it's prefixed with !!int. This will cause a YAML interpreter to treat it not as the string it has been encoded in. It does, however, render it as invalid JSON, so may not be within the scope of this particular package.

Describe alternatives you've considered

  • Post-processing
    • "manual" coercion of the resulting JSON based on known messages.
    • if we did the same protoreflect-based processing, we'd have re-implemented this package.
  • Option to emit numbers instead of strings for int64 types. This hits a cliff where it's no longer legitimate JSON, even if it would be legitimate YAML.
  • Option to emit numbers where possible instead of strings. This muddles the output type and makes it harder on any parser to consume them intelligently.
  • Create a new encoding/protoyaml package that goes directly to YAML and not YAML-via-JSON.

Additional context
This happens to occur when using Kubernetes' generated protos, e.g., core.v1, to configure Kubernetes. These types contain int64, but if we render it to JSON, we lose the type information, and they get treated incorrectly as strings. The generated protos are created using go-to-protobuf which maps the int64 types straight across.

@Capstan
Copy link
Author

Capstan commented Feb 23, 2021

FWIW, I've prototyped something in the monorepo, and could probably turn it into a PR if y'all thought it was worth entertaining.

@dsnet
Copy link
Member

dsnet commented Feb 23, 2021

I don't think we could ever accept EmitYAMLIntTags for the reason you gave:

It does, however, render it as invalid JSON, so may not be within the scope of this particular package.

The most reasonable option would be:

Option to emit numbers instead of strings for int64 types

However, we have a policy of not accepting any features that are not supported by the other major language implementations. For example, C++ has no such feature, Java has no such feature, and Python has no such feature. Thus, you would need to propose such a feature to the protobuf team and gain consensus that all languages should support this.

This hits a cliff where it's no longer legitimate JSON, even if it would be legitimate YAML.

RFC 8259 doesn't put any requirements that a JSON not have 64-bit precision. It only warns that some implementations may lose precision for numbers that don't fit within a IEEE-754 double precision number.

@Capstan
Copy link
Author

Capstan commented Feb 23, 2021

Fair. I'll pursue that. I'll close this and open a new FR if/when that is accepted.

@Capstan Capstan closed this as completed Feb 23, 2021
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

No branches or pull requests

2 participants