Skip to content

Conversation

jhaber
Copy link
Member

@jhaber jhaber commented Jun 22, 2020

Protobuf has a Struct type which can be used to represent arbitrary JSON structures. However, all numbers are stored as doubles. This means it's easy to accidentally lose precision when sending an integral value. This PR ensures that we only accept integral values if they can be converted to a double without loss of precision.

@stevegutz @kmclarnon @Xcelled

@stevie400
Copy link
Contributor

LGTM, though I worry about existing data causing 500s.

Should we report this to the java protobuf maintainers?

@kmclarnon
Copy link
Member

I'm also concerned about just starting to throw in these cases. Can we add an option to enable this behavior and then enable it internally to hubspot?

@jhaber
Copy link
Member Author

jhaber commented Jun 22, 2020

It seems like the built-in JsonFormat parsing doesn't throw an exception in this case, eg, this code runs fine and silently loses precision also:

HasValue.Builder builder = HasValue.newBuilder();
JsonFormat.parser().merge("{\"value\":-8747832031878802303}", builder);

I can open an issue on protobuf-java to see whether this is desirable

@jhaber
Copy link
Member Author

jhaber commented Jun 22, 2020

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

Successfully merging this pull request may close these issues.

3 participants