Skip to content
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.

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

REST - Channels & publishing messages #45

Merged
merged 52 commits into from May 27, 2021

Conversation

tiholic
Copy link
Contributor

@tiholic tiholic commented Sep 14, 2020

Following conditions picked from Ruby Spec and IDL, as acceptance criteria for this PR:

Conditions to be satisfied for <String input>:

  1. can be UTF-8 string
  2. if given as SHIFT_JIS string will be converted to UTF-8, but is compatible with original encoding
  3. if given as ASCII_8BIT string will be converted to UTF-8, but is compatible with original encoding

Channels spec and Channels object spec: IDL | Ruby Spec | Ruby Spec - 1

  • (RSN1) Channels is a collection of Channel objects accessible through Rest#channels
    • Must override List accessor operator and return Channel instance if available, otherwise create one. This should work similar to the get function as per RSN3a, but without ChannelOptions.
    • Allows querying length
    • each returns an iterator and yields a channel on each iteration.
  • (RSN2) Methods should exist to check if a channel exists or iterate through the existing channels
  • (RSN3) Channels#get function:
    • (RSN3a) Creates a new Channel object for the specified channel if none exists, or returns the existing channel. ChannelOptions can be provided in an optional second argument
    • (RSN3b) If options are provided, the options are set on the Channel
    • (RSN3c) Accessing an existing Channel with options in the form Channels#get(channel, options) will update the options on the channel and then return the existing Channel object. (Note that this is soft-deprecated and may be removed in a future release, so should not be implemented in new client libraries. The supported way to update a set of ChannelOptions is Channel#setOptions -- Totally depends on ably-java and ably-cocoa)
    • (RSN4) Channels#release function:
      • (RSN4a) Takes one argument, the channel name, and releases the corresponding channel entity (that is, deletes it to allow it to be garbage collected)
      • (RSN4b) Calling release() with a channel name that does not correspond to an extant channel entity must return without error

