From b5538e3ab033c75d6f402878a9ea778b771e5ff9 Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Thu, 15 Jan 2026 21:36:10 +0100 Subject: [PATCH 1/4] Optimize deletion of composite roles Closes #45065 Signed-off-by: Alexander Schwartz --- .../models/cache/infinispan/RoleAdapter.java | 4 +- .../infinispan/entities/CachedGroup.java | 13 +- .../cache/infinispan/entities/CachedRole.java | 16 ++- .../infinispan/stream/HasRolePredicate.java | 4 +- .../keycloak/models/jpa/JpaRealmProvider.java | 25 ++-- .../org/keycloak/models/jpa/RoleAdapter.java | 43 ++++-- .../jpa/entities/CompositeRoleEntity.java | 129 ++++++++++++++++++ .../models/jpa/entities/RoleEntity.java | 35 ----- .../main/resources/default-persistence.xml | 1 + 9 files changed, 195 insertions(+), 75 deletions(-) create mode 100755 model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java index 95761e9606f4..5998a4752369 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/RoleAdapter.java @@ -142,7 +142,7 @@ public Stream getCompositesStream() { if (composites == null) { composites = new HashSet<>(); - for (String id : cached.getComposites()) { + for (String id : cached.getComposites(session, modelSupplier)) { RoleModel role = realm.getRoleById(id); if (role == null) { // chance that composite role was removed, so invalidate this entry and fallback to delegate @@ -160,7 +160,7 @@ public Stream getCompositesStream() { public Stream getCompositesStream(String search, Integer first, Integer max) { if (isUpdated()) return updated.getCompositesStream(search, first, max); - return cacheSession.getRoleDelegate().getRolesStream(realm, cached.getComposites().stream(), search, first, max); + return cacheSession.getRoleDelegate().getRolesStream(realm, cached.getComposites(session, modelSupplier).stream(), search, first, max); } @Override diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java index 486641873837..7d2eae88750a 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java @@ -18,6 +18,7 @@ package org.keycloak.models.cache.infinispan.entities; import java.util.Collections; +import java.util.HashSet; import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; @@ -43,6 +44,7 @@ public class CachedGroup extends AbstractRevisioned implements InRealm { private final String parentId; private final LazyLoader> attributes; private final LazyLoader> roleMappings; + private Set cachedRoleMappings = new HashSet<>(); private final LazyLoader> subGroups; private final Type type; @@ -68,11 +70,12 @@ public MultivaluedHashMap getAttributes(KeycloakSession session, } public Set getRoleMappings(KeycloakSession session, Supplier group) { - // it may happen that groups were not loaded before so we don't actually need to invalidate entries in the cache - if (group == null) { - return Collections.emptySet(); - } - return roleMappings.get(session, group); + cachedRoleMappings = roleMappings.get(session, group); + return cachedRoleMappings; + } + + public Set getCachedRoleMappings() { + return cachedRoleMappings; } public String getName() { diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java index fe2c14802bf5..4fb3e3c5d29e 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java @@ -39,7 +39,8 @@ public class CachedRole extends AbstractRevisioned implements InRealm { final protected String realm; final protected String description; final protected boolean composite; - final protected Set composites = new HashSet<>(); + protected LazyLoader> composites; + private Set cachedComposites = new HashSet<>(); private final LazyLoader> attributes; public CachedRole(Long revision, RoleModel model, RealmModel realm) { @@ -49,7 +50,9 @@ public CachedRole(Long revision, RoleModel model, RealmModel realm) { name = model.getName(); this.realm = realm.getId(); if (composite) { - composites.addAll(model.getCompositesStream().map(RoleModel::getId).collect(Collectors.toSet())); + composites = new DefaultLazyLoader<>(roleModel -> roleModel.getCompositesStream().map(RoleModel::getId).collect(Collectors.toSet()), HashSet::new); + } else { + composites = new DefaultLazyLoader<>(roleModel -> new HashSet<>(), HashSet::new); } attributes = new DefaultLazyLoader<>(roleModel -> new MultivaluedHashMap<>(roleModel.getAttributes()), MultivaluedHashMap::new); } @@ -70,8 +73,13 @@ public boolean isComposite() { return composite; } - public Set getComposites() { - return composites; + public Set getComposites(KeycloakSession session, Supplier roleModel) { + cachedComposites = composites.get(session, roleModel); + return cachedComposites; + } + + public Set getCachedComposites() { + return cachedComposites; } public MultivaluedHashMap getAttributes(KeycloakSession session, Supplier roleModel) { diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java index bf8809a7eedd..96846394ec28 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/stream/HasRolePredicate.java @@ -43,8 +43,8 @@ void setRole(String role) { @Override public boolean test(Map.Entry entry) { Object value = entry.getValue(); - return (value instanceof CachedRole cachedRole && cachedRole.getComposites().contains(role)) || - (value instanceof CachedGroup cachedGroup && cachedGroup.getRoleMappings(null, null).contains(role)) || + return (value instanceof CachedRole cachedRole && cachedRole.getCachedComposites().contains(role)) || + (value instanceof CachedGroup cachedGroup && cachedGroup.getCachedRoleMappings().contains(role)) || (value instanceof RoleQuery roleQuery && roleQuery.getRoles().contains(role)) || (value instanceof CachedClient cachedClient && cachedClient.getScope().contains(role)) || (value instanceof CachedClientScope cachedClientScope && cachedClientScope.getScope().contains(role)); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index 0c2717b86dae..b81f1121cdda 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -257,7 +257,7 @@ public RoleModel addRealmRole(RealmModel realm, String id, String name) { entity.setRealmId(realm.getId()); em.persist(entity); em.flush(); - RoleAdapter adapter = new RoleAdapter(session, realm, em, entity); + RoleAdapter adapter = new RoleAdapter(session, realm, em, entity, false); return adapter; } @@ -289,7 +289,7 @@ public RoleModel addClientRole(ClientModel client, String id, String name) { roleEntity.setClientId(client.getId()); roleEntity.setClientRole(true); em.persist(roleEntity); - RoleAdapter adapter = new RoleAdapter(session, client.getRealm(), em, roleEntity); + RoleAdapter adapter = new RoleAdapter(session, client.getRealm(), em, roleEntity, true); return adapter; } @@ -398,7 +398,7 @@ private Stream searchForClientRolesStream(RealmModel realm, Stream new RoleAdapter(session, realm, em, roleEntity)); + .map(roleEntity -> new RoleAdapter(session, realm, em, roleEntity, false)); } @@ -413,7 +413,7 @@ public Stream getClientRolesStream(ClientModel client, Integer first, protected Stream getRolesStream(TypedQuery query, RealmModel realm, Integer first, Integer max) { Stream results = paginateQuery(query, first, max).getResultStream(); - return closing(results.map(role -> new RoleAdapter(session, realm, em, role))); + return closing(results.map(role -> new RoleAdapter(session, realm, em, role, false))); } @Override @@ -435,7 +435,7 @@ protected Stream searchForRoles(TypedQuery query, RealmMo query.setParameter("search", "%" + search.trim().toLowerCase() + "%"); Stream results = paginateQuery(query, first, max).getResultStream(); - return closing(results.map(role -> new RoleAdapter(session, realm, em, role))); + return closing(results.map(role -> new RoleAdapter(session, realm, em, role, false))); } @Override @@ -455,17 +455,8 @@ public boolean removeRole(RoleModel role) { throw new ModelException("Role not found or trying to remove role from incorrect realm"); } - // Can't use a native query to delete the composite roles mappings because it causes TransientObjectException. - // At the same time, can't use the persist cascade type on the compositeRoles field because in that case - // we could not still use a native query as a different problem would arise - it may happen that a parent role that - // has this role as a composite is present in the persistence context. In that case it, the role would be re-created - // again after deletion through persist cascade type. - // So in any case, native query is not an option. This is not optimal as it executes additional queries but - // the alternative of clearing the persistence context is not either as we don't know if something currently present - // in the context is not needed later. - - roleEntity.getCompositeRoles().forEach(childRole -> childRole.getParentRoles().remove(roleEntity)); - roleEntity.getParentRoles().forEach(parentRole -> parentRole.getCompositeRoles().remove(roleEntity)); + em.createNamedQuery("deleteRoleFromComposites").setParameter("roleId", role.getId()) + .executeUpdate(); em.createNamedQuery("deleteClientScopeRoleMappingByRole").setParameter("role", roleEntity).executeUpdate(); @@ -508,7 +499,7 @@ public RoleModel getRoleById(RealmModel realm, String id) { RoleEntity entity = em.find(RoleEntity.class, id); if (entity == null) return null; if (!realm.getId().equals(entity.getRealmId())) return null; - RoleAdapter adapter = new RoleAdapter(session, realm, em, entity); + RoleAdapter adapter = new RoleAdapter(session, realm, em, entity, false); return adapter; } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java index fd9a9db9a152..fcea0440ed52 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java @@ -33,9 +33,11 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.RoleContainerModel; import org.keycloak.models.RoleModel; +import org.keycloak.models.jpa.entities.CompositeRoleEntity; import org.keycloak.models.jpa.entities.RoleAttributeEntity; import org.keycloak.models.jpa.entities.RoleEntity; import org.keycloak.models.utils.KeycloakModelUtils; +import org.keycloak.utils.StreamsUtil; /** * @author Bill Burke @@ -46,12 +48,14 @@ public class RoleAdapter implements RoleModel, JpaModel { protected EntityManager em; protected RealmModel realm; protected KeycloakSession session; + private boolean newEntity; - public RoleAdapter(KeycloakSession session, RealmModel realm, EntityManager em, RoleEntity role) { + public RoleAdapter(KeycloakSession session, RealmModel realm, EntityManager em, RoleEntity role, boolean newEntity) { this.em = em; this.realm = realm; this.role = role; this.session = session; + this.newEntity = newEntity; } @Override @@ -90,34 +94,53 @@ public void setName(String name) { @Override public boolean isComposite() { - return getCompositesStream().count() > 0; + return getChildRoles().findAny().isPresent(); } @Override public void addCompositeRole(RoleModel role) { - RoleEntity entity = toRoleEntity(role); - for (RoleEntity composite : getEntity().getCompositeRoles()) { - if (composite.equals(entity)) return; + if (em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(this.getId(), role.getId())) == null) { + CompositeRoleEntity compositeRoleEntity = new CompositeRoleEntity(getId(), role.getId()); + if (role instanceof RoleAdapter roleAdapter) { + roleAdapter.flushNewEntity(); + } + flushNewEntity(); // ensure that role is stored to the database first + em.persist(compositeRoleEntity); + } + } + + private void flushNewEntity() { + if (newEntity) { + // flush the current role to the database to ensure that we can join with it later + em.createQuery("select 1 from RoleEntity r where r.id = :id").setParameter("id", getId()).getResultList(); + newEntity = false; } - getEntity().getCompositeRoles().add(entity); } @Override public void removeCompositeRole(RoleModel role) { - RoleEntity entity = toRoleEntity(role); - getEntity().getCompositeRoles().remove(entity); + CompositeRoleEntity compositeRoleEntity = em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(this.getId(), role.getId())); + if (compositeRoleEntity != null) { + em.remove(compositeRoleEntity); + } } @Override public Stream getCompositesStream() { - Stream composites = getEntity().getCompositeRoles().stream().map(c -> new RoleAdapter(session, realm, em, c)); + Stream composites = getChildRoles().map(c -> new RoleAdapter(session, realm, em, c, false)); return composites.filter(Objects::nonNull); } + + private Stream getChildRoles() { + return StreamsUtil.closing(em.createNamedQuery("getChildRoles", RoleEntity.class) + .setParameter("parentRoleId", getId()).getResultStream()); + } + @Override public Stream getCompositesStream(String search, Integer first, Integer max) { return session.roles().getRolesStream(realm, - getEntity().getCompositeRoles().stream().map(RoleEntity::getId), + getChildRoles().map(RoleEntity::getId), search, first, max); } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java new file mode 100755 index 000000000000..a61f70e0de7e --- /dev/null +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java @@ -0,0 +1,129 @@ +/* + * Copyright 2026 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.models.jpa.entities; + +import java.io.Serializable; +import java.util.Objects; + +import jakarta.persistence.Column; +import jakarta.persistence.Entity; +import jakarta.persistence.Id; +import jakarta.persistence.IdClass; +import jakarta.persistence.NamedQueries; +import jakarta.persistence.NamedQuery; +import jakarta.persistence.Table; + +/** + * Manage compmosite role relations. + * This used to be a @ManyToMany relation in RoleEntity, and before that there was a native query which lead to stale entities. + * After those attempts, this is now a separate table that avoids iterating over a lot of parents their entries by applying a simple JPA deletion. + */ +@Entity +@Table(name="COMPOSITE_ROLE") +@NamedQueries({ + @NamedQuery(name="getChildRoles", query="select r from CompositeRoleEntity c join RoleEntity r on r.id = c.childRoleId where c.parentRoleId = :parentRoleId"), + @NamedQuery(name="deleteRoleFromComposites", query="delete CompositeRoleEntity c where c.parentRoleId = :roleId or c.childRoleId = :roleId"), +}) +@IdClass(CompositeRoleEntity.Key.class) +public class CompositeRoleEntity { + @Id + @Column(name="COMPOSITE", length = 36) + private String parentRoleId; + + @Id + @Column(name="CHILD_ROLE", length = 36) + private String childRoleId; + + public CompositeRoleEntity() { + } + + public CompositeRoleEntity(String parentRoleId, String childRoleId) { + this.childRoleId = childRoleId; + this.parentRoleId = parentRoleId; + } + + public String getParentRoleId() { + return parentRoleId; + } + + public void setParentRoleId(String parentRoleId) { + this.parentRoleId = parentRoleId; + } + + public String getChildRoleId() { + return childRoleId; + } + + public void setChildRoleId(String childRoleId) { + this.childRoleId = childRoleId; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null) return false; + if (!(o instanceof CompositeRoleEntity that)) return false; + + return parentRoleId.equals(that.parentRoleId) && childRoleId.equals(that.childRoleId); + } + + @Override + public int hashCode() { + return Objects.hash(childRoleId, parentRoleId); + } + + public static class Key implements Serializable { + private String childRoleId; + private String parentRoleId; + + public Key() { + } + + public Key(String parentRoleId, String childRoleId) { + this.childRoleId = childRoleId; + this.parentRoleId = parentRoleId; + } + + public String getChildRoleId() { + return childRoleId; + } + + public void setChildRoleId(String childRoleId) { + this.childRoleId = childRoleId; + } + + public String getParentRoleId() { + return parentRoleId; + } + + public void setParentRoleId(String parentRole) { + this.parentRoleId = parentRole; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof Key key)) return false; + return Objects.equals(childRoleId, key.childRoleId) && Objects.equals(parentRoleId, key.parentRoleId); + } + + @Override + public int hashCode() { + return Objects.hash(childRoleId, parentRoleId); + } + } +} diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java index 729b5f4a46f1..6a04555f7eca 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java @@ -17,21 +17,15 @@ package org.keycloak.models.jpa.entities; -import java.util.HashSet; import java.util.LinkedList; import java.util.List; -import java.util.Set; import jakarta.persistence.Access; import jakarta.persistence.AccessType; import jakarta.persistence.CascadeType; import jakarta.persistence.Column; import jakarta.persistence.Entity; -import jakarta.persistence.FetchType; import jakarta.persistence.Id; -import jakarta.persistence.JoinColumn; -import jakarta.persistence.JoinTable; -import jakarta.persistence.ManyToMany; import jakarta.persistence.NamedQueries; import jakarta.persistence.NamedQuery; import jakarta.persistence.OneToMany; @@ -95,13 +89,6 @@ public class RoleEntity { @Column(name="CLIENT_REALM_CONSTRAINT", length = 36) private String clientRealmConstraint; - @ManyToMany(fetch = FetchType.LAZY, cascade = {}) - @JoinTable(name = "COMPOSITE_ROLE", joinColumns = @JoinColumn(name = "COMPOSITE"), inverseJoinColumns = @JoinColumn(name = "CHILD_ROLE")) - private Set compositeRoles; - - @ManyToMany(mappedBy = "compositeRoles", fetch = FetchType.LAZY, cascade = {}) - private Set parentRoles; - // Explicitly not using OrphanRemoval as we're handling the removal manually through HQL but at the same time we still // want to remove elements from the entity's collection in a manual way. Without this, Hibernate would do a duplicit // delete query. @@ -154,28 +141,6 @@ public void setDescription(String description) { this.description = description; } - public Set getCompositeRoles() { - if (compositeRoles == null) { - compositeRoles = new HashSet<>(); - } - return compositeRoles; - } - - public Set getParentRoles() { - if (parentRoles == null) { - parentRoles = new HashSet<>(); - } - return parentRoles; - } - - public void setParentRoles(Set parentRoles) { - this.parentRoles = parentRoles; - } - - public void setCompositeRoles(Set compositeRoles) { - this.compositeRoles = compositeRoles; - } - public boolean isClientRole() { return clientRole; } diff --git a/model/jpa/src/main/resources/default-persistence.xml b/model/jpa/src/main/resources/default-persistence.xml index 79fdd51d7540..0f6b6c552cfb 100644 --- a/model/jpa/src/main/resources/default-persistence.xml +++ b/model/jpa/src/main/resources/default-persistence.xml @@ -31,6 +31,7 @@ org.keycloak.models.jpa.entities.UserFederationProviderEntity org.keycloak.models.jpa.entities.UserFederationMapperEntity org.keycloak.models.jpa.entities.RoleEntity + org.keycloak.models.jpa.entities.CompositeRoleEntity org.keycloak.models.jpa.entities.RoleAttributeEntity org.keycloak.models.jpa.entities.FederatedIdentityEntity org.keycloak.models.jpa.entities.MigrationModelEntity From 8c583a824495011620aaa1b89e21392fc542b89a Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Thu, 22 Jan 2026 16:24:25 +0100 Subject: [PATCH 2/4] Removing workaround after showing dependency to Hibernate Signed-off-by: Alexander Schwartz --- .../keycloak/models/jpa/JpaRealmProvider.java | 12 ++++---- .../org/keycloak/models/jpa/RoleAdapter.java | 28 +++++-------------- .../jpa/entities/CompositeRoleEntity.java | 26 ++++++++++++++--- .../models/jpa/entities/RoleEntity.java | 1 + .../main/resources/default-persistence.xml | 2 +- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index b81f1121cdda..faa7d8d61cd4 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -257,7 +257,7 @@ public RoleModel addRealmRole(RealmModel realm, String id, String name) { entity.setRealmId(realm.getId()); em.persist(entity); em.flush(); - RoleAdapter adapter = new RoleAdapter(session, realm, em, entity, false); + RoleAdapter adapter = new RoleAdapter(session, realm, em, entity); return adapter; } @@ -289,7 +289,7 @@ public RoleModel addClientRole(ClientModel client, String id, String name) { roleEntity.setClientId(client.getId()); roleEntity.setClientRole(true); em.persist(roleEntity); - RoleAdapter adapter = new RoleAdapter(session, client.getRealm(), em, roleEntity, true); + RoleAdapter adapter = new RoleAdapter(session, client.getRealm(), em, roleEntity); return adapter; } @@ -398,7 +398,7 @@ private Stream searchForClientRolesStream(RealmModel realm, Stream new RoleAdapter(session, realm, em, roleEntity, false)); + .map(roleEntity -> new RoleAdapter(session, realm, em, roleEntity)); } @@ -413,7 +413,7 @@ public Stream getClientRolesStream(ClientModel client, Integer first, protected Stream getRolesStream(TypedQuery query, RealmModel realm, Integer first, Integer max) { Stream results = paginateQuery(query, first, max).getResultStream(); - return closing(results.map(role -> new RoleAdapter(session, realm, em, role, false))); + return closing(results.map(role -> new RoleAdapter(session, realm, em, role))); } @Override @@ -435,7 +435,7 @@ protected Stream searchForRoles(TypedQuery query, RealmMo query.setParameter("search", "%" + search.trim().toLowerCase() + "%"); Stream results = paginateQuery(query, first, max).getResultStream(); - return closing(results.map(role -> new RoleAdapter(session, realm, em, role, false))); + return closing(results.map(role -> new RoleAdapter(session, realm, em, role))); } @Override @@ -499,7 +499,7 @@ public RoleModel getRoleById(RealmModel realm, String id) { RoleEntity entity = em.find(RoleEntity.class, id); if (entity == null) return null; if (!realm.getId().equals(entity.getRealmId())) return null; - RoleAdapter adapter = new RoleAdapter(session, realm, em, entity, false); + RoleAdapter adapter = new RoleAdapter(session, realm, em, entity); return adapter; } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java index fcea0440ed52..16af35a58c20 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java @@ -48,14 +48,12 @@ public class RoleAdapter implements RoleModel, JpaModel { protected EntityManager em; protected RealmModel realm; protected KeycloakSession session; - private boolean newEntity; - public RoleAdapter(KeycloakSession session, RealmModel realm, EntityManager em, RoleEntity role, boolean newEntity) { + public RoleAdapter(KeycloakSession session, RealmModel realm, EntityManager em, RoleEntity role) { this.em = em; this.realm = realm; this.role = role; this.session = session; - this.newEntity = newEntity; } @Override @@ -100,34 +98,22 @@ public boolean isComposite() { @Override public void addCompositeRole(RoleModel role) { if (em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(this.getId(), role.getId())) == null) { - CompositeRoleEntity compositeRoleEntity = new CompositeRoleEntity(getId(), role.getId()); - if (role instanceof RoleAdapter roleAdapter) { - roleAdapter.flushNewEntity(); - } - flushNewEntity(); // ensure that role is stored to the database first + CompositeRoleEntity compositeRoleEntity = new CompositeRoleEntity(getEntity(), toRoleEntity(role)); em.persist(compositeRoleEntity); } } - private void flushNewEntity() { - if (newEntity) { - // flush the current role to the database to ensure that we can join with it later - em.createQuery("select 1 from RoleEntity r where r.id = :id").setParameter("id", getId()).getResultList(); - newEntity = false; - } - } - @Override public void removeCompositeRole(RoleModel role) { - CompositeRoleEntity compositeRoleEntity = em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(this.getId(), role.getId())); - if (compositeRoleEntity != null) { - em.remove(compositeRoleEntity); - } + em.createNamedQuery("deleteSingleCompositeFromRole") + .setParameter("parentRoleId", this.role.getId()) + .setParameter("childRoleId", role.getId()) + .executeUpdate(); } @Override public Stream getCompositesStream() { - Stream composites = getChildRoles().map(c -> new RoleAdapter(session, realm, em, c, false)); + Stream composites = getChildRoles().map(c -> new RoleAdapter(session, realm, em, c)); return composites.filter(Objects::nonNull); } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java index a61f70e0de7e..959601d6f4ed 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java @@ -22,10 +22,13 @@ import jakarta.persistence.Column; import jakarta.persistence.Entity; +import jakarta.persistence.FetchType; import jakarta.persistence.Id; import jakarta.persistence.IdClass; +import jakarta.persistence.JoinColumn; import jakarta.persistence.NamedQueries; import jakarta.persistence.NamedQuery; +import jakarta.persistence.OneToOne; import jakarta.persistence.Table; /** @@ -36,8 +39,8 @@ @Entity @Table(name="COMPOSITE_ROLE") @NamedQueries({ - @NamedQuery(name="getChildRoles", query="select r from CompositeRoleEntity c join RoleEntity r on r.id = c.childRoleId where c.parentRoleId = :parentRoleId"), @NamedQuery(name="deleteRoleFromComposites", query="delete CompositeRoleEntity c where c.parentRoleId = :roleId or c.childRoleId = :roleId"), + @NamedQuery(name="deleteSingleCompositeFromRole", query="delete CompositeRoleEntity c where c.parentRoleId = :parentRoleId and c.childRoleId = :childRoleId"), }) @IdClass(CompositeRoleEntity.Key.class) public class CompositeRoleEntity { @@ -49,12 +52,27 @@ public class CompositeRoleEntity { @Column(name="CHILD_ROLE", length = 36) private String childRoleId; + /* Although this field seems not to be used, it is important for Hibernate to figure out the dependencies when + sorting the SQL statements for inserting and deleting. Otherwise, we might see primary key violations. */ + @OneToOne(fetch = FetchType.LAZY) + @JoinColumn(name = "COMPOSITE", updatable = false, insertable = false) + RoleEntity parentRole; + + /* Although this field seems not to be used, it is important for Hibernate to figure out the dependencies when + sorting the SQL statements for inserting and deleting. Otherwise, we might see primary key violations. */ + @OneToOne(fetch = FetchType.LAZY) + @JoinColumn(name = "CHILD_ROLE", updatable = false, insertable = false) + RoleEntity childRole; + public CompositeRoleEntity() { } - public CompositeRoleEntity(String parentRoleId, String childRoleId) { - this.childRoleId = childRoleId; - this.parentRoleId = parentRoleId; + public CompositeRoleEntity(RoleEntity parentRole, RoleEntity childRole) { + // Fields must not be null otherwise the automatic dependency detection of Hibernate will not work + this.parentRole = parentRole; + this.childRole = childRole; + this.childRoleId = childRole.getId(); + this.parentRoleId = parentRole.getId(); } public String getParentRoleId() { diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java index 6a04555f7eca..04712f474409 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java @@ -60,6 +60,7 @@ @NamedQuery(name="searchForRealmRoles", query="select role from RoleEntity role where role.clientRole = false and role.realmId = :realm and ( lower(role.name) like :search or lower(role.description) like :search ) order by role.name"), @NamedQuery(name="getRoleIdsFromIdList", query="select role.id from RoleEntity role where role.realmId = :realm and role.id in :ids order by role.name ASC"), @NamedQuery(name="getRoleIdsByNameContainingFromIdList", query="select role.id from RoleEntity role where role.realmId = :realm and lower(role.name) like lower(concat('%',:search,'%')) and role.id in :ids order by role.name ASC"), + @NamedQuery(name="getChildRoles", query="select r from RoleEntity r join CompositeRoleEntity c on r.id = c.childRoleId where c.parentRoleId = :parentRoleId"), }) public class RoleEntity { diff --git a/model/jpa/src/main/resources/default-persistence.xml b/model/jpa/src/main/resources/default-persistence.xml index 0f6b6c552cfb..16b08554e394 100644 --- a/model/jpa/src/main/resources/default-persistence.xml +++ b/model/jpa/src/main/resources/default-persistence.xml @@ -30,8 +30,8 @@ org.keycloak.models.jpa.entities.ComponentEntity org.keycloak.models.jpa.entities.UserFederationProviderEntity org.keycloak.models.jpa.entities.UserFederationMapperEntity - org.keycloak.models.jpa.entities.RoleEntity org.keycloak.models.jpa.entities.CompositeRoleEntity + org.keycloak.models.jpa.entities.RoleEntity org.keycloak.models.jpa.entities.RoleAttributeEntity org.keycloak.models.jpa.entities.FederatedIdentityEntity org.keycloak.models.jpa.entities.MigrationModelEntity From 218089f914da446e2fbbffce9bf1aef78d002154 Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Thu, 22 Jan 2026 19:29:44 +0100 Subject: [PATCH 3/4] More cleanup Signed-off-by: Alexander Schwartz --- .../keycloak/models/jpa/JpaRealmProvider.java | 2 +- .../org/keycloak/models/jpa/RoleAdapter.java | 7 +- .../jpa/entities/CompositeRoleEntity.java | 81 ++++++++----------- .../models/jpa/entities/RoleEntity.java | 2 +- 4 files changed, 40 insertions(+), 52 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java index faa7d8d61cd4..ff8e6467eff0 100644 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/JpaRealmProvider.java @@ -455,7 +455,7 @@ public boolean removeRole(RoleModel role) { throw new ModelException("Role not found or trying to remove role from incorrect realm"); } - em.createNamedQuery("deleteRoleFromComposites").setParameter("roleId", role.getId()) + em.createNamedQuery("deleteRoleFromComposites").setParameter("role", roleEntity) .executeUpdate(); em.createNamedQuery("deleteClientScopeRoleMappingByRole").setParameter("role", roleEntity).executeUpdate(); diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java index 16af35a58c20..baa1123a5701 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/RoleAdapter.java @@ -97,7 +97,7 @@ public boolean isComposite() { @Override public void addCompositeRole(RoleModel role) { - if (em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(this.getId(), role.getId())) == null) { + if (em.find(CompositeRoleEntity.class, new CompositeRoleEntity.Key(getEntity(), toRoleEntity(role))) == null) { CompositeRoleEntity compositeRoleEntity = new CompositeRoleEntity(getEntity(), toRoleEntity(role)); em.persist(compositeRoleEntity); } @@ -105,9 +105,10 @@ public void addCompositeRole(RoleModel role) { @Override public void removeCompositeRole(RoleModel role) { + RoleEntity child = toRoleEntity(role); em.createNamedQuery("deleteSingleCompositeFromRole") - .setParameter("parentRoleId", this.role.getId()) - .setParameter("childRoleId", role.getId()) + .setParameter("parentRole", getEntity()) + .setParameter("childRole", child) .executeUpdate(); } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java index 959601d6f4ed..eec8d0c6cb00 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/CompositeRoleEntity.java @@ -20,15 +20,14 @@ import java.io.Serializable; import java.util.Objects; -import jakarta.persistence.Column; import jakarta.persistence.Entity; import jakarta.persistence.FetchType; import jakarta.persistence.Id; import jakarta.persistence.IdClass; import jakarta.persistence.JoinColumn; +import jakarta.persistence.ManyToOne; import jakarta.persistence.NamedQueries; import jakarta.persistence.NamedQuery; -import jakarta.persistence.OneToOne; import jakarta.persistence.Table; /** @@ -39,30 +38,20 @@ @Entity @Table(name="COMPOSITE_ROLE") @NamedQueries({ - @NamedQuery(name="deleteRoleFromComposites", query="delete CompositeRoleEntity c where c.parentRoleId = :roleId or c.childRoleId = :roleId"), - @NamedQuery(name="deleteSingleCompositeFromRole", query="delete CompositeRoleEntity c where c.parentRoleId = :parentRoleId and c.childRoleId = :childRoleId"), + @NamedQuery(name="deleteRoleFromComposites", query="delete CompositeRoleEntity c where c.parentRole = :role or c.childRole = :role"), + @NamedQuery(name="deleteSingleCompositeFromRole", query="delete CompositeRoleEntity c where c.parentRole = :parentRole and c.childRole = :childRole"), }) @IdClass(CompositeRoleEntity.Key.class) public class CompositeRoleEntity { @Id - @Column(name="COMPOSITE", length = 36) - private String parentRoleId; + @ManyToOne(fetch=FetchType.LAZY) + @JoinColumn(name="COMPOSITE") + private RoleEntity parentRole; @Id - @Column(name="CHILD_ROLE", length = 36) - private String childRoleId; - - /* Although this field seems not to be used, it is important for Hibernate to figure out the dependencies when - sorting the SQL statements for inserting and deleting. Otherwise, we might see primary key violations. */ - @OneToOne(fetch = FetchType.LAZY) - @JoinColumn(name = "COMPOSITE", updatable = false, insertable = false) - RoleEntity parentRole; - - /* Although this field seems not to be used, it is important for Hibernate to figure out the dependencies when - sorting the SQL statements for inserting and deleting. Otherwise, we might see primary key violations. */ - @OneToOne(fetch = FetchType.LAZY) - @JoinColumn(name = "CHILD_ROLE", updatable = false, insertable = false) - RoleEntity childRole; + @ManyToOne(fetch=FetchType.LAZY) + @JoinColumn(name="CHILD_ROLE") + private RoleEntity childRole; public CompositeRoleEntity() { } @@ -71,24 +60,22 @@ public CompositeRoleEntity(RoleEntity parentRole, RoleEntity childRole) { // Fields must not be null otherwise the automatic dependency detection of Hibernate will not work this.parentRole = parentRole; this.childRole = childRole; - this.childRoleId = childRole.getId(); - this.parentRoleId = parentRole.getId(); } - public String getParentRoleId() { - return parentRoleId; + public RoleEntity getParentRole() { + return parentRole; } - public void setParentRoleId(String parentRoleId) { - this.parentRoleId = parentRoleId; + public void setParentRole(RoleEntity parentRole) { + this.parentRole = parentRole; } - public String getChildRoleId() { - return childRoleId; + public RoleEntity getChildRole() { + return childRole; } - public void setChildRoleId(String childRoleId) { - this.childRoleId = childRoleId; + public void setChildRole(RoleEntity childRole) { + this.childRole = childRole; } @Override @@ -97,51 +84,51 @@ public boolean equals(Object o) { if (o == null) return false; if (!(o instanceof CompositeRoleEntity that)) return false; - return parentRoleId.equals(that.parentRoleId) && childRoleId.equals(that.childRoleId); + return parentRole.equals(that.parentRole) && childRole.equals(that.childRole); } @Override public int hashCode() { - return Objects.hash(childRoleId, parentRoleId); + return Objects.hash(childRole, parentRole); } public static class Key implements Serializable { - private String childRoleId; - private String parentRoleId; + private RoleEntity childRole; + private RoleEntity parentRole; public Key() { } - public Key(String parentRoleId, String childRoleId) { - this.childRoleId = childRoleId; - this.parentRoleId = parentRoleId; + public Key(RoleEntity parentRole, RoleEntity childRole) { + this.childRole = childRole; + this.parentRole = parentRole; } - public String getChildRoleId() { - return childRoleId; + public RoleEntity getChildRole() { + return childRole; } - public void setChildRoleId(String childRoleId) { - this.childRoleId = childRoleId; + public void setChildRole(RoleEntity childRole) { + this.childRole = childRole; } - public String getParentRoleId() { - return parentRoleId; + public RoleEntity getParentRole() { + return parentRole; } - public void setParentRoleId(String parentRole) { - this.parentRoleId = parentRole; + public void setParentRole(RoleEntity parentRole) { + this.parentRole = parentRole; } @Override public boolean equals(Object o) { if (!(o instanceof Key key)) return false; - return Objects.equals(childRoleId, key.childRoleId) && Objects.equals(parentRoleId, key.parentRoleId); + return Objects.equals(childRole, key.childRole) && Objects.equals(parentRole, key.parentRole); } @Override public int hashCode() { - return Objects.hash(childRoleId, parentRoleId); + return Objects.hash(childRole, parentRole); } } } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java index 04712f474409..2b2921242514 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/entities/RoleEntity.java @@ -60,7 +60,7 @@ @NamedQuery(name="searchForRealmRoles", query="select role from RoleEntity role where role.clientRole = false and role.realmId = :realm and ( lower(role.name) like :search or lower(role.description) like :search ) order by role.name"), @NamedQuery(name="getRoleIdsFromIdList", query="select role.id from RoleEntity role where role.realmId = :realm and role.id in :ids order by role.name ASC"), @NamedQuery(name="getRoleIdsByNameContainingFromIdList", query="select role.id from RoleEntity role where role.realmId = :realm and lower(role.name) like lower(concat('%',:search,'%')) and role.id in :ids order by role.name ASC"), - @NamedQuery(name="getChildRoles", query="select r from RoleEntity r join CompositeRoleEntity c on r.id = c.childRoleId where c.parentRoleId = :parentRoleId"), + @NamedQuery(name="getChildRoles", query="select r from RoleEntity r join CompositeRoleEntity c on r.id = c.childRole.id where c.parentRole.id = :parentRoleId"), }) public class RoleEntity { From 4ce96607df236c260c335fa80d330e0ceac76d7c Mon Sep 17 00:00:00 2001 From: Alexander Schwartz Date: Fri, 23 Jan 2026 18:34:03 +0100 Subject: [PATCH 4/4] Rework as of review Signed-off-by: Alexander Schwartz --- .../cache/infinispan/entities/CachedGroup.java | 8 ++++++++ .../cache/infinispan/entities/CachedRole.java | 13 +++++++++++-- .../tests/admin/realm/RealmRolesCRUDTest.java | 8 ++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java index 7d2eae88750a..7afbd95e0916 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedGroup.java @@ -44,6 +44,10 @@ public class CachedGroup extends AbstractRevisioned implements InRealm { private final String parentId; private final LazyLoader> attributes; private final LazyLoader> roleMappings; + /** + * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this + * items should be evicted. + */ private Set cachedRoleMappings = new HashSet<>(); private final LazyLoader> subGroups; private final Type type; @@ -74,6 +78,10 @@ public Set getRoleMappings(KeycloakSession session, Supplier return cachedRoleMappings; } + /** + * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this + * items should be evicted. Will return an empty list if it hasn't been cached yet (and then no invalidation is necessary) + */ public Set getCachedRoleMappings() { return cachedRoleMappings; } diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java index 4fb3e3c5d29e..e912ee9e1245 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/entities/CachedRole.java @@ -38,8 +38,12 @@ public class CachedRole extends AbstractRevisioned implements InRealm { final protected String name; final protected String realm; final protected String description; - final protected boolean composite; - protected LazyLoader> composites; + protected boolean composite; + final protected LazyLoader> composites; + /** + * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this + * items should be evicted. + */ private Set cachedComposites = new HashSet<>(); private final LazyLoader> attributes; @@ -75,9 +79,14 @@ public boolean isComposite() { public Set getComposites(KeycloakSession session, Supplier roleModel) { cachedComposites = composites.get(session, roleModel); + composite = !cachedComposites.isEmpty(); return cachedComposites; } + /** + * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this + * items should be evicted. Will return an empty list if it hasn't been cached yet (and then no invalidation is necessary) + */ public Set getCachedComposites() { return cachedComposites; } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/realm/RealmRolesCRUDTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/realm/RealmRolesCRUDTest.java index ed3d52fbe955..00c8263fd6f4 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/realm/RealmRolesCRUDTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/realm/RealmRolesCRUDTest.java @@ -130,6 +130,14 @@ public void composites() { assertFalse(managedRealm.admin().roles().get("role-a").toRepresentation().isComposite()); assertEquals(0, managedRealm.admin().roles().get("role-a").getRoleComposites().size()); + + managedRealm.admin().roles().create(RoleConfigBuilder.create().name("role-z").build()); + managedRealm.admin().roles().get("role-z").addComposites(l); + // show that I can delete a role that has composite roles + managedRealm.admin().roles().deleteRole("role-z"); + // show that the roles still exist + assertNotNull(managedRealm.admin().roles().get("role-b").toRepresentation().getId()); + assertNotNull(managedRealm.admin().clients().get(clientA.getId()).roles().get("role-c").toRepresentation().getId()); } @Test