Closed Bug 386394 Opened 13 years ago Closed 13 years ago

Add a BackupDB method to mozIStorageConnection

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sdwilsh, Assigned: sdwilsh)

References

()

Details

Attachments

(1 file, 2 obsolete files)

Abstract away so that all consumers can enjoy it!

How does this sound:

/**
 * Copies the current database file to the specified parent directory with the
 * specifed file name.  If the parent directory is not specified, it places it in
 * the same directory as the current file.  This function ensures that the file
 * being created is unique.
 *
 * @param aFileName The name of the new file to create.
 * @param aParentDirectory The directory you'd like the file to be placed.
 */
 void BackupDB(in AString aFileName, [optional] in nsIFile aParentDirectory);
Attached patch v1.0 (obsolete) — Splinter Review
Mano, if you don't feel comfortable reviewing this, I can ask Seth to do it, but I figured you'd like to take a look since you proposed it :)
Attachment #270414 - Flags: review?(mano)
Comment on attachment 270414 [details] [diff] [review]
v1.0

>Index: storage/public/mozIStorageConnection.idl
>===================================================================
>+  /**
>+   * Copies the current database file to the specified parent directory with the
>+   * specified file name.  If the parent directory is not specified, it places
>+   * the backup in the same directory as the current file.  This function
>+   * ensures that the file being created is unqiue.

typo: unique.

>+   *
>+   * @param aFileName The name of the new file to create.
>+   * @param aParentDirectory The directory you'd like the file to be placed.
>+   * @return The location of the backup file.
>+   */
>+  nsIFile backupDB(in AString aFileName,
>+                   [optional] in nsIFile aParentDirectory);
>+

aParentDirectory should be marked as "@param [optional]" in the header.

>Index: storage/src/mozStorageConnection.cpp
>===================================================================

>+
>+NS_IMETHODIMP
>+mozStorageConnection::BackupDB(const nsAString &aFileName,
>+                               nsIFile *aParentDirectory,
>+                               nsIFile **backup)
>+{
>+    if (!mDatabaseFile)
>+        return NS_ERROR_NOT_INITIALIZED;
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIFile> parentDir = aParentDirectory;
>+    if (!parentDir) {
>+        // This argument is optional, and defaults to the same parent directory
>+        // as the current file.
>+        rv = mDatabaseFile->GetParent(getter_AddRefs(parentDir));
>+        NS_ENSURE_SUCCESS(rv, rv);
>+    }
>+
>+    nsCOMPtr<nsIFile> backupDB;
>+    rv = parentDir->Clone(getter_AddRefs(backupDB));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = backupDB->Append(aFileName);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = backupDB->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsString fileName;

nsAutoString

>+    rv = backupDB->GetLeafName(fileName);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    rv = backupDB->Remove(PR_FALSE);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    NS_ADDREF(*backup = backupDB);

prefer nsCOMPtr's swap.

>Index: storage/test/unit/test_storage_connection.js
>===================================================================

>+function test_backup_not_new_filename()
>+{
>+  try { backup.remove(false); } catch (e) { /* windows :( */ }

hrm? backup is declared later so this would throw XP... what's the windows issue? why do you need to do this anyway?

>+  var msc = getOpenedDatabase();
>+  const fname = getTestDB().leafName;
>+
>+  var backup = msc.backupDB(fname);
>+  do_check_neq(fname, backup.leafName);
>+
>+  try { backup.remove(false); } catch (e) { /* windows :( */ }
>+}
>+

>+function test_backup_new_filename()
>+{
>+  try { backup.remove(false); } catch (e) { /* windows :( */ }

ditto


>+{
>+  try { parentDir.remove(true); } catch (e) { /* windows :( */ }
>+  var msc = getOpenedDatabase();
>+  var parentDir = getTestDB().parent;

and ditto.
Attachment #270414 - Flags: review?(mano) → review-
Attached patch v1.1 (obsolete) — Splinter Review
yay patches on a plane!
Attachment #270414 - Attachment is obsolete: true
Attachment #270555 - Flags: review?(mano)
Blocks: 385871
Attached patch v1.2Splinter Review
Addresses more comments
Attachment #270555 - Attachment is obsolete: true
Attachment #270770 - Flags: review?(mano)
Attachment #270555 - Flags: review?(mano)
Comment on attachment 270770 [details] [diff] [review]
v1.2

>Index: storage/public/mozIStorageConnection.idl
>===================================================================

>+  /**
>+   * Copies the current database file to the specified parent directory with the
>+   * specified file name.  If the parent directory is not specified, it places
>+   * the backup in the same directory as the current file.  This function
>+   * ensures that the file being created is unique.
>+   *
>+   * @param aFileName
>+   *   The name of the new file to create.
>+   * @param [optional] aParentDirectory
>+   *   The directory you'd like the file to be placed.
>+   * @return
>+   *   The location of the backup file.
>+   */

fix the indent here... @return should be on the same line with "the location..." (see places IDLs for example).

Btw, this returns the file object, not its location (which would be its path).

r=mano otherwise.
Attachment #270770 - Flags: review?(mano) → review+
Checking in storage/public/mozIStorageConnection.idl;
new revision: 1.10; previous revision: 1.9
Checking in storage/src/mozStorageConnection.cpp;
new revision: 1.23; previous revision: 1.22
Checking in storage/test/unit/test_storage_connection.js;
new revision: 1.3; previous revision: 1.2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 386768
shawn, quick question, can't someone call BackupDB() at any time on a connection?

If so, what is the guarantee about the backup file that is made?

If a .sqlite file is always valid on disk, then I guess we're ok, but what about journaled writes?

see bug #408751 for how this came up.
(In reply to comment #7)
> shawn, quick question, can't someone call BackupDB() at any time on a
> connection?
Sorry - I must have missed this comment.  Yes, they can.

> If so, what is the guarantee about the backup file that is made?
It's going to be an exact copy of the database file, so if you call this while you have an open transaction, anything you've done within that transaction will not be in the new database.

> If a .sqlite file is always valid on disk, then I guess we're ok, but what
> about journaled writes?
Unless we are doing async writes (which we are not currently doing), no call will return until it's been fully flushed to disk.
You need to log in before you can comment on or make changes to this bug.