#publish method of Channel spec : IDL | Ruby Spec

  • (RSL1a) Expects either a Message object, an array of Message objects, or a name string and data payload
  • (RSL1b) When name and data (or a Message) is provided, a single message is published to Ably
  • (RSL1c) When an array of Message objects is provided, a single request is made to Ably
  • (RSL1d) Raises an error if the message was not successfully published to Ably
  • (RSL1e) Allows name and/or data to be null. If any of the values are null, that property is not sent to Ably
  • (RSL1m) The Message.clientId must be left alone (that is, it will be there if it was set in the Message object passed to publish(), else left unset). The client library must not try and set it from the client-library-wide clientId; that is achieved by RSA7e for basic auth or implicit in the credentials for token auth. The following tests can be used to check for correct clientId handling:
    • (RSL1m1) Publishing a Message with no clientId when the clientId is set to some value in the client options should result in a message received with the clientId property set to that value
    • (RSL1m2) Publishing a Message with a clientId set to the same value as the clientId in the client options should result in a message received with the clientId property set to that value
    • (RSL1m3) Publishing a Message with a clientId set to a value from an unidentified client (no clientId in the client options and credentials that can assume any clientId) should result in a message received with the clientId property set to that value
    • (RSL1m4) Publishing a Message with a clientId set to a different value from the clientId in the client options should result in a message being rejected by the server.
  • (RSL1h) The publish(name, data) form should not take any additional arguments. If a client library has supported additional arguments to the (name, data) form (e.g. separate arguments for clientId and extras, or a single attributes argument) in any 1.x version, it should continue to do so until version 2.0. (applicable for platform)
  • (RSL1i) If the total size of the message or (if publishing an array) messages, calculated per TO3l8, exceeds the maxMessageSize, then the client library should reject the publish and indicate an error with code 40009
  • (RSL1j) When Message objects are provided, any valid Message attribute (that is, an attribute specified in TM2) that is supplied by the caller must be included in the encoded message. (This does not mean it must be included unaltered; for example the data and encoding will be subject to processing per RSL4) (applicable for platform)
  • without adequate permissions on the channel raises a permission error when publishing (with code 40160. as per https://github.com/ably/ably-common/blob/main/protocol/errors.json )
  • With a non-ASCII channel name: stubs and correctly encodes channel name.

Message object spec : IDL | Ruby Spec

Attributes in message:

  • #id retrieves attribute :id
  • #name retrieves attribute :name
  • #client_id retrieves attribute :client_id
  • #data retrieves attribute :data
  • #encoding retrieves attribute :encoding
  • == - is true when attributes are the same, is false when attributes are not the same, is false when class type differs
  • Message once created is immutable
  • #timestamp retrieves timestamp as DateTime object
  • #extras (#TM2i) Can be null, map, or list. Should throw exception or not allow syntactically to pass string or number, or any other types.
  • #connection_id can be null or a string

Message object can be initialized with:

  • name as "<String input>" (defined above), or null but other types are not allowed
  • client_id as "<String input>" (defined above), or null but other types are not allowed
  • encoding as "<String input>" (defined above), or null but other types are not allowed

Channel object spec: Ruby Spec

channel name argument in constructor

  • Satisfies conditions defined above for "<String input>"
  • cannot be null, or any other type

name argument of #publish

  • Satisfies conditions defined above for "<String input>"
  • int or any other types is not permitted

lib/src/impl/platform_object.dart Outdated Show resolved Hide resolved
lib/src/impl/realtime/channels.dart Outdated Show resolved Hide resolved
lib/src/impl/realtime/connection.dart Outdated Show resolved Hide resolved
@tiholic tiholic changed the base branch from main to integration/stage-2 October 14, 2020 13:26
@QuintinWillison QuintinWillison added this to the Stage 2 milestone Oct 19, 2020
@tiholic tiholic modified the milestones: Stage 1.1 , Stage 2 Nov 25, 2020
Base automatically changed from integration/stage-2 to main February 16, 2021 13:16
@tiholic tiholic force-pushed the feature/rest-channels-and-messages branch from 180d083 to 13f828b Compare March 2, 2021 21:15
@tiholic tiholic changed the base branch from main to feature/optimize-integration-tests March 2, 2021 21:19
@tiholic tiholic force-pushed the feature/optimize-integration-tests branch from 13f828b to 3e49eec Compare March 2, 2021 21:26
@tiholic tiholic force-pushed the feature/rest-channels-and-messages branch 3 times, most recently from d0e2dc5 to 382baf0 Compare March 8, 2021 16:28
Base automatically changed from feature/optimize-integration-tests to main April 26, 2021 18:09
@github-actions github-actions bot temporarily deployed to staging/pull/45/dartdoc May 4, 2021 15:05 Inactive
@tiholic tiholic marked this pull request as ready for review May 4, 2021 16:37
@github-actions github-actions bot temporarily deployed to staging/pull/45/dartdoc May 4, 2021 16:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/45/dartdoc May 7, 2021 14:51 Inactive
I ran `flutter pub get` from repository root.
I also had to run `flutter but get` from the `test_integration` folder.

We should have done this as part of the last release.
@github-actions github-actions bot temporarily deployed to staging/pull/45/dartdoc May 11, 2021 12:58 Inactive
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Given the size of this pull request it's been impossible for me to go through every line at the level of detail I would ideally like to, but I've been able to spend enough time to get a reasonably good idea of what's changed.

Some changes needed, please.

lib/src/codec.dart Outdated Show resolved Hide resolved
lib/src/codec.dart Outdated Show resolved Hide resolved
lib/src/codec.dart Show resolved Hide resolved
lib/src/spec/common.dart Show resolved Hide resolved
lib/src/spec/constants.dart Outdated Show resolved Hide resolved
lib/src/spec/message.dart Outdated Show resolved Hide resolved
ios/Classes/codec/AblyPlatformConstants.h Outdated Show resolved Hide resolved
ios/Classes/AblyFlutterPlugin.m Outdated Show resolved Hide resolved
ios/Classes/codec/AblyPlatformConstants.m Outdated Show resolved Hide resolved
tiholic and others added 8 commits May 20, 2021 14:55
Co-authored-by: Quintin Willison <q@qwuk.net>
Co-authored-by: Quintin Willison <q@qwuk.net>
Co-authored-by: Quintin Willison <q@qwuk.net>
dart's @Protected is only an annotation and not
 a keyword. It only provides better hinting,
 and doesn't enforce/show errors compile time.
 So it isn't propagated to it's implementation.
@tiholic tiholic force-pushed the feature/rest-channels-and-messages branch from d552f09 to 5dc16a3 Compare May 20, 2021 10:58
@github-actions github-actions bot temporarily deployed to staging/pull/45/dartdoc May 27, 2021 13:16 Inactive
Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

I've not gone through line by line, though I have checked the commits you've submitted since my last review in order to resolve those conversations.

The main branch was out of date so I've boshed GitHub's update button, resolving that with the merge commit that is fa34b43.

Once checks pass then I'm happy to land this.

not supporting list anymore
extras doesn't support list type anymore
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants