Remove options in Managed Mode

[amckinney]

We recently had a feature request related to PHP code generation and protoc-gen-validate. In summary, PHP can't generate code for proto2 files, so anything that depends protoc-gen-validate will not compile.

We want users to incorporate protoc-gen-validate as they please, but it's really only relevant when generating the server implementation in the user's language of choice (go in this case).

We've discussed managing .proto options in general (not just file-level options), so this is a good first candidate to explore that space. In this case, we would need to remove all instances of a particular set of options (those provided by validate.rules) and we would need to remove the import validate/validate.proto; declaration so that the PHP stubs don't initialize the initOnce hook.

[bufdev]

For future reference, this is roughly what we want:

diff --git a/data/template/buf.go-client.gen.yaml b/data/template/buf.go-client.gen.yaml
index 68b84740..7b522a82 100644
--- a/data/template/buf.go-client.gen.yaml
+++ b/data/template/buf.go-client.gen.yaml
@@ -5,6 +5,12 @@ managed:
   enabled: true
   go_package_prefix:
     default: github.com/bufbuild/buf/private/gen/proto/go
+  options:
+    exclude:
+      - (validate.rules)
+  imports:
+    exclude:
+      - validate/validate.proto
 plugins:
   - name: go-grpc
     out: private/gen/proto/go

[amckinney]

At a high level, the proposal is fairly easy to implement - we can modify the Image just like the rest of the other Managed Mode modifiers, and strip each of the options/imports from the Image. Unfortunately, this means that the user could modify the Image in a way that makes it no longer compile.

For example, suppose that I strip the google/protobuf/descriptor.proto file from the buf.build/bufbuild/buf module. If I use this feature, the protoc-gen-go plugin yields the following (from the root of this repository):

$ buf generate proto --template data/template/buf.go.gen.yaml
protoc-gen-go: invalid FileDescriptorProto "buf/alpha/image/v1/image.proto": proto: message field "buf.alpha.image.v1.ImageFile.message_type" cannot resolve type: "google.protobuf.DescriptorProto" not found

We could consider this a user error and justify that the feature is working as expected, but it's still not an ideal user experience - the plugins could now receive a malformed Image from buf (which was previously a guarantee).

Removing options is also strange - we would need to clear the extension from the Image. This means we need to iterate over and interact with the relevant unknown fields (like we do in protosource, or something else along those lines). We would only affect the plugins that look for them (as intended). However, that still doesn't get us everything we want - the import statement would still remain which ends up affecting specific plugins. For example, protoc-gen-go will execute init hooks for unused imports unless the import is weak (link).

In order to satisfy both of these requirements, we'd need to verify that the Image compiles after we're applied the Managed Mode modifiers and return an error if it doesn't (before any of the plugins are invoked). This suggests a specific modifier ordering - should the options and imports be removed before any of the other standard modifiers are applied, or after? It probably makes sense to apply them before, but it still means we need to compile the image twice: once before it is modified, and again after it's been modified. With that said, we'd only need to re-compile the Image if certain modifiers were enabled (the import statement modifier in this case).

---

We should consider whether or not the complexity in this feature is worth it. There's already a fair amount of learning required for users to understand Managed Mode, and this adds even more questions into the mix. I'm not so worried about the implementation in this case - I'm only concerned about the UX.

[amckinney]

Another thing to keep in mind here -

We're beginning to see similar functionality appear in different parts of buf. Now that we have partial images with the --type flag, there's an argument that this kind of behavior would be there instead of in Managed Mode. The two features act similarly - they both modify an Image in some way. We should be careful with introducing functionality to one and not the other, or introducing this functionality to both places and have multiple ways to do the same thing.

Similarly, this might be a feature that we want to implement on the AST before it's compiled into an Image (kinda similar to a buf format-esque approach). The user might actually want to modify the source itself so it can be published in two different places, where one version contains the options and another doesn't.

TL;DR We'll need to think about this one a bit more to decide where it should go.

[rauanmayemir]

Another case where this feature would shine is docs and api spec generators like gnostic. You wouldn't need them in the language-specific runtime.

[bhollis]

I found this issue while looking to file a similar request. Like in the original comment, we too would like to eliminate all protovalidate options and imports when generating code for our clients, while retaining them when generating code for our server.

This is to help reduce the code size of our clients - for some languages like Ruby, the entire descriptor is emitted into the generated code, even though Ruby can't make use of the protovalidate options. In this case, validate_pb.rb is 63KB.

I'd be happy for this to either be a transform on the AST, or on the Image. In the meantime we may end up implementing an AST-level transformer that pre-processes our .proto files.

[bufdev]

A few thoughts here:

• I think we'd actually want to make this a per-plugin option in the new v2 of buf.gen.yaml. Same level as include_imports and include_wkt.
• We would need to be smart enough to only require the user to specify exclude_options. If the removal of these options results in the imports being unused, we remove the imports as well.
• This is going to be a pretty difficult one to implement, but I'd like if we do it at some point.

[emcfarlane]

This has been release in v1.51.0. Types (any package, message, enum, extension, service or method name) can now be excluded at the input or per plugin in your buf.gen.yaml. As an example to remove all protovalidate options you may specify the package buf.validate:

version:v2
plugins:
  - # ...
    exclude_types:
      - buf.validate
inputs:
  - # ...
    exclude_types:
      - buf.validate

[bhollis]

Can you link to some more comprehensive documentation/examples for this new option? I tried adding this as an argument to my plugins in buf.gen.yaml and I get:

Failure: exclusion of type "buf.validate": not found

Edit: I think this fails when the type isn't there (because in our case, we'd already stripped it manually). I would expect this to no-op, not error.