buf icon indicating copy to clipboard operation
buf copied to clipboard

Introspect message fields if the field type changes to see if the actual message types are breaking for WIRE, WIRE_JSON

Open ParkMyCar opened this issue 2 years ago • 3 comments

When detecting breaking changes the linter emits a false positive when changing the type of a mesasge. The format of our buf.yaml is:

version: v1
breaking:
  use:
    - WIRE

Example

Say you have the protobuf message:

message Error {
  oneof kind {
    google.protobuf.Empty overflow = 1;
  }
}

and you change it to:

message Error {
  message OverflowValue {
    string value = 1;
  }

  oneof kind {
    OverflowValue overflow = 1;
  }
}

this is a wire compatible change, but buf indicates that it is not.

google.protobuf.Empty is a message with zero fields, and OverflowValue is a message with one optional field. For a type originally serialized with the first version, and deserialized with the second, you'll get an OverflowValue message where the inner value field is an empty string.

Apologies if this is a duplicate issue, I searched but could not find anything 🙂

ParkMyCar avatar Jun 07 '23 15:06 ParkMyCar

The rule doesn't mention anything regarding messages: https://buf.build/docs/breaking/rules/#field_wire_compatible_type. But I don't see why this shouldn't be possible for WIRE compatibility. Thank you for reporting!

srikrsna-buf avatar Jun 07 '23 17:06 srikrsna-buf

buf does not do introspection on actual message field types - buf assumes that changing the type of a field is a breaking change. We could in theory go further than this specifically for WIRE and WIRE_JSON, by saying if a given field changes between message types, running breaking change detection between them, but to be candid I don't know if we'll get to this anytime soon - generally, changing your core message type for a field is not advisable.

bufdev avatar Jun 07 '23 19:06 bufdev

Thanks for the quick responses! 🙂

...but to be candid I don't know if we'll get to this anytime soon - generally, changing your core message type for a field is not advisable.

That's totally fair! I just wanted to call this out because it was a change that is technically wire compatible, but buf was telling me it's not

ParkMyCar avatar Jun 07 '23 19:06 ParkMyCar

We're going to close this as stale, as I don't think this is an update we're going to be able to staff unfortunately. Apologies.

bufdev avatar Jun 13 '24 21:06 bufdev

Actually on second thought, this might be something we want to do. But going to keep this issue closed in favor of https://github.com/bufbuild/buf/issues/2318.

bufdev avatar Jun 13 '24 21:06 bufdev