Skip to content

CHA-M5b1 and CHA-M13a1 proposals. #401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

CHA-M5b1 and CHA-M13a1 proposals. #401

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions textile/chat-features.textile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ Broadly speaking, messages are published via REST calls to the Chat HTTP API and
* @(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).
Copy link
Contributor

@AndyTWF AndyTWF Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@maratal maratal Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eventually

What if it's minutes?

Copy link
Contributor

@AndyTWF AndyTWF Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then right now, we wait

Copy link
Collaborator Author

@maratal maratal Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lawrence-forooghian wdyt, should we change the line to channel.once(.attached) to fit JS implementation?

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Collaborator Author

@maratal maratal Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

** @(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).

** @(CHA-M5c)@ @[Testable]@ If a channel leaves the @ATTACHED@ state and then re-enters @ATTACHED@ with @resumed=false@, then it must be assumed that messages have been missed. The @subscription point@ of any subscribers must be reset to the @attachSerial@.
** @(CHA-M5d)@ @[Testable]@ If a channel @UPDATE@ event is received and @resumed=false@, then it must be assumed that messages have been missed. The @subscription point@ of any subscribers must be reset to the @attachSerial@.
** @(CHA-M5e)@ Each subscription shall expose a method or callback that allows for messages to be queried. These messages are queried via the "REST API"#rest-fetching-messages-history.
Expand All @@ -409,6 +410,7 @@ Broadly speaking, messages are published via REST calls to the Chat HTTP API and
* @(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.
Copy link
Contributor

@AndyTWF AndyTWF Nov 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this covered by CHA-M13b (the REST API throws this combination as it stands)?

Copy link
Collaborator Author

@maratal maratal Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AndyTWF AndyTWF Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it does :)

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have asked about this in https://ably-real-time.slack.com/archives/CURL4U2FP/p1762431489639239

Copy link
Collaborator Author

@maratal maratal Nov 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be handled explicitly and not just described as "not found"

You mean define another 102_XXX for this?

Copy link
Collaborator Author

@maratal maratal Nov 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-M13b)@ @[Testable]@ If the REST API returns an error, then the method must throw its @ErrorInfo@ representation.
* @(CHA-M7)@ This specification point has been removed. It was valid up until the single-channel migration.
* @(CHA-M11)@ A @Message@ must have a method @with@ to apply changes from events (@MessageEvent@ and @MessageReactionSummaryEvent@) to produce updated message instances.
Expand Down
Loading