386394 - Add a BackupDB method to mozIStorageConnection

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

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);

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

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 :)

Comment 2

[Asaf Romano (gone)]

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.

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

yay patches on a plane!

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

Addresses more comments

Comment 5

[Asaf Romano (gone)]

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.

Comment 6

[Shawn Wilsher :sdwilsh]

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

Comment 7

[(not reading, please use [e-mail] instead)]

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.

Comment 8

[Shawn Wilsher :sdwilsh]

(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.