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

fix: Broken protobuf varint functions, add tests #653

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aiven-anton
Copy link
Contributor

About this change - What it does

Fixes #651. I have only looked at the functions locally. I'll leave this in draft until further investigation into why the code that relies on these broken fundamentals is not itself broken. There's a possibility these paths are never exercised, in which case a better course of action is to prune the code.

  • Adds round-trip hypothesis tests for read_indexes + write_indexes.
  • Adds round-trip hypothesis tests for read_varint + write_varint.

Both these tests fails without patching the functions.

  • write_varint was seemingly entirely broken. Replace bytearray casting with .to_bytes() invocation.
  • read_indexes expects first value to be size of array, adjust write_indexes to write length before writing values.
  • read_indexes special-cased size == 0 for no apparent reason, this is removed as it looks broken and doesn't have symmetry in write_indexes.

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.

karapace.protobuf.encoding_variants.write_varint is broken
1 participant