[ECO-5633] Add handling http JSON error which is not dictionary. #2136
Conversation
WalkthroughAdds a statusCode parameter to ARTEncoder.decodeErrorInfo, updates ARTJsonLikeEncoder to decode error payloads as dictionary or other types with explicit fallbacks using statusCode, propagates statusCode through ARTRest callers, extends test utilities to simulate status-coded payloads, and adds unit tests (duplicated) validating these cases. Changes
Sequence Diagram(s)sequenceDiagram
participant REST as ARTRest
participant Encoder as ARTJsonLikeEncoder
participant ARTError as ARTErrorInfo
REST->>Encoder: decodeErrorInfo(data, statusCode, &error)
alt payload is NSDictionary
Encoder->>Encoder: read error field (dict or other)
Encoder->>Encoder: code = error[@"code"] ?? statusCode*100
Encoder->>Encoder: status = error[@"statusCode"] ?? statusCode
Encoder->>Encoder: message = error[@"message"] ?? defaultHTTPMessage
Encoder->>ARTError: create(code,status,message)
Encoder-->>REST: return ARTErrorInfo
else payload is non-dictionary
Encoder->>Encoder: treat as non-dict payload
Encoder->>ARTError: create(code = statusCode*100, status = statusCode, message = defaultHTTPMessage)
Encoder-->>REST: return ARTErrorInfo
end
Note over Encoder,REST: statusCode used as primary fallback for missing fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
github-actions
bot
temporarily deployed
to
staging/pull/2136/features
github-actions
bot
temporarily deployed
to
staging/pull/2136/jazzydoc
![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
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(2 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1068-1072)simulateIncomingServerErrorOnNextRequest(1074-1078)data(915-917)execute(832-858)execute(927-938)execute(1016-1066)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
Test/AblyTests/Test Utilities/JSON.swift (1)
jsonObject(79-89)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/UtilitiesTests.swift
[Warning] 255-255: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 288-288: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: build
- GitHub Check: check
maratal
force-pushed
the
2135-http-error-response-as-string
branch
from
4bd71dc to
e3d37c9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2136/features
github-actions
bot
temporarily deployed
to
staging/pull/2136/jazzydoc
maratal
force-pushed
the
2135-http-error-response-as-string
branch
from
e3d37c9 to
76fd593
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
899-917: Critical: Fix statusCode calculation in stub JSON.The calculation on line 909 uses
modf(Float(self.value)/100).0, which returns the fractional component, not the whole number part. For error codes like 40400:
40400 / 100 = 404.0modf(404.0)returns(fractionalPart: 0.0, integerPart: 404.0)- Accessing
.0yields0.0(the fractional part)This causes all simulated errors to have
statusCode: 0in the JSON payload, undermining the test coverage.Apply this fix to use integer division:
let jsonObject: [String: Any] = [ "error": [ - "statusCode": modf(Float(self.value)/100).0, //whole number part + "statusCode": self.statusCode, "code": self.value, "message": self.description, "serverId": self.serverId,This uses the already-correct
statusCodeparameter passed to the initializer.
🧹 Nitpick comments (1)
Source/ARTJsonLikeEncoder.m (1)
1119-1136: Implementation correctly handles both error payload formats.The decoder now supports:
- String error:
{"error": "message"}→ constructs ARTErrorInfo withstatusCode * 100as code- Dictionary error:
{"error": {"code": 40400, "message": "..."}}→ extracts fields with fallback to statusCode-based valuesConsider using an empty string
@""ornilinstead of@"<Error message was not provided>"on line 1130 to match the behavior when the error field itself is a string (which uses the string value directly). This would provide more consistent behavior across both cases:- message:decodedError[@"message"] ?: @"<Error message was not provided>"]; + message:decodedError[@"message"] ?: @""];
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
Test/AblyTests/Test Utilities/JSON.swift (1)
jsonObject(79-89)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1085-1089)simulateIncomingServerErrorOnNextRequest(1091-1095)data(932-934)execute(832-858)execute(944-955)execute(1033-1083)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/UtilitiesTests.swift
[Warning] 262-262: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 297-297: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🔇 Additional comments (6)
Source/PrivateHeaders/Ably/ARTEncoder.h (1)
100-100: LGTM! Protocol signature correctly updated.The addition of the
statusCodeparameter enables the decoder to construct proper error information when the error payload lacks explicit status codes.Test/AblyTests/Tests/UtilitiesTests.swift (2)
242-272: Test correctly validates string error handling.This test verifies that when the server returns
{"error": "Object not found"}, the decoder constructs anARTErrorInfowith the appropriate code, statusCode, and message.Note: SwiftLint flags the unused
responseparameter on line 262. This is acceptable since the test only validates the error, but you can suppress the warning by replacingresponsewith_if preferred:-rest.internal.execute(request, wrapperSDKAgents:nil, completion: { response, _, error in +rest.internal.execute(request, wrapperSDKAgents:nil, completion: { _, _, error in
274-307: Test correctly validates dictionary error handling.This test verifies that when the server returns a nested error object
{"error": {"code": 40400, "message": "Object not found"}}, the decoder extracts the fields properly.Same SwiftLint note as the previous test—replace
responsewith_on line 297 if you want to suppress the warning:-rest.internal.execute(request, wrapperSDKAgents:nil, completion: { response, _, error in +rest.internal.execute(request, wrapperSDKAgents:nil, completion: { _, _, error inSource/ARTRest.m (1)
405-407: LGTM! All call sites correctly updated.All invocations of
decodeErrorInfonow passresponse.statusCode, enabling the encoder to use it as a fallback when constructing error information. The changes are consistent and correct.Also applies to: 416-418, 436-439, 567-569
Test/AblyTests/Test Utilities/TestUtilities.swift (2)
931-935: LGTM! Helper extension for JSON serialization.This extension provides a convenient way to convert dictionary payloads to
Datafor the error simulator.
1042-1055: LGTM! Improved error simulation API.The changes enhance
TestProxyHTTPExecutor:
- Line 1042: Uses
let requestURLinstead ofvar _for better clarity- Line 1085: Adds
statusCodeanddataparameters with sensible defaults, making error simulation more flexibleAlso applies to: 1085-1089
github-actions
bot
temporarily deployed
to
staging/pull/2136/features
github-actions
bot
temporarily deployed
to
staging/pull/2136/jazzydoc
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1085-1089)simulateIncomingServerErrorOnNextRequest(1091-1095)data(932-934)execute(832-858)execute(944-955)execute(1033-1083)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/UtilitiesTests.swift
[Warning] 262-262: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 297-297: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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
- GitHub Check: build
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
maratal
force-pushed
the
2135-http-error-response-as-string
branch
from
76fd593 to
32cbaf9
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2136/features
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Test/AblyTests/Tests/UtilitiesTests.swift (2)
242-272: Address SwiftLint warning: replace unused closure parameter with underscore.The
responseparameter in the completion closure is not used and should be replaced with_to address the SwiftLint warning.Apply this diff:
- rest.internal.execute(request, wrapperSDKAgents:nil, completion: { response, _, error in + rest.internal.execute(request, wrapperSDKAgents:nil, completion: { _, _, error in guard let error = error as? ARTErrorInfo else {
274-307: Address SwiftLint warning: replace unused closure parameter with underscore.The
responseparameter in the completion closure is not used and should be replaced with_to address the SwiftLint warning.Apply this diff:
- rest.internal.execute(request, wrapperSDKAgents:nil, completion: { response, _, error in + rest.internal.execute(request, wrapperSDKAgents:nil, completion: { _, _, error in guard let error = error as? ARTErrorInfo else {
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/PrivateHeaders/Ably/ARTEncoder.h
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1085-1089)simulateIncomingServerErrorOnNextRequest(1091-1095)data(932-934)execute(832-858)execute(944-955)execute(1033-1083)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
Test/AblyTests/Test Utilities/JSON.swift (1)
jsonObject(79-89)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/UtilitiesTests.swift
[Warning] 262-262: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 297-297: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: build
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check
🔇 Additional comments (6)
Source/ARTRest.m (2)
405-418: LGTM! StatusCode propagation is correct.The changes correctly propagate
response.statusCodeto the encoder'sdecodeErrorInfomethod and construct artificial errors with appropriate status codes when content type is invalid.
436-439: LGTM! Consistent statusCode handling throughout.The statusCode parameter is consistently passed to
decodeErrorInfoacross all error decoding paths, ensuring proper error information construction.Also applies to: 567-569
Source/ARTJsonLikeEncoder.m (1)
1119-1140: LGTM! Multi-branch error decoding correctly handles both formats.The implementation properly handles:
- String error payloads (lines 1121-1126) using statusCode-based defaults
- Dictionary error payloads (lines 1127-1135) with safe extraction via
artNumber/artStringhelpers- Invalid formats (lines 1136-1139) with debug logging
The use of
artNumberandartStringhelpers correctly handlesNSNullvalues by returningnil, which then triggers the nil-coalescing operators to use appropriate defaults. This addresses the concern from the previous review.Test/AblyTests/Test Utilities/TestUtilities.swift (3)
883-929: LGTM! Enhanced ErrorSimulator with flexible payload support.The new initializers provide flexibility:
- Explicit
stubDatainitializer (lines 891-897) for pre-serialized payloadsstubDataDictinitializer (lines 899-917) that builds default error JSON or accepts custom dictionariesThe
stubResponsemethod (lines 919-928) correctly builds HTTP responses with Ably-specific headers derived from error fields.
931-935: LGTM! Concise dictionary-to-Data extension.The extension provides a convenient way to serialize dictionaries for test payloads, with appropriate error handling via optional return type.
1042-1055: LGTM! Improved error simulation API.The updates provide:
- Optional
statusCodeanddataparameters for flexible error simulation (line 1085)- Overload accepting pre-built
ErrorSimulatorinstances (lines 1091-1095)- Cleaner
if letunwrapping pattern (line 1042) instead ofif varThese changes enable the new test scenarios for error decoding with both string and dictionary payloads.
Also applies to: 1085-1095
github-actions
bot
temporarily deployed
to
staging/pull/2136/jazzydoc
lawrence-forooghian
left a comment
lawrence-forooghian
left a comment
There was a problem hiding this comment.
If you believe that this fixes the customer's crash (which it sounds like is urgent for them), then we can merge it and do a release. But I believe that we're just covering up a broader issue — see #2135. If we do merge this, we should revert it soon and do a proper fix. What do you think?
I think that the exact behavior should be formalized in the specs and until then we can release this fix. I don't think it should be reverted though, but rather be applied on top because error parser method will be unchanged or even extended and the http callback rewritten significantly. |
Why would we need to handle an error being an |
Well, technically you are right, and I could just do this: But since we've already have decodedError as string error message, why not just put it there instead? |
|
I'm confused; why would we try and pick values out of any random API response that somebody might throw at us? It seems to me that trying to handle |
Ok, will remove. Should it be an empty string then in |
I would suggest just having a fallback error message something like |
actually it can be #2135; let's just not mark that as closed in this PR |
maratal
force-pushed
the
2135-http-error-response-as-string
branch
from
32cbaf9 to
cd70fb1
Compare
maratal
changed the title
github-actions
bot
temporarily deployed
to
staging/pull/2136/features
|
@lawrence-forooghian fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Source/PrivateHeaders/Ably/ARTEncoder.h
- Test/AblyTests/Test Utilities/TestUtilities.swift
🧰 Additional context used
🧬 Code graph analysis (1)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1085-1089)simulateIncomingServerErrorOnNextRequest(1091-1095)data(932-934)execute(832-858)execute(944-955)execute(1033-1083)
⏰ 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
- GitHub Check: check
- GitHub Check: build
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (tvOS, test_tvOS18_4)
🔇 Additional comments (4)
Source/ARTRest.m (2)
405-440: LGTM! Error handling correctly updated.The changes properly propagate
statusCodethrough the error decoding flow, aligning with the updatedARTEncoderprotocol signature. The multi-line formatting improves readability without changing semantics.
567-569: LGTM! Consistent error handling update.The
statusCodeparameter is correctly passed todecodeErrorInfoin the time method, maintaining consistency with other call sites.Test/AblyTests/Tests/UtilitiesTests.swift (2)
274-307: Test logic is correct.The test properly verifies error extraction when the response contains a complete error info dictionary with code and message fields.
242-272: Test logic is correct; no duplication detected.The test properly verifies the fallback behavior when the error response doesn't contain a dictionary error field, returning a generic HTTP status code message. Verification of the file shows these are two distinct test methods (
with_only_error_fieldat line 242 andwith_complete_error_infoat line 274), each testing different scenarios—no duplication exists.
github-actions
bot
temporarily deployed
to
staging/pull/2136/jazzydoc
maratal
force-pushed
the
2135-http-error-response-as-string
branch
from
cd70fb1 to
78b61c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
242-307: Cover the string-valuederrorfield explicitlyCould we tweak one of these stubs so the payload includes
"error": "Object not found"? That would exercise the exact shape that triggered ECO-5633 and guarantee this guard keeps working.
📜 Review details
Configuration used: CodeRabbit 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/ARTJsonLikeEncoder.m(1 hunks)Source/ARTRest.m(4 hunks)Source/PrivateHeaders/Ably/ARTEncoder.h(1 hunks)Test/AblyTests/Test Utilities/TestUtilities.swift(4 hunks)Test/AblyTests/Tests/UtilitiesTests.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Source/ARTRest.m
🧰 Additional context used
🧬 Code graph analysis (2)
Test/AblyTests/Tests/UtilitiesTests.swift (1)
Test/AblyTests/Test Utilities/TestUtilities.swift (8)
commonAppSetup(107-142)clientOptions(144-159)simulateIncomingServerErrorOnNextRequest(1085-1089)simulateIncomingServerErrorOnNextRequest(1091-1095)data(932-934)execute(832-858)execute(944-955)execute(1033-1083)
Test/AblyTests/Test Utilities/TestUtilities.swift (1)
Test/AblyTests/Test Utilities/JSON.swift (1)
jsonObject(79-89)
🪛 SwiftLint (0.57.0)
Test/AblyTests/Tests/UtilitiesTests.swift
[Warning] 262-262: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 297-297: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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
- GitHub Check: build
- GitHub Check: check (iOS, test_iOS18_4)
- GitHub Check: check (macOS, test_macOS)
- GitHub Check: check (tvOS, test_tvOS18_4)
- GitHub Check: check
🔇 Additional comments (4)
Source/PrivateHeaders/Ably/ARTEncoder.h (1)
100-101: Carry status codes through the decode contractGreat to see the status code flowing through the encoder API; that'll keep downstream contexts aligned with the HTTP layer.
Source/ARTJsonLikeEncoder.m (1)
1119-1136: Robust fallback for non-dictionary error payloadsAppreciate the type guard plus fallback message—this removes the crash we saw with string payloads while still surfacing a sensible
ARTErrorInfo.Test/AblyTests/Test Utilities/TestUtilities.swift (2)
883-918: Nice helper for pre-serialized stub payloadsThe tailored initializers plus the
[String: Any].data()helper make it far simpler to craft realistic server responses in tests while keeping the Ably headers intact.
1042-1053: Stub responses now surface deterministic bodiesReusing the stored
stubDatawhether or not the underlying request runs keeps the executor’s behaviour predictable for callers—thanks for tightening that up.
Addresses #2135
Summary by CodeRabbit
Bug Fixes
Tests
Test Utilities
Chores