Repository navigation
Create the lock table in the manual migration export for an empty database - #53096
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.
faefa3f to
d2e7f46
Compare
d2e7f46 to
20e3617
Compare
20e3617 to
59a6a09
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation consistently covers both providers, preserves pre-existing-table behavior, and includes focused regression tests and documentation.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
96dbe26 to
37bda78
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.cluster.RoleInvalidationClusterTest#crudWithoutFailoverorg.keycloak.testsuite.cluster.RoleInvalidationClusterTest#crudWithFailover |
…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>
37bda78 to
8472f78
Compare
…abase (keycloak#53096) 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>


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.