buf icon indicating copy to clipboard operation
buf copied to clipboard

Introspect message fields that change to see if the message definitions are compatible

Open dist1ll opened this issue 2 years ago • 4 comments

Consider two files

package foo.v1;
message A {
  uint64 x = 1;
}
package main.v1;
message Main {
  foo.v1.A a = 1;
}

If I now change foo to bar (renaming the directory, .proto filename and package name), without changing the type definition, buf breaking will report a breaking change, even though I configured buf.yaml to use WIRE.

main/v1/main.proto:4:3:Field "1" on message "Main" changed type from "foo.v1.A" to "bar.v1.A".

It's clear that renaming foo does not cause a WIRE-breaking change. Is this a known bug or limitation with buf? Or am I doing something wrong?

dist1ll avatar Jul 25 '23 09:07 dist1ll

Yes, buf considers this a breaking change - there's no way for buf to know that you intended to rename foo.A to bar.A - if you can come up with a way we'd detect that, let us know. In the absence of understanding this user intention, buf considers bar.A to be a new message, and foo.A to be deleted.

With regards to your example, this is a case where buf detects a field type change. Message fields are not introspected - buf just looks at the type of the field, which in this case changed from foo.A to bar.A. We could, for example, go further on a message field type change, and detect if there's any breaking changes between foo.A and bar.A, but haven't done so yet. We can explore this, however.

bufdev avatar Jul 25 '23 17:07 bufdev

I see, thanks for the explanation. It looks like I had a misunderstanding of WIRE compatibility.

I would have expected buf to just look at the message structure. On a high level, I'd do a full introspection of all nested types, ignore the names, and look at the ordering/position/encapsulation of all the primitive fields that make up the type (ignoring type semantics in the process).

Whether or not this weakening is a good idea is of course subjective. I was mostly interested to find out why this happens. Feel free to close this issue if you want! :)

dist1ll avatar Jul 25 '23 19:07 dist1ll

Just ran into this myself. I think buf's behavior here is very surprising.

To be honest, it's more than a little bit frustrating, too! Messages regularly get renamed in protobuf codebases. This is a perfectly safe operation if you're not e.g. using the JSON format. But adding a new field with the new message name and removing the old field with the old message name is not a safe operation if you're e.g. persisting messages on disk, because those persisted messages will only have data for the old field.

I see, thanks for the explanation. It looks like I had a misunderstanding of WIRE compatibility.

I think this is at the heart of the problem. You exactly understood protobuf's actual wire compatibility! But what buf calls wire compatibility is not actually protobuf wire compatibility—it's a much stricter standard about what the best practices are for protobuf evolution.

If buf called this mode BEST_PRACTICE rather than WIRE, I think this behavior would be wholly justifiable. But given that the mode is called WIRE, what I expected is that buf would exactly enforce wire compatibility—nothing more, and nothing less.

if you can come up with a way we'd detect that, let us know

What about a magic comment that specifies the old name for a message?

// buf:lint:old-name OldName
message NewName {
  // ...
}

Probably want something similar for packages, too:

// buf:lint:old-name old_name
package new_name;

Then buf could suppress warnings about changed types if the new type declares the right old-name. As long as buf insteads checks for breaking changes between OldName and NewName, I think it'd all work out.

benesch avatar Nov 06 '23 00:11 benesch