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

Remove options in Managed Mode #645

Closed
amckinney opened this issue Oct 8, 2021 · 10 comments · Fixed by #3624
Closed

Remove options in Managed Mode #645

amckinney opened this issue Oct 8, 2021 · 10 comments · Fixed by #3624
Assignees
Labels
Feature New feature or request

Comments

@amckinney
Copy link
Contributor

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.

@amckinney amckinney added Feature New feature or request P2 labels Oct 8, 2021
@bufdev
Copy link
Member

bufdev commented Oct 8, 2021

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
Copy link
Contributor Author

amckinney commented Mar 21, 2022

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
Copy link
Contributor Author

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.

@bufdev bufdev removed the P2 label Feb 2, 2023
@rauanmayemir
Copy link

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
Copy link

bhollis commented Apr 26, 2024

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
Copy link
Member

bufdev commented Jun 13, 2024

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
Copy link
Contributor

emcfarlane commented Mar 28, 2025

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
Copy link

bhollis commented Mar 29, 2025

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.

@bhollis
Copy link

bhollis commented Mar 29, 2025

I also sometimes get this when using exclude_types as a plugin option:

Failure: imported file "buf/validate/validate.proto" was filtered out

I can get everything to build with --exclude-type buf.validate as a command line arg, but unfortunately "buf.validate" options and file imports are still present, so it doesn't appear to do anything. I think we have to stick with our current solution which is to pre-process a separate copy of all our proto source files with buf validate stripped out.

@emcfarlane
Copy link
Contributor

emcfarlane commented Mar 29, 2025

Hi @bhollis thanks for reporting. The first error (exclusion of type "buf.validate": not found) is expected. This is to help users manage their config and be sure that no invalid type names are used.

The second error (imported file "buf/validate/validate.proto" was filtered out) I believe is fixed in #3717. This was an oversight in how includes are set when no includes are provided. Currently the full list of target files for the image is used. So depending on your input if buf.validate is declared as a target or import this would error. The default buf.yaml input (.) usecase should work, where buf.build/bufbuild/protovalidate is a dependency in the workspace.

Looking into the --exclude-type issue. It may be related to the above targeting issue. I'll sync up with you on slack to get some examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants