Improve testability and error checking

Author: remdui

README.md

@@ -48,3 +48,4 @@ bus.ifPresent(redisBus -> {
 
 - Caller identity is resolved at runtime from the platform plugin context.
 - `registerDatabase(...)` is reference-counted internally; repeated registrations reuse the same connection.
+- 

docs/ARCHITECTURE_REVIEW.md

@@ -0,0 +1,47 @@
+# Architecture Review
+
+Reviewed on: 2026-03-24
+
+## Scope
+
+- Public API ergonomics (`DataProviderAPI`, `DatabaseProvider` helpers)
+- Runtime safety in connection/config resolution internals
+- Documentation and test coverage quality
+
+## Strengths
+
+- Clear platform boundary (`BukkitDataProvider`, `VelocityDataProvider`) with a shared internal core.
+- Plugin-scoped identity model protects privileged operations from external callers.
+- Unified abstraction over relational, document, key-value, and messaging backends.
+- Reference-counted connection registry prevents duplicate connection churn.
+
+## Key Findings (Before Improvements)
+
+- Test coverage was effectively absent (`src/test/java/Test.java` placeholder only).
+- Bundled database templates used `default_credentials`, while docs/examples/integration commonly used `default`.
+- API default helper methods had null/readiness edge-cases that could produce unclear failures.
+- Missing docs files referenced by README reduced maintainability and onboarding quality.
+
+## Improvements Applied
+
+- Added real JUnit 5 unit tests for:
+  - `DatabaseProvider` helper semantics (`Optional` + `require*` behaviors)
+  - `DatabaseConfigMap` identifier fallback and diagnostics
+  - Document query/update builder input validation
+- Improved API helper behavior:
+  - `getDataAccessOptional(...)` now safely handles not-ready providers.
+  - `requireDataAccess(...)` now reports null provider data access explicitly.
+  - `getDataSourceOptional()` now tolerates unsupported and not-ready provider states.
+- Improved config usability:
+  - Added compatibility alias resolution between `default` and `default_credentials`.
+  - Added clearer missing-section warnings with available section names.
+  - Updated bundled config templates to include `default` as the primary identifier.
+  - Added `player_data_rw` MySQL template for common ORM write usage.
+- Hardened registry lookups:
+  - Stale/disconnected providers are now evicted during lookup, not just during registration.
+
+## Residual Risks / Next Steps
+
+- Most tests are unit-level; no automated integration tests currently validate real MySQL/Mongo/Redis instances.
+- Memcached implementations exist but are not reachable through `DatabaseType`; either expose or remove to reduce dead surface area.
+- Consider adding CI quality gates (`checkstyle`, test coverage thresholds, and smoke integration matrix).

docs/BEST_PRACTICES.md

@@ -27,7 +27,8 @@
 
 ## Configuration
 
-- Keep one connection identifier per concrete config section (for example: `default`, `player_data_rw`).
+- Keep one connection identifier per concrete config section (for example: `default`).
+- Keep identifier names consistent across code and YAML sections.
 - Use separate identifiers for different operational requirements (read-only, read-write, analytics).
 
 ## Concurrency

docs/USAGE_GUIDE.md

@@ -53,6 +53,10 @@ Optional<MessagingDataAccess> redisBus = api.registerDataAccess(
 );
 ```
 
+Identifier guidance:
+
+- Prefer `default` for single-connection setups.
+
 ## 3. Use the provider safely
 
 `DatabaseProvider` has helper methods to avoid raw casts:

pom.xml

@@ -15,6 +15,8 @@
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <paper.version>1.21.11-R0.1-SNAPSHOT</paper.version>
         <velocity.version>3.5.0-SNAPSHOT</velocity.version>
+        <junit.jupiter.version>5.10.2</junit.jupiter.version>
+        <maven.surefire.version>3.2.5</maven.surefire.version>
     </properties>
 
     <distributionManagement>
@@ -30,11 +32,16 @@
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-compiler-plugin</artifactId>
+                <version>3.13.0</version>
                 <configuration>
-                    <source>21</source>
-                    <target>21</target>
+                    <release>21</release>
                 </configuration>
             </plugin>
+            <plugin>
+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-surefire-plugin</artifactId>
+                <version>${maven.surefire.version}</version>
+            </plugin>
             <plugin>
                 <groupId>org.apache.maven.plugins</groupId>
                 <artifactId>maven-checkstyle-plugin</artifactId>
@@ -121,9 +128,9 @@
             <scope>provided</scope>
         </dependency>
         <dependency>
-            <groupId>junit</groupId>
-            <artifactId>junit</artifactId>
-            <version>3.8.1</version>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter</artifactId>
+            <version>${junit.jupiter.version}</version>
             <scope>test</scope>
         </dependency>
         <!-- Hibernate Core -->

src/main/java/nl/hauntedmc/dataprovider/api/DataProviderAPI.java

@@ -63,6 +63,7 @@ public <T extends DataAccess> Optional<T> registerDataAccess(
             String connectionIdentifier,
             Class<T> expectedDataAccessType
     ) {
+        Objects.requireNonNull(expectedDataAccessType, "Expected data access type cannot be null.");
         return registerDatabaseOptional(databaseType, connectionIdentifier)
                 .flatMap(provider -> provider.getDataAccessOptional(expectedDataAccessType));
     }
@@ -121,6 +122,7 @@ public <T extends DataAccess> Optional<T> getRegisteredDataAccess(
             String connectionIdentifier,
             Class<T> expectedDataAccessType
     ) {
+        Objects.requireNonNull(expectedDataAccessType, "Expected data access type cannot be null.");
         return getRegisteredDatabaseOptional(databaseType, connectionIdentifier)
                 .flatMap(provider -> provider.getDataAccessOptional(expectedDataAccessType));
     }

src/main/java/nl/hauntedmc/dataprovider/database/DatabaseProvider.java

@@ -50,7 +50,15 @@ public interface DatabaseProvider {
      */
     default <T extends DataAccess> Optional<T> getDataAccessOptional(Class<T> expectedType) {
         Objects.requireNonNull(expectedType, "Expected data access type cannot be null.");
-        DataAccess dataAccess = getDataAccess();
+        DataAccess dataAccess;
+        try {
+            dataAccess = getDataAccess();
+        } catch (IllegalStateException | UnsupportedOperationException ignored) {
+            return Optional.empty();
+        }
+        if (dataAccess == null) {
+            return Optional.empty();
+        }
         if (expectedType.isInstance(dataAccess)) {
             return Optional.of(expectedType.cast(dataAccess));
         }
@@ -67,6 +75,12 @@ default <T extends DataAccess> Optional<T> getDataAccessOptional(Class<T> expect
     default <T extends DataAccess> T requireDataAccess(Class<T> expectedType) {
         Objects.requireNonNull(expectedType, "Expected data access type cannot be null.");
         DataAccess dataAccess = getDataAccess();
+        if (dataAccess == null) {
+            throw new IllegalStateException(
+                    "Expected data access type " + expectedType.getName()
+                            + " but provider returned null."
+            );
+        }
         if (expectedType.isInstance(dataAccess)) {
             return expectedType.cast(dataAccess);
         }
@@ -84,7 +98,7 @@ default <T extends DataAccess> T requireDataAccess(Class<T> expectedType) {
     default Optional<DataSource> getDataSourceOptional() {
         try {
             return Optional.ofNullable(getDataSource());
-        } catch (UnsupportedOperationException ignored) {
+        } catch (UnsupportedOperationException | IllegalStateException ignored) {
             return Optional.empty();
         }
     }

src/main/java/nl/hauntedmc/dataprovider/database/document/model/DocumentQuery.java

@@ -3,6 +3,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 
 /**
  * A minimal DSL for building a "query" filter.
@@ -21,7 +22,8 @@ public class DocumentQuery {
      * @return this query instance for chaining
      */
     public DocumentQuery eq(String field, Object value) {
-        criteria.put(field, value);
+        String validatedField = requireFieldName(field);
+        criteria.put(validatedField, value);
         return this;
     }
 
@@ -33,9 +35,11 @@ public DocumentQuery eq(String field, Object value) {
      * @return this query instance for chaining
      */
     public DocumentQuery gte(String field, Object value) {
+        String validatedField = requireFieldName(field);
+        Objects.requireNonNull(value, "Greater-than-or-equal value cannot be null.");
         Map<String, Object> op = new HashMap<>();
         op.put("$gte", value);
-        criteria.put(field, op);
+        criteria.put(validatedField, op);
         return this;
     }
 
@@ -47,7 +51,9 @@ public DocumentQuery gte(String field, Object value) {
      * @return this query instance for chaining
      */
     public DocumentQuery raw(String field, Object expression) {
-        criteria.put(field, expression);
+        String validatedField = requireFieldName(field);
+        Objects.requireNonNull(expression, "Raw expression cannot be null.");
+        criteria.put(validatedField, expression);
         return this;
     }
 
@@ -59,4 +65,11 @@ public DocumentQuery raw(String field, Object expression) {
     public Map<String, Object> toMap() {
         return Collections.unmodifiableMap(criteria);
     }
+
+    private static String requireFieldName(String field) {
+        if (field == null || field.isBlank()) {
+            throw new IllegalArgumentException("Query field name cannot be null or blank.");
+        }
+        return field;
+    }
 }

src/main/java/nl/hauntedmc/dataprovider/database/document/model/DocumentUpdate.java

@@ -3,6 +3,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 
 /**
  * Represents an update operation.
@@ -20,10 +21,11 @@ public class DocumentUpdate {
      * @return this update instance for chaining
      */
     public DocumentUpdate set(String field, Object value) {
+        String validatedField = requireFieldName(field);
         operations.computeIfAbsent("$set", k -> new HashMap<String, Object>());
         @SuppressWarnings("unchecked")
         Map<String, Object> setMap = (Map<String, Object>) operations.get("$set");
-        setMap.put(field, value);
+        setMap.put(validatedField, value);
         return this;
     }
 
@@ -35,10 +37,12 @@ public DocumentUpdate set(String field, Object value) {
      * @return this update instance for chaining
      */
     public DocumentUpdate inc(String field, Number amount) {
+        String validatedField = requireFieldName(field);
+        Objects.requireNonNull(amount, "Increment amount cannot be null.");
         operations.computeIfAbsent("$inc", k -> new HashMap<String, Object>());
         @SuppressWarnings("unchecked")
         Map<String, Object> incMap = (Map<String, Object>) operations.get("$inc");
-        incMap.put(field, amount);
+        incMap.put(validatedField, amount);
         return this;
     }
 
@@ -50,4 +54,11 @@ public DocumentUpdate inc(String field, Number amount) {
     public Map<String, Object> toMap() {
         return Collections.unmodifiableMap(operations);
     }
+
+    private static String requireFieldName(String field) {
+        if (field == null || field.isBlank()) {
+            throw new IllegalArgumentException("Update field name cannot be null or blank.");
+        }
+        return field;
+    }
 }

src/main/java/nl/hauntedmc/dataprovider/internal/DataProviderRegistry.java

@@ -131,7 +131,18 @@ protected DatabaseProvider getDatabase(String pluginName, DatabaseType databaseT
         if (registration == null) {
             return null;
         }
-        return registration.provider();
+
+        DatabaseProvider provider = registration.provider();
+        if (isProviderHealthy(provider, key)) {
+            return provider;
+        }
+
+        if (activeDatabases.remove(key, registration)) {
+            disconnectQuietly(provider, key, "stale connection during lookup");
+            logger.warn("Removed stale " + databaseType.name() + " connection for " + pluginName
+                    + " (" + connectionIdentifier + ") while retrieving the provider.");
+        }
+        return null;
     }
 
     protected void unregisterDatabase(String pluginName, DatabaseType databaseType, String connectionIdentifier) {