✨ add key-aware decoding to the query string parser #14
Conversation
…n to '.' and ensure correct parsing
…rsing for accurate dot conversion
…Dots and decodeDotInKeys options
WalkthroughAdds a DecodeKind enum and a kind-aware Decoder fun interface; DecodeOptions centralizes decoding with decodeKey/decodeValue and encoded-dot protection for keys. Internal parser switched to decodeKey/decodeValue, refined bracket/dot handling, and tests updated to the new Decoder shape with encoded-dot key cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant DecodeOptions
participant CustomDecoder as Decoder (custom)
participant DefaultDecode
participant Utils
Client->>DecodeOptions: decode(value, charset, kind)
alt custom decoder present
DecodeOptions->>CustomDecoder: decode(value, charset, kind)
CustomDecoder-->>DecodeOptions: Any?
else no custom decoder
DecodeOptions->>DefaultDecode: defaultDecode(value, charset, kind)
alt kind == KEY
DefaultDecode->>DefaultDecode: protectEncodedDotsForKeys(...)
end
DefaultDecode->>Utils: decode(value, charset)
Utils-->>DefaultDecode: Any?
DefaultDecode-->>DecodeOptions: Any?
end
DecodeOptions-->>Client: Any?
sequenceDiagram
autonumber
participant Parser as Internal Parser
participant Options as DecodeOptions
note over Parser,Options: Key/value parsing flow
Parser->>Options: decodeKey(fragment, charset)
Options-->>Parser: decodedKeyString
Parser->>Options: decodeValue(fragment, charset)
Options-->>Parser: decodedValueAny
note over Parser: KEY path applies encoded-dot protection and bracket-aware trimming
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
⏰ 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)
🔇 Additional comments (18)
✨ 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 (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
============================================
- Coverage 88.08% 87.68% -0.41%
- Complexity 353 382 +29
============================================
Files 14 15 +1
Lines 1032 1104 +72
Branches 241 263 +22
============================================
+ Hits 909 968 +59
- Misses 52 56 +4
- Partials 71 80 +9
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (1)
296-311: Depth=0 semantics bug: dot-to-bracket mapping happens before the depth check.Comment says “Depth 0 … use the original key as a single segment”, but code converts dots to bracket notation before returning. This will change behavior for
allowDots=truewhendepth=0(e.g., "a.b" becomes "a[b]" as a single segment).Fix by moving the depth check before the dot→bracket mapping:
- // Apply dot→bracket *before* splitting, but when depth == 0, we do NOT split at all and do - // NOT throw. - val key: String = - if (allowDots) originalKey.replace(DOT_TO_BRACKET) { "[${it.groupValues[1]}]" } - else originalKey - - // Depth 0 semantics: use the original key as a single segment; never throw. - if (maxDepth <= 0) { - return listOf(key) - } + // Depth 0 semantics: use the original key as a single segment; never throw. + if (maxDepth <= 0) { + return listOf(originalKey) + } + + // Apply dot→bracket only when splitting is permitted by depth. + val key: String = + if (allowDots) originalKey.replace(DOT_TO_BRACKET) { "[${it.groupValues[1]}]" } + else originalKeyAdd a test (see my comment in DecodeSpec.kt) to lock this in.
🧹 Nitpick comments (8)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (2)
588-597: Decoder usage in tests aligns with the new API, but consider a key-decoding guard test.The number decoder works and ignores
kind. Since decoders now run for both keys and values, add a quick test ensuring keys aren’t unintentionally number-parsed when a decoder like this is supplied.Proposed test addition:
@@ it("use number decoder, parses string that has one number with comma option enabled") { val decoder = Decoder { str, charset, _ -> str?.toIntOrNull() ?: Utils.decode(str, charset) } decode("foo=1", DecodeOptions(comma = true, decoder = decoder)) shouldBe mapOf("foo" to 1) decode("foo=0", DecodeOptions(comma = true, decoder = decoder)) shouldBe mapOf("foo" to 0) + // ensure keys are not coerced to numbers + decode("1=foo", DecodeOptions(decoder = decoder)) shouldBe mapOf("1" to "foo") }
1073-1155: Excellent, exhaustive cases for encoded-dot behavior in keys. Add one more for depth=0.The matrix across allowDots/decodeDotInKeys, bracket segments, and bare keys is solid. Consider one more case to lock in the intended “depth 0 means do not split at all” behavior when allowDots=true.
Suggested test:
@@ describe("encoded dot behavior in keys (%2E / %2e)") { + it("depth=0 with allowDots=true: do not split key") { + decode("a.b=c", DecodeOptions(allowDots = true, depth = 0)) shouldBe mapOf("a.b" to "c") + } }qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
175-218: protectEncodedDotsForKeys: logic is correct; micro-optimizations possible.The bracket-depth state machine looks good and preserves case for
%2E/%2e. If desired, you can short-circuit when'%2E'/'%2e'are absent to avoid scanning (minor).Possible tweak:
- if (input.indexOf('%') < 0) return input + val pct = input.indexOf('%') + if (pct < 0) return input + if (input.indexOf("2E", pct) < 0 && input.indexOf("2e", pct) < 0) return input
225-233: Deprecation message could be clearer on behavior change.You might explicitly note that the deprecating method now ignores key/value context and will be removed in the next major.
Proposed message tweak:
- message = "Use decode(value, charset) or decodeKey/decodeValue for context‑aware decoding.", + message = "Deprecated: use decodeKey/decodeValue (or decode(value, charset, kind)) to honor key/value context. This will be removed in the next major.",qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (1)
284-306: Restrict dot→bracket mapping to top-level segments (optional).
DOT_TO_BRACKETtransforms any.not followed by.or[; this can still convert dots inside bracket segments likea[b.c]into nested segments, which may or may not be desired. If the intent is to treat dot-notation only at top level (common in qs), consider depth-aware conversion.Example approach:
- private val DOT_TO_BRACKET = Regex("""\.([^.\[]+)""") + private fun dotToBracketTopLevel(s: String): String { + val sb = StringBuilder(s.length) + var depth = 0 + var i = 0 + while (i < s.length) { + val ch = s[i] + when (ch) { + '[' -> { depth++; sb.append(ch); i++ } + ']' -> { if (depth > 0) depth--; sb.append(ch); i++ } + '.' -> { + if (depth == 0) { + // collect the next segment name (stop at '.' or '[') + val start = ++i + var j = start + while (j < s.length && s[j] != '.' && s[j] != '[') j++ + if (j > start) { + sb.append('[').append(s, start, j).append(']') + i = j + } else { + sb.append('.') // nothing to convert + } + } else { + sb.append('.'); i++ + } + } + else -> { sb.append(ch); i++ } + } + } + return sb.toString() + } @@ - val key: String = - if (allowDots) originalKey.replace(DOT_TO_BRACKET) { "[${it.groupValues[1]}]" } - else originalKey + val key: String = if (allowDots) dotToBracketTopLevel(originalKey) else originalKeyIf you prefer current semantics, consider adding tests for
a[b.c]to document the choice.qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt (3)
516-523: Restrict numeric transformation to values using DecodeKind and avoid "[null]" outputsRight now, this decoder also runs for keys and could turn numeric keys (e.g., "0") into "[0]". It can also produce "[null]" when value is null. Use the kind argument to scope the behavior to values and preserve nulls.
Apply this diff:
- val numberDecoder = Decoder { value, _, _ -> - try { - val intValue = value?.toInt() - "[$intValue]" - } catch (_: NumberFormatException) { - value - } - } + val numberDecoder = Decoder { value, _, kind -> + if (kind == io.github.techouse.qskotlin.enums.DecodeKind.VALUE) { + try { + value?.toInt()?.let { "[$it]" } ?: value + } catch (_: NumberFormatException) { + value + } + } else { + // Leave keys untouched + value + } + }Optional (for readability), add an import instead of the FQN:
import io.github.techouse.qskotlin.enums.DecodeKind
590-596: Use provided charset and preserve nulls for custom decoderHard-coding "Shift_JIS" ignores the charset passed by the parser and converting null to "" changes semantics (notably with strictNullHandling). Respect the charset param when provided and propagate null.
Apply this diff:
- val customDecoder: Decoder = Decoder { content: String?, _, _ -> - try { - java.net.URLDecoder.decode(content ?: "", "Shift_JIS") - } catch (_: Exception) { - content - } - } + val customDecoder: Decoder = Decoder { content: String?, charset, _ -> + if (content == null) { + null + } else { + try { + val cs = (charset ?: java.nio.charset.Charset.forName("Shift_JIS")).name() + java.net.URLDecoder.decode(content, cs) + } catch (_: Exception) { + content + } + } + }
671-676: Update comment (and optionally the implementation) to acknowledge key-aware decodingThe comment states keys/values aren’t distinguished, but the new API provides DecodeKind. Either keep the behavior (lowercase both) and fix the comment, or demonstrate using kind.
Apply this minimal comment fix:
- val keyValueDecoder: Decoder = Decoder { content: String?, _, _ -> - // Note: Kotlin implementation doesn't distinguish between key and value - // decoding - content?.lowercase() - } + val keyValueDecoder: Decoder = Decoder { content: String?, _, _ -> + // This decoder lowercases both keys and values. With DecodeKind available, + // you could branch on kind == DecodeKind.KEY or VALUE if different behaviors are desired. + content?.lowercase() + }Optional alternative showing kind-aware branching:
val keyValueDecoder: Decoder = Decoder { content: String?, _, kind -> // Example: lowercase keys only if (kind == io.github.techouse.qskotlin.enums.DecodeKind.KEY) content?.lowercase() else content }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/enums/DecodeKind.kt(1 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt(4 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt(2 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt(5 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(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/ExampleSpec.kt (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
Decoder(10-11)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
Decoder(10-11)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (1)
decode(452-473)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (3)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
Decoder(10-11)decode(145-156)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (1)
decode(452-473)
⏰ 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: Ensure compatibility with qs
- GitHub Check: JVM tests (Java 21)
- GitHub Check: Android AAR (AGP) + unit tests
- GitHub Check: JVM tests (Java 17)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (10)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/enums/DecodeKind.kt (1)
6-15: DecodeKind enum reads clean; intent is clear.KDoc crisply communicates the key/value context and why keys may need special treatment. No functional concerns.
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (2)
737-743: Good coverage for custom decoder path.This verifies the new Decoder fun interface wiring and demonstrates ignoring
charset/kind. No changes requested.
828-840: Null-returning decoder scenario covered well.This provides important regression coverage for honoring
nullfrom a user decoder. Looks good.qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
158-167: Key-aware default decode is sound.Protecting encoded dots for keys before URL-decoding avoids premature splits and matches the new tests. No changes requested.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (5)
112-119: Key decoding now uses key-aware path — good.Switching bare keys and key slices to
options.decodeKey(...).orEmpty()fixes premature dot-decoding and aligns with the new semantics.
129-131: Value decoding path updated correctly.Using
options.decodeValueensures values do not get the key-specific protections. Looks good.
206-216: Root bracket cleaning is more robust.Using
lastIndexOf(']')prevents malformed remainder segments from leaking trailing brackets. Solid improvement.
212-216: Case-insensitive mapping for %2E/%2e in keys is correct and matches tests.The replacements handle both cases reliably.
261-278: splitKeyIntoSegments → parseObject pipeline remains consistent.Segments are built and consumed in a way that preserves string-keyed maps and list indices with the new key-decoding logic. No change requested.
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/ExampleSpec.kt (1)
486-496: Decoder wrapper migration looks good.Tests now construct decoders with the 3-arg fun interface and continue to validate custom decoding. All good.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt
Show resolved
Hide resolved
… DecodeOptions for backward compatibility
…ith message, replacement, and level
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
29-35: Back-compat adapter for legacy 2-arg decoders: LGTM, matches prior guidance.The
LegacyDecodershim and adapter indecode()are correctly implemented and keep existing call sites working.Optional: if Java callers need a simple migration path without passing all prior constructor args, add a convenience secondary constructor:
data class DecodeOptions( ... ) { + @Deprecated("Use the `decoder` fun interface; this Java-friendly ctor is a temporary bridge") + constructor(legacy: LegacyDecoder) : this( + decoder = Decoder { v, c, _ -> legacy(v, c) }, + legacyDecoder = legacy + ) ... }Also applies to: 171-183
🧹 Nitpick comments (4)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (4)
9-12: Consider makingkindnon-null in the publicDecoderAPI.
DecodeKindis always passed by the library and never null in this file. Making it non-null tightens the contract and avoids null handling in user implementations.-fun interface Decoder { - fun decode(value: String?, charset: Charset?, kind: DecodeKind?): Any? -} +fun interface Decoder { + fun decode(value: String?, charset: Charset?, kind: DecodeKind): Any? +}
14-20: Fix misleadingReplaceWithinLegacyDecoderdeprecation.The suggested snippet references an identifier (
legacyDecoder) that won’t exist at most call sites, which will confuse IDE quick-fixes. Prefer a generic guidance message without an unusable replacement.@Deprecated( - message = "Use Decoder fun interface; this will be removed in a future major release", - replaceWith = ReplaceWith("Decoder { value, charset, _ -> legacyDecoder(value, charset) }"), + message = "Use Decoder fun interface; wrap your two‑arg lambda: Decoder { v, c, _ -> legacy(v, c) }", level = DeprecationLevel.WARNING, ) typealias LegacyDecoder = (String?, Charset?) -> Any?
200-251: Bracket-depth logic looks correct; consider explicitly documenting encoded bracket behavior.
depthonly tracks literal[/]. Percent-encoded brackets (%5B/%5D) are treated as non-structural, which is a reasonable choice. Add a brief KDoc note (or test) to lock this in as intended behavior and avoid future regressions.Example doc tweak near this method:
/** * Double‑encode %2E/%2e in KEY strings so the percent‑decoder does not turn them into '.' too * early. * - * When [includeOutsideBrackets] is true, occurrences both inside and outside bracket segments - * are protected. Otherwise, only those **inside** `[...]` are protected. + * When [includeOutsideBrackets] is true, occurrences both inside and outside bracket segments + * are protected. Otherwise, only those **inside** `[...]` are protected. + * Note: only literal `[`/`]` affect depth; percent‑encoded brackets (`%5B`/`%5D`) are treated + * as content, not structure. */
266-273:decodeKeystringifies custom decoder outputs—confirm this is intended.If a custom
Decoderreturns a non-String for keys,decodeKey(...)?. toString()will coerce it, potentially surprising callers. Since this isinternal, it's fine, but worth an assertion or doc note that keys are always treated as strings post-decode.- internal fun decodeKey(value: String?, charset: Charset?): String? = - decode(value, charset, DecodeKind.KEY)?.toString() + internal fun decodeKey(value: String?, charset: Charset?): String? = + decode(value, charset, DecodeKind.KEY)?.toString() // keys are always coerced to String
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.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: Ensure compatibility with qs
- GitHub Check: Android AAR (AGP) + unit tests
- 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 (4)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (4)
121-127: Depth semantics doc is clearer now.Calling out the throw behavior under
strictDepthreduces ambiguity during key splitting. No further action needed.
135-152: Dot-handling invariants are sound.
getAllowDotsreflectingallowDots || decodeDotInKeysaligns with the stated invariant.- The
require(!getDecodeDotInKeys || getAllowDots)guard matches the doc: disallowdecodeDotInKeys=truewhenallowDots=false.No changes required.
Also applies to: 158-161
257-265: Deprecation path forgetDecoder(...): good transitional API.The delegating deprecation with
ReplaceWith("decode(value, charset)")is helpful and keeps migration noise low.
163-183: Unified decode flow with adapter fallback: solid.The ordering (custom decoder → legacy adapter → default) is correct, and nulls from user decoders are respected. No changes needed.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt
Show resolved
Hide resolved
…ed dots are present; update deprecation message for getDecoder to clarify removal timeline
… to correctly handle encoded dots in key segments
…es for improved readability
…with allowDots and decodeDotInKeys options
… bracket handling in protectEncodedDotsForKeys
…ility in DecodeOptions
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (1)
739-745: Local variable ‘decode’ shadows the top-level decode(...) function and breaks the test
val decode = Decoder { ... }shadows the imported top-levelio.github.techouse.qskotlin.decode. The subsequent calldecode("%8c%a7=...")will try to invoke the fun-interface instance as a function, which does not compile. Rename the local variable to avoid shadowing.Apply this diff:
- val decode = Decoder { str, _, _ -> + val customDecoder = Decoder { str, _, _ -> str?.replace("%8c%a7", "県")?.replace("%91%e5%8d%e3%95%7b", "大阪府") } - decode("%8c%a7=%91%e5%8d%e3%95%7b", DecodeOptions(decoder = decode)) shouldBe + decode("%8c%a7=%91%e5%8d%e3%95%7b", DecodeOptions(decoder = customDecoder)) shouldBe expectedqs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
154-162: Bug: Guard uses derived getAllowDots, masking the intended error conditionThe constructor guard
require(!getDecodeDotInKeys || getAllowDots)will never throw whendecodeDotInKeys=trueandallowDots=false, becausegetAllowDotsitself becomestruewhendecodeDotInKeysistrue. This contradicts both the KDoc and tests that expect an error whendecodeDotInKeys=truebutallowDots=false.Fix by checking the raw
allowDotsflag explicitly:- require(!getDecodeDotInKeys || getAllowDots) { + // If decodeDotInKeys is enabled, allowDots must not be explicitly false. + require(!getDecodeDotInKeys || allowDots != false) { "decodeDotInKeys requires allowDots to be true" }This still permits
allowDots = null(unspecified), which your tests rely on to implyallowDots=truewhendecodeDotInKeys=true.
🧹 Nitpick comments (3)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (1)
206-216: Post-split encoded-dot mapping in keys: OK, but consider a case-insensitive single-passThe post-split mapping of
%2E/%2eto.whengetDecodeDotInKeysis enabled is aligned with the intended semantics (protect before decode, then map after splitting). Minor nit: two replace calls can be simplified to a single case-insensitive replacement for readability.You can reduce it to one pass:
- val decodedRoot = - if (options.getDecodeDotInKeys) - cleanRoot.replace("%2E", ".").replace("%2e", ".") - else cleanRoot + val decodedRoot = + if (options.getDecodeDotInKeys) + cleanRoot.replace(Regex("%2[eE]")) + { "." } + else cleanRootqs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt (1)
107-123: Reflection on a private method is brittle; prefer testing via the public surfaceUsing reflection to call
defaultDecodeworks but is fragile across refactors (name/signature/visibility changes). Consider asserting the same behavior throughDecodeOptions.decodeKey(...)/decodeValue(...)or by exercising the full parse path withDecoder.parseQueryStringValues.If you want, I can sketch equivalent public-surface tests that validate the same
%2E/%2eprotections across both charsets.qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
186-199: Default decode: includeOutsideBrackets flag should mirror the raw allowDots intentPassing
includeOutsideBrackets = getAllowDotsis fine functionally, but it becomes ambiguous givengetAllowDotsauto-enables whendecodeDotInKeys=true. The documentation says “include outside brackets only when allowDots is true”. Consider tying this toallowDots == truefor clarity, since KEY dot-protection outside brackets is unrelated todecodeDotInKeysper se.- val protected = protectEncodedDotsForKeys(value, includeOutsideBrackets = getAllowDots) + val protected = protectEncodedDotsForKeys( + value, + includeOutsideBrackets = (allowDots == true) + )This keeps behavior stable with the fixed ctor guard above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt(6 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt(5 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt(5 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt(4 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt
🧰 Additional context used
🧬 Code graph analysis (2)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (3)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
Decoder(10-11)decode(172-184)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)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/models/DecodeOptions.kt (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/qs.kt (1)
decode(26-73)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt (1)
decode(452-473)
⏰ 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). (1)
- GitHub Check: Ensure compatibility with qs
🔇 Additional comments (8)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (3)
112-131: Migration to key/value-aware decoding is correctSwitching to
options.decodeKey(...)for keys andoptions.decodeValue(...)for values is the right integration point for the new Decoder API. Coercing keys to strings withorEmpty()is also appropriate since map keys are stringly-typed in this module.
286-327: Top-level dot→bracket conversion: add guardrails for degenerate dotsThe
dotToBracketTopLevelimplementation looks solid and preserves dots inside bracket depth. Two edge cases worth covering in tests to avoid regressions:
- Leading/trailing dots:
"a.",".a"(ensure no accidental empty bracket segments like[]appear).- Double dots at top level:
"a..b"(ensure the middle empty segment is preserved literally and not bracketized to[]).Would you like me to add targeted test cases for these to
DecodeSpec?
345-353: Depth=0 semantics preservedShort-circuiting to
listOf(originalKey)whenmaxDepth <= 0matches the intended “no-split” behavior and aligns with the tests. LGTM.qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt (1)
124-156: Good coverage across UTF-8 and ISO-8859-1 branchesThese tests validate that KEY decoding protects encoded dots prior to percent-decoding and that VALUE decoding does not. This closes a subtle parity gap between the two
Utils.decodebranches. Nicely done.qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (2)
588-599: Custom numeric decoder: good check that keys are not coercedUsing a
Decoderthat coerces numeric-looking strings and asserting that keys remain strings is a valuable guard. This confirms the key path’s.toString()coercion is effective.
1075-1162: Encoded-dot behavior matrix looks comprehensiveThe new cases cover:
- allowDots off: encoded
%2E/%2edecoding to literal.without splitting- allowDots on + decodeDotInKeys off: plain dots split, encoded dots preserved within segments
- allowDots on + decodeDotInKeys on: encoded dots mapped to
.inside segments (no extra split)- bracket-segment case sensitivity and the error case when
decodeDotInKeys=truewithallowDots=falseThis is exactly what we need around the new logic. LGTM.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
9-22: API evolution and back-compat shim look goodThe 3-arg
Decoderfun interface and theLegacyDecoderadapter provide a reasonable migration path. The deprecation messages are actionable and includeReplaceWith. LGTM.
121-129: strictDepth docs match behavior in internal.Decoder.splitKeyIntoSegmentsThe KDoc clarifies throwing vs. preserving the remainder, consistent with
Decoder.splitKeyIntoSegments. Good alignment.
1 participant
This pull request refactors and improves the decoding logic in the query string parser, focusing on more robust and context-aware handling of percent-encoded dots in keys and values. The changes introduce a new
DecodeKindenum to distinguish between key and value decoding, unify the decoder interface, and enhance options for dot handling. The test suite is also updated to cover these behaviors.Decoding logic improvements:
DecodeKindenum to distinguish between key and value decoding contexts, allowing more precise control over how percent-encoded dots are handled.Decoderinterface to a unifiedfun interfacethat accepts value, charset, and kind, supporting more flexible and context-aware decoding.DecodeOptionswith newdecode,decodeKey, anddecodeValuemethods, and improved handling of percent-encoded dots in keys, including case-insensitive replacements and bracket segment logic.Core decoder changes:
Test suite updates:
Decoderinterface, ensuring compatibility with the refactored decoding logic. [1] [2] [3] [4] [5] [6] [7]allowDotsanddecodeDotInKeysoptions, bracket segments, and case sensitivity.Summary by CodeRabbit
New Features
Bug Fixes
Deprecations
Tests