Skip to content

Add type exclusion filters for buf generate#3624

Merged
emcfarlane merged 83 commits intomainfrom
ed/excludeOptions
Mar 28, 2025
Merged

Add type exclusion filters for buf generate#3624
emcfarlane merged 83 commits intomainfrom
ed/excludeOptions

Conversation

@emcfarlane
Copy link
Contributor

@emcfarlane emcfarlane commented Feb 6, 2025

This improves filtering of types for buf generate. A new parameter exclude_types is added to buf.gen.yaml. Type filters can now be applied to both the input and plugin. The flag --exclude-type overrides the input filters for inputs configured in buf.gen.yaml.

For example to filter the buf.validate types from the plugin protoc-gen-es you can now set the following exclude_types configs on the plugin:

 version: v2
 clean: true
 plugins:
   - local: protoc-gen-es
     opt: target=ts
     out: gen
     include_imports: true
+    types:
+      - acme.v1.example.Book
+    exclude_types:
+      - buf.validate
 inputs:
   - directory: .
     types:
       - acme.v1.example.Get
+    exclude_types:
+      - acme.v1.example.DebugLog

The input filter types are applied to the image before the plugin filter types are applied. If any types are missing a not found error is returned.

The image filter functions ImageFilteredByTypes and ImageFilteredByTypesWithOptions has been replaced by the single function FilterImage which takes the image and a set of options. Options include WithIncludeTypes and WithExcludeTypes. The previous behaviour is replicated by setting the types in WithIncludeTypes.

The filter image will, by default, do a shallow copy. This allows for sharing of the original image to multiple filters. To avoid the overhead of copying an option WithMutateInPlace can be set to mutate in place.

BenchmarkFilterImage_WithSourceCodeInfo-8 (current)      15538             77970 ns/op           42243 B/op        957 allocs/op
BenchmarkFilterImage_WithSourceCodeInfo-8 (copy)          7560            165533 ns/op          118865 B/op       2345 allocs/op
BenchmarkFilterImage_WithSourceCodeInfo-8 (mutate)       25021             47970 ns/op           41541 B/op        531 allocs/op

Fixes #645

This adds a new plugin option `exclude_options` for use on generation.
The excluded options are stripped from the Image creating a shallow
clone of the FileDescriptors that make up the ImageFiles. Any imports
that are no longer required due to options being removed are also
removed. The option is applied for the plugin. Multiple filtered images
that reference the original image may be created when exclude options
are set.
@emcfarlane emcfarlane requested review from doriable and jhump February 6, 2025 20:50
@emcfarlane emcfarlane marked this pull request as draft February 6, 2025 20:50
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedMar 28, 2025, 9:59 AM

Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a "types" config, allowing a filter of what types to include. We made it a struct with an "includes" field with the intention of perhaps adding "excludes" in the future.

So maybe we should that there and then allow the "types" to be specified per plugin, not just at the top-level. (May have to think about how the two interact, if types are specified at both the top-level and for a particular plugin: do they merge or does the plugin definition override the top-level one).

This would be more consistent and also be more expressive: one could strip any kind of element from the image, not just options. Also, there is already logic in the type filtering code to handle auto-pruning imports based on what's left. The main trick with excluding arbitrary elements is that, if an enum or a message is indicated, we may need to mutate other messages to remove fields that refer to that enum or message.

@emcfarlane
Copy link
Contributor Author

emcfarlane commented Feb 10, 2025

I think the approaches could be merged to a general filter too. The difference was required to get the non mutating behaviour. We want this filter applied at the plugin level so to avoid mutating the Image for every plugin option it only does a shallow copy as needed. Which is similar to the approach taken for source retention options in protopluginutil. There was also the performance case of excluding vs including on small set of types. The current walks from the set of types, but that would be all the types in the image for the option exclusions.

Storing the deletes as a trie I think we can get a performant approach for both use cases. When walking the file descriptors we can mark nodes for inclusion in the image on entry, then on the exit visit if any child node is required we can mark those excluded nodes as enclosing, updating the trie on the exit.

Edit: actually the mark on exit won't work. Will have to add nodes using the transitive closure. Will try merge the solutions.

@emcfarlane emcfarlane changed the title Add exclude_options configuration for generation Add support for filtering of options for generate Feb 24, 2025
@emcfarlane emcfarlane changed the title Add support for filtering of options for generate Add exclude_options configuration to filter options for generate Feb 24, 2025
@emcfarlane emcfarlane requested a review from jhump March 24, 2025 13:16
numFieldsToKeep := 0
options.Range(func(fd protoreflect.FieldDescriptor, val protoreflect.Value) bool {
if !b.closure.hasOption(fd, b.imageIndex, b.options) {
// Remove this option.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that last commit, I think it's safe to simply remove remapOptions entirely.

Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for bearing with me and making so many requested changes. And sorry it took so long to get through.

Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

protoImage, err := bufimage.ImageToProtoImage(filteredImage)
require.NoError(t, err)
// Clone here as `bufimage.NewImageForProto` mutates protoImage.
protoImage, _ = proto.Clone(protoImage).(*imagev1.Image) // Safe to assert.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update to protobuf-go v1.36.6: they finally added a generic proto.CloneOf[M]!
https://go-review.googlesource.com/c/protobuf/+/653536/4/proto/merge.go

(Not an action for this branch. I know there are likely lots of calls to proto.Clone that we could clean up... Just an FYI really 😄)

@emcfarlane emcfarlane merged commit 348b655 into main Mar 28, 2025
10 checks passed
@emcfarlane emcfarlane deleted the ed/excludeOptions branch March 28, 2025 13:39
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

Successfully merging this pull request may close these issues.

Remove options in Managed Mode

3 participants