Repository navigation
Keep realtime retrieval from changing the options of HTTP channels - #1376
Conversation
Realtime history, presence history, GetMessageAsync, GetMessageVersionsAsync and Annotations.GetAsync go over REST with the realtime channel's options. They got the REST channel from HttpClient.Channels with those options, which replaces the options of the app's channel of the same name (RSN3c). A realtime channel without a cipher therefore removed the cipher the app had set on that HTTP channel, so its later REST publishes went out unencrypted, and a realtime channel with a cipher encrypted the publishes of a plain one. Build a new HttpChannel from the realtime channel's current options for each retrieval instead, as ably-java does without touching its REST channels. Next pages keep the options of the call that started them. Add tests for both directions over all the retrievals, document that untilAttach replaces a caller-supplied fromSerial, and add the CHANGELOG entries for the earlier history fixes. This bug never shipped, so it gets no entry of its own.
Auth.CreateTokenRequest[Async] returns the signed token request as a JSON string by design, so the docs now say so and explain how to use it. UPDATING.md's row for the removed CreateTokenRequestObject[Async] says to return the JSON unchanged from an AuthUrl endpoint (as application/json) or an AuthCallback, and to use JsonHelper to turn it into a TokenRequest and back, since other JSON serializers don't produce Ably's token request format. The CHANGELOG bullet names the replacement, the README shows the JsonHelper call, and the IAblyAuth XML docs say the result is JSON. Also correct the 1.2.15 release date in the CHANGELOG to 2024-04-19, the date of its tag.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit details
 |
