From 2b47e544a853ece82f81d29b1fb15d61d684077c Mon Sep 17 00:00:00 2001 From: Damien Elmes Date: Sun, 10 May 2020 18:09:18 +1000 Subject: [PATCH] add tests for card fixes; implement deck recovery --- rslib/ftl/database-check.ftl | 6 +- rslib/src/backend/mod.rs | 5 +- rslib/src/dbcheck.rs | 271 +++++++++++++++++-------- rslib/src/decks/mod.rs | 24 ++- rslib/src/storage/revlog/fix_props.sql | 2 +- 5 files changed, 218 insertions(+), 90 deletions(-) diff --git a/rslib/ftl/database-check.ftl b/rslib/ftl/database-check.ftl index ed30321f9..0d06449c6 100644 --- a/rslib/ftl/database-check.ftl +++ b/rslib/ftl/database-check.ftl @@ -2,8 +2,8 @@ database-check-corrupt = Collection file is corrupt. Please restore from an auto database-check-rebuilt = Database rebuilt and optimized. database-check-card-properties = { $count -> - [one] Fixed { $count } card with invalid properties. - *[other] Fixed { $count } cards with invalid properties. + [one] Fixed { $count } invalid card property. + *[other] Fixed { $count } invalid card properties. } database-check-missing-templates = @@ -18,7 +18,7 @@ database-check-field-count = { $count -> } database-check-new-card-high-due = { $count -> - [one] Found { $count } new card with a due number >= 1,000,000 - consider repositioning them in the Browse screen. + [one] Found { $count } new card with a due number >= 1,000,000 - consider repositioning it in the Browse screen. *[other] Found { $count } new cards with a due number >= 1,000,000 - consider repositioning them in the Browse screen. } diff --git a/rslib/src/backend/mod.rs b/rslib/src/backend/mod.rs index 90a12f9e4..8caac1ad1 100644 --- a/rslib/src/backend/mod.rs +++ b/rslib/src/backend/mod.rs @@ -1041,8 +1041,9 @@ impl Backend { fn check_database(&self) -> Result { self.with_col(|col| { - col.check_database() - .map(|problems| pb::CheckDatabaseOut { problems }) + col.check_database().map(|problems| pb::CheckDatabaseOut { + problems: problems.to_i18n_strings(&col.i18n), + }) }) } diff --git a/rslib/src/dbcheck.rs b/rslib/src/dbcheck.rs index 4849da8ed..a91291805 100644 --- a/rslib/src/dbcheck.rs +++ b/rslib/src/dbcheck.rs @@ -4,7 +4,7 @@ use crate::{ collection::Collection, err::{AnkiError, DBErrorKind, Result}, - i18n::{tr_args, TR}, + i18n::{tr_args, I18n, TR}, notetype::{ all_stock_notetypes, AlreadyGeneratedCardInfo, CardGenContext, NoteType, NoteTypeKind, }, @@ -17,9 +17,78 @@ use std::{ sync::Arc, }; +#[derive(Debug, Default, PartialEq)] +pub struct CheckDatabaseOutput { + card_properties_invalid: usize, + card_position_too_high: usize, + cards_missing_note: usize, + deck_ids_missing: usize, + revlog_properties_invalid: usize, + templates_missing: usize, + card_ords_duplicated: usize, + field_count_mismatch: usize, +} + +impl CheckDatabaseOutput { + pub fn to_i18n_strings(&self, i18n: &I18n) -> Vec { + let mut probs = Vec::new(); + + if self.card_position_too_high > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckNewCardHighDue, + tr_args!["count"=>self.card_position_too_high], + )); + } + if self.card_properties_invalid > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckCardProperties, + tr_args!["count"=>self.card_properties_invalid], + )); + } + if self.cards_missing_note > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckCardMissingNote, + tr_args!["count"=>self.cards_missing_note], + )); + } + if self.deck_ids_missing > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckMissingDecks, + tr_args!["count"=>self.deck_ids_missing], + )); + } + if self.field_count_mismatch > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckFieldCount, + tr_args!["count"=>self.field_count_mismatch], + )); + } + if self.card_ords_duplicated > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckDuplicateCardOrds, + tr_args!["count"=>self.card_ords_duplicated], + )); + } + if self.templates_missing > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckMissingTemplates, + tr_args!["count"=>self.templates_missing], + )); + } + if self.revlog_properties_invalid > 0 { + probs.push(i18n.trn( + TR::DatabaseCheckRevlogProperties, + tr_args!["count"=>self.revlog_properties_invalid], + )); + } + + probs + } +} + impl Collection { /// Check the database, returning a list of problems that were fixed. - pub(crate) fn check_database(&mut self) -> Result> { + pub(crate) fn check_database(&mut self) -> Result { debug!(self.log, "quick check"); if self.storage.quick_check_corrupt() { debug!(self.log, "quick check failed"); @@ -35,23 +104,23 @@ impl Collection { self.transact(None, |col| col.check_database_inner()) } - fn check_database_inner(&mut self) -> Result> { - let mut probs = vec![]; + fn check_database_inner(&mut self) -> Result { + let mut out = CheckDatabaseOutput::default(); // cards first, as we need to be able to read them to process notes debug!(self.log, "check cards"); - self.check_card_properties(&mut probs)?; - self.check_orphaned_cards(&mut probs)?; + self.check_card_properties(&mut out)?; + self.check_orphaned_cards(&mut out)?; debug!(self.log, "check decks"); - self.check_missing_deck_ids(&mut probs)?; - self.check_filtered_cards(&mut probs)?; + self.check_missing_deck_ids(&mut out)?; + self.check_filtered_cards(&mut out)?; debug!(self.log, "check notetypes"); - self.check_notetypes(&mut probs)?; + self.check_notetypes(&mut out)?; debug!(self.log, "check review log"); - self.check_revlog(&mut probs)?; + self.check_revlog(&mut out)?; debug!(self.log, "missing decks"); let names = self.storage.get_all_deck_names()?; @@ -59,63 +128,41 @@ impl Collection { self.update_next_new_position()?; - debug!(self.log, "db check finished with problems: {:#?}", probs); + debug!(self.log, "db check finished: {:#?}", out); - Ok(probs) + Ok(out) } - fn check_card_properties(&mut self, probs: &mut Vec) -> Result<()> { + fn check_card_properties(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { let timing = self.timing_today()?; let (new_cnt, other_cnt) = self.storage.fix_card_properties( timing.days_elapsed, TimestampSecs::now(), self.usn()?, )?; - if new_cnt > 0 { - probs.push( - self.i18n - .trn(TR::DatabaseCheckNewCardHighDue, tr_args!["count"=>new_cnt]), - ); - } - if other_cnt > 0 { - probs.push(self.i18n.trn( - TR::DatabaseCheckCardProperties, - tr_args!["count"=>other_cnt], - )); - } + out.card_position_too_high = new_cnt; + out.card_properties_invalid += other_cnt; Ok(()) } - fn check_orphaned_cards(&mut self, probs: &mut Vec) -> Result<()> { - let orphaned = self.storage.delete_orphaned_cards()?; - if orphaned > 0 { + fn check_orphaned_cards(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { + let cnt = self.storage.delete_orphaned_cards()?; + if cnt > 0 { self.storage.set_schema_modified()?; - probs.push(self.i18n.trn( - TR::DatabaseCheckCardMissingNote, - tr_args!["count"=>orphaned], - )); + out.cards_missing_note = cnt; } Ok(()) } - fn check_missing_deck_ids(&mut self, probs: &mut Vec) -> Result<()> { - let mut cnt = 0; + fn check_missing_deck_ids(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { for did in self.storage.missing_decks()? { self.recover_missing_deck(did)?; - cnt += 1; + out.deck_ids_missing += 1; } - - if cnt > 0 { - probs.push( - self.i18n - .trn(TR::DatabaseCheckMissingDecks, tr_args!["count"=>cnt]), - ); - } - Ok(()) } - fn check_filtered_cards(&mut self, probs: &mut Vec) -> Result<()> { + fn check_filtered_cards(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { let decks: HashMap<_, _> = self .storage .get_all_decks()? @@ -124,7 +171,6 @@ impl Collection { .collect(); let mut wrong = 0; - for (cid, did) in self.storage.all_filtered_cards_by_deck()? { // we expect calling code to ensure all decks already exist if let Some(deck) = decks.get(&did) { @@ -140,16 +186,13 @@ impl Collection { if wrong > 0 { self.storage.set_schema_modified()?; - probs.push( - self.i18n - .trn(TR::DatabaseCheckCardProperties, tr_args!["count"=>wrong]), - ); + out.card_properties_invalid += wrong; } Ok(()) } - fn check_notetypes(&mut self, probs: &mut Vec) -> Result<()> { + fn check_notetypes(&mut self, out: &mut CheckDatabaseOutput) -> Result<()> { let nids_by_notetype = self.storage.all_note_ids_by_notetype()?; let norm = self.normalize_note_text(); let usn = self.usn()?; @@ -158,10 +201,6 @@ impl Collection { // will rebuild tag list below self.storage.clear_tags()?; - let mut note_fields = 0; - let mut dupe_ords = 0; - let mut missing_template_ords = 0; - for (ntid, group) in &nids_by_notetype.into_iter().group_by(|tup| tup.0) { debug!(self.log, "check notetype: {}", ntid); let mut group = group.peekable(); @@ -178,14 +217,15 @@ impl Collection { let mut note = self.storage.get_note(nid)?.unwrap(); let cards = self.storage.existing_cards_for_note(nid)?; - dupe_ords += self.remove_duplicate_card_ordinals(&cards)?; - missing_template_ords += self.remove_cards_without_template(&nt, &cards)?; + + out.card_ords_duplicated += self.remove_duplicate_card_ordinals(&cards)?; + out.templates_missing += self.remove_cards_without_template(&nt, &cards)?; // fix fields if note.fields.len() != nt.fields.len() { note.fix_field_count(&nt); - note_fields += 1; note.tags.push("db-check".into()); + out.field_count_mismatch += 1; } // write note, updating tags and generating missing cards @@ -201,26 +241,9 @@ impl Collection { self.add_notetype_inner(&mut nt)?; } - if note_fields > 0 { + if out.card_ords_duplicated > 0 || out.field_count_mismatch > 0 || out.templates_missing > 0 + { self.storage.set_schema_modified()?; - probs.push( - self.i18n - .trn(TR::DatabaseCheckFieldCount, tr_args!["count"=>note_fields]), - ); - } - if dupe_ords > 0 { - self.storage.set_schema_modified()?; - probs.push(self.i18n.trn( - TR::DatabaseCheckDuplicateCardOrds, - tr_args!["count"=>dupe_ords], - )); - } - if missing_template_ords > 0 { - self.storage.set_schema_modified()?; - probs.push(self.i18n.trn( - TR::DatabaseCheckMissingTemplates, - tr_args!["count"=>missing_template_ords], - )); } Ok(()) @@ -278,15 +301,11 @@ impl Collection { Ok(Arc::new(basic)) } - fn check_revlog(&self, probs: &mut Vec) -> Result<()> { + fn check_revlog(&self, out: &mut CheckDatabaseOutput) -> Result<()> { let cnt = self.storage.fix_revlog_properties()?; - if cnt > 0 { self.storage.set_schema_modified()?; - probs.push( - self.i18n - .trn(TR::DatabaseCheckRevlogProperties, tr_args!["count"=>cnt]), - ); + out.revlog_properties_invalid = cnt; } Ok(()) @@ -297,3 +316,91 @@ impl Collection { self.set_next_card_position(pos) } } + +#[cfg(test)] +mod test { + use super::*; + use crate::{collection::open_test_collection, decks::DeckID, search::SortMode}; + + #[test] + fn cards() -> Result<()> { + let mut col = open_test_collection(); + let nt = col.get_notetype_by_name("Basic")?.unwrap(); + let mut note = nt.new_note(); + col.add_note(&mut note, DeckID(1))?; + + // card properties + col.storage + .db + .execute_batch("update cards set ivl=1.5,due=2000000,odue=1.5")?; + + let out = col.check_database()?; + assert_eq!( + out, + CheckDatabaseOutput { + card_properties_invalid: 2, + card_position_too_high: 1, + ..Default::default() + } + ); + // should be idempotent + assert_eq!(col.check_database()?, Default::default()); + + // missing deck + col.storage.db.execute_batch("update cards set did=123")?; + + let out = col.check_database()?; + assert_eq!( + out, + CheckDatabaseOutput { + deck_ids_missing: 1, + ..Default::default() + } + ); + assert_eq!( + col.storage.get_deck(DeckID(123))?.unwrap().name, + "recovered123" + ); + + // duplicate ordinals + let cid = col.search_cards("", SortMode::NoOrder)?[0]; + let mut card = col.storage.get_card(cid)?.unwrap(); + card.id.0 += 1; + col.storage.add_card(&mut card)?; + + let out = col.check_database()?; + assert_eq!( + out, + CheckDatabaseOutput { + card_ords_duplicated: 1, + ..Default::default() + } + ); + assert_eq!( + col.storage.db_scalar::("select count(*) from cards")?, + 1 + ); + + // missing templates + let cid = col.search_cards("", SortMode::NoOrder)?[0]; + let mut card = col.storage.get_card(cid)?.unwrap(); + card.id.0 += 1; + card.ord = 10; + col.storage.add_card(&mut card)?; + + let out = col.check_database()?; + assert_eq!( + out, + CheckDatabaseOutput { + templates_missing: 1, + ..Default::default() + } + ); + assert_eq!( + col.storage.db_scalar::("select count(*) from cards")?, + 1 + ); + + Ok(()) + } +} diff --git a/rslib/src/decks/mod.rs b/rslib/src/decks/mod.rs index a9eca38ef..b8d6f85ce 100644 --- a/rslib/src/decks/mod.rs +++ b/rslib/src/decks/mod.rs @@ -209,8 +209,28 @@ impl Collection { }) } - pub(crate) fn recover_missing_deck(&mut self, _did: DeckID) -> Result<()> { - // todo + fn ensure_deck_name_unique(&self, deck: &mut Deck) -> Result<()> { + loop { + match self.storage.get_deck_id(&deck.name)? { + Some(did) if did == deck.id => { + break; + } + None => break, + _ => (), + } + deck.name += "_"; + } + + Ok(()) + } + + pub(crate) fn recover_missing_deck(&mut self, did: DeckID) -> Result<()> { + let mut deck = Deck::new_normal(); + deck.id = did; + deck.name = format!("recovered{}", did); + self.ensure_deck_name_unique(&mut deck)?; + deck.prepare_for_update(); + self.storage.update_deck(&deck)?; Ok(()) } diff --git a/rslib/src/storage/revlog/fix_props.sql b/rslib/src/storage/revlog/fix_props.sql index ddd950dc7..a21def30b 100644 --- a/rslib/src/storage/revlog/fix_props.sql +++ b/rslib/src/storage/revlog/fix_props.sql @@ -4,4 +4,4 @@ set lastIvl = min(max(round(lastIvl), -2147483648), 2147483647) where ivl != min(max(round(ivl), -2147483648), 2147483647) - or lastIVl != min(max(round(lastIvl), -2147483648), 2147483647) \ No newline at end of file + or lastIvl != min(max(round(lastIvl), -2147483648), 2147483647) \ No newline at end of file