8000 Evaluate CockroachDB as a PostgreSQL database in tests by ahus1 · Pull Request #54845 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/actions/run-store-tests/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ inputs:
db:
description: The database to use for tes 10BC8 ts
required: true
server-start-timeout:
description: Seconds to wait for the Keycloak server to start, which includes creating the schema
default: "360"

runs:
using: composite
Expand All @@ -16,12 +19,12 @@ runs:
- name: Run new base tests
shell: bash
run: |
KC_TEST_DATABASE=${{ inputs.db }} KC_TEST_DATABASE_REUSE=true TESTCONTAINERS_REUSE_ENABLE=true ./mvnw package -f tests/pom.xml -Dtest=DatabaseTestSuite -Dkc.test.server.start.timeout=360
KC_TEST_DATABASE=${{ inputs.db }} KC_TEST_DATABASE_REUSE=true TESTCONTAINERS_REUSE_ENABLE=true ./mvnw package -f tests/pom.xml -Dtest=DatabaseTestSuite -Dkc.test.server.start.timeout=${{ inputs.server-start-timeout }}

- name: Run stateless tests
shell: bash
run: |
KC_TEST_DATABASE=${{ inputs.db }} KC_TEST_DATABASE_REUSE=true TESTCONTAINERS_REUSE_ENABLE=true ./mvnw package -f tests/pom.xml -Dtest=StatelessTestSuite -Dkc.test.server.start.timeout=360
KC_TEST_DATABASE=${{ inputs.db }} KC_TEST_DATABASE_REUSE=true TESTCONTAINERS_REUSE_ENABLE=true ./mvnw package -f tests/pom.xml -Dtest=StatelessTestSuite -Dkc.test.server.start.timeout=${{ inputs.server-start-timeout }}

- uses: ./.github/actions/upload-flaky-tests
name: Upload flaky tests
Expand Down
37 changes: 37 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ jobs:
ci-aurora: ${{ steps.auroradb-tests.outputs.run-aurora-tests }}
ci-azure: ${{ steps.azure-tests.outputs.run-azure-tests }}
ci-additional-dbs: ${{ steps.additional-dbs-tests.outputs.run-additional-dbs-tests }}
ci-cockroachdb: ${{ steps.cockroachdb-tests.outputs.run-cockroachdb-tests }}
ci-admin-v2: ${{ steps.conditional.outputs.admin-v2 }}
ci-jackson3: ${{ steps.conditional.outputs.ci-jackson3 }}
permissions:
Expand Down Expand Up @@ -98,6 +99,24 @@ jobs:
env:
PRIVATE_DBS_QUAY_USERNAME: ${{ secrets.PRIVATE_DBS_QUAY_USERNAME }}
PRIVATE_DBS_QUAY_TOKEN: ${{ secrets.PRIVATE_DBS_QUAY_TOKEN }}
- name: CockroachDB conditional check
id: cockroachdb-tests
# Runs for manually or nightly triggered runs, and for pushes/PRs when any commit message in the PR/push contains "[cockroachdb ci]"
run: |
RUN_COCKROACHDB_TESTS=false
if [[ $GITHUB_EVENT_NAME == "pull_request" ]]; then
COMMIT_MESSAGE="$(gh api --paginate "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/commits" --jq '.[].commit.message')"
else
COMMIT_MESSAGE="${PUSH_COMMIT_MESSAGES}"
fi
if [[ $GITHUB_EVENT_NAME == "workflow_dispatch" || $GITHUB_EVENT_NAME == "schedule" || "${COMMIT_MESSAGE}" == *"[cockroachdb ci]"* ]]; then
RUN_COCKROACHDB_TESTS=true
fi
echo "run-cockroachdb-tests=$RUN_COCKROACHDB_TESTS" >> $GITHUB_OUTPUT
env:
GH_TOKEN: ${{ github.token }}
PR_NUMBER: ${{ github.event.pull_request.number }}
PUSH_COMMIT_MESSAGES: ${{ join(github.event.commits.*.message, '\n') }}
testsuite-deprecation-check:
name: Testsuite Deprecation Check
runs-on: ubuntu-latest
Expand Down Expand Up @@ -877,6 +896,23 @@ jobs:
with:
db: ${{ matrix.db }}

