Fix SQLite schema bootstrap and legacy database detection#21
Fix SQLite schema bootstrap and legacy database detection#21SaltyAlpha wants to merge 1 commit intoGroupeZ-dev:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves startup reliability for SQLite-backed installs by aligning migration bookkeeping with the configured table prefix, adding a schema self-check to recreate missing core tables, centralizing schema definitions, and selecting the correct legacy SQLite file when applicable.
Changes:
- Use a prefix-derived migrations table name instead of a hard-coded one.
- Add a startup check that verifies core tables exist and recreates any that are missing.
- Introduce
StorageSchemaDefinitionsand reuse it across migrations and self-healing table creation. - Prefer
storage.dbwhen it exists anddatabase.dbdoes not (SQLite only).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/fr/maxlego08/zauctionhouse/storage/ZStorageManager.java | Select legacy SQLite file, configure migration table name by prefix, and add required-table self-check logic. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/StorageSchemaDefinitions.java | Centralized schema definitions for core tables and a list of required tables. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/migrations/CreateTransactionsMigration.java | Reuse shared schema definition for transactions table migration. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/migrations/CreatePlayerMigration.java | Reuse shared schema definition for players table migration. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/migrations/CreateLogsMigration.java | Reuse shared schema definition for logs table migration. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/migrations/CreateItemMigration.java | Reuse shared schema definition for items table migration. |
| src/main/java/fr/maxlego08/zauctionhouse/storage/migrations/CreateAuctionItemMigration.java | Reuse shared schema definition for auction_items table migration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return hasTable(metaData, connection, resolvedTableName.toUpperCase(Locale.ROOT)); | ||
| } catch (SQLException exception) { | ||
| this.plugin.getLogger().warning("Failed to inspect table '" + resolvedTableName + "': " + exception.getMessage()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
tableExists() returns false on any SQLException while inspecting metadata, which makes ensureRequiredTablesExist() assume the table is missing and attempt a CREATE TABLE. If metadata inspection fails for driver/permission reasons, this can incorrectly try to recreate existing tables (and potentially fail startup). Consider treating inspection failure as a hard error (disable plugin) or skipping self-heal when table existence can't be determined reliably.
| MigrationManager.setMigrationTableName(resolveMigrationTableName(databaseConfiguration.getTablePrefix())); | ||
| MigrationManager.setDatabaseConfiguration(databaseConfiguration); |
There was a problem hiding this comment.
Switching the migration tracking table name from the previously hard-coded value to a prefix-derived value can orphan existing installs' migration history (e.g., upgrades that already have the old migrations table). Consider adding a compatibility fallback (detect old table name and reuse/rename/copy it) or at least log clearly that migrations will be re-evaluated under a new tracking table to avoid unexpected behavior on upgrade.
| StorageSchemaDefinitions.requiredTables().forEach((tableName, definition) -> { | ||
| if (tableExists(tableName)) { | ||
| return; | ||
| } | ||
|
|
||
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | ||
| this.plugin.getLogger().warning("Missing required table '" + resolvedTableName + "'. Recreating it now."); | ||
| try { | ||
| SchemaBuilder.create(null, tableName, definition).execute(this.databaseConnection, logger); | ||
| } catch (SQLException exception) { | ||
| throw new IllegalStateException("Failed to recreate missing table '" + resolvedTableName + "'", exception); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private boolean tableExists(String tableName) { | ||
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | ||
| try (Connection connection = this.databaseConnection.getConnection()) { | ||
| DatabaseMetaData metaData = connection.getMetaData(); |
There was a problem hiding this comment.
ensureRequiredTablesExist() opens a new JDBC connection and fetches metadata for each required table. On pooled DBs this is extra overhead during startup; on SQLite it can also increase file locking churn. Consider reusing a single Connection/DatabaseMetaData for the whole check to minimize connections and repeated metadata lookups.
| StorageSchemaDefinitions.requiredTables().forEach((tableName, definition) -> { | |
| if (tableExists(tableName)) { | |
| return; | |
| } | |
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | |
| this.plugin.getLogger().warning("Missing required table '" + resolvedTableName + "'. Recreating it now."); | |
| try { | |
| SchemaBuilder.create(null, tableName, definition).execute(this.databaseConnection, logger); | |
| } catch (SQLException exception) { | |
| throw new IllegalStateException("Failed to recreate missing table '" + resolvedTableName + "'", exception); | |
| } | |
| }); | |
| } | |
| private boolean tableExists(String tableName) { | |
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | |
| try (Connection connection = this.databaseConnection.getConnection()) { | |
| DatabaseMetaData metaData = connection.getMetaData(); | |
| try (Connection connection = this.databaseConnection.getConnection()) { | |
| DatabaseMetaData metaData = connection.getMetaData(); | |
| if (metaData == null) { | |
| this.plugin.getLogger().warning("Failed to inspect required tables: database metadata is unavailable."); | |
| return; | |
| } | |
| StorageSchemaDefinitions.requiredTables().forEach((tableName, definition) -> { | |
| if (tableExists(connection, metaData, tableName)) { | |
| return; | |
| } | |
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | |
| this.plugin.getLogger().warning("Missing required table '" + resolvedTableName + "'. Recreating it now."); | |
| try { | |
| SchemaBuilder.create(null, tableName, definition).execute(this.databaseConnection, logger); | |
| } catch (SQLException exception) { | |
| throw new IllegalStateException("Failed to recreate missing table '" + resolvedTableName + "'", exception); | |
| } | |
| }); | |
| } catch (SQLException exception) { | |
| throw new IllegalStateException("Failed to inspect required tables", exception); | |
| } | |
| } | |
| private boolean tableExists(String tableName) { | |
| try (Connection connection = this.databaseConnection.getConnection()) { | |
| DatabaseMetaData metaData = connection.getMetaData(); | |
| return tableExists(connection, metaData, tableName); | |
| } catch (SQLException exception) { | |
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | |
| this.plugin.getLogger().warning("Failed to inspect table '" + resolvedTableName + "': " + exception.getMessage()); | |
| return false; | |
| } | |
| } | |
| private boolean tableExists(Connection connection, DatabaseMetaData metaData, String tableName) { | |
| String resolvedTableName = this.databaseConnection.getDatabaseConfiguration().replacePrefix(tableName); | |
| try { |
|
I am not sure about the purpose of your PR, can you give more context? |
This fixes a startup issue where SQLite installs could query zauctionhouse_players / zauctionhouse_transactions before those tables existed.
What changed
Make the migration tracking table follow the active configured table prefix instead of using a hard-coded name.
Add a startup schema self-check that recreates required core tables when migration bookkeeping is stale or out of sync.
Reuse shared schema definitions for both migrations and self-healing table creation so the structure stays consistent.
Prefer the legacy storage.db SQLite file when it exists and database.db does not, to avoid silently pointing old installs at a fresh empty database.
Why
Some installs were in a bad state where the configured prefix no longer matched previously recorded migration metadata, or the plugin was looking at a different SQLite file than the one containing the live data.
In those cases migrations could be treated as already applied while the actual prefixed tables were missing, causing runtime failures on player join and claim lookups.