Removing entire message triggers NoRemovingFieldsWithoutReserve

[rodaine]

For the following test file:

syntax = "proto3";

package foo;

message Bar {
	int32 baz = 1;
}

Running protolock init, then deleting message Bar entirely. Then running protolock status.

Expected Behavior

No error.

Actual Behavior

CONFLICT: "Bar" field: "baz" has been removed, but is not reserved [test.proto]
CONFLICT: "Bar" ID: "1" has been removed, but is not reserved [test.proto]

When run with --strict false, the errors do not appear.

---

Would be glad to do a PR if there's consensus that these errors are erroneous.

[rodaine]

cc @btc

[nilslice]

@rodaine - I see how these errors being reported adds some confusion.. they're not really errors, but the tool is demonstrating expected behavior IMO. In order to provide the tool with the updated lock info, you would need to run a protolock commit after deleting the message.

However, if this doesn't meet your needs I'd be happy to consider a PR. I just don't see at the moment how changes to the RuleFunc logic would solve this without failing to notify a user that the API compatibility is broken. Introducing another RuleFunc that perhaps informed an entire message was removed (which breaks API compatibility) may be reasonable, but I could also be misinterpreting the issue 😄

[rodaine]

Sorry let me clarify more!

If a message is unused in any capacity (not part of a RPC, not a field type), deleting the entire message is not a breaking change. Of course this is a breaking change in code, but so is marking a field as reserved. Once you update, you'd have to remove references to it. Removing a message (assuming protoc compiles everything else) is not a binary incompatible change; it's removing dead code.

In our specific case, a message has been entirely deprecated from use, now we'd like to remove the message from codegen entirely. If we attempt to do so, it either complains with the above error. Or, if fully reserved, complains that the reserved was removed.

This is also happening in a CI pipeline where users do not manually control the lock file themselves, hence force/commit not being an option here.

[nilslice]

I see, thanks for the additional details 🙂

To me, it seems that you'd want to run protolock commit --force to re-declare the "locked" state of your proto files after deleting your proto message. --force indicates that the errors should be ignored, since you intend to overwrite the existing lock file regardless of the missing message fields previously reported as errors.

A successive run of protolock status should report no errors.

Again, apologies if I'm misinterpreting this.

[nilslice]

Ah I missed re: the running on CI...

The "commit" would be the responsibility of a user before pushing the code. How is the message removed from your codebase? Code-gen?

If you are automatically removing the message from a file, then I'd assume you can automatically run the "commit" as well.

[rodaine]

Ultimately we do want to allow users to force changes to the lockfile, but our current setup doesn't even give them access to the file. The details aren't all that important, but what you are saying is the solution we want to implement for anyone to force a breaking change. 😅

Where I want to move this discussion to is whether or not removing a message is considered a breaking change. If it is, then the rule should be changed to say so instead of less-accurately mentioning the fields being removed without being reserved. If it isn't (which is my stance), then it shouldn't error at all. In either case, I'm willing to submit a PR to make the appropriate change 😁

[nilslice]

Where I want to move this discussion to is whether or not removing a message is considered a breaking change. If it is, then the rule should be changed to say so instead of less-accurately mentioning the fields being removed without being reserved.

I agree that the NoRemovingFieldsWithoutReserve rule could be modified to detect wholesale message removal and report it more accurately. This could be gated by the --strict flag so that message removal can be suppressed when not in strict mode (which could solve this for your CI, I believe?)

My stance is that an API is changed in a breaking way if a message (that was in a production release) is removed completely. Especially since that message could have been nested in other messages (which would need to change, and could require breaking changes or proper field reservation). I understand in your case, that the message was not used, so the above doesn't specifically apply. But, I'd say it's generally true. The goal of protolock is to keep teams safe from these (typically) accidental proto changes that break APIs. No one would knowingly delete an in-use message and re-gen & deploy... but accidents happen

I would be in full support of a PR to detect message removal (enabled by default in strict mode, suppressed when strict==false), but I wouldn't want to silently allow messages to be removed without warning. If your issue is solved this way, and you're open to a PR, please go for it! I'm here to help however needed.

Thank you for all the detail and interest in this issue.

[rodaine]

I'll work on the PR to have the error message be more correct. And at the end of the day it's not a huge deal, just philosophically different opinion on the matter.

Removing a message is not a breaking change. Removing a field (without reservation), which uses that message as its type, is. That's where I think we have a fundamentally different idea about what is considered a breaking change. Removing a message without first deprecating all usages of it would simply not compile. I'm concerned almost exclusively with binary incompatible changes (ie, changing the type of a field or RPC IO).

Somewhat related, we do not want to run with --strict=false because some of the most important errors for us are the field removals. Ultimate solution is of course what I mentioned above (limited just by some technical/process related issues) where this is a non-issue.

Really appreciate the work on this plugin by the way! It's protecting all protos at Lyft now, including Envoy's xDS APIs.

[nilslice]

Removing a message without first deprecating all usages of it would simply not compile.

That's a very good point, and something I tend to forget where protolock linting and protoc compiling responsibilities differ.

Removing a message is not a breaking change. Removing a field (without reservation), which uses that message as its type, is.

Considering the first point, I think you are indeed correct. protolock doesn't need to enforce anything that is taken care of protoc, and the removal of an in-use message falls under that category.

I would actually like to defer to your experience on this, so please feel free to PR how you see fit.

Really appreciate the work on this plugin by the way! It's protecting all protos at Lyft now, including Envoy's xDS APIs.

Very glad to hear it is helpful! That's awesome to know how much Lyft & Envoy can take advantage of it -- do you mind if I (or you if you'd like) add both names to the README section: https://github.com/nilslice/protolock#related-projects--users

[rodaine]

Sorry for the delayed response (Holidays 😅)! Will get to a patch in the coming weeks, but feel free to mention that Lyft is using it.