store-integration-tests-cockroachdb:
name: Store IT (CockroachDB)
needs: [build, conditional]
runs-on: ubuntu-latest
timeout-minutes: 85
if: needs.conditional.outputs.ci-cockroachdb == 'true'
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1

- id: run-store-tests
name: Run Store Integration Tests - cockroachdb
uses: ./.github/actions/run-store-tests
with:
db: cockroachdb
# creating the schema takes several minutes
server-start-timeout: 1500

store-model-tests:
name: Store Model Tests
runs-on: ubuntu-latest
Expand Down Expand Up @@ -1362,6 +1398,7 @@ jobs:
- jdk-integration-tests
- store-integration-tests
- store-integration-tests-additional
- store-integration-tests-cockroachdb
- volatile-sessions-tests
- http-client-v2-tests
- store-model-tests
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.keycloak.connections.jpa.updater.liquibase;

import liquibase.database.core.CockroachDatabase;

/**
* Liquibase's {@link CockroachDatabase} reports no DDL support in transactions and then runs the migration in auto-commit mode, which doesn't allow the savepoints used by Keycloak's custom tasks.
* CockroachDB commits an open transaction before a DDL statement when {@code autocommit_before_ddl} is enabled, so Keycloak can keep using transactions.
*/
public class KeycloakCockroachDatabase extends CockroachDatabase {

@Override
public boolean supportsDDLInTransaction() {
return true;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@
package org.keycloak.connections.jpa.updater.liquibase.conn;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;

import org.keycloak.Config;
import org.keycloak.config.DatabaseOptions;
import org.keycloak.connections.jpa.updater.liquibase.KeycloakCockroachDatabase;
import org.keycloak.connections.jpa.updater.liquibase.LiquibaseJpaUpdaterProvider;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
Expand All @@ -33,6 +37,7 @@
import liquibase.database.AbstractJdbcDatabase;
import liquibase.database.Database;
import liquibase.database.DatabaseConnection;
import liquibase.database.core.PostgresDatabase;
import liquibase.database.jvm.JdbcConnection;
import liquibase.exception.LiquibaseException;
import liquibase.resource.ClassLoaderResourceAccessor;
Expand Down Expand Up @@ -171,6 +176,9 @@ private Database getLiquibaseDatabase(Connection connection) {
} catch (Exception e) {
throw new RuntimeException("Failed to create instance of " + liquibaseDatabaseClazz.getName());
}
if (liquibaseDatabase.getClass() == PostgresDatabase.class && isCockroachDB(connection)) {
liquibaseDatabase = new KeycloakCockroachDatabase();
}
DatabaseConnection liquibaseConnection = new JdbcConnection(connection);
try {
logger.debugf("DB Product Name: %s", liquibaseConnection.getDatabaseProductName());
Expand All @@ -182,4 +190,14 @@ private Database getLiquibaseDatabase(Connection connection) {
return liquibaseDatabase;
}

// CockroachDB reports itself as PostgreSQL, only the result of version() identifies it
private boolean isCockroachDB(Connection connection) {
try (Statement statement = connection.createStatement(); ResultSet rs = statement.executeQuery("SELECT version()")) {
return rs.next() && rs.getString(1).startsWith("CockroachDB");
} catch (SQLException e) {
logger.debug("Failed to detect DB product version", e);
return false;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package org.keycloak.connections.jpa.updater.liquibase.custom;

import liquibase.Scope;
import liquibase.change.core.DropPrimaryKeyChange;
import liquibase.changelog.ChangeSet;
import liquibase.database.Database;
import liquibase.database.core.CockroachDatabase;
import liquibase.sql.Sql;
import liquibase.sqlgenerator.SqlGeneratorChain;
import liquibase.sqlgenerator.core.AddPrimaryKeyGenerator;
import liquibase.statement.core.AddPrimaryKeyStatement;

/**
* Skips the primary key that was already created by {@link CockroachDropPrimaryKeyGenerator} when it processed the preceding {@code dropPrimaryKey} for the same table.
*/
public class CockroachAddPrimaryKeyGenerator extends AddPrimaryKeyGenerator {

@Override
public int getPriority() {
return PRIORITY_DATABASE + 100;
}

@Override
public boolean supports(AddPrimaryKeyStatement statement, Database database) {
return database instanceof CockroachDatabase;
}

@Override
public Sql[] generateSql(AddPrimaryKeyStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
if (hasDropInChangeSet(statement.getTableName())) {
return new Sql[0];
}
return super.generateSql(statement, database, sqlGeneratorChain);
}

private boolean hasDropInChangeSet(String tableName) {
ChangeSet changeSet = Scope.getCurrentScope().get(Scope.Attr.changeSet, ChangeSet.class);
return changeSet != null && changeSet.getChanges().stream()
.anyMatch(c -> c instanceof DropPrimaryKeyChange drop && tableName.equalsIgnoreCase(drop.getTableName()));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package org.keycloak.connections.jpa.updater.liquibase.custom;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

import liquibase.Scope;
import liquibase.change.core.AddPrimaryKeyChange;
import liquibase.changelog.ChangeSet;
import liquibase.database.Database;
import liquibase.database.core.CockroachDatabase;
import liquibase.database.jvm.JdbcConnection;
import liquibase.exception.UnexpectedLiquibaseException;
import liquibase.sql.Sql;
import liquibase.sql.UnparsedSql;
import liquibase.sqlgenerator.SqlGeneratorChain;
import liquibase.sqlgenerator.core.DropPrimaryKeyGenerator;
import liquibase.statement.core.DropPrimaryKeyStatement;

/**
* CockroachDB rejects dropping a primary key without adding a new one in the same statement, and doesn't allow dropping a column that is part of the primary key.
* The primary key is therefore replaced with {@code ALTER PRIMARY KEY} at the place of the drop, using the columns of the next {@code addPrimaryKey} for the same table in the changeset.
* The {@code addPrimaryKey} is then skipped by {@link CockroachAddPrimaryKeyGenerator}.
* <p>
* CockroachDB keeps the old primary key as a unique secondary index, which PostgreSQL wouldn't have and which would reject valid rows (for example multi-valued user attributes).
* That index is dropped right after, using the name CockroachDB assigns to it: {@code <table>_<column>_<column>_key}.
*/
public class CockroachDropPrimaryKeyGenerator extends DropPrimaryKeyGenerator {

@Override
public int getPriority() {
return PRIORITY_DATABASE + 100;
}

@Override
public boolean supports(DropPrimaryKeyStatement statement, Database database) {
return database instanceof CockroachDatabase;
}

@Override
public Sql[] generateSql(DropPrimaryKeyStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
Optional<AddPrimaryKeyChange> add = findAdd(statement.getTableName());
if (add.isEmpty()) {
return new Sql[0];
}
String table = database.escapeTableName(statement.getCatalogName(), statement.getSchemaName(), statement.getTableName());
String columns = database.escapeColumnNameList(add.get().getColumnNames());
List<String> oldColumns = currentPrimaryKeyColumns(database, statement.getTableName());
String oldIndex = (statement.getTableName() + "_" + String.join("_", oldColumns) + "_key").toLowerCase();
return new Sql[] {
new UnparsedSql("ALTER TABLE " + table + " ALTER PRIMARY KEY USING COLUMNS (" + columns + ")"),
new UnparsedSql("DROP INDEX IF EXISTS " + table + "@" + oldIndex)
};
}

private static List<String> currentPrimaryKeyColumns(Database database, String tableName) {
Connection connection = ((JdbcConnection) database.getConnection()).getUnderlyingConnection();
String sql = "SELECT kcu.column_name FROM information_schema.table_constraints tc"
+ " JOIN information_schema.key_column_usage kcu ON kcu.constraint_name = tc.constraint_name"
+ " AND kcu.table_schema = tc.table_schema AND kcu.table_name = tc.table_name"
+ " WHERE tc.constraint_type = 'PRIMARY KEY' AND tc.table_schema = current_schema() AND lower(tc.table_name) = lower(?)"
+ " ORDER BY kcu.ordinal_position";
try (PreparedStatement ps = connection.prepareStatement(sql)) {
ps.setString(1, tableName);
try (ResultSet rs = ps.executeQuery()) {
List<String> columns = new ArrayList<>();
while (rs.next()) {
columns.add(rs.getString(1));
}
return columns;
}
} catch (SQLException e) {
throw new UnexpectedLiquibaseException("Unable to read primary key of " + tableName, e);
}
}

static Optional<AddPrimaryKeyChange> findAdd(String tableName) {
ChangeSet changeSet = Scope.getCurrentScope().get(Scope.Attr.changeSet, ChangeSet.class);
if (changeSet == null) {
return Optional.empty();
}
return changeSet.getChanges().stream()
.filter(c -> c instanceof AddPrimaryKeyChange add && tableName.equalsIgnoreCase(add.getTableName()))
.map(c -> (AddPrimaryKeyChange) c)
.findFirst();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package org.keycloak.connections.jpa.updater.liquibase.custom;

import java.util.Arrays;
import java.util.regex.Pattern;

import liquibase.database.Database;
import liquibase.database.core.CockroachDatabase;
import liquibase.sql.Sql;
import liquibase.sql.UnparsedSql;
import liquibase.sqlgenerator.SqlGeneratorChain;
import liquibase.sqlgenerator.core.ModifyDataTypeGenerator;
import liquibase.statement.core.ModifyDataTypeStatement;
import liquibase.structure.DatabaseObject;

/**
* The statement generated for PostgreSQL contains a {@code USING} clause, which CockroachDB treats as a rewrite of the column.
* That is not supported for columns that are part of an index, while changing a column to a string type without {@code USING} is a metadata change only.
* Conversions to other types still need the clause.
*/
public class CockroachModifyDataTypeGenerator extends ModifyDataTypeGenerator {

private static final Pattern USING = Pattern.compile("\\s+USING\\s*\\(.*\\)\\s*$", Pattern.DOTALL);

@Override
public int getPriority() {
return PRIORITY_DATABASE + 100;
}

@Override
public boolean supports(ModifyDataTypeStatement statement, Database database) {
return database instanceof CockroachDatabase;
}

@Override
public Sql[] generateSql(ModifyDataTypeStatement statement, Database database, SqlGeneratorChain sqlGeneratorChain) {
Sql[] sql = super.generateSql(statement, database, sqlGeneratorChain);
String type = statement.getNewDataType().toUpperCase();
if (!type.contains("CHAR") && !type.contains("TEXT") && !type.contains("STRING")) {
return sql;
}
return Arrays.stream(sql)
.map(s -> (Sql) new UnparsedSql(USING.matcher(s.toSql()).replaceFirst(""), s.getAffectedDatabaseObjects().toArray(new DatabaseObject[0])))
.toArray(Sql[]::new);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,15 @@
<dbms type="mysql"/>
<dbms type="mariadb"/>
<dbms type="postgresql"/>
<dbms type="cockroachdb"/>
<dbms type="oracle"/>
</or>
</preConditions>
<createIndex tableName="CLIENT_ATTRIBUTES" indexName="IDX_CLIENT_ATT_BY_NAME_VALUE">
<column name="NAME" type="VARCHAR(255)"/>
<column name="VALUE(255)" valueComputed="VALUE(255)" />
</createIndex>
<modifySql dbms="postgresql">
<modifySql dbms="postgresql,cockroachdb">
<replace replace="VALUE(255)" with="(value::varchar(250))" />
</modifySql>
<modifySql dbms="oracle">
Expand All @@ -96,6 +97,7 @@
<dbms type="mysql"/>
<dbms type="mariadb"/>
<dbms type="postgresql"/>
<dbms type="cockroachdb"/>
<dbms type="oracle"/>
</or>
</not>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,15 @@
<dbms type="mysql"/>
<dbms type="mariadb"/>
<dbms type="postgresql"/>
<dbms type="cockroachdb"/>
<dbms type="oracle"/>
</or>
</preConditions>
<createIndex tableName="GROUP_ATTRIBUTE" indexName="IDX_GROUP_ATT_BY_NAME_VALUE">
<column name="NAME" type="VARCHAR(255)"/>
<column name="VALUE(255)" valueComputed="VALUE(255)" />
</createIndex>
<modifySql dbms="postgresql">
<modifySql dbms="postgresql,cockroachdb">
<replace replace="VALUE(255)" with="(value::varchar(250))" />
</modifySql>
<modifySql dbms="oracle">
Expand Down Expand Up @@ -59,6 +60,7 @@
<dbms type="mysql"/>
<dbms type="mariadb"/>
<dbms type="postgresql"/>
<dbms type="cockroachdb"/>
<dbms type="oracle"/>
</or>
</not>
Expand Down
Loading
Loading
0