Repository navigation
Conversation
RealtimeChannel took its logger from ChannelOptions.Logger only, which is null for Channels.Get(name) and is set to the static default logger by every public ChannelOptions constructor. Channels, their Presence and Annotations therefore never used a client's own logger, so the RTAN4e "AnnotationSubscribe not granted" warning missed a per-client log sink. ChannelOptions.Logger now stays null unless a logger is explicitly supplied (internal constructor), and the channel falls back to the client's logger. An explicitly supplied logger still wins. The message encoders keep falling back to the default logger when it is null. Un-gate the UTS test RTAN4e_SubscribeWarnsNoMode and drop its deviations.md entry; add native ChannelsSpecs tests for both Get forms.
PublishAsync caught the AblyException that EnsureCanPublish throws for a channel in SUSPENDED/FAILED or a connection that can't queue (RTL6c4) and wrapped it as a generic 50000 "Unexpected error", keeping the 40000 only as the InnerException. The edit methods and annotation publish already surfaced the 40000 unwrapped. The task wrappers now pass an AblyException through as is and wrap only non-Ably exceptions as 50000. The connection refusal message now names the connection state instead of the channel state. Correct the IRealtimeChannel PublishAsync and UpdateMessageAsync XML docs, which said every failure is a failed Result: a refusal faults the task. Update the uts-to-csharp realtime mapping, CHANGELOG and UPDATING, and add native PublishResultSpecs tests for the refusal codes and for the unchanged wrapping of unexpected exceptions.
RSAN3a/RSAN3b (IDL params?: Dict<String, Stringifiable>, RTAN3a for
realtime) let annotations.get send any query parameter, and ably-java
takes Param[]. AnnotationsRequestParams exposed only Limit.
Add AnnotationsRequestParams.ExtraParameters, mirroring
PaginatedRequestParams.ExtraParameters. Every entry goes to the
querystring; the typed Limit, when set, wins over a "limit" entry, and
the default limit of 100 is still sent when neither is set. An
out-of-range Limit now throws AblyException 40003 (as the other
annotation argument checks do) instead of ArgumentException.
The RSAN3b UTS test now passes the spec's params { "limit": "50" }
straight, so its adaptation and deviations.md entry are removed and
the mapping references updated.
The IDL lists fallbackHosts as a ClientOptions attribute (TO3k6, "String[] default nil"), and ably-java and ably-js expose it as a readable field. FallbackHosts was set-only, and since the Endpoint change made GetFallbackHosts() internal there was no public way to read it. FallbackHosts is now a get/set auto-property like every other ClientOptions member. It returns the hosts that were set, or null when none were; it does not return the default fallback hosts derived from Endpoint, which the internal GetFallbackHosts() still resolves. Clone() copies it through the property, so the write-only special case in the clone rot-guard is gone. The TO3k6 UTS test now reads options.FallbackHosts as the spec does, so its adaptation and deviations.md entry are removed and the mapping references updated.
UPDATING now says the unwrapped AblyException also applies to attach, SetOptions and presence calls (for example the "Not queuing messages" error when QueueMessages is false). The CHANGELOG moves the annotation Limit validation change under Behaviour changes and drops the channel logger bullet, which has no user-visible effect. The realtime Annotations.GetAsync query docs and the rest-mapping annotations row now match the REST behaviour.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes expose configured fallback hosts, support extra annotation query parameters, preserve realtime ChangesFallback host configuration
Annotation query parameters
Realtime exception handling
Channel logger fallback
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 34.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 18 files. (4 skipped:… | 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 summarizes the main changes: refused-publish error handling, annotation query parameters, and restoration of the FallbackHostsgetter.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 34.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 18 files. (4 skipped: 4 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 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.
I’m a rabbit, hopping past the host list,
I find extra query keys tucked in the request.
The limit stays clear, from zero to one thousand,
Refusal errors keep the details they carried.
The client’s logger joins each quiet channel,
And I thump my paws at tests that pass.
Comment @coderabbitai help to get the list of available commands.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes four issues found while reviewing the public API added by the protocol v6 and Endpoint PRs (#1346, #1347, #1348, #1349, #1359) against the features spec and ably-java:
PublishAsyncnow reports the real error (40000) instead of a generic 50000AnnotationsRequestParamscan send any query parameter, not justlimitClientOptions.FallbackHostscan be read againEach fix comes with native tests. Where a UTS test had been gated or adapted around the problem, it now runs as the spec writes it.
Changes
1. A refused realtime publish surfaces its own error
Problem. When
PublishAsyncwas refused because of the channel or connection state, the task faulted with a generic50000 "Unexpected error". The real refusal (40000, status 400) was only reachable throughInnerException.UpdateMessageAsync,DeleteMessageAsync,AppendMessageAsyncandAnnotations.PublishAsyncalready surface the original error, so the same channel reported different errors depending on the method.A refusal happens when the channel is SUSPENDED or FAILED, or the connection is neither CONNECTED nor allowed to queue (RTL6c4).
Fix. The realtime task wrappers (
TaskWrapper,TaskWrapper<T>,PublishTaskWrapper) now pass anAblyExceptionthrough unchanged. Any other exception is still wrapped as 50000, as before. The "can't queue" refusal message now names the connection state instead of the channel state.Also affected. The same wrappers back
AttachAsync,DetachAsync,SetOptionsAsync, the presence methods andPingAsync. Those calls now fault with the underlyingAblyExceptiontoo. For example, withQueueMessages = falseand the connection not CONNECTED,AttachAsyncfaults with "Not queuing messages …" (code 50000, status 400) instead of a wrapping "Unexpected error".Docs. The XML docs on the three
IRealtimeChannel.PublishAsyncoverloads and onUpdateMessageAsyncnow describe this correctly. CHANGELOG and UPDATING list it as a behaviour change.2.
AnnotationsRequestParamsaccepts any query parameterProblem.
Annotations.GetAsynccould only sendlimit. The spec (RSAN3a/b; the IDL types the parameter asparams?: Dict<String, Stringifiable>) and ably-java (Param[]) allow any parameter.Fix.
AnnotationsRequestParams.ExtraParameters(Dictionary<string, string>), the same shape asPaginatedRequestParams.ExtraParameters. Every entry goes into the querystring.limitis sent exactly once, in this order of precedence:Limit, if set;ExtraParameters["limit"]entry;Annotations.GetAsyncdelegates to REST.Why
ExtraParametersand notParams. In the IDL,paramsnames the whole argument ofget(messageSerial, params?: Dict<String, Stringifiable>), not a field inside it. ably-java likewise takes the whole argument asget(String messageSerial, Param[] params). In .NET that argument is theAnnotationsRequestParams queryobject; it is namedquerybecauseparamsis a C# keyword, as in the existingHistoryAsyncandGetMessageVersionsAsyncmethods. The closest precedent isgetMessageVersions(serial, params?: Dict<String, Stringifiable>). Its IDL argument has exactly the same shape, and .NET already maps it toGetMessageVersionsAsync(serial, PaginatedRequestParams query): typed members for the common parameters, plusPaginatedRequestParams.ExtraParametersfor any other entry.AnnotationsRequestParamsfollows the same pattern: the typedLimit, plusExtraParameters. (Channel and presencehistoryare different: their IDL lists only typed fields, andExtraParametersis a .NET extension there.)Behaviour change. A
Limitoutside 0–1000 now throws anAblyException(code 40003, status 400), like the other argument checks inAnnotations. It previously threw anArgumentException. Annotations are a new 2.0 API.3.
ClientOptions.FallbackHostsis readable againProblem.
FallbackHostsis set-only, so applications can't read it.ClientOptions.GetFallbackHosts()gave read access; Replace Environment, RestHost and RealtimeHost with Endpoint #1359 made itinternal.fallbackHostsas aClientOptionsattribute (TO3k6), and ably-java (a getter) and ably-js (fallbackHosts?: string[]) both expose it.Fix.
FallbackHostsis now a normal get/set property, like every otherClientOptionsmember.nullwhen unset. It never returns the defaults derived fromEndpoint, which matches Java and JS.GetFallbackHosts()still resolves the list actually used for fallback (REC2), unchanged.Clone()now copies the property directly.Migration note (in UPDATING). The old public
GetFallbackHosts()returned the effective list, including defaults. The new getter returns only what was set.4. Realtime channels log through their client's logger
Problem. Channels created with
Channels.Get(name)used the process-wide default logger instead of their client's logger.ClientOptions.Loggeris internal, and every client configures that same shared default logger throughLogHandlerandLogLevel.Fix.
RealtimeChannelnow usesoptions?.Logger ?? realtimeClient.Logger, so a logger passed in on purpose still wins.ChannelOptionsconstructors no longer capture the static logger.User impact. None. This isn't in the CHANGELOG.
Tests
Native tests (new)
Realtime/PublishResultSpecs.csHttp/ChannelAnnotationsSpecs.cslimitprecedence; nullExtraParameters; an out-of-rangeLimitgives 40003; the 0 and 1000 bounds.Realtime/AnnotationsSpecs.csClientOptionsTests.csFallbackHosts: unset readsnull; set, empty and reset values read back; cloning.Realtime/ChannelsSpecs.csChannelOptionsconstructor, and its Presence use the client's logger; an explicitly passed logger wins; the RTAN4e warning reaches a client-specific sink.With each fix reverted, its new tests fail. The exceptions are the two guard tests, which are meant to pass either way.
UTS tests
These tests now run as the spec writes them, and their
deviations.mdentries are removed:RealtimeTests.Unit.ChannelAnnotationsTests.RTAN4e_SubscribeWarnsNoMode[DeviationFact]RestTests.Unit.AnnotationsTests.NoId_Rsan3bGetPassesParamsAsQuerystring{ Limit = 50 }{ "limit": "50" }RestTests.Unit.OptionsTypesTests.TO3_ClientOptionsCustomHostsGetFallbackHosts()FallbackHostsThe
uts-to-csharpskill's mapping notes (rest-mapping.md,realtime-mapping.md) are updated to match.Validation
An independent reviewer checked this PR before it was opened. It confirmed that each fix addresses the root cause, the new tests fail without the fixes, the UTS edits are faithful to the spec, and the docs are accurate. Results (net6.0, Release):
./build.sh --target=Build.NetStandard(StyleCop as errors)Test.NetStandard.Unit)RUN_DEVIATIONS=1[DeviationFact]test; the un-gated tests passRTN15j_FatalErrorEstablishedConn, failed once on a race and passed on re-run)Not run locally: the .NET Framework legs (they need Windows or Mono) and net7.0–net9.0. CI covers these.
Size
23 files, +417 / −112:
src/Ably.PubSub.Shared,Side)deviations.mdCommits
Reviewer notes
ArgumentException→AblyException40003.ArgumentException).Summary by CodeRabbit
New Features
limithandling and validation.ClientOptions.FallbackHostscan now be read, with clear behavior for configured, empty, and unset values.Bug Fixes
Documentation