refactor: update fetchfromdatabase and associated APIs to new Entity trait family [WPB-22195]
#1697
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We already knew that there is a real difference between key types which are owned and those which are borrowed. We already knew that entity keys need to be of the owned kind. Adding this trait allows us to formalize that notion, and add a utility to construct an owned variant from some arbitrary bytes.
Just looking to give it complete coverage of the kinds of things that the new entity traits offer.
- break up the impls into separate files for easier navigation - separate the concept of an entity type and an entity id - borrow the actual entity instead of owning it - make `execute_save` and `execute_delete` methods not free functions - restrict visibility: nothing external should really need this dynamic dispatch stuff
We used to do some crazy thing involving unencrypted serialization for some reason. It's much simpler this way: when an entity turns itself into a dynamic entity, we just keep it. We do Arc-wrap it, because there are times we want to iterate over a bunch of owned instances and don't want the expense of cloning these potentially- large items every time. But from there the API is straightforward. While it was cool we could make the borrowing approach work for dynamic entities, in practice it turned out that in general when we hand an entity to the database to save, we really want it to take over ownership, which means this pattern is better.
The traits didn't previously allow for multiple distinct implementations of borrowing types sharing an owned type (I think) anyway, and we already had an associated type naming a specific borrowed primary key, so it was pointless overkill to accept a complicated generic parameter type in these methods. Better and simpler to just use the associated type.
Pending messages have no real primary key and are always/only accessed and removed in bulk, which turns out to be _really annoying_ with regard to the new entity traits, which assumes that some primary key does exist. For now we're faking the foreign id as the primary key so that encryption works at all, and also for dynamic dispatch. But I'd really love to rework this at some point in the future.
Fascinatingly, we're allowed to simply omit the trait bounds where they can be statically proven, i.e. on an associated type. We're learning about Rust today!
Turns out that there's a real benefit to storing the dynamic-dispatch entities the way we do, and that's a huge simplification of the transaction stuff.
The trait used to be a bit loosely specified, since nobody was implementing it. Now we know precisely what it needs and how to implement it.
coriolinus
force-pushed
the
prgn/refactor/22195-update-fetchfromdatabase
branch
from
d14ff2d to
df7fa6b
Compare
So, I thought there were no more trait refactors incoming. But then it turned out that there was a bug in the auto-implementations of the unique entity helper on wasm only: `Entity` had an auto-impl where `T: UniqueEntity` (so it could reference `T::KEY`), but `UniqueEntity` had an auto-impl where `T: Entity` (so it could reference `T::PrimaryKey`), and rustc was running out of stack trying to figure out if a particular instance of `T` implemented both or neither of those. So we broke out `PrimaryKey` from `Entity` which broke that loop, and as a nice side effect means that there's now better symmetry between `PrimaryKey` and `BorrowPrimaryKey`, and `Entity` and `EntityGetBorrowed` (as well as `EntityDatabaseMutation` and `EntityDeleteBorrowed`). But now I have to fix all the impls.
Just a bunch of busywork necessary to support the switch to `PrimaryKey`, `BorrowPrimaryKey`, `EntityGetBorrowed`.
coriolinus
force-pushed
the
prgn/refactor/22195-update-fetchfromdatabase
branch
from
6aaf525 to
224ca2d
Compare
coriolinus
force-pushed
the
prgn/refactor/22195-update-fetchfromdatabase
branch
from
224ca2d to
b62eb8d
Compare
If the open transaction contains invalid state (i.e. violating DB constraints), we'd rather not panic within the Drop impl.
For some reason we're computing and storing a sha256 hash for these types, but we're not using it as the primary key. OpenMLS uses the raw id field as the primary key in a way that we can't modify, so that's what has to be the pk. As for the hash: wasted space.
Update the previous version with this which fixes the correct primary key
We'd had an issue where keypackages couldn't be loaded due to improper handling of hex-encoded keys, but this test now proves that we've resolved that issue.
…iate Turns out that when we have a field whose ID is a hex-encoded value, the field is a string and the hex is lowercase. And what that means is that we have to perform that transform when searching for that key!
1. Only fetch the database once 2. Given that we no longer raise errors on missing values, update the condition to produce an "already registered" error appropriately.
…d set Without this behavior, if we have these operations - Rm the entity with id X - add a new entity with id X then it gets added and then immediately deleted again, which is not helpful.
This entity type is really annoying because it just doesn't have a single primary key per entity. So when we faked it as just the conversation id, as soon as there was more than 1 pending message per conversation, they started overwriting each other within the transaction. So we've added a bunch of changes and workarounds to make it go. I'm hopeful that in the future we can get rid of all or nearly all of this, but for now, we're stuck with it.
For whatever reason the entity new derive macro was calling `new_get` and `new_delete` with `String` or `&str` instances instead of the expected `Vec<u8>` / `&[u8]`, and of course that was failing type checking. But actually we'd prefer to have those functions available. So broadening the acceptable types solves the problem for us (probably).
Bencher ReportTurns out that rusqlite won't read null values as an empty byte vector, so we have to add another column method governing how to generate those get expressions.
Turns out that we were loading from the wrong keys this whole time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's new in this PR
Reorganize the core mechanisms in the keystore to require new entity trait impls instead of old entity trait impls.
PR Submission Checklist for internal contributors
SQPIT-764feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.