Repository navigation
pubsub: test side agents, document server token auth, drop deprecated leftovers - #722
Conversation
Message.ConnectionKey was marked deprecated, but spec TM2h keeps it: it is what PublishWithConnectionKey sends when one client publishes on behalf of another, and ably-java keeps the field undeprecated. Replace the marker with the TM2h description. clientOptions.TimeoutConnect and timeoutConnect() were deprecated in favour of RealtimeRequestTimeout. No ClientOption sets the field and nothing calls the method, so delete both.
The entry points' side agent was only tested on the HTTP client. Add tests that the realtime client sends ably-pubsub-server in the Ably-Agent header of its WebSocket handshake, that a user-supplied ably-pubsub-server entry cannot replace the bare flag, and that a device client merges user agents after its side flag. WithAgents now merges instead of replacing, with no unit tests. Cover a nil start, accumulation across calls, same-key overrides, and that neither the options nor the caller's map aliases the other. Also pin the header format: products in alphabetical order, and an empty version rendered as a bare name. Note on both agentName constants that the -server and -device suffixes decide how Ably classifies a client, as ably-java's Side.java does.
…rences Clients from server/pubsub declare themselves as servers, and Ably rejects such a client with error 40167 when it authenticates with a token that lacks the signed x-ably-clientType=server claim (today only a JWT can carry it). The package doc said server clients "typically" use an API key. Say what is required instead, in the package doc, the README and UPDATING.md table rows, and a new UPDATING.md section. The README and UPDATING.md also said only the import path differs between the two entry points. The constructors differ and the device package has no HTTP-only API, so say that most call sites read the same instead.
|
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 47 minutes. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe changes clarify server and device package constructors, server token authentication requirements, and the client-side identifiers in agent headers. Tests cover agent identifier formatting and HTTP and realtime headers. The deprecated connection timeout option is removed, and the ChangesClient identity and API guidance
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| 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. Analyzed 11 functions across 6 files. (2 skipped: 2… | 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 accurately summarizes the main changes: adding side-agent tests, documenting server token authentication, and removing deprecated leftovers. It is concise and specific. 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. Analyzed 11 functions across 6 files. (2 skipped: 2 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.
A rabbit checks the agent string,
Then tests the headers as they spring.
The server claim is noted clear,
The device suffix stays in gear.
Old timeout notes hop out of sight,
And ConnectionKey is set right.
Comment @coderabbitai help to get the list of available commands.
The comments on agentName and the token-auth package doc used jargon that made them hard to follow. Say plainly what the agent name does, why its "-server" or "-device" ending must be kept, and what token authentication needs. Shorten the new test comments the same way. Comments only; no code changes.
Summary
Follow-up to the review of #712. This PR resolves the open review points that don't depend on a pending decision:
Deprecated:marker onMessage.ConnectionKey, and the deadTimeoutConnectoptionWithAgentsmerging and the header formatWithAgents, agent testsUPDATING.mdnoteBase:
integration/v2. 4 commits, 9 files. The only production-code change is deleting dead code; everything else is tests and docs. Each choice follows ably-java'sintegration/v2, cited below.Changes
1.
ably: drop the stale deprecation leftoversMessage.ConnectionKey: replace// Deprecated: … will be removedwith the TM2h descriptionPublishWithConnectionKeydepends on itMessage.java:43-48, "Spec: TM2h")clientOptions.TimeoutConnectandtimeoutConnect()RealtimeRequestTimeout. NoWith…option sets the field and nothing calls the method, so it was never public API.There's no
UPDATING.mdentry, since nothing public was removed.2.
pubsub: test the side agents and WithAgents mergingTestNewRealtimeClient_DeclaresServerAgentserver/pubsub/server_test.goAbly-Agentheader of the WebSocket handshake carriesably-pubsub-serverTestNewHTTPClient_UserCannotOverrideSideAgentserver/pubsub/server_test.goWithAgents({"ably-pubsub-server": "9.9"})still leaves the bare flagTestNewClient_MergesUserAgentsdevice/pubsub/device_test.goably-pubsub-device foo/1.2.3TestWithAgents_FromNil,…_AccumulatesAcrossCalls,…_SameKeyOverrides,…_DoesNotAliasCallersMapinternal/ably/options_test.goWithAgentsTestAblyAgentIdentifier_SortedAndBareFlaginternal/ably/options_test.goAlso, a short comment on both
agentNameconstants explaining that Ably recognises the client's side by the-server/-deviceending, so the ending must be kept if the constant is renamed. The same point is made in ably-java'sSide.java.These mirror ably-java's
PubSubServerTest/PubSubDeviceTest. TheWithAgentsunit tests are extra Go-side coverage.3.
docs: document token auth on server clients and the entry-point differencesToken auth (40167): server clients declare themselves as servers, and Ably rejects one that authenticates with a token lacking the signed
x-ably-clientType=serverclaim, with error 40167 ("Client type declaration not signed by token", registered in ably/ably-common#355). Today only a JWT can carry that claim.server/pubsubpackage doc (previously "typically authenticate with an API key"), the server rows of the README andUPDATING.mdtables, and a new "Server clients and token authentication" section inUPDATING.md.Entry-point differences: the README and
UPDATING.mdsaid only the import path differs. Now they say most call sites read the same, but the constructors differ (NewHTTPClient/NewRealtimeClientvsNewClient) and the device package has no HTTP-only API.4.
pubsub: reword the agentName and token-auth commentsComments only, no code changes: the
agentName, token-auth package doc and new test comments are reworded in plain language after review.Validation
All run locally before pushing:
go build ./...;go vetwith default,unitandintegrationtags;gofmt -lgo test -tags=unit ./...-race• remove the sort → sorted/merge tests fail
•
WithAgentsassigns without merging →WithAgentsand merge tests fail• rename
agentName→ all 4 server agent tests failTimeoutConnectreferences;ConnectionKeystill used byPublishWithConnectionKeygo generate ./...drift check,go build ./examples/...TestHTTP_RSC7_AblyAgent,TestRealtime_RSC7_AblyAgent,TestRSC7,TestHTTPChannel*,PublishWithConnectionKeyNot in this PR
These are waiting on decisions in #712:
/v2);clientLibraryVersionbump and a literal version canary, which follow the versioning decision;Also deferred: the errors generator, which can't parse the current
errors.jsonformat and would change user-visible error strings if fixed. That belongs in a separate PR.Summary by CodeRabbit