[ECO-5664] Original request id propagation #2172
Conversation
WalkthroughThe changes replace the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/2172/features
github-actions
bot
temporarily deployed
to
staging/pull/2172/jazzydoc
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Test/AblyTests/Tests/RestClientTests.swift (1)
2077-2105: LGTM! Test effectively verifies request_id propagation.The test correctly validates that a single
request_idis propagated across all retry attempts (primary + 2 fallbacks). This aligns with the PR objectives to fix request ID propagation during fallback retries.Optional: More robust query parameter extraction
Line 2103 uses
substring(after: "request_id=")which extracts everything after the delimiter. If other query parameters followrequest_id, they would be included in the extracted value. While the test still works (since the structure is consistent), you could make it more robust:-let requestIds = capturedURLs.map { $0.substring(after: "request_id=") } +let requestIds = capturedURLs.compactMap { url in + guard let urlObj = URL(string: url), + let components = URLComponents(url: urlObj, resolvingAgainstBaseURL: true), + let requestId = components.queryItems?.first(where: { $0.name == "request_id" })?.value else { + return nil + } + return requestId +}This uses proper URL parsing instead of string manipulation, making the test more resilient to URL structure changes.
Source/ARTNSMutableURLRequest+ARTUtils.m (1)
5-32: Consider adding nil checks for parameters.The implementation correctly removes existing query items with the same name before adding the new one, ensuring uniqueness. However, the method doesn't validate the
nameandvalueparameters.Add parameter validation
While
NSURLQueryItemmay handle nil values, it's better to be explicit about expectations:- (void)setQueryItemNamed:(NSString *)name withValue:(NSString *)value { + if (name == nil || value == nil) { + return; + } + NSURLComponents *components = [NSURLComponents componentsWithURL:self.URL resolvingAgainstBaseURL:YES];This makes the API contract clearer and prevents potential issues if
NSURLQueryItembehavior changes or if nil values cause unexpected query string formatting.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Source/ARTNSMutableURLRequest+ARTUtils.mSource/ARTRest.mSource/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.hTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/RestClientTests.swift
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTNSMutableURLRequest+ARTUtils.mSource/ARTRest.m
🔇 Additional comments (4)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
1545-1572: LGTM! Clean string utilities for test assertions.The three substring helper methods are well-implemented and will be useful for extracting query parameter values in tests. They handle missing delimiters gracefully by returning nil.
Source/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.h (1)
12-12: LGTM! API change clarifies intent.The new
setQueryItemNamed:withValue:method name better communicates that it replaces any existing query item with the same name, rather than appending a duplicate. The simpler signature (taking name and value directly) is also more convenient.Source/ARTRest.m (2)
504-513: LGTM! Request ID generation logic is correct.The code correctly handles request ID propagation:
- First request (fallbacks == nil): generates a new UUID-based request ID
- Retry requests (fallbacks != nil): reuses the original request ID from the first attempt
This ensures all retries for a given request share the same request_id value.
447-460: LGTM! Core fix for request ID propagation.This is the key fix described in the PR objectives. By passing
requestIdto theoriginalRequestId:parameter at line 457, the request ID is correctly propagated through the entire fallback retry chain. Previously,originalRequestIdwould have been nil on the first retry, causing either a missing request_id or a new one to be generated.The fix ensures:
- All fallback attempts share the same request_id from the original request
- ARTErrorInfo instances for fallback attempts carry the correct requestId
maratal
force-pushed
the
fix/2163-originalRequestId-propagation
branch
from
82c8759 to
af36992
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Test/AblyTests/Tests/RestClientTests.swift (1)
2081-2104: Avoid brittlerequest_idextraction via substring; parse the query instead.
substring(after: "request_id=")can silently produce wrong values if the parameter is missing, reordered, followed by other params, or encoded. Prefer the existingextractURLQueryValue(..., key:)helper and assert non-nil IDs before comparing.Proposed change
- let capturedURLs = requests.map { $0.url!.absoluteString } + let capturedURLs = requests.map { $0.url!.absoluteString } XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(0), pattern: "//rest.ably.io")) XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(1), pattern: "//[a-e].ably-realtime.com")) XCTAssertTrue(NSRegularExpression.match(capturedURLs.at(2), pattern: "//[a-e].ably-realtime.com")) - let requestIds = capturedURLs.map { $0.substring(after: "request_id=") } - XCTAssertEqual(Set(requestIds).count, 1) // all requestIds are equal + let requestIds = requests.compactMap { extractURLQueryValue($0.url, key: "request_id") } + XCTAssertEqual(requestIds.count, requests.count) // every request has request_id + XCTAssertEqual(Set(requestIds).count, 1) // all requestIds are equalSource/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.h (1)
12-12: Good API shape; consider documenting replacement semantics.Source/ARTNSMutableURLRequest+ARTUtils.m (1)
5-32: Correct duplicate-removal, but note it can reorder query params (remove + append).
If you want to preserve the original position of an existing query item, update the first match in-place and only remove subsequent duplicates.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
Source/ARTNSMutableURLRequest+ARTUtils.mSource/ARTRest.mSource/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.hSource/include/Ably/ARTChannelProtocol.hSource/include/Ably/ARTMessageOperation.hTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/RestClientTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Test/AblyTests/Test Utilities/TestUtilities.swift
- Source/ARTRest.m
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTNSMutableURLRequest+ARTUtils.m
🔇 Additional comments (2)
Source/include/Ably/ARTMessageOperation.h (1)
8-20: Doc updates look consistent and clearer.Source/include/Ably/ARTChannelProtocol.h (1)
73-113: Doc updates are helpful and align with expected semantics.
…d when connecting via fallback hosts. `appendQueryItem:` adds request_id each time, so to avoid multiple additions I've replaced it with `setQueryItemNamed:`.
maratal
force-pushed
the
fix/2163-originalRequestId-propagation
branch
from
af36992 to
20d8384
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Test/AblyTests/Tests/RestClientTests.swift (1)
2103-2104: Verify non-nil request_id values before equality check.The test correctly validates that all requests share the same
request_id, but the current implementation usingsubstring(after:)could be more robust. Ifrequest_idis followed by other query parameters, the extracted values would include those additional parameters, potentially causing false positives.Consider these improvements:
♻️ Suggested improvements for robustness
let requestIds = capturedURLs.map { $0.substring(after: "request_id=") } +// Verify all request_ids are non-nil +XCTAssertTrue(requestIds.allSatisfy { $0 != nil && !$0!.isEmpty }) + +// Extract just the request_id value (before any subsequent query params) +let cleanRequestIds = requestIds.compactMap { $0?.substring(before: "&") ?? $0 } +XCTAssertEqual(cleanRequestIds.count, 3) -XCTAssertEqual(Set(requestIds).count, 1) // all requestIds are equal +XCTAssertEqual(Set(cleanRequestIds).count, 1) // all requestIds are equal + +// Verify the request_id is a valid UUID-like string +let requestId = cleanRequestIds.first! +XCTAssertGreaterThan(requestId.count, 0)Alternative: Use the existing
extractURLQueryValue(_:key:)helper (line 738) for more reliable extraction:-let requestIds = capturedURLs.map { $0.substring(after: "request_id=") } +let requestIds = requests.compactMap { extractURLQueryValue($0.url, key: "request_id") } +XCTAssertEqual(requestIds.count, 3) XCTAssertEqual(Set(requestIds).count, 1) // all requestIds are equal
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
Source/ARTNSMutableURLRequest+ARTUtils.mSource/ARTRest.mSource/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.hTest/AblyTests/Test Utilities/TestUtilities.swiftTest/AblyTests/Tests/RestClientTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/ARTRest.m
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-10T14:14:47.456Z
Learnt from: lawrence-forooghian
Repo: ably/ably-cocoa PR: 2162
File: Source/ARTJsonLikeEncoder.m:522-525
Timestamp: 2025-12-10T14:14:47.456Z
Learning: In Source/ARTJsonLikeEncoder.m, ensure that artMessage.actionIsInternallySet is only set to YES when the SDK explicitly sets the action field (e.g., in ARTMessage initWithName:data:). Messages decoded from the wire must not have actionIsInternallySet set, even if they contain an action field, because the flag should distinguish SDK-internal actions from wire-provided actions. Apply this behavior broadly to Objective-C files in the Source directory that involve message encoding/decoding, and add tests to verify that decoding does not flip the flag.
Applied to files:
Source/ARTNSMutableURLRequest+ARTUtils.m
⏰ 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). (6)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: check
🔇 Additional comments (7)
Source/PrivateHeaders/Ably/ARTNSMutableURLRequest+ARTUtils.h (1)
12-12: LGTM! API change appropriately reflects replacement semantics.The method signature change from
appendQueryItem:tosetQueryItemNamed:withValue:is well-designed. The "set" terminology correctly communicates that the method replaces (rather than appends) query items with the same name, which aligns with the PR objective to prevent duplicaterequest_idparameters.Source/ARTNSMutableURLRequest+ARTUtils.m (3)
5-9: LGTM! Proper early return guard.The method signature and early return pattern are correct and consistent with the existing
replaceHostWith:method in the same file.
11-20: LGTM! Safe and efficient duplicate removal.The implementation correctly handles the removal of existing query items with the same name:
- Uses
NSMutableIndexSetto collect indices, avoiding mutation-during-iteration issues- The
?: @[]fallback safely handles nilqueryItemsremoveObjectsAtIndexes:is the appropriate method for batch removal
22-32: LGTM! Correct query item addition and URL update.The implementation properly:
- Creates a new
NSURLQueryItemwith the provided name and value- Updates
components.queryItemswith the modified array- Guards against nil when updating
self.URLThe pattern is consistent with the existing
replaceHostWith:method.Test/AblyTests/Test Utilities/TestUtilities.swift (3)
1545-1551: LGTM! Correct implementation.The
substring(after:)method correctly returns the portion of the string after the first occurrence of the delimiter, or nil if not found.
1553-1559: LGTM! Correct implementation.The
substring(before:)method correctly returns the portion of the string before the first occurrence of the delimiter, or nil if not found.
1561-1572: LGTM! Correct implementation.The
substring(between:andString:)method correctly:
- Finds the first delimiter and returns nil if not found
- Searches for the second delimiter only in the remaining string after the first
- Returns the substring between the two delimiters
The implementation properly handles edge cases.
Closes #2163
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.