Skip to content

[SDK] generate struct Module to hold dispatch functions #239

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Sep 29, 2023
Merged

[SDK] generate struct Module to hold dispatch functions #239

merged 4 commits into from
Sep 29, 2023

Conversation

@gefjon
Copy link
Contributor

@gefjon gefjon commented Aug 25, 2023

Description of Changes

Prior to this commit, the Rust SDK passed autogenerated dispatch functions around as function pointers (i.e. values of fn types). This was brittle and inextensible; adding a new dispatch function or changing the type of a dispatch function required threading a new argument through BackgroundDbConnection::connect, and a new field to whatever struct was supposed to invoke the dispatch function.

This commit adds a new trait SpacetimeModule, which has a method for each autogenerated function. BackgroundDbConnection::connect accepts an Arc<dyn SpacetimeModule>, which it passes to various components which need access to the autogenerated methods. The CLI's generate with --lang rust generates a unit struct Module, which implements the trait. The generated connect function passes Arc::new(Module) to the internal connect method.

This greatly reduces the maintenence burden of adding new dispatch functions. Now, we only need to alter:

  • The definition of trait SpacetimeModule.
  • The generation of the instance.
  • Whatever internals call the method.

API and ABI

  • This is a breaking change to the module ABI
  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI
  • [?] This is a breaking change to the SDK API

If the API is breaking, please state below what will break

Rust client authors will need to re-run spacetime generate, but will not need to edit any of their code.

@gefjon gefjon requested a review from coolreader18 August 25, 2023 15:04
@gefjon
[SDK] generate struct Module to hold dispatch functions
fec3d69 
Prior to this commit, the Rust SDK passed autogenerated dispatch functions around as
function pointers (i.e. values of `fn` types).
This was brittle and inextensible; adding a new dispatch function
or changing the type of a dispatch function
required threading a new argument through `BackgroundDbConnection::connect`,
and a new field to whatever struct was supposed to invoke the dispatch function.

This commit adds a new trait `SpacetimeModule`,
which has a method for each autogenerated function.
`BackgroundDbConnection::connect` accepts an `Arc<dyn SpacetimeModule>`,
which it passes to various components which need access to the autogenerated methods.
The CLI's `generate` with `--lang rust` generates a unit struct `Module`,
which implements the trait.
The generated `connect` function passes `Arc::new(Module)` to the internal connect method.

This greatly reduces the maintenence burden of adding new dispatch functions.
Now, we only need to alter:
- The definition of `trait SpacetimeModule`.
- The generation of the instance.
- Whatever internals call the method.
@gefjon gefjon force-pushed the phoebe/sdk-module-struct branch from 564a17d to fec3d69 Compare August 25, 2023 15:07
coolreader18
coolreader18 approved these changes Aug 29, 2023
View reviewed changes
Copy link
Collaborator

@coolreader18 coolreader18 left a comment

Choose a reason for hiding this comment

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

LGTM! It'd be cool to have the struct called the CamelCase version of the module name instead of just "Module", but I don't think codegen has that info iirc

@gefjon
Copy link
Contributor Author

gefjon commented Aug 29, 2023

LGTM! It'd be cool to have the struct called the CamelCase version of the module name instead of just "Module", but I don't think codegen has that info iirc

Yeah, that was my first thought as well, but it'd require deeper changes to the CLI's generate command than I felt comfortable with right now.

@gefjon gefjon merged commit b51e551 into master Sep 29, 2023
@kazimuth kazimuth mentioned this pull request Mar 26, 2025
Merged
4 tasks
kazimuth added a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Mar 28, 2025
@kazimuth
Fix for unsubscribe bug + regression tests (#264)
5820881 
## Description of Changes

This PR fixes one issue and adds regression tests for it, plus another
issue. These regression tests are run on CI. You can also run them
locally -- run a local spacetime instance and then
`tools~/run-regression-tests.sh ../SpacetimeDB`).

### Issue 1: Unsubscribe is non-functional

See
#279.
This was a coding mistake made during the rush to 1.0. It was easily
fixed.

### Issue 2: Indexes not tracking deletes

This was recently discovered by the BitCraft team.

The problem was introduced in
20c6480
which introduced `RemoteTableHandle<EventContext,
Row>.IndexBase<Column>`. This class stores a `Dictionary<Column,
List<Row>>` to allow fast filtering. It relies on `List<Row>.Remove` to
remove elements from a list when a row delete is received.
`List<Row>.Remove` relies on `Row.Equals` to determine when elements are
equal.

`Row.Equals` formerly was not implemented for `[SpacetimeDB.Type]`s or
for generated Row types. So, this didn't work.

Now it does: it was fixed by
clockworklabs/SpacetimeDB#239 . So all this PR
does to address this issue is add a regression test.

## API

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

The interface IDbConnection was changed; however, this changed part of
the interface is `internal`, so this should be entirely invisible to
users.

## Requires SpacetimeDB PRs
clockworklabs/SpacetimeDB#239 (merged)

## 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: master

## Testing
*Write instructions for a test that you performed for this PR*

- [x] Added a repro for both issues
- [x] Added the repro to CI
- [x] Fixed both issues
jdetter pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Apr 2, 2025
@kazimuth @jdetter
Fix for unsubscribe bug + regression tests (#264)
4ec89af 
## Description of Changes

This PR fixes one issue and adds regression tests for it, plus another
issue. These regression tests are run on CI. You can also run them
locally -- run a local spacetime instance and then
`tools~/run-regression-tests.sh ../SpacetimeDB`).

### Issue 1: Unsubscribe is non-functional

See
#279.
This was a coding mistake made during the rush to 1.0. It was easily
fixed.

### Issue 2: Indexes not tracking deletes

This was recently discovered by the BitCraft team.

The problem was introduced in
20c6480
which introduced `RemoteTableHandle<EventContext,
Row>.IndexBase<Column>`. This class stores a `Dictionary<Column,
List<Row>>` to allow fast filtering. It relies on `List<Row>.Remove` to
remove elements from a list when a row delete is received.
`List<Row>.Remove` relies on `Row.Equals` to determine when elements are
equal.

`Row.Equals` formerly was not implemented for `[SpacetimeDB.Type]`s or
for generated Row types. So, this didn't work.

Now it does: it was fixed by
clockworklabs/SpacetimeDB#239 . So all this PR
does to address this issue is add a regression test.

## API

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

The interface IDbConnection was changed; however, this changed part of
the interface is `internal`, so this should be entirely invisible to
users.

## Requires SpacetimeDB PRs
clockworklabs/SpacetimeDB#239 (merged)

## 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: master

## Testing
*Write instructions for a test that you performed for this PR*

- [x] Added a repro for both issues
- [x] Added the repro to CI
- [x] Fixed both issues
bfops pushed a commit that referenced this pull request Jul 17, 2025
@gefjon @kazimuth
Rename Address to ConnectionId (#239)
96d52c1 
## Description of Changes

Companion to [Rename `Address` to
`ConnectionId`](#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

- #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]>
bfops pushed a commit that referenced this pull request Jul 17, 2025
@kazimuth
Fix for unsubscribe bug + regression tests (#264)
3543132 
## Description of Changes

This PR fixes one issue and adds regression tests for it, plus another
issue. These regression tests are run on CI. You can also run them
locally -- run a local spacetime instance and then
`tools~/run-regression-tests.sh ../SpacetimeDB`).

### Issue 1: Unsubscribe is non-functional

See
clockworklabs/com.clockworklabs.spacetimedbsdk#279.
This was a coding mistake made during the rush to 1.0. It was easily
fixed.

### Issue 2: Indexes not tracking deletes

This was recently discovered by the BitCraft team.

The problem was introduced in
clockworklabs/com.clockworklabs.spacetimedbsdk@cefc727
which introduced `RemoteTableHandle<EventContext,
Row>.IndexBase<Column>`. This class stores a `Dictionary<Column,
List<Row>>` to allow fast filtering. It relies on `List<Row>.Remove` to
remove elements from a list when a row delete is received.
`List<Row>.Remove` relies on `Row.Equals` to determine when elements are
equal.

`Row.Equals` formerly was not implemented for `[SpacetimeDB.Type]`s or
for generated Row types. So, this didn't work.

Now it does: it was fixed by
#239 . So all this PR
does to address this issue is add a regression test.

## API

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

The interface IDbConnection was changed; however, this changed part of
the interface is `internal`, so this should be entirely invisible to
users.

## Requires SpacetimeDB PRs
#239 (merged)

## 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: master

## Testing
*Write instructions for a test that you performed for this PR*

- [x] Added a repro for both issues
- [x] Added the repro to CI
- [x] Fixed both issues
bfops pushed a commit to clockworklabs/com.clockworklabs.spacetimedbsdk that referenced this pull request Jul 28, 2025
@kazimuth
Fix for unsubscribe bug + regression tests (#264)
cf25e6b 
## Description of Changes

This PR fixes one issue and adds regression tests for it, plus another
issue. These regression tests are run on CI. You can also run them
locally -- run a local spacetime instance and then
`tools~/run-regression-tests.sh ../SpacetimeDB`).

### Issue 1: Unsubscribe is non-functional

See
#279.
This was a coding mistake made during the rush to 1.0. It was easily
fixed.

### Issue 2: Indexes not tracking deletes

This was recently discovered by the BitCraft team.

The problem was introduced in
cefc727
which introduced `RemoteTableHandle<EventContext,
Row>.IndexBase<Column>`. This class stores a `Dictionary<Column,
List<Row>>` to allow fast filtering. It relies on `List<Row>.Remove` to
remove elements from a list when a row delete is received.
`List<Row>.Remove` relies on `Row.Equals` to determine when elements are
equal.

`Row.Equals` formerly was not implemented for `[SpacetimeDB.Type]`s or
for generated Row types. So, this didn't work.

Now it does: it was fixed by
clockworklabs/SpacetimeDB#239 . So all this PR
does to address this issue is add a regression test.

## API

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

The interface IDbConnection was changed; however, this changed part of
the interface is `internal`, so this should be entirely invisible to
users.

## Requires SpacetimeDB PRs
clockworklabs/SpacetimeDB#239 (merged)

## 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: master

## Testing
*Write instructions for a test that you performed for this PR*

- [x] Added a repro for both issues
- [x] Added the repro to CI
- [x] Fixed both issues
bfops pushed a commit that referenced this pull request Aug 7, 2025
@gefjon @kazimuth
Rename Address to ConnectionId (#239)
c291472 
## Description of Changes

Companion to [Rename `Address` to
`ConnectionId`](#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

- #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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coolreader18 coolreader18 coolreader18 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gefjon @coolreader18