Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix edge cases to allow sqlite backend #1157

Merged
merged 2 commits into from Dec 18, 2020
Merged

Fix edge cases to allow sqlite backend #1157

merged 2 commits into from Dec 18, 2020

Conversation

nouish
Copy link

@nouish nouish commented Dec 16, 2020

This makes some non-breaking changes to DatabaseDataStore to let it work with a SQLite database. These changes make absolutely no difference with MySQL, besides no longer logging a warning the first time GP is 'installed'. For more information, see the referenced issue comment below.

Changes:

  • SQL constants are now actually constants. This resolves an error suppressed as a warning, where the first time running GP, it would first fail to set the schema version, as a result of deleteSchemaVersionSQL being null. This is suppressed by MySQL, as the driver throws SQLException. Xerial's SQLite driver would throw NullPointerException, which was not suppressed.
  • Removes trailing ;s from SQL statements. These are pointless, and when batching, could cause problems, as MySQL driver sometimes fails to rewrite these (when rewrite enabled). Not a current concern, but regardless.
  • Don't bother with Class.forName(driverName) as this has been unnecessary since JDBC 4 - which came with Java 6!

With this PR, GriefPrevention may be configured to use SQLite, using GriefPrevention.Database.URL: jdbc:sqlite:database.db, but I don't see this as being "proper" support for SQLite, however.

Reference: #1068 (comment)

This makes some non-breaking changes to DatabaseDataStore to let it work with a SQLite database. These changes make absolutely no difference with MySQL, besides no longer logging a warning the first time GP is 'installed'. For more information, see the referenced issue comment below.

Changes:
- SQL constants are now actually constants. This resolves an error suppressed as a warning, where the first time running GP, it would first fail to set the schema version, as a result of `deleteSchemaVersionSQL` being `null`. This is suppressed by MySQL, as the driver throws `SQLException`. Xerial's SQLite driver would throw `NullPointerException`, which was not suppressed.
- Removes trailing `;`s from SQL statements. These are pointless, and when batching, could cause problems, as MySQL driver sometimes fails to rewrite these (when rewrite enabled). Not a current concern, but regardless.
- Don't bother with `Class.forName(driverName)` as this has been unnecessary since JDBC 4 - which came with Java 6!

With this commit, GriefPrevention may be configured to use SQLite, using `GriefPrevention.Database.URL: jdbc:sqlite:database.db`, but I don't see this as being "proper" support for SQLite, however.

Reference: #1068 (comment)
@RoboMWM
Copy link

RoboMWM commented Dec 18, 2020

Am presuming you tested with data from an existing mysql database?

@nouish
Copy link
Author

nouish commented Dec 18, 2020

Am presuming you tested with data from an existing mysql database?

Indeed, using a backup from a friends server dated April 25th, containing 3739 claims and 5438 players.

This PR contains no logic changes, besides making the ALTAR TABLE statements only run under the MySQL driver, leaving no regressions for MySQL.

All this does is clean up raw SQL (removing trailing ;s) and move them to static final constants, which stops DatabaseDataStore.updateSchemaVersionInStorage(int) from logging a warning (unsuppressed NPE in SQLite driver) the first time GP is run.


For what it's worth, I would like to see automated tests for MySQL integration, as that will absolutely help future improvements here. If someone could provide a really old, early version data set then that could be used to verify migration to newer schemas. If not, I have no problem using the database I used to verify this, but it was first created October 2019, meaning it was created in the current schema.

@RoboMWM RoboMWM merged commit 7ce30c6 into GriefPrevention:master Dec 18, 2020
@nouish nouish deleted the fix/sql-indirect-sqlite-support branch December 18, 2020 17:17
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.

None yet

2 participants