| Layer / File(s) | Summary |
|---|---|
Token-request JSON contract and examples src/Ably.PubSub.Shared/IAblyAuth.cs, CHANGELOG.md, UPDATING.md, README.md |
API comments, migration guidance, and examples specify that token requests are JSON strings and describe how callers can deserialize and return them. The changelog also clarifies AuthCallback JSON interpretation and corrects the 1.2.15 release date. |
Realtime REST-channel retrieval
| Layer / File(s) | Summary |
|---|---|
REST-channel ownership and option updates src/Ably.PubSub.Shared/Realtime/IRealtimeChannel.cs, src/Ably.PubSub.Shared/Realtime/RealtimeChannel.cs, src/Ably.PubSub.Tests.Shared/Realtime/ChannelSpecs.cs, CHANGELOG.md |
RealtimeChannel caches and reuses its REST channel while the options object remains unchanged, and replaces it when the options change. Tests cover retrieval reuse, option changes, and preservation of a same-name application HttpChannel. Documentation clarifies the untilAttach query behavior and realtime history options. |
Estimated code review effort: 2 (Simple) | ~12 minutes
Change: Bug fix
Suggested reviewers: umair-ably
Merge Risk: 
25994
-
The release note misstates how callback JSON containing an issued field is handled. Correcting it is a bounded documentation follow-up.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preventing realtime retrieval from altering HTTP channel options. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 3 functions across 4 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the token string,
Then hops where realtime channels sing.
The REST path keeps its options near,
New settings bring a channel clear.
Soft paws approve the tested flow.
|
@coderabbitai review |
Action not completed
|
RestChannel built a new HttpChannel on every retrieval. Keep one per realtime channel instead, rebuilt when the channel's options change. Every change replaces the ChannelOptions object (the Options setter, SetOptions, and Channels.Get with options), and HttpChannel reads its options on each request, so comparing the cached channel's options reference with the current one never leaves stale options in use. A concurrent rebuild only creates a spare instance with the same options. The cached channel is still separate from HttpClient.Channels. Add tests that the instance is reused while the options are unchanged, and that retrieval decodes with the new cipher after SetOptions or Channels.Get with options.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
A JSON object returned from AuthCallback is also treated as a serialized TokenDetails when it has an issued field (TokenDetails.IsToken), not only when it has a string token field.
Summary
On a realtime client, five retrieval calls went over REST through
HttpClient.Channels.Get(name, realtimeOptions):GetMessageAsync;GetMessageVersionsAsync;Annotations.GetAsync.That call replaces the options of the app's own channel of the same name (RSN3c). Two things went wrong as a result:
client.HttpClient.Channels.Get("x", cipherOptions)and its realtime channel"x"had none, any of those calls dropped the cipher. The app's next REST publish on that channel then went out unencrypted, without any error.This is new in 2.0. 1.x used
Get(name)without options, and the message, version and annotation retrieval APIs didn't exist. CodeRabbit raised it on #1371. History joined this path in that PR.Fix
RealtimeChannel.RestChannelnow returns the realtime channel's ownHttpChannel, built from its current options. It never reads or writesHttpClient.Channels. All five paths go through it.The instance is cached and rebuilt when the channel's options change. Every change (
SetOptions,Channels.Getwith options) replaces theChannelOptionsobject, andHttpChannelreads its options on each request, so comparing references is enough to never use stale options. A concurrent rebuild only creates a spare instance with the same options. Next pages keep the options of the call that started them.Tests
ChannelSpecs+RetrievalHttpChannelOptionsSpecshas 2 theories, each run over 6 retrievals: history, history withuntilAttach, presence history,GetMessage,GetMessageVersionsand annotations get.PublishAsyncis still encrypted (cipher+aes-256-cbc).Two more tests cover the cache:
SetOptionsorChannels.Getwith a new cipher, history, presence history andGetMessagedecode with the new cipher.With a cache that never rebuilds, the second test fails.
On
integration/v2without the fix, the 12HttpClient-channel cases fail. With it, they pass. The existingHistoryAsync_OnACipherChannel_ShouldDecryptTheRetrievedMessagesstill passes.CHANGELOG / docs
2.0 "Fixed bugs" in
CHANGELOG.md:HttpClientchannel of the same name;HistoryAsync(query, untilAttach: true)no longer modifies the caller'sPaginatedRequestParams, and it replaces anyfromSerialthe caller set;tokenfield".There is no separate entry for the options replacement itself, because it never shipped.
The
untilAttachXML doc onIRealtimeChannel.HistoryAsync(PaginatedRequestParams, bool)now says the attach serial replaces a caller-suppliedfromSerial.Token request docs (#23)
This is a separate docs-only commit:
UPDATING.md: theCreateTokenRequestObject[Async]row says thatCreateTokenRequest[Async]returns the signed token request as a JSON string. Return it unchanged from anAuthUrlendpoint (asapplication/json) or from anAuthCallback, and useJsonHelper.Deserialize<TokenRequest>/JsonHelper.Serializefor aTokenRequestobject.CHANGELOG.md: the[Obsolete]-removed bullet names the replacement, and the 1.2.15 date is corrected to 2024-04-19, its tag date.README.md: showsJsonHelper.Deserialize<TokenRequest>(tokenRequest). The snippet compiles and runs.IAblyAuthXML docs: say the result is JSON.The code is unchanged by decision.
CreateTokenRequest[Async]returns JSON on purpose: issues 110 and 1268, where serialising the object with other serializers produced bodies Ably rejects.Validation
./build.sh --target=Build.NetStandard: 0 warnings, 0 errors../build.sh --target=Test.NetStandard.Unit.WithRetry --framework=net6.0: 1507 total, 1488 passed, 19 skipped, 0 failed.246c1d26. It reproduced the bug in both directions for all five paths, failing without the fix and passing with it, and checked that next pages keep the original call's options. It also reviewed both commits and the docs against the code.25994387, the cache. It checked that no options state goes stale, ran a concurrent stress test withSetOptions, and ran the reproductions for the original code, the per-call version, a cache that never rebuilds, and the final code.Spec and ably-java context
Channels#get(name, options)updates an existing channel's options. It is soft-deprecated, and it describes the app's own call. Nothing in the spec asks the library to apply realtime options to the app's REST channels.Summary by CodeRabbit
Bug Fixes
untilAttach: trueuse the attach serial instead of any suppliedfromSerial, without changing the caller’s query parameters.Documentation