Flush auto-created roles during user import to avoid duplicate role inserts in batch mode by wilmerdooley · Pull Request #50056 · keycloak/keycloak

wilmerdooley · 2026-06-17T05:53:04Z

Resolves #49731.

The duplicate-role PK violation happens during user import, not role-definition import. RepresentationToModel.createRoleMappings / createClientRoleMappings auto-create roles that a user's realmRoles / clientRoles reference but that were never declared in roles.realm / roles.client. These run inside EntityManagers.runInBatch, where every query is in FlushMode.COMMIT. When two users reference the same undeclared client role, the second user's getRole cannot see the first user's unflushed addRole, so a duplicate role is created and the end-of-batch flush throws the unique-constraint violation on (NAME, CLIENT_REALM_CONSTRAINT), matching the 'CALL_ACCESS' row in the issue.

Thanks to @sguilhen for tracing this to the real path. The fix follows the suggested approach: in the role == null branch of createRoleMappings / createClientRoleMappings, flush the newly created role when EntityManagers.isBatchMode() so later lookups in the same batch can see it. A KeycloakSession is plumbed into both methods (the single createRoleMappings call site has the session in scope).

I kept the defensive get-or-create change in importRoles from the first version of this PR, since @sguilhen confirmed it is a reasonable improvement to keep.

Changes:

RepresentationToModel.createRoleMappings / createClientRoleMappings: flush an auto-created role in batch mode (plus the EntityManagers import and a session parameter).
DefaultExportImportManager: pass the session into createRoleMappings.
New integration test RealmImportSharedUndefinedClientRoleTest: imports a realm where two users reference the same client role not declared in roles.client, and asserts the import succeeds, exactly one such client role exists, and both users receive the mapping. It reproduces the original KEYCLOAK_ROLE unique-constraint violation without the fix.
The original defensive importRoles change and its unit test are retained.

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR updates role import logic to reuse existing realm/client roles (by name) and consolidate role updates, preventing duplicate role creation and ensuring imported metadata is applied.

Changes:

Replaced unconditional role creation during import with “get-or-create” logic for realm and client roles.
Centralized role updates (description + attributes) into a shared helper.
Added a unit test to verify that importing a client role reuses an existing role and updates its description.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java	Adds get-or-create helpers for realm/client roles and centralizes role field updates during import.
server-spi-private/src/test/java/org/keycloak/models/utils/RepresentationToModelTest.java	Adds a test ensuring client role import reuses an existing role and updates description without creating a new role.

+    private static Object defaultValue(Class<?> returnType) {
+        if (void.class.equals(returnType)) {
+            return null;
+        }
+        if (!returnType.isPrimitive()) {
+            return null;
+        }
+        if (boolean.class.equals(returnType)) {
+            return false;
+        }
+        if (char.class.equals(returnType)) {
+            return '\0';
+        }
+        return 0;
+    }


+    private static RoleModel importRealmRole(RealmModel realm, RoleRepresentation roleRep) {
+        RoleModel role = realm.getRole(roleRep.getName());
+        if (role == null) {
+            role = roleRep.getId() != null ? realm.addRole(roleRep.getId(), roleRep.getName()) : realm.addRole(roleRep.getName());
+        }
+        updateRole(role, roleRep);
+        return role;
+    }
+
+    private static RoleModel importClientRole(ClientModel client, RoleRepresentation roleRep) {
+        RoleModel role = client.getRole(roleRep.getName());
+        if (role == null) {
+            role = roleRep.getId() != null ? client.addRole(roleRep.getId(), roleRep.getName()) : client.addRole(roleRep.getName());
+        }
+        updateRole(role, roleRep);
+        return role;
+    }
+
+    private static void updateRole(RoleModel role, RoleRepresentation roleRep) {
+        role.setDescription(roleRep.getDescription());
+        if (roleRep.getAttributes() != null) {
+            roleRep.getAttributes().forEach(role::setAttribute);
+        }
+    }


SomeDeveloper13 · 2026-06-17T13:10:39Z

+    private static RoleModel importRealmRole(RealmModel realm, RoleRepresentation roleRep) {
+        RoleModel role = realm.getRole(roleRep.getName());
+        if (role == null) {
+            role = roleRep.getId() != null ? realm.addRole(roleRep.getId(), roleRep.getName()) : realm.addRole(roleRep.getName());
+        }
+        updateRole(role, roleRep);
+        return role;
+    }
+
+    private static RoleModel importClientRole(ClientModel client, RoleRepresentation roleRep) {
+        RoleModel role = client.getRole(roleRep.getName());
+        if (role == null) {
+            role = roleRep.getId() != null ? client.addRole(roleRep.getId(), roleRep.getName()) : client.addRole(roleRep.getName());
+        }
+        updateRole(role, roleRep);
+        return role;
+    }


Consolidating this using lambda expressions would add unnecessary cognitive complexity and reduce maintainability.
Additionally, while similar, the realm role and client role logic is distinct enough in source class and intent, that keeping the methods distinct makes sense to me.

Thanks for reporting this and for taking the time to review, @SomeDeveloper13. I agree on keeping importRealmRole and importClientRole as separate helpers rather than folding them into shared lambda-based code; as you noted, the lambda version adds cognitive complexity, and the two paths read from different sources (realm roles vs a client's roles), so keeping them distinct makes the intent clearer.

sguilhen

Thanks for looking into this, @wilmerdooley.

After tracing through the stack trace and the realm JSON from the issue, I believe the fix targets the wrong code path. The PK violation doesn't occur during importRoles, it occurs during user import, which runs inside EntityManagers.runInBatch (the stack trace shows EntityManagers.flush -> runInBatch -> DefaultExportImportManager.importRealm:481).

The issue is specifically in RepresentationToModel.createClientRoleMappings. This method auto-creates client roles that are referenced in user clientRoles but weren't defined in roles.client. Batch mode sets all queries to FlushMode.COMMIT via EntityManagerProxy, which breaks the get-or-create pattern: when two users reference the same undefined client role, the second user's getRole query doesn't see the first user's unflushed persist, causing a duplicate insert at the end-of-batch flush.

The importRoles changes in this PR are a reasonable defensive improvement (handling pre-existing roles from defaultRoles or other sources), but importRoles runs before batch mode starts, where auto-flush works correctly - so these changes don't address the reported crash.

The minimal fix would be to ensure that when createClientRoleMappings (and createRoleMappings for realm roles) auto-creates a missing role inside a batch, the new role is flushed to the database immediately so that subsequent users referencing the same role can see it. This requires plumbing a flush call into the role == null branch, conditional on EntityManagers.isBatchMode().

Regarding the test: the unit test uses in-memory dynamic proxies where getRole is backed by a HashMap, so lookups always see freshly added entries immediately. This can't reproduce the actual bug, which is fundamentally about JPA query visibility under FlushMode.COMMIT in batch mode. An integration test that imports a realm JSON where two users share a client role not defined in roles.client would be needed to exercise the real failure path.

wilmerdooley · 2026-06-24T15:05:41Z

Thanks for the detailed trace, @sguilhen. You're right, I had the fix in the wrong place. importRoles runs before runInBatch, so its get-or-create is covered by normal auto-flush. The duplicate insert comes from the auto-create in createClientRoleMappings (and createRoleMappings) running under FlushMode.COMMIT, where a second user's getRole can't see the first user's unflushed addRole.

I'll re-target to your suggested fix: flush the newly created role in the role == null branch when EntityManagers.isBatchMode(), plumbing a KeycloakSession into the two methods. And I'll replace the unit test with an integration test that imports a realm where two users reference a client role not declared in roles.client, so it exercises the real batch-mode path.

One question on scope: would you prefer I keep the importRoles get-or-create as a defensive change in this PR, or drop it so this PR is just the batch-flush fix? I can propose the defensive part separately if it's useful.

sguilhen · 2026-06-24T17:41:54Z

@wilmerdooley I think it doesn't hurt to have the defensive check you've proposed. Feel free to keep it. And thanks again for volunteering!

Signed-off-by: wilmerdooley <wilmerdooley1@gmail.com>

wilmerdooley · 2026-06-26T22:33:49Z

Thanks again, @sguilhen. I re-targeted the fix to the path you identified: the role == null branch of createRoleMappings / createClientRoleMappings now flushes the auto-created role when EntityManagers.isBatchMode(), with a KeycloakSession plumbed into both methods.

Since you were happy to keep the defensive importRoles change, I kept it along with its unit test, and added a new integration test (RealmImportSharedUndefinedClientRoleTest) for the batch-mode path: it imports a realm where two users reference the same undeclared client role, reproduces the original PK violation on the current code, and passes with the fix. I updated the PR description to match. Happy to adjust anything.

sguilhen · 2026-06-29T18:32:38Z

Thanks for the updated PR, @wilmerdooley - the core fix (flushing auto-created roles in batch mode) looks good to me.

There are two CI failures that need to be addressed before we can merge:

Testsuite Deprecation Check: the integration test RealmImportSharedUndefinedClientRoleTest is added to testsuite/integration-arquillian/tests/base/, which is deprecated and forbids new files. It needs to be moved to the new test framework location. The good news is that the migration is mostly mechanical IMO:

Move the test to tests/base/src/test/java/org/keycloak/tests/exportimport/
Change the package to org.keycloak.tests.exportimport
Replace extends AbstractKeycloakTest + the addTestRealms override with:

@KeycloakIntegrationTest
public class RealmImportSharedUndefinedClientRoleTest {

   @InjectAdminClient
   Keycloak adminClient;

Switch from JUnit 4 to JUnit 5: org.junit.Test -> org.junit.jupiter.api.Test, and org.junit.Assert.* -> org.junit.jupiter.api.Assertions.*
The rest of the test body (realm creation via adminClient.realms().create(realmRep) and assertions) stays essentially the same
Remove the removeRealm helper - just use adminClient.realm(REALM_NAME).remove() directly in the finally block

Spotless: the import ordering in the integration test doesn't match spotless rules. After making the changes above, run mvn spotless:apply to fix formatting.

Let me know if you need any help with the migration!

theohh0 · 2026-07-28T13:47:19Z

Opened #51210 to fix failing checks.

sguilhen · 2026-07-28T14:53:56Z

Closing in favor of #51210 - @wilmerdooley your contribution will be there as well as @theohh0 has cherry picked your commit and worked on top of it.

wilmerdooley requested a review from a team as a code owner June 17, 2026 05:53

Copilot AI review requested due to automatic review settings June 17, 2026 05:53

Copilot AI reviewed Jun 17, 2026

View reviewed changes

keycloak-github-bot Bot added the flaky-test label Jun 17, 2026

keycloak-github-bot Bot mentioned this pull request Jun 17, 2026

Flaky test: org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClient #49985

Closed

wilmerdooley mentioned this pull request Jun 17, 2026

Creating a new realm from json using kcadm gives a PK violation on KEYCLOAK_ROLE despite it only being defined once in the JSON. #49731

Open

2 tasks

stianst added the team/core-iam label Jun 19, 2026

sguilhen reviewed Jun 24, 2026

View reviewed changes

fix: avoid duplicate role creation when importing realm roles

daaa0d0

Signed-off-by: wilmerdooley <wilmerdooley1@gmail.com>

wilmerdooley force-pushed the oss/issue-49731 branch from 91f6b84 to daaa0d0 Compare June 26, 2026 20:18

wilmerdooley requested review from a team as code owners June 26, 2026 20:18

wilmerdooley changed the title ~~fix: avoid duplicate role creation when importing realm roles~~ Flush auto-created roles during user import to avoid duplicate role inserts in batch mode Jun 26, 2026

theohh0 mentioned this pull request Jul 27, 2026

Flush auto-created roles during user import to avoid duplicate role inserts in batch mode #51210

Open

sguilhen closed this Jul 28, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Flush auto-created roles during user import to avoid duplicate role inserts in batch mode - #50056

Flush auto-created roles during user import to avoid duplicate role inserts in batch mode#50056
wilmerdooley wants to merge 1 commit into
keycloak:mainfrom
wilmerdooley:oss/issue-49731

wilmerdooley commented Jun 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

SomeDeveloper13 Jun 17, 2026

Uh oh!

wilmerdooley Jun 17, 2026

Uh oh!

sguilhen left a comment

Uh oh!

wilmerdooley commented Jun 24, 2026

Uh oh!

sguilhen commented Jun 24, 2026

Uh oh!

wilmerdooley commented Jun 26, 2026

Uh oh!

sguilhen commented Jun 29, 2026

Uh oh!

theohh0 commented Jul 28, 2026

Uh oh!

sguilhen commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

wilmerdooley commented Jun 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

SomeDeveloper13 Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

wilmerdooley Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

sguilhen left a comment

Choose a reason for hiding this comment

Uh oh!

wilmerdooley commented Jun 24, 2026

Uh oh!

sguilhen commented Jun 24, 2026

Uh oh!

wilmerdooley commented Jun 26, 2026

Uh oh!

sguilhen commented Jun 29, 2026

Uh oh!

theohh0 commented Jul 28, 2026

Uh oh!

sguilhen commented Jul 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wilmerdooley commented Jun 17, 2026 •

edited

Loading