fix(core): remove custom JSON field scanner#201
Merged
SantiagoDePolonia merged 1 commit intomainfrom Apr 1, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughRefactored JSON unknown-fields extraction to replace custom byte-scanning with the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/json_fields_test.go`:
- Around line 59-72: Add a brief comment above
TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys (and/or near
extractUnknownJSONFields) stating that preserving duplicate JSON object keys is
an intentional design choice for passthrough/round-trip of provider payloads
despite JSON RFC guidance; mention that this behavior ensures raw (fields.raw)
retains duplicates and Lookup returns the first occurrence. Keep the note
concise and reference the rationale (round-tripping provider payloads) so future
maintainers know this is deliberate rather than accidental.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6ec39f31-8c8f-42d1-8676-e9d84a9ed91a
📒 Files selected for processing (2)
internal/core/json_fields.gointernal/core/json_fields_test.go
Comment on lines
+59
to
+72
| func TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys(t *testing.T) { | ||
| data := []byte(`{"known":"value","x_meta":1,"x_meta":2}`) | ||
|
|
||
| fields, err := extractUnknownJSONFields(data, "known") | ||
| if err != nil { | ||
| t.Fatalf("extractUnknownJSONFields() error = %v", err) | ||
| } | ||
| if got := string(fields.raw); got != `{"x_meta":1,"x_meta":2}` { | ||
| t.Fatalf("raw = %s, want duplicate keys preserved", got) | ||
| } | ||
| if got := fields.Lookup("x_meta"); !bytes.Equal(got, []byte("1")) { | ||
| t.Fatalf("Lookup(x_meta) = %s, want first duplicate value", got) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document that duplicate-key preservation is intentional for passthrough scenarios.
JSON with duplicate keys has undefined behavior per RFC 7159 (§4: "names within an object SHOULD be unique"). This test asserts preservation which is valuable for round-tripping provider payloads, but a brief comment explaining the design choice would help future maintainers understand this isn't accidental tolerance.
📝 Suggested documentation comment
func TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys(t *testing.T) {
+ // JSON with duplicate keys is technically non-conformant (RFC 7159 §4), but
+ // we preserve them to faithfully round-trip provider payloads that may
+ // contain such quirks. Lookup returns the first occurrence.
data := []byte(`{"known":"value","x_meta":1,"x_meta":2}`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys(t *testing.T) { | |
| data := []byte(`{"known":"value","x_meta":1,"x_meta":2}`) | |
| fields, err := extractUnknownJSONFields(data, "known") | |
| if err != nil { | |
| t.Fatalf("extractUnknownJSONFields() error = %v", err) | |
| } | |
| if got := string(fields.raw); got != `{"x_meta":1,"x_meta":2}` { | |
| t.Fatalf("raw = %s, want duplicate keys preserved", got) | |
| } | |
| if got := fields.Lookup("x_meta"); !bytes.Equal(got, []byte("1")) { | |
| t.Fatalf("Lookup(x_meta) = %s, want first duplicate value", got) | |
| } | |
| } | |
| func TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys(t *testing.T) { | |
| // JSON with duplicate keys is technically non-conformant (RFC 7159 §4), but | |
| // we preserve them to faithfully round-trip provider payloads that may | |
| // contain such quirks. Lookup returns the first occurrence. | |
| data := []byte(`{"known":"value","x_meta":1,"x_meta":2}`) | |
| fields, err := extractUnknownJSONFields(data, "known") | |
| if err != nil { | |
| t.Fatalf("extractUnknownJSONFields() error = %v", err) | |
| } | |
| if got := string(fields.raw); got != `{"x_meta":1,"x_meta":2}` { | |
| t.Fatalf("raw = %s, want duplicate keys preserved", got) | |
| } | |
| if got := fields.Lookup("x_meta"); !bytes.Equal(got, []byte("1")) { | |
| t.Fatalf("Lookup(x_meta) = %s, want first duplicate value", got) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/json_fields_test.go` around lines 59 - 72, Add a brief comment
above TestExtractUnknownJSONFields_PreservesDuplicateUnknownKeys (and/or near
extractUnknownJSONFields) stating that preserving duplicate JSON object keys is
an intentional design choice for passthrough/round-trip of provider payloads
despite JSON RFC guidance; mention that this behavior ensures raw (fields.raw)
retains duplicates and Lookup returns the first occurrence. Keep the note
concise and reference the rationale (round-tripping provider payloads) so future
maintainers know this is deliberate rather than accidental.
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
1 participant
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.
Summary
gjsoniteration and remove redundant scanner helpersTesting
Summary by CodeRabbit
Bug Fixes
Tests