image filtering: don't include entire enclosing message/service #1659
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
commented
Dec 13, 2022
robbertvanginkel
approved these changes
Dec 13, 2022
Monirul1
pushed a commit
to Monirul1/buf
that referenced
this pull request
Apr 30, 2023
…uild#1659) 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
errorreturn 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