✨ add key-aware decoding to the query string parser #9
Conversation
…oved key/value decoding clarity
…eDotInKeys options
WalkthroughAdds context-aware decoding: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Parser as Internal.Decoder
participant Opts as DecodeOptions
participant KindDec as DecoderWithKind
participant Legacy as Decoder (legacy)
participant Default as DefaultDecode
Caller->>Parser: Parse(query, options)
Parser->>Opts: DecodeKey(rawKey, charset)
alt DecoderWithKind provided
Opts->>KindDec: Invoke(rawKey, charset, Key)
KindDec-->>Opts: result
alt result is null
Opts->>Default: DefaultDecode(rawKey, charset)
Default-->>Opts: decoded string (or null)
end
else legacy Decoder provided
Opts->>Legacy: Invoke(rawKey, charset)
Legacy-->>Opts: result
else
Opts->>Default: DefaultDecode(rawKey, charset)
Default-->>Opts: decoded string
end
Opts-->>Parser: decoded key
Parser->>Opts: DecodeValue(rawValue, charset)
alt DecoderWithKind provided
Opts->>KindDec: Invoke(rawValue, charset, Value)
KindDec-->>Opts: result
else legacy Decoder provided
Opts->>Legacy: Invoke(rawValue, charset)
Legacy-->>Opts: result
else
Opts->>Default: DefaultDecode(rawValue, charset)
Default-->>Opts: decoded value
end
Opts-->>Parser: decoded value
Parser-->>Caller: parsed structure
sequenceDiagram
autonumber
participant Opts as DecodeOptions
Note over Opts: ProtectEncodedDotsForKeys / DotToBracketTopLevel applies only when kind=Key
Opts->>Opts: ProtectEncodedDotsForKeys("a%2E[b].c")
rect rgba(230,240,255,0.5)
Note right of Opts: Preserve %2E/%2e inside/outside brackets to avoid premature dot-splitting
end
Opts-->>Opts: Split into segments (respect brackets) -> Percent-decode segments -> Return decoded key segments
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
🧰 Additional context used🧬 Code graph analysis (1)QsNet/Models/DecodeOptions.cs (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). (3)
🔇 Additional comments (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 #9 +/- ##
==========================================
+ Coverage 90.25% 90.45% +0.19%
==========================================
Files 15 15
Lines 1242 1299 +57
Branches 349 364 +15
==========================================
+ Hits 1121 1175 +54
- Misses 121 124 +3
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 (5)
QsNet/Internal/Decoder.cs (2)
321-328: Consider centralizing “%2E → .” mapping in DecodeOptions to remove duplicationWe’re now protecting encoded dots in DecodeOptions.DefaultDecode for keys, and we also post-process here to map “%2E/%2e” to “.” when DecodeDotInKeys is true. Functionally OK, but the split responsibilities can be surprising and easy to drift.
Optional refactor: move the “%2E → .” mapping logic entirely behind DecodeOptions.DecodeKey (e.g., a small helper that is only applied for bracket segments), and keep Decoder.cs focused on structural parsing. This reduces the risk of future inconsistencies.
99-113: Nit: consolidate parameter limit validation message across codebaseThe thrown message here is “Parameter limit must be a positive integer.” while DecodeOptions constructor throws “Parameter limit must be positive”. Consider normalizing the phrasing to a single message to simplify testing and operator diagnostics.
QsNet/Models/DecodeOptions.cs (3)
159-164: Document implicit AllowDots=true when DecodeDotInKeys=true (unless explicitly set false)AllowDots getter returning true when _allowDots is unset and _decodeDotInKeys is true is intentional and helpful, but it is a non-obvious coupling. Please call this out in the property’s XML doc to prevent confusion.
Suggested doc tweak:
/// <summary> -/// Set to true to use dot dictionary notation in the encoded output. +/// Set to true to use dot dictionary notation in the encoded output. +/// Note: when not explicitly set, this property implicitly evaluates to true +/// if <see cref="DecodeDotInKeys" /> is true, to keep option combinations coherent. /// </summary>
225-235: ProtectEncodedDotsForKeys logic is robust; add a couple of edge-case comments/testsImplementation details look solid (bracket depth tracking, case preservation for 2E/2e, early bail-out when no '%'). Two minor suggestions:
- Comment that only “%2E/%2e” are special-cased and “%2F” etc. are intentionally not protected.
- Add a test for mixed content like "a[%2E].b%2E[c]" with AllowDots=true and DecodeDotInKeys=true to assert the exact final segmentation and mapping (inside brackets always maps to '.', outside follows AllowDots rules).
No code changes necessary; just clarifying comments and a test.
If you want, I can draft the additional test case covering the mixed bracket/top-level scenario.
Also applies to: 239-285
298-303: Obsolete message: small clarity tweakThe message says “Use Decode(value, encoding) or DecodeKey/DecodeValue…”. Since Decode(value, encoding) defaults to Value kind, consider: “Use Decode(value, encoding, kind) or DecodeKey/DecodeValue…” to point readers to the context-aware overload explicitly.
-[Obsolete("Use Decode(value, encoding) or DecodeKey/DecodeValue for context-aware decoding.")] +[Obsolete("Use Decode(value, encoding, kind) or DecodeKey/DecodeValue for context-aware decoding.")]
📜 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)
QsNet.Tests/DecodeOptionsTests.cs(2 hunks)QsNet.Tests/DecodeTests.cs(1 hunks)QsNet/Enums/DecodeKind.cs(1 hunks)QsNet/Internal/Decoder.cs(2 hunks)QsNet/Models/DecodeOptions.cs(5 hunks)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (12)
QsNet/Enums/DecodeKind.cs (1)
3-31: Well-scoped enum for decoding context; XML docs clearly state semanticsClean separation of Key vs Value contexts with precise docs that align with the PR’s behavior changes. No issues spotted.
QsNet/Internal/Decoder.cs (2)
167-178: Routing through DecodeKey/DecodeValue is correct and future‑proofSwitching from the legacy decoder entry point to options.DecodeKey/options.DecodeValue aligns this class with the new context‑aware decoding API and enables the encoded-dot protections and kind-aware decoders. Looks good.
Also applies to: 182-191
167-178: No remainingGetDecodercall sites foundA repository-wide search confirms that the only occurrence of
GetDecoder(is its obsolete definition inQsNet/Models/DecodeOptions.cs(line 299). There are no other call sites bypassing the kind-aware decoding paths.• QsNet/Models/DecodeOptions.cs:299 – obsolete method definition only
QsNet.Tests/DecodeTests.cs (1)
4127-4254: Excellent coverage for encoded-dot behavior; cases match stated semanticsThe new tests precisely exercise:
- Top-level behavior with AllowDots on/off and DecodeDotInKeys on/off
- Bracket segments always protected vs not, driven by DecodeDotInKeys
- Value decoding always turning %2E into “.”
- Latin1 variants
This gives strong regression protection for the new rules.
QsNet/Models/DecodeOptions.cs (2)
191-201: Decoder precedence and validation order look right
- Early validation for the invalid combo (DecodeDotInKeys && !AllowDots) when kind==Key prevents foot‑guns.
- DecoderWithKind strictly takes precedence, including honoring nulls with no fallback to Decoder. This matches the tests and is a good contract.
No changes requested.
329-374: CopyWith correctly preserves both decoders and allows selective overridesThe precedence (DecoderWithKind over Decoder) is preserved in the copy, and init-only properties are handled as expected. Looks good.
QsNet.Tests/DecodeOptionsTests.cs (6)
114-127: Good guardrail: invalid option combo throws with clear messageAsserting that DecodeKey throws when DecodeDotInKeys=true and AllowDots=false documents the contract well.
129-141: Key-decoding protects encoded dots prior to percent-decoding — correct and necessaryThis verifies the critical “protect before decode” step; nice.
150-170: DecoderWithKind precedence and kind attribution validatedCapturing both calls and kinds ensures the precedence contract won’t regress.
173-182: Null-return semantics honored (no fallback) — good to have explicitly testedPrevents accidental future fallback to the legacy decoder when null is meaningful.
185-196: Legacy decoder path verified for both values and keysUppercasing "%2E" without dot-mapping shows the intended bypass when no kind-aware decoder is present.
198-219: CopyWith preserves/overrides decoders with correct precedenceGreat coverage to ensure both decoder delegates carry across copies and that kind-aware remains in effect even if the legacy decoder is changed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
QsNet.Tests/DecodeOptionsTests.cs (2)
114-127: Good negative-path assertion; consider loosening strict message matching.The test asserts the full exception message string. This can make tests brittle if wording changes. Prefer Contains or a predicate that checks the essential part (e.g., decodeDotInKeys/allowDots relationship) to keep intent while allowing harmless copy tweaks.
- act.Should().Throw<ArgumentException>() - .WithMessage("decodeDotInKeys requires allowDots to be true"); + act.Should().Throw<ArgumentException>() + .Where(e => e.Message.Contains("decodeDotInKeys", StringComparison.OrdinalIgnoreCase) + && e.Message.Contains("allowDots", StringComparison.OrdinalIgnoreCase));
150-170: Decoder precedence and null behavior are well-specified; add a guard test for key type safety.
- Precedence of DecoderWithKind over Decoder and honoring null returns are clearly exercised. CopyWith behavior is also verified.
- One gap: if a custom kind-aware decoder mistakenly returns a non-string for keys, DecodeKey currently casts via "as string", silently producing null (and later an empty key). Consider adding a test that expects an InvalidOperationException for non-string key results once the guard is implemented (see suggested change in DecodeOptions).
If you want, I can add a unit test like:
- “DecodeKey_Throws_When_KindAwareDecoder_Returns_NonString()”
that asserts an InvalidOperationException with a helpful message.Also applies to: 173-182, 185-195, 198-219
QsNet/Models/DecodeOptions.cs (4)
205-209: Avoid silent nulls when a custom key decoder returns a non-string.Currently, DecodeKey casts via "as string", which turns non-string results into null, potentially collapsing keys to "", leading to surprising dictionary entries. Fail fast with a clear error when a custom decoder returns an invalid type for keys.
Apply this change:
public string? DecodeKey(string? value, Encoding? encoding = null) { - return Decode(value, encoding, DecodeKind.Key) as string; + var decoded = Decode(value, encoding, DecodeKind.Key); + if (decoded is null) return null; + if (decoded is string s) return s; + throw new InvalidOperationException( + $"Key decoder must return a string or null; got {decoded.GetType().FullName}." + ); }
239-285: ProtectEncodedDotsForKeys implementation is correct and efficient.
- Linear scan with minimal allocations; early exit when no '%' present.
- Case handling for 'E'/'e' is covered; bracket depth logic is straightforward.
Optional: add a micro-optimization to pre-check for "%2E" or "%2e" specifically (IndexOf with OrdinalIgnoreCase) before allocating the StringBuilder, to skip scanning paths with other '%' sequences only. Likely negligible unless keys are very long.
159-164: AllowDots getter derives from DecodeDotInKeys; document this subtlety.AllowDots returns true when DecodeDotInKeys==true and AllowDots was not explicitly set. This is intentional per the PR, but it’s a subtle behavior. Consider adding one line to the XML doc to call this out so integrators aren’t surprised.
/// Set to true to use dot dictionary notation in the encoded output. /// </summary> public bool AllowDots { init => _allowDots = value; - get => _allowDots ?? _decodeDotInKeys == true; + get => _allowDots ?? _decodeDotInKeys == true; // implied true when DecodeDotInKeys is true }
41-53: Constructor validation note.The ParameterLimit > 0 check runs before property initializers apply user-provided values; the effective validation is done in Decoder.ParseQueryStringValues anyway. Keeping the constructor check is harmless, but it won’t catch invalid values set via object initializers. If you want constructor-time enforcement, consider moving to a factory or making ParameterLimit set-only with a validating setter (non-trivial change; optional).
📜 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)
QsNet.Tests/DecodeOptionsTests.cs(2 hunks)QsNet.Tests/DecodeTests.cs(1 hunks)QsNet/Enums/DecodeKind.cs(1 hunks)QsNet/Internal/Decoder.cs(2 hunks)QsNet/Models/DecodeOptions.cs(5 hunks)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (10)
QsNet/Enums/DecodeKind.cs (1)
1-31: Enum introduction looks solid; semantics are clear.
- Public surface area is minimal and well-documented. Naming (Key/Value) is intuitive.
- No compatibility concerns identified.
QsNet.Tests/DecodeOptionsTests.cs (1)
131-140: Nice coverage of key-specific dot protection and normal value decoding.These tests pin the intended behavior: encoded dots are preserved in keys (pre-split) but values decode normally to '.'. This guards against regressions in the key/value decode paths.
Also applies to: 142-147
QsNet/Internal/Decoder.cs (3)
167-168: Switch to context-aware DecodeKey/DecodeValue is correct and aligns with new API.
- Decoding the raw key segment via DecodeKey and the value via DecodeValue is the right integration pivot.
- This ensures key-specific protections happen before any structural splitting, matching the PR’s objectives.
Also applies to: 173-178, 183-191
321-329: Bracket-segment dot normalization logic is appropriate; ensure both cases are covered on NETSTANDARD2_0 and newer.
- Using case-insensitive replace for "%2E/%2e" inside bracket segments when DecodeDotInKeys is true is correct.
- Tests cover both upper/lowercase; good.
416-424: Depth=0 semantics with AllowDots=true: clarify intent and consider a test.The code applies dot→bracket translation before checking depth and then returns the un-split key when maxDepth <= 0. With AllowDots=true and Depth=0, a key like "a.b" becomes "a[b]" (i.e., the final key string changes even though we do not split). If that’s intentional, great—if not, consider skipping the DotToBracket.Replace when maxDepth <= 0.
Would you like me to add a test proving the intended behavior? Example: "Decode_DepthZero_WithAllowDots_LeavesOriginalKeyUnchanged" (or the inverse, if the current behavior is desired).
QsNet.Tests/DecodeTests.cs (1)
4127-4254: Thorough encoded-dot coverage across cases (top-level, bracketed, latin1).
- Validates the nuanced contract: plain dots split when AllowDots=true, percent-encoded dots remain literal unless DecodeDotInKeys=true; bracket segments honor DecodeDotInKeys regardless of AllowDots; value tokens always decode normally.
- This suite will catch subtle regressions.
QsNet/Models/DecodeOptions.cs (4)
191-200: Guard for invalid configuration is in the right place.Throwing when kind==Key and DecodeDotInKeys=true but AllowDots=false enforces the configuration contract at the call site, which is where it matters.
225-236: DefaultDecode key-path behavior matches spec.
- Protecting "%2E/%2e" prior to percent-decoding only in the key path (and selectively outside brackets) is exactly what we want.
- Value path remains a direct Utils.Decode.
329-373: CopyWith preserves both decoder delegates and precedence; good.
- DecoderWithKind is preserved unless explicitly overridden, keeping precedence stable.
- Legacy Decoder remains useful for value-only or simple use cases.
299-303: Obsolete GetDecoder delegation maintains back-compat.Marking as Obsolete while delegating to Decode keeps existing callers functional without duplicating logic.
…g for dot handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
QsNet.Tests/DecodeOptionsTests.cs (2)
129-142: Add bracket-segment coverage to validate “always protect inside brackets.”
This test covers top-level keys. To fully exercise ProtectEncodedDotsForKeys semantics, add cases with brackets (e.g., a[%2Eb], a[b%2Ec], nested) and verify protection occurs regardless of AllowDots.Here is a self-contained test you can append to this class:
[Fact] public void DecodeKey_ProtectsEncodedDots_InsideBrackets_RegardlessOfAllowDots() { var o1 = new DecodeOptions { AllowDots = false, DecodeDotInKeys = false }; var o2 = new DecodeOptions { AllowDots = true, DecodeDotInKeys = false }; // Inside bracket: always protected o1.DecodeKey("a[%2Eb]", Encoding.UTF8).Should().Be("a[%2Eb]"); o1.DecodeKey("a[b%2Ec]", Encoding.UTF8).Should().Be("a[b%2Ec]"); o2.DecodeKey("a[%2Eb]", Encoding.UTF8).Should().Be("a[%2Eb]"); o2.DecodeKey("a[b%2Ec]", Encoding.UTF8).Should().Be("a[b%2Ec]"); }
185-197: Legacy decoder fallback behavior for keys is verified; consider adding one more assertion.
Current check ensures legacy decoder is used when no kind-aware decoder is present. Consider also asserting that the default key-protection path is not used in this case by choosing an input that would be altered by DefaultDecode, e.g. "a%2Eb", which you already cover. Looks good overall.QsNet/Models/DecodeOptions.cs (6)
17-26: Kind-aware decoder delegate is well-scoped; consider clarifying key return expectations.
The delegate returns object?, but keys ultimately must be string|null. It’s enforced later in DecodeKey; adding a short note here can reduce surprises for extender authors./// <returns>The decoded value, or null if the value is not present.</returns> -public delegate object? KindAwareDecoder(string? value, Encoding? encoding, DecodeKind kind); +/// <remarks>When <paramref name="kind"/> is <see cref="DecodeKind.Key"/>, the decoder is expected to return a string or null.</remarks> +public delegate object? KindAwareDecoder(string? value, Encoding? encoding, DecodeKind kind);
156-165: Doc wording and implicit-true behavior are good; minor phrasing fix suggested.
The property correctly implements “implicit true when DecodeDotInKeys is true and AllowDots was not explicitly set.” The doc currently says “encoded output” (more apt for encoding); this is a decode option.-/// Set to true to use dot dictionary notation in the encoded output. +/// Set to true to parse dot dictionary notation in the encoded input. /// Note: when not explicitly set, this property implicitly evaluates to true /// if <see cref="DecodeDotInKeys" /> is true, to keep option combinations coherent.
187-203: Decode pipeline and option validation look correct; consider exception type/message polish.
The guard throws ArgumentException when option combination is invalid during a key decode. This is acceptable, although some libraries prefer InvalidOperationException for misconfiguration. If you stick with ArgumentException, consider tightening the message to reference options explicitly for better user guidance.-throw new ArgumentException("decodeDotInKeys requires allowDots to be true"); +throw new ArgumentException("Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys.");
204-217: Type safety check for keys is excellent; optionally add a more actionable error hint.
The InvalidOperationException is precise. Consider hinting that extenders should return string|null for keys when using custom decoders.- $"Key decoder must return a string or null; got {decoded.GetType().FullName}.") + $"Key decoder must return a string or null; got {decoded.GetType().FullName}. " + + "If using a custom decoder, ensure it returns string for keys.")
246-294: ProtectEncodedDotsForKeys: correctness looks solid; consider minor robustness and perf tweaks.
- Robustness: treat lowercase ‘%2e’ and uppercase consistently (already done) and short-circuit early (already done). You could also optionally handle stray ‘%2’ trailing sequences defensively, although Utils.Decode likely handles malformed encodings.
- Perf/readability: using spans would avoid allocations, but this is micro; the current loop is clear and fast enough.
If you want the defensive guard on incomplete percent sequences:
-else if (ch == '%' && i + 2 < input.Length && input[i + 1] == '2' && +else if (ch == '%' && i + 2 < input.Length && input[i + 1] == '2' && (input[i + 2] == 'E' || input[i + 2] == 'e')) { ... } +else if (ch == '%' && (i + 2 >= input.Length || input[i + 1] == '\0' || input[i + 2] == '\0')) +{ + // Leave malformed/incomplete escape as-is + sb.Append(ch); + i++; +}
353-356: CopyWith preserves both decoders and their precedence — good. Note on implicit AllowDots sentinel.
Because AllowDots uses a nullable backing field, CopyWith currently materializes its effective value into the new instance (AllowDots = allowDots ?? AllowDots). If you intended to preserve the “unset” sentinel (so that it keeps tracking DecodeDotInKeys implicitly), consider copying the backing field instead.Here’s one way to preserve the sentinel within the class:
public DecodeOptions CopyWith( ... ) { - return new DecodeOptions - { - AllowDots = allowDots ?? AllowDots, + var next = new DecodeOptions + { + // Keep all plain options as before AllowEmptyLists = allowEmptyLists ?? AllowEmptyLists, ... ThrowOnLimitExceeded = throwOnLimitExceeded ?? ThrowOnLimitExceeded - }; + }; + + // Preserve AllowDots’ sentinel semantics + next._allowDots = allowDots is null ? this._allowDots : allowDots; + // Preserve DecodeDotInKeys similarly + next._decodeDotInKeys = decodeDotInKeys is null ? this._decodeDotInKeys : decodeDotInKeys; + + // Preserve/override decoders (must be assigned after 'next' is created) + next.Decoder = decoder ?? this.Decoder; + next.DecoderWithKind = decoderWithKind ?? this.DecoderWithKind; + + return next; }If the current behavior is intentional, feel free to ignore this.
📜 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)
QsNet.Tests/DecodeOptionsTests.cs(2 hunks)QsNet/Models/DecodeOptions.cs(5 hunks)
⏰ 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). (3)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
QsNet.Tests/DecodeOptionsTests.cs (6)
1-2: Imports look correct; no action needed.
No concerns with the additional using directives.
114-128: Good guard-rail test for invalid option combo (DecodeDotInKeys=true with AllowDots=false).
The assertion checks both exception type and key substrings in the message; this is robust and matches the intended behavior enforced in DecodeOptions.Decode.
143-149: Value decoding sanity check is appropriate.
Confirms values continue to percent-decode normally and independently of key handling.
150-172: Correctly verifies precedence and context for DecoderWithKind.
Covers both invocation count and kind ordering. Nice.
173-184: Null short-circuit behavior is well specified and tested.
Ensures no fallback occurs when the kind-aware decoder returns null. This is important for users who want to intentionally suppress values/keys.
198-220: CopyWith preserves/overrides decoders and honors precedence.
Solid coverage of both preserving decoders and overriding one at a time. No changes needed.QsNet/Models/DecodeOptions.cs (1)
227-245: DefaultDecode applies key protection prior to percent-decoding; logic aligns with tests and PR goals.
The separation of key vs. value paths and use of AllowDots to steer out-of-bracket protection are correct.
…requirement for DecodeDotInKeys
…ecodeKey regardless of AllowDots setting
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
QsNet/Models/DecodeOptions.cs (1)
1-377: Obsolete GetDecoder removal appears completePast reviews requested deleting the obsolete GetDecoder method. It’s no longer present in this file. As a follow-up, verify no call sites remain across the repo.
Run:
#!/bin/bash # Ensure no stragglers for the old API remain. rg -n -S '\bGetDecoder\s*\(' -g '!**/bin/**' -g '!**/obj/**' -C2 # Spot-check that new APIs are wired in at key call sites. rg -nP --type cs -C2 '\bDecode(Key|Value)\s*\('
🧹 Nitpick comments (5)
QsNet/Models/DecodeOptions.cs (5)
17-26: Key-aware decoder contract is only enforced via DecodeKey; consider stronger typing or clearer guidanceThe delegate returns object?, but keys are required to be string/null. Today this is only enforced when callers go through DecodeKey; direct Decode(..., kind: Key) calls can bypass that until later use. Optional, but consider:
- Add a short note in the XML summary that calling Decode(kind: Key) directly should return string/null and that DecodeKey is the recommended API.
- Alternatively, split the contract into two delegates (KeyDecoder returns string?, ValueDecoder returns object?) or expose a small wrapper factory to adapt older decoders safely.
157-166: Implicit AllowDots from DecodeDotInKeys: behavior is sensible; document edge-case semanticsGetter logic aligns with the doc: when not explicitly set, AllowDots is implied true if DecodeDotInKeys is true. Two notes:
- This means explicitly setting AllowDots=false while DecodeDotInKeys=true is the only conflicting state (and will throw later in Decode for keys). Consider adding this explicit-state caveat to the AllowDots XML doc to avoid surprises.
- Verify tests cover all four combinations of explicit/null AllowDots with DecodeDotInKeys true/false.
Would you like me to add tests asserting AllowDots’ implied value when only DecodeDotInKeys is set?
188-205: Exception type and message for invalid option combinationThrowing for DecodeDotInKeys=true while AllowDots=false only when decoding keys is acceptable. Minor nits:
- Consider InvalidOperationException since this is an invalid object state rather than a bad method argument; or populate ArgumentException’s paramName for clarity.
- Tiny copy tweak: include property names to aid diagnostics (and possibly the current option values).
Apply one of these diffs:
- throw new ArgumentException( - "Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys."); + throw new InvalidOperationException( + "Invalid DecodeOptions state: when decoding keys, DecodeDotInKeys=true requires AllowDots=true.");or
- throw new ArgumentException( - "Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys."); + throw new ArgumentException( + "DecodeDotInKeys=true requires AllowDots=true when decoding keys.", + paramName: nameof(DecodeDotInKeys));
249-304: ProtectEncodedDotsForKeys: handle percent-encoded brackets to honor “inside bracket” rule; minor cleanupsCurrently bracket depth is tracked only for literal '['/']'. If brackets are percent-encoded (%5B/%5D), depth stays 0 and encoded dots inside such segments won’t be protected “always,” violating the stated rule. Recommend recognizing %5B/%5b and %5D/%5d for depth, and avoiding negative depth. Also, the '\0' checks are unnecessary given the bounds check.
Apply this diff:
@@ - for (var i = 0; i < input.Length;) + for (var i = 0; i < input.Length;) { var ch = input[i]; if (ch == '[') { depth++; sb.Append(ch); i++; } else if (ch == ']') { - if (depth > 0) depth--; + if (depth > 0) depth--; sb.Append(ch); i++; } + // Handle percent-encoded brackets to track depth even when [] are encoded. + else if (ch == '%' && i + 2 < input.Length && input[i + 1] == '5' && + (input[i + 2] == 'B' || input[i + 2] == 'b')) + { + depth++; + sb.Append('%').Append('5').Append(input[i + 2]); + i += 3; + } + else if (ch == '%' && i + 2 < input.Length && input[i + 1] == '5' && + (input[i + 2] == 'D' || input[i + 2] == 'd')) + { + if (depth > 0) depth--; + sb.Append('%').Append('5').Append(input[i + 2]); + i += 3; + } else if (ch == '%' && i + 2 < input.Length && input[i + 1] == '2' && (input[i + 2] == 'E' || input[i + 2] == 'e')) { var inside = depth > 0; if (inside || includeOutsideBrackets) { sb.Append("%25"); sb.Append(input[i + 2] == 'E' ? "2E" : "2e"); } else { sb.Append('%').Append('2').Append(input[i + 2]); } i += 3; } - else if (ch == '%' && (i + 2 >= input.Length || input[i + 1] == '\0' || input[i + 2] == '\0')) + else if (ch == '%' && i + 2 >= input.Length) { // Leave malformed/incomplete escape as-is sb.Append(ch); i++; }This keeps the original bytes while correctly tracking depth when brackets are percent-encoded.
333-334: CopyWith signature: good to expose override for DecoderWithKindSignature change is appropriate. One minor UX nit: today CopyWith can’t “clear” a previously set decoder; passing null preserves the old one. Consider adding explicit clear flags to revert to defaults when needed.
Possible pattern:
- Decoder? decoder = null, - KindAwareDecoder? decoderWithKind = null, + Decoder? decoder = null, + KindAwareDecoder? decoderWithKind = null, + bool clearDecoder = false, + bool clearDecoderWithKind = false, @@ - Decoder = decoder ?? Decoder, - DecoderWithKind = decoderWithKind ?? DecoderWithKind, + Decoder = clearDecoder ? null : (decoder ?? Decoder), + DecoderWithKind = clearDecoderWithKind ? null : (decoderWithKind ?? DecoderWithKind),
📜 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)
QsNet.Tests/DecodeOptionsTests.cs(2 hunks)QsNet/Models/DecodeOptions.cs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QsNet.Tests/DecodeOptionsTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
QsNet/Models/DecodeOptions.cs (2)
QsNet/Internal/Decoder.cs (1)
Decoder(16-525)QsNet/Internal/Utils.cs (2)
Decode(488-516)Utils(20-1172)
⏰ 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: Codacy Static Code Analysis
🔇 Additional comments (6)
QsNet/Models/DecodeOptions.cs (6)
173-178: Precedence of DecoderWithKind over Decoder is clearPrivate getter with init-only setter is appropriate; precedence is well-documented and avoids ambiguity.
206-221: Type safety at the DecodeKey boundary is goodEnforcing string/null here is the right choke point. The error message is precise and actionable.
222-229: Value decoding pass-through is fineDecodeValue delegating to Decode(kind: Value) keeps the surface area coherent.
230-248: DefaultDecode key path: confirm final key representation for encoded dots meets expectationsProtectEncodedDotsForKeys + Utils.Decode preserves “%2E” as literal text in decoded keys, preventing unintended dot-splitting. That means a key like a%2Eb will ultimately be “a%2Eb”, not “a.b”. If your intent is to produce a literal “.” in the final key while still avoiding splitting, an additional “unprotect” step must occur after key segmentation. If the intended behavior is to keep “%2E”, current code is correct—just ensure tests assert that shape.
Would you like me to add tests for:
- a%2Eb=1 with AllowDots=true/false and DecodeDotInKeys=true/false (asserting final key is “a%2Eb” vs “a.b”)?
311-312: CopyWith XML docs include the new decoderWithKind paramDoc addition looks good and consistent with the new API surface.
363-364: CopyWith propagation order preserves existing decoders correctlyPrecedence (DecoderWithKind over Decoder) remains intact after copying. LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
QsNet/Models/DecodeOptions.cs (3)
17-29: Context-aware decoder delegate looks good; minor naming consistency nit.The new KindAwareDecoder signature and docs are clear. Consider aligning the delegate name with the property name (DecoderWithKind) to reduce cognitive mapping for users skimming the API surface. Not a blocker.
191-209: Guardrail for invalid configuration (DecodeDotInKeys=true with AllowDots=false).Throwing on key-decoding when an explicit contradiction exists is aligned with the PR objectives. Tiny readability/perf nit: checking the backing fields makes the intention explicit and avoids invoking the derived property.
Apply this diff to make the intent explicit:
- if (kind == DecodeKind.Key && DecodeDotInKeys && !AllowDots) + if (kind == DecodeKind.Key && (_decodeDotInKeys == true) && (_allowDots == false)) throw new ArgumentException( "DecodeDotInKeys=true requires AllowDots=true when decoding keys.", nameof(DecodeDotInKeys) );If you prefer to keep the current version for clarity, that's fine too; behavior is equivalent.
254-324: Encoded dot protection is robust; consider small guard and tests.The bracket depth tracking (including %5B/%5D) and case handling for %2E/%2e are solid. Two optional improvements:
- Micro-guard: early-return when there’s no encoded dot to protect to avoid scanning long keys that only contain unrelated escapes.
Apply this optional short-circuit (keeps netstandard2.0-friendly APIs):
- if (string.IsNullOrEmpty(input) || input.IndexOf('%') < 0) + if (string.IsNullOrEmpty(input) || input.IndexOf('%') < 0) return input; + // Fast-path: if there are no encoded dots or brackets, skip scanning. + if (input.IndexOf("%2E", StringComparison.OrdinalIgnoreCase) < 0 + && input.IndexOf("%5B", StringComparison.OrdinalIgnoreCase) < 0 + && input.IndexOf("%5D", StringComparison.OrdinalIgnoreCase) < 0) + return input;
- Tests: ensure coverage for mixed-case encodings and nested brackets, e.g. keys like a%5Bb%5D%5Bc%5D%2Ed and a[b%5Bc%5D].e with AllowDots true/false and DecodeDotInKeys true to assert the precise protection boundaries.
If helpful, I can draft targeted unit tests for these cases.
📜 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)
QsNet/Models/DecodeOptions.cs(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QsNet/Models/DecodeOptions.cs (2)
QsNet/Internal/Decoder.cs (1)
Decoder(16-525)QsNet/Internal/Utils.cs (2)
Decode(488-516)Utils(20-1172)
⏰ 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). (4)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
QsNet/Models/DecodeOptions.cs (6)
160-169: Implicit AllowDots semantics are coherent and match the PR goals.Having AllowDots evaluate to true when DecodeDotInKeys is true (unless explicitly set) keeps the combinations consistent and avoids footguns. The inline comment makes the behavior discoverable. LGTM.
176-181: Precedence of DecoderWithKind over Decoder is the right call.Private getter with init-only fits the immutability pattern already used for Decoder. Good addition.
211-225: Type safety for key decoding is enforced correctly.Validating that key decoders return string or null prevents subtle downstream issues in key-splitting and mapping. Message is actionable. LGTM.
227-233: Value decoding wrapper is appropriately thin.Delegating to Decode with the default kind keeps the surface consistent and future-proof if value-specific logic is added later.
235-253: DefaultDecode: correct protection strategy before percent-decoding.Protecting encoded dots pre-decode only in keys, and only outside brackets when AllowDots is true, matches the spec described in the PR. Nice separation of concerns (DefaultDecode vs. ProtectEncodedDotsForKeys).
331-331: CopyWith correctly preserves existing decoders and exposes the new kind-aware override.Passing through both Decoder and DecoderWithKind while allowing overrides preserves behavior on copies. XML doc addition for decoderWithKind is appreciated.
Also applies to: 353-355, 383-385
…s are present; refactor bracket/dot decoding logic with switch statement
…ectEncodedDotsForKeys
…ment splitting to handle trailing text after last closing bracket
…nside brackets and handle degenerate cases in segment splitting
…ies; ensure bracketed dot-encoded segments decode to literal dots
…encoded brackets and dot splitting with AllowDots and DecodeDotInKeys options
…ests to DecodeTests; remove unused using from UtilsTests
…and DecodeDotInKeys options; verify splitting, bracket mapping, trailing dots, and decoder callback behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
QsNet/Internal/Decoder.cs (2)
320-328: Bracket-segment dot mapping may be redundant with current DecodeKey defaultdecodedRoot = options.DecodeDotInKeys ? cleanRoot.Replace("%2E", ".") ... only takes effect when a custom key decoder leaves “%2E” intact until here. With the current default pipeline, DecodeKey decodes “%2E” to “.” before ParseObject, so cleanRoot will rarely contain “%2E”. This is fine for supporting custom DecoderWithKind, but consider adding a brief comment to state this block mainly serves custom decoders.
469-548: Depth-aware splitting loop looks correct; confirm remainder wrapping matches legacy behavior
- Balanced-bracket scanning with level is solid and prevents premature closes.
- On unterminated groups, returning [key] preserves the literal input, which matches qs semantics.
- Trailing remainder after the last ‘]’ is wrapped into a final bracket segment unless it’s a lone “.”. This is a behavioral refinement; ensure it aligns with expected/legacy outputs for inputs like “a[b]c” (now segments => "a", "[b]", "[c]").
If legacy intended “a[b]c” to be a literal “a[b]c”, you may need a feature flag or additional tests; otherwise this seems sensible.
Would you like me to add a couple of focused tests (“a[b]c”, “a[b]c.d”, StrictDepth on/off) to lock this in?
QsNet/Models/DecodeOptions.cs (1)
255-330: Dead code: ProtectEncodedDotsForKeys is currently unusedGiven DefaultDecode ignores kind, this helper never runs. After applying the previous change, it becomes essential. If you decide not to wire it in, please remove the method to avoid confusion.
QsNet.Tests/DecodeTests.cs (4)
4156-4174: Test name contradicts asserted behaviorMethod name says “EncodedDotRemainsPercentSequence” but the expectation asserts a split into ["a"]["b"]. Rename to reflect actual expectation (it splits like a plain dot).
Apply this diff:
- public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysFalse_EncodedDotRemainsPercentSequence() + public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysFalse_SplitsLikePlainDot()
4204-4222: Same issue: name vs. behavior mismatchThe bracket-segment test’s name says “RemainsPercentSequence” but the assertion expects ["."]. Adjust to avoid confusion.
- public void EncodedDot_BracketSegment_RemainsPercentSequence_WhenDecodeDotInKeysFalse() + public void EncodedDot_BracketSegment_DecodesToDot_WhenDecodeDotInKeysFalse()
4354-4377: Redundant coverage with earlier top-level encoded-dot testsTopLevel_EncodedDot_AllowDotsTrue_DecodeDotInKeysTrue_Splits and the “AlsoSplits” variant with DecodeDotInKeys=false assert the same observable behavior under AllowDots=true. Consider consolidating into a single [Theory] with inline data for DecodeDotInKeys to reduce duplication and test time.
I can convert these into a single theory with (decodeDotInKeys: true/false, input casing: upper/lower) if you like.
4480-4492: Duplicate of earlier “BracketSegment_EncodedDot_MappedToDot” testThis repeats the case already covered at lines 4184–4202. Keeping one version is sufficient unless you intend to validate different encodings/casing (then fold into a [Theory]).
📜 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)
QsNet.Tests/DecodeOptionsTests.cs(2 hunks)QsNet.Tests/DecodeTests.cs(1 hunks)QsNet.Tests/UtilsTests.cs(0 hunks)QsNet/Internal/Decoder.cs(5 hunks)QsNet/Models/DecodeOptions.cs(5 hunks)
💤 Files with no reviewable changes (1)
- QsNet.Tests/UtilsTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- QsNet.Tests/DecodeOptionsTests.cs
🧰 Additional context used
🧬 Code graph analysis (3)
QsNet/Models/DecodeOptions.cs (2)
QsNet/Internal/Decoder.cs (1)
Decoder(16-608)QsNet/Internal/Utils.cs (2)
Decode(488-516)Utils(20-1172)
QsNet/Internal/Decoder.cs (2)
QsNet/Models/DecodeOptions.cs (2)
DecodeKey(214-225)DecodeValue(230-233)QsNet/Internal/Utils.cs (2)
Utils(20-1172)Apply(709-725)
QsNet.Tests/DecodeTests.cs (3)
QsNet/Models/DecodeOptions.cs (5)
DecodeOptions(34-404)DecodeOptions(43-57)DecodeOptions(357-403)Decode(197-209)DecodeKey(214-225)QsNet/Internal/Utils.cs (9)
Decode(488-516)Dictionary(524-665)Dictionary(848-854)Dictionary(861-870)Dictionary(877-889)Dictionary(942-945)Dictionary(953-1019)Dictionary(1048-1171)List(673-701)QsNet/Internal/Decoder.cs (2)
Dictionary(92-230)List(462-548)
⏰ 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). (3)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
QsNet/Internal/Decoder.cs (4)
165-169: Good: switched to DecodeKey for key-only partsUsing options.DecodeKey(part, charset) ensures kind-aware/legacy decoders are honored for keys without values. Clean and correct.
172-179: Good: decode key before value-splitting logicExtracting rawKey and passing it to options.DecodeKey(...) is consistent across TFMs and centralizes decoding policy in DecodeOptions.
183-192: Value decoding now honors user decoders per elementWrapping ParseListValue with Utils.Apply and invoking options.DecodeValue(v?.ToString(), charset) correctly applies decoding to each scalar and preserves lists. This also keeps percent-encoded commas from splitting prematurely. Nice.
400-451: Top-level dot→bracket conversion is robust and avoids regex; minor edge-case noteDotToBracketTopLevel correctly:
- Ignores degenerate dots (leading/double/trailing),
- Preserves dots inside brackets,
- Converts “a.[b]” to “a[b]” by dropping the extraneous dot,
- Stops tokens at the next ‘.’ or ‘[’.
Edge-case to keep in mind (FYI, no change needed): mixed encoded brackets are already normalized earlier in ParseQueryStringValues, so this routine does not need to handle “%5B/%5D”.
If you want extra assurance, add a test for multi-dot inputs like "a..b.c" with AllowDots=true to confirm degenerate segments are ignored as intended.
QsNet/Models/DecodeOptions.cs (3)
198-209: Precondition enforcement is correctThrowing when kind==Key and DecodeDotInKeys=true but AllowDots=false prevents ambiguous configurations early. Good guardrail.
160-169: Nice: AllowDots implicitly true when DecodeDotInKeys is trueThis keeps option combinations coherent and matches the new guard in Decode(..). No action.
357-403: CopyWith correctly preserves both decoder pathwaysIncluding DecoderWithKind in CopyWith ensures overrides propagate while keeping existing delegates when not replaced. Good API ergonomics.
QsNet.Tests/DecodeTests.cs (1)
4516-4536: Great: asserts KindAwareDecoder receives Key and Value tokensThis confirms DecoderWithKind precedence and that keys are passed through the key pipeline. Helpful for guarding future refactors.
…or keys and values
…date invocation to match new signature
This pull request introduces comprehensive improvements to how query string decoding handles percent-encoded dots (
%2E/%2e) in keys, adds more flexible decoder configuration, and significantly expands test coverage for these behaviors. The changes ensure that decoding logic matches the referenceqslibrary's semantics and allow for context-aware decoding via new APIs.Encoded dot handling in keys
AllowDotsis true.DecodeDotInKeys=trueandAllowDots=falsenow throws anArgumentException, matching expected configuration constraints.Decoder extensibility
KindAwareDecoder, a delegate that receives decoding context (KeyorValue) for more precise control. If provided, it takes precedence over the legacy decoder. [1] [2]DecodeKeyandDecodeValuefor context-aware decoding, and refactored internal usage to prefer these over the legacy decoder. [1] [2]CopyWithto allow overriding both decoder types, and fixed preservation of decoder functions when copying options. [1] [2]Expanded test coverage
AllowDotsandDecodeDotInKeys, including charset variations.Internal refactoring
New enum for decode context
DecodeKindenum to distinguish between key and value decoding contexts, enabling precise decoder logic.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores