Skip to content

fix: use prefixed flyway table name#683

Open
darksaid98 wants to merge 2 commits intoHexaoxide:trunkfrom
darksaid98:fix/prefix-flyway-table
Open

fix: use prefixed flyway table name#683
darksaid98 wants to merge 2 commits intoHexaoxide:trunkfrom
darksaid98:fix/prefix-flyway-table

Conversation

@darksaid98
Copy link

Adds a prefix to the flyway_schema_history table, resulting in the new table carbon_flyway_schema_history. This also includes migration code to provide seamless migration for existing users from the old table to the new table.

Currently, the migration works as follows: If flyway_schema_history exists and carbon_flyway_schema_history doesn't, then we migrate to the new table.

Closes #679.

Testing

I've thoroughly tested this to ensure compatibility for all supported JDBC providers and existing instances of Carbon.

  • H2
    • Migrating from existing database.
    • Clean run using empty database
    • Ensure idempotency on migrated database
  • PostgreSQL
    • Migrating from existing database.
    • Clean run using empty database
    • Ensure idempotency on migrated database
  • MySQL
    • Migrating from existing database.
    • Clean run using empty database
    • Ensure idempotency on migrated database
  • MariaDB
    • Migrating from existing database.
    • Clean run using empty database
    • Ensure idempotency on migrated database

Adds a prefix to the `flyway_schema_history` table resulting in `carbon_flyway_schema_history`. This also includes migration code to provide seamless migration for existing users from the old table to the new.
@darksaid98
Copy link
Author

I couldn't get Checkstyle to work (Even with the self-compiled plugin), so I compiled & tested the plugin without it.

There are a few things on my mind I'm unsure of how you want:

  1. Error handling in the new method
  2. Stylistic inputs (like the use of var) which I'm not a common user of, but I threw together the code quite quickly

@jpenilla
Copy link
Member

I'm not sure I would call this a fix, I think adding a note to the wiki that carbon expects to own the database it uses is enough.

@darksaid98
Copy link
Author

I'm not sure I would call this a fix, I think adding a note to the wiki that carbon expects to own the database it uses is enough.

I agree this isn't a fix, but that's sort of completely dependent on what you consider the expected behavior. It's definitely not the prettiest piece of code.

I guess my question to you is? Would you rather have the plugin occupy the flyway_schema_history being inconsistent and potentially causing conflicts, or all tables use the carbon_ prefix, minimizing the risk of table name conflicts and providing clarity as to what software owns the flyway_schema_history table?

Here's where I'm coming from:
I think having the plugins' tables under a common prefix makes the most sense. While the plugin expecting to own the database is fine, it would in my mind be the least compatible way of dealing with this. Especially if you consider that a large number of users will be using shared hosting, without access to creating multiple databases. In the world of Minecraft plugin development, it's even fairly common to allow the user to specify the table prefix.

I'm open to getting this to a mergeable state or making that Wiki PR as suggested. Let me know which one you prefer! ❤️

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates Carbon’s Flyway configuration to use a prefixed schema history table (carbon_flyway_schema_history) and adds a startup-time migration to move existing installations off the unprefixed flyway_schema_history table to prevent cross-plugin table name conflicts.

Changes:

  • Add a manual migration step that copies flyway_schema_history to carbon_flyway_schema_history (when needed) and drops the old table.
  • Configure Flyway to use carbon_flyway_schema_history going forward.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +331 to +332
final String databaseType = metaData.getDatabaseProductName().toLowerCase();

Comment on lines +333 to +337
if (databaseType.contains("h2")) {
stmt.execute("CREATE TABLE \"carbon_flyway_schema_history\" AS SELECT * FROM \"flyway_schema_history\"");
stmt.execute("CREATE INDEX \"carbon_flyway_schema_history_s_idx\" ON \"carbon_flyway_schema_history\" (\"success\")");
stmt.execute("DROP TABLE \"flyway_schema_history\"");
this.logger.info("Successfully migrated flyway_schema_history to carbon_flyway_schema_history");
Comment on lines +341 to +344
stmt.execute("CREATE INDEX carbon_flyway_schema_history_s_idx ON carbon_flyway_schema_history (success)");
stmt.execute("DROP TABLE flyway_schema_history");
this.logger.info("Successfully migrated flyway_schema_history to carbon_flyway_schema_history");
} else if (databaseType.contains("postgresql")) {
Comment on lines +312 to +351
final var metaData = connection.getMetaData();

try (
final var oldTableCheck = metaData.getTables(null, null, "flyway_schema_history", null);
final var newTableCheck = metaData.getTables(null, null, "carbon_flyway_schema_history", null)
) {
final boolean oldTableExists = oldTableCheck.next();
if (!oldTableExists) {
return;
}

final boolean newTableExists = newTableCheck.next();
if (newTableExists) {
this.logger.debug("carbon_flyway_schema_history already exists, skipping migration");
return;
}
}

try (final var stmt = connection.createStatement()) {
final String databaseType = metaData.getDatabaseProductName().toLowerCase();

if (databaseType.contains("h2")) {
stmt.execute("CREATE TABLE \"carbon_flyway_schema_history\" AS SELECT * FROM \"flyway_schema_history\"");
stmt.execute("CREATE INDEX \"carbon_flyway_schema_history_s_idx\" ON \"carbon_flyway_schema_history\" (\"success\")");
stmt.execute("DROP TABLE \"flyway_schema_history\"");
this.logger.info("Successfully migrated flyway_schema_history to carbon_flyway_schema_history");
} else if (databaseType.contains("mysql") || databaseType.contains("mariadb")) {
stmt.execute("CREATE TABLE carbon_flyway_schema_history LIKE flyway_schema_history");
stmt.execute("INSERT INTO carbon_flyway_schema_history SELECT * FROM flyway_schema_history");
stmt.execute("CREATE INDEX carbon_flyway_schema_history_s_idx ON carbon_flyway_schema_history (success)");
stmt.execute("DROP TABLE flyway_schema_history");
this.logger.info("Successfully migrated flyway_schema_history to carbon_flyway_schema_history");
} else if (databaseType.contains("postgresql")) {
stmt.execute("CREATE TABLE carbon_flyway_schema_history AS SELECT * FROM flyway_schema_history");
stmt.execute("CREATE INDEX carbon_flyway_schema_history_s_idx ON carbon_flyway_schema_history (success)");
stmt.execute("DROP TABLE flyway_schema_history");
this.logger.info("Successfully migrated flyway_schema_history to carbon_flyway_schema_history");
} else {
this.logger.warn("Unknown database type: {}. Skipping flyway schema history migration.", databaseType);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] flyway_schema_history should be prefixed

3 participants