-
Notifications
You must be signed in to change notification settings - Fork 8.3k
Optimize deletion of composite roles #45490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,11 @@ public class CachedGroup extends AbstractRevisioned implements InRealm { | |
| private final String parentId; | ||
| private final LazyLoader<GroupModel, MultivaluedHashMap<String, String>> attributes; | ||
| private final LazyLoader<GroupModel, Set<String>> roleMappings; | ||
| /** | ||
| * Use this so the cache invalidation can retrieve any previously cached role mappings to determine if this | ||
| * items should be evicted. | ||
| */ | ||
| private Set<String> cachedRoleMappings = new HashSet<>(); | ||
| private final LazyLoader<GroupModel, Set<String>> subGroups; | ||
| private final Type type; | ||
|
|
||
|
|
@@ -68,11 +74,16 @@ public MultivaluedHashMap<String, String> getAttributes(KeycloakSession session, | |
| } | ||
|
|
||
| public Set<String> getRoleMappings(KeycloakSession session, Supplier<GroupModel> 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; | ||
| } | ||
|
|
||
| /** | ||
| * 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<String> getCachedRoleMappings() { | ||
| return cachedRoleMappings; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it expected to eventually return an empty set if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated in next commit. |
||
| } | ||
|
|
||
| public String getName() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 <a href="mailto:[email protected]">Bill Burke</a> | ||
|
|
@@ -90,34 +92,42 @@ 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(getEntity(), toRoleEntity(role))) == null) { | ||
| CompositeRoleEntity compositeRoleEntity = new CompositeRoleEntity(getEntity(), toRoleEntity(role)); | ||
| em.persist(compositeRoleEntity); | ||
| } | ||
| getEntity().getCompositeRoles().add(entity); | ||
| } | ||
|
|
||
| @Override | ||
| public void removeCompositeRole(RoleModel role) { | ||
| RoleEntity entity = toRoleEntity(role); | ||
| getEntity().getCompositeRoles().remove(entity); | ||
| RoleEntity child = toRoleEntity(role); | ||
| em.createNamedQuery("deleteSingleCompositeFromRole") | ||
| .setParameter("parentRole", getEntity()) | ||
| .setParameter("childRole", child) | ||
| .executeUpdate(); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<RoleModel> getCompositesStream() { | ||
| Stream<RoleModel> composites = getEntity().getCompositeRoles().stream().map(c -> new RoleAdapter(session, realm, em, c)); | ||
| Stream<RoleModel> composites = getChildRoles().map(c -> new RoleAdapter(session, realm, em, c)); | ||
| return composites.filter(Objects::nonNull); | ||
| } | ||
|
|
||
|
|
||
| private Stream<RoleEntity> getChildRoles() { | ||
| return StreamsUtil.closing(em.createNamedQuery("getChildRoles", RoleEntity.class) | ||
| .setParameter("parentRoleId", getId()).getResultStream()); | ||
| } | ||
|
|
||
| @Override | ||
| public Stream<RoleModel> 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); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| /* | ||
| * 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.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.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="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 | ||
| @ManyToOne(fetch=FetchType.LAZY) | ||
| @JoinColumn(name="COMPOSITE") | ||
| private RoleEntity parentRole; | ||
|
|
||
| @Id | ||
| @ManyToOne(fetch=FetchType.LAZY) | ||
| @JoinColumn(name="CHILD_ROLE") | ||
| private RoleEntity childRole; | ||
|
|
||
| public CompositeRoleEntity() { | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| public RoleEntity getParentRole() { | ||
| return parentRole; | ||
| } | ||
|
|
||
| public void setParentRole(RoleEntity parentRole) { | ||
| this.parentRole = parentRole; | ||
| } | ||
|
|
||
| public RoleEntity getChildRole() { | ||
| return childRole; | ||
| } | ||
|
|
||
| public void setChildRole(RoleEntity childRole) { | ||
| this.childRole = childRole; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null) return false; | ||
| if (!(o instanceof CompositeRoleEntity that)) return false; | ||
|
|
||
| return parentRole.equals(that.parentRole) && childRole.equals(that.childRole); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(childRole, parentRole); | ||
| } | ||
|
|
||
| public static class Key implements Serializable { | ||
| private RoleEntity childRole; | ||
| private RoleEntity parentRole; | ||
|
|
||
| public Key() { | ||
| } | ||
|
|
||
| public Key(RoleEntity parentRole, RoleEntity childRole) { | ||
| this.childRole = childRole; | ||
| this.parentRole = parentRole; | ||
| } | ||
|
|
||
| public RoleEntity getChildRole() { | ||
| return childRole; | ||
| } | ||
|
|
||
| public void setChildRole(RoleEntity childRole) { | ||
| this.childRole = childRole; | ||
| } | ||
|
|
||
| public RoleEntity getParentRole() { | ||
| return 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(childRole, key.childRole) && Objects.equals(parentRole, key.parentRole); | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(childRole, parentRole); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't enough to rely on
roleMappings? Caching is implicit when using aLazyLoader.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in next commit.