From edb292664c32092184ec65491060298f2b6374c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hynek=20Mlna=C5=99=C3=ADk?= Date: Tue, 16 May 2023 12:03:59 +0200 Subject: [PATCH] File store freeze * File store: Fix ID determination * Forbid changing ID (other setters) * Improve handling of null values * Support convertible keys in maps * Fix writing empty values * Fix updated flag * Proceed if an object has been deleted in the same tx * Fix condition Co-authored-by: Michal Hajas --------- Co-authored-by: Michal Hajas --- ...enerateEntityImplementationsProcessor.java | 12 ++ .../map/storage/file/FileCrudOperations.java | 54 +++++---- .../map/storage/file/FileMapStorage.java | 62 ++++++++-- .../storage/file/FileMapStorageProvider.java | 2 +- .../file/FileMapStorageProviderFactory.java | 6 +- .../map/storage/file/common/BlockContext.java | 2 +- .../storage/file/common/MapEntityContext.java | 5 + .../map/storage/file/yaml/YamlParser.java | 21 +++- .../file/yaml/YamlWritingMechanism.java | 11 +- .../hotRod/HotRodMapStorageProvider.java | 2 +- .../AllAreasHotRodStoresWrapper.java | 6 +- .../HotRodUserSessionMapStorage.java | 7 +- .../models/map/common/UpdatableEntity.java | 11 ++ .../common/delegate/EntityFieldDelegate.java | 10 ++ .../models/map/storage/CrudOperations.java | 2 +- .../storage/chm/ConcurrentHashMapStorage.java | 107 ++++++++++++++++-- .../chm/ConcurrentHashMapStorageProvider.java | 2 +- ...ncurrentHashMapStorageProviderFactory.java | 12 +- .../chm/SingleUseObjectMapStorage.java | 2 +- .../models/map/user/MapUserEntity.java | 2 +- .../model/ConcurrentHashMapStorageTest.java | 14 +-- 21 files changed, 282 insertions(+), 70 deletions(-) diff --git a/model/build-processor/src/main/java/org/keycloak/models/map/processor/GenerateEntityImplementationsProcessor.java b/model/build-processor/src/main/java/org/keycloak/models/map/processor/GenerateEntityImplementationsProcessor.java index 0d0c5fa08e..0cf154d86e 100644 --- a/model/build-processor/src/main/java/org/keycloak/models/map/processor/GenerateEntityImplementationsProcessor.java +++ b/model/build-processor/src/main/java/org/keycloak/models/map/processor/GenerateEntityImplementationsProcessor.java @@ -575,10 +575,18 @@ public class GenerateEntityImplementationsProcessor extends AbstractGenerateEnti pw.println(" return entityFieldDelegate.isUpdated();"); pw.println(" }"); + pw.println(" @Override public void markUpdatedFlag() {"); + pw.println(" entityFieldDelegate.markUpdatedFlag();"); + pw.println(" }"); + pw.println(" @Override public void clearUpdatedFlag() {"); pw.println(" entityFieldDelegate.clearUpdatedFlag();"); pw.println(" }"); + pw.println(" @Override public String toString() {"); + pw.println(" return \"%\" + String.valueOf(entityFieldDelegate);"); + pw.println(" }"); + getAllAbstractMethods(e) .forEach(ee -> { String originalField = m2field.get(ee); @@ -701,6 +709,10 @@ public class GenerateEntityImplementationsProcessor extends AbstractGenerateEnti pw.println(" return this.delegateProvider;"); pw.println(" }"); + pw.println(" @Override public String toString() {"); + pw.println(" return \"/\" + String.valueOf(this.delegateProvider);"); + pw.println(" }"); + getAllAbstractMethods(e) .forEach(ee -> { printMethodHeader(pw, ee); diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileCrudOperations.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileCrudOperations.java index 8cb3081fed..46ea553fe6 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileCrudOperations.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileCrudOperations.java @@ -25,6 +25,7 @@ import org.keycloak.models.map.common.AbstractEntity; import org.keycloak.models.map.common.ExpirationUtils; import org.keycloak.models.map.common.HasRealmId; import org.keycloak.models.map.common.StringKeyConverter; +import org.keycloak.models.map.common.StringKeyConverter.StringKey; import org.keycloak.models.map.common.UpdatableEntity; import org.keycloak.models.map.realm.MapRealmEntity; import org.keycloak.models.map.storage.ModelEntityUtil; @@ -136,7 +137,7 @@ public abstract class FileCrudOperations\"/\\\\|?*=]"); - private static final String ID_COMPONENT_SEPARATOR = ":"; + public static final String ID_COMPONENT_SEPARATOR = ":"; private static final String ESCAPING_CHARACTER = "="; private static final Pattern ID_COMPONENT_SEPARATOR_PATTERN = Pattern.compile(Pattern.quote(ID_COMPONENT_SEPARATOR) + "+"); @@ -184,11 +185,11 @@ public abstract class FileCrudOperations dirStream = Files.walk(dataDirectory, entityClass == MapRealmEntity.class ? 1 : 2)) { + try (Stream dirStream = Files.walk(dataDirectory, entityClass == MapRealmEntity.class ? 1 : 3)) { // The paths list has to be materialized first, otherwise "dirStream" would be closed // before the resulting stream would be read and would return empty result paths = dirStream.collect(Collectors.toList()); @@ -396,7 +408,7 @@ public abstract class FileCrudOperationsStefan Guilhen */ public class FileMapStorage - extends ConcurrentHashMapStorage { + extends ConcurrentHashMapStorage> { private static final Logger LOG = Logger.getLogger(FileMapStorage.class); private final List createdPaths = new LinkedList<>(); private final List pathsToDelete = new LinkedList<>(); - private final Map renameOnCommit = new HashMap<>(); + private final Map renameOnCommit = new LinkedHashMap<>(); private final Map lastModified = new HashMap<>(); private final String txId = StringKey.INSTANCE.yieldNewUniqueKey(); @@ -132,7 +136,11 @@ public class FileMapStorage } } - public void touch(Path path) throws IOException { + public void touch(String proposedId, Path path) throws IOException { + if (Optional.ofNullable(tasks.get(proposedId)).map(MapTaskWithValue::getOperation).orElse(null) == MapOperation.DELETE) { + // If deleted in the current transaction before this operation, then do not touch + return; + } Files.createFile(path); createdPaths.add(path); } @@ -144,6 +152,7 @@ public class FileMapStorage } void registerRenameOnCommit(Path from, Path to) { + pathsToDelete.remove(to); this.renameOnCommit.put(from, to); } @@ -203,15 +212,15 @@ public class FileMapStorage private static class Crud extends FileCrudOperations { - private FileMapStorage store; + private FileMapStorage store; public Crud(Class entityClass, Function dataDirectoryFunc, Function suggestedPath, boolean isExpirableEntity) { super(entityClass, dataDirectoryFunc, suggestedPath, isExpirableEntity); } @Override - protected void touch(Path sp) throws IOException { - store.touch(sp); + protected void touch(String proposedId, Path sp) throws IOException { + store.touch(proposedId, sp); } @Override @@ -250,7 +259,46 @@ public class FileMapStorage public > & org.keycloak.models.map.common.EntityField> void set(EF field, T value) { String id = entity.getId(); super.set(field, value); - if (! Objects.equals(id, map.determineKeyFromValue(entity, false))) { + checkIdMatches(id, field); + } + + @Override + public > & org.keycloak.models.map.common.EntityField> void collectionAdd(EF field, T value) { + String id = entity.getId(); + super.collectionAdd(field, value); + checkIdMatches(id, field); + } + + @Override + public > & org.keycloak.models.map.common.EntityField> Object collectionRemove(EF field, T value) { + String id = entity.getId(); + final Object res = super.collectionRemove(field, value); + checkIdMatches(id, field); + return res; + } + + @Override + public > & org.keycloak.models.map.common.EntityField> void mapPut(EF field, K key, T value) { + String id = entity.getId(); + super.mapPut(field, key, value); + checkIdMatches(id, field); + } + + @Override + public > & org.keycloak.models.map.common.EntityField> Object mapRemove(EF field, K key) { + String id = entity.getId(); + final Object res = super.mapRemove(field, key); + checkIdMatches(id, field); + return res; + } + + private > & org.keycloak.models.map.common.EntityField> void checkIdMatches(String id, EF field) throws ReadOnlyException { + final String idNow = map.determineKeyFromValue(entity, ""); + if (! Objects.equals(id, idNow)) { + if (idNow.endsWith(ID_COMPONENT_SEPARATOR) && id.startsWith(idNow)) { + return; + } + throw new ReadOnlyException("Cannot change " + field + " as that would change primary key"); } } diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProvider.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProvider.java index 3462cc6d03..610daaaef8 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProvider.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProvider.java @@ -55,7 +55,7 @@ public class FileMapStorageProvider implements MapStorageProvider { return (MapStorage) SessionAttributesUtils.createMapStorageIfAbsent(session, getClass(), modelType, factoryId, () -> createFileMapStorage(modelType)); } - private ConcurrentHashMapStorage createFileMapStorage(Class modelType) { + private ConcurrentHashMapStorage> createFileMapStorage(Class modelType) { String areaName = getModelName(modelType, modelType.getSimpleName()); final Class et = ModelEntityUtil.getEntityType(modelType); Function uniqueHumanReadableField = (Function) UNIQUE_HUMAN_READABLE_NAME_FIELD.get(et); diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java index 06e40e4ba5..bd5980a188 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/FileMapStorageProviderFactory.java @@ -78,8 +78,10 @@ public class FileMapStorageProviderFactory implements AmphibianProviderFactory) v -> new String[] { v.getClientId() })), entry(MapPolicyEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })), - entry(MapPermissionTicketEntity.class,((Function) v -> new String[] { v.getResourceServerId(), v.getId()})), - entry(MapResourceEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })), + entry(MapPermissionTicketEntity.class,((Function) v -> new String[] { v.getResourceServerId(), null })), + entry(MapResourceEntity.class, ((Function) v -> Objects.equals(v.getResourceServerId(), v.getOwner()) + ? new String[] { v.getResourceServerId(), v.getName() } + : new String[] { v.getResourceServerId(), v.getName(), v.getOwner() })), entry(MapScopeEntity.class, ((Function) v -> new String[] { v.getResourceServerId(), v.getName() })) ); diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/BlockContext.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/BlockContext.java index e09c97f93c..3358dd622e 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/BlockContext.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/BlockContext.java @@ -135,7 +135,7 @@ public interface BlockContext { @Override public void writeValue(Object value, WritingMechanism mech) { - if (UndefinedValuesUtils.isUndefined(value)) return; + if (value == null || (value.getClass() != String.class && UndefinedValuesUtils.isUndefined(value))) return; mech.writeObject(value); } diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/MapEntityContext.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/MapEntityContext.java index 72d7f3a334..34d0857e0a 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/MapEntityContext.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/common/MapEntityContext.java @@ -243,6 +243,11 @@ public class MapEntityContext implements BlockContext { }); } + @Override + public String toString() { + return "MapEntityContext[" + objectClass.getCanonicalName() + ']'; + } + public static class MapEntitySequenceYamlContext extends DefaultListContext { public MapEntitySequenceYamlContext(Class itemClass) { diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlParser.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlParser.java index 0cebb7aaf3..6c467ced22 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlParser.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlParser.java @@ -16,6 +16,7 @@ */ package org.keycloak.models.map.storage.file.yaml; +import org.keycloak.models.map.common.CastUtils; import org.keycloak.models.map.storage.file.common.BlockContextStack; import org.keycloak.models.map.storage.file.common.BlockContext.DefaultListContext; import org.keycloak.models.map.storage.file.common.BlockContext.DefaultMapContext; @@ -46,6 +47,9 @@ import org.snakeyaml.engine.v2.resolver.ScalarResolver; import org.snakeyaml.engine.v2.scanner.StreamReader; import org.keycloak.models.map.storage.file.common.BlockContext; +import java.util.Map; +import org.snakeyaml.engine.v2.constructor.ConstructScalar; +import org.snakeyaml.engine.v2.nodes.Node; import static org.keycloak.common.util.StackUtil.getShortStackTrace; /** @@ -73,15 +77,24 @@ public class YamlParser { public Object constructStandardJavaInstance(ScalarNode node) { return findConstructorFor(node) .map(constructor -> constructor.construct(node)) - .orElseThrow(() -> new ConstructorException(null, Optional.empty(), "could not determine a constructor for the tag " + node.getTag(), node.getStartMark())); + .orElseThrow(() -> new ConstructorException(null, Optional.empty(), "Could not determine a constructor for the tag " + node.getTag(), node.getStartMark())); } public static final MiniConstructor INSTANCE = new MiniConstructor(); } + private static final class NullConstructor extends ConstructScalar { + + @Override + public Object construct(Node node) { + return null; + } + } + private static final LoadSettings SETTINGS = LoadSettings.builder() .setAllowRecursiveKeys(false) .setParseComments(false) + .setTagConstructors(Map.of(Tag.NULL, new NullConstructor())) .build(); public static E parse(Path path, BlockContext initialContext) { @@ -175,7 +188,11 @@ public class YamlParser { Object key = parseNodeInFreshContext(); LOG.tracef("Parsed mapping key: %s", key); if (! (key instanceof String)) { - throw new IllegalStateException("Invalid key in map: " + key); + try { + key = CastUtils.cast(key, String.class); + } catch (IllegalStateException ex) { + throw new IllegalStateException("Invalid key in map: " + key); + } } Object value = parseNodeInFreshContext((String) key); LOG.tracef("Parsed mapping value: %s", value); diff --git a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlWritingMechanism.java b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlWritingMechanism.java index 9a1ff92d80..66a1b1d648 100644 --- a/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlWritingMechanism.java +++ b/model/map-file/src/main/java/org/keycloak/models/map/storage/file/yaml/YamlWritingMechanism.java @@ -141,8 +141,15 @@ public class YamlWritingMechanism implements WritingMechanism, Closeable { } private ScalarStyle determineStyle(Object value) { - if (value instanceof String && ((String) value).lastIndexOf('\n') > 0) { - return ScalarStyle.FOLDED; + if (value instanceof String) { + String sValue = (String) value; + // TODO: Check numeric values and quote those as well + if ("null".equals(sValue)) { + return ScalarStyle.DOUBLE_QUOTED; + } + if (sValue.length() > 120 || sValue.lastIndexOf('\n') > 0) { + return ScalarStyle.FOLDED; + } } return ScalarStyle.PLAIN; } diff --git a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/HotRodMapStorageProvider.java b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/HotRodMapStorageProvider.java index 69b16c60d9..99e8c0aa2a 100644 --- a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/HotRodMapStorageProvider.java +++ b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/HotRodMapStorageProvider.java @@ -88,7 +88,7 @@ public class HotRodMapStorageProvider implements MapStorageProvider { } } - private & AbstractEntity, M> ConcurrentHashMapStorage createHotRodMapStorage(KeycloakSession session, Class modelType, MapStorageProviderFactory.Flag... flags) { + private & AbstractEntity, M> ConcurrentHashMapStorage createHotRodMapStorage(KeycloakSession session, Class modelType, MapStorageProviderFactory.Flag... flags) { HotRodConnectionProvider connectionProvider = session.getProvider(HotRodConnectionProvider.class); HotRodEntityDescriptor entityDescriptor = (HotRodEntityDescriptor) factory.getEntityDescriptor(modelType); Map, MapModelCriteriaBuilder.UpdatePredicatesFunc> fieldPredicates = MapFieldPredicates.getPredicates((Class) entityDescriptor.getModelTypeClass()); diff --git a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/transaction/AllAreasHotRodStoresWrapper.java b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/transaction/AllAreasHotRodStoresWrapper.java index bd50d872c6..6469789b4f 100644 --- a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/transaction/AllAreasHotRodStoresWrapper.java +++ b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/transaction/AllAreasHotRodStoresWrapper.java @@ -30,10 +30,10 @@ import java.util.function.Supplier; */ public class AllAreasHotRodStoresWrapper extends AbstractKeycloakTransaction { - private final Map, ConcurrentHashMapStorage> MapKeycloakStoresMap = new ConcurrentHashMap<>(); + private final Map, ConcurrentHashMapStorage> MapKeycloakStoresMap = new ConcurrentHashMap<>(); - public ConcurrentHashMapStorage getOrCreateStoreForModel(Class modelType, Supplier> supplier) { - ConcurrentHashMapStorage store = MapKeycloakStoresMap.computeIfAbsent(modelType, t -> supplier.get()); + public ConcurrentHashMapStorage getOrCreateStoreForModel(Class modelType, Supplier> supplier) { + ConcurrentHashMapStorage store = MapKeycloakStoresMap.computeIfAbsent(modelType, t -> supplier.get()); if (!store.isActive()) { store.begin(); } diff --git a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/userSession/HotRodUserSessionMapStorage.java b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/userSession/HotRodUserSessionMapStorage.java index fe44b37066..3d7e99c09d 100644 --- a/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/userSession/HotRodUserSessionMapStorage.java +++ b/model/map-hot-rod/src/main/java/org/keycloak/models/map/storage/hotRod/userSession/HotRodUserSessionMapStorage.java @@ -25,6 +25,7 @@ import org.keycloak.models.map.common.StringKeyConverter; import org.keycloak.models.map.common.delegate.SimpleDelegateProvider; import org.keycloak.models.map.storage.QueryParameters; import org.keycloak.models.map.storage.CrudOperations; +import org.keycloak.models.map.storage.chm.ConcurrentHashMapCrudOperations; import org.keycloak.models.map.storage.chm.ConcurrentHashMapStorage; import org.keycloak.models.map.storage.chm.MapModelCriteriaBuilder; import org.keycloak.models.map.storage.criteria.DefaultModelCriteria; @@ -43,15 +44,15 @@ import java.util.stream.Stream; import static org.keycloak.models.map.storage.ModelCriteriaBuilder.Operator.IN; -public class HotRodUserSessionMapStorage extends ConcurrentHashMapStorage { +public class HotRodUserSessionMapStorage extends ConcurrentHashMapStorage> { - private final ConcurrentHashMapStorage clientSessionStore; + private final ConcurrentHashMapStorage> clientSessionStore; public HotRodUserSessionMapStorage(CrudOperations map, StringKeyConverter keyConverter, DeepCloner cloner, Map, MapModelCriteriaBuilder.UpdatePredicatesFunc> fieldPredicates, - ConcurrentHashMapStorage clientSessionStore + ConcurrentHashMapStorage> clientSessionStore ) { super(map, keyConverter, cloner, fieldPredicates); this.clientSessionStore = clientSessionStore; diff --git a/model/map/src/main/java/org/keycloak/models/map/common/UpdatableEntity.java b/model/map/src/main/java/org/keycloak/models/map/common/UpdatableEntity.java index a8b28cdb04..31b51e8567 100644 --- a/model/map/src/main/java/org/keycloak/models/map/common/UpdatableEntity.java +++ b/model/map/src/main/java/org/keycloak/models/map/common/UpdatableEntity.java @@ -33,6 +33,11 @@ public interface UpdatableEntity { public void clearUpdatedFlag() { this.updated = false; } + + @Override + public void markUpdatedFlag() { + this.updated = true; + } } /** @@ -46,4 +51,10 @@ public interface UpdatableEntity { * {@link #isUpdated()} would return {@code false}. */ default void clearUpdatedFlag() { } + + /** + * An optional operation setting the updated flag. Right after using this method, the + * {@link #isUpdated()} would return {@code true}. + */ + default void markUpdatedFlag() { } } diff --git a/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java b/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java index df8f89f3b2..a8f6c39fe0 100644 --- a/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java +++ b/model/map/src/main/java/org/keycloak/models/map/common/delegate/EntityFieldDelegate.java @@ -53,6 +53,16 @@ public interface EntityFieldDelegate extends UpdatableEntity { return entity.isUpdated(); } + @Override + public void markUpdatedFlag() { + entity.markUpdatedFlag(); + } + + @Override + public void clearUpdatedFlag() { + entity.clearUpdatedFlag(); + } + @Override public String toString() { return "&" + String.valueOf(entity); diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/CrudOperations.java b/model/map/src/main/java/org/keycloak/models/map/storage/CrudOperations.java index 10a25651b6..bfe8642400 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/CrudOperations.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/CrudOperations.java @@ -134,7 +134,7 @@ public interface CrudOperations { * @param value * @return */ - default String determineKeyFromValue(V value, boolean forCreate) { + default String determineKeyFromValue(V value) { return value == null ? null : value.getId(); } } diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java index 565ef3c599..82d0fb2a76 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorage.java @@ -41,15 +41,18 @@ import org.keycloak.models.map.storage.chm.MapModelCriteriaBuilder.UpdatePredica import org.keycloak.models.map.storage.criteria.DefaultModelCriteria; import org.keycloak.storage.SearchableModelField; import java.util.function.Consumer; +import java.util.Collection; +import java.util.Set; +import java.util.function.BiFunction; -public class ConcurrentHashMapStorage implements MapStorage, KeycloakTransaction, HasRealmId { +public class ConcurrentHashMapStorage> implements MapStorage, KeycloakTransaction, HasRealmId { private final static Logger log = Logger.getLogger(ConcurrentHashMapStorage.class); protected boolean active; protected boolean rollback; - protected final Map tasks = new LinkedHashMap<>(); - protected final CrudOperations map; + protected final TaskMap tasks = new TaskMap(); + protected final CRUD map; protected final StringKeyConverter keyConverter; protected final DeepCloner cloner; protected final Map, UpdatePredicatesFunc> fieldPredicates; @@ -57,15 +60,99 @@ public class ConcurrentHashMapStorage map = new LinkedHashMap<>(); + + public boolean isEmpty() { + return map.isEmpty(); + } + + public boolean containsKey(String key) { + return map.containsKey(TaskKey.keyFor(realmId, key)); + } + + public MapTaskWithValue get(String key) { + return map.get(TaskKey.keyFor(realmId, key)); + } + + public MapTaskWithValue put(String key, MapTaskWithValue value) { + return map.put(TaskKey.keyFor(realmId, key), value); + } + + public void clear() { + map.clear(); + } + + public Collection values() { + return map.values(); + } + + public Set> entrySet() { + return map.entrySet(); + } + + public MapTaskWithValue merge(String key, MapTaskWithValue value, BiFunction remappingFunction) { + return map.merge(TaskKey.keyFor(realmId, key), value, remappingFunction); + } + } + + protected enum MapOperation { CREATE, UPDATE, DELETE, } - public ConcurrentHashMapStorage(CrudOperations map, StringKeyConverter keyConverter, DeepCloner cloner, Map, UpdatePredicatesFunc> fieldPredicates) { + public ConcurrentHashMapStorage(CRUD map, StringKeyConverter keyConverter, DeepCloner cloner, Map, UpdatePredicatesFunc> fieldPredicates) { this(map, keyConverter, cloner, fieldPredicates, null); } - public ConcurrentHashMapStorage(CrudOperations map, StringKeyConverter keyConverter, DeepCloner cloner, Map, UpdatePredicatesFunc> fieldPredicates, EntityField realmIdEntityField) { + public ConcurrentHashMapStorage(CRUD map, StringKeyConverter keyConverter, DeepCloner cloner, Map, UpdatePredicatesFunc> fieldPredicates, EntityField realmIdEntityField) { this.map = map; this.keyConverter = keyConverter; this.cloner = cloner; @@ -248,7 +335,7 @@ public class ConcurrentHashMapStorage filterForNonDeletedObjects = bdo.getFilterForNonDeletedObjects(); long res = 0; - for (Iterator> it = tasks.entrySet().iterator(); it.hasNext();) { - Entry me = it.next(); + for (Iterator> it = tasks.entrySet().iterator(); it.hasNext();) { + Entry me = it.next(); if (! filterForNonDeletedObjects.test(me.getValue().getValue())) { log.tracef(" [DELETE_BULK] removing %s", me.getKey()); it.remove(); @@ -328,7 +415,7 @@ public class ConcurrentHashMapStorage createdValuesStream(Predicate keyFilter, Predicate entityFilter) { return this.tasks.entrySet().stream() - .filter(me -> keyFilter.test(keyConverter.fromStringSafe(me.getKey()))) + .filter(me -> Objects.equals(realmId, me.getKey().getRealmId()) && keyFilter.test(keyConverter.fromStringSafe(me.getKey().getKey()))) .map(Map.Entry::getValue) .filter(v -> v.containsCreate() && ! v.isReplace()) .map(MapTaskWithValue::getValue) diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProvider.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProvider.java index 9ce7253d03..76ef8a2a19 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProvider.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProvider.java @@ -58,7 +58,7 @@ public class ConcurrentHashMapStorageProvider implements MapStorageProvider { }); } - private ConcurrentHashMapStorage getMapStorage(Class modelType, CrudOperations crud) { + private ConcurrentHashMapStorage getMapStorage(Class modelType, ConcurrentHashMapCrudOperations crud) { if (modelType == SingleUseObjectValueModel.class) { return new SingleUseObjectMapStorage(crud, factory.getKeyConverter(modelType), CLONER, MapFieldPredicates.getPredicates(modelType)); } diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProviderFactory.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProviderFactory.java index 992ba88bf3..5315031b9d 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProviderFactory.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/ConcurrentHashMapStorageProviderFactory.java @@ -236,14 +236,14 @@ public class ConcurrentHashMapStorageProviderFactory implements AmphibianProvide } @SuppressWarnings("unchecked") - private CrudOperations loadMap(String mapName, + private ConcurrentHashMapCrudOperations loadMap(String mapName, Class modelType, EnumSet flags) { - final StringKeyConverter kc = keyConverters.getOrDefault(mapName, defaultKeyConverter); + final StringKeyConverter kc = keyConverters.getOrDefault(mapName, defaultKeyConverter); Class valueType = ModelEntityUtil.getEntityType(modelType); LOG.debugf("Initializing new map storage: %s", mapName); - CrudOperations store; + ConcurrentHashMapCrudOperations store; if(modelType == SingleUseObjectValueModel.class) { store = new SingleUseObjectConcurrentHashMapCrudOperations(kc, CLONER) { @Override @@ -252,7 +252,7 @@ public class ConcurrentHashMapStorageProviderFactory implements AmphibianProvide } }; } else { - store = new ConcurrentHashMapCrudOperations(modelType, kc, CLONER) { + store = new ConcurrentHashMapCrudOperations<>(modelType, kc, CLONER) { @Override public String toString() { return "ConcurrentHashMapStorage(" + mapName + suffix + ")"; @@ -288,7 +288,7 @@ public class ConcurrentHashMapStorageProviderFactory implements AmphibianProvide } @SuppressWarnings("unchecked") - public CrudOperations getStorage( + public ConcurrentHashMapCrudOperations getStorage( Class modelType, Flag... flags) { EnumSet f = flags == null || flags.length == 0 ? EnumSet.noneOf(Flag.class) : EnumSet.of(flags[0], flags); String name = getModelName(modelType, modelType.getSimpleName()); @@ -297,7 +297,7 @@ public class ConcurrentHashMapStorageProviderFactory implements AmphibianProvide * "... the computation [...] must not attempt to update any other mappings of this map." */ - return (CrudOperations) storages.computeIfAbsent(name, n -> loadMap(name, modelType, f)); + return (ConcurrentHashMapCrudOperations) storages.computeIfAbsent(name, n -> loadMap(name, modelType, f)); } public StringKeyConverter getKeyConverter(Class modelType) { diff --git a/model/map/src/main/java/org/keycloak/models/map/storage/chm/SingleUseObjectMapStorage.java b/model/map/src/main/java/org/keycloak/models/map/storage/chm/SingleUseObjectMapStorage.java index fa835ba4b1..a062a1481c 100644 --- a/model/map/src/main/java/org/keycloak/models/map/storage/chm/SingleUseObjectMapStorage.java +++ b/model/map/src/main/java/org/keycloak/models/map/storage/chm/SingleUseObjectMapStorage.java @@ -29,7 +29,7 @@ import java.util.Map; /** * @author Martin Kanis */ -public class SingleUseObjectMapStorage extends ConcurrentHashMapStorage { +public class SingleUseObjectMapStorage extends ConcurrentHashMapStorage> { public SingleUseObjectMapStorage(CrudOperations map, StringKeyConverter keyConverter, diff --git a/model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java b/model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java index b0fe0ff9e3..20db5a7222 100644 --- a/model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java +++ b/model/map/src/main/java/org/keycloak/models/map/user/MapUserEntity.java @@ -119,7 +119,7 @@ public interface MapUserEntity extends UpdatableEntity, AbstractEntity, EntityWi int indexToRemove = toMoveIndex < ourCredentialIndex ? ourCredentialIndex + 1 : ourCredentialIndex; credentialsList.remove(indexToRemove); - this.updated = true; + markUpdatedFlag(); return true; } } diff --git a/testsuite/model/src/test/java/org/keycloak/testsuite/model/ConcurrentHashMapStorageTest.java b/testsuite/model/src/test/java/org/keycloak/testsuite/model/ConcurrentHashMapStorageTest.java index 113f0fc578..b64c1901cd 100644 --- a/testsuite/model/src/test/java/org/keycloak/testsuite/model/ConcurrentHashMapStorageTest.java +++ b/testsuite/model/src/test/java/org/keycloak/testsuite/model/ConcurrentHashMapStorageTest.java @@ -86,9 +86,9 @@ public class ConcurrentHashMapStorageTest extends KeycloakModelTest { String component2Id = createMapStorageComponent("component2", "keyType", "string"); String[] ids = withRealm(realmId, (session, realm) -> { - ConcurrentHashMapStorage storageMain = (ConcurrentHashMapStorage) (MapStorage) session.getProvider(MapStorageProvider.class, ConcurrentHashMapStorageProviderFactory.PROVIDER_ID).getMapStorage(ClientModel.class); - ConcurrentHashMapStorage storage1 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component1Id).getMapStorage(ClientModel.class); - ConcurrentHashMapStorage storage2 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component2Id).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storageMain = (ConcurrentHashMapStorage) (MapStorage) session.getProvider(MapStorageProvider.class, ConcurrentHashMapStorageProviderFactory.PROVIDER_ID).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storage1 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component1Id).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storage2 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component2Id).getMapStorage(ClientModel.class); // Assert that the map storage can be used both as a standalone store and a component assertThat(storageMain, notNullValue()); @@ -157,7 +157,7 @@ public class ConcurrentHashMapStorageTest extends KeycloakModelTest { assertClientsPersisted(component1Id, component2Id, idMain, id1, id2); } - private void assertClientDoesNotExist(ConcurrentHashMapStorage storage, String id, final StringKeyConverter kc, final StringKeyConverter kcStorage) { + private void assertClientDoesNotExist(ConcurrentHashMapStorage storage, String id, final StringKeyConverter kc, final StringKeyConverter kcStorage) { // Assert that the other stores do not contain the to-be-created clients (if they use compatible key format) try { assertThat(storage.read(id), nullValue()); @@ -170,11 +170,11 @@ public class ConcurrentHashMapStorageTest extends KeycloakModelTest { // Check that in the next transaction, the objects are still there withRealm(realmId, (session, realm) -> { @SuppressWarnings("unchecked") - ConcurrentHashMapStorage storageMain = (ConcurrentHashMapStorage) (MapStorage) session.getProvider(MapStorageProvider.class, ConcurrentHashMapStorageProviderFactory.PROVIDER_ID).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storageMain = (ConcurrentHashMapStorage) (MapStorage) session.getProvider(MapStorageProvider.class, ConcurrentHashMapStorageProviderFactory.PROVIDER_ID).getMapStorage(ClientModel.class); @SuppressWarnings("unchecked") - ConcurrentHashMapStorage storage1 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component1Id).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storage1 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component1Id).getMapStorage(ClientModel.class); @SuppressWarnings("unchecked") - ConcurrentHashMapStorage storage2 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component2Id).getMapStorage(ClientModel.class); + ConcurrentHashMapStorage storage2 = (ConcurrentHashMapStorage) (MapStorage) session.getComponentProvider(MapStorageProvider.class, component2Id).getMapStorage(ClientModel.class); final StringKeyConverter kcMain = (StringKeyConverter) StringKeyConverter.UUIDKey.INSTANCE; final StringKeyConverter kc1 = (StringKeyConverter) StringKeyConverter.ULongKey.INSTANCE;