From 8dfd543d887701ad8b8e80e06f00c1e8ca7294cc Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 2 Oct 2024 16:45:34 -0700 Subject: [PATCH 1/2] PropertyMaps can no longer be saved or deleted --- .../api/collections/CollectionUtils.java | 15 +- .../api/data/AbstractPropertyStore.java | 8 +- .../org/labkey/api/data/PropertyManager.java | 173 +++++++----------- 3 files changed, 79 insertions(+), 117 deletions(-) diff --git a/api/src/org/labkey/api/collections/CollectionUtils.java b/api/src/org/labkey/api/collections/CollectionUtils.java index 5532223f6c6..d45757a4d30 100644 --- a/api/src/org/labkey/api/collections/CollectionUtils.java +++ b/api/src/org/labkey/api/collections/CollectionUtils.java @@ -26,7 +26,8 @@ import org.junit.Assert; import org.junit.Test; import org.labkey.api.cache.CacheManager; -import org.labkey.api.data.PropertyManager; +import org.labkey.api.data.PropertyManager.PropertyMap; +import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.util.PageFlowUtil; import java.util.ArrayList; @@ -90,12 +91,7 @@ public static boolean isModifiableCollectionMapOrArray(@Nullable Object value) if (value instanceof CacheManager.Sealable sealable && sealable.isSealed()) return null; - if (value instanceof PropertyManager.PropertyMap) - { - if (!((PropertyManager.PropertyMap)value).isLocked()) - return "a modifiable PropertyMap"; - } - else if (value instanceof Collection) + if (value instanceof Collection) { if (!UNMODIFIABLE_COLLECTION_CLASS.isInstance(value)) { @@ -115,6 +111,11 @@ else if (value instanceof List) } } } + else if (value instanceof PropertyMap pm) + { + if (pm instanceof WritablePropertyMap) + return "a WritablePropertyMap"; + } else if (value instanceof Map) { if (UNMODIFIABLE_MAP_CLASSES.stream().noneMatch(c->c.isInstance(value))) diff --git a/api/src/org/labkey/api/data/AbstractPropertyStore.java b/api/src/org/labkey/api/data/AbstractPropertyStore.java index 524b1104113..1cf4281c47f 100644 --- a/api/src/org/labkey/api/data/AbstractPropertyStore.java +++ b/api/src/org/labkey/api/data/AbstractPropertyStore.java @@ -140,7 +140,7 @@ public void deletePropertySet(User user, Container container, String category) { try { - PropertyMap propertyMap = getWritableProperties(user, container, category, false); + WritablePropertyMap propertyMap = getWritableProperties(user, container, category, false); if (propertyMap != null) { propertyMap.delete(); @@ -206,12 +206,8 @@ public PropertyMap load(@NotNull String key, Object argument) String category = (String)params[2]; validateStore(); - PropertyMap m = getPropertyMapFromDatabase(user, container, category); - if (m == null) return null; - m.afterPropertiesSet(); // clear modified flag - m.lock(); - return m; + return getPropertyMapFromDatabase(user, container, category); } } diff --git a/api/src/org/labkey/api/data/PropertyManager.java b/api/src/org/labkey/api/data/PropertyManager.java index 2974cb7e5e5..48549cc24b7 100644 --- a/api/src/org/labkey/api/data/PropertyManager.java +++ b/api/src/org/labkey/api/data/PropertyManager.java @@ -42,7 +42,6 @@ import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.TestContext; import org.labkey.api.util.logging.LogHelper; -import org.springframework.beans.factory.InitializingBean; import java.util.Collection; import java.util.Collections; @@ -284,14 +283,14 @@ public static PropertyEntry[] findPropertyEntries(@Nullable User user, @Nullable // Instances of this specific class are guaranteed to be immutable; all mutating Map methods (put(), remove(), etc.) // throw exceptions. keySet(), values(), entrySet(), and mapIterator() return immutable data structures. // Instances of subclass WritablePropertyMap, however, are mutable Maps and can be saved & deleted. - public static class PropertyMap extends AbstractMapDecorator implements InitializingBean + public static class PropertyMap extends AbstractMapDecorator { - private int _set; - private final @NotNull User _user; - private final @NotNull String _objectId; - private final @NotNull String _category; - private final PropertyEncryption _propertyEncryption; - private final AbstractPropertyStore _store; + protected int _set; + protected final @NotNull User _user; + protected final @NotNull String _objectId; + protected final @NotNull String _category; + protected final PropertyEncryption _propertyEncryption; + protected final AbstractPropertyStore _store; protected PropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store, Map map) { @@ -367,20 +366,71 @@ public int hashCode() result = 31 * result + _category.hashCode(); return result; } + } - // TODO: Once all repos are migrated to use WritablePropertyMaps, everything below will be moved to WritablePropertyMap. - // _locked handling will be removed and cache checking will simply forbid WritablePropertyMap. - // I believe that InitializingBean & afterPropertiesSet() can be removed, since we're now loading the map before - // constructing WritablePropertyMap. - + /** + * This subclass is a mutable Map and can be saved & deleted + */ + public static class WritablePropertyMap extends PropertyMap + { private boolean _modified = false; private Set _removedKeys = null; - private boolean _locked = false; + + /** Clone the existing map, creating our own copy of the underlying data */ + WritablePropertyMap(PropertyMap copy) + { + super(copy._set, copy._user, copy._objectId, copy._category, copy._propertyEncryption, copy._store, new HashMap<>(copy)); + } + + /** New empty, writable map */ + WritablePropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store) + { + super(set, user, objectId, category, propertyEncryption, store, new HashMap<>()); + } + + @Override + protected void validate(Map map) + { + // This class allows modifiable maps + } + + // The following four overrides are not needed in PropertyMap since instances of that class are guaranteed to + // be unmodifiable. + + @Override + public MapIterator mapIterator() { + Map map = decorated(); + if (map instanceof IterableMap) { + final MapIterator it = ((IterableMap) decorated()).mapIterator(); + return UnmodifiableMapIterator.unmodifiableMapIterator(it); + } + final MapIterator it = new EntrySetMapIterator<>(map); + return UnmodifiableMapIterator.unmodifiableMapIterator(it); + } + + @Override + public @NotNull Set> entrySet() { + final Set> set = super.entrySet(); + return UnmodifiableEntrySet.unmodifiableEntrySet(set); + } + + @Override + public @NotNull Set keySet() { + final Set set = super.keySet(); + return UnmodifiableSet.unmodifiableSet(set); + } + + @Override + public @NotNull Collection values() { + final Collection coll = super.values(); + return UnmodifiableCollection.unmodifiableCollection(coll); + } + + // Map mutating methods below @Override public String remove(Object key) { - checkLocked(); if (null == _removedKeys) _removedKeys = new HashSet<>(); if (containsKey(key)) @@ -391,19 +441,9 @@ public String remove(Object key) return super.remove(key); } - private void checkLocked() - { - if (_locked) - { - throw new IllegalStateException("Cannot modify a locked PropertyMap - use getWritableProperties() for a mutable copy"); - } - } - - @Override public String put(String key, @Nullable String value) { - checkLocked(); if (null != _removedKeys) _removedKeys.remove(key); if (!StringUtils.equals(value, get(key))) @@ -414,16 +454,13 @@ public String put(String key, @Nullable String value) @Override public void putAll(Map m) { - checkLocked(); _modified = true; // putAll() calls put(), but just to be safe super.putAll(m); } - @Override public void clear() { - checkLocked(); if (null == _removedKeys) _removedKeys = new HashSet<>(); _removedKeys.addAll(keySet()); @@ -431,19 +468,13 @@ public void clear() super.clear(); } + // Persistence methods below + public boolean isModified() { return _modified; } - - @Override - public void afterPropertiesSet() - { - _modified = false; - } - - private static String toNullString(Object o) { return null == o ? null : String.valueOf(o); @@ -533,7 +564,7 @@ public void save() saveValue(name, value); } // Make sure that we clear the previously cached version of the map - transaction.addCommitTask(() -> _store.clearCache(PropertyMap.this), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT); + transaction.addCommitTask(() -> _store.clearCache(this), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT); transaction.commit(); LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); @@ -552,77 +583,11 @@ public void delete() deleteSetDirectly(_user, _objectId, _category, _store); // Make sure that we clear the previously cached version of the map - transaction.addCommitTask(() -> _store.clearCache(PropertyMap.this), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT); + transaction.addCommitTask(() -> _store.clearCache(this), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT); transaction.commit(); LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); } } - - public void lock() - { - _locked = true; - } - - public boolean isLocked() - { - return _locked; - } - } - - /** - * This subclass is a mutable Map and can be saved & deleted - */ - public static class WritablePropertyMap extends PropertyMap - { - /** Clone the existing map, creating our own copy of the underlying data */ - WritablePropertyMap(PropertyMap copy) - { - super(copy._set, copy._user, copy._objectId, copy._category, copy._propertyEncryption, copy._store, new HashMap<>(copy)); - } - - /** New empty, writable map */ - WritablePropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store) - { - super(set, user, objectId, category, propertyEncryption, store, new HashMap<>()); - } - - @Override - protected void validate(Map map) - { - // This class allows modifiable maps - } - - // The following four overrides are not needed in PropertyMap since instances of that class are guaranteed to - // be unmodifiable. - - @Override - public MapIterator mapIterator() { - Map map = decorated(); - if (map instanceof IterableMap) { - final MapIterator it = ((IterableMap) decorated()).mapIterator(); - return UnmodifiableMapIterator.unmodifiableMapIterator(it); - } - final MapIterator it = new EntrySetMapIterator<>(map); - return UnmodifiableMapIterator.unmodifiableMapIterator(it); - } - - @Override - public @NotNull Set> entrySet() { - final Set> set = super.entrySet(); - return UnmodifiableEntrySet.unmodifiableEntrySet(set); - } - - @Override - public @NotNull Set keySet() { - final Set set = super.keySet(); - return UnmodifiableSet.unmodifiableSet(set); - } - - @Override - public @NotNull Collection values() { - final Collection coll = super.values(); - return UnmodifiableCollection.unmodifiableCollection(coll); - } } /** From 9fa84df7249988666b16fa464cdbb2e67532077b Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 2 Oct 2024 22:22:43 -0700 Subject: [PATCH 2/2] Adjust expected exception --- api/src/org/labkey/api/data/PropertyManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/src/org/labkey/api/data/PropertyManager.java b/api/src/org/labkey/api/data/PropertyManager.java index 48549cc24b7..22b037691b1 100644 --- a/api/src/org/labkey/api/data/PropertyManager.java +++ b/api/src/org/labkey/api/data/PropertyManager.java @@ -831,10 +831,10 @@ private void testProperties(PropertyStore store, User user, Container test, Stri assertFalse(map.containsKey("zoo")); // Test immutability of PropertyMap - assertThrows(IllegalStateException.class, () -> map.put("bar", "blue")); - assertThrows(IllegalStateException.class, () -> map.putAll(Map.of("foo", "bar", "color", "red"))); - assertThrows(IllegalStateException.class, () -> map.remove("foo")); - assertThrows(IllegalStateException.class, map::clear); + assertThrows(UnsupportedOperationException.class, () -> map.put("bar", "blue")); + assertThrows(UnsupportedOperationException.class, () -> map.putAll(Map.of("foo", "bar", "color", "red"))); + assertThrows(UnsupportedOperationException.class, () -> map.remove("foo")); + assertThrows(UnsupportedOperationException.class, map::clear); } @Test