Add more thorough checks for when to adapt a value when resolving custom feature #306
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
…alue to right descriptor
|
Welp, let's hold off on review for now... This is not quite working as desired (broken tests in bufbuild/buf#2967). I'll mark as ready for review after I get time to do more digging/troubleshooting/fixing. |
|
I was able to step through the code to see where it was breaking. I added a test to cover that logic (which failed) and then added a fix. See the later two commits. I also tested with |
|
@emcfarlane, ping. |
emcfarlane
approved these changes
May 21, 2024
emcfarlane
reviewed
May 21, 2024
kralicky
pushed a commit
to kralicky/protocompile
that referenced
this pull request
Jun 19, 2024
…tom feature (bufbuild#306) Previously, the protoutil.ResolveCustomFeature code was unable to reliably handle the following case: 1. The descriptor includes dynamic extensions for all custom options and custom features. 2. The user supplies a _generated_ extension type, to query for the custom feature. The code here _ostensibly_ handles this. But the cases where it decides it needs to adapt the value (by marshalling and then unmarshalling into different type) isn't quite thorough enough. The issue is when the custom feature is a _message_. In that case, it works fine to query for the extension itself. But if we then need to extract a field from the message, the field descriptor is likely referring to a generated message type, whose descriptor is an instance in `protoregistry.GlobalTypes`. But the value is likely to be a dynamic message, whose descriptor may have been constructed dynamically (such as being parsed from source). In that case, the `protoreflect` stuff will happily panic when it sees the mismatch 😱. This commit fixes this issue so that custom feature values can be successfully queried under these conditions, without panic. (cherry picked from commit 85801e4)
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.
As is, this code is unable to handle the following case:
The code here ostensibly handles this. But the cases where it decides it needs to adapt the value (by marshalling and then unmarshalling into different type) isn't quite thorough enough. The issue is when the custom feature is a message. In that case, it works fine to query for the extension itself. But if we then need to extract a field from the message, the field descriptor is likely referring to a generated message type, whose descriptor is an instance in
protoregistry.GlobalTypes. But the value is likely to be a dynamic message, whose descriptor may have been constructed dynamically (such as being parsed from source). In that case, theprotoreflectstuff will happily panic when it sees the mismatch 😱.Up to now, I had worked around this in a rather gross way: in the "buf breaking" implementation where I need to resolve custom features, I had a bunch of code that looks very similar to this stuff, to re-parse the features (here).
But if I simply beef up this function to do a slightly more thorough check for this case, then I can remove a bunch of that redundant (and less efficient) code from the "buf breaking" implementation.