ALTER DB: never damage an existing target database (#2800)

basex-core/src/main/java/org/basex/core/cmd/AlterDB.java

@@ -48,6 +48,9 @@ protected boolean run() {
     if(!closed) closed = close(context, src);
     // check if source database is still opened
     if(context.pinned(src)) return error(DB_PINNED_X, src);
+    // an existing target database will be replaced: close it, check if it is still opened
+    close(context, trg);
+    if(context.pinned(trg)) return error(DB_PINNED_X, trg);
 
     // try to alter database
     return alter(src, trg, soptions) && (!closed || new Open(trg).run(context)) ?
@@ -60,7 +63,8 @@ public void addLocks() {
   }
 
   /**
-   * Renames the specified database.
+   * Renames the specified database. An existing target database is replaced.
+   * If renaming fails, both databases are left unchanged.
    * @param source name of the existing database
    * @param target new database name
    * @param sopts static options
@@ -69,16 +73,38 @@ public void addLocks() {
   public static synchronized boolean alter(final String source, final String target,
       final StaticOptions sopts) {
 
-    // drop target database (unless it is the source, e.g. if only the case differs)
     final IOFile src = sopts.dbPath(source), trg = sopts.dbPath(target);
-    if(!src.sameFile(trg)) DropDB.drop(target, sopts);
+    // move an existing target database aside (unless it is the source, e.g. if only the case
+    // differs). On Windows, this fails if a file of the target is opened by another process.
+    IOFile old = null;
+    if(trg.exists() && !src.sameFile(trg)) {
+      for(int c = 0; old == null || old.exists(); c++) {
+        old = new IOFile(sopts.dbPath(), '.' + target + '.' + c);
+      }
+      try {
+        trg.moveTo(old, false);
+      } catch(final IOException ex) {
+        Util.debug(ex);
+        return false;
+      }
+    }
     try {
       src.moveTo(trg, false);
-      return true;
     } catch(final IOException ex) {
       Util.debug(ex);
+      // restore the target database
+      if(old != null) {
+        try {
+          old.moveTo(trg, false);
+        } catch(final IOException ex2) {
+          Util.debug(ex2);
+        }
+      }
       return false;
     }
+    // delete the replaced database
+    if(old != null && !old.delete()) Util.debugln("Could not delete %", old);
+    return true;
   }
 
   @Override

basex-core/src/test/java/org/basex/core/AlterDBTest.java

@@ -0,0 +1,102 @@
+package org.basex.core;
+
+import static org.basex.query.QueryError.*;
+import static org.basex.query.func.Function.*;
+import static org.junit.jupiter.api.Assertions.*;
+
+import java.io.*;
+
+import org.basex.*;
+import org.basex.api.client.*;
+import org.basex.core.cmd.*;
+import org.basex.data.*;
+import org.basex.io.*;
+import org.basex.util.*;
+import org.junit.jupiter.api.*;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for renaming a database to the name of an existing database.
+ *
+ * @author BaseX Team, BSD License
+ * @author WingedGuardian
+ */
+public final class AlterDBTest extends SandboxTest {
+  /** Name of the target database. */
+  private static final String NAME2 = NAME + '2';
+
+  /** Creates the source and the target database. */
+  @BeforeEach public void init() {
+    execute(new CreateDB(NAME, "<a/>"));
+    execute(new CreateDB(NAME2, "<b/>"));
+    execute(new Close());
+  }
+
+  /** Drops the test databases. */
+  @AfterEach public void finish() {
+    execute(new DropDB(NAME));
+    execute(new DropDB(NAME2));
+  }
+
+  /** An existing target database is replaced, and no temporary files are left behind. */
+  @Test public void replace() {
+    execute(new AlterDB(NAME, NAME2));
+    assertFalse(context.soptions.dbExists(NAME));
+    query(_DB_GET.args(NAME2) + "/*/name()", "a");
+    for(final IOFile file : context.soptions.dbPath().children()) {
+      assertFalse(file.name().startsWith('.' + NAME2), file.name());
+    }
+  }
+
+  /**
+   * A target database that is opened by another client is not replaced.
+   * @throws IOException I/O exception
+   */
+  @Test public void targetOpened() throws IOException {
+    try(LocalSession session = new LocalSession(context)) {
+      session.execute(new Open(NAME2));
+      final BaseXException ex = assertThrows(BaseXException.class,
+          () -> new AlterDB(NAME, NAME2).execute(context));
+      assertTrue(ex.getMessage().contains(NAME2), ex.getMessage());
+      error(_DB_ALTER.args(NAME, NAME2), DB_LOCK1_X);
+    }
+    query(_DB_GET.args(NAME) + "/*/name()", "a");
+    query(_DB_GET.args(NAME2) + "/*/name()", "b");
+  }
+
+  /**
+   * A target database with a file that is opened by another process is left intact.
+   * Windows only: on other systems, open files do not prevent renaming or deleting a directory.
+   * @throws IOException I/O exception
+   */
+  @Test public void targetFileOpened() throws IOException {
+    Assumptions.assumeTrue(Prop.WIN, "Windows only");
+    final IOFile file = new IOFile(context.soptions.dbPath(NAME2),
+        DataText.DATAINF + IO.BASEXSUFFIX);
+    try(RandomAccessFile raf = new RandomAccessFile(file.file(), "r")) {
+      assertThrows(BaseXException.class, () -> new AlterDB(NAME, NAME2).execute(context));
+      error(_DB_ALTER.args(NAME, NAME2), UPDBERROR_X_X);
+    }
+    query(_DB_GET.args(NAME) + "/*/name()", "a");
+    query(_DB_GET.args(NAME2) + "/*/name()", "b");
+    // the operation succeeds after the file has been closed
+    execute(new AlterDB(NAME, NAME2));
+    query(_DB_GET.args(NAME2) + "/*/name()", "a");
+  }
+
+  /**
+   * A target database is restored if the source database cannot be renamed.
+   * Windows only: on other systems, open files do not prevent renaming a directory.
+   * @throws IOException I/O exception
+   */
+  @Test public void sourceFileOpened() throws IOException {
+    Assumptions.assumeTrue(Prop.WIN, "Windows only");
+    final IOFile file = new IOFile(context.soptions.dbPath(NAME),
+        DataText.DATAINF + IO.BASEXSUFFIX);
+    try(RandomAccessFile raf = new RandomAccessFile(file.file(), "r")) {
+      assertThrows(BaseXException.class, () -> new AlterDB(NAME, NAME2).execute(context));
+    }
+    query(_DB_GET.args(NAME) + "/*/name()", "a");
+    query(_DB_GET.args(NAME2) + "/*/name()", "b");
+  }
+}