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

int is unsupported for Avro long #421

Closed
lovromazgon opened this issue Jul 29, 2024 · 5 comments · Fixed by #422
Closed

int is unsupported for Avro long #421

lovromazgon opened this issue Jul 29, 2024 · 5 comments · Fixed by #422

Comments

@lovromazgon
Copy link
Contributor

lovromazgon commented Jul 29, 2024

I think there's an issue with safely encoding values of the Go type int on 64-bit systems. The type can contain values up to math.MaxInt64, while only values up to math.MaxInt32 can be safely encoded in the Avro type int. The type could safely get encoded into long, but the library doesn't support that.

You can find a reproducible test case here: https://gist.github.com/lovromazgon/50ecf204e65f7d1ad080fbd0fa7d52fb

The test tries to encode and decode a Go value of type int containing 2147483648 (i.e. math.MaxInt32 1) with two schemas:

  • with schema int it succeeds, but the decoded value doesn't match (overflow)
  • with schema long it fails because int is unsupported for Avro long
=== RUN   TestHamba_Int
    hamba_test.go:23: expected 2147483648, got -2147483648
    hamba_test.go:30: unexpected error: failed to encode: avro: int is unsupported for Avro long

The first case is a possible footgun, it's debatable if you want to add an option for the encoder to be "strict" and disable the support for encoding values of Go type int into Avro type int.

I personally care more about the second case, encoding a Go int into the Avro type long should work.

Let me know what you think, I can try to work on this and open a PR with a fix.

Possibly related to #59.

@lovromazgon lovromazgon changed the title int is unsupported for Avto long int is unsupported for Avro long Jul 29, 2024
@nrwiersma
Copy link
Member

int is only 64-bit on a 64-bit system, so if you only look at 64-bit systems, you are correct. But on a 32-bit system this would break.

@lovromazgon
Copy link
Contributor Author

Right, when decoding this can be unsafe, but encoding should be safe either way.

What do you think about this approach, which ensures that decoding is only allowed on 64-bit systems?

Diff: main...lovromazgon:avro:encode-int-with-long

@nrwiersma
Copy link
Member

This seems like a good option to me. There will need an explanation in the README to explain this though.

@lovromazgon
Copy link
Contributor Author

Ok, I opened the PR and added a note to the readme, let me know what you think. And thanks for the quick response @nrwiersma! 🙇

@nrwiersma
Copy link
Member

Thanks for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants