fix(core): make Metadata keys consistently case-insensitive#1827
Open
anuq wants to merge 3 commits intoapache:mainfrom
Open
fix(core): make Metadata keys consistently case-insensitive#1827anuq wants to merge 3 commits intoapache:mainfrom
anuq wants to merge 3 commits intoapache:mainfrom
Conversation
Normalize all keys to lowercase (Locale.ROOT) on every read and write operation via a private normalizeKey() helper. Previously, getValues() and containsKey() had a case-insensitive fallback but addValue(), setValue(), setValues(), addValues(), and remove() stored keys with their original casing, causing inconsistent behavior. Fixes apache#1588 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Thanks for the PR @anuq - please verify the build output. |
…sitive lookup After making all keys lowercase via normalizeKey(), keySet(prefix) was still doing a case-sensitive startsWith() match, causing lookups with uppercase prefixes (e.g. IMAGE., NEWS.) to return empty sets. Fixes SiteMapParserBoltTest failures for extension attribute assertions.
sigee
reviewed
Mar 15, 2026
| class MetadataTest { | ||
|
|
||
| @Test | ||
| void testCaseInsensitiveKeys() { |
Member
There was a problem hiding this comment.
Good work! One thing I'd push back on slightly: testCaseInsensitiveKeys is doing a lot of work - it tests getFirstValue, containsKey, addValue, and remove all in one go. If this test fails, it can take some digging to identify which behavior broke.
Would you consider splitting it into separate test cases, each focused on a single method?
testGetFirstValueIsCaseInsensitivetestContainsKeyIsCaseInsensitivetestAddValueIsCaseInsensitivetestRemoveIsCaseInsensitive
This keeps tests easier to read, maintain, and debug. Let me know your thoughts!
Author
There was a problem hiding this comment.
makes sense, let me take another look at the test cases
…key lookup - Replace testCaseInsensitiveKeys with four single-method tests: testGetFirstValueIsCaseInsensitive, testContainsKeyIsCaseInsensitive, testAddValueIsCaseInsensitive, testRemoveIsCaseInsensitive - Fix testAddValueIsCaseInsensitive size assertion (1, not 2 — same normalized key) - Fix checkListKeyFromOpensearch: look up "somekey" (lowercase) since normalizeKey() lowercases all keys before storing in OpenSearch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
3 participants
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
Fixes #1588
HTTP headers are case-insensitive per RFC 2616, but
Metadatahad inconsistent behavior —getValues()andcontainsKey()did a case-insensitive fallback while write operations (addValue,setValue,setValues,addValues,remove) stored keys with their original casing.Changes
normalizeKey(String key)helper that lowercases keys usingLocale.ROOTgetValues()andcontainsKey()— no more two-step fallback neededtestCaseInsensitiveKeys()test covering get, containsKey, addValue deduplication, and remove across mixed casingsTest