Print count of unrecognized bytes in "buf curl"#2586
Merged
Conversation
…t of unrecognized bytes in responses with -v
bufdev
approved these changes
Nov 14, 2023
Member
bufdev
left a comment
There was a problem hiding this comment.
These wrappers give us a common way to do (and enforce) additional behavior on top of the main libs, such as json.compact (to get around the non-determinism problem), and if I remember, reparsing for custom options
Member
Author
|
@bufdev, ack. I forgot about protojson's non-deterministic output 😭. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Currently,
buf curltranscodes the response from the binary format to JSON. However, if the response includes any unrecognized fields, they are just discarded since JSON does not emit unrecognized field data.So this provides a cue to user's that such data might be in the response and getting dropped (which suggests an incomplete or out-of-date schema) by logging the presence of unrecognized bytes when the
-vflag is used.While in here, I noticed that this was allowing unrecognized fields in the JSON request provided to
buf curl, which seems wrong since a likely issue with formulating a request will be a typo in a field name, which would just be ignored instead of validated. I was tempted to fix this by simply usingprotojsondirectly. I don't really understand the value ofprotoencodinghaving to reproduce the same functionality with a different API. But I suspected you would object to that, so I instead addressed a TODO in here.But it does beg the question in mind: why have these
protoencodingwrappers? Why not directly useprotoandprotojsonfor serialization?