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 failing test case for unsigned integers #125

Closed
wants to merge 3 commits into from

Conversation

paulyoung
Copy link

According to @natefaubion, the representation of uint64 over the wire should be the same as int64. These changes demonstrate that when using proto3-suite, it isn't.

@gbaz suggested that this is actually a swagger2 issue, but I'm not familiar enough with protobuf and/or swagger to know where this falls so I've added a failing test case here for now.

For me the test fails as follows:

Running 1 test suites...
Test suite tests: RUNNING...
Tests
  doctests:                                                                 Running all doctests...
tests/TestCodeGen.hs:259: failure in expression `schemaOf @UnsignedInts'
expected: {"properties":{"unsigned32":{"maximum":2147483647,"format":"int32","minimum":-2147483648,"type":"integer"},"unsigned64":{"maximum":9223372036854775807,"format":"int64","minimum":-9223372036854775808,"type":"integer"}},"type":"object"}
 but got: {"properties":{"unsigned32":{"maximum":4294967295,"minimum":0,"type":"integer"},"unsigned64":{"maximum":18446744073709551615,"minimum":0,"type":"integer"}},"type":"object"}

@j6carey
Copy link
Collaborator

j6carey commented May 13, 2020

My understanding of the situation is that int32 and sint32 both have range [2^31, 2^31-1], and that uint32 has range [0, 2^32-1], as one would expect. The only surprising part is the encoding scheme: encoding an int32 means computing the value of uint32 to which it is equal modulo 2^32, then encoding that unsigned value as a varint. However, encoding an sint32 uses a different mapping to uint32 that favors numbers having low absolute value, instead of favoring small nonnegative numbers. See the Protobuf specification: https://developers.google.com/protocol-buffers/docs/encoding#varints .

As far as I know, proto3-wire does in fact follow the Protobuf specification in regards to how it encodes these various integral types.

@natefaubion
Copy link

I don't think there's anything wrong with the wire format. The problem is in how our swagger spec is reporting the type. It's reporting it as an unformatted integer with a particular range, which isn't correct. It's formatted as a string, in the same manner that int64 is.

tests/TestCodeGen.hs Outdated Show resolved Hide resolved
@natefaubion
Copy link

For posterity, I'm copying over the note I made in an internal ticket:

We do not use uint64 anywhere else in the API. Swagger is tagging this as an integer, which we interpret to be a JSON number over the wire. int64 (which is very common) is tagged as an integer($int64). We use the additional format annotation to interpret this as a JSON string over the wire and parse it as BigInt. We need an additional format tag on the swagger side for uint64 (which has the same wire representation as int64) in order to support this.

@paulyoung
Copy link
Author

I'm going to make a pull request to the swagger2 package.

@paulyoung
Copy link
Author

This should be fixed by GetShopTV/swagger2#215

@paulyoung paulyoung closed this May 14, 2020
@paulyoung paulyoung deleted the paulyoung/uint branch May 14, 2020 21:03
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.

None yet

4 participants