-
Notifications
You must be signed in to change notification settings - Fork 24
feat: Migrate to encoding/json/v2 #342
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates the codebase from the standard encoding/json package to the experimental encoding/json/v2 package, which is enabled via the GOEXPERIMENT=jsonv2 flag in Go 1.25.
Changes:
- Updated all JSON import statements from
encoding/jsontoencoding/json/v2across 14 files - Updated Go version from 1.24 to 1.25 in go.mod, README.md, and .golangci.yml
- Modified build and test commands to include
GOEXPERIMENT=jsonv2flag - Updated test expectations to reflect json/v2 behavior differences (error messages, null vs empty array serialization)
- Refactored
IsPingbackRequestto use streaming token parsing withencoding/json/jsontext - Changed JSON struct tag from
omitemptytoomitzerofor integer fields - Updated test JSON data to include
@osfield (appears to be a protocol alignment fix) - Added Docker build cache optimization
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| go.mod | Updated Go version to 1.25.0 |
| README.md | Updated Go dependency documentation to 1.25 |
| .golangci.yml | Updated linter Go version to 1.25 |
| Makefile | Added GOEXPERIMENT=jsonv2 flag to build and test commands |
| Dockerfile | Added build cache mount and uses golang:1.25 base image |
| omaha/protocol/protocol.go | Refactored IsPingbackRequest with streaming parser, added jsontext import |
| omaha/protocol/protocol_test.go | Added comprehensive tests for IsPingbackRequest function |
| omaha/v3/protocol.go | Updated json import to v2 |
| omaha/v3/protocol_test.go | Updated json import to v2 |
| omaha/v3/request.go | Updated json import to v2 |
| omaha/v3/request_test.go | Updated json import to v2, updated test data with @os field |
| omaha/v3/response.go | Updated json import to v2, changed SizeDiff tag to omitzero |
| omaha/v3/response_test.go | Updated json import to v2 |
| omaha/v4/protocol.go | Updated json import to v2 |
| omaha/v4/protocol_test.go | Updated json import to v2 |
| omaha/v4/request.go | Updated json import to v2 |
| omaha/v4/request_test.go | Updated json import to v2, updated test data with @os field |
| omaha/v4/response.go | Updated json import to v2 |
| omaha/v4/response_test.go | Updated json import to v2 |
| extension/extension.go | Updated json import to v2 |
| extension/extensiontest/extensiontest.go | Updated test JSON strings with @os field |
| server/server_test.go | Updated json import, test expectations for error messages and empty arrays, changed assert.Nil to assert.Empty |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if events array has content (length > 2 means more than just "[]") | ||
| if len(val) > 2 { |
Copilot
AI
Jan 22, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IsPingbackRequest function only checks the length of the value after finding an "events" key, but doesn't validate that the value is actually an array. If the JSON contains "events":"some-string" or "events":123, the function would incorrectly return true as long as the value's byte representation is longer than 2 characters.
Consider adding validation to ensure the value starts with '[' to confirm it's an array before checking the length, or using a more robust check that the value is a non-empty array.
| // Check if events array has content (length > 2 means more than just "[]") | |
| if len(val) > 2 { | |
| // Check that the "events" value is a non-empty JSON array. | |
| trimmed := bytes.TrimSpace(val) | |
| if len(trimmed) > 2 && trimmed[0] == '[' && trimmed[len(trimmed)-1] == ']' { |
c357887 to
0df3435
Compare
- Improved the `IsPingbackRequest` function to utilize streaming token parsing for better performance - Added tests for the `IsPingbackRequest` function
a44d197 to
3f970a5
Compare
No description provided.