[ECO-5633] Add handling http JSON error field as string.

[maratal]

Closes #2135

Summary by CodeRabbit

• Bug Fixes

  • Error decoding now respects HTTP status codes and properly handles error payloads that are either a string or an object, providing sensible defaults when fields are missing.

• Tests

  • Added tests verifying decoding for minimal ("error" string) and full nested error payloads.

• Test Utilities

  • Enhanced test HTTP simulator to support custom status codes, optional payloads, and re-usable error simulators for more realistic error scenarios.

[coderabbitai[bot]]

Walkthrough

Adds 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

Cohort / File(s)	Summary
Protocol Definition Source/PrivateHeaders/Ably/ARTEncoder.h	Added statusCode:(NSInteger)statusCode parameter to - (nullable ARTErrorInfo *)decodeErrorInfo:... in the ARTEncoder protocol.
Encoder Implementation Source/ARTJsonLikeEncoder.m	Method signature changed to decodeErrorInfo:statusCode:error:; decode now distinguishes NSDictionary vs non-dictionary payloads, extracts code, status and message with fallbacks (code → statusCode * 100, status → statusCode, message → provided or default HTTP message), and returns ARTErrorInfo in all branches.
Call Sites / REST Source/ARTRest.m	Updated calls to pass response.statusCode into decodeErrorInfo:statusCode:error:; minor formatting/rewrap of artificial error creation only.
Test Utilities Test/AblyTests/Test Utilities/TestUtilities.swift	Added explicit stubData to ErrorSimulator, new initializers accepting stubData/stubDataDict, dictionary→Data helper, simulateIncomingServerErrorOnNextRequest overloads with optional statusCode and payload data, and added Ably-specific response headers in stubs.
Unit Tests Test/AblyTests/Tests/UtilitiesTests.swift	Added tests verifying decoding when error is a string and when error is an object; note: each new test appears duplicated in the file.

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

• Review focus:
  • Source/ARTJsonLikeEncoder.m: correctness of type checks (NSDictionary vs others), proper use of statusCode fallbacks and arithmetic (statusCode * 100), memory/error pointer conventions, and comments about non-dictionary payloads.
  • Source/ARTRest.m: ensure every decodeErrorInfo call site now passes response.statusCode and preserves existing error semantics.
  • Test/AblyTests/Tests/UtilitiesTests.swift: remove duplicated tests or confirm intentional duplication; verify assertions match new fallback semantics.
  • Test/AblyTests/Test Utilities/TestUtilities.swift: confirm stubbed response headers and serialized stubData match decoder expectations.

Poem

🐰 I nibbled bytes of JSON, string and map,

Status seeded codes upon my lap.
When errors come odd, be they text or art,
I stitch fallbacks tidy — a rabbit's small part. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly addresses the main change: handling HTTP JSON errors that are not dictionaries, which matches the PR's core objective from ECO-5633.
Linked Issues check	✅ Passed	The PR successfully implements handling for HTTP JSON error responses where the error field is not a dictionary, supporting both dictionary and string/other payloads with appropriate fallbacks.
Out of Scope Changes check	✅ Passed	All changes are directly related to the stated objective of handling non-dictionary error payloads; signature updates, error decoding logic, test utilities, and new tests all support this primary goal.

✨ 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 2135-http-error-response-as-string

---

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

[maratal]

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.

[lawrence-forooghian]

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

Why would we need to handle an error being an NSString? In fact, now that I look at it, why do we even want such a handling now? There's no reason to expect a string-valued error in the response body. We should just make sure that we do something sensible in the case where the error is missing or not a dictionary, no?

[maratal]

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

Why would we need to handle an error being an NSString? In fact, now that I look at it, why do we even want such a handling now? There's no reason to expect a string-valued error in the response body. We should just make sure that we do something sensible in the case where the error is missing or not a dictionary, no?

Well, technically you are right, and I could just do this:

        return [ARTErrorInfo createWithCode:statusCode * 100
                                     status:statusCode
                                    message:@"<empty message>"];

But since we've already have decodedError as string error message, why not just put it there instead?

[lawrence-forooghian]

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 error: String in the response is no different to trying to handle any other arbitrary key-value pair in the response (e.g. errorCode: Int or fooCorpCustomErrorDescription: String).

[maratal]

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 error: String in the response is no different to trying to handle any other arbitrary key-value pair in the response (e.g. errorCode: Int or fooCorpCustomErrorDescription: String).

Ok, will remove. Should it be an empty string then in [ARTErrorInfo createWithCode:status:message:]?

[lawrence-forooghian]

Should it be an empty string then in [ARTErrorInfo createWithCode:status:message:]?

I would suggest just having a fallback error message something like "HTTP request failed with status code \(statusCode)". And I'd add a comment explaining why we have this handling (i.e. we are currently mistakenly trying to handle authURL responses as if they were from the Ably REST API, so we need to not blow up if there isn't an Ably-formatted error, but that we'll fix this properly soon, with a link to a GitHub issue).

[lawrence-forooghian]

with a link to a GitHub issue

actually it can be #2135; let's just not mark that as closed in this PR

[maratal]

@lawrence-forooghian fixed