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

Rust client SDK: send SubscribeMulti and UnsubscribeMulti #2281

Merged
merged 13 commits into from
Feb 21, 2025

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Feb 19, 2025

Description of Changes

Based on #2278 .

This commit modifies the Rust client SDK to use the new setwise subscription API.

This includes:

  • Changing SubscriptionBuilder::subscribe to accept any implementor of IntoQueries, including single strings and also arrays of strings, for various definitions of "string" and "array." And fiddling a bit with the implementations of IntoQueries.
  • Sending the SubscribeMulti and UnsubscribeMulti websocket messages instead of Subscribe and Unsubscribe.
  • Stubbing the handling of SubscribeApplied and UnsubscribeApplied websocket messages, since those should now be unreachable.
  • Handling the SubscribeMultiApplied and UnsubscribeMultiApplied messages instead.
  • Simplifying the quickstart-chat and test-client clients to just subscribe to one big set, where previously they had a kind of wonky semaphore-based scheme to wait for all queries to be applied.

New syntax

You can now do:

ctx.subscription_builder()
  .subscribe(["SELECT * FROM user", "SELECT * FROM client"]);

The array passed here may be a [T; N] (fixed-size array), a &[T] (slice), Vec<T> (vec) for a few different string-like Ts, or a Box<[Box<str>]>.

You can also still subscribe to a single query without wrapping it in an array, like:

ctx.subscription_builder()
  .subscribe("SELECT * FROM user");

The string here may be &str, String or Box<str>. These are the same "string-like" types which can be put in arrays as above.

API and ABI breaking changes

I have not been super careful about the changed implementations of IntoQueries, so minor incompatibilities are possible between our most recent 1.0-rc release and this patch in terms of valid types to pass to SubscriptionBuilder::subscribe. Common cases, like &str and String, still work.

Expected complexity level and risk

1

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

  • I believe automated testing is sufficient.

jsdt and others added 5 commits February 20, 2025 14:20
This commit modifies the Rust client SDK to use the new setwise subscription API.

This includes:
- Changing `SubscriptionBuilder::subscribe` to accept any implementor of `IntoQueries`,
  including single strings and also arrays of strings,
  for various definitions of "string" and "array."
  And fiddling a bit with the implementations of `IntoQueries`.
- Sending the `SubscribeMulti` and `UnsubscribeMulti` websocket messages
  instead of `Subscribe` and `Unsubscribe`.
- Stubbing the handling of `SubscribeApplied` and `UnsubscribeApplied` websocket messages,
  since those should now be unreachable.
- Handling the `SubscribeMultiApplied` and `UnsubscribeMultiApplied` messages instead.
- Simplifying the `quickstart-chat` and `test-client` clients
  to just subscribe to one big set,
  where previously they had a kind of wonky semaphore-based scheme
  to wait for all queries to be applied.
@gefjon gefjon force-pushed the phoebe/rust-sdk-multi-subscribe branch 2 times, most recently from a4ccf89 to dff56a6 Compare February 21, 2025 19:12
@gefjon gefjon requested a review from Centril as a code owner February 21, 2025 19:12
@gefjon gefjon force-pushed the phoebe/rust-sdk-multi-subscribe branch from dff56a6 to a4ccf89 Compare February 21, 2025 19:14
@gefjon gefjon changed the base branch from jsdt/multi-subscribe to master February 21, 2025 19:15
@gefjon
Copy link
Contributor Author

gefjon commented Feb 21, 2025

Attempting to rebase has proved once more to have been a mistake. Merging seems to have worked fine, though. Sorry for the confusion and force-pushes!

@gefjon gefjon added this pull request to the merge queue Feb 21, 2025
Merged via the queue into master with commit d310583 Feb 21, 2025
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants