From 8060e3b3acff9ba2f0a5ff0298344da465a2f1b7 Mon Sep 17 00:00:00 2001 From: Hynek Mlnarik Date: Fri, 30 Oct 2020 21:05:01 +0100 Subject: [PATCH] KEYCLOAK-16115 Remove need for MapStorage.keySet() and values() --- .../models/map/client/MapClientProvider.java | 5 +- .../models/map/group/MapGroupProvider.java | 2 +- .../models/map/role/MapRoleAdapter.java | 5 ++ .../models/map/role/MapRoleProvider.java | 2 +- .../map/storage/MapKeycloakTransaction.java | 87 +++++++++++++------ .../models/map/storage/MapStorage.java | 5 -- 6 files changed, 72 insertions(+), 34 deletions(-) diff --git a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java index a11ee5a26a..1e618c65d6 100644 --- a/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/client/MapClientProvider.java @@ -85,7 +85,7 @@ public class MapClientProvider implements ClientProvider { } private MapClientEntity registerEntityForChanges(MapClientEntity origEntity) { - final MapClientEntity res = Serialization.from(origEntity); + final MapClientEntity res = tx.get(origEntity.getId(), id -> Serialization.from(origEntity)); tx.putIfChanged(origEntity.getId(), res, MapClientEntity::isUpdated); return res; } @@ -96,6 +96,7 @@ public class MapClientProvider implements ClientProvider { return origEntity -> new MapClientAdapter(session, realm, registerEntityForChanges(origEntity)) { @Override public void updateClient() { + LOG.tracef("updateClient(%s)%s", realm, origEntity.getId(), getShortStackTrace()); session.getKeycloakSessionFactory().publish(clientUpdatedEvent(this)); } @@ -143,7 +144,7 @@ public class MapClientProvider implements ClientProvider { Stream updatedAndNotRemovedClientsStream = clientStore.entrySet().stream() .map(tx::getUpdated) // If the client has been removed, tx.get will return null, otherwise it will return me.getValue() .filter(Objects::nonNull); - return Stream.concat(tx.createdValuesStream(clientStore.keySet()), updatedAndNotRemovedClientsStream); + return Stream.concat(tx.createdValuesStream(), updatedAndNotRemovedClientsStream); } @Override diff --git a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java index 091ed5ee4b..2e9f02b29b 100644 --- a/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/group/MapGroupProvider.java @@ -97,7 +97,7 @@ public class MapGroupProvider implements GroupProvider { Stream updatedAndNotRemovedGroupsStream = groupStore.entrySet().stream() .map(tx::getUpdated) // If the group has been removed, tx.get will return null, otherwise it will return me.getValue() .filter(Objects::nonNull); - return Stream.concat(tx.createdValuesStream(groupStore.keySet()), updatedAndNotRemovedGroupsStream); + return Stream.concat(tx.createdValuesStream(), updatedAndNotRemovedGroupsStream); } private Stream getUnsortedGroupEntitiesStream(RealmModel realm) { diff --git a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleAdapter.java b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleAdapter.java index 26b3748a95..17930961ae 100644 --- a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleAdapter.java +++ b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleAdapter.java @@ -133,4 +133,9 @@ public class MapRoleAdapter extends AbstractRoleModel implements return getAttributes().getOrDefault(name, Collections.EMPTY_LIST).stream(); } + @Override + public String toString() { + return "MapRoleAdapter{" + getId() + '}'; + } + } diff --git a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java index 8b4ec1e8d6..5653a5b55a 100644 --- a/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/role/MapRoleProvider.java @@ -100,7 +100,7 @@ public class MapRoleProvider implements RoleProvider { Stream updatedAndNotRemovedRolesStream = roleStore.entrySet().stream() .map(tx::getUpdated) // If the role has been removed, tx.get will return null, otherwise it will return me.getValue() .filter(Objects::nonNull); - return Stream.concat(tx.createdValuesStream(roleStore.keySet()), updatedAndNotRemovedRolesStream) + return Stream.concat(tx.createdValuesStream(), updatedAndNotRemovedRolesStream) .filter(entityRealmFilter(realm)); } diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/MapKeycloakTransaction.java b/model/map/src/main/java/org/keycloak/models/map/storage/MapKeycloakTransaction.java index 55605bd9fd..3fae262cdd 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/MapKeycloakTransaction.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/MapKeycloakTransaction.java @@ -18,7 +18,6 @@ package org.keycloak.models.map.storage; import org.keycloak.models.KeycloakTransaction; -import java.util.Collection; import java.util.LinkedHashMap; import java.util.Map; import java.util.Objects; @@ -32,36 +31,30 @@ public class MapKeycloakTransaction implements KeycloakTransaction { private final static Logger log = Logger.getLogger(MapKeycloakTransaction.class); private enum MapOperation { - PUT { + CREATE { @Override protected MapTaskWithValue taskFor(K key, V value) { return new MapTaskWithValue(value) { - @Override - public void execute(MapStorage map) { - map.put(key, getValue()); - } + @Override public void execute(MapStorage map) { map.putIfAbsent(key, getValue()); } + @Override public MapOperation getOperation() { return CREATE; } }; } }, - PUT_IF_ABSENT { + UPDATE { @Override protected MapTaskWithValue taskFor(K key, V value) { return new MapTaskWithValue(value) { - @Override - public void execute(MapStorage map) { - map.putIfAbsent(key, getValue()); - } + @Override public void execute(MapStorage map) { map.put(key, getValue()); } + @Override public MapOperation getOperation() { return UPDATE; } }; } }, - REMOVE { + DELETE { @Override protected MapTaskWithValue taskFor(K key, V value) { return new MapTaskWithValue(null) { - @Override - public void execute(MapStorage map) { - map.remove(key); - } + @Override public void execute(MapStorage map) { map.remove(key); } + @Override public MapOperation getOperation() { return DELETE; } }; } }, @@ -87,6 +80,8 @@ public class MapKeycloakTransaction implements KeycloakTransaction { @Override public void commit() { + log.trace("Commit"); + if (rollback) { throw new RuntimeException("Rollback only!"); } @@ -146,15 +141,15 @@ public class MapKeycloakTransaction implements KeycloakTransaction { } public void put(K key, V value) { - addTask(MapOperation.PUT, key, value); + addTask(MapOperation.UPDATE, key, value); } public void putIfAbsent(K key, V value) { - addTask(MapOperation.PUT_IF_ABSENT, key, value); + addTask(MapOperation.CREATE, key, value); } public void putIfChanged(K key, V value, Predicate shouldPut) { - log.tracev("Adding operation PUT_IF_CHANGED for {0}", key); + log.tracev("Adding operation UPDATE_IF_CHANGED for {0}", key); K taskKey = key; MapTaskWithValue op = new MapTaskWithValue(value) { @@ -164,12 +159,13 @@ public class MapKeycloakTransaction implements KeycloakTransaction { map.put(key, getValue()); } } + @Override public MapOperation getOperation() { return MapOperation.UPDATE; } }; - tasks.merge(taskKey, op, MapTaskCompose::new); + tasks.merge(taskKey, op, MapKeycloakTransaction::merge); } public void remove(K key) { - addTask(MapOperation.REMOVE, key, null); + addTask(MapOperation.DELETE, key, null); } public Stream valuesStream() { @@ -178,14 +174,22 @@ public class MapKeycloakTransaction implements KeycloakTransaction { .filter(Objects::nonNull); } - public Stream createdValuesStream(Collection existingKeys) { - return this.tasks.entrySet().stream() - .filter(me -> ! existingKeys.contains(me.getKey())) - .map(Map.Entry::getValue) + public Stream createdValuesStream() { + return this.tasks.values().stream() + .filter(v -> v.containsCreate() && ! v.isReplace()) .map(MapTaskWithValue::getValue) .filter(Objects::nonNull); } + private static MapTaskWithValue merge(MapTaskWithValue oldValue, MapTaskWithValue newValue) { + switch (newValue.getOperation()) { + case DELETE: + return oldValue.containsCreate() ? null : newValue; + default: + return new MapTaskCompose<>(oldValue, newValue); + } + } + private static abstract class MapTaskWithValue { protected final V value; @@ -197,6 +201,19 @@ public class MapKeycloakTransaction implements KeycloakTransaction { return value; } + public boolean containsCreate() { + return MapOperation.CREATE == getOperation(); + } + + public boolean containsRemove() { + return MapOperation.DELETE == getOperation(); + } + + public boolean isReplace() { + return false; + } + + public abstract MapOperation getOperation(); public abstract void execute(MapStorage map); } @@ -222,5 +239,25 @@ public class MapKeycloakTransaction implements KeycloakTransaction { return newValue.getValue(); } + @Override + public MapOperation getOperation() { + return null; + } + + @Override + public boolean containsCreate() { + return oldValue.containsCreate() || newValue.containsCreate(); + } + + @Override + public boolean containsRemove() { + return oldValue.containsRemove() || newValue.containsRemove(); + } + + @Override + public boolean isReplace() { + return (newValue.getOperation() == MapOperation.CREATE && oldValue.containsRemove()) || + (oldValue instanceof MapTaskCompose && ((MapTaskCompose) oldValue).isReplace()); + } } } \ No newline at end of file diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java b/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java index b7935f758f..2e25fd4ffe 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/MapStorage.java @@ -16,7 +16,6 @@ */ package org.keycloak.models.map.storage; -import java.util.Collection; import java.util.Map; import java.util.Set; @@ -34,10 +33,6 @@ public interface MapStorage { V remove(K key); - Set keySet(); - Set> entrySet(); - Collection values(); - }