🎨 standardize exception handling and improve culture-invariant string formatting #11
Conversation
…nvariant array key formatting
…t encoding for NETSTANDARD2_0 compatibility
…ndexOutOfRangeException or ArgumentException
… limit and depth checks in Decoder; ensure culture-invariant numeric parsing and NETSTANDARD2_0 compatibility
…ot decoding option errors
…owDots combination; update DefaultDecode to return string
…able analyzers and validation, configure symbol and source embedding, add SourceLink for GitHub
…ssor formatting in Decoder
WalkthroughStandardized error semantics to InvalidOperationException across encode/decode paths; switched culture-sensitive formatting/parsing to CultureInfo.InvariantCulture; tightened DecodeOptions key-decoder contract and changed DefaultDecode to return Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Decoder
participant Utils
Caller->>Decoder: Decode(input, options)
alt DecodeDotInKeys=true && decoding keys && AllowDots=false
Decoder-->>Caller: throw InvalidOperationException("Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys.")
else depth/limit exceeded (param/list/depth)
Decoder-->>Caller: throw InvalidOperationException("...limit exceeded...")
else
Decoder->>Utils: Unescape / InterpretNumericEntities (decimal & hex, surrogates)
Utils-->>Decoder: decoded values
Decoder-->>Caller: parsed object
end
sequenceDiagram
autonumber
participant Caller
participant Encoder
Caller->>Encoder: Encode(object)
alt cyclic/self-referential detected
Encoder-->>Caller: throw InvalidOperationException("Cyclic object value")
else
Encoder-->>Caller: encoded string
end
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). (4)
✨ 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 #11 +/- ##
==========================================
- Coverage 90.40% 90.22% -0.18%
==========================================
Files 15 15
Lines 1323 1351 +28
Branches 374 381 +7
==========================================
+ Hits 1196 1219 +23
- Misses 127 132 +5
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
QsNet/Qs.cs (1)
29-30: Docs still reference IndexOutOfRangeException — update to InvalidOperationExceptionThe exception type documented for “limits exceeded” is now InvalidOperationException in the implementation. Update the XML docs to avoid misleading consumers.
/// <exception cref="ArgumentException">If the input is not a string or Dictionary</exception> -/// <exception cref="IndexOutOfRangeException">If limits are exceeded and ThrowOnLimitExceeded is true</exception> +/// <exception cref="InvalidOperationException">If limits are exceeded and ThrowOnLimitExceeded is true</exception> @@ -/// <exception cref="IndexOutOfRangeException">Thrown when index is out of bounds</exception> +/// <exception cref="InvalidOperationException">Thrown when options/limits are violated during encoding</exception>Also applies to: 125-126
QsNet/Internal/Decoder.cs (1)
73-75: Decoder XML docs still reference IndexOutOfRangeException — update to InvalidOperationExceptionUpdate comments to reflect the current behavior and keep documentation accurate.
-/// <exception cref="IndexOutOfRangeException">If the parameter limit is exceeded and ThrowOnLimitExceeded is true.</exception> +/// <exception cref="InvalidOperationException">If the parameter limit is exceeded and ThrowOnLimitExceeded is true.</exception> @@ -/// <exception cref="IndexOutOfRangeException">If the depth exceeds maxDepth and strictDepth is true.</exception> +/// <exception cref="InvalidOperationException">If the depth exceeds maxDepth and strictDepth is true.</exception>Also applies to: 499-500
🧹 Nitpick comments (11)
QsNet/Internal/Encoder.cs (1)
373-377: Prefer ordinal-based checks and Replace overload to avoid culture-sensitive comparisons.Small robustness tweak: for non-NETSTANDARD2_0, use StringComparison.Ordinal in Contains, and also prefer the Replace overload with StringComparison.Ordinal (guarded with the same conditional).
Within the current block:
- if (allowDots && encodeDotInKeys && keyStr.Contains('.')) + if (allowDots && encodeDotInKeys && keyStr.Contains('.', StringComparison.Ordinal))Additionally, right below (outside the changed lines), consider applying ordinal Replace on supported TFMs:
- encodedKey = keyStr.Replace(".", "%2E"); +#if NETSTANDARD2_0 + encodedKey = keyStr.Replace(".", "%2E"); +#else + encodedKey = keyStr.Replace(".", "%2E", StringComparison.Ordinal); +#endifQsNet/QsNet.csproj (1)
10-15: Packaging/diagnostics upgrades look solid; optionally fail CI on analyzer warnings.Great additions: SourceLink, deterministic builds, analyzers, and richer tags. To keep quality high, consider treating warnings as errors in CI to catch analyzer regressions early.
+ <PropertyGroup> + <!-- Make analyzer and build warnings fail CI builds --> + <TreatWarningsAsErrors Condition="'$(CI)' == 'true'">true</TreatWarningsAsErrors> + </PropertyGroup>Also applies to: 22-36, 38-40, 47-49
QsNet/Internal/Utils.cs (1)
790-797: Fast-path guard is good; consider supporting hex entities (&#xNNNN;).Optional enhancement: InterpretNumericEntities currently handles only decimal entities (e.g., ©). Many sources also use hex (e.g., ©). If in scope, supporting hex would increase robustness.
Example sketch (not exhaustive; ensure bounds checks and overflow handling):
- if (!str.Contains("&#", StringComparison.Ordinal)) + if (!str.Contains("&#", StringComparison.Ordinal)) return str; ... - if (j < n && char.IsDigit(str[j])) + if (j < n && (char.IsDigit(str[j]) || (str[j] is 'x' or 'X' && j + 1 < n))) { - var code = 0; + var code = 0; var startDigits = j; - while (j < n && char.IsDigit(str[j])) + var hex = false; + if (str[j] is 'x' or 'X') { hex = true; j++; startDigits = j; } + while (j < n && (hex ? Uri.IsHexDigit(str[j]) : char.IsDigit(str[j]))) { - code = code * 10 + (str[j] - '0'); + code = hex + ? (code << 4) + Convert.ToInt32(str[j].ToString(), 16) + : code * 10 + (str[j] - '0'); j++; }If you’d like, I can send a fuller, allocation-free version using integer math only.
QsNet.Tests/DecodeTests.cs (2)
2279-2308: StrictDepth tests now expect InvalidOperationException — consistent with decoder changesAll four assertions changed to Throw() are in line with the decoder’s strict-depth overflow behavior. Consider asserting an indicative message (even a wildcard) to catch regressions in error source vs. other InvalidOperationException paths.
4182-4185: Dot-in-keys option conflict: double-check message casing to match assertionThe tests now expect InvalidOperationException, which matches DecodeOptions.Decode’s new validation. One caveat: the .WithMessage("decodeDotInKeysallowDots*") matcher in Line 4353 uses lowercase property names. If FluentAssertions’ WithMessage is case-sensitive in your version, the current thrown message from DecodeOptions uses “DecodeDotInKeys” / “AllowDots” (PascalCase) and may not match. Either:
- Update the thrown message to use lowercase property names, or
- Relax the assertion (e.g., use case-insensitive verification or a broader wildcard).
Proposed change (lowercase the property identifiers in the message):
-throw new InvalidOperationException( - "Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys." -); +throw new InvalidOperationException( + "Invalid DecodeOptions: decodeDotInKeys=true requires allowDots=true when decoding keys." +);Also applies to: 4350-4354
QsNet/Qs.cs (2)
256-263: Micro-optimization: chained Append over interpolationChaining Append for the charset sentinel avoids a transient string. Low risk; fine as-is.
256-267: Optional: avoid trailing ampersand when only sentinel is emittedIf CharsetSentinel is enabled and there are no parts (joined is empty), the output ends with “&” (this was already the case before the change). Consider omitting the trailing “&” for a slightly cleaner result:
- if (opts.CharsetSentinel) + if (opts.CharsetSentinel) { if (opts.Charset.WebName.Equals("iso-8859-1", StringComparison.OrdinalIgnoreCase)) - sb.Append(Sentinel.Iso.GetEncoded()).Append('&'); + sb.Append(Sentinel.Iso.GetEncoded()).Append(joined.Length > 0 ? "&" : ""); else if (opts.Charset.WebName.Equals("utf-8", StringComparison.OrdinalIgnoreCase)) - sb.Append(Sentinel.Charset.GetEncoded()).Append('&'); + sb.Append(Sentinel.Charset.GetEncoded()).Append(joined.Length > 0 ? "&" : ""); }QsNet/Models/DecodeOptions.cs (2)
199-202: Conflict validation now throws InvalidOperationException — good, but align message casing with testsThe exception type aligns with the standardized approach. To avoid fragile assertions around casing, consider using lowercase option names in the message (or change the test to be case-insensitive). Suggested tweak:
- "Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys." + "Invalid DecodeOptions: decodeDotInKeys=true requires allowDots=true when decoding keys."
210-224: Key decoder contract clarification — niceExplicitly requiring string or null for keys and throwing InvalidOperationException otherwise tightens guarantees. Consider adding a short remark in the XML summary on Decoder/KindAwareDecoder delegates noting the key-return contract to help custom decoder implementers.
I can add a brief XML comment update for the delegates if you want.
QsNet/Internal/Decoder.cs (2)
300-335: Synthetic double-bracket remainder trimming: add guardrails/testsThe post-unwrapping trimming (opens > closes and ends with ‘]’) correctly targets cases like "[[b[c]]" so the inner literal becomes "[b[c". Two suggestions:
- Extract this into a small helper with a unit test matrix (e.g., "[[c][d]]", "[[b[c]]", "[][x]]", balanced inputs) for maintainability and to prevent regressions.
- Add comments explaining why opens > closes implies the trailing ‘]’ is synthetic.
I can contribute targeted tests under Split Key Segmentation Remainder Tests if helpful.
657-686: Enumerator join helper: small micro-optimization optionalJoinAsCommaSeparatedStrings could pre-size the StringBuilder with a small constant (e.g., 32) to reduce allocations for tiny lists; not critical.
📜 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 (9)
QsNet.Tests/DecodeOptionsTests.cs(1 hunks)QsNet.Tests/DecodeTests.cs(12 hunks)QsNet.Tests/EncodeTests.cs(3 hunks)QsNet/Internal/Decoder.cs(9 hunks)QsNet/Internal/Encoder.cs(2 hunks)QsNet/Internal/Utils.cs(8 hunks)QsNet/Models/DecodeOptions.cs(2 hunks)QsNet/Qs.cs(3 hunks)QsNet/QsNet.csproj(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
QsNet/Qs.cs (1)
QsNet/Enums/Sentinel.cs (2)
GetEncoded(50-58)ToString(65-68)
QsNet.Tests/EncodeTests.cs (2)
QsNet/Qs.cs (3)
Dictionary(30-117)Qs(20-280)Encode(126-279)QsNet/Internal/Encoder.cs (1)
Encode(42-428)
⏰ 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 Static Code Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (19)
QsNet/Internal/Encoder.cs (1)
89-90: No further doc updates required for cycle exceptionI verified that the only cycle check in QsNet/Internal/Encoder.cs now throws an InvalidOperationException("Cyclic object value") and there are no XML docs or public‐API comments still referencing IndexOutOfRangeException for cyclic object values. All remaining ArgumentException and IndexOutOfRangeException entries in the codebase correspond to other, distinct error conditions (invalid input types, limit overflows, etc.) and should remain unchanged.
QsNet/Internal/Utils.cs (5)
191-193: Invariant culture for index keys into maps: LGTM.Using i.ToString(CultureInfo.InvariantCulture) ensures locale-agnostic keys and matches the PR goal.
236-236: Invariant culture for list-to-dictionary merge keys: LGTM.Consistent with the rest of the invariant-culture changes.
299-302: Hex formatting now invariant: LGTM.ToString("X2"/"X4", InvariantCulture) avoids locale surprises in Escape output.
330-357: Invariant hex parsing in Unescape across TFMs: LGTM.int.TryParse(..., NumberStyles.HexNumber, CultureInfo.InvariantCulture, ...) is the right fix; also good job using Span on modern TFMs.
410-413: Invariant hex parsing when converting %uXXXX to numeric entities: LGTM.This keeps ISO-8859-1 sentinel mode stable across locales.
QsNet.Tests/DecodeOptionsTests.cs (1)
124-126: Test updated to expect InvalidOperationException: LGTM.The assertion aligns with the standardized error type and still verifies the option names in the message.
QsNet.Tests/EncodeTests.cs (1)
1794-1794: Updated expectations to InvalidOperationException for circular/self-referential inputs: LGTM.These assertions now match Encoder’s behavior and the PR objective.
Also applies to: 1800-1800, 3408-3408, 3419-3419
QsNet.Tests/DecodeTests.cs (2)
273-277: Exception type migration to InvalidOperationException looks correctUsing InvalidOperationException here aligns with the PR’s objective of standardizing limit-related failures. Message remains stable, which is good for consumers relying on it.
2407-2409: Parameter/List limit tests updated to InvalidOperationException — good coverageThe migration to InvalidOperationException for parameter limit exceedance, list limit exceedance, negative list limit, and nested list overflow test cases stays consistent with the core changes.
Also applies to: 2485-2487, 2531-2534, 2541-2543
QsNet/Qs.cs (2)
6-6: Culture-invariant index formatting import — goodAdding System.Globalization sets up for consistent ToString(CultureInfo.InvariantCulture) usage.
270-277: InvariantCulture for index keys — correct and necessaryUsing invariant ToString for auto-indexed enumerable encoding prevents locale-dependent digits.
QsNet/Models/DecodeOptions.cs (1)
238-241: DefaultDecode now returns string? — compatible and clearerReturning string? is more precise and still upcasts to object? at call sites. No external API break since it’s private.
QsNet/Internal/Decoder.cs (6)
49-56: List limit violations now throw InvalidOperationException — consistent and message-stableException type and message align with the tests and PR goals.
Also applies to: 59-63
107-111: Parameter limit overflow standardized to InvalidOperationExceptionMatches the test updates and improves semantic clarity over IndexOutOfRangeException.
191-196: Explicit “[]=“ detection uses ordinal comparisons per target — goodThe NETSTANDARD2_0 IndexOf and modern Contains(StringComparison.Ordinal) keep behavior stable across TFMs.
347-351: Culture-invariant numeric check for bracketed indices — correctComparing idx.ToString(CultureInfo.InvariantCulture) to the decoded token avoids locale pitfalls.
429-488: Dot-to-bracket conversion is careful with degenerate cases — goodTop-level '.' handling respects:
- double dots → literal first dot
- trailing dot → literal dot
- “.[” → skips the dot
This matches the new tests under “Top-level dot guardrails & depth remainder”.
578-599: Remainder handling: strictDepth overflow vs unterminated groups separated — correct
- Throws for well‑formed overflow when strictDepth is true.
- Doesn’t throw for unterminated segments; wraps the remainder once.
- Special-cases keys starting with '[' to return the original literal.
This matches the updated tests and provides robust parsing.
… for limit and option violations in Decode and Encode methods
… for limit and depth violations in Decoder methods
…stency across frameworks
…ferences and enhance preprocessor formatting
…ty for charset sentinels
…s for key decoding
… in key segment parsing
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)
QsNet/Internal/Decoder.cs (2)
49-57: Fix cumulative list-limit check for comma-split values (can exceed ListLimit).When Comma is enabled and value contains commas, the code checks splitVal.Length against ListLimit but ignores currentListLength. This allows exceeding the limit when appending comma-split values to an existing list.
Apply this diff to enforce the cumulative limit:
- var splitVal = str.Split(','); - if (options.ThrowOnLimitExceeded && splitVal.Length > options.ListLimit) + var splitVal = str.Split(','); + if (options.ThrowOnLimitExceeded && currentListLength + splitVal.Length > options.ListLimit) throw new InvalidOperationException( $"List limit exceeded. Only {options.ListLimit} element{(options.ListLimit == 1 ? "" : "s")} allowed in a list." ); return splitVal.ToList<object?>();Consider adding tests:
- existing list length = N, incoming comma-split length = M, N + M == limit → allowed
- N + M == limit + 1 and ThrowOnLimitExceeded = true → throws InvalidOperationException
Also applies to: 59-65
227-233: Fix CA1859 build failure: change ParseObject parameter to List.The pipeline fails with CA1859 (treating analyzer warnings as errors). Since SplitKeyIntoSegments already returns List and ParseObject is private, switching the parameter type is safe and resolves the warning.
Apply this diff:
- private static object? ParseObject( - IReadOnlyList<string> chain, + private static object? ParseObject( + List<string> chain, object? value, DecodeOptions options, bool valuesParsed ) @@ - return ParseObject(segments, value, options, valuesParsed); + return ParseObject(segments, value, options, valuesParsed);Alternative: if you intentionally prefer IReadOnlyList for API intent, you can suppress CA1859 locally with a pragma or [SuppressMessage] attribute, but changing the type is simplest here.
Also applies to: 417-418
🧹 Nitpick comments (7)
QsNet/QsNet.csproj (1)
22-37: Packaging/diagnostics hardening LGTM; consider adding a package iconThe new SourceLink, symbols, determinism, analyzers, and CI-guarded Werror settings are on point. Optional: add a package icon so the NuGet gallery shows branding.
Example (requires adding an icon file at repo root or project folder):
<PropertyGroup> + <PackageIcon>icon.png</PackageIcon> </PropertyGroup> <ItemGroup> + <None Include="..\icon.png" Pack="true" PackagePath="\" /> </ItemGroup>QsNet.Tests/UtilsTests.cs (1)
1305-1337: Broaden hex-entity coverage (upper ‘X’, max range, empty digits)The new tests validate lowercase hex and surrogate-pair decoding. To harden coverage, add:
- Uppercase ‘X’ prefix: A -> A
- Max valid hex: � -> U+10FFFF
- Out-of-range hex remains unchanged: �
- Empty digits after x/X: &#x; and &#X; remain unchanged
Suggested additional tests (place near the existing hex tests):
[Fact] public void InterpretNumericEntities_DecodesSingleHexEntity_UppercaseX() { Utils.InterpretNumericEntities("A").Should().Be("A"); } [Fact] public void InterpretNumericEntities_AcceptsMaxValidHexAndRejectsBeyond() { // U+10FFFF is valid Utils.InterpretNumericEntities("").Should().Be(char.ConvertFromUtf32(0x10FFFF)); // One above max should remain unchanged Utils.InterpretNumericEntities("�").Should().Be("�"); } [Fact] public void InterpretNumericEntities_EmptyHexDigitsRemainUnchanged() { Utils.InterpretNumericEntities("&#x;").Should().Be("&#x;"); Utils.InterpretNumericEntities("&#X;").Should().Be("&#X;"); }QsNet/Internal/Utils.cs (3)
411-415: Encode (latin-1 path): invariant parse of %uXXXX — good; consider avoiding obsolete EscapeCurrent approach is correct functionally. Optional: avoid the obsolete Escape call by computing the code points directly and replacing %uXXXX with numeric entities in one pass to reduce allocations and remove the Obsolete API dependency.
810-842: InterpretNumericEntities: avoid per-digit allocations and guard against overflowThe decoder now supports hex entities — great. Two improvements:
- Convert the entire digits span once (via int.TryParse on a ReadOnlySpan) instead of Convert.ToInt32 on each digit to cut allocations.
- Guard against excessively long sequences that can overflow int during accumulation (malformed input like �). Parsing the span with TryParse naturally handles this; otherwise, bail out and leave the input unchanged.
Proposed refactor:
- var code = 0; - var startDigits = j; - var hex = false; - if (str[j] is 'x' or 'X') { hex = true; j++; startDigits = j; } - while (j < n && (hex ? Uri.IsHexDigit(str[j]) : char.IsDigit(str[j]))) - { - code = hex - ? (code << 4) + Convert.ToInt32(str[j].ToString(), 16) - : code * 10 + (str[j] - '0'); - j++; - } - - if (j < n && str[j] == ';' && j > startDigits) + var startDigits = j; + var hex = false; + if (str[j] is 'x' or 'X') { hex = true; j++; startDigits = j; } + while (j < n && (hex ? Uri.IsHexDigit(str[j]) : char.IsDigit(str[j]))) j++; + if (j < n && str[j] == ';' && j > startDigits) { - switch (code) + ReadOnlySpan<char> digits = str.AsSpan(startDigits, j - startDigits); + if (!int.TryParse(digits, + hex ? System.Globalization.NumberStyles.HexNumber : System.Globalization.NumberStyles.None, + System.Globalization.CultureInfo.InvariantCulture, + out var code)) + { + // leave as-is + sb.Append(ch); + i++; + continue; + } + switch (code) { case <= 0xFFFF: sb.Append((char)code); break; case <= 0x10FFFF: sb.Append(char.ConvertFromUtf32(code)); break; default: sb.Append('&'); i++; continue; }
468-476: Surrogate pair emission: validate low surrogate before 4-byte pathToday, any char in the surrogate range takes the 4-byte path without checking for a valid low surrogate, which can emit invalid UTF-8 for malformed input. Optional guard:
- // 4 bytes (surrogate pair) - var nextC = i + 1 < segment.Length ? segment[i + 1] : 0; - var codePoint = 0x10000 + (((c & 0x3FF) << 10) | (nextC & 0x3FF)); + // 4 bytes (surrogate pair) – only if valid pair; otherwise treat as 3-byte fallback + if (i + 1 >= segment.Length || !char.IsSurrogatePair(segment[i], segment[i + 1])) + { + // Fallback: percent-encode the single surrogate code unit to remain lossless + buffer.Append(HexTable.Table[0xE0 | (c >> 12)]); + buffer.Append(HexTable.Table[0x80 | ((c >> 6) & 0x3F)]); + buffer.Append(HexTable.Table[0x80 | (c & 0x3F)]); + i++; + continue; + } + var nextC = segment[i + 1]; + var codePoint = char.ConvertToUtf32((char)c, nextC);This mirrors the JS behavior of losslessly encoding lone surrogates rather than producing an invalid sequence.
QsNet/Internal/Decoder.cs (2)
350-355: Make numeric parsing culture-invariant, too.Formatting uses CultureInfo.InvariantCulture; mirror that in parsing to avoid locale surprises with signs/whitespace.
Apply this diff:
- var isPureNumeric = - int.TryParse(decodedRoot, out var idx) && !string.IsNullOrEmpty(decodedRoot); + var isPureNumeric = + !string.IsNullOrEmpty(decodedRoot) + && int.TryParse(decodedRoot, System.Globalization.NumberStyles.Integer, System.Globalization.CultureInfo.InvariantCulture, out var idx);
21-27: Minor: cache Latin1Encoding for NETSTANDARD2_0.Encoding.GetEncoding(28591) may be slightly costlier if accessed frequently. Consider caching in a static readonly field under NETSTANDARD2_0.
Example:
- private static Encoding Latin1Encoding => -#if NETSTANDARD2_0 - Encoding.GetEncoding(28591); -#else - Encoding.Latin1; -#endif + private static readonly Encoding Latin1Encoding = +#if NETSTANDARD2_0 + Encoding.GetEncoding(28591); +#else + Encoding.Latin1; +#endif
📜 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 (7)
QsNet.Tests/UtilsTests.cs(2 hunks)QsNet/Internal/Decoder.cs(11 hunks)QsNet/Internal/Encoder.cs(2 hunks)QsNet/Internal/Utils.cs(9 hunks)QsNet/Models/DecodeOptions.cs(4 hunks)QsNet/Qs.cs(5 hunks)QsNet/QsNet.csproj(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QsNet/Internal/Encoder.cs
🧰 Additional context used
🧬 Code graph analysis (2)
QsNet.Tests/UtilsTests.cs (1)
QsNet/Internal/Utils.cs (2)
Utils(20-1180)InterpretNumericEntities(788-851)
QsNet/Qs.cs (1)
QsNet/Enums/Sentinel.cs (2)
GetEncoded(50-58)ToString(65-68)
🪛 GitHub Actions: Test
QsNet/Internal/Decoder.cs
[error] 228-228: dotnet build --configuration Release --no-restore failed: CA1859: Change type of parameter 'chain' from 'System.Collections.Generic.IReadOnlyList' to 'System.Collections.Generic.List' for improved performance (https://learn.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1859).
⏰ 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). (2)
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (23)
QsNet/QsNet.csproj (3)
10-15: Richer, multi-line PackageTags look goodThe expanded, categorized tags will help discoverability on NuGet. No issues spotted.
39-41: net8-only ImplicitUsings is a sensible tradeoffLeaving ImplicitUsings off for netstandard2.0 avoids noise. Looks good.
48-50: SourceLink reference is correct and scopedMicrosoft.SourceLink.GitHub v8.0.0 with PrivateAssets=All is appropriate for these targets.
QsNet.Tests/UtilsTests.cs (1)
1288-1290: Updated expectation for hex entities is correctAcknowledging that hex numeric entities are now supported makes this test consistent with the new decoder behavior.
QsNet/Qs.cs (4)
29-29: Doc: standardized to InvalidOperationException — goodThe Decode XML docs now reflect the standardized exception type for limit violations. Matches PR objective.
125-125: Doc: Encode also standardized to InvalidOperationExceptionConsistent with the encoder/decoder changes; good alignment.
260-263: Avoid trailing ‘&’ after charset sentinelConditionally appending “&” only when there are subsequent parts prevents dangling delimiters. Nice polish.
276-276: InvariantCulture for index keys is the right callUsing ToString(CultureInfo.InvariantCulture) for numeric indices ensures locale independence across platforms.
QsNet/Models/DecodeOptions.cs (4)
15-19: Key-decoder contract clarifiedRemarks make the “key must be string or null” rule explicit, which prevents subtle downstream type issues. Good documentation tightening.
31-34: Kind-aware decoder key contract clarifiedSame commendation here; explicit contract reduces ambiguity for custom decoders.
206-208: Switch to InvalidOperationException on option conflict is appropriateThrowing InvalidOperationException for incompatible options (DecodeDotInKeys vs AllowDots) matches the PR’s exception standardization.
244-247: DefaultDecode returns string? — consistent and saferReturning string? from the default path aligns with Utils.Decode and simplifies key-path enforcement. No issues.
QsNet/Internal/Utils.cs (4)
299-302: Hex formatting should be invariant — goodUsing ToString("X2"/"X4", CultureInfo.InvariantCulture) removes locale variance from Escape’s output.
331-358: Unescape: invariant hex parsing — goodPassing CultureInfo.InvariantCulture to int.TryParse for both %uXXXX and %XX paths is correct and cross-locale safe.
191-193: InvariantCulture for index-stringification in mergesConverting indices with InvariantCulture both when merging into maps and when list→map promotion occurs keeps dictionary key shape stable across locales.
Also applies to: 236-236
792-799: Fast-path ‘&#’ presence checks are correct per target frameworkOrdinal IndexOf/Contains split by TFMs is appropriate for netstandard2.0 vs newer frameworks. Good.
QsNet/Internal/Decoder.cs (7)
108-111: Standardized exception type for parameter overflow — LGTM.Switching to InvalidOperationException on parameter overflow aligns with the PR’s error semantics. Message is clear and pluralization is handled.
300-305: Synthetic-wrapper trimming logic looks sound; please add edge-case tests.The opens>closes heuristic to trim a leaked trailing ']' from the inner slice (after removing the outer synthetic wrapper) is reasonable. To guard against regressions, add cases such as:
- original: "a[[b[c" (unterminated remainder wrapped once) → segment becomes "[b[c"
- original: "a[[b[c]]d" with strictDepth=false → verify resulting segments
- deeply nested balanced groups followed by unterminated: "a[b[c]d[e" → behavior per comments
I can generate targeted unit tests for SplitKeyIntoSegments if useful. Do you want me to open a follow-up PR with those?
Also applies to: 307-338
433-439: Efficient dot short-circuit for NETSTANDARD2_0 — LGTM.Using IndexOf('.') < 0 avoids allocations and keeps behavior consistent across TFMs.
586-589: Depth overflow semantics switched to InvalidOperationException — LGTM.Exception type now matches the PR objective and the message is actionable.
Also applies to: 615-617
191-195: Validate behavior when "[]=" appears alongside comma-split values.Wrapping non-string IEnumerable into a one-element list preserves identity, but combined with ParseListValue (which may return List<object?> for comma splits) this can yield nested lists for inputs like a[]=1,2. If that’s the desired semantics, fine; otherwise, flatten once when "[]=" is present.
Would you like me to draft tests for:
- "a[]=1&a[]=2" (no comma)
- "a[]=1,2" with Comma=true
- "a[]=1,2&a[]=3" with Comma=true
…to assert expected nesting/flattening?
6-6: Import for CultureInfo — LGTM.Necessary for invariant numeric formatting introduced below.
73-75: Doc comment updated for new exception type — LGTM.Documentation now reflects InvalidOperationException when ThrowOnLimitExceeded is true.
…ting comma-separated values
…and empty hex digits in InterpretNumericEntities
…ns and handle overflow
…s for lossless encoding
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)
QsNet/Internal/Utils.cs (1)
468-485: Prevent surrogate pairs from splitting across segment boundaries inUtils.EncodeThe current implementation in
QsNet/Internal/Utils.cs(around lines 426–550) usesSubstringto process the input in fixed‐size chunks (defined bySegmentLimit = 1024). It then checks surrogate pairs only within each chunk:
- If a high surrogate appears at the end of a chunk (
i + 1 >= segment.Length), it’s treated as “lone” and encoded as a 3-byte sequence, even if its matching low surrogate is the first character of the next chunk.- This causes valid astral code points (e.g. U+1F600) landing at
SegmentLimit-1/SegmentLimitto be mis-encoded.Locations needing fixes
- File:
QsNet/Internal/Utils.cs- Encoding loop starting at the declaration of
var segment = nonNullStr.Substring(j, ...)through the outerwhile (j < nonNullStr.Length).Recommended refactor
Replace the chunk-based loops with a single pass over the full string, using the overloadchar.IsSurrogatePair(string, index)to detect pairs across any boundary:@@ -424,30 +424,18 @@ - var buffer = new StringBuilder(); - var j = 0; - - while (j < nonNullStr.Length) - { - var segment = - nonNullStr.Length >= SegmentLimit - ? nonNullStr.Substring(j, Math.Min(SegmentLimit, nonNullStr.Length - j)) - : nonNullStr; - - var i = 0; - while (i < segment.Length) - { - var c = (int)segment[i]; - … - // 4 bytes (surrogate pair) – only if valid pair; otherwise treat as 3-byte fallback - if (i + 1 >= segment.Length || !char.IsSurrogatePair(segment[i], segment[i + 1])) - { - // encode lone surrogate as 3-byte sequence - … - i++; - continue; - } - var nextC = segment[i + 1]; - var codePoint = char.ConvertToUtf32((char)c, nextC); - // emit 4-byte sequence - … - i += 2; - } - - j += SegmentLimit; - } + // Single-pass encoding without chunking + var buffer = new StringBuilder(); + for (int i = 0; i < nonNullStr.Length;) + { + if (char.IsSurrogatePair(nonNullStr, i)) + { + int codePoint = char.ConvertToUtf32(nonNullStr[i], nonNullStr[i + 1]); + // append 4-byte UTF-8 percent-encoded sequence + buffer.Append(HexTable.Table[0xF0 | (codePoint >> 18)]); + buffer.Append(HexTable.Table[0x80 | ((codePoint >> 12) & 0x3F)]); + buffer.Append(HexTable.Table[0x80 | ((codePoint >> 6) & 0x3F)]); + buffer.Append(HexTable.Table[0x80 | (codePoint & 0x3F)]); + i += 2; + } + else + { + int c = nonNullStr[i]; + // handle ASCII, 2-byte, 3-byte, and lone surrogates (WTF-8 fallback) + … + i++; + } + }Add a regression test to catch this boundary case:
// Ensure astral characters at SegmentLimit-1/SegmentLimit encode as 4-byte sequences var s = new string('a', SegmentLimit - 1) + "\U0001F600" + "b"; var encoded = Utils.Encode(s, Encoding.UTF8, Format.Rfc3986); Assert.Contains("%F0%9F%98%80", encoded);This change restores correct 4-byte UTF-8 emission for all valid surrogate pairs and preserves the 3-byte fallback for isolated surrogates.
QsNet/Internal/Decoder.cs (1)
243-261: Bug risk: incorrect parent index detection when chain ends with "[]"parentKeyStr is built by concatenating all prior segments, e.g. "a[0]" for chain ["a", "[0]", "[]"]. int.TryParse on "a[0]" will always fail, so currentListLength stays 0 and nested list limit checks may undercount.
Parse the immediate parent segment only (chain[^2]), strip brackets, and TryParse that.
- string parentKeyStr; - if (chain.Count > 1) - { - var sbTmp = new StringBuilder(); - for (var t = 0; t < chain.Count - 1; t++) sbTmp.Append(chain[t]); - parentKeyStr = sbTmp.ToString(); - } - else - { - parentKeyStr = string.Empty; - } - - if ( - int.TryParse(parentKeyStr, NumberStyles.Integer, CultureInfo.InvariantCulture, out var parentKey) - && value is IList<object?> list - && parentKey < list.Count - ) - currentListLength = (list[parentKey] as IList<object?>)?.Count ?? 0; + // Look only at the immediate parent segment, e.g. "[0]" in ["a", "[0]", "[]"] + if (chain.Count > 1) + { +#if NETSTANDARD2_0 + var parentSeg = chain[chain.Count - 2]; +#else + var parentSeg = chain[^2]; +#endif + if (parentSeg.Length >= 2 && parentSeg[0] == '[' && parentSeg[parentSeg.Length - 1] == ']') + { +#if NETSTANDARD2_0 + var idxStr = parentSeg.Substring(1, parentSeg.Length - 2); +#else + var idxStr = parentSeg[1..^1]; +#endif + if (int.TryParse(idxStr, NumberStyles.Integer, CultureInfo.InvariantCulture, out var parentIndex) + && value is IList<object?> incomingList + && parentIndex >= 0 + && parentIndex < incomingList.Count) + { + currentListLength = (incomingList[parentIndex] as IList<object?>)?.Count ?? 0; + } + } + }Please confirm this matches the intended nested list behavior and limits; I can push a follow-up PR if needed.
🧹 Nitpick comments (9)
QsNet/Internal/Utils.cs (1)
819-873: Robust, culture-invariant numeric entity parsing; a couple of edge-case considerations.What’s great:
- Zero-allocation scanning with spans on newer TFMs.
- Culture-invariant TryParse across hex/decimal.
- Graceful failure path that preserves input on overflow/invalid digits.
- Proper handling of BMP vs non-BMP code points.
Optional refinements:
- Consider treating surrogate-range code values (0xD800–0xDFFF) similar to your WTF‑8 stance: either preserve input unchanged (strict) or keep current behavior (append lone surrogate). Document the choice for consumers.
- If you want HTML5 compliance, only the “&#x...;” hex form is standard (your code correctly supports that). If support for non-standard forms is desired later, gate them behind an option.
No changes strictly required; just be explicit in docs/tests about intended behavior for surrogate-range numeric entities.
If you want, I can add focused tests for:
- Overflowing entities like �
- Surrogate-range entities like � and mixed-case hex like 😀.
Also applies to: 833-848, 849-856
QsNet/Internal/Decoder.cs (4)
52-63: List-limit check now accounts for both existing and incoming comma-split items (N + M) — correctUsing currentListLength + splitVal.Length ensures you enforce the limit across merges. Minor: consider guarding the non-comma path with the same message constant to avoid future string drift.
- if (options.ThrowOnLimitExceeded && currentListLength >= options.ListLimit) - throw new InvalidOperationException( - $"List limit exceeded. Only {options.ListLimit} element{(options.ListLimit == 1 ? "" : "s")} allowed in a list." - ); + if (options.ThrowOnLimitExceeded && currentListLength >= options.ListLimit) + throw new InvalidOperationException( + $"List limit exceeded. Only {options.ListLimit} element{(options.ListLimit == 1 ? "" : "s")} allowed in a list." + );
191-197: Behavior of "[]=" with Comma: confirm intended nesting for single occurrenceWhen the part contains "[]=", value is wrapped as a single element if it’s IEnumerable (and not string):
value = value is IEnumerable and not string ? new List<object?> { value } : value;With Comma=true, a single occurrence like a[]=1,2,3 will make value a List, which then gets wrapped into List<object?> { List }, yielding a nested list for a single bracketed occurrence. The new tests cover multiple parts (which should indeed produce a list of lists), but not the single-occurence case.
Is the nested result for a single a[]=1,2,3 intentional? If you intend a flat list for a single bracketed comma value, consider flattening when no previous value exists for that key.
Example tweak (flatten only when first occurrence):
- if (part.Contains("[]=", StringComparison.Ordinal)) - value = value is IEnumerable and not string ? new List<object?> { value } : value; + if (part.Contains("[]=", StringComparison.Ordinal)) + { + // If this is the first occurrence for this key, keep comma-split values flat. + var isFirstForKey = !obj.ContainsKey(key); + if (!isFirstForKey && value is IEnumerable and not string) + value = new List<object?> { value }; + }Would you like me to add a targeted test for a single a[]=1,2,3 to lock this down?
350-356: Micro: avoid TryParse on empty strings firstYou call int.TryParse before checking for empty. Swap the order to skip parsing on empty and reduce allocations/branches slightly.
- var isPureNumeric = - int.TryParse(decodedRoot, NumberStyles.Integer, CultureInfo.InvariantCulture, out var idx) && - !string.IsNullOrEmpty(decodedRoot); + var isPureNumeric = + !string.IsNullOrEmpty(decodedRoot) && + int.TryParse(decodedRoot, NumberStyles.Integer, CultureInfo.InvariantCulture, out var idx);
504-621: Depth handling and error semantics look right; messages could be centralized
- InvalidOperationException for strictDepth overflow is consistent.
- Unterminated groups do not trigger strictDepth exceptions and yield a wrapped remainder — matches tests.
Optional: extract the repeated error message into a const to avoid drift.
+ private const string StrictDepthExceededMsg = "Input depth exceeded depth option of {0} and strictDepth is true"; ... - throw new InvalidOperationException( - $"Input depth exceeded depth option of {maxDepth} and strictDepth is true" - ); + throw new InvalidOperationException(string.Format(StrictDepthExceededMsg, maxDepth)); ... - if (strictDepth) - throw new InvalidOperationException( - $"Input depth exceeded depth option of {maxDepth} and strictDepth is true" - ); + if (strictDepth) + throw new InvalidOperationException(string.Format(StrictDepthExceededMsg, maxDepth));QsNet.Tests/DecodeTests.cs (4)
2408-2410: ParameterLimit exceedance should assert the message for stronger contractCurrently you assert only the exception type. Consider also asserting the message "Parameter limit exceeded. Only {limit} parameter(s) allowed." to lock the behavior, as you already do for list-limit.
- Action act = () => Qs.Decode("a=1&b=2&c=3&d=4&e=5&f=6", options); - act.Should().Throw<InvalidOperationException>(); + Action act = () => Qs.Decode("a=1&b=2&c=3&d=4&e=5&f=6", options); + act.Should() + .Throw<InvalidOperationException>() + .WithMessage("Parameter limit exceeded. Only 3 parameter*allowed.");(Use a wildcard for pluralization to avoid brittleness.)
2486-2488: ListLimit exceedance test: type-only assertion is fine; consider asserting message tooMirroring the comma-list test would make failures easier to diagnose.
- Action act = () => Qs.Decode("a[]=1&a[]=2&a[]=3&a[]=4", options); - act.Should().Throw<InvalidOperationException>(); + Action act = () => Qs.Decode("a[]=1&a[]=2&a[]=3&a[]=4", options); + act.Should().Throw<InvalidOperationException>() + .WithMessage("List limit exceeded. Only 3 elements allowed in a list.");
4686-4724: New tests for comma split vs existing list length: great boundary coverage
- N + M == limit allowed.
- N + M > limit throws when ThrowOnLimitExceeded is true.
Consider adding an additional test where ThrowOnLimitExceeded = false to ensure silent truncation or well-defined behavior when combining parts pushes over the limit.
+ [Fact] + public void Decode_CommaSplit_TruncatesWhenSumExceedsLimit_AndThrowOff() + { + var opts = new DecodeOptions { Comma = true, ListLimit = 3, ThrowOnLimitExceeded = false, Duplicates = Duplicates.Combine }; + var result = Qs.Decode("a=1,2&a=3,4,5", opts); + // Define expected behavior explicitly: either first 3, or last 3, or exactly how ParseListValue truncates upstream. + // Assert accordingly once decided. + }Would you like me to pin the exact truncation semantics and add an assertion?
1714-1766: Array-of-arrays with comma-split: confirms desired nesting semanticsThis suite validates that each []= occurrence can carry a flat list and that duplicates produce a list of lists. Consider adding the single-occurrence case a[]=1,2,3 to explicitly document expected behavior (see Decoder comment).
+ [Fact] + public void Decode_BracketSingle_CommaSplit_DefinesSingleOccurrenceBehavior() + { + var opts = new DecodeOptions { Comma = true }; + var res = Qs.Decode("a=1,2,3", opts); // control + var res2 = Qs.Decode("a[]=1,2,3", opts); // bracketed + // Decide and assert: flat ["1","2","3"] or nested [["1","2","3"]] + }
📜 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)
QsNet.Tests/DecodeTests.cs(14 hunks)QsNet.Tests/UtilsTests.cs(2 hunks)QsNet/Internal/Decoder.cs(14 hunks)QsNet/Internal/Utils.cs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QsNet.Tests/UtilsTests.cs
🧰 Additional context used
🧬 Code graph analysis (2)
QsNet/Internal/Utils.cs (1)
QsNet/Constants/HexTable.cs (1)
HexTable(6-41)
QsNet.Tests/DecodeTests.cs (4)
QsNet/Models/DecodeOptions.cs (4)
DecodeOptions(40-321)DecodeOptions(49-63)DecodeOptions(274-320)Decode(203-214)QsNet/Internal/Decoder.cs (2)
Dictionary(75-217)List(505-622)QsNet/Internal/Utils.cs (9)
Dictionary(535-676)Dictionary(889-895)Dictionary(902-911)Dictionary(918-930)Dictionary(983-986)Dictionary(994-1060)Dictionary(1089-1210)Decode(499-527)List(684-712)QsNet/Qs.cs (2)
Dictionary(30-117)Qs(20-280)
⏰ 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). (2)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (15)
QsNet/Internal/Utils.cs (5)
191-193: Good move: use culture-invariant index keys when materializing list indices into maps.Switching to i.ToString(CultureInfo.InvariantCulture) ensures ASCII digits regardless of the current UI culture. This removes subtle bugs in locales with non-Latin native digits and stabilizes serialized shapes. No further action needed.
Also applies to: 235-237
299-302: Hex formatting now culture-invariant — aligned with intent.Using ToString("X2", CultureInfo.InvariantCulture)/ToString("X4", CultureInfo.InvariantCulture) guarantees consistent percent-encoding for ASCII and Unicode escapes across cultures. Looks correct.
331-341: Hex parsing in Unescape standardized to InvariantCulture.The TryParse calls for both %uXXXX and %XX are now culture-invariant and branch correctly for NETSTANDARD2_0 vs newer. This removes locale surprises and avoids allocation in newer TFMs via spans. Good change.
Also applies to: 354-358
411-415: Invariant hex parse in ISO-8859-1 branch of Encode is consistent.Parsing match.Value’s hex digits with InvariantCulture keeps behavior deterministic across cultures. Implementation looks solid.
801-807: Efficient pre-check for numeric entities with TFM-appropriate APIs.Using IndexOf for NETSTANDARD2_0 and Contains with StringComparison.Ordinal otherwise is the right tradeoff to avoid unnecessary work. Looks good.
QsNet/Internal/Decoder.cs (5)
21-33: Good: consolidate Latin‑1 handling and runtime checkSwitching Latin1Encoding to a private static readonly field and using a helper IsLatin1 for platform-conditional checks is clean and avoids repeated calls. This also keeps NETSTANDARD2_0 parity.
74-111: Parameter limit: exception standardization and truncation logic look consistent
- Throws ArgumentException for non-positive limit (input validation).
- Throws InvalidOperationException when ThrowOnLimitExceeded is true and the count exceeds the limit; otherwise silently truncates.
All aligned with the PR objective and tests.
300-338: Bracket-wrapper leak fix: trimming synthetic trailing ']' is a solid edge-case recoveryThe explanation and guard (opens > closes and inner ends with ']') are careful and avoid breaking balanced groups. Nice resilience improvement for unterminated inputs.
432-493: DotToBracketTopLevel: guardrails for degenerate cases are well thought-out
- Skipping ".[" avoids creating empty segments.
- Preserving a literal '.' for trailing/double dots and ignoring trailing '.' later in SplitKeyIntoSegments gives predictable results.
- NETSTANDARD2_0 vs newer Contains/IndexOf split is correct.
No changes requested.
632-655: Efficient ordinal, case-insensitive replace for NETSTANDARD2_0: niceThe streaming approach with deferred StringBuilder allocation is efficient and avoids regex overhead on NS2.0.
QsNet.Tests/DecodeTests.cs (5)
276-278: Exception type update for comma list-limit exceedance matches production changeSwapping IndexOutOfRangeException to InvalidOperationException is consistent with Decoder.ParseListValue.
2280-2310: StrictDepth tests now expect InvalidOperationException — aligned with SplitKeyIntoSegmentsAll four scenarios cover objects, lists, mixed, and differing types. Good boundary coverage.
2533-2544: Negative ListLimit with ThrowOnLimitExceeded: good guardrailAsserting InvalidOperationException for negative limits exercises the non-comma path too.
2686-2700: Invalid options combination test is on-point (DecodeDotInKeys requires AllowDots)The message wildcard check already accounts for wording variations; thanks for making it resilient.
4470-4617: Top-level dot guardrails and remainder handling tests provide strong coverage
- Leading dot, double dot, trailing dot behavior.
- Depth interaction with dot-to-bracket conversion.
- Unterminated bracket under strictDepth.
These directly exercise the complex paths in DotToBracketTopLevel and SplitKeyIntoSegments. Nicely done.
…currence decoding behavior
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
QsNet/QsNet.csproj (1)
39-41: Minor: consider aligning tooling features across TFMsYou enable ImplicitUsings only for net8.0. If you want identical compile experiences across TFMs, consider enabling it conditionally when supported (keeping it disabled on NETSTANDARD2_0), or explicitly documenting the difference.
<PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'"> - <ImplicitUsings>enable</ImplicitUsings> + <ImplicitUsings>enable</ImplicitUsings> </PropertyGroup> +<!-- Optionally: for other TFMs where supported in future --> +<!-- <PropertyGroup Condition="'$(TargetFramework)' == 'netX.Y'"> --> +<!-- <ImplicitUsings>enable</ImplicitUsings> --> +<!-- </PropertyGroup> -->Also applies to: 48-50
QsNet/Internal/Decoder.cs (2)
235-263: Current-length discovery for nested [] relies on parent index and incoming list shapeThe parent-index probe is careful and culture-invariant. One limitation: if value isn’t an IList<object?> at this stage (e.g., custom enumerable), currentListLength remains 0 and the sum-check won’t reflect existing nested counts. If that scenario exists in inputs, you might want a defensive fallback (e.g., treat non-list enumerables as 1 item when Comma=false). Otherwise, this is acceptable.
435-496: Dot-to-bracket conversion: good guardrails for degenerate casesHandling of leading/trailing/double dots and “.[” cases at top-level is precise. Given the complex semantics, consider adding a short XML remark with examples (you already have inline comments) so IDE hovers capture it too.
QsNet/Internal/Utils.cs (1)
808-898: InterpretNumericEntities: robust scanning with invariant parsing and bounds checksEfficient early-outs, Span/Substring branches per TFM, and overflow handling are all solid. Minor nit: in the “> 0x10FFFF” path you append only ‘&’ and advance by one; this faithfully preserves input as “&#…;” on subsequent iterations, but a short code comment explaining the intent would help future readers.
- default: - sb.Append('&'); - i++; - continue; + default: + // Out-of-range codepoint: preserve input verbatim by emitting '&' + // and advancing one char; the subsequent loop iterations will append + // the remaining "#digits;" characters unchanged. + sb.Append('&'); + i++; + continue;QsNet.Tests/DecodeTests.cs (3)
4152-4177: Test name contradicts expected behaviorMethod name claims “EncodedDotDoesNotSplit”, but the expectations for both "a.b" and "a%2Eb" assert split into nested objects. Rename to reflect behavior.
-[Fact] -public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysTrue_PlainDotSplits_EncodedDotDoesNotSplit() +[Fact] +public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysTrue_PlainAndEncodedDotSplit()
4180-4197: Another misnamed test: encoded dot “remains percent sequence” but expected splitGiven the expectation, rename to indicate that encoded dot also splits when AllowDots=true (independent of DecodeDotInKeys for top-level).
-[Fact] -public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysFalse_EncodedDotRemainsPercentSequence() +[Fact] +public void EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysFalse_EncodedDotAlsoSplits()
4228-4245: Bracketed encoded dot: expected output assumes decoding to '.' even when DecodeDotInKeys=falsePer current implementation, percent-decoding of keys happens before structural splitting; bracket inner “%2E” will be decoded to '.' by key decoding. The test name “RemainsPercentSequence” is misleading; either rename or adjust expectation if you intend different semantics.
-[Fact] -public void EncodedDot_BracketSegment_RemainsPercentSequence_WhenDecodeDotInKeysFalse() +[Fact] +public void EncodedDot_BracketSegment_DecodesToDot_WhenDecodeDotInKeysFalse()
📜 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 (8)
QsNet.Tests/DecodeTests.cs(15 hunks)QsNet.Tests/Fixtures/DummyEnum.cs(1 hunks)QsNet.Tests/UtilsTests.cs(3 hunks)QsNet/Internal/Decoder.cs(14 hunks)QsNet/Internal/Utils.cs(11 hunks)QsNet/Models/DecodeOptions.cs(4 hunks)QsNet/Qs.cs(5 hunks)QsNet/QsNet.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- QsNet.Tests/Fixtures/DummyEnum.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- QsNet.Tests/UtilsTests.cs
- QsNet/Qs.cs
- QsNet/Models/DecodeOptions.cs
🧰 Additional context used
🧬 Code graph analysis (2)
QsNet.Tests/DecodeTests.cs (4)
QsNet/Models/DecodeOptions.cs (4)
DecodeOptions(40-321)DecodeOptions(49-63)DecodeOptions(274-320)Decode(203-214)QsNet/Qs.cs (2)
Qs(20-280)Dictionary(30-117)QsNet/Internal/Utils.cs (9)
Decode(510-538)Dictionary(546-687)Dictionary(905-911)Dictionary(918-927)Dictionary(934-946)Dictionary(999-1002)Dictionary(1010-1076)Dictionary(1105-1226)List(695-723)QsNet/Internal/Decoder.cs (2)
Dictionary(75-217)List(508-625)
QsNet/Internal/Utils.cs (1)
QsNet/Constants/HexTable.cs (1)
HexTable(6-41)
⏰ 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 (13)
QsNet/QsNet.csproj (1)
10-15: Packaging metadata hardening looks solidExpanded tags, SourceLink, deterministic build, analyzers, and CI-conditional flags are all good moves for package quality/reproducibility. No issues spotted.
Also applies to: 22-37
QsNet/Internal/Decoder.cs (4)
21-26: Static Latin1Encoding cache is correct and avoids repeated lookupsCaching Encoding.Latin1 / code page 28591 is a good micro-optimization and simplifies equality checks via IsLatin1().
49-57: Comma-split limit now accounts for existing length + incoming split (correct)The sum-based check fixes the common footgun where multiple comma-splits could bypass a per-part limit. The exception type standardization to InvalidOperationException matches the PR goals.
59-65: Edge case: ListLimit == 0 with ThrowOnLimitExceeded = true throws immediatelyThis condition makes any list addition invalid when ListLimit is 0 and ThrowOnLimitExceeded is true (currentListLength >= 0). If that’s intended, consider adding/adjusting a test that explicitly documents the behavior for (ListLimit=0, ThrowOnLimitExceeded=true) vs (ThrowOnLimitExceeded=false).
Would you like me to add a focused test to lock this in?
507-625: Depth/strictDepth overflow semantics are clearer and consistent
- Throws InvalidOperationException when strictDepth is true and overflow occurs.
- Remainder-wrapping logic for overflow/unterminated groups is well explained and should improve downstream reconstruction.
Nice improvement.
QsNet/Internal/Utils.cs (3)
191-196: Invariant-culture index keys in merges are the right callUsing i.ToString(CultureInfo.InvariantCulture) removes locale sensitivity from objectified list keys. Matches the PR’s culture-invariant objective.
Also applies to: 235-236
299-302: Hex formatting/parsing switched to invariant culture (correct)ToString("X2"/"X4", InvariantCulture) and int.TryParse(..., NumberStyles.HexNumber, InvariantCulture, ...) ensure stable behavior across locales.
Also applies to: 411-415, 354-358
427-499: UTF-8 encoding: surrogate-pair boundary handling and lone-surrogate fallback
- Segment boundary avoids splitting valid pairs.
- Valid pairs encode as 4-byte UTF-8.
- Lone surrogates emit 3-byte “code unit” encoding to remain lossless.
This mirrors web behavior and avoids exceptions on malformed input. Good trade-off.
QsNet.Tests/DecodeTests.cs (5)
4-4: LINQ import is justifiedUsed later to project list contents in new comma-limit tests.
272-278: Exception type updated to InvalidOperationException is consistent with decoder changesMessage assertion is precise; good.
2280-2309: All limit/overflow tests now expect InvalidOperationException (consistent)Aligns with the standardized error semantics in the implementation.
Also applies to: 2408-2410, 2486-2488, 2531-2535, 2540-2544
4368-4374: Good: invalid option combo throws InvalidOperationExceptionAllowDots=false with DecodeDotInKeys=true is invalid; test now asserts the right exception type and message pattern.
4705-4745: Comma-limit combined across duplicates: solid boundary tests
- Equal-to-limit aggregate allowed.
- Exceeding with ThrowOnLimitExceeded=true throws.
These lock in the new sum-check behavior.
…currence, and encoded dot splitting behavior
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 (1)
QsNet.Tests/DecodeTests.cs (1)
4221-4226: Fix message assertion pattern to match actual exception text
The thrown exception message is:Invalid DecodeOptions: DecodeDotInKeys=true requires AllowDots=true when decoding keys.The test’s
.WithMessagepattern currently uses lowercase and won’t match. Update it to use the correct casing and/or include the exact values.• File: QsNet.Tests/DecodeTests.cs (lines 4221–4226)
-act.Should().Throw<InvalidOperationException>() - .WithMessage("*decodeDotInKeys*allowDots*"); +act.Should().Throw<InvalidOperationException>() + .WithMessage("*DecodeDotInKeys*AllowDots*");Optional: tighten the pattern to assert the numeric values as well:
+ .WithMessage("*DecodeDotInKeys=true*AllowDots=true*");
♻️ Duplicate comments (1)
QsNet.Tests/DecodeTests.cs (1)
4131-4149: Resolved earlier placeholder: comma-split no-truncation with ThrowOn=false now has concrete expectations.This addresses the prior review about tests without assertions by validating full concatenation behavior under Duplicates.Combine. Nice.
🧹 Nitpick comments (4)
QsNet/QsNet.csproj (3)
10-15: NuGet tags look solid; consider a couple more discoverability terms.Current tags are comprehensive. You may optionally add: "query-parameters" and "http-utility" to improve search hits on NuGet. No action required if you prefer the current set.
35-36: Release notes link is helpful; consider adding PackageIcon.NuGet Gallery now favors embedded icons. If you have a project icon, add a PackageIcon and include the asset in the package.
Example change (adjust the icon path if/when you add it):
<PackageReadmeFile>README.md</PackageReadmeFile> + <PackageIcon>icon.png</PackageIcon>And include the file for packing:
<ItemGroup> <None Include="..\README.md" Pack="true" PackagePath="\"/> <None Include="..\LICENSE" Pack="true" PackagePath="\"/> + <!-- Adjust the path to where your icon actually lives --> + <None Include="icon.png" Pack="true" PackagePath="\"/> <InternalsVisibleTo Include="QsNet.Tests"/> </ItemGroup>
52-54: Pinned SourceLink reference is appropriate.Microsoft.SourceLink.GitHub 8.0.0 with PrivateAssets=All is correct. Consider periodically bumping to the latest patch to pick up fixes.
QsNet.Tests/DecodeTests.cs (1)
4131-4170: Minor test ergonomics: factor small helpers to reduce repetition.Several tests project list values via Select(x => x?.ToString()). Consider a small helper to DRY this and improve failure messages.
For example, add:
private static IEnumerable<string?> AsStrings(object? list) => Assert.IsAssignableFrom<IEnumerable>(list).Cast<object?>().Select(x => x?.ToString());Then assertions reduce to:
AsStrings(dict["a"]).Should().Equal("1","2","3","4","5");Also applies to: 4726-4766
📜 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/DecodeTests.cs(17 hunks)QsNet/QsNet.csproj(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
QsNet.Tests/DecodeTests.cs (3)
QsNet/Models/DecodeOptions.cs (4)
DecodeOptions(40-321)DecodeOptions(49-63)DecodeOptions(274-320)Decode(203-214)QsNet/Qs.cs (2)
Qs(20-280)Dictionary(30-117)QsNet/Internal/Decoder.cs (2)
Dictionary(75-217)List(508-625)
⏰ 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 (17)
QsNet/QsNet.csproj (4)
22-26: Good SourceLink packaging setup.Using PublishRepositoryUrl + EmbedUntrackedSources aligns with SourceLink best practices. This will improve package debugging experience.
27-29: Analyzer and package validation: nice hardening.EnableNETAnalyzers with AnalysisLevel=latest-recommended and EnablePackageValidation will catch regressions early. LGTM.
31-33: Deterministic + CI-only WarningsAsErrors is a good balance.This keeps local dev flexible while enforcing cleanliness in CI. Approved.
39-41: net8.0-only ImplicitUsings is correct.Scoping ImplicitUsings to net8.0 avoids surprises on netstandard2.0. Approved.
QsNet.Tests/DecodeTests.cs (13)
4-4: Added System.Linq is necessary for new list projections in assertions.Required for Select() usage later. Good catch.
272-278: Correct exception type for list limit with comma-splitting.Switching to InvalidOperationException aligns with library changes for limit violations. Message assertion looks precise.
2281-2309: StrictDepth overflow assertions updated to InvalidOperationException — consistent with decoder changes.These now reflect SplitKeyIntoSegments throwing InvalidOperationException on well-formed overflows under StrictDepth=true.
2408-2410: Parameter limit exceed now expects InvalidOperationException — matches ParseQueryStringValues semantics when ThrowOn is true.Looks correct given the updated enforcement path.
2486-2488: List limit exceed (flat list) -> InvalidOperationException is correct.Matches standardized exception policy.
2534-2535: Negative ListLimit with ThrowOnLimitExceeded=true throwing InvalidOperationException is consistent with “invalid options in operation” semantics.Approved.
2542-2544: Nested list limit exceed also asserts InvalidOperationException — consistent.Good coverage of nested case.
4152-4169: Behavior clarified: bracketed single occurrence with comma-split yields a nested list.This precisely documents the intended shape difference between “a=1,2,3” and “a[]=1,2,3”. Approved.
4174-4198: Top-level plain/encoded dot splitting with AllowDots=true, DecodeDotInKeys=true — solid tests.These ensure percent-decoded dots behave equivalently to literal dots at the top level.
4201-4218: Encoded dot splits even when DecodeDotInKeys=false (with AllowDots=true).This codifies that DecodeDotInKeys controls key-decoding semantics, while top-level dot splitting remains governed by AllowDots. Approved.
4249-4266: Bracket-segment percent-decoding to '.' regardless of DecodeDotInKeys — good clarification.This matches the “inside brackets still percent-decodes” contract.
4686-4691: StrictDepth well-formed overflow unit test matches SplitKeyIntoSegments behavior.Asserting InvalidOperationException here is correct.
4726-4766: Comma limit aggregation tests add valuable boundary coverage.
- Sum equals limit → allowed.
- Sum exceeds limit with ThrowOn → throws.
These confirm the combine-and-check semantics across duplicate parts. Nicely scoped.
…s and AllowDots options
1 participant
This pull request updates the test suite for the QsNet library, primarily focusing on improving exception handling and expanding coverage for edge cases in decoding and encoding. The most significant changes involve replacing
IndexOutOfRangeExceptionwithInvalidOperationExceptionfor various error scenarios, adding new tests for comma-split and dot behavior in keys, and increasing coverage for Unicode and numeric entity decoding. Additionally, some minor code cleanups and improvements were made.Exception Handling Improvements
QsNet.Tests/DecodeTests.csandQsNet.Tests/EncodeTests.csto expectInvalidOperationExceptioninstead ofIndexOutOfRangeExceptionwhen decoding or encoding fails due to limits, invalid options, or circular references. This makes error reporting more consistent and accurate. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]Expanded Test Coverage for Decoding Features
Unicode and Numeric Entity Decoding Enhancements
QsNet.Tests/UtilsTests.csto verify decoding of hexadecimal numeric entities, astral Unicode characters, and mixed decimal/hex entities. Added tests for edge cases such as empty hex digits, invalid hex entities, and surrogate pairs. [1] [2] [3]Minor Codebase Improvements
using System.Globalization;inQsNet/Internal/Decoder.csand changedLatin1Encodingto a readonly field for efficiency. [1] [2]These changes make the test suite more robust, clarify error handling, and ensure better coverage for complex decoding scenarios.
Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Compatibility
Packaging
Tests