CHA-M5b1 and CHA-M13a1 proposals. #401
Conversation
- failedToResolveSubscriptionPointBecauseChannelSerialNotDefined: renamed since `channelSerial` checked first. Introduced error code used in JS - 102_110; - failedToResolveSubscriptionPointBecauseChannelFailedToAttach: changed error code to 102_112 (roomInInvalidState), JS doesn't throw anything and just waits for channel attach event. Proposed spec for throwing an error with 102_112 code; - noItemInResponse: changed error code to 404 and proposed spec for that; - failedToGetPaginatedResult: renamed and added full error info taken from realtime, JS also throws error with values from realtime. Am not sure what to do with `badRequest` here, probably it's fine to leave it (or `internalServerError` maybe?); - headersValueJSONDecodingError and jsonValueDecodingError are JSON parser errors and probably should stay with `badRequest`. Removed unused `MessagesError` enum. Proposed specs: ably/specification#401
| * @(CHA-M6b)@ @[Testable]@ If the REST API returns an error, then the method must throw its @ErrorInfo@ representation. | ||
| * @(CHA-M13)@ A single message must be retrievable from the REST API. | ||
| ** @(CHA-M13a)@ @[Testable]@ A method must be exposed that accepts a message serial and returns a single @Message@ object. It shall call the "REST API single message endpoint"#rest-fetching-single-message and return the message if found. | ||
| *** @(CHA-M13a1)@ @[Testable]@ If not found an error should be thrown with an error code 40400 and status code 404. |
There was a problem hiding this comment.
Isn't this covered by CHA-M13b (the REST API throws this combination as it stands)?
There was a problem hiding this comment.
Does realtime throw 404 tho? Also to what exactly other than 404 I can attribute our internal noItemInResponse error? The closest thing for the end user is 404 imho.
There was a problem hiding this comment.
Does realtime throw 404 tho?
Please confirm and also check that ably-cocoa is indeed translating this error response correctly to an ErrorInfo, since this core SDK behaviour is currently underspecified; see ably/ably-chat-swift#453
There was a problem hiding this comment.
I'm actually even more confused now about the expected behaviour of the core request() method in the face of non-2xx responses, having just found HP5 which describes HTTPPaginatedResponse.success:
success is a boolean attribute which is true when the HTTP status code indicates success i.e. 200 <= statusCode < 300
but if the core SDK is meant to intercept non-2xx and treat them as a request failure then I don't know under what circumstances we'd end up with an HTTPPaginatedResponse with success == false. Going to ask about this internally.
And then, yes, @maratal, your question about how to handle an empty response (our noItemInResponse) is still a valid one, but I would suggest that should be handled explicitly and not just described as "not found".
There was a problem hiding this comment.
Have asked about this in https://ably-real-time.slack.com/archives/CURL4U2FP/p1762431489639239
There was a problem hiding this comment.
should be handled explicitly and not just described as "not found"
You mean define another 102_XXX for this?
There was a problem hiding this comment.
Please confirm and also check that ably-cocoa is indeed translating this error response correctly to an ErrorInfo
It does, we now have test for it in this PR - test__Utilities__JSON_Encoder__should_decode_rest_error_response_with_only_error_field
It wasn't intended for this particular error code, but happened to be using it and cocoa creates ARTErrorInfo for it.
| * @(CHA-M5)@ For a given subscription, messages prior to the point of subscription can be retrieved in a history-like request. Note that this is the point in the message flow @(subscription point)@ at which the subscription was made, NOT the channel attachment point. | ||
| ** @(CHA-M5a)@ @[Testable]@ If a subscription is added when the underlying realtime channel is @ATTACHED@, then the @subscription point@ is the current @channelSerial@ of the realtime channel. | ||
| ** @(CHA-M5b)@ @[Testable]@ If a subscription is added when the underlying realtime channel is in any other state, then its @subscription point@ becomes the @attachSerial@ at the the point of channel attachment. | ||
| *** @(CHA-M5b1)@ @[Testable]@ If the channel fails to attach then an error should be thrown with an error code 102112 (room in invalid state). |
There was a problem hiding this comment.
In chat-js, we don't throw an error here - the SDK just waits until we eventually do become attached (we add a once listener for attached events).
I think it's worth considering though whether or not we want to specify that calls will fail with an error of disposed if the room gets released in the meantime?
There was a problem hiding this comment.
eventually
What if it's minutes?
There was a problem hiding this comment.
@lawrence-forooghian wdyt, should we change the line to channel.once(.attached) to fit JS implementation?
There was a problem hiding this comment.
Happy for us to do so but sounds like the spec needs to be more explicit e.g. CHA-M5b say "when the channel next transitions to the ATTACHED state"
There was a problem hiding this comment.
** @(CHA-M5b)@ @[Testable]@ If a subscription is added when the underlying realtime channel is in any other state, then its @subscription point@ becomes the @attachSerial@ when the channel next transitions to the ATTACHED state (in languages with await calling mechanism it means waiting indefinitely until channel transitions to ATTACHED).
- failedToResolveSubscriptionPointBecauseChannelSerialNotDefined: renamed since `channelSerial` checked first. Introduced error code used in JS - 102_110; - failedToResolveSubscriptionPointBecauseChannelFailedToAttach: changed error code to 102_112 (roomInInvalidState), JS doesn't throw anything and just waits for channel attach event. Proposed spec for throwing an error with 102_112 code; - noItemInResponse: changed error code to 404 and proposed spec for that; - failedToGetPaginatedResult: renamed and added full error info taken from realtime, JS also throws error with values from realtime. Am not sure what to do with `badRequest` here, probably it's fine to leave it (or `internalServerError` maybe?); - headersValueJSONDecodingError and jsonValueDecodingError are JSON parser errors and probably should stay with `badRequest`. Removed unused `MessagesError` enum. Proposed specs: ably/specification#401
- failedToResolveSubscriptionPointBecauseChannelSerialNotDefined: renamed since `channelSerial` checked first. Introduced error code used in JS - 102_110; - failedToResolveSubscriptionPointBecauseChannelFailedToAttach: changed error code to 102_112 (roomInInvalidState), JS doesn't throw anything and just waits for channel attach event. Proposed spec for throwing an error with 102_112 code; - noItemInResponse: changed error code to 404 and proposed spec for that; - failedToGetPaginatedResult: renamed and added full error info taken from realtime, JS also throws error with values from realtime. Am not sure what to do with `badRequest` here, probably it's fine to leave it (or `internalServerError` maybe?); - headersValueJSONDecodingError and jsonValueDecodingError are JSON parser errors and probably should stay with `badRequest`. Removed unused `MessagesError` enum. Proposed specs: ably/specification#401
- failedToResolveSubscriptionPointBecauseChannelSerialNotDefined: renamed since `channelSerial` checked first. Introduced error code used in JS - 102_110; - failedToResolveSubscriptionPointBecauseChannelFailedToAttach: changed error code to 102_112 (roomInInvalidState), JS doesn't throw anything and just waits for channel attach event. Proposed spec for throwing an error with 102_112 code; - noItemInResponse: changed error code to 404 and proposed spec for that; - failedToGetPaginatedResult: renamed and added full error info taken from realtime, JS also throws error with values from realtime. Am not sure what to do with `badRequest` here, probably it's fine to leave it (or `internalServerError` maybe?); - headersValueJSONDecodingError and jsonValueDecodingError are JSON parser errors and probably should stay with `badRequest`. Removed unused `MessagesError` enum. Proposed specs: ably/specification#401
maratal
requested review from
ttypic
and removed request for
SimonWoolf,
owenpearson and
vladvelici
No description provided.