[ECO-5633] Avoid parsing an http body error if response is not from Ably

[maratal]

Closes #2135

Summary by CodeRabbit

• Bug Fixes

  • Improved error handling with better error messages and accurate HTTP status code mapping.
  • Enhanced underlying error propagation for authentication and connection failures.
  • Better handling of malformed server responses and edge cases.

• Tests

  • Added authentication error scenario test coverage.
  • Updated error handling tests for improved validation.

[coderabbitai]

Walkthrough

This PR enhances error handling across REST and Realtime components by introducing HTTP-status-aware error construction, supporting underlying error propagation, and improving validation of JSON error responses to handle non-dictionary formats gracefully.

Changes

Cohort / File(s)	Summary
Error handling foundation Source/ARTStatus.m, Source/include/Ably/ARTStatus.h, Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h	Introduces ARTHttpStatusCodeFromErrorCode() function to map error codes to HTTP-like statuses. Adds new factory methods createWithCode:status:message:underlyingError: and extends existing factory signatures to include underlyingError parameter for better error context propagation.
REST client error validation Source/ARTRest.m	Adds three public helper methods: isAblyResponse:, hasValidContentType:, and isCustomAuthRequest: for improved response/request validation. Refines token renewal logic to decode error info only when response is Ably-related and encoder exists. Introduces deviceAccessQueue class method for device-access serialization.
Realtime connection error handling Source/ARTRealtime.m	Replaces generic error constructions with HTTP-status-aware variants in onConnectionTimeOut and handleTokenAuthError. Maps specific error codes (ARTErrorForbidden, ARTErrorUnauthorized) to appropriate HTTP statuses via ARTHttpStatusCodeFromErrorCode. Attaches underlyingError to ARTErrorInfo creation.
JSON error decoding Source/ARTJsonLikeEncoder.m	Changes decodeErrorInfo to return ARTClientCodeErrorInvalidType with status 0 when encountering non-dictionary error responses, replacing generic error construction for improved error distinction.
Authentication tests Test/AblyTests/Tests/AuthTests.swift	Adds new test validating realtime client behavior when authUrl returns HTTP 404, asserting proper state transition and error code propagation.
Error response handling tests Test/AblyTests/Tests/UtilitiesTests.swift	Updates test methods to verify proper handling of error responses with non-dictionary structures, adjusting assertions to expect invalidType error codes for malformed JSON error responses.

Sequence Diagram

sequenceDiagram
    participant REST as REST Client
    participant Server as HTTP Server
    participant Decoder as JSON Decoder
    participant ErrorInfo as ErrorInfo Factory

    rect rgb(220, 240, 220)
    note over REST,ErrorInfo: New: Error Response Validation Flow
    REST->>Server: HTTP Request
    Server-->>REST: HTTP Error Response (JSON)
    rect rgb(240, 240, 255)
    note over REST: isAblyResponse() + hasValidContentType()
    REST->>REST: Validate Ably headers & Content-Type
    end
    alt Valid Ably Response
        REST->>Decoder: Decode error info
        alt Valid dictionary
            Decoder-->>ErrorInfo: error dict
            ErrorInfo->>ErrorInfo: createWithCode:status:message:underlyingError:
            ErrorInfo-->>REST: ARTErrorInfo with status & underlying error
        else Non-dictionary error
            Decoder-->>ErrorInfo: ARTClientCodeErrorInvalidType
            ErrorInfo-->>REST: 0 status + decode failure message
        end
    else Invalid/Non-Ably Response
        REST->>REST: Handle as plaintext/HTML error
    end
    end

    rect rgb(240, 220, 220)
    note over REST,ErrorInfo: Realtime: Connection Error Handling
    REST->>REST: Auth timeout or token error
    rect rgb(240, 240, 255)
    note over REST: Check auth provider type
    end
    alt Auth via provider
        REST->>ErrorInfo: ARTHttpStatusCodeFromErrorCode()
        ErrorInfo-->>REST: HTTP status from error code
        REST->>ErrorInfo: createWithCode:status:message:underlyingError:
    else Token auth error
        REST->>ErrorInfo: Map ARTErrorForbidden/Unauthorized
        ErrorInfo-->>REST: Appropriate HTTP status + underlying error
    end
    ErrorInfo-->>REST: Enhanced ARTErrorInfo
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

