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

Allow for unmarshalling Numbers to Strings #875

Closed
abergmeier opened this issue Jun 20, 2019 · 5 comments
Closed

Allow for unmarshalling Numbers to Strings #875

abergmeier opened this issue Jun 20, 2019 · 5 comments

Comments

@abergmeier
Copy link

abergmeier commented Jun 20, 2019

Is your feature request related to a problem? Please describe.
With regards to the JSON spec, a Number is not limited in scale or precision. As such, there are cases where currently there is no way of unmarshalling huge numbers with jsonpb without losing information.

Describe the solution you'd like
Allow Number to be unmarshalled to string.
Since the Protobuf protocol does not natively or via a WellKnownType support 64bit+ scalar types, rather allow application specific code, later on, to convert that information properly.
Should probably make use of json.Number, then.

Describe alternatives you've considered
We could add a WellKnownType and implement unmarshalling to that.
For a general purpose type it could be something like:

message BigInt {
   bytes lhs = 1;
   bytes rhs = 2;
}

which looks ugly enough and probably also not particularly useful.

@puellanivis
Copy link
Collaborator

The project has pretty much entirely rewritten the json protobuf encoding system in the “v2” reimplementation, and is hoping to release that soon. As such, work on this implementation is largely isolated to bug fixes only.

Addressing your PR at merit, though:

The JSON standard states:

This specification allows implementations to set limits on the range and precision of numbers accepted. ref

As the JSON implementation works in Go though, the full precision of a int64 will parse into an int64 even though some implementations of JSON might truncate its accuracy through a float64.

However, the Protobuf standard only defines the types double, float, int32, int64, uint32, uint64, sint32, sint64 (as well as fixed integer widths) anyways, and the jsonpb library was never intended to support arbitrary JSON mapping into a protobuf structure, so, a JSON value that does not fit into the appropriate field’s type would be considered out of spec, and we do not need to support it.

But that does not matter so much, because the code you offer only changes the behavior to include parsing an unquoted JSON number into a protobuf field of type string. However, there was never anything stopping anyone from quoting a number of arbitrary precision and storing it in a string value. So, the added functionality, besides not matching any spec we need to meet, is minimal. i.e. it saves two bytes on the JSON input/output for a field that is explicitly string already, where the value also happens to be numeric.

Finally, implementing a new WellKnownType is out of scope for this project, and should be brought up with the principle protobuf team, as this project is concerned solely with the Go implementation of the protobuf standard.

I hope this has been more informative than just simply a, “we have rewritten the whole package, and are not accepting anything but non-critical bug fixes to the current implementation.”

@abergmeier
Copy link
Author

The project has pretty much entirely rewritten the json protobuf encoding system in the “v2” reimplementation, and is hoping to release that soon.

Interesting. I based csvpb on jsonpb - so naturally will have a look whether to incorporate v2 once it has been released.

As such, work on this implementation is largely isolated to bug fixes only.

Understandably.

As the JSON implementation works in Go though, the full precision of a int64 will parse into an int64 even though some implementations of JSON might truncate its accuracy through a float64.

Not sure I get your point. Go's JSON implementation explicitly implements decoding Numbers to json.Number (backed by a string).

a JSON value that does not fit into the appropriate field’s type would be considered out of spec, and we do not need to support it.

That is fair. Having a practical need, though:

Java -> JSON -> Go
BigDecimal -> Number -> something

it seems excessive to pre-transform JSON just to appease Protobuf. That said - having a quick look at v2 it seems like there might be some better ways of implementing this kind of "custom" decoding without having to change the library internas, right?

I hope this has been more informative than just simply a, “we have rewritten the whole package, and are not accepting anything but non-critical bug fixes to the current implementation.”

Yes, thanks for taking the time.
I think I will propose to my team to use the modified jsonpb variant going forward. Then we have no need for added features in jsonpb (and will close the PR).

Side note - would you consider moving v2 to another URL? I think these breaking changes could disrupt a lot of projects.

@puellanivis
Copy link
Collaborator

Yes, Go’s JSON implementation does allow decoding numbers into json.Number, but the field has to actually be a json.Number. However, if I am unmarshaling a numeric value in JSON into a field that is of type int64, it will not support an arbitrary bit width, and since the protobuf standard only defines scalars with limited bit widths, having support for an arbitrary length numeric value is entirely out of the scope of these packages.

it seems excessive to pre-transform JSON just to appease Protobuf.

But this is the very purpose of jsonpb, to implement the narrow and specifically defined mapping of protobufs into and out of JSON.

If you are looking for far more generalized use of JSON, then you should be using the standard library encoding/json where you can use json.Number directly, and perhaps even define your own type with MarshalJSON/UnmarshalJSON into/out-of a math/big.Int.

@abergmeier
Copy link
Author

Yes, Go’s JSON implementation does allow decoding numbers into json.Number, but the field has to actually be a json.Number. However, if I am unmarshaling a numeric value in JSON into a field that is of type int64, it will not support an arbitrary bit width, and since the protobuf standard only defines scalars with limited bit widths, having support for an arbitrary length numeric value is entirely out of the scope of these packages.

✔️

But this is the very purpose of jsonpb, to implement the narrow and specifically defined mapping of protobufs into and out of JSON.

Really? From looking at the code there are precedences, where the mapping is way looser than I would expect from a narrow implementation.

If you are looking for far more generalized use of JSON, then you should be using the standard library encoding/json where you can use json.Number directly, and perhaps even define your own type with MarshalJSON/UnmarshalJSON into/out-of a math/big.Int.

Our scenario is rather that some teams haven't bought into Protobuf yet. And for them we convert JSON to Protobuf. What we ideally want is to have a way to custom unmarshal e.g. a Number to string.

@dsnet
Copy link
Member

dsnet commented Jun 24, 2019

We cannot accept this change as our JSON implementation is fundamentally subservient to the protobuf <-> JSON specification. Since that specification does not permit string fields to accept numbers, neither can we.

@dsnet dsnet closed this as completed Jun 24, 2019
@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants