From 764695db4aeb62f9fc5db9d9d3dd84b0ab1aaf60 Mon Sep 17 00:00:00 2001 From: William Brown Date: Tue, 22 Jan 2019 14:39:56 +1300 Subject: [PATCH] Working backend delete --- src/lib/be/mod.rs | 165 +++++++++++++++++++++++++++++++++++-------- src/lib/constants.rs | 3 +- src/lib/entry.rs | 6 ++ src/lib/error.rs | 4 +- src/lib/event.rs | 20 ++++++ src/lib/schema.rs | 6 +- src/lib/server.rs | 88 +++++++++++++++-------- 7 files changed, 231 insertions(+), 61 deletions(-) diff --git a/src/lib/be/mod.rs b/src/lib/be/mod.rs index 259aca1fc..eefcaa26b 100644 --- a/src/lib/be/mod.rs +++ b/src/lib/be/mod.rs @@ -17,9 +17,8 @@ mod sqlite_be; #[derive(Debug)] struct IdEntry { - // FIXME: This should be u64, but sqlite uses i32 ... - // Should we use a bigint pk and just be done? - id: i32, + // FIXME: This should be u64, but sqlite uses i64 ... + id: i64, data: String, } @@ -33,6 +32,7 @@ pub enum BackendType { #[derive(Debug, PartialEq)] pub enum BackendError { EmptyRequest, + EntryMissingId, } pub struct Backend { @@ -67,7 +67,7 @@ pub trait BackendReadTransaction { // Do a final optimise of the filter let filt = filt.optimise(); - let mut raw_entries: Vec = Vec::new(); + let mut raw_entries: Vec = Vec::new(); { // Actually do a search now! // let conn = self.pool.get().unwrap(); @@ -87,19 +87,18 @@ pub trait BackendReadTransaction { .unwrap(); for row in id2entry_iter { audit_log!(au, "raw entry: {:?}", row); - // FIXME: Handle this properly. - raw_entries.push(row.unwrap().data); + // FIXME: Handle possible errors correctly. + raw_entries.push(row.unwrap()); } - // Rollback, we should have done nothing. - // conn.execute("ROLLBACK TRANSACTION", NO_PARAMS).unwrap(); } // Do other things - // Now, de-serialise the raw_entries back to entries + // Now, de-serialise the raw_entries back to entries, and populate their ID's let entries: Vec = raw_entries .iter() - .filter_map(|val| { + .filter_map(|id_ent| { // TODO: Should we do better than unwrap? - let e: Entry = serde_json::from_str(val.as_str()).unwrap(); + let mut e: Entry = serde_json::from_str(id_ent.data.as_str()).unwrap(); + e.id = Some(id_ent.id); if filt.entry_match_no_index(&e) { Some(e) } else { @@ -206,6 +205,18 @@ impl BackendWriteTransaction { } } + fn get_id2entry_max_id(&self) -> i64 { + let mut stmt = self.conn.prepare("SELECT MAX(id) as id_max FROM id2entry").unwrap(); + assert!(stmt.exists(NO_PARAMS).unwrap()); + + let i: Option = stmt.query_row(NO_PARAMS, |row| row.get(0)) + .expect("failed to execute"); + match i { + Some(e) => e, + None => 0, + } + } + pub fn create(&self, au: &mut AuditScope, entries: &Vec) -> Result<(), BackendError> { audit_segment!(au, || { // Start be audit timer @@ -220,11 +231,18 @@ impl BackendWriteTransaction { // we do this outside the txn to avoid blocking needlessly. // However, it could be pointless due to the extra string allocs ... - let ser_entries: Vec = entries + // Get the max id from the db. We store this ourselves to avoid max(). + let mut id_max = self.get_id2entry_max_id(); + + let ser_entries: Vec = entries .iter() .map(|val| { // TODO: Should we do better than unwrap? - serde_json::to_string(&val).unwrap() + id_max = id_max + 1; + IdEntry { + id: id_max, + data: serde_json::to_string(&val).unwrap(), + } }) .collect(); @@ -237,10 +255,11 @@ impl BackendWriteTransaction { // write them all for ser_entry in ser_entries { + // TODO: Prepared statement. self.conn .execute( - "INSERT INTO id2entry (data) VALUES (?1)", - &[&ser_entry as &ToSql], + "INSERT INTO id2entry (id, data) VALUES (?1, ?2)", + &[&ser_entry.id as &ToSql, &ser_entry.data as &ToSql], ) .unwrap(); } @@ -254,20 +273,56 @@ impl BackendWriteTransaction { }) } - pub fn modify() { + pub fn modify() -> Result<(), BackendError> { unimplemented!() } - pub fn delete() { - unimplemented!() + pub fn delete(&self, au: &mut AuditScope, entries: &Vec) -> Result<(), BackendError> { + // Perform a search for the entries --> This is a problem for the caller + + if entries.is_empty() { + // TODO: Better error + // End the timer + return Err(BackendError::EmptyRequest); + } + + // Assert the Id's exist on the entry. + let id_list: Vec = entries.iter() + .filter_map(|entry| { + entry.id + }) + .collect(); + + // Simple: If the list of id's is not the same as the input list, we are missing id's + if entries.len() != id_list.len() { + return Err(BackendError::EntryMissingId); + } + + + + // Now, given the list of id's, delete them. + { + // SQL doesn't say if the thing "does or does not exist anymore". As a result, + // two deletes is a safe and valid operation. Given how we allocate ID's we are + // probably okay with this. + + // TODO: ACTUALLY HANDLE THIS ERROR WILLIAM YOU LAZY SHIT. + let mut stmt = self.conn.prepare("DELETE FROM id2entry WHERE id = :id").unwrap(); + + id_list.iter().for_each(|id| { + stmt.execute(&[id]).unwrap(); + }); + } + + Ok(()) } - pub fn backup() { + pub fn backup() -> Result<(), BackendError> { unimplemented!() } // Should this be offline only? - pub fn restore() { + pub fn restore() -> Result<(), BackendError> { unimplemented!() } @@ -286,7 +341,7 @@ impl BackendWriteTransaction { // ===== inner helpers ===== // Some of these are not self due to use in new() - fn get_db_version_key(&self, key: &str) -> i32 { + fn get_db_version_key(&self, key: &str) -> i64 { match self.conn.query_row_named( "SELECT version FROM db_version WHERE id = :id", &[(":id", &key)], @@ -458,6 +513,14 @@ mod tests { }}; } + macro_rules! entry_exists { + ($audit:expr, $be:expr, $ent:expr) => {{ + let filt = $ent.filter_from_attrs(&vec![String::from("userid")]).unwrap(); + let entries = $be.search($audit, &filt).unwrap(); + entries.first().is_some() + }} + } + #[test] fn test_simple_create() { run_test!(|audit: &mut AuditScope, be: &BackendWriteTransaction| { @@ -470,17 +533,12 @@ mod tests { let mut e: Entry = Entry::new(); e.add_ava(String::from("userid"), String::from("william")); - let single_result = be.create(audit, &vec![e]); + let single_result = be.create(audit, &vec![e.clone()]); assert!(single_result.is_ok()); // Construct a filter - let filt = Filter::Pres(String::from("userid")); - let entries = be.search(audit, &filt).unwrap(); - - // There should only be one entry so is this enough? - assert!(entries.first().is_some()); - // Later we could check consistency of the entry saved ... + assert!(entry_exists!(audit, be, e)); }); } @@ -502,6 +560,57 @@ mod tests { fn test_simple_delete() { run_test!(|audit: &mut AuditScope, be: &BackendWriteTransaction| { audit_log!(audit, "Simple Delete"); + + // First create some entries (3?) + let mut e1: Entry = Entry::new(); + e1.add_ava(String::from("userid"), String::from("william")); + + let mut e2: Entry = Entry::new(); + e2.add_ava(String::from("userid"), String::from("alice")); + + let mut e3: Entry = Entry::new(); + e3.add_ava(String::from("userid"), String::from("lucy")); + + assert!(be.create(audit, &vec![e1.clone(), e2.clone(), e3.clone()]).is_ok()); + assert!(entry_exists!(audit, be, e1)); + assert!(entry_exists!(audit, be, e2)); + assert!(entry_exists!(audit, be, e3)); + + + // You need to now retrieve the entries back out to get the entry id's + let mut results = be.search(audit, &Filter::Pres(String::from("userid"))).unwrap(); + + // Get these out to usable entries. + let r1 = results.remove(0); + let r2 = results.remove(0); + let r3 = results.remove(0); + + // Delete one + assert!(be.delete(audit, &vec![r1.clone()]).is_ok()); + assert!(! entry_exists!(audit, be, r1)); + + // delete none (no match filter) + assert!(be.delete(audit, &vec![]).is_err()); + + // Delete with no id + let mut e4: Entry = Entry::new(); + e4.add_ava(String::from("userid"), String::from("amy")); + + assert!(be.delete(audit, &vec![e4.clone()]).is_err()); + + assert!(entry_exists!(audit, be, r2)); + assert!(entry_exists!(audit, be, r3)); + + // delete batch + assert!(be.delete(audit, &vec![r2.clone(), r3.clone()]).is_ok()); + + assert!(! entry_exists!(audit, be, r2)); + assert!(! entry_exists!(audit, be, r3)); + + // delete none (no entries left) + // see fn delete for why this is ok, not err + assert!(be.delete(audit, &vec![r2.clone(), r3.clone()]).is_ok()); + }); } } diff --git a/src/lib/constants.rs b/src/lib/constants.rs index 989c355e9..20907557a 100644 --- a/src/lib/constants.rs +++ b/src/lib/constants.rs @@ -7,7 +7,8 @@ pub static JSON_ANONYMOUS_V1: &'static str = r#"{ "name": ["anonymous"], "uuid": ["00000000-0000-0000-0000-ffffffffffff"], "description": ["Anonymous access account."], - "version": ["1"] + "version": ["1"], + "displayname": ["Anonymous"] } }"#; diff --git a/src/lib/entry.rs b/src/lib/entry.rs index f6d7fac61..6be905485 100644 --- a/src/lib/entry.rs +++ b/src/lib/entry.rs @@ -93,14 +93,18 @@ impl<'a> Iterator for EntryAvasMut<'a> { } } +// This is a BE concept, so move it there! #[derive(Serialize, Deserialize, Debug)] pub struct Entry { + pub id: Option, attrs: BTreeMap>, } impl Entry { pub fn new() -> Self { Entry { + // This means NEVER COMMITED + id: None, attrs: BTreeMap::new(), } } @@ -222,6 +226,7 @@ impl Entry { Entry { // For now, we do a straight move, and we sort the incoming data // sets so that BST works. + id: None, attrs: e .attrs .iter() @@ -250,6 +255,7 @@ impl Entry { impl Clone for Entry { fn clone(&self) -> Entry { Entry { + id: self.id, attrs: self.attrs.clone(), } } diff --git a/src/lib/error.rs b/src/lib/error.rs index 57e5c0047..5585ee83c 100644 --- a/src/lib/error.rs +++ b/src/lib/error.rs @@ -4,7 +4,7 @@ pub enum SchemaError { InvalidClass, // FIXME: Is there a way to say what we are missing on error? // Yes, add a string on the enum. - MissingMustAttribute, + MissingMustAttribute(String), InvalidAttribute, InvalidAttributeSyntax, EmptyFilter, @@ -17,4 +17,6 @@ pub enum OperationError { SchemaViolation, Plugin, FilterGeneration, + InvalidDBState, + InvalidRequestState, } diff --git a/src/lib/event.rs b/src/lib/event.rs index 555a71d28..533c5bb02 100644 --- a/src/lib/event.rs +++ b/src/lib/event.rs @@ -136,3 +136,23 @@ impl ExistsEvent { } } } + +#[derive(Debug)] +pub struct DeleteEvent { + pub filter: Filter, + pub internal: bool, +} + +impl Message for DeleteEvent { + type Result = Result; +} + +impl DeleteEvent { + pub fn new_internal(filter: Filter) -> Self { + DeleteEvent { + filter: filter, + internal: true, + } + } +} + diff --git a/src/lib/schema.rs b/src/lib/schema.rs index d5b28555b..c685f1892 100644 --- a/src/lib/schema.rs +++ b/src/lib/schema.rs @@ -910,7 +910,9 @@ impl SchemaInner { for (attr_name, _attr) in must { let avas = entry.get_ava(&attr_name); if avas.is_none() { - return Err(SchemaError::MissingMustAttribute); + return Err(SchemaError::MissingMustAttribute( + String::from(attr_name) + )); } } @@ -1373,7 +1375,7 @@ mod tests { assert_eq!( schema.validate_entry(&e_attr_invalid), - Err(SchemaError::MissingMustAttribute) + Err(SchemaError::MissingMustAttribute(String::from("system"))) ); let e_attr_invalid_may: Entry = serde_json::from_str( diff --git a/src/lib/server.rs b/src/lib/server.rs index e4fc36913..3ac686740 100644 --- a/src/lib/server.rs +++ b/src/lib/server.rs @@ -12,7 +12,7 @@ use be::{ use constants::{JSON_ANONYMOUS_V1, JSON_SYSTEM_INFO_V1}; use entry::Entry; use error::OperationError; -use event::{CreateEvent, ExistsEvent, OpResult, SearchEvent, SearchResult}; +use event::{CreateEvent, ExistsEvent, OpResult, SearchEvent, SearchResult, DeleteEvent}; use filter::Filter; use log::EventLog; use plugins::Plugins; @@ -139,7 +139,7 @@ pub trait QueryServerReadTransaction { res } - fn internal_search(&self, _au: &mut AuditScope, _filter: Filter) -> Result<(), ()> { + fn internal_search(&self, _au: &mut AuditScope, _filter: Filter) -> Result, OperationError> { unimplemented!() } } @@ -258,7 +258,10 @@ impl<'a> QueryServerWriteTransaction<'a> { if acc.is_ok() { self.schema .validate_entry(e) - .map_err(|_| OperationError::SchemaViolation) + .map_err(|err| { + audit_log!(au, "Schema Violation: {:?}", err); + OperationError::SchemaViolation + }) } else { acc } @@ -284,6 +287,7 @@ impl<'a> QueryServerWriteTransaction<'a> { .map(|_| ()) .map_err(|e| match e { BackendError::EmptyRequest => OperationError::EmptyRequest, + BackendError::EntryMissingId => OperationError::InvalidRequestState, }); au.append_scope(audit_be); @@ -304,6 +308,41 @@ impl<'a> QueryServerWriteTransaction<'a> { res } + + pub fn delete(&self, au: &mut AuditScope, ce: &DeleteEvent) -> Result<(), OperationError> { + unimplemented!() + } + + // These are where searches and other actions are actually implemented. This + // is the "internal" version, where we define the event as being internal + // only, allowing certain plugin by passes etc. + + pub fn internal_create( + &self, + audit: &mut AuditScope, + entries: Vec, + ) -> Result<(), OperationError> { + // Start the audit scope + let mut audit_int = AuditScope::new("internal_create"); + // Create the CreateEvent + let ce = CreateEvent::new_internal(entries); + let res = self.create(&mut audit_int, &ce); + audit.append_scope(audit_int); + res + } + + pub fn internal_delete( + &self, + audit: &mut AuditScope, + filter: Filter + ) -> Result<(), OperationError> { + let mut audit_int = AuditScope::new("internal_delete"); + let de = DeleteEvent::new_internal(filter); + let res = self.delete(&mut audit_int, &de); + audit.append_scope(audit_int); + res + } + // internal server operation types. // These just wrap the fn create/search etc, but they allow // creating the needed create event with the correct internal flags @@ -370,14 +409,21 @@ impl<'a> QueryServerWriteTransaction<'a> { }; // Does it exist? (TODO: Should be search, not exists ...) - match self.internal_exists(audit, filt) { - Ok(true) => { - // it exists. We need to ensure the content now. - unimplemented!() - } - Ok(false) => { - // It does not exist. Create it. - self.internal_create(audit, vec![e]) + match self.internal_search(audit, filt.clone()) { + Ok(results) => { + if results.len() == 0 { + // It does not exist. Create it. + self.internal_create(audit, vec![e]) + } else if results.len() == 1 { + // it exists. To guarantee content exactly as is, we compare if it's identical. + if e != results[0] { + self.internal_delete(audit, filt); + self.internal_create(audit, vec![e]); + }; + Ok(()) + } else { + Err(OperationError::InvalidDBState) + } } Err(e) => { // An error occured. pass it back up. @@ -386,24 +432,6 @@ impl<'a> QueryServerWriteTransaction<'a> { } } - // These are where searches and other actions are actually implemented. This - // is the "internal" version, where we define the event as being internal - // only, allowing certain plugin by passes etc. - - pub fn internal_create( - &self, - audit: &mut AuditScope, - entries: Vec, - ) -> Result<(), OperationError> { - // Start the audit scope - let mut audit_int = AuditScope::new("internal_create"); - // Create the CreateEvent - let ce = CreateEvent::new_internal(entries); - let res = self.create(&mut audit_int, &ce); - audit.append_scope(audit_int); - res - } - // This function is idempotent pub fn initialise(&self, audit: &mut AuditScope) -> Result<(), OperationError> { // First, check the system_info object. This stores some server information @@ -414,6 +442,7 @@ impl<'a> QueryServerWriteTransaction<'a> { self.internal_assert_or_create(audit, e) }); audit.append_scope(audit_si); + assert!(res.is_ok()); if res.is_err() { return res; } @@ -425,6 +454,7 @@ impl<'a> QueryServerWriteTransaction<'a> { self.internal_migrate_or_create(audit, e) }); audit.append_scope(audit_an); + assert!(res.is_ok()); if res.is_err() { return res; }