🐛 fix decode dot in keys #15
Conversation
…decodeKey/decodeValue visibility
…C# parity and edge case coverage
…ndling of top-level dots and bracket segments
…ter in key decoding
…lDefaultDecode reflection helper
WalkthroughRefactors query parsing to normalize percent-encoded brackets, strengthens top-level dot handling and balanced-bracket key splitting with maxDepth/strictDepth semantics, and adjusts parse flow. Introduces a public, pluggable Decoder API (and LegacyDecoder), unifies key/value decode paths, removes the backward-compat helper, and expands tests for encoded-dot behavior across options and charsets. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Parser as QueryParser
participant DOpts as DecodeOptions
participant Dec as Decoder
participant LDec as LegacyDecoder
Client->>Parser: parse(query, options)
Note right of Parser #f6f8ff: Normalize `%5B`/`%5D` → `[`/`]` (cleanStr)
Parser->>Parser: dotToBracketTopLevel (top-level '.' rules & %2E preservation)
Parser->>Parser: splitKeyIntoSegments (balanced `[]`, maxDepth/strictDepth)
loop For each key=value pair
Parser->>DOpts: decode(keyToken, charset, KEY)
alt Custom Decoder provided
DOpts->>Dec: invoke(keyToken, charset, KEY)
Dec-->>DOpts: decodedKey
else Legacy only
DOpts->>LDec: invoke(keyToken, KEY)
LDec-->>DOpts: decodedKey
else No custom decoder
DOpts-->>Parser: defaultDecode(keyToken, charset)
end
Parser->>DOpts: decode(valueToken, charset, VALUE)
alt Custom Decoder provided
DOpts->>Dec: invoke(valueToken, charset, VALUE)
Dec-->>DOpts: decodedValue
else Legacy only
DOpts->>LDec: invoke(valueToken, VALUE)
LDec-->>DOpts: decodedValue
else No custom decoder
DOpts-->>Parser: defaultDecode(valueToken, charset)
end
Parser->>Parser: assemble object using segments
end
Parser-->>Client: result object
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ 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 #15 +/- ##
============================================
+ Coverage 87.68% 87.92% +0.24%
+ Complexity 382 375 -7
============================================
Files 15 15
Lines 1104 1093 -11
Branches 263 257 -6
============================================
- Hits 968 961 -7
Misses 56 56
+ Partials 80 76 -4
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
🧹 Nitpick comments (2)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (1)
1076-1179: Test consolidation opportunity detectedThe original test suite "encoded dot behavior in keys (%2E / %2e)" and the new "C# parity" test suite have overlapping coverage. Consider consolidating these to avoid redundancy and improve maintainability.
Since both test suites are testing similar behavior (encoded dot handling), consider merging them into a single comprehensive test suite with clear subsections:
- describe("encoded dot behavior in keys (%2E / %2e)") { + describe("encoded dot behavior in keys (%2E / %2e) - comprehensive coverage") { // Keep the original test cases it("allowDots=false, decodeDotInKeys=false: encoded dots decode to literal '.'; no dot-splitting") { // ... existing test code } // ... other original tests + // Add C# parity tests as a subsection + describe("C# parity compliance") { + it("top-level: allowDots=true, decodeDotInKeys=true → plain dot splits; encoded dot also splits") { + // ... C# parity test code + } + // ... other C# parity tests + } - } - - describe("C# parity: encoded dot behavior in keys (%2E / %2e)") { - // Remove duplicate test suite }qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
213-259: Consider removing unused codeThe
protectEncodedDotsForKeysfunction is marked as unused with@Suppress("unused"). While the documentation explains it's kept for parity with other ports, consider removing it to reduce code maintenance burden unless there's a concrete plan to use it.If this function is truly not needed in the current implementation and there's no immediate plan to use it, consider removing it entirely rather than keeping dead code:
- /** - * (Currently unused) Utility that double‑encodes `%2E`/`%2e` in *key* strings so a - * percent‑decoder would not prematurely turn them into `.`. - * - * In the current implementation, encoded‑dot behavior is handled during key‑splitting, so this - * helper is kept for parity with other ports and potential future reuse. - * - * 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. - */ - @Suppress("unused") - private fun protectEncodedDotsForKeys(input: String, includeOutsideBrackets: Boolean): String { - // ... function implementation - }
📜 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 (4)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt(8 hunks)qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt(4 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt(6 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
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)
decode(175-187)Decoder(10-11)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)
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)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt (1)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
Decoder(10-11)
⏰ 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 17)
- GitHub Check: JVM tests (Java 21)
- GitHub Check: Android AAR (AGP) + unit tests
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (8)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (1)
1091-1105: Updated test expectation is correct!The test expectation change from
mapOf("a%2Eobj" to mapOf("first" to "John"))tomapOf("a" to mapOf("b" to mapOf("c" to "d")))for the input"a%2eb.c=d"correctly reflects the new behavior where encoded dots now split at the top level whenallowDots=true.qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt (3)
45-63: Good practice: Normalizing percent-encoded brackets earlyThe normalization of percent-encoded brackets (
%5B/%5Dto[/]) before parsing is a clean approach that simplifies downstream logic. The case-insensitive replacement handles both uppercase and lowercase variants correctly.
299-360: Well-structured implementation of dot-to-bracket conversion!The
dotToBracketTopLevelfunction elegantly handles complex edge cases:
- Correctly preserves dots inside brackets
- Handles degenerate cases (leading/trailing/double dots)
- Maintains percent sequences verbatim for later processing
The explicit documentation of rules and examples is particularly helpful for maintainability.
402-430: Excellent implementation of balanced bracket parsing!The balanced bracket parsing logic correctly handles nested brackets within segments (e.g.,
[with[inner]]as one segment), which is a significant improvement over simple bracket matching. The handling of unterminated brackets is also well-defined.qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (2)
9-21: Good API design with backward compatibilityThe introduction of the
Decoderfunctional interface with the deprecation ofLegacyDecoderprovides a clean migration path while maintaining backward compatibility. The deprecation message is clear and helpful.
262-268: Clean public API for key and value decodingMaking
decodeKeyanddecodeValuepublic methods provides a clear API for consumers while maintaining consistent behavior through the internaldecodemethod.qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/models/DecodeOptionsSpec.kt (2)
197-216: Comprehensive decoder testing!Excellent test coverage for the custom decoder functionality, including:
- Verification that decoder is called for both KEY and VALUE types
- Proper tracking of decode kinds
- Echo-back validation
271-276: Good validation of configuration constraintsThe test properly validates that inconsistent configurations (allowDots=false with decodeDotInKeys=true) throw exceptions, ensuring API contract enforcement.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (4)
148-154: Doc: Clarify when encoded dots become '.' inside bracket segmentsCurrent Javadoc implies encoded dots in key segments are only mapped to '.' when
decodeDotInKeys == true. However, the test suite exercises that bracket-segment tokens percent-decode to '.' regardless ofdecodeDotInKeys(splitting behavior is separate). Please tighten the wording to avoid implying that mapping inside brackets is gated bydecodeDotInKeys.Apply this doc-only tweak:
- * Defaults to `false` when unspecified. When `true`, encoded dots (`%2E`/`%2e`) inside key - * segments are mapped to `.` *after* key‑splitting (in the parser), without introducing extra - * dot‑splits. + * Defaults to `false` when unspecified. Inside bracket segments, percent-decoding will naturally + * yield '.' from `%2E/%2e`. `decodeDotInKeys` controls whether encoded dots at the top level are + * treated as additional split points; it does not affect the literal '.' produced by + * percent-decoding inside bracket segments.
166-187: Decoder.kind nullability and API shape
Decoder.decode(_, _, kind: DecodeKind?)is always called with a non-nullkindin this class. Makingkindnon-null in the publicDecoderinterface will remove downstream "kind ?: VALUE" defensive code in user decoders and make the API clearer. If binary/API stability is a concern, keep as-is for now.-fun interface Decoder { - fun decode(value: String?, charset: Charset?, kind: DecodeKind?): Any? -} +fun interface Decoder { + fun decode(value: String?, charset: Charset?, kind: DecodeKind): Any? +}Follow-up in this file:
- d.decode(value, charset, kind) // honor nulls from user decoder + d.decode(value, charset, kind)
192-199: Doc: Default decode vs dot-handling responsibilityThe comment says encoded-dot handling is “performed in the parser’s key‑splitting logic”. Practically,
defaultDecodepercent-decodes%2Eto '.' (viaUtils.decode); the parser then decides whether that '.' participates in splitting. Suggest wording that reflects the split of responsibilities to avoid confusion.- * Keys are decoded identically to values via [Utils.decode]. Encoded‑dot handling (e.g. - * `%2E`/`%2e` in key segments) is performed in the parser’s key‑splitting logic, not here. + * Keys are decoded identically to values via [Utils.decode], which percent‑decodes `%2E/%2e` to '.'. + * Whether a '.' participates in key splitting is decided by the parser (based on options).
202-208: Ergonomics: add no-charset overloads fordecodeKey/decodeValueThese are now public and commonly called with the effective options charset. Add convenience overloads that default to
this.charsetto reduce call-site noise; also add@JvmOverloadsfor Java callers.-/** Convenience: decode a key to String? */ -fun decodeKey(value: String?, charset: Charset?): String? = - decode(value, charset, DecodeKind.KEY)?.toString() // keys are always coerced to String - -/** Convenience: decode a value */ -fun decodeValue(value: String?, charset: Charset?): Any? = - decode(value, charset, DecodeKind.VALUE) +/** Convenience: decode a key to String? */ +@JvmOverloads +fun decodeKey(value: String?, charset: Charset? = this.charset): String? = + decode(value, charset, DecodeKind.KEY)?.toString() // keys are always coerced to String + +/** Convenience: decode a value */ +@JvmOverloads +fun decodeValue(value: String?, charset: Charset? = this.charset): Any? = + decode(value, charset, DecodeKind.VALUE)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (4)
1125-1135: Fix description: behavior whendecodeDotInKeys=falseinside bracketsThe description says “When disabled, keep %2E literal (no conversion)”, but the assertions correctly expect '.' (percent-decoded) inside bracket segments. Update the test title to avoid confusion.
- it("bracket segment: %2E mapped based on decodeDotInKeys; case-insensitive") { - // When disabled, keep %2E literal (no conversion) + it("bracket segment: percent-decoding yields '.' regardless of decodeDotInKeys") { + // When disabled, still percent-decodes to '.'; no extra split occurs
1145-1157: Fix description to match assertions for bare-key withallowDots=true, decodeDotInKeys=falseThe text says “keep %2E inside key segment”, but the expectation shows a split into
a -> b. Please align the description with the intended behavior, or adjust the expectation per the overall encoded-dot policy (see prior comment).Option (description-only tweak):
- it("bare-key (no '='): behavior matches key decoding path") { + it("bare-key (no '='): with allowDots=true, encoded dot behaves like a split") {If keeping the “ignore encoded dots” spec, change the assertion instead:
- decode("a%2Eb", DecodeOptions(allowDots = true, decodeDotInKeys = false)) shouldBe - mapOf("a" to mapOf("b" to "")) + decode("a%2Eb", DecodeOptions(allowDots = true, decodeDotInKeys = false)) shouldBe + mapOf("a.b" to "")
1206-1221: Bracket segment behavior LGTM; align DecodeOptions docs accordinglyThese tests assert that inside brackets,
%2E/%2epercent‑decodes to '.' regardless ofdecodeDotInKeys. That’s sensible and stable across charsets. Please ensure the DecodeOptions KDoc reflects this nuance (see earlier doc comment).
1091-1106: Optional: consolidate the spec narrative in test descriptionsSeveral descriptions still reference prior semantics (“keep %2E literal”) or omit the top-level vs bracket distinction. Tighten the wording across these cases to reduce future confusion for contributors.
I can push a small PR comment-only patch to harmonize the titles/comments once the encoded-dot top-level policy is confirmed.
Also applies to: 1125-1135, 1145-1157, 1180-1207
📜 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 (2)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt(3 hunks)qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt(7 hunks)
🧰 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)
decode(175-187)Decoder(10-11)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/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 17)
- GitHub Check: JVM tests (Java 21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (java-kotlin)
🔇 Additional comments (6)
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/models/DecodeOptions.kt (1)
156-164: Guardrails LGTMCharset validation, positive parameter limit, and the
decodeDotInKeys/allowDotsrelationship check are correct and align with the tests.qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt (5)
1076-1088: Good expansion of baseline semanticsThe initial cases for “allowDots=false, decodeDotInKeys=false” correctly capture that
%2E/%2epercent‑decodes to '.' without causing a split when dot‑splitting is disabled.
1091-1106: Spec/test mismatch: should encoded top‑level dots split whendecodeDotInKeys=false?Here the test asserts that with
allowDots=true, decodeDotInKeys=false, a top‑level encoded dot (a%2eb.c) causes an extra split (a → b → c). The PR objectives state “dotToBracketTopLevel now splits only on literal top‑level dots, ignores percent‑encoded dots”, which conflicts with this expectation.Choose one:
- Keep the new spec (ignore encoded dots at top level): adjust this expectation to not split
a%2ebintoa/b.- Change the spec: update the docs to say that with
allowDots=true, encoded dots also split at top level even whendecodeDotInKeys=false.If you keep the “ignore encoded dots” spec, update the expectation as follows:
- ) shouldBe mapOf("a" to mapOf("b" to mapOf("c" to "d"))) + ) shouldBe mapOf("a.b" to mapOf("c" to "d"))
1180-1207: “C# parity” top-level cases contradict PR summary about encoded dotsThese cases assert that encoded top-level dots split with
allowDots=trueregardless ofdecodeDotInKeys. That directly contradicts the PR objective (“ignore percent‑encoded dots at top level”). Please reconcile spec vs tests and update one or the other consistently.Would you like me to generate a follow-up patch that:
- Updates the docs/comments across DecodeOptions and parser to state “encoded top-level dots split when allowDots=true”, or
- Adjusts these expectations to keep
a%2Ebas a single segment whendecodeDotInKeys=false?
1255-1264: Great: unit tests for KEY-path decodingVerifies that
DecodeOptions.decode(..., kind = KEY)percent‑decodes inside brackets; this guards regressions in key decoding pathways.
1364-1385: Good verification: custom decoder receives KEY/ VALUE contextsThis ensures the
Decoderis invoked withDecodeKind.KEYfor keys andVALUEfor values. Nice coverage.
…alue convenience methods with default charset
1 participant
This pull request refactors the query string decoder to improve handling of dots and percent-encoded dots in keys, bringing its behavior in line with other implementations and clarifying the logic for splitting keys into segments. It updates how dot notation is converted to bracket notation, ensures encoded dots are treated consistently, and improves key splitting for nested brackets. The tests are updated to reflect the new behavior.
Key parsing and dot/encoded-dot handling:
dotToBracketTopLevelto only split on literal dots at the top level, ignoring percent-encoded dots (%2E/%2e) and handling degenerate cases (e.g., leading, trailing, or double dots). Dots inside brackets are preserved, and percent-encoded brackets are normalized before splitting. [1] [2][with[inner]]as a single segment, and clarified behavior for unterminated brackets and depth limits. [1] [2]API and code cleanup:
decodeKeyanddecodeValuepublic for convenience and removed deprecated helpers, simplifying the decoding API.Tests and documentation:
"a%2eb.c=d"and"a[%2E]=x". [1] [2] [3]Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores