From 5c73fbcba5d06488ab86007ed366226bb2647170 Mon Sep 17 00:00:00 2001 From: Michael James Gratton Date: Fri, 27 Apr 2018 23:08:51 +1000 Subject: [PATCH] Push all ImapDB path management down into to ImapDb.Account. This stops generating account storage paths, and in particular the location of attachment files, in a number of different places. Instead, we determine the paths once, in ImapDb.Account and pass them around as needed. This will help making the ImapDB classes unit-testable, and in particular ImapDb.Folder by passing an instance of Db.Database, rather than ImapDb.Database. --- src/engine/imap-db/imap-db-account.vala | 102 +++++++++++------- src/engine/imap-db/imap-db-attachment.vala | 8 +- src/engine/imap-db/imap-db-database.vala | 40 ++++--- src/engine/imap-db/imap-db-folder.vala | 88 ++++++++++----- src/engine/imap-db/imap-db-gc.vala | 16 +-- .../engine/imap-db/imap-db-database-test.vala | 3 +- 6 files changed, 163 insertions(+), 94 deletions(-) diff --git a/src/engine/imap-db/imap-db-account.vala b/src/engine/imap-db/imap-db-account.vala index a91acd01..b142dab5 100644 --- a/src/engine/imap-db/imap-db-account.vala +++ b/src/engine/imap-db/imap-db-account.vala @@ -38,6 +38,20 @@ private class Geary.ImapDB.Account : BaseObject { private const string SEARCH_OP_VALUE_STARRED = "starred"; private const string SEARCH_OP_VALUE_UNREAD = "unread"; + // Storage path names + private const string DB_FILENAME = "geary.db"; + private const string ATTACHMENTS_DIR = "attachments"; + + /** + * Returns the on-disk paths used for storage by this account. + */ + public static void get_imap_db_storage_locations(File user_data_dir, out File db_file, + out File attachments_dir) { + db_file = user_data_dir.get_child(DB_FILENAME); + attachments_dir = user_data_dir.get_child(ATTACHMENTS_DIR); + } + + private class FolderReference : Geary.SmartReference { public Geary.FolderPath path; @@ -248,21 +262,27 @@ private class Geary.ImapDB.Account : BaseObject { return query; } - - public static void get_imap_db_storage_locations(File user_data_dir, out File db_file, - out File attachments_dir) { - db_file = ImapDB.Database.get_db_file(user_data_dir); - attachments_dir = ImapDB.Attachment.get_attachments_dir(user_data_dir); - } - + public async void open_async(File user_data_dir, File schema_dir, Cancellable? cancellable) throws Error { - if (db != null) + if (this.db != null) throw new EngineError.ALREADY_OPEN("IMAP database already open"); - - db = new ImapDB.Database(user_data_dir, schema_dir, upgrade_monitor, vacuum_monitor, - account_information.primary_mailbox.address); - + + File db_file; + File attachments_dir; + Account.get_imap_db_storage_locations( + user_data_dir, out db_file, out attachments_dir + ); + + this.db = new ImapDB.Database( + db_file, + schema_dir, + attachments_dir, + upgrade_monitor, + vacuum_monitor, + account_information.primary_mailbox.address + ); + try { yield db.open( Db.DatabaseFlags.CREATE_DIRECTORY | Db.DatabaseFlags.CREATE_FILE | Db.DatabaseFlags.CHECK_CORRUPTION, @@ -650,28 +670,30 @@ private class Geary.ImapDB.Account : BaseObject { // return current if already created ImapDB.Folder? folder = get_local_folder(path); if (folder != null) { - // update properties folder.set_properties(properties); - - return folder; + } else { + folder = new Geary.ImapDB.Folder( + db, + path, + db.attachments_path, + contact_store, + account_information.primary_mailbox.address, + folder_id, + properties + ); + + // build a reference to it + FolderReference folder_ref = new FolderReference(folder, path); + folder_ref.reference_broken.connect(on_folder_reference_broken); + + // add to the references table + folder_refs.set(folder_ref.path, folder_ref); + + folder.unread_updated.connect(on_unread_updated); } - - // create folder - folder = new Geary.ImapDB.Folder(db, path, contact_store, account_information.primary_mailbox.address, folder_id, - properties); - - // build a reference to it - FolderReference folder_ref = new FolderReference(folder, path); - folder_ref.reference_broken.connect(on_folder_reference_broken); - - // add to the references table - folder_refs.set(folder_ref.path, folder_ref); - - folder.unread_updated.connect(on_unread_updated); - return folder; } - + private void on_folder_reference_broken(Geary.SmartReference reference) { FolderReference folder_ref = (FolderReference) reference; @@ -706,8 +728,10 @@ private class Geary.ImapDB.Account : BaseObject { // Ignore any messages that don't have the required fields. if (partial_ok || row.fields.fulfills(requested_fields)) { Geary.Email email = row.to_email(new Geary.ImapDB.EmailIdentifier(id, null)); - Geary.ImapDB.Folder.do_add_attachments(cx, email, id, cancellable); - + Geary.ImapDB.Folder.do_add_attachments( + cx, this.db.attachments_path, email, id, cancellable + ); + Gee.Set? folders = do_find_email_folders(cx, id, true, cancellable); if (folders == null) { if (folder_blacklist == null || !folder_blacklist.contains(null)) @@ -1367,10 +1391,12 @@ private class Geary.ImapDB.Account : BaseObject { throw new EngineError.INCOMPLETE_MESSAGE( "Message %s only fulfills %Xh fields (required: %Xh)", email_id.to_string(), row.fields, required_fields); - + email = row.to_email(email_id); - Geary.ImapDB.Folder.do_add_attachments(cx, email, email_id.message_id, cancellable); - + Geary.ImapDB.Folder.do_add_attachments( + cx, this.db.attachments_path, email, email_id.message_id, cancellable + ); + return Db.TransactionOutcome.DONE; }, cancellable); @@ -1530,8 +1556,10 @@ private class Geary.ImapDB.Account : BaseObject { MessageRow row = Geary.ImapDB.Folder.do_fetch_message_row( cx, message_id, search_fields, out db_fields, cancellable); Geary.Email email = row.to_email(new Geary.ImapDB.EmailIdentifier(message_id, null)); - Geary.ImapDB.Folder.do_add_attachments(cx, email, message_id, cancellable); - + Geary.ImapDB.Folder.do_add_attachments( + cx, this.db.attachments_path, email, message_id, cancellable + ); + Geary.ImapDB.Folder.do_add_email_to_search_table(cx, message_id, email, cancellable); } catch (Error e) { // This is a somewhat serious issue since we rely on diff --git a/src/engine/imap-db/imap-db-attachment.vala b/src/engine/imap-db/imap-db-attachment.vala index 74fde3fd..c7752d1c 100644 --- a/src/engine/imap-db/imap-db-attachment.vala +++ b/src/engine/imap-db/imap-db-attachment.vala @@ -8,7 +8,6 @@ private class Geary.ImapDB.Attachment : Geary.Attachment { public const Email.Field REQUIRED_FIELDS = Email.REQUIRED_FOR_MESSAGE; internal const string NULL_FILE_NAME = "none"; - private const string ATTACHMENTS_DIR = "attachments"; public Attachment(int64 message_id, int64 attachment_id, @@ -33,15 +32,12 @@ private class Geary.ImapDB.Attachment : Geary.Attachment { return "imap-db:%s".printf(attachment_id.to_string()); } - public static File generate_file(File data_dir, int64 message_id, int64 attachment_id, + public static File generate_file(File attachements_dir, int64 message_id, int64 attachment_id, string? filename) { - return get_attachments_dir(data_dir) + return attachements_dir .get_child(message_id.to_string()) .get_child(attachment_id.to_string()) .get_child(filename ?? NULL_FILE_NAME); } - public static File get_attachments_dir(File data_dir) { - return data_dir.get_child(ATTACHMENTS_DIR); - } } diff --git a/src/engine/imap-db/imap-db-database.vala b/src/engine/imap-db/imap-db-database.vala index d7686d2a..bced6634 100644 --- a/src/engine/imap-db/imap-db-database.vala +++ b/src/engine/imap-db/imap-db-database.vala @@ -8,32 +8,32 @@ extern int sqlite3_unicodesn_register_tokenizer(Sqlite.Database db); private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { - private const string DB_FILENAME = "geary.db"; + + internal GLib.File attachments_path; + private const int OPEN_PUMP_EVENT_LOOP_MSEC = 100; - + private ProgressMonitor upgrade_monitor; private ProgressMonitor vacuum_monitor; private string account_owner_email; private bool new_db = false; private Cancellable gc_cancellable = new Cancellable(); - public Database(GLib.File db_dir, + public Database(GLib.File db_file, GLib.File schema_dir, + GLib.File attachments_path, ProgressMonitor upgrade_monitor, ProgressMonitor vacuum_monitor, string account_owner_email) { - base.persistent(get_db_file(db_dir), schema_dir); + base.persistent(db_file, schema_dir); + this.attachments_path = attachments_path; this.upgrade_monitor = upgrade_monitor; this.vacuum_monitor = vacuum_monitor; // Update to use all addresses on the account. Bug 768779 this.account_owner_email = account_owner_email; } - - public static File get_db_file(File db_dir) { - return db_dir.get_child(DB_FILENAME); - } - + /** * Prepares the ImapDB database for use. */ @@ -336,8 +336,13 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { Mime.DispositionType target_disposition = Mime.DispositionType.UNSPECIFIED; if (message.get_sub_messages().is_empty) target_disposition = Mime.DispositionType.INLINE; - Geary.ImapDB.Folder.do_save_attachments_db(cx, id, - message.get_attachments(target_disposition), this, null); + Geary.ImapDB.Folder.do_save_attachments_db( + cx, + id, + message.get_attachments(target_disposition), + this.attachments_path, + null + ); } catch (Error e) { debug("Error fetching inline Mime parts: %s", e.message); } @@ -495,7 +500,9 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { message.get_attachments(); try { - Geary.ImapDB.Folder.do_delete_attachments(cx, message_id); + Geary.ImapDB.Folder.do_delete_attachments( + cx, this.attachments_path, message_id + ); } catch (Error err) { debug("Error deleting existing attachments: %s", err.message); @@ -504,8 +511,13 @@ private class Geary.ImapDB.Database : Geary.Db.VersionedDatabase { // rebuild all try { - Geary.ImapDB.Folder.do_save_attachments_db(cx, message_id, msg_attachments, - this, null); + Geary.ImapDB.Folder.do_save_attachments_db( + cx, + message_id, + msg_attachments, + this.attachments_path, + null + ); } catch (Error err) { debug("Error saving attachments: %s", err.message); diff --git a/src/engine/imap-db/imap-db-folder.vala b/src/engine/imap-db/imap-db-folder.vala index 0d4dcd82..f7aa2643 100644 --- a/src/engine/imap-db/imap-db-folder.vala +++ b/src/engine/imap-db/imap-db-folder.vala @@ -82,9 +82,10 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } protected int manual_ref_count { get; protected set; } - - private ImapDB.Database db; + + private Geary.Db.Database db; private Geary.FolderPath path; + private GLib.File attachments_path; private ContactStore contact_store; private string account_owner_email; private int64 folder_id; @@ -102,14 +103,16 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { */ public signal void unread_updated(Gee.Map unread_status); - internal Folder(ImapDB.Database db, + internal Folder(Geary.Db.Database db, Geary.FolderPath path, + GLib.File attachments_path, ContactStore contact_store, string account_owner_email, int64 folder_id, Geary.Imap.FolderProperties properties) { this.db = db; this.path = path; + this.attachments_path = attachments_path; this.contact_store = contact_store; // Update to use all addresses on the account. Bug 768779 this.account_owner_email = account_owner_email; @@ -1593,16 +1596,23 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } Geary.Email email = row.to_email(location.email_id); - - return do_add_attachments(cx, email, location.message_id, cancellable); + + return do_add_attachments( + cx, this.attachments_path, email, location.message_id, cancellable + ); } - - internal static Geary.Email do_add_attachments(Db.Connection cx, Geary.Email email, - int64 message_id, Cancellable? cancellable = null) throws Error { + + internal static Geary.Email do_add_attachments(Db.Connection cx, + GLib.File attachments_path, + Geary.Email email, + int64 message_id, + Cancellable? cancellable = null) + throws Error { // Add attachments if available if (email.fields.fulfills(ImapDB.Attachment.REQUIRED_FIELDS)) { - Gee.List? attachments = do_list_attachments(cx, message_id, - cancellable); + Gee.List? attachments = do_list_attachments( + cx, attachments_path, message_id, cancellable + ); if (attachments != null) email.add_attachments(attachments); } @@ -2038,11 +2048,17 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { do_save_attachments(cx, location.message_id, combined_email.get_message().get_attachments(), cancellable); } - + // Must add attachments to the email object after they're saved to // the database. - do_add_attachments(cx, combined_email, location.message_id, cancellable); - + do_add_attachments( + cx, + this.attachments_path, + combined_email, + location.message_id, + cancellable + ); + Geary.Email.Field new_fields; do_merge_message_row(cx, row, out new_fields, out updated_contacts, ref new_unread_count, cancellable); @@ -2061,9 +2077,13 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { unread_count_change += new_unread_count; } - - private static Gee.List? do_list_attachments(Db.Connection cx, int64 message_id, - Cancellable? cancellable) throws Error { + + private static Gee.List? + do_list_attachments(Db.Connection cx, + GLib.File attachments_path, + int64 message_id, + Cancellable? cancellable) + throws Error { Db.Statement stmt = cx.prepare(""" SELECT id, filename, mime_type, filesize, disposition, content_id, description FROM MessageAttachmentTable @@ -2097,7 +2117,7 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { results.string_at(6), disposition, content_filename, - cx.database.file.get_parent(), + attachments_path, results.int64_at(3) ) ); @@ -2108,11 +2128,17 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { private void do_save_attachments(Db.Connection cx, int64 message_id, Gee.List? attachments, Cancellable? cancellable) throws Error { - do_save_attachments_db(cx, message_id, attachments, db, cancellable); + do_save_attachments_db( + cx, message_id, attachments, this.attachments_path, cancellable + ); } - - public static void do_save_attachments_db(Db.Connection cx, int64 message_id, - Gee.List? attachments, ImapDB.Database db, Cancellable? cancellable) throws Error { + + public static void do_save_attachments_db(Db.Connection cx, + int64 message_id, + Gee.List? attachments, + GLib.File attachments_path, + Cancellable? cancellable) + throws Error { // nothing to do if no attachments if (attachments == null || attachments.size == 0) return; @@ -2156,11 +2182,11 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { stmt.bind_int(4, disposition_type); stmt.bind_string(5, content_id); stmt.bind_string(6, description); - - int64 attachment_id = stmt.exec_insert(cancellable); - File saved_file = ImapDB.Attachment.generate_file(db.file.get_parent(), message_id, - attachment_id, filename); + int64 attachment_id = stmt.exec_insert(cancellable); + File saved_file = ImapDB.Attachment.generate_file( + attachments_path, message_id, attachment_id, filename + ); // On the off-chance this is marked for deletion, unmark it try { @@ -2231,13 +2257,17 @@ private class Geary.ImapDB.Folder : BaseObject, Geary.ReferenceSemantics { } } } - - public static void do_delete_attachments(Db.Connection cx, int64 message_id) + + public static void do_delete_attachments(Db.Connection cx, + GLib.File attachments_path, + int64 message_id) throws Error { - Gee.List? attachments = do_list_attachments(cx, message_id, null); + Gee.List? attachments = do_list_attachments( + cx, attachments_path, message_id, null + ); if (attachments == null || attachments.size == 0) return; - + // delete all files foreach (Geary.Attachment attachment in attachments) { try { diff --git a/src/engine/imap-db/imap-db-gc.vala b/src/engine/imap-db/imap-db-gc.vala index b567cb4e..36c7870b 100644 --- a/src/engine/imap-db/imap-db-gc.vala +++ b/src/engine/imap-db/imap-db-gc.vala @@ -83,12 +83,10 @@ private class Geary.ImapDB.GC { private ImapDB.Database db; private int priority; - private File data_dir; - + public GC(ImapDB.Database db, int priority) { this.db = db; this.priority = priority; - data_dir = db.file.get_parent(); } /** @@ -432,10 +430,14 @@ private class Geary.ImapDB.GC { result = stmt.exec(cancellable); while (!result.finished) { - File file = Attachment.generate_file(data_dir, message_id, result.rowid_for("id"), - result.string_for("filename")); + File file = Attachment.generate_file( + this.db.attachments_path, + message_id, + result.rowid_for("id"), + result.string_for("filename") + ); attachment_files.add(file); - + result.next(cancellable); } @@ -572,7 +574,7 @@ private class Geary.ImapDB.GC { private async int delete_empty_attachment_directories_async(File? current, out bool empty, Cancellable? cancellable) throws Error { - File current_dir = current ?? Attachment.get_attachments_dir(db.file.get_parent()); + File current_dir = current ?? db.attachments_path; // directory is considered empty until file or non-deleted child directory is found empty = true; diff --git a/test/engine/imap-db/imap-db-database-test.vala b/test/engine/imap-db/imap-db-database-test.vala index 209694ae..21c61ada 100644 --- a/test/engine/imap-db/imap-db-database-test.vala +++ b/test/engine/imap-db/imap-db-database-test.vala @@ -20,8 +20,9 @@ class Geary.ImapDB.DatabaseTest : TestCase { ); Database db = new Database( - tmp_dir, + tmp_dir.get_child("test.db"), GLib.File.new_for_path(_SOURCE_ROOT_DIR).get_child("sql"), + tmp_dir.get_child("attachments"), new Geary.SimpleProgressMonitor(Geary.ProgressType.DB_UPGRADE), new Geary.SimpleProgressMonitor(Geary.ProgressType.DB_VACUUM), "test@example.com"