Skip to content

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! ❤️

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

2 participants