• ARTRest.m: New public helper methods and refined token renewal logic require careful review of validation conditions and error-response handling pathways
• ARTStatus.m & ARTStatus.h: Changes to error factory method signatures and new underlyingError propagation need validation across all call sites
• ARTRealtime.m: Multiple error handling branches with HTTP-status mapping require verification of correct status/error-code pairing
• ARTJsonLikeEncoder.m: Behavioral shift from generic error to invalidType requires confirmation that downstream code handles the new error code properly
• Cross-file interaction: The new ARTHttpStatusCodeFromErrorCode() function is used across multiple files; ensure consistent mapping logic

Poem

🐰 Through tangled error paths we hop and bound,
With status codes and reasons newly found,
No more we stumble on a missing dict,
Our underlying errors now are strict!
Each REST response now validates with care,
And realtime knows precisely why it's there! ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly references ECO-5633 and directly describes the main change: avoiding parsing HTTP body errors when responses are not from Ably.
Linked Issues check	✅ Passed	The PR addresses the core requirement from #2135/ECO-5633: handling JSON error responses where the 'error' field may be a string rather than an object, by introducing validation to avoid parsing non-Ably responses.
Out of Scope Changes check	✅ Passed	Changes extend error handling infrastructure and add helper methods to detect Ably responses and validate content types, which are necessary to implement the scope requirement of safely handling non-dictionary error values.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/2135-handle-error-in-user-content

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 (1)

Source/ARTJsonLikeEncoder.m (1)

1123-1137: Guard against nil error pointer in decodeErrorInfo

The new branch correctly maps non-dictionary (or missing) error payloads to an ARTClientCodeErrorInvalidType, which is what the new tests expect. However, it unconditionally writes through *error, which will crash if the caller passes error == NULL.

Safer pattern:

-    // We expect `decodedError` as a dictionary from Ably REST API
-    // In case it's something else we set the decoding error
-    *error = [ARTErrorInfo createWithCode:ARTClientCodeErrorInvalidType
-                                   status:0
-                                  message:[NSString stringWithFormat:@"Failed to decode error dictionary from the provided error data."]];
+    // We expect `decodedError` as a dictionary from the Ably REST API.
+    // If it's missing or has the wrong type, surface a decoding error.
+    if (error) {
+        *error = [ARTErrorInfo createWithCode:ARTClientCodeErrorInvalidType
+                                       status:0
+                                      message:@"Failed to decode error dictionary from the provided error data."];
+    }

This keeps the new behavior but avoids relying on every caller always passing a non-null error pointer.

🧹 Nitpick comments (5)

Test/AblyTests/Tests/UtilitiesTests.swift (1)

242-271: New invalid-type REST HTTP error test looks correct

This test cleanly verifies the new behavior when the response body doesn’t contain a proper error dictionary and asserts the ARTClientCodeError.invalidType mapping and message. This matches the new decoder semantics and guards against mis-parsing non-Ably-style error payloads. Only minor nit: the method name has a small grammar issue (doesnt_contains) if you ever want to tidy it.

Source/ARTRealtime.m (1)

1196-1219: Improved token auth error modeling and underlying error propagation

The updated handleTokenAuthError: path for custom auth providers is a solid improvement:

• Forbidden (ARTErrorForbidden) errors are mapped to ARTErrorAuthConfiguredProviderFailure with an HTTP-like 403 status and the underlying NSError attached.
• Other auth-provider errors are mapped similarly but with a 401-like status, with behavior correctly split between:
  • Connected state (connection.errorReason updated, RSA4c3), and
  • Other states (transition via performTransitionToDisconnectedOrSuspendedWithParams, RSA4c1/c2).

