Add case-insensitive map key merge option - #310
Open
AgentGoose32 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a new configuration option Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
issue256_test.go (1)
10-50: Add regression coverage forWithCaseInsensitiveMapKeys+WithOverwriteWithEmptyValue.Current tests cover override/default behavior well, but not the empty-overwrite cleanup path that should also honor case-insensitive matching. Adding this case will lock the intended behavior.
🧪 Suggested test
+func TestIssue256CaseInsensitiveMapKeysWithOverwriteEmptyValue(t *testing.T) { + dst := map[string]string{"Host": "localhost"} + src := map[string]string{"host": "example.com"} + + if err := mergo.Merge(&dst, src, mergo.WithOverwriteWithEmptyValue, mergo.WithCaseInsensitiveMapKeys); err != nil { + t.Fatal(err) + } + + expected := map[string]string{"Host": "example.com"} + if !reflect.DeepEqual(dst, expected) { + t.Fatalf("got %#v, want %#v", dst, expected) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@issue256_test.go` around lines 10 - 50, Add a new test (e.g., TestIssue256MergeCaseInsensitiveWithEmptyOverwrite) that mirrors TestIssue256MergeMapCaseInsensitively but passes mergo.WithOverwriteWithEmptyValue together with mergo.WithOverride and mergo.WithCaseInsensitiveMapKeys; create dst with mixed-case keys like "Config"->"Host": "localhost" and src with same keys in different case (e.g., "config"->"host": "") so the empty value must overwrite via case-insensitive matching, call mergo.Merge(&dst, ...), and assert that the dst "Host" entry becomes the empty string (and other keys merge as expected) to cover the empty-overwrite cleanup path honoring case-insensitive keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@merge.go`:
- Around line 120-129: The case-insensitive map-key match implemented using
mergeKey (in the loop over src.MapKeys() with config.caseInsensitiveMapKeys) is
not used later when performing overwrite-with-empty cleanup, which still calls
src.MapIndex(key) and can therefore miss the matched key and delete the
newly-merged entry; update the cleanup logic to perform lookups and deletions
using the resolved mergeKey (or otherwise perform the same case-insensitive key
resolution there) so that the same canonical key (mergeKey) is used for both
merging and subsequent src.MapIndex/... deletion; refer to symbols mergeKey,
config.caseInsensitiveMapKeys, src.MapKeys(), and src.MapIndex() (and the
WithCaseInsensitiveMapKeys / WithOverwriteWithEmptyValue behavior) when making
the change.
---
Nitpick comments:
In `@issue256_test.go`:
- Around line 10-50: Add a new test (e.g.,
TestIssue256MergeCaseInsensitiveWithEmptyOverwrite) that mirrors
TestIssue256MergeMapCaseInsensitively but passes
mergo.WithOverwriteWithEmptyValue together with mergo.WithOverride and
mergo.WithCaseInsensitiveMapKeys; create dst with mixed-case keys like
"Config"->"Host": "localhost" and src with same keys in different case (e.g.,
"config"->"host": "") so the empty value must overwrite via case-insensitive
matching, call mergo.Merge(&dst, ...), and assert that the dst "Host" entry
becomes the empty string (and other keys merge as expected) to cover the
empty-overwrite cleanup path honoring case-insensitive keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
♻️ Duplicate comments (1)
merge.go (1)
122-127:⚠️ Potential issue | 🟠 MajorPrefer exact-key match before
EqualFoldfallback to avoid nondeterministic merges.On Line 123-Line 127, the first case-insensitive destination key is chosen by map iteration order. If
dstcontains both"Host"and"host", a source key"host"can merge into either entry non-deterministically.💡 Proposed fix
for _, key := range src.MapKeys() { mergeKey := key if config.caseInsensitiveMapKeys && key.Kind() == reflect.String && dst.Type().Key().Kind() == reflect.String { - for _, dstKey := range dst.MapKeys() { - if strings.EqualFold(dstKey.String(), key.String()) { - mergeKey = dstKey - break - } - } + // Prefer exact-key hit; fallback to case-insensitive lookup. + if !dst.MapIndex(key).IsValid() { + for _, dstKey := range dst.MapKeys() { + if strings.EqualFold(dstKey.String(), key.String()) { + mergeKey = dstKey + break + } + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merge.go` around lines 122 - 127, The current case-insensitive map-key lookup picks the first EqualFold match from dst.MapKeys(), which is nondeterministic when dst has keys like "Host" and "host"; modify the logic around config.caseInsensitiveMapKeys so it first scans dst.MapKeys() for an exact string match (dstKey.String() == key.String()) and uses that as mergeKey if found, and only if no exact match exists fall back to the existing strings.EqualFold check to pick a case-insensitive match; ensure you update the block that sets mergeKey (the loops over dst.MapKeys()) to implement this two-pass lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@merge.go`:
- Around line 122-127: The current case-insensitive map-key lookup picks the
first EqualFold match from dst.MapKeys(), which is nondeterministic when dst has
keys like "Host" and "host"; modify the logic around
config.caseInsensitiveMapKeys so it first scans dst.MapKeys() for an exact
string match (dstKey.String() == key.String()) and uses that as mergeKey if
found, and only if no exact match exists fall back to the existing
strings.EqualFold check to pick a case-insensitive match; ensure you update the
block that sets mergeKey (the loops over dst.MapKeys()) to implement this
two-pass lookup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30f7aa7b-d268-47df-b7a7-587e34b6dcc4
📒 Files selected for processing (2)
issue256_test.gomerge.go
🚧 Files skipped from review as they are similar to previous changes (1)
- issue256_test.go
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.
Fixes #256
Summary
WithCaseInsensitiveMapKeysfor matching string map keys withstrings.EqualFoldduring merge.Validation
go test ./...git diff --checkSummary by CodeRabbit
New Features
Tests