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

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

Open
dist1ll opened this issue Jul 25, 2023 · 3 comments

Comments

@dist1ll
Copy link

dist1ll commented Jul 25, 2023

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?

@bufdev
Copy link
Member

bufdev commented Jul 25, 2023

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 bufdev changed the title Buf considers moving messages to other package a wire-breaking change Introspect message fields that change to see if the message definitions are compatible Jul 25, 2023
@dist1ll
Copy link
Author

dist1ll commented Jul 25, 2023

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! :)

@benesch
Copy link

benesch commented Nov 6, 2023

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.

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

No branches or pull requests

3 participants