This should make provider failures easier to diagnose without changing the public API surface.

Source/ARTRest.m (3)

338-363: Helper methods for Ably-response and content-type detection look good, with a minor robustness nit

The new helpers cleanly encapsulate key decisions:

• isAblyResponse: infers Ably responses by checking for any header name containing "-ably", which is a simple and effective heuristic given the existing X-Ably-* headers.
• hasValidContentType: checks the response Content-Type against the known encoder MIME types.
• isCustomAuthRequest: centralizes the authUrl-host check for later guards.

One minor robustness improvement you might consider: normalize Content-Type to lowercase before containsString: to cope with servers that emit mixed-case types like "Application/JSON; charset=utf-8":

-    NSString *contentType = [response.allHeaderFields objectForKey:@"Content-Type"];
+    NSString *contentType = [[response.allHeaderFields objectForKey:@"Content-Type"] lowercaseString];
@@
-    for (NSString *mimeType in [self->_encoders.allValues valueForKeyPath:@"mimeType"]) {
-        if ([contentType containsString:mimeType]) {
+    for (NSString *mimeType in [[self->_encoders.allValues valueForKeyPath:@"mimeType"] valueForKey:@"lowercaseString"]) {
+        if (contentType && [contentType containsString:mimeType]) {
             return true;
         }
     }

Not strictly required, but it would make hasValidContentType: more tolerant of non-standard casing.

---

416-461: HTTP error handling now correctly distinguishes Ably vs non-Ably responses

The updated executeRequest:...completion: logic for error responses is a solid improvement:

• For non-auth requests with bodies and unknown content types, it continues to synthesize an ARTErrorInfo from the plaintext/HTML body, but now guarded against custom authUrl requests via isCustomAuthRequest:.
• For statusCode >= 400, it:
  • Attempts to decode an Ably error only when an encoder is available and isAblyResponse: is true, so arbitrary third-party JSON (e.g. {"error": "string"}) is no longer misinterpreted as an Ably error.
  • Uses dataError from the encoder when present, otherwise falls back to decodeError (e.g. invalid-type decoding issues) or a generic, body-derived artificial error when decoding fails.
  • Only calls shouldRenewToken: when dataError comes from a bona fide Ably error, avoiding spurious token renewal attempts on non-Ably endpoints.

This addresses the ECO‑5633 scenario while preserving existing retry/fallback behavior.

---

799-865: Device access serialization via deviceAccessQueue is a nice concurrency improvement

On iOS, introducing deviceAccessQueue and routing device_nosync / sharedDevice_onlyCallOnDeviceAccessQueue through it is a good step towards eliminating races on the shared device state and persisted storage. The pattern (single static queue + “needs loading” flag) looks correct and avoids nested dispatches on the same queue.

Just ensure any new call sites that touch the shared device go through these helpers rather than accessing the static directly.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 39da0cf and 3615757.

📒 Files selected for processing (8)

• Source/ARTJsonLikeEncoder.m (1 hunks)
• Source/ARTRealtime.m (3 hunks)
• Source/ARTRest.m (4 hunks)
• Source/ARTStatus.m (3 hunks)
• Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h (1 hunks)
• Source/include/Ably/ARTStatus.h (2 hunks)
• Test/AblyTests/Tests/AuthTests.swift (1 hunks)
• Test/AblyTests/Tests/UtilitiesTests.swift (2 hunks)

🔇 Additional comments (12)

Test/AblyTests/Tests/AuthTests.swift (1)

884-884: Use a controlled test endpoint instead of an external API.

The test relies on https://api.restful-api.dev/objects/{UUID} to generate a 404 response. This external, uncontrolled endpoint could become unavailable, change behavior, or cause test flakiness in CI environments.

Similar tests in this file (e.g., line 529) use https://echo.ably.io/respondwith?status=404 which provides a controlled, deterministic way to simulate HTTP errors.

Consider changing line 884 to:

-        options.authUrl = URL(string: "https://api.restful-api.dev/objects/\(UUID())")! // random address to get 404
+        options.authUrl = URL(string: "https://echo.ably.io/respondwith?status=404")!

Test/AblyTests/Tests/UtilitiesTests.swift (1)

273-306: Happy-path REST HTTP error decoding test is well-aligned with decoder behavior

This test correctly exercises the case where an Ably-style error dictionary is present under the error key and asserts code, statusCode, and message. It gives good coverage for the updated decodeErrorInfo and ARTRest error construction flow.

Source/PrivateHeaders/Ably/ARTErrorInfo+Private.h (1)

15-17: HTTP-status mapping helper is reasonable and well-scoped

The ARTHttpStatusCodeFromErrorCode helper and its comment are clear and match how it’s used (e.g. mapping 40100/40300 to 401/403). Keeping it in a private header is appropriate given the “no validation” contract.

Source/ARTRealtime.m (1)

1071-1079: Timeout error differentiation for custom auth vs generic timeouts looks sound

The new onConnectionTimeOut logic correctly:

• Treats timeouts while authorizing via authUrl/authCallback as ARTErrorAuthConfiguredProviderFailure with an HTTP-like 401 status (via ARTHttpStatusCodeFromErrorCode(ARTErrorUnauthorized)), and
• Falls back to ARTErrorConnectionTimedOut with status:ARTStateConnectionFailed otherwise.

This aligns Realtime behavior with the auth-provider-specific RSA4c expectations while preserving existing semantics for non-auth timeouts.

Source/include/Ably/ARTStatus.h (1)

234-236: Underlying-error-capable ARTErrorInfo factories are a good addition

The new createWithCode:status:message:underlyingError: and extended variant with requestId/additionalUserInfo give a clear path to propagate underlying NSErrors while keeping Ably-specific metadata. This aligns well with the new usages in Realtime/REST.

Since these are declared in the public header (even if :nodoc:), just double-check that:

• The corresponding implementations exist in ARTStatus.m, and
• All call sites that need an underlying error are migrated to these overloads (so we avoid continuing to wrap and lose the original error).

Also applies to: 256-256

Source/ARTStatus.m (7)

24-34: LGTM! Consistent status code derivation.

All factory methods now consistently use ARTHttpStatusCodeFromErrorCode to derive the HTTP status code from the error code. This ensures uniform behavior across different error creation paths.

---

36-38: LGTM! New factory method supports error chaining.

This new factory method enables creating errors with an underlying error, which is useful for wrapping lower-level errors (such as JSON parsing failures) in Ably-specific error objects. The method properly delegates to the main factory with appropriate nil defaults for optional parameters.

---

40-50: LGTM! Factory methods properly updated with underlyingError support.

All factory methods now properly chain to the main factory method with the underlyingError parameter (defaulting to nil when not explicitly provided). This maintains backward compatibility while enabling error chaining throughout the codebase.

---

52-65: LGTM! Underlying error properly stored using standard iOS pattern.

The main factory method now accepts and stores the underlyingError parameter using NSUnderlyingErrorKey, which is the standard iOS pattern for error chaining. The implementation correctly handles nil values (the key simply won't be added to userInfo), and the existing cause property (lines 126-132) is already compatible with this approach.

---

138-144: LGTM! Minor formatting improvements.

The formatting changes in the description method improve code readability without affecting functionality. The error description output remains unchanged.

---

191-193: LGTM! Minor formatting update.

Cosmetic formatting change to the private setter with no functional impact.

---

18-20: LGTM! Canonical status code mapping function introduced.

The function correctly derives HTTP status codes from error codes using integer division by 100. This assumes error codes follow the convention where the first digits represent the HTTP status (e.g., 40000-40099 → 400).

Please verify that ARTHttpStatusCodeFromErrorCode is properly declared in the public or private header file (likely ARTErrorInfo+Private.h as mentioned in the summary).