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

protobuf: recognize tag alter modification #821

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

keejon
Copy link
Contributor

@keejon keejon commented Feb 21, 2024

About this change - What it does

This PR introduces a new FIELD_TAG_ALTER modification in order to recognize tag change of fields.

Why this way

Changing the tag of a field is not backwards compatible but currently goes unnoticed because fields are compared by tag and not by field name. Because of this compatibility check will only recognize FIELD_NAME_ALTER modification which is backward compatible. This PR changes this so that if field name exists in both schemas it will compare the tags and add FIELD_TAG_ALTER modification if the tags don't match.

@keejon keejon force-pushed the protobuf-recognize-tag-alter-modification branch 2 times, most recently from 93b90e7 to 3447697 Compare February 21, 2024 13:56
@keejon keejon marked this pull request as ready for review February 21, 2024 14:57
@keejon keejon requested review from a team as code owners February 21, 2024 14:57
Copy link
Contributor

@eliax1996 eliax1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, understood the implementation by reading the protobuf doc. Leaving inlined the doc as reference to understand the implementation.

  1. From the protobuf doc on common errors:

renumbering fields (sometimes done to achieve a more aesthetically pleasing number order for fields). Renumbering effectively deletes and re-adds all the fields involved in the renumbering, resulting in incompatible wire-format changes.

  1. And also, under the message structure description:

Message Structure

A protocol buffer message is a series of key-value pairs. The binary version of a message just uses the field’s number as the key – the name and declared type for each field can only be determined on the decoding end by referencing the message type’s definition (i.e. the .proto file). Protoscope does not have access to this information, so it can only provide the field numbers.

When a message is encoded, each key-value pair is turned into a record consisting of the field number, a wire type and a payload. The wire type tells the parser how big the payload after it is. This allows old parsers to skip over new fields they don’t understand. This type of scheme is sometimes called Tag-Length-Value, or TLV.

Other that that I have a couple of doubts/requests that I've left as comment to the review.
Thanks for you work! Great job 😄

tests/unit/protobuf/test_protobuf_compatibility.py Outdated Show resolved Hide resolved
karapace/protobuf/message_element.py Show resolved Hide resolved
@keejon keejon force-pushed the protobuf-recognize-tag-alter-modification branch from 3447697 to ddc3ddb Compare February 23, 2024 11:51
eliax1996
eliax1996 previously approved these changes Feb 23, 2024
@keejon keejon force-pushed the protobuf-recognize-tag-alter-modification branch from ddc3ddb to d9ab8db Compare February 23, 2024 12:44
@eliax1996 eliax1996 enabled auto-merge February 23, 2024 13:02
@eliax1996 eliax1996 merged commit 021640d into main Feb 23, 2024
8 checks passed
@eliax1996 eliax1996 deleted the protobuf-recognize-tag-alter-modification branch February 23, 2024 13:20
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.

2 participants