Repository navigation
Export custom provider changesets in manual migration-strategy - #49061
Conversation
There was a problem hiding this comment.
Thanks for the fix. I think it looks fine though I have optional suggestion (see the comment) and I also believe that we need a test. I verified that this fails without the fix and passes with the fix:
diff --git a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestEntity.java b/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestEntity.java
deleted file mode 100644
index a5500eb7bc..0000000000
--- a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestEntity.java
+++ /dev/null
@@ -1,34 +0,0 @@
-package com.acme.provider.legacy.jpa.migration;
-
-import jakarta.persistence.Column;
-import jakarta.persistence.Entity;
-import jakarta.persistence.Id;
-import jakarta.persistence.Table;
-
-@Entity
-@Table(name = "MIGRATION_TEST_ENTITY")
-public class MigrationTestEntity {
-
- @Id
- @Column(name = "ID")
- private String id;
-
- @Column(name = "NAME")
- private String name;
-
- public String getId() {
- return id;
- }
-
- public void setId(String id) {
- this.id = id;
- }
-
- public String getName() {
- return name;
- }
-
- public void setName(String name) {
- this.name = name;
- }
-}
diff --git a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProvider.java b/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProvider.java
deleted file mode 100644
index 308fe4d90b..0000000000
--- a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProvider.java
+++ /dev/null
@@ -1,28 +0,0 @@
-package com.acme.provider.legacy.jpa.migration;
-
-import java.util.Collections;
-import java.util.List;
-
-import org.keycloak.connections.jpa.entityprovider.JpaEntityProvider;
-
-public class MigrationTestJpaEntityProvider implements JpaEntityProvider {
-
- @Override
- public List<Class<?>> getEntities() {
- return Collections.singletonList(MigrationTestEntity.class);
- }
-
- @Override
- public String getChangelogLocation() {
- return "META-INF/migration-test-changelog.xml";
- }
-
- @Override
- public String getFactoryId() {
- return MigrationTestJpaEntityProviderFactory.ID;
- }
-
- @Override
- public void close() {
- }
-}
diff --git a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProviderFactory.java b/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProviderFactory.java
deleted file mode 100644
index 71a994bad0..0000000000
--- a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestJpaEntityProviderFactory.java
+++ /dev/null
@@ -1,34 +0,0 @@
-package com.acme.provider.legacy.jpa.migration;
-
-import org.keycloak.Config.Scope;
-import org.keycloak.connections.jpa.entityprovider.JpaEntityProvider;
-import org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory;
-import org.keycloak.models.KeycloakSession;
-import org.keycloak.models.KeycloakSessionFactory;
-
-public class MigrationTestJpaEntityProviderFactory implements JpaEntityProviderFactory {
-
- static final String ID = "migration-test-entity-provider";
-
- @Override
- public JpaEntityProvider create(KeycloakSession session) {
- return new MigrationTestJpaEntityProvider();
- }
-
- @Override
- public String getId() {
- return ID;
- }
-
- @Override
- public void init(Scope config) {
- }
-
- @Override
- public void postInit(KeycloakSessionFactory factory) {
- }
-
- @Override
- public void close() {
- }
-}
diff --git a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestProvider.java b/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestProvider.java
deleted file mode 100644
index 5a71c51936..0000000000
--- a/quarkus/tests/integration/src/test-providers/java/com/acme/provider/legacy/jpa/migration/MigrationTestProvider.java
+++ /dev/null
@@ -1,26 +0,0 @@
-package com.acme.provider.legacy.jpa.migration;
-
-import java.util.Map;
-
-import org.keycloak.it.TestProvider;
-
-public class MigrationTestProvider implements TestProvider {
-
- @Override
- public Class[] getClasses() {
- return new Class[] {
- MigrationTestEntity.class,
- MigrationTestJpaEntityProvider.class,
- MigrationTestJpaEntityProviderFactory.class
- };
- }
-
- @Override
- public Map<String, String> getManifestResources() {
- return Map.of(
- "migration-test-changelog.xml", "migration-test-changelog.xml",
- "services/org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory",
- "services/org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory"
- );
- }
-}
diff --git a/quarkus/tests/integration/src/test-providers/java/org/keycloak/it/jpa/migration/MigrationTestJpaEntityProvider.java b/quarkus/tests/integration/src/test-providers/java/org/keycloak/it/jpa/migration/MigrationTestJpaEntityProvider.java
index 78a38d19a0..c5a3b2947d 100644
--- a/quarkus/tests/integration/src/test-providers/java/org/keycloak/it/jpa/migration/MigrationTestJpaEntityProvider.java
+++ b/quarkus/tests/integration/src/test-providers/java/org/keycloak/it/jpa/migration/MigrationTestJpaEntityProvider.java
@@ -1,6 +1,5 @@
package org.keycloak.it.jpa.migration;
-import java.util.Collections;
import java.util.List;
import org.keycloak.connections.jpa.entityprovider.JpaEntityProvider;
@@ -9,7 +8,7 @@ public class MigrationTestJpaEntityProvider implements JpaEntityProvider {
@Override
public List<Class<?>> getEntities() {
- return Collections.singletonList(MigrationTestEntity.class);
+ return List.of(MigrationTestEntity.class);
}
@Override
diff --git a/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/migration-test-changelog.xml b/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/migration-test-changelog.xml
deleted file mode 100644
index f9d02e91f4..0000000000
--- a/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/migration-test-changelog.xml
+++ /dev/null
@@ -1,16 +0,0 @@
-<?xml version="1.0" encoding="UTF-8" standalone="no"?>
-<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
- xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
- xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">
- <changeSet author="keycloak" id="migration-test-1.0">
- <createTable tableName="MIGRATION_TEST_ENTITY">
- <column name="ID" type="VARCHAR(36)">
- <constraints nullable="false"/>
- </column>
- <column name="NAME" type="VARCHAR(255)"/>
- </createTable>
- <addPrimaryKey constraintName="PK_MIGRATION_TEST_ENTITY"
- tableName="MIGRATION_TEST_ENTITY"
- columnNames="ID"/>
- </changeSet>
-</databaseChangeLog>
diff --git a/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/services/org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory b/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/services/org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory
deleted file mode 100644
index 247c52a049..0000000000
--- a/quarkus/tests/integration/src/test-providers/resources/com/acme/provider/legacy/jpa/migration/services/org.keycloak.connections.jpa.entityprovider.JpaEntityProviderFactory
+++ /dev/null
@@ -1 +0,0 @@
-com.acme.provider.legacy.jpa.migration.MigrationTestJpaEntityProviderFactory
diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationCustomProviderDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationCustomProviderDistTest.java
index b4f1765b4d..9e2d931c6b 100644
--- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationCustomProviderDistTest.java
+++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ManualMigrationCustomProviderDistTest.java
@@ -3,8 +3,6 @@ package org.keycloak.it.cli.dist;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
-import java.nio.file.Path;
-import java.nio.file.Paths;
import org.apache.commons.io.FileUtils;
import org.keycloak.it.junit5.extension.CLIResult;
@@ -13,13 +11,13 @@ import org.keycloak.it.junit5.extension.RawDistOnly;
import org.keycloak.it.junit5.extension.TestProvider;
import org.keycloak.it.utils.RawDistRootPath;
-import com.acme.provider.legacy.jpa.migration.MigrationTestProvider;
+import org.keycloak.it.jpa.migration.MigrationTestProvider;
import io.quarkus.test.junit.main.Launch;
import org.junit.jupiter.api.Test;
import static org.hamcrest.CoreMatchers.containsString;
-import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertTrue;
@DistributionTest
@RawDistOnly(reason = "Containers are immutable")
@@ -31,9 +29,8 @@ public class ManualMigrationCustomProviderDistTest {
void testCustomProviderChangesetExportedInManualMode(CLIResult cliResult, RawDistRootPath rawDistRootPath) {
cliResult.assertMessage("Database not up-to-date, please migrate database with");
- Path scriptPath = Paths.get(rawDistRootPath.getDistRootPath() + File.separator + "bin" + File.separator + "keycloak-database-update.sql");
- File script = scriptPath.toFile();
- assertThat("Export file must exist when migration-strategy=manual", script.isFile(), is(true));
+ File script = rawDistRootPath.getDistRootPath().resolve("bin").resolve("keycloak-database-update.sql").toFile();
+ assertTrue(script.isFile(), "Export file must exist when migration-strategy=manual");
String output;
try {
| @@ -96,12 +96,14 @@ private void update(Connection connection, File file, String defaultSchema) { | |||
|
|
|||
| Writer exportWriter = null; | |||
| try { | |||
|
|
|||
| if (file != null) { | |||
There was a problem hiding this comment.
Current Keycloak code only invokes this update when validate determines there are pending changes, but org.keycloak.connections.jpa.updater.JpaUpdaterProvider#export does not restrict when they are called. I'd like to avoid empty file unless we change javadoc to specify mention it. I know it is slightly more verbose, but instead of this change, would you mind keeping the original code on lines 102-104 and apply this after if (customChangelog != null) { :
if (exportWriter == null && file != null) {
exportWriter = new FileWriter(file);
}? it is not important, so if you disagree I do not insist.
There was a problem hiding this comment.
Hey @michalvavrik , thanks for the careful review.
I applied your lazy init suggestion as is. The writer is still initialized inside the master changelog branch like the original code, and the custom provider loop now has the same if (exportWriter == null && file != null) guard. So when neither path has pending changes the file is never opened, which keeps the current JpaUpdaterProvider#export Javadoc accurate.
For the test, I added ManualMigrationCustomProviderDistTest under quarkus/tests/integration, running against PostgreSQL on the raw distribution. It is two-phase so it actually exercises the path the fix targets. The first start applies the master changelog without the custom provider, so the DB ends up with MIGRATION_MODEL.version matching Version.VERSION. Then the custom MigrationTestJpaEntityProvider is copied in, the dist is rebuilt, and the server is started with migration-strategy=manual. At that point the master path is skipped (needVerifyMasterChangelog() returns false), only the custom changelog is pending, validate returns OUTDATED, and the server fails to start with the expected "Database not up-to-date, please migrate database with ..." message. The assertions then check that keycloak-database-update.sql was actually written and contains the custom changeset (MIGRATION_TEST_ENTITY).
I verified locally that this test fails without the fix (writer stays null in the custom only path, so the DDL is applied directly and no script is produced) and passes with it. I used the package layout you suggested, org.keycloak.it.jpa.migration.
Everything is squashed into a single commit per the contributing guidelines and force pushed. Let me know if you want me to adjust anything.
There was a problem hiding this comment.
@Alnyli07 it doesn't need to be 2-phased, what was wrong with my test suggestion? it was much simpler. Now it is 100 % fine if you have a your way to test this, but we can leverage the fact that the DB is preinitialized so that you are not increasing test execution time for any reason. We are trying to keep tests quick, because it becomes a burden to wait for PR CI. Please have a look again, if I missed something, sure, let's go your way, but I'd like you to provide some reasons why a simpler version wouldn't work.
47837aa to
ba9a377
Compare
|
@Alnyli07 please rebase on the current main and resolve compilation failures, thanks. |
ba9a377 to
bde6fd9
Compare
|
|
||
| @Test | ||
| void testCustomProviderChangesetExportedInManualMode(KeycloakRunner runner, RawDistRootPath rawDistRootPath) { | ||
| // Phase 1: apply the master changelog without the custom provider installed. |
There was a problem hiding this comment.
The raw distribution test already has preinitalized database with applied master changelog without the custom provider unless I am missing something. I tested it 2 weeks ago.
There was a problem hiding this comment.
Thanks, I checked before changing anything.
I tried dropping phase 1 and running only copyProvider plus build plus manual start. The server fails with "Database not initialized, please initialize database with ..." instead of "not up-to-date",
because the postgres testcontainer comes up fresh per test. Looking at the code, preInitH2 in RawKeycloakDistribution only seeds the H2 dist, and init-postgres.sql just creates the foo schema,
so master ends up EMPTY and validate() returns before it ever gets to the custom changelog.
Phase 1 is what puts MIGRATION_MODEL.version in place so phase 2 actually hits the custom only path the fix targets. Were you thinking of the H2 raw dist case, or am I missing where postgres gets preinitialized?
There was a problem hiding this comment.
Actually, please disregard my previous message. You were totally right, and sorry for the noise!
What threw me off was that I had a conflicting @WithDatabase annotation in my test. It was switching the distribution away from the H2 path and bypassing the pre-initialization step entirely, which is why I thought I needed that first phase for Postgres.
Once I dropped that annotation and let the test run on the pre-initialized H2 distribution as you suggested, everything worked perfectly. Local execution time dropped from 184s down to 61s!
Force pushed the clean fix.
There was a problem hiding this comment.
It sounds like things are straightened out, but just wanted to add that there is at least one test (LiquibaseDistTest) that deletes the data/h2 directory if a full initialization is needed. Hopefully that will rarely be needed.
bde6fd9 to
a941c4b
Compare
|
|
||
| CLIResult startResult = runner.run("start", "--optimized", | ||
| "--spi-connections-jpa-quarkus-migration-strategy=manual", | ||
| "--spi-connections-jpa-quarkus-initialize-empty=false", |
There was a problem hiding this comment.
I think this line is not necessary --spi-connections-jpa-quarkus-initialize-empty=false but it is perfectly fine to keep it as your original reproducer had it.
| @@ -113,6 +113,9 @@ private void update(Connection connection, File file, String defaultSchema) { | |||
| String factoryId = jpaProvider.getFactoryId(); | |||
| String changelogTableName = JpaUtils.getCustomChangelogTableName(factoryId); | |||
| KeycloakLiquibase liquibase = getLiquibaseForCustomProviderUpdate(connection, defaultSchema, customChangelog, jpaProvider.getClass().getClassLoader(), changelogTableName); | |||
| if (exportWriter == null && file != null) { | |||
There was a problem hiding this comment.
This could be handled in a follow-up, but it is probably better to move the FileWriter creation in general to the start of the update method. Wouldn't it be better for export to create an empty file, rather than to leave a previous file unmodified when there are no updates?
There was a problem hiding this comment.
This could be handled in a follow-up, but it is probably better to move the FileWriter creation in general to the start of the update method. Wouldn't it be better for export to create an empty file, rather than to leave a previous file unmodified when there are no updates?
That's how @Alnyli07 had it at the first place. I haven't considered the re-recreate scenario. Apologies @Alnyli07 .
There was a problem hiding this comment.
No worries at all, @michalvavrik! It's a tricky edge case, glad we cleared it up.
Thanks both for the great catch. If you want, I can quickly move the FileWriter back to the start in this PR, or we can just leave it as-is and handle it in a follow-up. Let me know what works best for you!
There was a problem hiding this comment.
I've updated the code and moved the FileWriter back to the start as discussed. Force pushed!
There was a problem hiding this comment.
Looks good, thank you @Alnyli07 and @michalvavrik. Have a look at the comment about the FileWriter and whether you think it makes sense to address here.
Lazy-init the export writer so the file is only opened when a changelog actually has pending changes — either the master path or the custom-only path. Adds a DistributionTest against PostgreSQL that applies the master changelog first, then installs a custom JpaEntityProvider and starts in manual mode, asserting the generated keycloak-database-update.sql contains the custom changeset. Closes keycloak#49057 Signed-off-by: Ali Tugrul Pinar <ali@keymate.io>
a941c4b to
88ed34b
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.forms.MultipleTabsLoginTest#testWithAuthSessionExpiredInTheMiddle_badRedirectUriKeycloak CI - Forms IT (chrome) |
There was a problem hiding this comment.
LGTM, thanks @Alnyli07 and @michalvavrik
…oak#49061) init the export writer so the file is always created. Adds a DistributionTest against PostgreSQL that applies the master changelog first, then installs a custom JpaEntityProvider and starts in manual mode, asserting the generated keycloak-database-update.sql contains the custom changeset. Closes keycloak#49057 Signed-off-by: Ali Tugrul Pinar <ali@keymate.io>
What this PR does
Writer exportWriterinQuarkusJpaUpdaterProvider.update(Connection, File, String)is now initialized once at the top of the method, instead of inside theif (needVerifyMasterChangelog())branch. The same writer is used for both the master changelog and the customJpaEntityProviderloop, somigration-strategy=manualcorrectly writes the export file (and avoids applying DDL directly) when only custom-provider changesets are pending and the master is skipped.Resource handling is unchanged. The writer is still closed in the existing
finallyblock.Why
See #49057 for the full root-cause walkthrough. Short version: when
MIGRATION_MODEL.versionequalsVersion.VERSION, the master branch is skipped, so the writer never gets initialized. The custom-provider loop then callsupdateChangeSet(liquibase, null)and Liquibase applies the DDL directly, which silently breaks the DBA workflow thatmanualmode is supposed to guarantee.How it was tested
I built the patched class, swapped it into a Keycloak 26.6.1 image, and ran the matrix from the issue against PostgreSQL 15 with one custom
JpaEntityProvidercontributing its own changelog. The only row that changes versus the unpatched image is the bug case:MIGRATION_MODEL.versionTo make the export file easy to inspect without
kubectl cp, I used a small entrypoint wrapper thatcatsKC_SPI_CONNECTIONS_JPA__QUARKUS__MIGRATION_EXPORTto stdout when the server exits. That wrapper is not part of this PR, it is just how I read the file during verification. With the fix in place the file contains the expectedALTER TABLE/CREATE INDEX/INSERT INTO databasechangelog_<provider>statements. Without the fix the file is empty or missing and the schema is mutated.Notes on the contribution checklist
QuarkusJpaUpdaterProviderwithmigration-strategy=manualagainst a customJpaEntityProvider, so a regression test needs new fixtures (a custom provider plus a separate changelog). Happy to add one in a follow-up if you want, just tell me which style you prefer (a unit test onupdate(...)with mocked Liquibase, or an integration test undertestsuite/). The matrix above covers the four behavioural paths ofupdate(...)end-to-end../mvnw spotless:checkis clean on the change.migration-strategy=manualalready documents the intended behaviour, this PR closes an implementation gap rather than changing the contract.AI disclosure
I used an AI assistant (Anthropic Claude) to help read through
QuarkusJpaUpdaterProviderandQuarkusJpaConnectionProviderFactory, pinpoint theexportWriterscope issue, draft the issue/PR text, and run the empirical matrix above. The four-line code change itself I reviewed and understand, and I can answer follow-up questions or revise it independently.Closes
Closes #49057