Support recursive glob (.**) in bufimageutil type filters#4571
Conversation
A trailing ".**" on an include or exclude type name is a recursive glob that matches the named element and every symbol nested beneath it: a package and all its sub-packages and types, or a message and all its nested types. Without the glob, behavior is unchanged: a non-recursive package include or exclude affects only that package's own types, not its sub-packages.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
|
Wdyt about adding a test where the globbed package has no files directly in it? Something like
And With versioned packages this is the most common shape and nobody puts files directly in acme.order, so the glob target has zero files of its own and everything lives in sub-packages. afaics all packages in testdata have their own files, so this path is never exercised (which is the one we care about). |
| } | ||
| if !options.allowImportedTypes { | ||
| // if package includes only imported files, then reject | ||
| // If the package contains only imported files, then reject. For a |
There was a problem hiding this comment.
Do we need/want improved error messages to cover this new functionality? E.g.,
Say we have something like
$ ls proto/
acme/order/v1/order.proto # package acme.order.v1And the user runs
$ buf build --type acme.order
Failure: inclusion of type "acme.order": type declared in imported moduleWould a guard before the if onlyImported be useful, and we could surface something like:
Failure: package "acme.order" contains no types; did you mean "acme.order.**"?
Otherwise that original error is a bit misleading.
| for includeType := range options.includeTypes { | ||
| includeType := protoreflect.FullName(includeType) | ||
| if err := closure.includeType(includeType, imageIndex, options); err != nil { | ||
| includeType, recursive := strings.CutSuffix(includeType, ".**") |
There was a problem hiding this comment.
Should we be a bit more defensive here to prevent things like "foo.**.bar" or ".**" and surface a more meaningful error?
Right now "foo.**.bar" falls through to a literal lookup and produces a confusing not found, and ".**" cuts to an empty name that matches the root package entry, so it silently includes (or excludes) the entire image.The latter is prob unlikely, but the former I could see tripping someone up.
includeType, recursive := strings.CutSuffix(includeType, ".**")
if !protoreflect.FullName(includeType).IsValid() {
return nil, fmt.Errorf("invalid type name %q", includeType)
}| if !isTopLevel { | ||
| continue | ||
| } | ||
| return fmt.Errorf("inclusion of excluded package %q", pkg.fullName) |
There was a problem hiding this comment.
Do we want a sentinel error to match on, similar to ErrImageFilterTypeNotFound, but something like ErrImageFilterTypeExcluded.
That way the caller of this package (assume an API) can surface this as an invalid argument error for contradictory filters. There's probably other places too of this kind of cateogry.
emcfarlane
left a comment
emcfarlane
left a comment
There was a problem hiding this comment.
Maybe glob of a.** where a.b is excluded and a is empty should error. And we should maybe provide a better error on something like foo.**.bar where a user tries to use an unsupported glob syntax. Do we need to doc this change in the --type strings flags?
|
|
||
| // includeChildElements recursively adds the symbols nested beneath the given | ||
| // element to the closure. Only messages have such children: their nested | ||
| // messages and enums. Other element kinds have nothing to expand. |
There was a problem hiding this comment.
I think extensions are missing here. A services methods and a enums values are always included.
There was a problem hiding this comment.
Yeah, good catch. I thought about this when creating this branch and somehow convinced myself it wasn't necessary. But you're right that it is in fact needed to be correct.
3 participants
A trailing ".**" on an include or exclude type name now matches the named element and every symbol nested beneath it: a package and all its sub-packages and types, or a message and all its nested types.
(Note that exclusion was already recursive if the root type was a message that contain nested types: we can't realistically keep the nested types if we remove their container.)
This addresses a long-standing TODO since the question of whether including a type should include its children/descendants definitely came up when this functionality was first implemented.