encoder: use protojson encoder instead of encoding/json#1099
Merged
Conversation
willfindlay
force-pushed
the
pr/willfindlay/use-protojson-encoder
branch
from
June 20, 2023 18:39
8ae2290 to
0010ecc
Compare
✅ Deploy Preview for tetragon ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
willfindlay
force-pushed
the
pr/willfindlay/use-protojson-encoder
branch
3 times, most recently
from
June 26, 2023 14:39
76aee9d to
e771daf
Compare
willfindlay
marked this pull request as ready for review
June 26, 2023 16:54
willfindlay
force-pushed
the
pr/willfindlay/use-protojson-encoder
branch
2 times, most recently
from
June 27, 2023 16:53
59c4ee2 to
e760c51
Compare
The standard json encoder does not work well with protobuf messages. For example, we may experience issues during encoding/decoding such as [0]. To fix this, we need to switch over to protojson and this patch prepares for the switch by implementing a simple encoder that uses protojson.Marshal under the hood. It's not as fancy as the streaming encoder from encoding/json, but I think it's good enough to at least fix the issue for now. Subsequent patches will switch our primary JSON encoder over to this new protojson one. [0]: golang/protobuf#1433 Signed-off-by: William Findlay <will@isovalent.com>
Make the switch from json.Encoder to encoder.ProtojsonEncoder which will handle protobuf messages better and avoid buggy behaviour. Signed-off-by: William Findlay <will@isovalent.com>
The new protojson encoder requires that the event type always be a part of the protobuf. This broke rate_limit_info, so let's add it to the official API. Signed-off-by: William Findlay <will@isovalent.com>
Introduce a fuzz test to ensure that the new protojson encoder is backward compatible with the old json encoder. The test generates a random GetEventsResponse protomessage using a seed value and verifies that it can be roundtripped through both serializers and produce an equivalent result. The test comes with a seed corpus of a few randomly generated values that can be used reliably in CI. I ran this for over an hour on my local workstation without any issues. Signed-off-by: William Findlay <will@isovalent.com>
willfindlay
force-pushed
the
pr/willfindlay/use-protojson-encoder
branch
from
June 27, 2023 18:10
e760c51 to
a1a0150
Compare
jrfastab
approved these changes
Jun 27, 2023
mtardy
reviewed
Jun 28, 2023
| } { | ||
| f.Add(n) | ||
| } | ||
| f.Fuzz(func(t *testing.T, seed int64) { |
kkourt
approved these changes
Jun 28, 2023
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
4 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.
The standard json encoder does not work well with protobuf messages. For example, we may
experience issues during encoding/decoding such as [0]. To fix this, we need to switch
over to protojson. See commits.
[0]: golang/protobuf#1433