✨ add key-aware decoding to the query string parser #3
Conversation
…oder and legacy decoder
…; improve segment splitting and list parsing logic
…erplay, and segment splitting behavior
…ing logic to prefer decoderBlock over valueDecoderBlock
…ils, and percent-encoded dot handling
…ons, list/map behaviors, charset handling, and filters
… behaviors, error handling, and options support
…ecodeOrDefault, error handling, and options propagation
…ic object error propagation, and options support
…o use Set<ObjectIdentifier> for cycle detection instead of NSHashTable
…on and artifact upload
|
Warning Rate limit exceeded@techouse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds an Objective‑C macOS E2E job and CI workflow edits; introduces DecodeKind and a context-aware ScalarDecoder (with Obj‑C bridge); replaces regex dot logic with depth‑aware splitting; switches identity tracking to Set; adds many Obj‑C/Swift tests and a crash-log collection script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ObjC as ObjC DecodeOptionsObjC
participant Bridge as QsObjC Bridge
participant Core as QsSwift.DecodeOptions
participant Decoder as Decoder
Caller->>ObjC: set `decoderBlock` / `legacyDecoderBlock`
ObjC->>Bridge: create Swift DecodeOptions (ScalarDecoder?, LegacyDecoder?)
Bridge->>Core: init/copy DecodeOptions(decoder: scalar, legacyDecoder: legacy)
Caller->>Decoder: decode(query, options)
alt token is key
Decoder->>Core: options.decodeKey(token, charset)
Core->>Core: decode(value: token, charset, kind: .key)
else token is value
Decoder->>Core: options.decodeValue(value, charset)
Core->>Core: decode(value: value, charset, kind: .value)
end
alt ScalarDecoder present
Core->>Caller: invoke ScalarDecoder(value, charset, kind)
else LegacyDecoder present
Core->>Caller: invoke LegacyDecoder(value, charset)
else
Core->>Core: defaultDecode(value, charset)
end
Note over Core: splitKeyIntoSegments (depth-aware) runs during key parsing
sequenceDiagram
autonumber
actor EncoderCaller
participant Bridge as QsObjC.QsBridge
participant Seen as Set:ObjectIdentifier
participant SwiftEnc as QsSwift.encode
EncoderCaller->>Bridge: bridgeInputForEncode(input)
Bridge->>Seen: init empty Set<ObjectIdentifier>
loop traverse containers
Bridge->>Bridge: _bridgeInputForEncode(obj, seen: &Seen)
alt ObjectIdentifier in Seen
Bridge-->>Bridge: return original Foundation container (cycle)
else
Bridge->>Seen: insert ObjectIdentifier(obj)
Bridge->>Bridge: recurse children with &Seen
end
end
Bridge->>SwiftEnc: encode bridged input
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
✨ 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 #3 +/- ##
=======================================
Coverage ? 86.22%
=======================================
Files ? 43
Lines ? 3303
Branches ? 0
=======================================
Hits ? 2848
Misses ? 455
Partials ? 0
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: 2
🧹 Nitpick comments (29)
Sources/QsSwift/Enums/DecodeKind.swift (1)
3-11: Clarify raw value stability for cross-language consumers.Since DecodeKind underpins the ObjC bridge, consider documenting that the raw values are stable (e.g., 0 = key, 1 = value) to avoid accidental reorderings in future refactors. If you want to enforce it, explicitly assign raw values.
- case key + case key = 0 @@ - case value + case value = 1.github/workflows/test.yml (6)
10-13: Reduce permissions to least-privilege; replace or remove actions: write.actions: write isn’t needed for the steps shown (checkout, xcodebuild, upload-artifact, codecov). If you plan to use Codecov OIDC, you want id-token: write instead. Otherwise, drop the extra permission.
Option A (OIDC with Codecov v5, no token):
permissions: contents: read - actions: write + id-token: writeThen remove CODECOV_TOKEN env in the Codecov step.
Option B (keep token-based auth):
permissions: contents: read - actions: write
33-36: Checkout v5 is fine; confirm submodule needs.actions/checkout@v5 uses Node 20 by default. If submodules are needed for ObjCE2ETests, add with: submodules: true. If not, LGTM.
- - name: Checkout - uses: actions/checkout@v5 + - name: Checkout + uses: actions/checkout@v5 + with: + submodules: falseAlso applies to: 56-58
52-55: Matrix coverage consistency.Style runs on macOS 15 and 14; tests run only on macOS 15/Swift 6.1. If you intend to keep Swift 5.10 validation, add the macOS 14 leg to tests as well.
matrix: include: - os: macos-15 swift: '6.1' xcode: '16.3' + - os: macos-14 + swift: '5.10' + xcode: '15.4'
73-117: Crash-log collector is useful; consider DRY-ing into a composite action.The same logic appears in objc-tests below. A small .github/actions/collect-crashlogs composite action would reduce duplication and keep fixes in one place.
119-127: Codecov v5 setup check.With v5, OIDC is preferred. If you keep the token path, ensure CODECOV_TOKEN is scoped to this repo. If you switch to OIDC, add id-token: write and drop the token. Also consider using fail_ci_if_error: true to surface upload issues.
- name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: files: coverage/info.lcov flags: swift,${{ matrix.os }},swift-${{ matrix.swift }} name: QsSwift-${{ matrix.os }}-swift-${{ matrix.swift }} + fail_ci_if_error: true - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
128-216: New ObjC job is well-structured; one small hardening.Good: Xcode selection, result bundle upload, crash logs, CODE_SIGNING_ALLOWED=NO. Consider adding -quiet to xcodebuild to reduce log noise and using -derivedDataPath to isolate builds/artifacts.
- xcodebuild \ + xcodebuild -quiet \ -project ObjCE2ETests/ObjCE2ETests.xcodeproj \ -scheme ObjCE2ETests \ -configuration Debug \ + -derivedDataPath .derivedData-objc \ -destination "platform=macOS" \ CODE_SIGNING_ALLOWED=NO \ -resultBundlePath ObjC-Tests.xcresult \ testSources/QsObjC/QsBridge.swift (3)
169-178: Revisit-on-identity returns Foundation containers; verify mixed-container tolerance.On revisiting a seen NSDictionary/NSArray, you return the original Foundation instance to preserve identity cycles. That can yield a mixed tree (OrderedDictionary/[Any] in one branch, NSDictionary/NSArray in another). Confirm the encoder core accepts Foundation containers at any depth and still honors ordering expectations for the ordered branch.
If the core expects Swift containers only, consider path-aware cycle detection instead: track an ancestors set (current recursion stack) to return the original on true cycles but continue normal bridging for repeated-but-acyclic sharing.
Sketch:
-internal static func bridgeInputForEncode(_ input: Any) -> Any { - var seen = Set<ObjectIdentifier>() - return _bridgeInputForEncode(input, seen: &seen) +internal static func bridgeInputForEncode(_ input: Any) -> Any { + var ancestors = Set<ObjectIdentifier>() // path-local + return _bridgeInputForEncode(input, ancestors: &ancestors) } @@ -private static func _bridgeInputForEncode(_ input: Any, seen: inout Set<ObjectIdentifier>) -> Any { +private static func _bridgeInputForEncode( + _ input: Any, + ancestors: inout Set<ObjectIdentifier> +) -> Any { @@ - let id = ObjectIdentifier(obj) - if seen.contains(id) { return d /* or a */ } - seen.insert(id) + let id = ObjectIdentifier(obj) + if ancestors.contains(id) { return d /* or a */ } // true cycle + let inserted = ancestors.insert(id).inserted @@ - return out + if inserted { ancestors.remove(id) } + return outThis keeps Swift containers unless there’s an actual cycle on the current path.
Also applies to: 186-193
181-185: Key stringification strategy is consistent; note potential ambiguity.String(describing:) ensures all keys become readable Strings, but heterogeneous keys that stringify to the same text will collide. If that’s acceptable for ObjC bridging parity, great—else consider surfacing a decode-time error or namespacing non-String keys.
Also applies to: 259-263
101-123: Decode-path bridging: reduce(into:) tuple casting relies on NSDictionary.Element shape.The cast (k, v) = kv as? (AnyHashable, Any) is fine given NSDictionary.Element == (key: Any, value: Any). Just flagging that this relies on Foundation’s ABI; if Swift overlays ever evolve, a more explicit iteration via for (key, value) in d would avoid the extra cast.
- return d.reduce(into: [AnyHashable: Any]()) { acc, kv in - if let (k, v) = kv as? (AnyHashable, Any) { - acc[k] = v - } else { - acc[AnyHashable(stringifyKey(kv.key))] = kv.value - } - } + var out: [AnyHashable: Any] = [:] + d.forEach { key, value in + if let hk = key as? AnyHashable { + out[hk] = value + } else { + out[AnyHashable(stringifyKey(key))] = value + } + } + return outTests/QsSwiftTests/ExampleTests.swift (1)
603-611: Decoder closure updated to ScalarDecoder (3-arity) — good migration.Signature matches the new options API. Consider adding one assertion that decode-only-on-values vs keys works as intended when DecodeKind is .key (e.g., ensure a custom decoder doesn’t transform dots in keys).
ObjCE2ETests/ObjCE2ETests/ObjCEncodeConvenienceTests.m (1)
39-59: Cycle detection assertions are strong; add code check to tighten.You assert domain and isCyclicObject. Consider also asserting the error code matches the cyclic case to guard against domain collisions.
- XCTAssertEqualObjects(err.domain, QsEncodeErrorInfo.domain); - XCTAssertTrue([QsEncodeError isCyclicObject:err]); + XCTAssertEqualObjects(err.domain, QsEncodeErrorInfo.domain); + XCTAssertTrue([QsEncodeError isCyclicObject:err]); + XCTAssertEqual(err.code, QsEncodeErrorInfo.cyclicObject);ObjCE2ETests/ObjCE2ETests/ObjCDecodeConvenienceTests.m (2)
52-56: Avoid magic error code; use the public enum constantHardcoding 4 is brittle. Prefer the bridged constant for depthExceeded, which you already use elsewhere.
- XCTAssertEqual(err.code, 4); + XCTAssertEqual(err.code, QsDecodeErrorCodeDepthExceeded);
14-22: Consider adding a nil-input coverage testA small extra test for Qs_DecodeOrEmpty/Qs_DecodeOrDefault with input=nil would cement the semantics for “no query string”.
Example test to add:
- (void)test_helpers_nil_input { QsDecodeOptions *o = [QsDecodeOptions new]; XCTAssertEqual(Qs_DecodeOrEmpty(nil, o).count, (NSUInteger)0); NSDictionary *def = @{@"k": @"v"}; XCTAssertEqualObjects(Qs_DecodeOrDefault(nil, o, def), def); XCTAssertNil(Qs_DecodeOrNil(nil, o)); }Sources/QsObjC/Enums/DecodeKindObjC.swift (1)
17-23: Optional: widen access for the Swift counterpart if external callers need itIf any external Swift code (that imports QsObjC) needs to round-trip via the Obj-C enum, consider making
var swiftpublic. If not, keeping it internal is fine.ObjCE2ETests/ObjCE2ETests/ObjCBridgeExtrasTests.m (3)
193-204: Bump async test timeout to reduce CI flakinessmacOS runners can be sluggish intermittently. A 2s timeout (like in other async tests) is safer.
- [self waitForExpectations:@[exp] timeout:1.0]; + [self waitForExpectations:@[exp] timeout:2.0];
205-214: Likewise: align decode-async timeout to 2sKeep async timeouts consistent across the suite.
- [self waitForExpectations:@[exp] timeout:1.0]; + [self waitForExpectations:@[exp] timeout:2.0];
279-291: Avoid magic numbers for DecodeKind; use the enum constantsUsing 0/1 for key/value invites drift if the enum evolves. Compare against QsDecodeKindKey/QsDecodeKindValue instead.
- d.decoderBlock = ^id _Nullable(NSString * _Nullable token, - NSNumber * _Nullable charsetNum, - NSNumber * _Nullable kindNum) { - NSInteger kind = kindNum.integerValue; // 0 = key, 1 = value - if (kind == 0) { + d.decoderBlock = ^id _Nullable(NSString * _Nullable token, + NSNumber * _Nullable charsetNum, + NSNumber * _Nullable kindNum) { + QsDecodeKind kind = (QsDecodeKind)kindNum.integerValue; // enum-backed + if (kind == QsDecodeKindKey) { sawKey = YES; return [NSString stringWithFormat:@"K:%@", token ?: @"<nil>"]; - } else { + } else { // QsDecodeKindValue sawValue = YES; return [NSString stringWithFormat:@"V:%@", token ?: @"<nil>"]; } };ObjCE2ETests/ObjCE2ETests/ObjCExampleTests.m (1)
139-169: Duplicates policy coverage is thoroughAll three modes are exercised. Minor style nit: consider comparing counts with
(NSUInteger)2for consistency with the rest of the file.ObjCE2ETests/ObjCE2ETests/ObjCEncodeAsyncTests.m (4)
11-28: Prefer inferred block parameter types; drop__unsafe_unretainedqualifiersRelying on the method signature for type inference avoids coupling to qualifiers emitted in the bridging header and reduces churn if the header changes.
- [Qs encodeAsyncOnMain:input options:opts completion:^(__unsafe_unretained NSString * _Nullable s, NSError * _Nullable err) { + [Qs encodeAsyncOnMain:input options:opts completion:^(NSString * _Nullable s, NSError * _Nullable err) {
30-46: Same here forencodeAsyncKeep the blocks simple and inferred.
- [Qs encodeAsync:input options:opts completion:^(__unsafe_unretained NSString * _Nullable s, NSError * _Nullable err) { + [Qs encodeAsync:input options:opts completion:^(NSString * _Nullable s, NSError * _Nullable err) {
48-67: And for the error case off mainConsistent parameter style across all async tests.
- [Qs encodeAsync:a options:nil completion:^(__unsafe_unretained NSString * _Nullable s, NSError * _Nullable err) { + [Qs encodeAsync:a options:nil completion:^(NSString * _Nullable s, NSError * _Nullable err) {
69-87: And for the error case on mainDitto.
- [Qs encodeAsyncOnMain:a options:nil completion:^(__unsafe_unretained NSString * _Nullable s, NSError * _Nullable err) { + [Qs encodeAsyncOnMain:a options:nil completion:^(NSString * _Nullable s, NSError * _Nullable err) {Tests/QsSwiftTests/DecodeTests.swift (2)
1918-1923: Minor: local Shift_JIS percent decoder could live in Helpers to avoid duplicationOptional: extract percentDecode into the Helpers section so it can be reused by future charset tests.
- // Make the closure type explicitly @Sendable. - let decoder: @Sendable (String?, String.Encoding?, DecodeKind?) -> Any? = { s, _, _ in + // Make the closure type explicitly @Sendable. + let decoder: @Sendable (String?, String.Encoding?, DecodeKind?) -> Any? = { s, _, _ in guard let s else { return nil } return percentDecode(s, encoding: .shiftJIS) ?? s }
3405-3410: Resolve “FIXME” lint warnings in commentsReplace “FIXME” with a neutral note; the tests are intentionally disabled due to a precondition enforced in the initializer.
- // FIXME: this one fails the precondition + // NOTE: intentionally disabled — this combination violates a precondition in the initializer- // FIXME: this one fails the precondition + // NOTE: intentionally disabled — this combination violates a precondition in the initializerAlso applies to: 3487-3491
Sources/QsSwift/Internal/Decoder.swift (3)
61-63: Remove unused dotRegex (dead code and confusing docs)dotRegex is never used (dotToBracket does manual scanning). Drop it to avoid confusion and unnecessary compile work.
- /// Cached regex that rewrites dot-notation `.segment` → `[segment]` - /// when `allowDots == true`. It matches a dot followed by a token that - /// contains neither `.` nor `[` (i.e., `\.([^.\[]+)`). - private static let dotRegex = try! NSRegularExpression( - pattern: #"\.([^.\[]+)"#, options: [] - )
97-99: Be precise about dropping a single leading '?'drop(while:) removes all leading '?' characters. If you want to drop only a single prefix, prefer hasPrefix + dropFirst. Not critical, but it’s clearer and avoids surprising normalization.
- let cleanStr = (options.ignoreQueryPrefix ? String(str.drop(while: { $0 == "?" })) : str) + let cleanStr = options.ignoreQueryPrefix && str.hasPrefix("?") + ? String(str.dropFirst()) + : str .replacingOccurrences(of: "%5B", with: "[", options: [.caseInsensitive]) .replacingOccurrences(of: "%5D", with: "]", options: [.caseInsensitive])
479-498: Clarify “[]” as dictionary key when lists are disabled or listLimit < 0The condition (!parseLists || listLimit < 0) && decodedRoot.isEmpty intentionally treats “[]” as key "0". This aligns with tests, but add a short comment referencing behavior parity to prevent future regressions.
- if (!options.parseLists || options.listLimit < 0) && decodedRoot.isEmpty { + // Parity: when list parsing is disabled or listLimit < 0, + // treat "[]" as a dictionary key "0". + if (!options.parseLists || options.listLimit < 0) && decodedRoot.isEmpty {Sources/QsSwift/Models/DecodeOptions.swift (1)
223-227: Consider deprecating getDecoder(_:charset:) in favor of decodeValue/decodeKeyYou’re keeping getDecoder for compatibility, but surfacing deprecation will guide callers to the clearer decodeKey/decodeValue APIs before the next major.
- public func getDecoder(_ value: String?, charset: String.Encoding? = nil) -> Any? { + @available(*, deprecated, message: "Use decodeValue(_:charset:) or decodeKey(_:charset:) instead") + public func getDecoder(_ value: String?, charset: String.Encoding? = nil) -> Any? { // Back‑compat: treat this as a VALUE decode. return decode(value, charset ?? self.charset, .value) }
📜 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 (14)
.github/workflows/test.yml(4 hunks)ObjCE2ETests/ObjCE2ETests/ObjCBridgeExtrasTests.m(13 hunks)ObjCE2ETests/ObjCE2ETests/ObjCDecodeConvenienceTests.m(1 hunks)ObjCE2ETests/ObjCE2ETests/ObjCEncodeAsyncTests.m(1 hunks)ObjCE2ETests/ObjCE2ETests/ObjCEncodeConvenienceTests.m(1 hunks)ObjCE2ETests/ObjCE2ETests/ObjCExampleTests.m(1 hunks)Sources/QsObjC/Enums/DecodeKindObjC.swift(1 hunks)Sources/QsObjC/Models/DecodeOptionsObjC.swift(5 hunks)Sources/QsObjC/QsBridge.swift(5 hunks)Sources/QsSwift/Enums/DecodeKind.swift(1 hunks)Sources/QsSwift/Internal/Decoder.swift(6 hunks)Sources/QsSwift/Models/DecodeOptions.swift(8 hunks)Tests/QsSwiftTests/DecodeTests.swift(7 hunks)Tests/QsSwiftTests/ExampleTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
Tests/QsSwiftTests/DecodeTests.swift (4)
Sources/QsSwift/Models/DecodeOptions.swift (1)
decode(230-239)Sources/QsSwift/Qs+Decode.swift (1)
decode(34-39)Sources/QsSwift/Internal/Utils.swift (3)
decode(548-579)deprecated(243-246)deprecated(332-335)Sources/QsSwift/Internal/Decoder.swift (1)
splitKeyIntoSegments(308-394)
Sources/QsSwift/Internal/Decoder.swift (2)
Sources/QsSwift/Models/DecodeOptions.swift (2)
decodeKey(251-255)decode(230-239)Sources/QsSwift/Internal/Utils.swift (1)
decode(548-579)
Sources/QsObjC/Models/DecodeOptionsObjC.swift (1)
Sources/QsObjC/Models/EncodeOptionsObjC.swift (1)
with(194-198)
Sources/QsSwift/Models/DecodeOptions.swift (1)
Sources/QsSwift/Internal/Utils.swift (1)
decode(548-579)
🪛 SwiftLint (0.57.0)
Tests/QsSwiftTests/DecodeTests.swift
[Warning] 3405-3405: FIXMEs should be resolved (this one fails the preconditio...)
(todo)
[Warning] 3487-3487: FIXMEs should be resolved (this one fails the preconditio...)
(todo)
[Error] 3138-3138: Type name 'DecodeOptionsInterplayTests_Parity' should only contain alphanumeric and other allowed characters
(type_name)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Swift tests + coverage (macos-15, Swift 6.1)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (swift)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (26)
Sources/QsSwift/Enums/DecodeKind.swift (1)
6-27: Enum shape and intent are clear; good addition.Public, Sendable, and CustomStringConvertible are appropriate here. Raw Int backing is sensible for ObjC bridging symmetry.
Sources/QsObjC/QsBridge.swift (2)
141-149: Switch to Set is a solid simplification over NSHashTable.Lower allocation overhead, value semantics, and no ObjC runtime hash table required. Good change.
218-227: Undefined bridging looks correct and preserves shape; nice.You map UndefinedObjC to QsSwift.Undefined.instance and keep ordered containers and identity behavior consistent. Good attention to detail.
Also applies to: 233-239, 247-249, 251-261, 265-271, 273-279, 281-289
ObjCE2ETests/ObjCE2ETests/ObjCEncodeConvenienceTests.m (2)
9-19: Convenience wrappers mirror Swift helpers cleanly.Qs_EncodeOrNil and Qs_EncodeOrEmpty match expected semantics and keep tests concise. Nice.
75-82: Options propagation verified; good coverage.The addQueryPrefix assertion ensures ObjC options flow through the bridge correctly.
ObjCE2ETests/ObjCE2ETests/ObjCDecodeConvenienceTests.m (2)
6-22: Helpers mirror Swift conveniences cleanlyThe small wrappers are concise and correctly handle NSError propagation. Good additions for Obj-C ergonomics.
98-107: LGTM: allowDots propagation verified through helpersThis confirms the helpers pass options through correctly and that dot-splitting behavior is exercised. No changes needed.
Sources/QsObjC/Enums/DecodeKindObjC.swift (1)
12-32: Solid Obj-C bridge for DecodeKind; raw-value stability documentedMapping is correct and the doc explains KEY vs VALUE semantics clearly. CustomStringConvertible is a nice touch for logs.
ObjCE2ETests/ObjCE2ETests/ObjCBridgeExtrasTests.m (4)
22-33: Cycle detection error path covered wellGood end-to-end assertion of domain and code for cyclic objects.
329-338: LGTM: depth=0 guardrail for dot-splittingThis asserts an important edge case and prevents accidental top-level splits when depth is disabled.
380-414: Docs follow-up: clarify encoded-dot semantics when allowDots=trueYou assert that a%2Eb splits even when decodeDotInKeys=false. That’s a subtle but important rule. Ensure the public docs for QsDecodeOptions spell this out so Obj-C consumers don’t misinterpret decodeDotInKeys.
Would you like me to draft a short README blurb that explains:
- literal dot vs percent-encoded dot
- top-level vs bracket segment handling
- the effect of depth=0?
431-438: LGTM: value token decoding of %2E to "."Good to have this explicit to prevent regressions in scalar value decoding.
ObjCE2ETests/ObjCE2ETests/ObjCExampleTests.m (2)
118-125: Nice cross-check of encodeDotInKeys (implies allowDots) round-tripClear expectations and good coverage with precent-encoded “name%252Eobj”.
565-577: Query prefix assertions are order-agnostic—goodUsing a set for equality avoids incidental ordering differences. No changes needed.
ObjCE2ETests/ObjCE2ETests/ObjCEncodeAsyncTests.m (1)
11-87: Async tests look solid overallDeterministic ordering via sortKeysCaseInsensitively and explicit main/off-main assertions are nice touches. Error-path assertions use both a type helper and the domain check—great coverage.
Tests/QsSwiftTests/DecodeTests.swift (3)
1138-1146: LGTM: tests updated to the new ScalarDecoder signatureAdapting to the three-argument ScalarDecoder (including the unused DecodeKind) is correct and keeps the test future-proof.
1536-1543: LGTM: custom ScalarDecoder example is correctUsing a kind-agnostic ScalarDecoder here is fine; returning a String directly mirrors the new decoder semantics.
1750-1761: LGTM: nil-return behavior validatedGood coverage that a decoder returning nil yields NSNull in the decoded output while still interpreting numeric entities for other values.
Sources/QsSwift/Internal/Decoder.swift (3)
168-176: LGTM: route key decoding through DecodeOptions.decodeKeySwitching to options.decodeKey(..., .key) ensures KEY/VALUE-aware decoders receive the correct context, and the fallback preserves the raw key when decoding yields nil.
183-200: LGTM: preserve element-level nils when comma-splitting with custom decodersMapping arrays via the custom ScalarDecoder and keeping nils aligns with the documented “nil means absent” semantics.
314-342: Depth- and bracket-aware segmentation matches the new testsThe early return for depth <= 0, top-level dot-to-bracket rewrite, and balanced group collection all line up with the remainder-wrapping tests you added.
Sources/QsObjC/Models/DecodeOptionsObjC.swift (2)
126-171: LGTM: ObjC bridge correctly prioritizes decoderBlock > valueDecoderBlock > legacyDecoderBlockBridging to ScalarDecoder with a boxed block, mapping kind to 0/1, and passing legacy decoders separately matches the Swift core API shape.
208-211: LGTM: with(_:) made public, matches EncodeOptionsObjC usabilityConsistency with EncodeOptionsObjC improves ergonomics for ObjC/Swift call sites.
Sources/QsSwift/Models/DecodeOptions.swift (3)
5-11: LGTM: introduce ScalarDecoder and keep LegacyDecoder for back-compatThe new decoder shape unlocks KEY/VALUE-aware customization without breaking existing integrations.
249-256: LGTM: decodeKey coerces non-String results using String(describing:)Matches the tests that return Int for keys and expect "42".
212-219: Precondition coupling between decodeDotInKeys and allowDots is well-definedThe invariant enforcement matches the test plan where invalid combos are disabled. Documented clearly in the initializer.
…ound for stale SwiftPM path
…ParityTests for consistency
…ed data path; remove unnecessary actions write permission
…er precedence; improve comments and parameter descriptions
…rove comments and examples for bracket/dot logic
… tracking of seen objects
…n collection script
…ified log predicate to include xcodebuild and CoreSimulator
… symlink safety check in Objective-C test setup
…d during decoding
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 (4)
Sources/QsSwift/Internal/Decoder.swift (4)
151-173: Don’t select a “]=” that appears in the value; restrict heuristic to before the first “=”Current logic searches for “]=” anywhere in the token and may split at a “]=” that occurs in the value, e.g.
k=abc]=xyz(incorrectly makingk=abc]the key). Choose the first “=” by default, and only prefer a “]=” when it occurs before that first “=”. This preserves the intended heuristic without mis-splitting.Apply this diff:
- let bracketEqualsPos: Int = { - if let range = part.range(of: "]=") { - return part.distance(from: part.startIndex, to: range.lowerBound) + 1 - } - return -1 - }() - let pos: Int = { - if bracketEqualsPos == -1 { - if let r = part.firstIndex(of: "=") { - return part.distance(from: part.startIndex, to: r) - } - return -1 - } - return bracketEqualsPos - }() + // Find the first '='; prefer a preceding ']=' only if it occurs before that '='. + let firstEqIndex = part.firstIndex(of: "=") + let firstEqPos = firstEqIndex.map { part.distance(from: part.startIndex, to: $0) } ?? -1 + let pos: Int = { + guard let fe = firstEqIndex else { return -1 } + if let br = part.range(of: "]="), br.upperBound <= fe { + return part.distance(from: part.startIndex, to: br.lowerBound) + 1 + } + return firstEqPos + }()
225-237: Numeric-entity decoding collapses arrays into a single stringWhen
interpretNumericEntitiesis active, arrays are joined with commas and then decoded into a single string, losing structure. Decode per element and preserve arrays.Apply this diff:
- if let v = value, !Utils.isEmpty(v), options.interpretNumericEntities, + if let v = value, !Utils.isEmpty(v), options.interpretNumericEntities, charset == .isoLatin1 { - let text: String - if let arr = v as? [Any] { - text = arr.map { String(describing: $0) }.joined(separator: ",") - } else if let arrOpt = v as? [Any?] { - text = arrOpt.map { String(describing: $0 ?? NSNull()) }.joined(separator: ",") - } else { - text = String(describing: v) - } - value = Utils.interpretNumericEntities(text) + if let arr = v as? [Any] { + value = arr.map { elem in + if let s = elem as? String { + return Utils.interpretNumericEntities(s) + } + return elem + } + } else if let arrOpt = v as? [Any?] { + value = arrOpt.map { elem in + if let s = elem as? String { + return Utils.interpretNumericEntities(s) + } + return elem ?? NSNull() + } + } else if let s = v as? String { + value = Utils.interpretNumericEntities(s) + } }
40-67: Fix listLimit semantics; current ≤0 check will throw spuriously (especially when lists are disabled)
- The pre-check
options.listLimit <= 0combined withcurrentListLength >= options.listLimitwill always be true for negative limits, causing unintended throws.- Negative
listLimitis used elsewhere to signal “lists disabled” (treated as dict keys). In such cases, do not enforce list limits here.- Enforce limits only when
throwOnLimitExceeded == trueandlistLimit >= 0.Apply this diff:
private static func parseListValue( _ value: Any?, options: DecodeOptions, currentListLength: Int ) throws -> Any? { - if options.throwOnLimitExceeded, options.listLimit <= 0 { - // Defer to existing global validation or treat as exceeded uniformly. - if currentListLength >= options.listLimit { - throw DecodeError.listLimitExceeded(limit: options.listLimit) - } - } + let enforceLimits = options.throwOnLimitExceeded && options.listLimit >= 0 if let s = value as? String, !s.isEmpty, options.comma, s.contains(",") { let splitVal = s.split(separator: ",", omittingEmptySubsequences: false).map( String.init) - if options.throwOnLimitExceeded, - (currentListLength + splitVal.count) > options.listLimit + if enforceLimits && (currentListLength + splitVal.count) > options.listLimit { throw DecodeError.listLimitExceeded(limit: options.listLimit) } return splitVal } - if options.throwOnLimitExceeded, currentListLength >= options.listLimit { + if enforceLimits && (currentListLength + 1) > options.listLimit { throw DecodeError.listLimitExceeded(limit: options.listLimit) } return value }
552-613: dotToBracket: leading dot is not preserved as documentedDocs state “leading ‘.’ is preserved (e.g.
.a→.a)” but current logic converts.a→[a]. Add a guard fori == 0to keep the leading dot literal.Apply this diff:
case ".": if depth == 0 { let hasNext = (i + 1) < n let next = hasNext ? chars[i + 1] : "\0" - if next == "[" { + if i == 0 { + // Preserve a leading "." literally (".a" -> ".a") + out.append(".") + i += 1 + } else if next == "[" { // Skip the dot so "a.[b]" behaves like "a[b]" i += 1 } else if !hasNext || next == "." { // Trailing dot, or first dot in "a..b": keep literal "." out.append(".") i += 1 } else {
♻️ Duplicate comments (1)
Sources/QsSwift/Internal/Decoder.swift (1)
102-107: Scoped bracket normalization fix looks good and resolves prior parsing bugLimiting %5B/%5D normalization to the key slice (and removing the global replacements) addresses the earlier “key/value boundary” corruption issue flagged in a previous review. Nice job.
🧹 Nitpick comments (2)
Sources/QsSwift/Internal/Decoder.swift (2)
340-420: Depth and remainder handling look solid; one suggestion for pathological keysThe balanced-group collector and synthetic “wrap remainder” segment match the documented behavior and preserve malformed tails. Consider adding tests for:
- Unterminated inner groups:
a[b[c[d](ensure only one synthetic segment is produced)- Mixed dot/bracket overflow with
strictDepth=truevsfalseWould you like me to draft these unit tests in Swift to lock in behavior?
451-462: Heuristic for current list length is fragileDeriving
currentListLengthby convertingchain.dropLast().joined()to anIntwill benilfor most keys (e.g., “a[]”, “a[b][]”) and silently returns 0. If the goal is to enforce list limits accurately, consider:
- Tracking length from the accumulating object during merge, or
- Passing the existing array length of the target key from the caller when available.
I can propose a minimal refactor to plumb the current array length from the merge site if you share that call path.
📜 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 (3)
.github/workflows/test.yml(4 hunks)Sources/QsSwift/Internal/Decoder.swift(12 hunks)scripts/collect-macos-crashlogs.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/collect-macos-crashlogs.sh
- .github/workflows/test.yml
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/QsSwift/Internal/Decoder.swift (3)
Sources/QsSwift/Internal/Utils.swift (1)
decode(548-579)Sources/QsSwift/Models/Delimiter.swift (2)
split(21-24)split(51-81)Sources/QsSwift/Models/DecodeOptions.swift (2)
decodeKey(266-270)decode(243-254)
⏰ 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: Swift tests + coverage (macos-15, Swift 6.1)
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (swift)
🔇 Additional comments (3)
Sources/QsSwift/Internal/Decoder.swift (3)
239-247: “[]” wrapping only when RHS is already an array is correctLimiting list-of-lists wrapping to pre-split RHS arrays (comma path) avoids surprising
[[]]when users intended scalars. This matches qs-style expectations.
175-184: Key-slice-only detection of “[]” (including %5B/%5D) is the right trade-offNormalizing only the key slice and checking for a trailing “[]” safely captures encoded empty-bracket notation without risking value corruption. Good call.
205-216: Three-argument custom decoder wiring for arrays and scalars looks correct
- Arrays: mapping per element and normalizing
nil → NSNull()is consistent.- Scalars: allow
nilfrom the custom decoder and normalize at assignment sites.
…arrays in ISO-8859-1 mode
…cov upload in Objective-C test workflow
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/QsSwift/Internal/Decoder.swift (2)
45-67: List-limit checks will throw almost always when listLimit ≤ 0.
- If
listLimitis 0 or negative, the early check (currentListLength >= options.listLimit) will be true for virtually all inputs, causing spurious.listLimitExceeded.- Later checks (Lines 62–64) have the same problem when
listLimit < 0, which is often used to mean “unlimited/disabled” in qs ports.Gate limit enforcement on
listLimit >= 0and rely on the post-split check to account for growth.Apply this diff to make the checks consistent and non-spurious:
@@ - if options.throwOnLimitExceeded, options.listLimit <= 0 { - // Defer to existing global validation or treat as exceeded uniformly. - if currentListLength >= options.listLimit { - throw DecodeError.listLimitExceeded(limit: options.listLimit) - } - } + // Only enforce limits when non-negative. Negative values disable the limit. + // Early check intentionally omitted; enforce after we know how many elements will be added. @@ - if options.throwOnLimitExceeded, - (currentListLength + splitVal.count) > options.listLimit + if options.throwOnLimitExceeded, options.listLimit >= 0, + (currentListLength + splitVal.count) > options.listLimit { throw DecodeError.listLimitExceeded(limit: options.listLimit) } @@ - if options.throwOnLimitExceeded, currentListLength >= options.listLimit { + if options.throwOnLimitExceeded, options.listLimit >= 0, + currentListLength >= options.listLimit { throw DecodeError.listLimitExceeded(limit: options.listLimit) }
204-221: Use DecodeOptions.decode(...) and honor “nil means absent” semantics.
- This code bypasses
DecodeOptions.decode(...)by callingoptions._decoderdirectly, which skips legacy/default decoding paths and may not compile if_decoderisn’t an exposed API.- Per PR objectives, a custom decoder returning
nilshould drop the parameter entirely. Here, arrays mapniltoNSNull, and scalarnilis later coerced toNSNullat insertion (value ?? NSNull()), violating the “absent value” contract.Refactor to:
- Always use
options.decode(_,_,.value).- For arrays:
compactMapto dropnils; if empty, skip insertion.- For scalars: if decode yields
nil, skip insertion.Apply this diff:
@@ - // IMPORTANT: distinguish custom decoder vs default decoder - if let arr = parsed as? [String] { - if let custom = options._decoder { - let mapped = arr.map { custom($0, charset, .value) } - value = mapped.map { $0 ?? NSNull() } as [Any] - } else { - // default decoder: fall back to original literal when decoding fails - value = arr.map { Utils.decode($0, charset: charset) ?? $0 } as [Any] - } - } else if let s = parsed as? String { - if let custom = options._decoder { - value = custom(s, charset, .value) // may be nil; keep it nil - } else { - value = Utils.decode(s, charset: charset) ?? s - } - } else { - value = parsed - } + if let arr = parsed as? [String] { + let mapped: [Any] = arr.compactMap { options.decode($0, charset, .value) } + value = mapped.isEmpty ? nil : mapped + } else if let s = parsed as? String { + value = options.decode(s, charset, .value) // nil ⇒ drop + } else { + value = parsed + } @@ - // Duplicates handling (only arrayify on subsequent duplicates, like Kotlin) + // Drop entirely if decoding requested absence (nil). + if value == nil { continue } + // Duplicates handling (only arrayify on subsequent duplicates, like Kotlin) let exists = (obj[key] != nil)Also applies to: 261-276
♻️ Duplicate comments (1)
Sources/QsSwift/Internal/Decoder.swift (1)
102-107: Fix validated: bracket normalization no longer corrupts values.Removing global
%5B/%5Dnormalization from the entire query string avoids mis-splitting tokens likek=abc%5D=xyzand correctly defers any bracket handling to the key slice only. This resolves the previously raised correctness issue.
🧹 Nitpick comments (11)
.github/workflows/test.yml (8)
12-12: Top-level actions: read is fine; confirm necessity vs. least-privilegeTop-level
actions: readis generally harmless, and job-level overrides grantactions: writewhere needed. If no steps outside jobs need Actions API, consider omitting the top-levelactions: readto keep surface minimal. Your call.
61-63: Artifact naming is clear; consider adding if-no-files-found: ignore for robustnessYou already did this for Swift crashlogs. Consistency with ObjC artifacts (see Lines 179–185) would prevent warns on runs without results.
- name: Upload ObjC test result bundle if: ${{ always() }} uses: actions/upload-artifact@v4 with: name: ObjC-Tests-xcode-${{ matrix.xcode }} path: ObjC-Tests.xcresult + if-no-files-found: warn
74-88: ObjC job depends on Swift tests; confirm intended gating
needs: testsblocks ObjC E2E when Swift tests fail. If you prefer ObjC E2E to still run (to collect coverage or independent signal), remove the dependency or useif: ${{ always() }}at the job level.Proposed alternative to always attempt ObjC E2E (still sharing matrix and env):
- objc-tests: + objc-tests: permissions: contents: read actions: write name: Objective-C E2E tests (Xcode) timeout-minutes: 45 - runs-on: ${{ matrix.os }} - needs: tests + runs-on: ${{ matrix.os }} + if: ${{ always() }}
103-118: Symlink workaround looks safe; minor hardening optionalQuoting is correct and the step is idempotent. Optional: guard against rare stale symlinks pointing elsewhere by validating target before
ln -sfn.- ln -sfn "$ACTUAL" "$EXPECTED" + if [ -L "$EXPECTED" ]; then + TARGET="$(readlink "$EXPECTED")" + if [ "$TARGET" != "$ACTUAL" ]; then + echo "Refreshing symlink: $EXPECTED -> $ACTUAL (was $TARGET)" + fi + fi + ln -sfn "$ACTUAL" "$EXPECTED"
137-152: ObjC test invocation is solid; consider explicit SDK to avoid image drift
-destination "platform=macOS"is fine. To avoid runner image drift, you can pin-sdk macosxexplicitly. Optional.xcodebuild -quiet \ -project ObjCE2ETests/ObjCE2ETests.xcodeproj \ -scheme ObjCE2ETests \ -configuration Debug \ -derivedDataPath .derivedData-objc \ - -destination "platform=macOS" \ + -destination "platform=macOS" -sdk macosx \ CODE_SIGNING_ALLOWED=NO \ -enableCodeCoverage YES \ -resultBundlePath ObjC-Tests.xcresult \ test
162-174: Codecov v5 usage looks good; optionally switch to OIDC (no token)You’re using
CODECOV_TOKEN. If you prefer tokenless uploads, enable OIDC and addpermissions: id-token: writeat the job level; Codecov v5 supports this. Not mandatory.Proposed minimal change:
objc-tests: permissions: contents: read actions: write + id-token: write…and remove
CODECOV_TOKENfor public repos after configuring Codecov.
179-185: Add if-no-files-found to avoid warnings on canceled/failed runsKeeps logs clean when
ObjC-Tests.xcresultisn’t produced.- name: Upload ObjC test result bundle if: ${{ always() }} uses: actions/upload-artifact@v4 with: name: ObjC-Tests-xcode-${{ matrix.xcode }} path: ObjC-Tests.xcresult + if-no-files-found: warn
190-196: Crash logs artifact naming is consistent; consider unifying with Swift logsSwift crashlogs include Swift version; ObjC ones don’t. Optional consistency tweak if you care about cross-job filtering.
- name: macOS-crashlogs-objc-xcode-${{ matrix.xcode }} + name: macOS-crashlogs-objc-xcode-${{ matrix.xcode }}-${{ matrix.os }}Sources/QsSwift/Internal/Decoder.swift (3)
151-174: Heuristic for picking the "]=" boundary is good; consider minor enhancement.Preferring the '=' immediately after a literal closing bracket maintains parity with qs behavior for inputs like
a[b]=c=d. If you want to cover the rare case where brackets are percent-encoded in the key (e.g.,a%5Bb%5D=c=d), you could optionally scan for"%5D="(case-insensitive) and use it only when it appears before the first raw '='. Low priority.
224-249: Numeric-entity interpretation collapses arrays into a scalar. Verify intent.Joining arrays with commas and then interpreting entities changes shape (array → string) before later “[]” handling re-wraps it. If preserving array shape is preferred, map
interpretNumericEntitieselement-wise instead of flattening.Follow-up (choose one):
- Keep as-is for Kotlin parity; ensure tests pin the behavior.
- Or refactor to preserve shape by mapping per element.
Would you like me to provide a targeted diff that preserves array shape during entity interpretation?
518-524: Edge-case: treating "[]" as dict key "0" when parseLists is off or listLimit < 0.This diverges from typical qs behavior where “[]” drives array semantics only when arrays are enabled; otherwise the segment often becomes a literal empty key. If this change is intentional, please ensure tests cover:
- parseLists=false with
a[]=1yields{ "a": { "0": "1" } }.- listLimit < 0 behaves the same.
If not intentional, consider preserving the empty-key literal instead.
📜 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)
.github/workflows/test.yml(4 hunks)Sources/QsSwift/Internal/Decoder.swift(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Sources/QsSwift/Internal/Decoder.swift (2)
Sources/QsSwift/Internal/Utils.swift (2)
isEmpty(819-836)decode(548-579)Sources/QsSwift/Models/DecodeOptions.swift (2)
decodeKey(266-270)decode(243-254)
⏰ 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: Analyze (swift)
- GitHub Check: Swift tests + coverage (macos-15, Swift 6.1)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (8)
.github/workflows/test.yml (5)
20-22: Correct: job-level actions: write for artifact uploadsGranting
actions: writeat the job level is required foractions/upload-artifact@v4. This is the right scope and matches least-privilege (only where used).
24-24: Good guardrail: timeout-minutes set to 45Setting an explicit timeout prevents hung macOS runners from burning minutes.
42-45: Nice: emit both Xcode and Swift versionsThis helps correlate toolchains in logs for reproducibility and Codecov flags.
55-55: Crash log collector script verified and executableThe
scripts/collect-macos-crashlogs.shfile is present, includes a proper shebang on the first line, and has its executable bit set. No further changes are needed.
- scripts/collect-macos-crashlogs.sh: file exists
- First line starts with
#!- Executable permission confirmed
35-35: Confirmed actions/checkout@v5 availability and recommendation
- GitHub tags list includes both
v5andv5.0.0, showing that v5 is released and available.- The official README’s usage examples reference
actions/checkout@v5as the default, recommended version.- Note: Checkout V5 requires GitHub Actions runner version ≥ v2.327.1 to execute successfully.
Since v5 is generally available and officially recommended, you can keep
uses: actions/checkout@v5without falling back to v4.Sources/QsSwift/Internal/Decoder.swift (3)
175-185: Key-slice-only empty-bracket detection looks correct.Normalizing
%5B/%5Dcase-insensitively on the key prefix and checkinghasSuffix("[]")is safe and avoids touching the value.
352-355: Depth-aware key splitting looks solid; ensure nested-bracket cases are covered.Balanced group extraction and synthetic remainder wrapping should deal with tricky keys (e.g.,
a[b[c]], unterminated brackets, and depth overflow). Please add/confirm tests for:
- Unterminated groups:
a[b[c→["a","[[b[c]"]- Overflow with strictDepth=true vs false:
a[b][c][d]at depth=2- No brackets + allowDots=true/false paths
Also applies to: 357-367, 369-381, 383-405, 406-414, 416-420, 422-430
305-318: Parse pipeline interface remains consistent.The
parseKeysentry point now delegates to the new splitter and object builder with a clearvaluesParsedflag. Looks good.
…rcePackages directory
…Objective-C test workflow
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
10-13: Tighten top-level permissions (drop unneeded actions: read).Unless a specific step reads workflow runs via the Actions API, the workflow-level
actions: readcan be omitted; job-level scoping is already present. Apply least privilege and rely on per‑job permissions.Apply this diff:
permissions: contents: read - actions: read
65-73: Codecov v5: make failure non-blocking (match ObjC job).For consistency with the ObjC upload (which sets
fail_ci_if_error: false) and to avoid CI flakes on upload hiccups, set the same here.Apply this diff:
- name: Upload coverage to Codecov uses: codecov/codecov-action@v5 with: files: coverage/info.lcov flags: swift,${{ matrix.os }},swift-${{ matrix.swift }} name: QsSwift-${{ matrix.os }}-swift-${{ matrix.swift }} + fail_ci_if_error: false env: CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
89-124: Symlink workaround: add a safety guard before rm -rf.Defensive check ensures we never remove anything outside the intended CI parent directory if paths differ unexpectedly.
Apply this diff:
# Idempotent: force-create/refresh the symlink if [ -e "$EXPECTED" ] && [ ! -L "$EXPECTED" ]; then - echo "Warning: $EXPECTED exists and is not a symlink; removing for CI." - rm -rf "$EXPECTED" + echo "Warning: $EXPECTED exists and is not a symlink; validating path and removing for CI." + case "$EXPECTED" in + "$PARENT"/*) : ;; + *) echo "Refusing to remove non-symlink outside $PARENT: $EXPECTED" >&2; exit 1 ;; + esac + [ "$EXPECTED" != "/" ] && rm -rf "$EXPECTED" fi
23-33: Add SwiftPM caching to the Swift tests jobWe’ve confirmed that
scripts/coverage.shinvokesswift buildandswift test(e.g. lines 44, 70, and 84), so caching SwiftPM artifacts will meaningfully speed up the CI runs.• Insert the following step at the top of the Swift tests + coverage job in
.github/workflows/test.yml(around lines 23–33), before runningcoverage.sh:steps: - name: Cache SwiftPM artifacts uses: actions/cache@v4 with: path: | ~/.swiftpm .build key: spm-${{ runner.os }}-swift-${{ matrix.swift }}-${{ hashFiles('**/Package.resolved') }} restore-keys: | spm-${{ runner.os }}-swift-${{ matrix.swift }}- # …existing steps…This will cache the
.builddirectory and any downloaded SwiftPM packages across runs, shaving several minutes off your workflow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Swift tests + coverage (macos-15, Swift 6.1)
- GitHub Check: Analyze (swift)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (7)
.github/workflows/test.yml (7)
53-64: Crash log collection is solid.Nice consolidation to a single script with conditional artifact upload on failure. This will simplify triage without polluting green runs.
74-88: ObjC job structure looks good.Matrix, timeout, dependency on Swift tests, and environment targeting Xcode 16.3/macOS 15 are appropriate for the new E2E path.
125-135: Good cache coverage for Xcode SPM (including custom DerivedData path).Including
.derivedData-objc/SourcePackagesaddresses missed cache hits when using a custom-derivedDataPath. This resolves the earlier review concern.
136-144: Using the same DerivedData for resolve and test avoids duplicate fetch/build.Passing
-derivedDataPath .derivedData-objcto the resolve step is the right call; it improves cache reuse and reduces network churn.
145-160: xcodebuild invocation looks correct for macOS E2E.
CODE_SIGNING_ALLOWED=NO, coverage enabled, explicit destination, and result bundle capture are all set properly.
169-179: Codecov upload wiring is correct (zip + JSON).Using the zipped
.xcresultalongsidexccovJSON is a pragmatic combo. Keepingfail_ci_if_error: falseavoids spurious failures.
185-191: Result bundle always uploaded — good for postmortems.Using
if: ${{ always() }}ensures artifacts are available even on failures.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/test.yml (2)
138-148: SPM cache now includes repo DerivedData SourcePackagesNice follow-through: adding .derivedData-objc/SourcePackages prevents re-resolves when using a custom -derivedDataPath.
149-157: Resolve step now uses the same DerivedData pathAligning -resolvePackageDependencies with -derivedDataPath .derivedData-objc avoids duplicated fetch/build work. Thanks for addressing this.
🧹 Nitpick comments (5)
.github/workflows/test.yml (5)
12-12: Drop top-level actions: read to minimize token scopeNo step calls the Actions API; this permission is unnecessary at workflow scope. Keep it lean and rely on per-job permissions.
permissions: contents: read - actions: read
75-83: Codecov v5 config is reasonable; consider OIDC laterfail_ci_if_error: false is a pragmatic choice. If you plan to go tokenless later, switch to OIDC with id-token: write and drop CODECOV_TOKEN.
- contents: read + contents: read + id-token: write @@ - - name: Upload coverage to Codecov - uses: codecov/codecov-action@v5 - with: - files: coverage/info.lcov - flags: swift,${{ matrix.os }},swift-${{ matrix.swift }} - name: QsSwift-${{ matrix.os }}-swift-${{ matrix.swift }} - fail_ci_if_error: false - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + - name: Upload coverage to Codecov (OIDC) + uses: codecov/codecov-action@v5 + with: + files: coverage/info.lcov + flags: swift,${{ matrix.os }},swift-${{ matrix.swift }} + name: QsSwift-${{ matrix.os }}-swift-${{ matrix.swift }} + fail_ci_if_error: false + use_oidc: true
112-137: Symlink workaround: make the destructive path removal saferThis block can rm -rf a non-symlink directory at $PARENT/QsSwift. While guarded to stay under $PARENT, prefer failing fast instead of deleting unknown content to avoid surprises if that path is legitimately present for any reason (e.g., manual debugging on self-hosted runners).
Would the Xcode project accept failing here (with a clear message) instead of force-removing the directory? If yes, consider:
- if [ -e "$EXPECTED" ] && [ ! -L "$EXPECTED" ]; then - echo "Warning: $EXPECTED exists and is not a symlink; validating path and removing for CI." - case "$EXPECTED" in - "$PARENT"/*) : ;; - *) echo "Refusing to remove non-symlink outside $PARENT: $EXPECTED" >&2; exit 1 ;; - esac - [ "$EXPECTED" != "/" ] && rm -rf "$EXPECTED" - fi + if [ -e "$EXPECTED" ] && [ ! -L "$EXPECTED" ]; then + echo "Error: $EXPECTED exists and is not a symlink. Refusing to remove it." >&2 + exit 1 + fi
158-173: ObjC test invocation looks solid; optionally disable auto package resolutionThe invocation is clean and reproducible. To further reduce network churn during test runs, consider disabling automatic package resolution since you already ran a resolve step.
xcodebuild -quiet \ -project ObjCE2ETests/ObjCE2ETests.xcodeproj \ -scheme ObjCE2ETests \ -configuration Debug \ -derivedDataPath .derivedData-objc \ -destination "platform=macOS" -sdk macosx \ CODE_SIGNING_ALLOWED=NO \ + -disableAutomaticPackageResolution \ -enableCodeCoverage YES \ -resultBundlePath ObjC-Tests.xcresult \ test
182-193: Codecov upload for ObjC artifacts is well-structuredUploading both the zipped xcresult and JSON with clear flags/names is good. If you migrate to OIDC per earlier comment, mirror that here and drop the token.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Swift tests + coverage (macos-15, Swift 6.1)
- GitHub Check: Analyze (swift)
- GitHub Check: Codacy Security Scan
🔇 Additional comments (8)
.github/workflows/test.yml (8)
20-23: Per-job least-privilege looks goodtests job limits permissions to contents: read and removes actions: write as previously suggested. Good hardening.
34-45: Checkout v5 + clear toolchain visibilityBumping to actions/checkout@v5 and printing both Xcode and Swift versions improves reproducibility and debug-ability. LGTM.
46-55: SwiftPM cache keys/paths are sensibleCaching ~/.swiftpm and .build keyed by OS+Swift+Package.resolved is appropriate for the SwiftPM-driven tests. Good setup.
62-73: Crash log collection/upload flow is correctConditionally collecting and uploading macOS crash logs on failure is standard and the artifact naming is clear.
84-97: New ObjC E2E job and gating with needs: testsSolid addition. Serializing after Swift tests is a reasonable way to save macOS minutes when core tests fail early.
174-181: xcresult archiving and JSON coverage extraction are correctUsing xccov to emit JSON plus zipping the bundle provides both summary and raw data for downstream analysis. LGTM.
198-205: Always uploading the ObjC .xcresult is helpful for debuggingGood choice to make this step unconditional.
206-216: Crash log handling on ObjC job is correctConditional collection and upload look fine; artifact naming is consistent with the Swift job.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
133-143: Nice: past cache/permissions alignment implemented
- Added
.derivedData-objc/SourcePackagesto cache and passed-derivedDataPath .derivedData-objcin resolve — this avoids duplicate resolves/builds and improves cache hit rate.- Dropped
actions: write— aligns with least-privilege.Also applies to: 144-152, 84-86
🧹 Nitpick comments (5)
.github/workflows/test.yml (5)
73-82: Consider switching Codecov to tokenless OIDCYou can remove the secret token and use GitHub OIDC for uploads. This tightens security (no long‑lived secret) and reduces secret management.
Proposed changes:
- Grant
id-token: writeat job level.- Remove
CODECOV_TOKENenv.- Optionally add the action input
use_oidc: true(keeps intent explicit).jobs: tests: permissions: contents: read + id-token: write @@ - - name: Upload coverage to Codecov + - name: Upload coverage to Codecov (OIDC) uses: codecov/codecov-action@v5 with: files: coverage/info.lcov flags: swift,${{ matrix.os }},swift-${{ matrix.swift }} name: QsSwift-${{ matrix.os }}-swift-${{ matrix.swift }} fail_ci_if_error: false - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + # No token needed with OIDC + # with: + # use_oidc: true @@ objc-tests: permissions: contents: read + id-token: write @@ - - name: Upload Objective-C coverage to Codecov + - name: Upload Objective-C coverage to Codecov (OIDC) uses: codecov/codecov-action@v5 with: files: | ${{ github.workspace }}/ObjC-Tests.xcresult.zip ${{ github.workspace }}/objc-coverage.json flags: objc,${{ matrix.os }},xcode-${{ matrix.xcode }} name: QsSwift-ObjC-${{ matrix.os }}-xcode-${{ matrix.xcode }} fail_ci_if_error: false - env: - CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }} + # with: + # use_oidc: trueIf you prefer, I can verify the latest Codecov v5 OIDC guidance and adjust the diffs accordingly.
Also applies to: 177-188, 19-21, 84-86
32-34: Enable full git history for better coverage diffsCodecov and some tools produce more accurate PR comparisons with an unshallow clone.
- - name: Checkout - uses: actions/checkout@v5 + - name: Checkout + uses: actions/checkout@v5 + with: + fetch-depth: 0Apply to both checkout steps.
Also applies to: 98-100
111-132: Symlink workaround: add a brief rationale comment + guard railsThe step is careful and idempotent. Add a one‑liner about why
QsSwiftpath is expected (e.g., Xcode/SwiftPM package name casing vs repo slug) to help future maintainers, and bail out if$PARENTisn’t writable (rare but clearer failure).- - name: Work around stale local SwiftPM path (symlink) + - name: Work around stale local SwiftPM path (symlink) run: | set -euxo pipefail + # Some SPM/Xcode setups expect the package folder name "QsSwift" (package name) + # while the repo slug is "qs-swift"; create/refresh a sibling symlink to avoid stale paths. ACTUAL="${GITHUB_WORKSPACE}" PARENT="$(dirname "$ACTUAL")" EXPECTED="${PARENT}/QsSwift" + test -w "$PARENT" mkdir -p "$PARENT"Confirm that the ObjC project paths (xcodeproj/workspace/package refs) genuinely rely on this
QsSwiftpath and not another alias.
158-169: Drop-quietto improve failure triage (or gate it behind env)
-quietsuppresses useful logs when a test fails; the.xcresulthelps, but inline logs speed up debugging.- xcodebuild -quiet \ + xcodebuild \ -project ObjCE2ETests/ObjCE2ETests.xcodeproj \Alternative: keep
-quietunlessCI_VERBOSE=1.
45-54: Optional: cache SwiftPM shared cache for CLI buildsFor the Swift CLI job, consider also caching the SwiftPM shared cache to reduce network fetches across runs at the cost of larger caches.
with: path: | ~/.swiftpm .build + ~/Library/Caches/org.swift.swiftpmIf cache size becomes an issue, skip this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/test.yml(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Objective-C E2E tests (Xcode) (macos-15, 16.3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyze (swift)
- GitHub Check: Codacy Security Scan
…tPM path in CI workflow
… test failures in coverage script
This pull request adds new Objective-C E2E tests and enhances the Objective-C bridge for the query string library, focusing on improved decoding/encoding customization and convenience methods. It also updates the CI workflow to support these new tests and modernizes some GitHub Actions. The most important changes are grouped below.
Testing and CI workflow improvements:
objc-testsjob to.github/workflows/test.ymlto run Objective-C E2E tests on macOS with Xcode 16.3, including steps for dependency resolution, test execution, artifact upload, and crash log collection.actions/checkoutusage to v5 in all jobs and grantedactions: writepermissions for workflow actions. [1] [2] [3]Objective-C bridge enhancements:
DecodeKindObjCenum to mirror Swift’sDecodeKind, allowing decoder blocks to distinguish between key and value decoding contexts.decoderBlocktoQsDecodeOptionsfor KEY/VALUE-aware decoding, along with a legacy two-argument block for backward compatibility.nilnow always produces an absent value, with no fallback to default decoding.New and expanded test coverage:
ObjCDecodeConvenienceTests.m,ObjCEncodeConvenienceTests.m), async encoding (ObjCEncodeAsyncTests.m), and advanced decoding scenarios (custom decoder blocks, dot syntax, error propagation, etc.) inObjCBridgeExtrasTests.m. [1] [2] [3] [4]References: [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]
Summary by CodeRabbit
New Features
Refactor
Tests
Chores