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

image filtering: don't include entire enclosing message/service #1659

merged 3 commits into from Dec 14, 2022


Copy link

@jhump jhump commented Dec 8, 2022

This addresses four of the TODOs I recently added to this code.

Two of them were about eliding the contents of messages that are only present in the graph because they contain a connected type (but are otherwise unused). For these cases, the message really only serves as a namespace, so it only needs to be present as a container for other referenced types.

Another of them was to allow the caller to provide a method name and to be able to prune its enclosing service of any other unreferenced methods.

The final TODO was to improve the logic around when to include custom options. Previously, if an extension was a custom option, it would only be included if referenced by actual option declarations on elements in the graph. But now such extensions will also be included, even if excluding custom options, if the operation is including known extensions and the options types (which custom options extend) are otherwise in the graph of types. (This is implemented via the notion of "implicit" vs. "explicit" members of the transitive closure.)

Finally, this overhauls the code that trimmed a file descriptor proto to be much more concise and more DRY (via a generic function and removing the error return value, which was never needed because these operations cannot fail).

I apologize for the big diff stats, but FWIW a big majority of the new lines come from new golden files for new tests.

Fixes TCN-886

…ced; don't include all custom options if only reason options type is in closure is implicitly via any custom option; make more DRY
@jhump jhump merged commit bc1fb94 into main Dec 14, 2022
6 checks passed
@jhump jhump deleted the jh/even-leaner branch December 14, 2022 04:07
Monirul1 pushed a commit to Monirul1/buf that referenced this pull request Apr 30, 2023

If an enclosing message is not part of the transitive dependency graph,
but a nested message therein is, the enclosing message will be stripped
of its fields. This produces a smaller filtered descriptor set but does not
impede the use of the results for dynamic messages or dynamic RPC.

Similarly, if a method is indicated as a type filter, its enclosing service
will be stripped of other unreferenced methods.

Finally, this attempts to fix/clarify the behavior around when custom
options are included since they are also known extensions. If known
extensions are included in the filtered set AND one of the options
message types is part of the transitive dependency graph, all of the
relevant custom options will be included. Otherwise, if custom
options are included, ones actually referenced in options on the
elements in the transitive dependency graph will be present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants