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

Rename Address to ConnectionId #2220

Merged
merged 22 commits into from
Feb 10, 2025
Merged

Conversation

gefjon
Copy link
Contributor

@gefjon gefjon commented Feb 6, 2025

Description of Changes

This PR renames Address, which was used only for identifying client connections at this point, to ConnectionId. Semantic changes are kept to a minimum.

Notable features of this PR:

  • My workflow significantly involved grepping for "address" and replacing instances with correct new terms. Some of the uses were stale, as they referred to "database address," which has since been replaced by the concept of a "database identity." I chose to update these uses, both because they were incorrect and because leaving them in place would have negatively impacted my grep-based workflow. This adds some lines to the diff which are not strictly necessary.
  • I changed Address::from_byte_array and Address::to_byte_array to ConnectionId::from_le_byte_array and ConnectionId::to_le_byte_array. My theory is that the name is changing anyways, so I might as well make the endianness explicit. This is a direct response to the comments all over address.rs which talk about correct endianness, often IN ALL CAPS.
  • I retained the interface for setting the connection ID via a query parameter to the /database/subscribe HTTP endpoint, which is the one that establishes a WS connection. However, I removed it from /database/call.
    • I added a deprecation log, currently at debug level, when that query parameter is supplied.
    • Cloud still depends on being able to set its connection ID.
    • I did not any of the client SDKs' handling of connection IDs, so they will still generate a random connection ID and pass it to that query parameter. This means that the host will always log a note when any client establishes a connection.
      • I would like to make this change to the SDKs in a follow-up, post-1.0.

Companion PRs

API and ABI breaking changes

Yep! A significant, though superficial, API break.

Expected complexity level and risk

5: the word "address" was in just about all of our APIs, both internal and external, and they're all changing. Every single component we have needs to be updated in lockstep to account for this.

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!

  • Really just automated tests.
  • Someone should run Blackholio and make sure it works.
    • I cannot do this because I don't have a Unity license.
  • Someone should run the TypeScript SDK quickstart.
    • I am unsure how to do this, though I could potentially puzzle it out given time.

also:

- removed `impl default for connectionid`, i want to be explicit about using zero.
- renamed or re-documented many uses of the word "address" which were actually referring
  to the database identity, despite the concept of the database identity not having
  changed in this PR.
- removed client address query param from call HTTP API
- added warn log to connection id param in subscribe HTTP API so users don't do it
- removed `client_address` from `MetadataFile`. It seemed to be unused, and I have no clue
  what it would possibly be for.
- rust SDK is still supplying connection id to subscribe. this needs to change, but
  inconvenient.
- C# `CallerIdentity` renamed to `Sender`, per tyler's request

Expected to be in a very broken state right now, but I'm stepping away for a moment and
wanted to save what I had so far.
Incl. two files that were not reachable and seemed severely bit-rotted:
- `tracelog.rs`
- `reducer.rs`

The latter I myself forgot to delete in a past PR;
the former I do not know about, but it's not part of our project,
it wouldn't compile, and I don't see any reason to keep it in our repo.
@gefjon gefjon changed the title Phoebe/rename address to connection Rename Address to ConnectionId Feb 7, 2025
@gefjon gefjon added the api-break A PR that makes an API breaking change label Feb 7, 2025
@gefjon gefjon marked this pull request as ready for review February 7, 2025 17:02
Copy link
Collaborator

@bfops bfops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My codeowned files LGTM 👍

crates/cli/src/subcommands/call.rs
crates/cli/src/subcommands/subscribe.rs

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving websocket.rs changes

@gefjon gefjon requested a review from jsdt February 7, 2025 19:18
Copy link
Contributor

@jsdt jsdt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked through all but the generated files, and all of the changes seemed right to me.

Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the files I'm a code owner of. Left just one suggested change.

gefjon added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Feb 10, 2025
## Description of Changes

Companion to [Rename `Address` to
`ConnectionId`](clockworklabs/SpacetimeDB#2220).
See that PR's description for more.

Like all the SDKs, this PR does not change the SDK's behavior; the SDK
still generates a connection ID locally and passes it through the HTTP
API. This is not exposed to users, and can be changed in a follow-up.

Also, use `/usr/bin/env bash` instead of `/bin/bash` in tools, for
portability reasons.

## API

 - [x] This is an API breaking change to the SDK

`Address` is renamed to `ConnectionId`.

## Requires SpacetimeDB PRs

- clockworklabs/SpacetimeDB#2220
- 
## Testsuite
*If you would like to run the your SDK changes in this PR against a
specific SpacetimeDB branch, specify that here. This can be a branch
name or a link to a PR.*

SpacetimeDB branch name: phoebe/rename-address-to-connection-id

## Testing

- [x] Pretty much just automated testing.
- [x] @kazimuth will need to update and run Blackholio.

---------

Co-authored-by: James Gilles <[email protected]>
@gefjon gefjon added this pull request to the merge queue Feb 10, 2025
Merged via the queue into master with commit aedc601 Feb 10, 2025
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-break A PR that makes an API breaking change release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants