fixup: centralize backup error handling

nodejs · Jan 16, 2025 · 371f368 · 371f368
1 parent acec23e
commit 371f368
Show file tree

Hide file tree

Showing 3 changed files with 40 additions and 42 deletions.
diff --git a/doc/api/sqlite.md b/doc/api/sqlite.md
@@ -132,8 +132,8 @@ added: REPLACEME
     pages.
 * Returns: {Promise} A promise that resolves when the backup is completed and rejects if an error occurs.
 
-This method makes a database backup. This method abstracts the [`sqlite3_backup_init()`][], [`sqlite3_backup_step()`][] and
-[`sqlite3_backup_finish()`][] functions.
+This method makes a database backup. This method abstracts the [`sqlite3_backup_init()`][], [`sqlite3_backup_step()`][]
+and [`sqlite3_backup_finish()`][] functions.
 
 ### `database.close()`
 

diff --git a/src/node_sqlite.cc b/src/node_sqlite.cc
@@ -172,35 +172,18 @@ class BackupJob : public ThreadPoolWork {
   void ScheduleBackup() {
     Isolate* isolate = env()->isolate();
     HandleScope handle_scope(isolate);
-
     backup_status_ = sqlite3_open(destination_name_.c_str(), &dest_);
-
     Local<Promise::Resolver> resolver =
         Local<Promise::Resolver>::New(env()->isolate(), resolver_);
-
-    Local<Object> e = Local<Object>();
-
     if (backup_status_ != SQLITE_OK) {
-      if (!CreateSQLiteError(isolate, dest_).ToLocal(&e)) {
-        return;
-      }
-
-      Finalize();
-      resolver->Reject(env()->context(), e).ToChecked();
+      HandleBackupError(resolver);
       return;
     }
 
     backup_ = sqlite3_backup_init(
         dest_, dest_db_.c_str(), source_->Connection(), source_db_.c_str());
-
     if (backup_ == nullptr) {
-      if (!CreateSQLiteError(isolate, dest_).ToLocal(&e)) {
-        Finalize();
-        return;
-      }
-
-      Finalize();
-      resolver->Reject(env()->context(), e).ToChecked();
+      HandleBackupError(resolver);
       return;
     }
 
@@ -218,14 +201,7 @@ class BackupJob : public ThreadPoolWork {
 
     if (!(backup_status_ == SQLITE_OK || backup_status_ == SQLITE_DONE ||
           backup_status_ == SQLITE_BUSY || backup_status_ == SQLITE_LOCKED)) {
-      Local<Object> e;
-      if (!CreateSQLiteError(env()->isolate(), backup_status_).ToLocal(&e)) {
-        Finalize();
-        return;
-      }
-
-      Finalize();
-      resolver->Reject(env()->context(), e).ToChecked();
+      HandleBackupError(resolver, backup_status_);
       return;
     }
 
@@ -267,21 +243,21 @@ class BackupJob : public ThreadPoolWork {
       return;
     }
 
-    Local<Object> e;
-    if (!CreateSQLiteError(env()->isolate(), dest_).ToLocal(&e)) {
+    if (backup_status_ != SQLITE_OK && backup_status_ != SQLITE_DONE) {
+      Local<Object> e;
+      if (!CreateSQLiteError(env()->isolate(), dest_).ToLocal(&e)) {
+        Finalize();
+        return;
+      }
+
       Finalize();
+      resolver->Reject(env()->context(), e).ToChecked();
       return;
     }
 
-    Finalize();
-    if (backup_status_ == SQLITE_OK) {
-      resolver
-          ->Resolve(env()->context(),
-                    Integer::New(env()->isolate(), total_pages))
-          .ToChecked();
-    } else {
-      resolver->Reject(env()->context(), e).ToChecked();
-    }
+    resolver
+        ->Resolve(env()->context(), Integer::New(env()->isolate(), total_pages))
+        .ToChecked();
   }
 
   void Finalize() {
@@ -303,6 +279,28 @@ class BackupJob : public ThreadPoolWork {
   }
 
  private:
+  void HandleBackupError(Local<Promise::Resolver> resolver) {
+    Local<Object> e;
+    if (!CreateSQLiteError(env()->isolate(), dest_).ToLocal(&e)) {
+      Finalize();
+      return;
+    }
+
+    Finalize();
+    resolver->Reject(env()->context(), e).ToChecked();
+  }
+
+  void HandleBackupError(Local<Promise::Resolver> resolver, int errcode) {
+    Local<Object> e;
+    if (!CreateSQLiteError(env()->isolate(), errcode).ToLocal(&e)) {
+      Finalize();
+      return;
+    }
+
+    Finalize();
+    resolver->Reject(env()->context(), e).ToChecked();
+  }
+
   Environment* env() const { return env_; }
 
   Environment* env_;
@@ -805,7 +803,7 @@ void DatabaseSync::Backup(const FunctionCallbackInfo<Value>& args) {
       if (!target_v->IsString()) {
         THROW_ERR_INVALID_ARG_TYPE(
             env->isolate(),
-            "The \"options.targetDb\" argument must be a string.");
+            "The \"options.target\" argument must be a string.");
         return;
       }
 

diff --git a/test/parallel/test-sqlite-backup.mjs b/test/parallel/test-sqlite-backup.mjs
@@ -80,7 +80,7 @@ describe('DatabaseSync.prototype.backup()', () => {
       });
     }, {
       code: 'ERR_INVALID_ARG_TYPE',
-      message: 'The "options.targetDb" argument must be a string.'
+      message: 'The "options.target" argument must be a string.'
     });
 
     t.assert.throws(() => {