Skip to content

Commit

Permalink
Fix handling of extensions
Browse files Browse the repository at this point in the history
  • Loading branch information
emcfarlane committed Feb 25, 2025
1 parent 4935430 commit 400f958
Show file tree
Hide file tree
Showing 8 changed files with 176 additions and 171 deletions.
2 changes: 1 addition & 1 deletion private/buf/bufctl/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -1332,7 +1332,7 @@ func filterImage(
newImage = bufimage.ImageWithoutImports(newImage)
}
if len(functionOptions.imageTypes) > 0 {
newImage, err = bufimageutil.ImageFilteredByTypes(newImage, functionOptions.imageTypes...)
newImage, err = bufimageutil.FilterImage(newImage, bufimageutil.WithIncludeTypes(functionOptions.imageTypes...))
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion private/buf/bufgen/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,7 @@ func (g *generator) execLocalPlugin(
}
if len(excludeOptions) > 0 {
for i, pluginImage := range pluginImages {
pluginImage, err := bufimageutil.ExcludeOptions(pluginImage, excludeOptions...)
pluginImage, err := bufimageutil.FilterImage(pluginImage, bufimageutil.WithExcludeOptions(excludeOptions...))
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions private/buf/cmd/buf/command/convert/convert.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ func run(
resolveWellKnownType = true
}
if schemaImage != nil {
_, filterErr := bufimageutil.ImageFilteredByTypes(schemaImage, flags.Type)
_, filterErr := bufimageutil.FilterImage(schemaImage, bufimageutil.WithIncludeTypes(flags.Type))
if errors.Is(filterErr, bufimageutil.ErrImageFilterTypeNotFound) {
resolveWellKnownType = true
}
Expand Down Expand Up @@ -283,5 +283,5 @@ func wellKnownTypeImage(
if err != nil {
return nil, err
}
return bufimageutil.ImageFilteredByTypes(image, wellKnownTypeName)
return bufimageutil.FilterImage(image, bufimageutil.WithIncludeTypes(wellKnownTypeName))
}
33 changes: 15 additions & 18 deletions private/bufpkg/bufimage/bufimageutil/bufimageutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,22 @@ func WithExcludeOptions(typeNames ...string) ImageFilterOption {
}
}

// ImageFilteredByTypes returns a minimal image containing only the descriptors
// required to define those types. The resulting contains only files in which
// those descriptors and their transitive closure of required descriptors, with
// each file only contains the minimal required types and imports.
// FilterImage returns a minimal image containing only the descriptors
// required to define the set of types provided by the filter options. If no
// filter options are provided, the original image is returned.
//
// Although this returns a new [bufimage.Image], it mutates the original image's
// underlying file's [descriptorpb.FileDescriptorProto]. So the old image should
// not continue to be used.
// The filtered image will contain only the files that contain the definitions of
// the specified types, and their transitive dependencies. If a file is no longer
// required, it will be removed from the image. Only the minimal set of types
// required to define the specified types will be included in the filtered image.
//
// Excluded types and options are not included in the filtered image. If an
// included type transitively depens on the excluded type, the descriptor will
// be altered to remove the dependency.
//
// This returns a new [bufimage.Image] that is a shallow copy of the underlying
// [descriptorpb.FileDescriptorProto]s of the original. The new image may therefore
// share state with the original image, so it should not be modified.
//
// A descriptor is said to require another descriptor if the dependent
// descriptor is needed to accurately and completely describe that descriptor.
Expand Down Expand Up @@ -212,10 +220,6 @@ func WithExcludeOptions(typeNames ...string) ImageFilterOption {
// files: [foo.proto, bar.proto]
// messages: [pkg.Baz, other.Quux, other.Qux]
// extensions: [other.my_option]
func ImageFilteredByTypes(image bufimage.Image, types ...string) (bufimage.Image, error) {
return ImageFilteredByTypesWithOptions(image, types)
}

func FilterImage(image bufimage.Image, options ...ImageFilterOption) (bufimage.Image, error) {
if len(options) == 0 {
return image, nil
Expand All @@ -227,13 +231,6 @@ func FilterImage(image bufimage.Image, options ...ImageFilterOption) (bufimage.I
return filterImage(image, filterOptions)
}

// ImageFilteredByTypesWithOptions returns a minimal image containing only the descriptors
// required to define those types. See ImageFilteredByTypes for more details. This version
// allows for customizing the behavior with options.
func ImageFilteredByTypesWithOptions(image bufimage.Image, types []string, opts ...ImageFilterOption) (bufimage.Image, error) {
return FilterImage(image, append(opts, WithIncludeTypes(types...))...)
}

// StripSourceRetentionOptions strips any options with a retention of "source" from
// the descriptors in the given image. The image is not mutated but instead a new
// image is returned. The returned image may share state with the original.
Expand Down
14 changes: 7 additions & 7 deletions private/bufpkg/bufimage/bufimageutil/bufimageutil_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func TestTransitivePublic(t *testing.T) {
)
require.NoError(t, err)

filteredImage, err := ImageFilteredByTypes(image, "c.Baz")
filteredImage, err := FilterImage(image, WithIncludeTypes("c.Baz"))
require.NoError(t, err)

_, err = protodesc.NewFiles(bufimage.ImageToFileDescriptorSet(filteredImage))
Expand Down Expand Up @@ -220,15 +220,15 @@ func TestTypesFromMainModule(t *testing.T) {
bProtoFileInfo, err := dep.StatFileInfo(ctx, "b.proto")
require.NoError(t, err)
require.False(t, bProtoFileInfo.IsTargetFile())
_, err = ImageFilteredByTypes(image, "dependency.Dep")
_, err = FilterImage(image, WithIncludeTypes("dependency.Dep"))
require.Error(t, err)
assert.ErrorIs(t, err, ErrImageFilterTypeIsImport)

// allowed if we specify option
_, err = ImageFilteredByTypesWithOptions(image, []string{"dependency.Dep"}, WithAllowFilterByImportedType())
_, err = FilterImage(image, WithIncludeTypes("dependency.Dep"), WithAllowFilterByImportedType())
require.NoError(t, err)

_, err = ImageFilteredByTypes(image, "nonexisting")
_, err = FilterImage(image, WithIncludeTypes("nonexisting"))
require.Error(t, err)
assert.ErrorIs(t, err, ErrImageFilterTypeNotFound)
}
Expand Down Expand Up @@ -259,7 +259,7 @@ func runDiffTest(t *testing.T, testdataDir string, typenames []string, expectedF
bucket, image, err := getImage(ctx, slogtestext.NewLogger(t), testdataDir, bufimage.WithExcludeSourceCodeInfo())
require.NoError(t, err)

filteredImage, err := ImageFilteredByTypesWithOptions(image, typenames, opts...)
filteredImage, err := FilterImage(image, append(opts, WithIncludeTypes(typenames...))...)
require.NoError(t, err)
assert.NotNil(t, image)
assert.True(t, imageIsDependencyOrdered(filteredImage), "image files not in dependency order")
Expand Down Expand Up @@ -323,7 +323,7 @@ func runSourceCodeInfoTest(t *testing.T, typename string, expectedFile string, o
bucket, image, err := getImage(ctx, slogtestext.NewLogger(t), "testdata/sourcecodeinfo")
require.NoError(t, err)

filteredImage, err := ImageFilteredByTypesWithOptions(image, []string{typename}, opts...)
filteredImage, err := FilterImage(image, append(opts, WithIncludeTypes(typename))...)
require.NoError(t, err)

imageFile := filteredImage.GetFile("test.proto")
Expand Down Expand Up @@ -477,7 +477,7 @@ func benchmarkFilterImage(b *testing.B, opts ...bufimage.BuildImageOption) {
require.NoError(b, err)
b.StartTimer()

_, err = ImageFilteredByTypes(image, typeName)
_, err = FilterImage(image, WithIncludeTypes(typeName))
require.NoError(b, err)
i++
if i == b.N {
Expand Down
Loading

0 comments on commit 400f958

Please sign in to comment.