Merged
Conversation
WalkthroughRefactors decoder error handling to use PathBuilder-based wrapping and Prepend* APIs, adds UTF-8 validation for struct tags that causes decode to error, introduces ReaderOption for reader configuration, and tightens bounds checks in verifier and pointer decoding constants. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
oschwald
force-pushed
the
greg/cleanups
branch
from
8d515e6 to
23381f5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/decoder/reflection.go`:
- Around line 950-959: The current check uses utf8.ValidString on the entire
rawTag which can misattribute invalid UTF-8 from unrelated struct tags to the
maxminddb tag; modify the logic so you either remove the rawTag utf8 check
entirely (since validateTag(field, tag) already verifies the extracted value)
or, if you want an explicit pre-check, extract the maxminddb tag first via
field.Tag.Get("maxminddb") and run utf8.ValidString only on that extracted tag
string before calling validateTag; update the conditional that sets
validationErr (current use of rawTag and validationErr) to reference the
extracted tag value or remove it and rely on validateTag.
In `@verifier_test.go`:
- Around line 75-79: Inside the require.NotPanics callback in the test that
calls v.verifyDataSectionSeparator(), replace the inner require.Error call with
assert.Error (and keep assert.ErrorContains) so no require (which calls
t.FailNow/Goexit) runs inside the NotPanics closure; update the assertion on the
error returned from v.verifyDataSectionSeparator() to use assert.Error(t, err)
and keep assert.ErrorContains(t, err, "unexpected end of database") to avoid
interfering with testify's recover handling.
23381f5 to
201dd7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/decoder/reflection.go`:
- Around line 1054-1073: Add a brief clarifying comment near the call site of
validateRawMaxMindDBTagValue (around the validateTag usage) explaining that
validateRawMaxMindDBTagValue manually parses the raw struct tag to catch invalid
UTF-8 cases that reflect.StructTag.Get may miss, and that this raw parsing is a
defensive, layered validation used alongside validateTag (which handles the
common/normalized case); mention both function names
(validateRawMaxMindDBTagValue and validateTag) so future readers understand the
ordering and purpose of the two checks.
Comment on lines
+1054
to
+1073
| func validateRawMaxMindDBTagValue(field reflect.StructField, rawTag string) error { | ||
| const key = `maxminddb:"` | ||
|
|
||
| start := strings.Index(rawTag, key) | ||
| if start == -1 { | ||
| return nil | ||
| } | ||
|
|
||
| start += len(key) | ||
| end := strings.IndexByte(rawTag[start:], '"') | ||
| if end == -1 { | ||
| return nil | ||
| } | ||
|
|
||
| if !utf8.ValidString(rawTag[start : start+end]) { | ||
| return invalidMaxMindDBTagError(field.Name) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
validateRawMaxMindDBTagValue manually parses the raw tag — consider documenting why this is needed alongside validateTag.
This function addresses the edge case where reflect.StructTag.Get() may not correctly extract a tag value containing invalid UTF-8. The manual parsing provides defense-in-depth, but the relationship between this function and validateTag (lines 850–860) isn't immediately obvious to a reader.
A brief comment on lines 951–956 explaining the layered validation (raw parse for robustness + Tag.Get for the common case) would improve maintainability.
🤖 Prompt for AI Agents
In `@internal/decoder/reflection.go` around lines 1054 - 1073, Add a brief
clarifying comment near the call site of validateRawMaxMindDBTagValue (around
the validateTag usage) explaining that validateRawMaxMindDBTagValue manually
parses the raw struct tag to catch invalid UTF-8 cases that
reflect.StructTag.Get may miss, and that this raw parsing is a defensive,
layered validation used alongside validateTag (which handles the
common/normalized case); mention both function names
(validateRawMaxMindDBTagValue and validateTag) so future readers understand the
ordering and purpose of the two checks.
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 by CodeRabbit
Bug Fixes
Improvements