Throw exception on duplicate Database registration (#3838) (#3847)

* Throw exception on duplicate Database registration (#3838)

DatabaseFactory.create() with register(true) previously returned the
existing Database instance when another instance was already registered
under the same name (added in #3759). This silently caused two
independently created Database instances (e.g. with different
DataSourceConfig.url values) to collide/share state, since the
second "instance" was actually the first one in disguise.

Change this to throw an IllegalStateException instead, forcing
callers to either use a unique DatabaseConfig name or setRegister(false)
when the Database is not intended to be registered/looked up by name.

* Also handle deregistration on shutdown

---------

Co-authored-by: robin.bygrave <robin.bygrave@eroad.com>

ebean-api/src/main/java/io/ebean/DatabaseBuilder.java

@@ -62,9 +62,10 @@ public interface DatabaseBuilder {
   /**
    * Build and return the Database instance.
    * <p>
-   * When {@link #setRegister(boolean)} is set to true, and a database with the same
-   * name is already registered, this may return the existing registered database
-   * rather than creating a new one.
+   * When {@link #setRegister(boolean)} is set to true (the default), and a database
+   * with the same name is already registered, this throws an {@link IllegalStateException}.
+   * Use a unique name, or use {@link #setRegister(boolean)} with {@code false} if the
+   * Database instance is not intended to be registered/looked up by name.
    */
   Database build();
 

ebean-api/src/main/java/io/ebean/DatabaseFactory.java

@@ -7,8 +7,6 @@ import jakarta.persistence.PersistenceException;
 
 import java.util.concurrent.locks.ReentrantLock;
 
-import static java.lang.System.Logger.Level.WARNING;
-
 /**
  * Low-level factory for creating {@link Database} instances.
  * <p>
@@ -81,10 +79,10 @@ public final class DatabaseFactory {
         // We're explicitly creating a database to be registered, so avoid
         // triggering DbContext static initialisation to auto-create a default one.
         DbPrimary.setSkip(true);
-        Database existing = DbContext.getInstance().getRegistered(name);
-        if (existing != null) {
-          EbeanVersion.log.log(WARNING, "Using existing database with name:{0}", name);
-          return existing;
+        if (DbContext.getInstance().contains(name)) {
+          throw new IllegalStateException("A Database with name [" + name + "] is already registered."
+            + " Use a unique DatabaseConfig name, or set DatabaseConfig.setRegister(false)"
+            + " if this Database instance is not intended to be registered/looked up by name.");
         }
       }
       Database server = createInternal(config);
@@ -123,6 +121,24 @@ public final class DatabaseFactory {
     }
   }
 
+  /**
+   * Remove the registration of this Database.
+   * <p>
+   * This is invoked when a Database is shutdown so that its registered name
+   * becomes available again for a subsequently created Database with the same name.
+   */
+  public static void unregister(Database server) {
+    lock.lock();
+    try {
+      DbContext.getInstance().deregister(server);
+      if (server.name().equals(defaultServerName)) {
+        defaultServerName = null;
+      }
+    } finally {
+      lock.unlock();
+    }
+  }
+
   /**
    * Shutdown gracefully all Database instances cleaning up any resources as required.
    * <p>

ebean-api/src/main/java/io/ebean/DbContext.java

@@ -4,7 +4,6 @@ import io.ebean.config.BeanNotEnhancedException;
 import io.ebean.datasource.DataSourceConfigurationException;
 
 import jakarta.persistence.PersistenceException;
-import org.jspecify.annotations.Nullable;
 
 import java.util.HashMap;
 import java.util.concurrent.ConcurrentHashMap;
@@ -77,9 +76,8 @@ final class DbContext {
     return defaultDatabase;
   }
 
-  @Nullable
-  Database getRegistered(String name) {
-    return concMap.get(name);
+  boolean contains(String name) {
+    return concMap.containsKey(name);
   }
 
   /**
@@ -122,6 +120,27 @@ final class DbContext {
     registerWithName(server.name(), server, isDefault);
   }
 
+  /**
+   * Remove the registration for this Database (typically on shutdown) so that
+   * its name becomes available again for a subsequently created Database.
+   * <p>
+   * Only removes the registration if it currently maps to this exact instance
+   * (avoids removing a different Database subsequently registered with the same name).
+   */
+  void deregister(Database server) {
+    lock.lock();
+    try {
+      String name = server.name();
+      concMap.remove(name, server);
+      syncMap.remove(name, server);
+      if (defaultDatabase == server) {
+        defaultDatabase = null;
+      }
+    } finally {
+      lock.unlock();
+    }
+  }
+
   private void registerWithName(String name, Database server, boolean isDefault) {
     lock.lock();
     try {

ebean-api/src/main/java/io/ebean/event/ShutdownManager.java

@@ -1,6 +1,7 @@
 package io.ebean.event;
 
 import io.ebean.Database;
+import io.ebean.DatabaseFactory;
 import io.ebean.EbeanVersion;
 import io.ebean.service.SpiContainer;
 
@@ -188,6 +189,7 @@ public final class ShutdownManager {
    */
   public static void unregisterDatabase(Database server) {
     databases.remove(server);
+    DatabaseFactory.unregister(server);
   }
 
   private static class ShutdownHook extends Thread {

ebean-redis/src/test/java/org/integration/ClusterTest.java

@@ -5,17 +5,23 @@ import io.ebean.Database;
 import io.ebean.redis.DuelCache;
 import org.domain.Person;
 import org.domain.query.QPerson;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
-import javax.sql.DataSource;
-
 import static org.assertj.core.api.Assertions.assertThat;
 
 class ClusterTest {
 
-  private Database createOther(DataSource dataSource) {
-    return Database.builder()
-      .dataSource(dataSource)
+  private static Database db;
+  private static Database other;
+
+  @BeforeAll
+  static void setup() {
+    // ensure the default server exists first
+    db = DB.getDefault();
+    other = Database.builder()
+      .dataSource(db.pluginApi().dataSource())
       .loadFromProperties()
       .defaultDatabase(false)
       .name("other")
@@ -24,12 +30,13 @@ class ClusterTest {
       .build();
   }
 
+  @AfterAll
+  static void tearDown() {
+    other.shutdown(false, false);
+  }
+
   @Test
   void testBothNear() throws InterruptedException {
-    // ensure the default server exists first
-    final Database db = DB.getDefault();
-    Database other = createOther(db.pluginApi().dataSource());
-
     new QPerson()
       .name.eq("Someone")
       .delete();
@@ -59,10 +66,6 @@ class ClusterTest {
 
   @Test
   void test() throws InterruptedException {
-    // ensure the default server exists first
-    final Database db = DB.getDefault();
-    Database other = createOther(db.pluginApi().dataSource());
-
     for (int i = 0; i < 10; i++) {
       Person foo = new Person("name " + i);
       foo.save();

ebean-redisson/src/test/java/org/integration/ClusterTest.java

@@ -5,17 +5,23 @@ import io.ebean.Database;
 import io.ebean.redisson.DuelCache;
 import org.domain.Person;
 import org.domain.query.QPerson;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
-import javax.sql.DataSource;
-
 import static org.assertj.core.api.Assertions.assertThat;
 
 class ClusterTest {
 
-  private Database createOther(DataSource dataSource) {
-    return Database.builder()
-      .dataSource(dataSource)
+  private static Database db;
+  private static Database other;
+
+  @BeforeAll
+  static void setup() {
+    // ensure the default server exists first
+    db = DB.getDefault();
+    other = Database.builder()
+      .dataSource(db.pluginApi().dataSource())
       .loadFromProperties()
       .defaultDatabase(false)
       .name("other")
@@ -24,12 +30,13 @@ class ClusterTest {
       .build();
   }
 
+  @AfterAll
+  static void tearDown() {
+    other.shutdown(false, false);
+  }
+
   @Test
   void testBothNear() throws InterruptedException {
-    // ensure the default server exists first
-    final Database db = DB.getDefault();
-    Database other = createOther(db.pluginApi().dataSource());
-
     new QPerson()
       .name.eq("Someone")
       .delete();
@@ -59,10 +66,6 @@ class ClusterTest {
 
   @Test
   void test() throws InterruptedException {
-    // ensure the default server exists first
-    final Database db = DB.getDefault();
-    Database other = createOther(db.pluginApi().dataSource());
-
     for (int i = 0; i < 10; i++) {
       Person foo = new Person("name " + i);
       foo.save();

ebean-test/src/test/java/io/ebean/xtest/base/EbeanServerFactory_ServerConfigStart_Test.java

@@ -15,6 +15,7 @@ import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 public class EbeanServerFactory_ServerConfigStart_Test {
 
@@ -83,7 +84,7 @@ public class EbeanServerFactory_ServerConfigStart_Test {
   }
 
   @Test
-  public void create_registeredDatabase_twice_returnsExistingInstance() {
+  public void create_registeredDatabase_twice_throwsException() {
 
     DatabaseBuilder config = new DatabaseConfig();
     config.setName("h2");
@@ -100,12 +101,15 @@ public class EbeanServerFactory_ServerConfigStart_Test {
     config.addServerConfigStartup(serverConfig -> startupCount.incrementAndGet());
 
     Database db = DatabaseFactory.create(config);
-    Database existing = DatabaseFactory.create(config);
+    try {
+      assertThatThrownBy(() -> DatabaseFactory.create(config))
+        .isInstanceOf(IllegalStateException.class)
+        .hasMessageContaining("already registered");
 
-    assertThat(existing).isSameAs(db);
-    assertThat(startupCount.get()).isEqualTo(1);
-
-    db.shutdown(true, false);
+      assertThat(startupCount.get()).isEqualTo(1);
+    } finally {
+      db.shutdown(true, false);
+    }
   }
 
   @Test