-
Notifications
You must be signed in to change notification settings - Fork 34
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
Private Preferences DB #946
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
b518cc9
create the database migration for the private preference work
nplasterer d65af0d
Merge branch 'mls-dm-release' of https://github.com/xmtp/libxmtp into…
nplasterer 882581b
Merge branch 'mls-dm-release' of https://github.com/xmtp/libxmtp into…
nplasterer 07ac8d4
update the table to be focused on consent
nplasterer 53775f3
first pass at database storage structure
nplasterer 56d0da2
update the get method for consent records
nplasterer af31a14
fix up the set method
nplasterer c3297eb
Merge branch 'mls-dm-release' of https://github.com/xmtp/libxmtp into…
nplasterer ce5c562
add a test
nplasterer cc70354
fix up the test
nplasterer d0c149c
fix up the clippy error with consent record
nplasterer 5957273
fix up the clippy error with consent record
nplasterer 714aa8d
fix up all clippy issues
nplasterer c038001
cargo fmt
nplasterer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
2 changes: 2 additions & 0 deletions
2
xmtp_mls/migrations/2024-08-07-213816_create-private-preference-store/down.sql
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
-- This file should undo anything in `up.sql` | ||
DROP TABLE IF EXISTS "consent_records"; |
9 changes: 9 additions & 0 deletions
9
xmtp_mls/migrations/2024-08-07-213816_create-private-preference-store/up.sql
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
CREATE TABLE "consent_records"( | ||
-- Enum of the CONSENT_TYPE (GROUP_ID, INBOX_ID, etc..) | ||
"entity_type" int NOT NULL, | ||
-- Enum of CONSENT_STATE (ALLOWED, DENIED, etc..) | ||
"state" int NOT NULL, | ||
-- The entity of what has consent (0x00 etc..) | ||
"entity" text NOT NULL, | ||
PRIMARY KEY (entity_type, entity) | ||
); | ||
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,8 @@ pub enum GroupMetadataError { | |
InvalidConversationType, | ||
#[error("missing extension")] | ||
MissingExtension, | ||
#[error("missing a dm member")] | ||
MissingDmMember, | ||
} | ||
|
||
#[derive(Debug, Clone, PartialEq)] | ||
|
@@ -72,14 +74,10 @@ impl TryFrom<GroupMetadataProto> for GroupMetadata { | |
type Error = GroupMetadataError; | ||
|
||
fn try_from(value: GroupMetadataProto) -> Result<Self, Self::Error> { | ||
let dm_members = if value.dm_members.is_some() { | ||
Some(DmMembers::try_from(value.dm_members.unwrap())?) | ||
} else { | ||
None | ||
}; | ||
let dm_members = value.dm_members.map(DmMembers::try_from).transpose()?; | ||
Ok(Self::new( | ||
value.conversation_type.try_into()?, | ||
value.creator_inbox_id.clone(), | ||
value.creator_inbox_id, | ||
dm_members, | ||
)) | ||
} | ||
|
@@ -150,8 +148,14 @@ impl TryFrom<DmMembersProto> for DmMembers { | |
|
||
fn try_from(value: DmMembersProto) -> Result<Self, Self::Error> { | ||
Ok(Self { | ||
member_one_inbox_id: value.dm_member_one.unwrap().inbox_id.clone(), | ||
member_two_inbox_id: value.dm_member_two.unwrap().inbox_id.clone(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These unwraps were just on the mls_dms feature branch already. Just cleaning up the linter in this PR. cc @cameronvoell |
||
member_one_inbox_id: value | ||
.dm_member_one | ||
.ok_or(GroupMetadataError::MissingDmMember)? | ||
.inbox_id, | ||
member_two_inbox_id: value | ||
.dm_member_two | ||
.ok_or(GroupMetadataError::MissingDmMember)? | ||
.inbox_id, | ||
}) | ||
} | ||
} | ||
|
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
use crate::{impl_store, storage::StorageError}; | ||
|
||
use super::{ | ||
db_connection::DbConnection, | ||
schema::consent_records::{self, dsl}, | ||
}; | ||
use diesel::{ | ||
backend::Backend, | ||
deserialize::{self, FromSql, FromSqlRow}, | ||
expression::AsExpression, | ||
prelude::*, | ||
serialize::{self, IsNull, Output, ToSql}, | ||
sql_types::Integer, | ||
sqlite::Sqlite, | ||
upsert::excluded, | ||
}; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
/// StoredConsentRecord holds a serialized ConsentRecord | ||
#[derive(Insertable, Queryable, Debug, Clone, PartialEq, Eq)] | ||
#[diesel(table_name = consent_records)] | ||
#[diesel(primary_key(entity_type, entity))] | ||
pub struct StoredConsentRecord { | ||
/// Enum, [`ConsentType`] representing the type of consent (group_id inbox_id, etc..) | ||
pub entity_type: ConsentType, | ||
/// Enum, [`ConsentState`] representing the state of consent (allowed, denied, etc..) | ||
pub state: ConsentState, | ||
/// The entity of what was consented (0x00 etc..) | ||
pub entity: String, | ||
} | ||
|
||
impl StoredConsentRecord { | ||
pub fn new(entity_type: ConsentType, state: ConsentState, entity: String) -> Self { | ||
Self { | ||
entity_type, | ||
state, | ||
entity, | ||
} | ||
} | ||
} | ||
|
||
impl_store!(StoredConsentRecord, consent_records); | ||
|
||
impl DbConnection { | ||
/// Returns the consent_records for the given entity up | ||
pub fn get_consent_record( | ||
&self, | ||
entity: String, | ||
entity_type: ConsentType, | ||
) -> Result<Option<StoredConsentRecord>, StorageError> { | ||
Ok(self.raw_query(|conn| { | ||
dsl::consent_records | ||
.filter(dsl::entity.eq(entity)) | ||
.filter(dsl::entity_type.eq(entity_type)) | ||
.first(conn) | ||
.optional() | ||
})?) | ||
} | ||
|
||
/// Insert consent_record, and replace existing entries | ||
pub fn insert_or_replace_consent_record( | ||
&self, | ||
record: StoredConsentRecord, | ||
) -> Result<(), StorageError> { | ||
self.raw_query(|conn| { | ||
diesel::insert_into(dsl::consent_records) | ||
.values(&record) | ||
.on_conflict((dsl::entity_type, dsl::entity)) | ||
.do_update() | ||
.set(dsl::state.eq(excluded(dsl::state))) | ||
.execute(conn)?; | ||
Ok(()) | ||
})?; | ||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
#[repr(i32)] | ||
#[derive(Debug, Copy, Clone, Serialize, Deserialize, Eq, PartialEq, AsExpression, FromSqlRow)] | ||
#[diesel(sql_type = Integer)] | ||
/// Type of consent record stored | ||
pub enum ConsentType { | ||
/// Consent is for a group | ||
GroupId = 1, | ||
/// Consent is for an inbox | ||
InboxId = 2, | ||
/// Consent is for an address | ||
Address = 3, | ||
} | ||
|
||
impl ToSql<Integer, Sqlite> for ConsentType | ||
where | ||
i32: ToSql<Integer, Sqlite>, | ||
{ | ||
fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result { | ||
out.set_value(*self as i32); | ||
Ok(IsNull::No) | ||
} | ||
} | ||
|
||
impl FromSql<Integer, Sqlite> for ConsentType | ||
where | ||
i32: FromSql<Integer, Sqlite>, | ||
{ | ||
fn from_sql(bytes: <Sqlite as Backend>::RawValue<'_>) -> deserialize::Result<Self> { | ||
match i32::from_sql(bytes)? { | ||
1 => Ok(ConsentType::GroupId), | ||
2 => Ok(ConsentType::InboxId), | ||
3 => Ok(ConsentType::Address), | ||
x => Err(format!("Unrecognized variant {}", x).into()), | ||
} | ||
} | ||
} | ||
|
||
#[repr(i32)] | ||
#[derive(Debug, Copy, Clone, Serialize, Deserialize, Eq, PartialEq, AsExpression, FromSqlRow)] | ||
#[diesel(sql_type = Integer)] | ||
/// The state of the consent | ||
pub enum ConsentState { | ||
/// Consent is allowed | ||
Allowed = 1, | ||
/// Consent is denied | ||
Denied = 2, | ||
} | ||
|
||
impl ToSql<Integer, Sqlite> for ConsentState | ||
where | ||
i32: ToSql<Integer, Sqlite>, | ||
{ | ||
fn to_sql<'b>(&'b self, out: &mut Output<'b, '_, Sqlite>) -> serialize::Result { | ||
out.set_value(*self as i32); | ||
Ok(IsNull::No) | ||
} | ||
} | ||
|
||
impl FromSql<Integer, Sqlite> for ConsentState | ||
where | ||
i32: FromSql<Integer, Sqlite>, | ||
{ | ||
fn from_sql(bytes: <Sqlite as Backend>::RawValue<'_>) -> deserialize::Result<Self> { | ||
match i32::from_sql(bytes)? { | ||
1 => Ok(ConsentState::Allowed), | ||
2 => Ok(ConsentState::Denied), | ||
x => Err(format!("Unrecognized variant {}", x).into()), | ||
} | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use crate::storage::encrypted_store::tests::with_connection; | ||
|
||
use super::*; | ||
|
||
fn generate_consent_record( | ||
entity_type: ConsentType, | ||
state: ConsentState, | ||
entity: String, | ||
) -> StoredConsentRecord { | ||
StoredConsentRecord { | ||
entity_type, | ||
state, | ||
entity, | ||
} | ||
} | ||
|
||
#[test] | ||
fn insert_and_read() { | ||
with_connection(|conn| { | ||
let inbox_id = "inbox_1"; | ||
let consent_record = generate_consent_record( | ||
ConsentType::InboxId, | ||
ConsentState::Denied, | ||
inbox_id.to_string(), | ||
); | ||
let consent_record_entity = consent_record.entity.clone(); | ||
|
||
conn.insert_or_replace_consent_record(consent_record) | ||
.expect("should store without error"); | ||
|
||
let consent_record = conn | ||
.get_consent_record(inbox_id.to_owned(), ConsentType::InboxId) | ||
.expect("query should work"); | ||
|
||
assert_eq!(consent_record.unwrap().entity, consent_record_entity); | ||
}); | ||
} | ||
} |
This file contains 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
This file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to make sure I'm reasoning about this correctly we will want a way to get the current consent state for a group or inbox id performantly so the primary key should be the value and the valueType so that you can fetch for type group and value group_id and get what the state is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does feel like what we'll want at the end of the process. Very cheap to look up, has all the information a
One thing that was a struggle in the past is just making sure that we processed all the events in the correct order, so that if you allowed and then blocked the same group you'd get a consistent value in the end. What happens if you received a preferences event that you couldn't read before, but now you can? How do we rewind things so that you can rebuild the state? Maybe the right answer is to truncate this table and start from scratch in that scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to make an index here, like we did for inbox id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the primary key made an index by default so we shouldn't need to add another index.