⚡ add structured key detection to optimize flat query decoding#55
Merged
⚡ add structured key detection to optimize flat query decoding#55
Conversation
Owner
Author
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds new unit tests and test helpers; refactors decode internals to introduce structured-key scanning fast-path, consolidated token-collection helpers, and percent/plus short-circuits; applies NETSTANDARD2_0 conditional using directives. Public Decode/Encode APIs unchanged. Changes
Sequence DiagramsequenceDiagram
participant Caller as Decode Caller
participant Qs as Qs.Decode
participant Scanner as StructuredKeyScan
participant Parser as Decoder.ParseQueryStringValues
participant Utils as Utils.Decode/Encode
Caller->>Qs: Decode(input, options)
Qs->>Scanner: ScanStructuredKeys(inputKeys, options)
Scanner-->>Qs: HasAnyStructuredSyntax? (true/false)
alt structured syntax detected
Qs->>Parser: ParseQueryStringValues(parts, options)
Parser->>Utils: CollectNonEmptyParts(rawStr, delimiter)
Utils-->>Parser: parts
Parser->>Utils: DecodeValue(value, options)
Utils-->>Parser: decoded value(s)
Parser-->>Qs: structured object
Qs->>Qs: NormalizeObjectMap(parsed)
else no structured syntax
Qs->>Qs: Build flattened string-key dictionary and compact
end
Qs-->>Caller: return decoded result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Optimizes query string decoding by adding a fast path for flat queries, reducing unnecessary parsing/decoding work, and expanding unit test coverage for tricky decoding cases.
Changes:
- Added structured-key detection to bypass full key parsing for flat query strings.
- Refactored query token splitting/filtering to a helper that avoids empty-key tokens and improves delimiter handling.
- Optimized
Utils.Decodeto skip work when no+or%exists, plus added tests for new behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
QsNet/Qs.cs |
Adds structured-key scanning and a fast path for flat query decoding, plus merge normalization helper. |
QsNet/Internal/Decoder.cs |
Introduces CollectNonEmptyParts fast-path splitting and improves list/value decoding logic. |
QsNet/Internal/Utils.cs |
Avoids unnecessary decode work when inputs contain no escapes; improves +/% handling. |
QsNet.Tests/DecodeTests.cs |
Adds tests comparing string vs map input decoding for mixed/flat cases. |
QsNet.Tests/UtilsTests.cs |
Adds tests for Utils.Decode fast paths and + replacement without %. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55 +/- ##
==========================================
- Coverage 99.69% 99.37% -0.32%
==========================================
Files 19 20 +1
Lines 1959 2093 +134
Branches 493 534 +41
==========================================
+ Hits 1953 2080 +127
- Misses 6 13 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
QsNet/Internal/Decoder.cs (1)
307-339: Consider pre-sizingpartsin the string-delimiter fast path.This method is hot; a small capacity estimate can reduce list growth reallocations on large query strings.
As per coding guidelines `QsNet/Internal/**/*.cs`: Keep parser/encoder hot paths allocation-light; avoid LINQ in hot paths and prefer explicit loops with pre-sized collections.♻️ Optional micro-optimization
- var parts = new List<string>(); + var estimated = 1; + if (singleCharDelimiter) + { + for (var i = 0; i < input.Length; i++) + if (input[i] == delimiterChar) + estimated++; + } + + var parts = new List<string>(estimated);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QsNet/Internal/Decoder.cs` around lines 307 - 339, CollectNonEmptyStringParts is a hot path and currently creates parts with default capacity causing reallocations; compute a small capacity estimate (e.g., count occurrences of delimiterChar in input when singleCharDelimiter is true, without LINQ) and initialize parts with new List<string>(estimatedCount + 1); leave the existing conditional logic and HasNonEmptyRawKey checks unchanged so only the parts list creation is replaced with a pre-sized list in the single-char delimiter fast path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@QsNet.Tests/UtilsTests.cs`:
- Around line 3010-3021: Two test methods in UtilsTests.cs use the old
"Decode_*" naming; rename
Decode_ReturnsOriginalWhenNoEscapesAndDecoderWouldThrow to
ShouldReturnOriginalWhenNoEscapesAndDecoderWouldThrow and
Decode_ReplacesPlusWithoutInvokingUrlDecodeWhenNoPercent to
ShouldReplacePlusWithoutInvokingUrlDecodeWhenNoPercent so they follow the
"Should..." test naming convention; update any references (e.g., in test
runners) and keep them under the same UtilsTests class (or move into a nested
class grouping if scenarios expand) while leaving the test bodies and assertions
unchanged.
---
Nitpick comments:
In `@QsNet/Internal/Decoder.cs`:
- Around line 307-339: CollectNonEmptyStringParts is a hot path and currently
creates parts with default capacity causing reallocations; compute a small
capacity estimate (e.g., count occurrences of delimiterChar in input when
singleCharDelimiter is true, without LINQ) and initialize parts with new
List<string>(estimatedCount + 1); leave the existing conditional logic and
HasNonEmptyRawKey checks unchanged so only the parts list creation is replaced
with a pre-sized list in the single-char delimiter fast path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf92e978-7051-4c55-8c04-6550f6edf528
📒 Files selected for processing (5)
QsNet.Tests/DecodeTests.csQsNet.Tests/UtilsTests.csQsNet/Internal/Decoder.csQsNet/Internal/Utils.csQsNet/Qs.cs
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
techouse
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
QsNet/Internal/Decoder.cs:1
limitis declared asint?, soaccepted > limituses lifted comparison semantics (producingbool?) and won’t compile in anifcondition. Keeplimitnon-nullable (e.g., useint.MaxValueas the sentinel) or revert to an explicit null check (limit != null && accepted > limit.Value).
using System;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@QsNet.Tests/DecodeTests.cs`:
- Around line 4417-4459: These three tests
(ShouldDecodeMixedStructuredAndFlatLikeMapInputWhenFlatComesFirst,
ShouldPreserveLeadingBracketRootCollisionSemantics,
ShouldDecodeFlatQueriesConsistentlyWithMapInput) currently only use
BeEquivalentTo and need explicit key-order assertions: after calling Qs.Decode
and creating fromString and fromMap, add an assertion that the sequence of root
keys (the iteration order) for fromString matches the sequence for fromMap (e.g.
compare the key lists or use FluentAssertions' order-aware assertions) so any
regression in flat/structured ordering is detected in each test.
In `@QsNet.Tests/UtilsTests.cs`:
- Around line 3017-3021: The test
ShouldReplacePlusWithoutInvokingUrlDecodeWhenNoPercent only asserts output and
can pass even if UrlDecode was invoked and swallowed; update the test to use a
TrackingThrowingEncoding (or similar) that flips a WasDecodePathUsed flag when
any of the decoding paths (GetCharCount/GetChars) are called, call
Utils.Decode("a+b+c", new TrackingThrowingEncoding()), assert the decoded result
equals "a b c" and also assert that TrackingThrowingEncoding.WasDecodePathUsed
is false to ensure UrlDecode was not invoked; reference the existing
Utils.Decode method and replace ThrowingEncoding with the new
TrackingThrowingEncoding class used inside the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e47ecf65-befc-48c6-acc2-60110cff728f
📒 Files selected for processing (3)
QsNet.Tests/DecodeTests.csQsNet.Tests/UtilsTests.csQsNet/Internal/Decoder.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
QsNet/Qs.cs (2)
144-155: Fallback case may silently discard merge results.When
mergedis neither aDictionary<object, object?>norIDictionary, the method returns the originalfallbackunchanged. IfUtils.Mergeunexpectedly returns a non-dictionary type (e.g., a list or primitive), this would silently ignore the merge result.Consider whether a defensive log or exception for unexpected types would help catch integration issues during development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QsNet/Qs.cs` around lines 144 - 155, NormalizeObjectMap currently returns the original fallback when merged is neither Dictionary<object, object?> nor IDictionary, which silently discards unexpected merge results; modify NormalizeObjectMap to detect unexpected types and throw an informative InvalidOperationException (or at minimum log an error) that includes the actual runtime type name of merged and a brief message referencing the caller intent (e.g., "NormalizeObjectMap expected a dictionary but received {type}"), so callers (and Utils.Merge) fail fast and surface integration bugs instead of silently using fallback values.
216-235: Consider documenting the"0"fallback for empty roots.Line 234 returns
"0"whencleanRootis empty (e.g., for keys starting with"[]"). While this aligns with array index semantics, adding a brief comment would clarify the intent for future maintainers.📝 Suggested comment
- return cleanRoot.Length == 0 ? "0" : cleanRoot; + // Empty bracket root (e.g., "[]") defaults to array index "0" + return cleanRoot.Length == 0 ? "0" : cleanRoot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@QsNet/Qs.cs` around lines 216 - 235, Add a short inline comment in LeadingStructuredRoot explaining why an empty cleaned root falls back to "0" (it represents an empty array index like "[]", so we treat it as index 0), referencing the cleanRoot variable and the return of "0" to make intent clear for future maintainers; place the comment just above the line that returns "0" (the ternary that checks cleanRoot.Length == 0) so it's obvious which case it documents.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@QsNet/Qs.cs`:
- Around line 187-214: FirstStructuredSplitIndex incorrectly treats "%2E"/"%2e"
as structural dots whenever allowDots is true, causing keys like "a%2Eb" to be
split even when Decoder.ParseObject/DecodeDotInKeys would leave them encoded;
fix by adding a boolean parameter decodeDotInKeys to FirstStructuredSplitIndex,
pass the Decoder.DecodeDotInKeys value from ScanStructuredKeys, and only perform
the encoded-dot detection block when decodeDotInKeys is true; update all callers
(notably ScanStructuredKeys) to supply the new parameter and adjust related
tests
(EncodedDot_TopLevel_AllowDotsTrue_DecodeDotInKeysFalse_EncodedDotAlsoSplits) to
reflect the corrected behavior.
---
Nitpick comments:
In `@QsNet/Qs.cs`:
- Around line 144-155: NormalizeObjectMap currently returns the original
fallback when merged is neither Dictionary<object, object?> nor IDictionary,
which silently discards unexpected merge results; modify NormalizeObjectMap to
detect unexpected types and throw an informative InvalidOperationException (or
at minimum log an error) that includes the actual runtime type name of merged
and a brief message referencing the caller intent (e.g., "NormalizeObjectMap
expected a dictionary but received {type}"), so callers (and Utils.Merge) fail
fast and surface integration bugs instead of silently using fallback values.
- Around line 216-235: Add a short inline comment in LeadingStructuredRoot
explaining why an empty cleaned root falls back to "0" (it represents an empty
array index like "[]", so we treat it as index 0), referencing the cleanRoot
variable and the return of "0" to make intent clear for future maintainers;
place the comment just above the line that returns "0" (the ternary that checks
cleanRoot.Length == 0) so it's obvious which case it documents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eef89789-f584-468a-95e8-d89247a63ce3
📒 Files selected for processing (3)
QsNet.Tests/DecodeTests.csQsNet.Tests/UtilsTests.csQsNet/Qs.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- QsNet.Tests/DecodeTests.cs
- QsNet.Tests/UtilsTests.cs
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
techouse
force-pushed
the
chore/optimize-decoder
branch
from
5c218ed to
94787f9
Compare
Owner
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements and fixes to the query string decoding logic and enhances compatibility with .NET Standard 2.0. The most significant changes include a refactor to how query string parts are collected and filtered, improved handling of decoding and edge cases, and the addition of new tests to verify consistent decoding behavior. Additionally, conditional compilation directives are added throughout the codebase to ensure .NET Standard 2.0 compatibility.
Decoder logic improvements
CollectNonEmptyParts,CollectNonEmptyStringParts,CollectNonEmptyEnumerableParts) to efficiently collect only those parts with non-empty raw keys, improving both performance and correctness. (QsNet/Internal/Decoder.cs) [1] [2] [3]QsNet/Internal/Decoder.cs)QsNet/Internal/Decoder.cs) [1] [2] [3].NET Standard 2.0 compatibility
#if NETSTANDARD2_0conditional compilation directives and moved certainusingstatements inside these blocks in multiple files to improve compatibility with .NET Standard 2.0. (QsNet/Enums/Format.cs [1] QsNet/Enums/ListFormat.cs [2] QsNet/Enums/Sentinel.cs [3] QsNet/Extensions.cs [4] QsNet/Internal/Decoder.cs [5] QsNet/Internal/EncodeFrame.cs [6] QsNet/Internal/Encoder.cs [7] QsNet/Internal/MergeFrame.cs [8] QsNet/Internal/SideChannelFrame.cs [9]Test coverage and reliability
QsNet.Tests/DecodeTests.cs)TrackingThrowingEncodingto verify that decoding logic does not unnecessarily invoke URL decoding or throw when not required, improving robustness. (QsNet.Tests/UtilsTests.cs)Miscellaneous fixes and refactoring
QsNet/Internal/Decoder.cs) [1] [2] [3] [4] [5] [6]These changes collectively improve the correctness, performance, and portability of the query string decoder.
Benchmarks
Count=100, CommaLists=false, Utf8Sentinel=false, ValueLen=8)Count=1000, CommaLists=false, Utf8Sentinel=false, ValueLen=40)Count=1000, CommaLists=true, Utf8Sentinel=true, ValueLen=40)Summary by CodeRabbit
Tests
Bug Fixes
Performance
Refactor
Chores