Skip to content

Reject non-string JSON values when decoding IDs - #125

Merged
rs merged 1 commit into
rs:masterfrom
vitalivo:fix/json-value-type
Sep 12, 2026
Merged

rs merged 1 commit into
rs:masterfrom
vitalivo:fix/json-value-type

Conversation

@vitalivo

Copy link
Copy Markdown
Contributor

This replaces #124, which was accidentally closed and its source fork deleted. The implementation is unchanged; the original discussion and reviews remain linked there.


json.Unmarshal accepts certain numbers and arrays as IDs because UnmarshalJSON strips the first and last byte without checking for quotes. For example, both 1111111111111111111101 and [11111111111111111110] decode successfully to the ID 11111111111111111110.

Require a JSON string before delegating to UnmarshalText, retaining the existing null handling and short-input guard. Regression tests exercise both valid JSON inputs through encoding/json and verify that rejection leaves the receiver unchanged.

Validation: both cases fail before the fix. go test -race -cover ./... passes (94.3% main-package coverage), go vet ./... passes, and staticcheck passes with tests disabled. Full staticcheck reports the pre-existing unused error assignment in id_test.go:153.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No unresolved blocking issues were identified.

Pull request overview

Updates JSON ID decoding to reject non-string values while preserving null handling.

Changes:

  • Validate JSON string delimiters before decoding.
  • Add regression tests for numeric and array inputs.
File summaries
File Description
id.go Enforces JSON string input for IDs.
id_test.go Tests rejection of invalid JSON values.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@rs
rs merged commit 34d3905 into rs:master Sep 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants