pubsub: breaking changes to channels and channel operations

UPDATING.md

@@ -676,6 +676,112 @@ token = device.push.recipient['deviceToken']
 decoding a dict, where 3.x raised `TypeError` on any field the server added. A misspelled key
 in a dict you pass to `save()` is therefore ignored rather than rejected.
 
+### Setting channel options replaces them, cipher included
+
+**`set_options(ChannelOptions(params=...))` on an encrypted channel now turns encryption off.**
+`channel.set_options(options)`, and `channels.get(name, options)` on a channel that already
+exists, replace the channel's options whole rather than merging them, so options without a
+`cipher` leave the channel unencrypted. 3.x kept the old cipher whenever the new options left
+it out, and on a realtime channel even when they gave `cipher=None`. Pass the cipher again
+whenever you set the options of an encrypted channel:
+
+Example 3.x code:
+```python
+channel = realtime.channels.get('my-channel', ChannelOptions(cipher=cipher_params))
+await channel.set_options(ChannelOptions(params={'rewind': '1'}))
+```
+
+Example 4.0.0 code:
+```python
+channel = pubsub_realtime_client.channels.get('my-channel', ChannelOptions(cipher=cipher_params))
+await channel.set_options(ChannelOptions(cipher=cipher_params, params={'rewind': '1'}))
+```
+
+`channel.options` is now the `ChannelOptions` that was last set, rather than a dict, so read
+its attributes rather than its keys:
+
+Example 3.x code:
+```python
+rewind = channel.options['params']['rewind']
+```
+
+Example 4.0.0 code:
+```python
+rewind = channel.options.params['rewind']
+```
+
+HTTP channels now take a `ChannelOptions` too, through `channels.get(name, options)` or
+`await channel.set_options(options)`. `channels.get` raises when given options that would make
+an attaching or attached realtime channel reattach, which only a change to `params` or `modes`
+does; a change to the cipher alone no longer raises.
+
+### Channels are no longer attributes of the channels collection
+
+`client.channels.my_channel` used to get or create the channel called `my_channel`, because the
+collection answered any attribute it did not define with a channel of that name. That made a
+typo, or even `hasattr(client.channels, ...)`, create a channel. In 4.0.0 an unknown attribute
+raises `AttributeError`, so get the channel by name instead:
+
+Example 3.x code:
+```python
+channel = rest.channels.my_channel
+```
+
+Example 4.0.0 code:
+```python
+channel = pubsub_http_client.channels.get('my_channel')
+```
+
+### A space in a channel name reaches the channel it names over HTTP
+
+3.x wrote a space in a channel name as `+` in HTTP request paths, which the Ably service reads
+as a literal plus, so HTTP publishes, history, presence, annotations, message reads and
+`status()` for `my channel` all went to a different channel, `my+channel`, from the one
+realtime clients attach to. 4.0.0 writes the space as `%20`, so they reach `my channel`.
+Messages that 3.x stored over HTTP for such a name are in the `+` channel, and stay reachable
+by that name:
+
+Example 4.0.0 code:
+```python
+stored_by_3x = pubsub_http_client.channels.get('my+channel')
+history = await stored_by_3x.history()
+```
+
+`status()` now escapes the channel name too, so a name containing `/`, `?` or `#` no longer
+addresses the wrong resource.
+
+### `attach()` raises when the channel does not attach
+
+`await channel.attach()` raised only when the channel became `SUSPENDED` or `FAILED`. When the
+connection closed while the channel was attaching, the channel became `DETACHED` and `attach()`
+returned as if it had succeeded; so did an `attach()` overtaken by a `detach()`. In 4.0.0 it
+raises an `AblyException` whenever the channel moves to any state but `ATTACHED`, carrying the
+reason for that state, or code 90001 when there is none. `subscribe()`, which attaches, raises
+in the same cases. Code that closes the client, or detaches, while an attach may be pending
+should expect the exception:
+
+Example 4.0.0 code:
+```python
+try:
+    await channel.attach()
+except AblyException:
+    # the channel did not attach, for instance because the client was closed meanwhile
+    ...
+```
+
+### `connection.ping()` waits for a disconnected connection
+
+`await connection.ping()` raised an `AblyException` with code 40000 at once when the connection
+was `DISCONNECTED`. In 4.0.0 it waits for the connection to reconnect, as it already did while
+`CONNECTING`, and then times out `realtime_request_timeout` after its heartbeat goes out. It
+still fails if the connection becomes `SUSPENDED`, `CLOSED` or `FAILED` first, which can be up to
+two minutes later. Bound the wait yourself if you need an answer sooner:
+
+Example 4.0.0 code:
+```python
+response_time_ms = await asyncio.wait_for(pubsub_realtime_client.connection.ping(), timeout=5)
+```
+
 ## Version 2.x to 3.0.0
 
 The 3.0.0 version of ably-python introduces several breaking changes to improve the realtime experience and align the API with the Ably specification. These include:

ably/pubsub/http/annotations.py

@@ -116,8 +116,7 @@ def __base_path_for_serial(self, serial):
         Returns:
             str: The API path
         """
-        channel_path = '/channels/{}/'.format(parse.quote_plus(self.__channel.name, safe=':'))
-        return channel_path + 'messages/' + parse.quote_plus(serial, safe=':') + '/annotations'
+        return self.__channel.base_path + 'messages/' + parse.quote(serial, safe=':') + '/annotations'
 
     async def __send_annotation(self, annotation: Annotation, params: dict | None = None):
         """

ably/pubsub/http/channel.py

@@ -9,6 +9,7 @@
 from ably.pubsub.http.annotations import HttpAnnotations
 from ably.pubsub.request.paginatedresult import PaginatedResult, format_params
 from ably.pubsub.types.channeldetails import ChannelDetails
+from ably.pubsub.types.channeloptions import ChannelOptions
 from ably.pubsub.types.message import (
     Message,
     MessageAction,
@@ -33,12 +34,12 @@
 class Channel:
     __annotations: HttpAnnotations
 
-    def __init__(self, ably, name, options):
+    def __init__(self, ably, name, options: Optional[ChannelOptions] = None):
         self.__ably = ably
         self.__name = name
-        self.__base_path = '/channels/{}/'.format(parse.quote_plus(name, safe=':'))
-        self.__cipher = None
-        self.options = options
+        # RSL2: quote_plus is form encoding, and its `+` for a space is a literal plus in a path
+        self.__base_path = '/channels/{}/'.format(parse.quote(name, safe=':'))
+        self._set_options(options or ChannelOptions())
         self.__presence = Presence(self)
         self.__annotations = HttpAnnotations(self)
 
@@ -48,7 +49,7 @@ async def history(self, direction=None, limit: int = None, start=None, end=None)
         params = format_params({}, direction=direction, start=start, end=end, limit=limit)
         path = self.__base_path + 'messages' + params
 
-        message_handler = make_message_response_handler(self.__cipher)
+        message_handler = make_message_response_handler(self.cipher)
         return await PaginatedResult.paginated_query(
             self.ably.http, url=path, response_processor=message_handler)
 
@@ -73,7 +74,7 @@ def __publish_request_body(self, messages):
                     400, 40012)
 
             if self.cipher:
-                m.encrypt(self.__cipher)
+                m.encrypt(self.cipher)
 
             request_body_list.append(m)
 
@@ -156,7 +157,8 @@ async def publish(self, *args, **kwargs):
     async def status(self):
         """Retrieves current channel active status with no. of publishers, subscribers, presence_members etc"""
 
-        path = f'/channels/{self.name}'
+        # RSL8
+        path = '/channels/{}'.format(parse.quote(self.name, safe=':'))
         response = await self.ably.http.get(path)
         obj = response.to_native()
         return ChannelDetails.from_dict(obj)
