Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Both providers must record lock-table state during direct export, and the guard needs targeted test coverage.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds DATABASECHANGELOGLOCK creation to manual migration exports while avoiding duplicate DDL.
Changes:
- Tracks lock-table pre-existence in both Liquibase providers.
- Emits lock-table DDL only for master changelogs.
- Extends integration coverage for empty and initialized databases.
| File | Review |
|---|---|
quarkus/tests/integration/src/test/java/org/keycloak/it/storage/database/BasicDatabaseTest.java |
Verifies lock-table DDL count across databases. |
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationEmptyDatabaseDistTest.java |
Tests empty-database lock-table export. |
quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationCustomProviderDistTest.java |
Nit: Does not exercise the pre-existence guard with an exported master changelog. |
quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/storage/database/liquibase/QuarkusJpaUpdaterProvider.java |
Critical: Direct exports do not initialize the lock-table pre-existence flag, suppressing required DDL. |
model/jpa/src/main/java/org/keycloak/connections/jpa/updater/liquibase/LiquibaseJpaUpdaterProvider.java |
Critical: Direct exports do not initialize the lock-table pre-existence flag, suppressing required DDL. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (lockTablePreExisted == null && isMasterChangelogTable(database)) { | ||
| lockTablePreExisted = SnapshotGeneratorFactory.getInstance().hasDatabaseChangeLogLockTable(database); | ||
| } |
There was a problem hiding this comment.
Both callers of export() run validate() first (QuarkusJpaConnectionProviderFactory#createOrUpdateSchema and DefaultJpaConnectionProviderFactory#migration), so the flag is always recorded before the export. Recording it in updateChangeSet() as well would not work: by then the database lock is held and CustomLockService has already created the table, so it would always read as pre-existing and the DDL would never be emitted. Validation is the only point before the lock. This is now spelled out in a comment next to the changelog backfill, and a null (not observed) deliberately emits nothing rather than guessing.
faefa3f to
d2e7f46
Compare
| if (isMasterChangelogTable(database) && Boolean.FALSE.equals(lockTablePreExisted)) { | ||
| loggingExecutor.comment("Create Database Lock Table"); | ||
| loggingExecutor.execute(new CreateDatabaseChangeLogLockTableStatement()); |
There was a problem hiding this comment.
Right, that paragraph was written for the state before this PR. Updated migrate_db.adoc: the script now creates both bookkeeping tables for an empty database, so a database set up purely by the script can be started against by a user without DDL rights. The remaining requirement is only on the database the script is generated against, where the server still creates the tables itself.
…abase The generated script creates the DATABASECHANGELOG table but not DATABASECHANGELOGLOCK, which the server creates itself on startup. That was a deliberate choice when the changelog table DDL was first added, on the grounds that the lock table is always created before the update runs, so emitting it would duplicate. That holds while the script is applied to the database it was generated from. It does not hold for the flow a database user without DDL privileges is left with: generate the script against one database, hand it to an administrator to apply to an empty one. The target then ends up without the lock table and the first startup still has to create it, which that user is not allowed to do. Emit it under the same rule as the changelog table, only when it did not exist before this server started, and only once, with the master changelog. A database populated purely by the generated script can then be started under a user with no DDL privileges at all. Closes keycloak#53088 Signed-off-by: Ali Tugrul Pinar <ali@keymate.io>
d2e7f46 to
20e3617
Compare
Closes #53088
What
With
migration-strategy=manualagainst an empty database, the generated script creates the schema and, since#51730, the
DATABASECHANGELOGtable, but notDATABASECHANGELOGLOCK. The server creates that one itself onstartup, in
CustomLockService.init(), before the migration runs.That was a deliberate choice when the changelog DDL was first added (3d47ab3, KEYCLOAK-3698): on the database
the script was generated from the lock table always exists by then, so emitting it would duplicate. It holds as
long as the script is applied to the same database. It does not hold for the flow the manual strategy exists for:
generate the script on one database, have an administrator apply it to a really empty one, and run Keycloak there
under a user without DDL rights. That database ends up without the lock table, and the first startup fails trying
to create it (#10149, #32535).
How
outputChangeLogTableCreationScriptemitsCreateDatabaseChangeLogLockTableStatementunder the same rule as thechangelog table:
bookkeeping tables up front so that a user without DDL rights can produce the script at all; the target then
already has the table and the DDL would break the script with
already exists;JpaEntityProviderchangelogs share the same lock table, so emitting itper changelog would duplicate.
Whether it pre-existed is recorded in
recordWhetherMasterChangelogTablePreExisted, i.e. during validation,which runs before the database lock is taken and the table gets created. The same change is applied to the
legacy
LiquibaseJpaUpdaterProvider.Testing
ManualMigrationEmptyDatabaseDistTest(H2, empty database): the script contains the lock table DDL exactly once.ManualMigrationCustomProviderDistTest(database already initialized, custom changelog outdated): the scriptcontains no lock table DDL, since the table existed before the server started.
BasicDatabaseTest.assertManualDbInitialization: the lock table is created exactly once, soPostgreSQLDistTest,MySQLDistTestandMariaDBDistTestguard it through thetest-databaseprofile(
PostgreSQLDistTestrun locally through Testcontainers, 7/7).Verified end to end on PostgreSQL 17 with two roles, an owner with DDL and an application user with DML only
(
has_schema_privilege(app, schema, 'CREATE')= false):CREATE TABLE ...databasechangelogand...databasechangeloglockonce each, in the headervalidatepermission deniedCustomLockService:permission denied for schema keycloakonCREATE TABLE ...databasechangeloglockCREATE TABLEfor either tablealready existsfor the bookkeeping tablesUnrelated, noticed while applying the script to PostgreSQL: the
26.8.0-51851-drop-old-expiration-idx-*changesets are exported as
MARK_RANbecause theirindexExistsprecondition is evaluated against the live(empty) database, so the script later fails on
CREATE INDEX ... already existsfor the re-created indexes.I will open a separate issue for that.