Conversation
…byte arrays and buffers
…byte array handling
…e list limits, and improve byte array handling
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors encoder and merge engines from recursive to stack-based state machines, adds identity-based cycle detection, refines decoder handling for list limits and empty segments, improves byte/surrogate handling in utilities, expands public EncodeOptions with commaCompactNulls, and broadens tests for encoding/decoding/merge/overflow behaviors. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #74 +/- ##
============================================
- Coverage 95.14% 94.71% -0.43%
- Complexity 452 530 +78
============================================
Files 15 15
Lines 1277 1609 +332
Branches 276 348 +72
============================================
+ Hits 1215 1524 +309
- Misses 11 16 +5
- Partials 51 69 +18
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
🤖 Fix all issues with AI agents
In `@qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt`:
- Around line 147-167: When a FunctionFilter (frame.filter of type
FunctionFilter) is applied the code returns early and skips date normalization;
after invoking the filter via f.function(frame.prefix, obj) ensure the returned
obj still passes through the same date/temporal normalization branch (i.e. if
obj is LocalDateTime apply frame.serializeDate?.invoke(obj) ?: obj.toString(),
and if isCommaGenerator and obj is Iterable<*> map each element applying the
Instant/LocalDateTime handling). Update the block handling FunctionFilter
(and/or the subsequent else branch) so date serialization logic is executed for
the filtered result using the same checks and frame.serializeDate reference as
used for non-filtered values.
🧹 Nitpick comments (1)
qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/UtilsSpec.kt (1)
511-553: Comprehensive overflow-map merge coverage.Four distinct scenarios are tested: scalar append, iterable append (with Undefined skipping), scalar-as-target with index shifting, and map merge preserving
maxIndex. One minor note: at line 517, the@Suppress("UNCHECKED_CAST") val map = result as Map<String, Any?>cast is redundant sinceresultis already asserted asUtils.OverflowMap(which implementsMap) on line 516 — but this is a stylistic nit.🔧 Minor: remove redundant cast
- val result = Utils.merge(overflow, "d") - result.shouldBeInstanceOf<Utils.OverflowMap>() - `@Suppress`("UNCHECKED_CAST") val map = result as Map<String, Any?> - map["3"] shouldBe "d" + val result = Utils.merge(overflow, "d") + val map = result.shouldBeInstanceOf<Utils.OverflowMap>() + map["3"] shouldBe "d"
shouldBeInstanceOfreturns the value cast to the asserted type, so the subsequentascast is unnecessary.
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt
Show resolved
Hide resolved
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7639ec04f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt
Show resolved
Hide resolved
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This pull request enhances the query string parsing and encoding library by addressing edge cases and improving reliability. The changes focus on stricter parsing semantics, improved list limit enforcement, better handling of special data types, and internal safety improvements for deeply nested structures.
Changes:
- Skip empty query segments and empty keys during decoding to match
qslibrary behavior - Enforce comma-separated list limits with proper truncation/throwing and handle negative limits
- Correct UTF-16 surrogate encoding and prevent splitting surrogate pairs across boundaries
- Improve byte array and buffer handling across encoding modes
- Replace undefined holes during list merges and normalize when
parseLists=false - Detect cycles introduced by user-defined filters during encoding
- Refactor encode/merge internals to stack-based traversal for deep-nesting safety
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt | Skips empty segments and keys during parsing; enforces comma list limits with negative limit handling |
| qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Encoder.kt | Replaces WeakHashMap cycle tracking with IdentityHashMap; refactors to stack-based traversal; adds ByteBuffer support |
| qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt | Refactors merge to stack-based approach; adds overflow map handling; corrects UTF-16 surrogate encoding; adds byte array/buffer string conversion |
| qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/DecodeSpec.kt | Adds tests for empty segments, comma list limits, negative list limits, and parameter counting |
| qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/EncodeSpec.kt | Adds tests for date serialization with filters, byte array/buffer encoding in various modes |
| qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/QsParserSpec.kt | Updates expected results for list overflow behavior with listLimit=0 |
| qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/UtilsSpec.kt | Adds tests for surrogate encoding, byte buffer handling, merge with undefined holes, overflow map operations |
| qs-kotlin/src/test/kotlin/io/github/techouse/qskotlin/unit/internal/EncoderInternalSpec.kt | Adds test for cycle detection through user-defined filters |
| CHANGELOG.md | Documents all fixes and improvements in version 1.5.0-wip |
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt
Outdated
Show resolved
Hide resolved
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40747c80f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Utils.kt
Outdated
Show resolved
Hide resolved
…perations fix: prevent splitting surrogate pairs across segment boundaries in decode function
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt
Show resolved
Hide resolved
qs-kotlin/src/main/kotlin/io/github/techouse/qskotlin/internal/Decoder.kt
Show resolved
Hide resolved
…ments, keys, and overflow handling
…ist size in Encoder
1 participant
This pull request introduces several important bug fixes and improvements to the query string parsing and encoding logic, focusing on correct handling of empty segments, list and parameter limits, and edge cases with byte arrays and surrogate pairs. It also expands and corrects the test suite to cover these scenarios and updates the changelog to reflect these changes.
Decoder and parameter/list limit handling:
parameterLimit, ensuring delimiters do not consume the limit budget and matching the behavior of theqslibrary. Empty keys are also skipped during decoding. [1] [2] [3]listLimitvalues are treated as unlimited, and overflow semantics are preserved in merges. [1] [2] [3] [4]Encoding and decoding edge cases:
Utils.encode, and ensures proper handling of lone surrogates.encode=false, and COMMA list encoding honors this for decoding as well. [1] [2] [3]FunctionFilter and date serialization:
FunctionFilterstill pass through date serialization and COMMA temporal normalization during encoding.Test suite and documentation:
CHANGELOG.mdto document all fixes and improvements in version 1.5.0-wip.Summary of most important changes:
Parameter and list limit enforcement:
parameterLimit, and skips empty keys during decoding, matchingqsbehavior. [1] [2] [3]listLimitfor unlimited lists. [1] [2] [3]Encoding/decoding edge cases:
Utils.encode.encode=false, including in COMMA lists. [1] [2]FunctionFilter and date serialization:
FunctionFilterresults pass through date serialization and COMMA temporal normalization.Test suite improvements:
Documentation:
CHANGELOG.mdfor version 1.5.0-wip to document these fixes and improvements.Summary by CodeRabbit
Chore
New Features
Bug Fixes
Improvements
Tests