@@ -199,7 +201,7 @@ async def _send_update(
 
         # Encrypt if needed
         if self.cipher:
-            update_message.encrypt(self.__cipher)
+            update_message.encrypt(self.cipher)
 
         # Serialize the message
         request_body = update_message.as_dict(binary=self.ably.options.use_binary_protocol)
@@ -210,7 +212,7 @@ async def _send_update(
             request_body = msgpack.packb(request_body, use_bin_type=True)
 
         # Build path with params
-        path = self.__base_path + 'messages/{}'.format(parse.quote_plus(message.serial, safe=':'))
+        path = self.__base_path + 'messages/{}'.format(parse.quote(message.serial, safe=':'))
         if params:
             params = {k: str(v).lower() if isinstance(v, bool) else v for k, v in params.items()}
             path += '?' + parse.urlencode(params)
@@ -292,13 +294,13 @@ async def get_message(self, serial_or_message):
             )
 
         # Build the path
-        path = self.__base_path + 'messages/' + parse.quote_plus(serial, safe=':')
+        path = self.__base_path + 'messages/' + parse.quote(serial, safe=':')
 
         # Make the request
         response = await self.ably.http.get(path)
 
         # Create Message from the response
-        message_handler = make_single_message_response_handler(self.__cipher)
+        message_handler = make_single_message_response_handler(self.cipher)
         return message_handler(response)
 
     async def get_message_versions(self, serial_or_message, params=None):
@@ -332,10 +334,10 @@ async def get_message_versions(self, serial_or_message, params=None):
 
         # Build the path
         params_str = format_params({}, **params) if params else ''
-        path = self.__base_path + 'messages/' + parse.quote_plus(serial, safe=':') + '/versions' + params_str
+        path = self.__base_path + 'messages/' + parse.quote(serial, safe=':') + '/versions' + params_str
 
         # Create message handler for decoding
-        message_handler = make_message_response_handler(self.__cipher)
+        message_handler = make_message_response_handler(self.cipher)
 
         # Return paginated result
         return await PaginatedResult.paginated_query(
@@ -358,10 +360,13 @@ def base_path(self):
 
     @property
     def cipher(self):
+        """The cipher built from `options.cipher`, or None when the channel is not encrypted"""
         return self.__cipher
 
+    # TB2
     @property
-    def options(self):
+    def options(self) -> ChannelOptions:
+        """The channel's options, as last set"""
         return self.__options
 
     @property
@@ -372,41 +377,59 @@ def presence(self):
     def annotations(self) -> HttpAnnotations:
         return self.__annotations
 
-    @options.setter
-    def options(self, options):
-        self.__options = options
+    # RSL7
+    async def set_options(self, options: ChannelOptions) -> None:
+        """Replaces the channel's options.
+
+        The options are replaced whole rather than merged, so options without a cipher turn
+        encryption off.
+        """
+        self._set_options(options)
 
-        if options and 'cipher' in options:
-            cipher = options.get('cipher')
-            if cipher is not None:
-                cipher = get_cipher(cipher)
-            self.__cipher = cipher
+    def _set_options(self, options: ChannelOptions) -> None:
+        self.__options = options
+        # The cipher carries the CBC chaining state from one message to the next, so it is built
+        # once per set of options rather than on every read
+        self.__cipher = get_cipher(options.cipher) if options.cipher is not None else None
 
 
 class Channels:
     def __init__(self, rest):
         self.__ably = rest
         self.__all: dict = OrderedDict()
 
-    def get(self, name, **kwargs):
+    @staticmethod
+    def _channel_options(options, kwargs) -> Optional[ChannelOptions]:
+        """What `get` was given - a ChannelOptions, a dict of its fields or keyword arguments - as a
+        ChannelOptions, or None if it was given no options"""
+        if options is None and kwargs:
+            return ChannelOptions(**kwargs)
+        if isinstance(options, dict):
+            return ChannelOptions.from_dict(options)
+        return options
+
+    # RSN3
+    def get(self, name, options: Optional[ChannelOptions] = None, **kwargs):
+        """Returns the channel called `name`, creating it if need be.
+
+        Options given for an existing channel replace its options (RSN3c).
+        """
         if isinstance(name, bytes):
             name = name.decode('ascii')
+        options = self._channel_options(options, kwargs)
 
         if name not in self.__all:
-            result = self.__all[name] = Channel(self.__ably, name, kwargs)
+            result = self.__all[name] = Channel(self.__ably, name, options)
         else:
             result = self.__all[name]
-            if len(kwargs) != 0:
-                result.options = kwargs
+            if options is not None:
+                result._set_options(options)
 
         return result
 
     def __getitem__(self, key):
         return self.get(key)
 
-    def __getattr__(self, name):
-        return self.get(name)
-
     def __contains__(self, item):
         if isinstance(item, Channel):
             name = item.name

ably/pubsub/liveobjects/realtimeobject.py

@@ -570,7 +570,7 @@ def _throw_if_missing_channel_mode(self, mode: ChannelMode) -> None:
 
         modes = self._channel.modes if self._channel.state == ChannelState.ATTACHED else None  # RTO2a
         if not modes:
-            modes = (self._channel.options or {}).get('modes') or []  # RTO2b
+            modes = self._channel.options.modes or []  # RTO2b
         if mode not in modes:
             # RTO2a2, RTO2b2
             raise AblyException(f'"{mode.name}" channel mode must be set for this operation', 400, 40024)