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

Add well-known types as fields in the compliance service #1216

Open
vchudnov-g opened this issue Sep 9, 2022 · 2 comments
Open

Add well-known types as fields in the compliance service #1216

vchudnov-g opened this issue Sep 9, 2022 · 2 comments
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@vchudnov-g
Copy link
Contributor

Well known types are listed in https://developers.google.com/protocol-buffers/docs/reference/google.protobuf, and their JSON representations are summarized in the bottom part of this table.

We should have well-known types whose JSON representation is a string be fields in compliance.proto's ComplianceData to ensure clients serialize/deserialize them correctly.

@vchudnov-g vchudnov-g added priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. labels Sep 9, 2022
@jskeet
Copy link
Contributor

jskeet commented Sep 15, 2022

Yup, I've just fixed a bug in .NET's transcoding due to this.

We should have well-known types whose JSON representation is a string

More than that, I think - I'd expect all of BoolValue, Int32Value, Int64Value, UInt32Value, UInt64Value, SingleValue and DoubleValue to have tests. The 64-bit integer and floating point types are particularly tricky, as they can appear in JSON as either strings or numbers. (For floating point, "Infinity", "-Infinity", "NaN" are all represented as strings; others are numbers. 64-bit integers can be represented as numbers, but only as far as 2^53 after which they have to be represented as strings in order to avoid a loss of precision.)

I'm adding this to the unit test conformance data we use in .NET, and which I hope to move to the conformance-tests repo soon. Having it in Showcase as well would be good though :)

@jskeet
Copy link
Contributor

jskeet commented Sep 15, 2022

One tricky aspect of StringValue: the JSON representation would escape some characters, where we presumably don't want to. For example, a value of \ (backslash) is "\\" in JSON, and I suspect we need to make sure that we don't have those double backslashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants