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

Make ColumnIndexAttribute bitflags in the ABI #72

Closed
wants to merge 64 commits into from

Conversation

RReverser
Copy link
Member

Description of Changes

As we're adding more variants, it's getting harder to keep track of the "A implies B" relationships between enum variants and to correctly do checks like "is this column autoinc" in different places.

Using bitflags makes expressing those relationships more natural and checking simpler - e.g. you can just do contains(AUTO_INC) as a single bit operation instead of having to remember and list all possible variants that imply autoinc, especially if we're going to add more flags in the future.

Initially I was going to make this change just in C# module generator and convert between ABI enum and bitflags over there, but it seems better to do this at ABI level so it simplifies Rust handling as well.

API

  • This is a breaking change to the module API
  • This is a breaking change to the ClientAPI

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

ABI representation of ColumnIndexAttribute (its numeric values) will be different after this change.

cloutiertyler and others added 30 commits June 18, 2023 22:19
* test

* Fixed CI

* Fixed tests

* Fixed formatting

* Fixed test error by removing chrono default features

* Rename smoketests CI thing

* Hopefully fix the testing issue

* Fix typos

* Fixed install git-hooks

* Fixed formatting

* Fixed compile error

* fixed testing issues
Signed-off-by: Tyler Cloutier <cloutiertyler@users.noreply.github.com>
Co-authored-by: Tyler Cloutier <cloutiertyler@aol.com>
* Clippy runs on default module

* Fix clippy issue with default module

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
* I think somehow we are adding all tables as orphans, added log

* Removed unneeded clone

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
)

* Fix bug for subcription that was not executing or returning if empty

* Clippy

* Filter out deleted rows

* Recover the op_type (DELETED, INSERTED) flag for the row in the subscription
* grandfathered docs improvements in bindings

* improve bindings

* address review comments

* remove late bound lifetime

* undo derive for Timestamp to bisect bug
* Forbid unsupported syntax in SELECT (like ORDER BY) & improve accuracy of sql test suite

* Update crates/sqltest/src/main.rs

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>

---------

Signed-off-by: Mario Montoya <mamcx@elmalabarista.com>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>
Co-authored-by: Boppy <no-reply@boppygames.gg>
* Working on improving commands that use identities

* Fix lints

* Reverted file that shouldn't have changed

* Found and fixed all other todos

* Addressed more CLI TODOs

* Fixes for formatting issues

* Set names of identities

* Set name of identities + clippy

* Small fix

* Added the start of a doc comment, switching over to another PR

* Fixed tests that needed to be updated

* Addressed more feedback and fixed several clippy issues

* Small fix

* Apply suggestions from code review

Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>

* Added more doc comments

* Addressing more feedback

* Fixed really old bug in SpacetimeDB

* Tests to verify new functionality

* Fix clippy lints

* Email during identity creation is optional

* Fix output so testsuite passes

---------

Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>
Co-authored-by: Boppy <no-reply@boppygames.gg>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com>
* Some work

* Getting smoketests working on mac

* All tests are passing except known failing tests

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
…urn wether or not reducer has subscribers; Moved all ReducerEvent and ReducerArgs code into a single file (#10)

Co-authored-by: Steve <steve@codefics.com>
Co-authored-by: Boppy <no-reply@boppygames.gg>
* Changed C# reducer signature; Change reducer invocation method to return wether or not reducer has subscribers; Moved all ReducerEvent and ReducerArgs code into a single file

* Formatting, tests

* Clippy

* Removed union from C# codegen

* Tests

* Update crates/cli/src/subcommands/generate/csharp.rs

Co-authored-by: Tyler Cloutier <cloutiertyler@users.noreply.github.com>
Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>

* Removed duplicate code

* Lint

* Clippy

* CLIPPY

* Generating custom indexes for unique / primary keys

---------

Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>
Co-authored-by: Steve <steve@codefics.com>
Co-authored-by: John Detter <4099508+jdetter@users.noreply.github.com>
Co-authored-by: Tyler Cloutier <cloutiertyler@users.noreply.github.com>
* Fix incomplete transaction bug

* Clippy
Co-authored-by: Steve <steve@codefics.com>
tokio runtimes cannot be nested, so we only create one if no ambient
context can be found and pass around a runtime handle for spawning
tasks.
* integrate jdetter's review

* address phoebe's comments

* improve errnos naming
Prior to this commit, callbacks in the Rust client SDK shared access to the global
ClientCache while running asynchronously. This meant that a long-running or delayed
callback could observe the ClientCache in a state later than the one that caused the
callback's invocation, and had no way to access the specific state for which it was
invoked.

With this commit, each `Invoke` message to the callback worker includes an
`Arc<ClientCache>` snapshot of the DB state when that callback was invoked. The callback
worker stores that state in a `thread_local`, and methods that inspect
tables (e.g. `TableType::iter`) read the state out of the `thread_local` when it is
present. This allows callbacks to observe exactly the state which caused their invocation,
never a later state, while maintaining the C#-like API where `ClientCache` access is based
on free functions or static trait methods.

---------

Co-authored-by: John Detter <4099508+jdetter@users.noreply.github.com>
* Fix bug and added a test

* Test updates

* Fix tests

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
drogus and others added 15 commits July 1, 2023 08:06
* [TypeScript] Remove entries from filtering

In the TypeScript SDK we keep "entries" - an algebraic value of records.
I added them because they were also used in the C# SDK, but we don't
really need them. This commit changes generated filtering functions so
that they compare actual instances instead of product values.
* Restoring Mazdak's nits

* Update auth.rs

Small issue

Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>

---------

Signed-off-by: John Detter <4099508+jdetter@users.noreply.github.com>
Co-authored-by: Boppy <no-reply@boppygames.gg>
* Cargo profile for building more quickly

* opt level is redundant

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
* Initial bootstrap for private tables

* Separate Access (private, public) for kind o table (system, user)

* Validates the access to private tables

* Check auth for drop table
* Allow to authenticate with a short lived token for websocket

* Add CORS

* Lints

* Name errors properly

* lint
* Script for publishing crates to crates.io

* Added sdk to the publish-crates script, renamed client-sdk to sdk

* Updated Cargo.lock and added 2 crates to the upgrade version script

* Small fix

* Actual fix

* Updated client API messages crate to be publishable

* Added LICENSE file to sdk

* Chose specific version for tokio-tungstenite

---------

Co-authored-by: Boppy <no-reply@boppygames.gg>
Co-authored-by: Tyler Cloutier <cloutiertyler@aol.com>
As we're adding more variants, it's getting harder to keep track of the "A implies B" relationships between enum variants and to correctly do checks like "is this column autoinc" in different places.

Using bitflags makes expressing those relationships more natural and checking simpler - e.g. you can just do `contains(AUTO_INC)` as a single bit operation instead of having to remember and list all possible variants that imply autoinc, especially if we're going to add more flags in the future.

Initially I was going to make this change just in C# module generator and convert between ABI enum and bitflags over there, but it seems better to do this at ABI level so it simplifies Rust handling as well.
ColumnAttr::Autoinc(span) => (ColumnIndexAttribute::AUTO_INC, span),
ColumnAttr::Primarykey(span) => (ColumnIndexAttribute::PRIMARY_KEY, span),
};
if col_attr.intersects(extra_col_attr) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: this condition is probably incorrect since intersection will also catch INDEXED bit, needs to remove it before checking.

Comment on lines 445 to 457
const UNSET = Self::empty().bits();
/// Index no unique
const INDEXED = 0b0001;
/// Generate the next [Sequence]
const AUTO_INC = 0b0010;
/// Index unique
const UNIQUE = Self::INDEXED.bits() | 0b0100;
/// Unique + AutoInc
const IDENTITY = Self::UNIQUE.bits() | Self::AUTO_INC.bits();
/// Primary key column (implies Unique)
const PRIMARY_KEY = Self::UNIQUE.bits() | 0b1000;
/// PrimaryKey + AutoInc
const PRIMARY_KEY_AUTO = Self::PRIMARY_KEY.bits() | Self::AUTO_INC.bits();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const UNSET = Self::empty().bits();
/// Index no unique
const INDEXED = 0b0001;
/// Generate the next [Sequence]
const AUTO_INC = 0b0010;
/// Index unique
const UNIQUE = Self::INDEXED.bits() | 0b0100;
/// Unique + AutoInc
const IDENTITY = Self::UNIQUE.bits() | Self::AUTO_INC.bits();
/// Primary key column (implies Unique)
const PRIMARY_KEY = Self::UNIQUE.bits() | 0b1000;
/// PrimaryKey + AutoInc
const PRIMARY_KEY_AUTO = Self::PRIMARY_KEY.bits() | Self::AUTO_INC.bits();
/// No attributes / flags set.
const UNSET = Self::empty().bits();
/// Indexed but not unique.
const INDEXED = 0b0001;
/// Generate the next [Sequence].
const AUTO_INC = 0b0010;
/// Indexed + unique.
const UNIQUE = Self::INDEXED.bits() | 0b0100;
/// Unique + AutoInc.
const IDENTITY = Self::UNIQUE.bits() | Self::AUTO_INC.bits();
/// Primary key column (implies Unique).
const PRIMARY_KEY = Self::UNIQUE.bits() | 0b1000;
/// PrimaryKey + AutoInc
const PRIMARY_KEY_AUTO = Self::PRIMARY_KEY.bits() | Self::AUTO_INC.bits();

});
let (unique_columns, nonunique_columns): (Vec<_>, Vec<_>) = columns
.iter()
.partition(|x| x.attr.contains(ColumnIndexAttribute::UNIQUE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines 701 to 710
match col.attr {
ColumnIndexAttribute::INDEXED => "INDEXED",
ColumnIndexAttribute::AUTO_INC => "AUTO_INC",
ColumnIndexAttribute::UNIQUE => "UNIQUE",
ColumnIndexAttribute::IDENTITY => "IDENTITY",
ColumnIndexAttribute::PRIMARY_KEY => "PRIMARY_KEY",
ColumnIndexAttribute::PRIMARY_KEY_AUTO => "PRIMARY_KEY_AUTO",
v if v.is_empty() => "UNSET",
_ => unreachable!("Invalid column attribute {:?}", col.attr),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider extracting out bits like this to methods / functions since the current one is getting rather large.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah fwiw ideally I'd just use iter_names of bitflags here, but it has a quirk where it deduplicates bitflags only if the larger ones (those that encompass others) come first.

I changed the definition of struct at first but figured it's more obvious/maintainable to just write out the names explicitly.

PrimaryKey = 5,
/// PrimaryKey + AutoInc
PrimaryKeyAuto = 6,
// TODO: any way to avoid duplication with same structure in bindings crate? Extra crate?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, extra crate would be good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.