🚸 ensure consistent string key usage for maps and correct numeric key handling in Decoder
#10
Conversation
… handling in `Decoder` and test cases
WalkthroughThe decoder and utils now consistently produce string-keyed maps for numeric indices when not forming lists. Decoder logic distinguishes bracketed numeric indices within limits to build lists; otherwise, it stores values under string keys. Utils.merge aligns by constructing string-keyed maps from Iterable inputs. Tests updated accordingly. Encoder imports consolidated. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Query String
participant Decoder
participant Builder as Obj Builder
participant Output as Decoded Result
Input->>Decoder: key=value (e.g., a[1]=b, a.b=c)
Decoder->>Decoder: Parse key segments
Decoder->>Decoder: Detect bracketed index?
alt Bracketed index within listLimit and lists enabled
Decoder->>Builder: Ensure List size idx+1
Builder->>Builder: Place value at list[idx]
else Otherwise
Decoder->>Builder: Use Map<String, Any?> path
Builder->>Builder: Put value under string key
end
Builder->>Output: Return assembled structure
sequenceDiagram
participant Utils as Utils.merge
participant L as Left Target
participant R as Right Source
participant M as Merge Result
L->>Utils: merge(left, right)
alt Iterable involved
Utils->>Utils: Build Map<String, Any?> from indices
Utils->>M: Assign entries with stringified keys
else Map vs Map or scalar
Utils->>M: Standard merge logic
end
Utils-->>L: Return merged result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
techouse
changed the title
DecoderDecoder
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
============================================
- Coverage 88.22% 88.08% -0.15%
Complexity 353 353
============================================
Files 14 14
Lines 1036 1032 -4
Branches 244 241 -3
============================================
- Hits 914 909 -5
Misses 52 52
- Partials 70 71 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (1)
184-193: Make map-to-map merges string-keyed to avoid “1” vs 1 key duplicationIn the map+map merge path, keys from source are currently used as-is. If any numeric keys are Int (not String), this can produce mixed keys for equivalent indices (e.g., "1" and 1). Given the PR goal of standardizing on string keys everywhere, coerce both target and source keys to strings and use a String-keyed merge target.
Apply this diff:
- @Suppress("UNCHECKED_CAST") - val mergeTarget: MutableMap<Any, Any?> = - when { - target is Iterable<*> && source !is Iterable<*> -> - target - .withIndex() - .associate { it.index.toString() to it.value } - .filterValues { it !is Undefined } - .toMutableMap() - else -> (target as Map<Any, Any?>).toMutableMap() - } + @Suppress("UNCHECKED_CAST") + val mergeTarget: MutableMap<String, Any?> = + when { + target is Iterable<*> && source !is Iterable<*> -> + target + .withIndex() + .associate { it.index.toString() to it.value } + .filterValues { it !is Undefined } + .toMutableMap() + else -> (target as Map<*, *>) + .mapKeys { (k, _) -> k.toString() } + .toMutableMap() + } - @Suppress("UNCHECKED_CAST") - (source as Map<Any, Any?>).forEach { (key, value) -> - mergeTarget[key] = - if (mergeTarget.containsKey(key)) { - merge(mergeTarget[key], value, options) - } else { - value - } - } + @Suppress("UNCHECKED_CAST") + (source as Map<*, *>).forEach { (key, value) -> + val k = key.toString() + mergeTarget[k] = + if (mergeTarget.containsKey(k)) { + merge(mergeTarget[k], value, options) + } else { + value + } + }Notes:
- Keeps existing behavior for list-to-map coercion.
- Ensures both target and source map keys are consistently strings in this path, preventing subtle duplication bugs.
Also applies to: 196-203
🧹 Nitpick comments (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt (1)
12-12: Avoid star import from java.util to prevent name clashes with Kotlin collectionsImporting java.util.* can introduce ambiguous references with Kotlin’s Map/List types and makes the file’s dependencies less explicit. Only WeakHashMap is used here.
Apply this diff:
-import java.util.* +import java.util.WeakHashMapqs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (1)
212-216: Simplify numeric-index detection by using toIntOrNull()Current detection uses an extra isPureNumeric check. You can simplify and keep the same behavior (including leading-zero safeguards via string comparison) by relying on toIntOrNull().
Apply:
- val isPureNumeric = decodedRoot.isNotEmpty() && decodedRoot.all { it.isDigit() } - val idx: Int? = if (isPureNumeric) decodedRoot.toInt() else null - val isBracketedNumeric = - idx != null && root != decodedRoot && idx.toString() == decodedRoot + val idx: Int? = decodedRoot.toIntOrNull() + val isBracketedNumeric = + idx != null && root != decodedRoot && idx.toString() == decodedRoot
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt(1 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt(1 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt(4 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/fixtures/data/EmptyTestCases.kt(2 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt(9 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/ExampleSpec.kt(1 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt(9 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/UtilsSpec.kt(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (1)
decode(452-473)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Android AAR (AGP) + unit tests
- GitHub Check: Ensure compatibility with qs
- GitHub Check: JVM tests (Java 21)
- GitHub Check: JVM tests (Java 17)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (37)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt (1)
7-7: Wildcard import from models is acceptable; no functional impactConsolidating multiple imports under io.github.techouse.qskotlin.models.* keeps things readable and has no behavioral change. Good.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (5)
40-45: Correct: iterable-to-map indices now stringifiedConverting Iterable indices to string keys aligns with the PR objective and prevents mixed-type keys downstream.
50-55: Correct: appending elements with string keysUsing i.toString() for newly appended elements preserves the string-key invariant when merging Iterable into a sparse target.
54-65: Result shape handling is sensible
- parseLists=false returns a Map (filtering Undefined)
- Set/List targets return corresponding collection values
- Otherwise returns a List
Behavior is consistent and predictable.
119-126: Correct: when merging Iterable into a Map, use string keysEnsures newly added entries are keyed consistently as strings.
152-162: Good: when target is Iterable and source is Map, convert target to string-keyed map and string-coerce source keysThis is required to guarantee uniform string-key maps during merges.
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/fixtures/data/EmptyTestCases.kt (1)
181-182: Fixture updates match string-index expectationsSwitching inner map keys from numeric to string ("0"/"1") aligns fixtures with the new decoder behavior.
Also applies to: 186-187, 210-211
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/ExampleSpec.kt (1)
182-183: Tests correctly reflect string-keyed indices when lists are not formed
- High indices and listLimit=0 produce maps with string keys
- parseLists=false yields string indices starting at "0"
- Mixed numeric and non-numeric indices merge into a map with consistent string keys
Also applies to: 186-193, 196-197
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/UtilsSpec.kt (3)
372-374: Intentional mixed-key result is acceptable for externally provided numeric-keyed mapsThe pre-existing Int key (0) is retained, while new indices from the list are stringified ("1"). This mirrors Utils.merge behavior and avoids destructive key type coercion on user-provided maps.
441-443: Updated expectation to string indices is correctList indices are stringified when merged into an object, matching Utils changes.
524-529: Stringifying indices when mixing list/object under same key looks goodBoth scenarios (object into list, and list into object) now produce string-keyed maps under "foo", ensuring consistency.
Also applies to: 537-542
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (5)
201-203: Consistently using string-keyed maps in non-list branches — aligns with PR objectiveSwitching to LinkedHashMap<String, Any?> here ensures we never leak integer keys into map structures. This is the right move for consistency across the decoder and Utils.merge.
210-211: Localized dot-decoding for keys looks correctDecoding "%2E" to "." in decodedRoot based on getDecodeDotInKeys keeps behavior aligned with options while avoiding over-decoding. Good.
218-224: Correct: when lists are disabled or listLimit < 0, always return a string-keyed mapUsing "0" when decodedRoot is empty ensures "[]" becomes a map entry "0" -> leaf, which matches updated tests and the PR’s consistency goal.
233-239: Else-path correctly stores leaves under string keysNumeric-looking keys that are not valid list indices (e.g., out of limit, negative, leading zeros) fall back to string-keyed maps as intended. This matches the updated test expectations across the suite.
226-231: No changes needed:Undefined.Companion()correctly invokes the sentinel
The companion object’soperator fun invoke()returns the singleInstance, so callingUndefined.Companion()yields the same sentinel asUndefined(). The fill–and later compaction (is Undefined)–logic remains correct and all existing tests pass.
- Decoder.kt:228 (
MutableList<Any?>(idx + 1) { Undefined.Companion() })Likely an incorrect or invalid review comment.
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (11)
77-77: Expectation updated to string key for numeric top-level key — correct"0" should be a string key now; this mirrors the decoder’s consistent string-key policy.
307-317: Correct: string keys for indices when arrays are not formed
- listLimit = 0 -> a[1] becomes {"1": "c"}
- parseLists = false -> indexed entries map to string keys
This aligns with the new decoder behavior.
320-326: Boundary handling at listLimit: correct fallback to string-keyed mapIndex 21 with listLimit 20 falls back to map key "21" as a string, both explicitly and with defaults — matches intended behavior.
352-363: Arrays-to-objects transformation now uses string indices — goodAll numeric indexes inside transformed maps are asserted as strings ("0", "1"), ensuring consistency throughout the data model.
389-396: Dot-notation arrays-to-objects: indices asserted as strings — goodThese expectations correctly reflect the new string-keyed behavior under allowDots = true.
407-410: Pruning with large indices preserves numeric indices as strings — correctKeys "2" and "99999999" are strings, aligning with the normal form for map keys.
484-489: No-parent parsing: empty bracket index becomes key "0" (string) — correctMaintains uniformity for synthetic indices under both empty value and strictNullHandling.
726-731: Map produced from mixed list/object input uses stringified index — correctThe expected map uses "0" (string) for the array element; consistent with Utils.merge changes.
520-534: List-limit handling: explicit indices and negative indices now stringified in maps — correctAll updated assertions ("0", "1", "-1") are correct given the new decoding rules.
536-541: parseLists=false path: indices are string keys — correctDisabling list parsing consistently yields string-keyed maps for indices.
1035-1045: Converting long lists into maps uses string keys — matches Decoder behaviorWhen list length exceeds the limit, string-keyed maps are asserted; this matches the Decoder branch that falls back to maps.
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt (10)
24-24: Top-level numeric key expected as string — correctThis aligns with the decoder’s normalization to string keys.
169-176: Array index with listLimit = 0 falls back to object with string key — correct"1" becomes a map key instead of an array index under listLimit = 0; expectation is right.
184-191: Limit-bound indices fallback to string-keyed objects — correctAt 21 > 20, both tests properly expect a string-keyed object ("21" -> "a").
233-247: Arrays-to-objects conversion uses string keys — correct and consistentAll numeric indices are asserted as strings, covering multiple ordering scenarios.
279-290: Dot-notation arrays-to-objects: numeric indices as strings — correctThese cover the allowDots path and maintain consistent key typing.
301-303: Undefined pruning scenario asserts stringified numeric keys — correct"2" and "99999999" remain strings as expected.
419-424: Missing parent parsing assigns "0" (string) — correctMatches the decoder’s "[]" -> "0" normalization.
456-464: Negative listLimit: indices are always mapped to string keys — correct"0" and "-1" assertions reflect the “list parsing off” behavior for negative limits.
469-473: parseLists=false path explicitly asserts string-keyed indices — correctBoth multi-index and bracket-only cases are covered.
1013-1015: Utils.merge expected now asserts string keys for iterable-derived targets — correct"0"/"1" are string keys, consistent with the underlying Utils changes.
1 participant
This pull request standardizes the handling of map keys throughout the query string decoder and related utilities to always use string keys, even for numeric indices. This change ensures consistency in the decoded output and prevents issues that could arise from mixing integer and string keys in maps. The update also affects test cases and fixtures to align with this new behavior.
Core decoder and utility logic:
Decoder.ktandUtils.ktto always create maps with string keys instead of integer keys, including for numeric indices and when converting lists to maps. This affects how parsed query strings are represented internally. [1] [2] [3] [4] [5]Test fixtures and cases:
DecodeSpec.kt,ExampleSpec.kt,QsParserSpec.kt, andEmptyTestCases.ktso that expected decoded maps use string keys for indices, ensuring tests match the new decoder output. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13]Code style and imports:
Encoder.ktby using wildcard imports for models and updating standard library imports for consistency.Summary by CodeRabbit
No changes to public APIs.