From 3f1141dd0efbae5ce767c80b6114bf41e0d62cdc Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 1 Oct 2024 12:16:54 -0700 Subject: [PATCH] Introduce WritablePropertyMap (#5893) --- .../api/collections/CollectionUtils.java | 10 +- .../api/data/AbstractPropertyStore.java | 78 +++--- .../api/data/EncryptedPropertyStore.java | 12 +- .../labkey/api/data/NormalPropertyStore.java | 11 +- .../org/labkey/api/data/PropertyManager.java | 229 ++++++++++-------- .../org/labkey/api/data/PropertyStore.java | 9 +- api/src/org/labkey/api/util/DateUtil.java | 12 +- api/src/org/labkey/api/util/PageFlowUtil.java | 12 +- 8 files changed, 193 insertions(+), 180 deletions(-) diff --git a/api/src/org/labkey/api/collections/CollectionUtils.java b/api/src/org/labkey/api/collections/CollectionUtils.java index 11fcd1ff691..5532223f6c6 100644 --- a/api/src/org/labkey/api/collections/CollectionUtils.java +++ b/api/src/org/labkey/api/collections/CollectionUtils.java @@ -140,12 +140,10 @@ else if (value instanceof MultiValuedMap) /** * Attempts to determine if the provided Set implementation is stable-ordered, i.e., its iteration order matches its * insertion order. Currently, LinkedHashSet, Collections.singleton(), and Collections.emptySet() are considered - * stable-ordered; HashSet and TreeSet are not. Sets returned by Set.of() are also considered not stable-ordered; - * although they seem to iterate in insertion order in current JVMs, their JavaDoc clearly states that "the - * iteration order of set elements is unspecified and is subject to change." Similarly, we used to special case - * unstable sets with size 1, allowing them since they obviously iterate in a predicable manner. However, we then - * encountered code paths that usually pass a one-element set, but in some circumstances pass a larger set. We now - * flag all unstable sets regardless of size. If you want to pass a single value then use Collections.singleton(). + * stable-ordered; HashSet, TreeSet, and sets returned from Set.of() are not. We used to special case unstable sets + * with size 1, allowing them since they obviously iterate in a predicable manner. However, we then encountered code + * paths that usually pass a one-element set, but in some circumstances pass a larger set. We now flag all unstable + * sets regardless of size. If you want to pass a single value then use Collections.singleton(). */ public static boolean isStableOrderedSet(@NotNull Set set) { diff --git a/api/src/org/labkey/api/data/AbstractPropertyStore.java b/api/src/org/labkey/api/data/AbstractPropertyStore.java index 67c60ab27c3..524b1104113 100644 --- a/api/src/org/labkey/api/data/AbstractPropertyStore.java +++ b/api/src/org/labkey/api/data/AbstractPropertyStore.java @@ -15,13 +15,17 @@ */ package org.labkey.api.data; +import org.apache.commons.collections4.map.UnmodifiableMap; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.labkey.api.cache.CacheLoader; import org.labkey.api.data.PropertyManager.PropertyMap; +import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.security.Encryption; import org.labkey.api.security.User; +import java.util.Collections; +import java.util.HashMap; import java.util.Map; import java.util.Objects; import java.util.stream.Stream; @@ -40,7 +44,7 @@ public abstract class AbstractPropertyStore implements PropertyStore protected abstract void validateStore(); protected abstract boolean isValidPropertyMap(PropertyMap props); protected abstract String getSaveValue(PropertyMap props, @Nullable String value); - protected abstract void fillValueMap(TableSelector selector, PropertyMap props); + protected abstract String decryptValue(@Nullable String value, PropertyEncryption encryption); protected abstract PropertyEncryption getPreferredPropertyEncryption(); protected abstract void appendWhereFilter(SQLFragment sql); @@ -77,37 +81,37 @@ public PropertyMap getProperties(String category) } @Override - public PropertyMap getWritableProperties(User user, Container container, String category, boolean create) + public WritablePropertyMap getWritableProperties(User user, Container container, String category, boolean create) { PropertyMap existingMap = _cache.getProperties(user, container, category); if (existingMap != null) { - return new PropertyMap(existingMap); + return new WritablePropertyMap(existingMap); } if (create) { - // Assign a dummy ID we can use later to tell if we need to insert a brand new set during save - return new PropertyMap(-1, user, container.getId(), category, getPreferredPropertyEncryption(), this); + // Assign a dummy ID we can use later to tell if we need to insert a brand-new set during save + return new WritablePropertyMap(-1, user, container.getId(), category, getPreferredPropertyEncryption(), this); } return null; } @Override - public PropertyMap getWritableProperties(User user, String category, boolean create) + public WritablePropertyMap getWritableProperties(User user, String category, boolean create) { return getWritableProperties(user, ContainerManager.getRoot(), category, create); } @Override - public PropertyMap getWritableProperties(Container container, String category, boolean create) + public WritablePropertyMap getWritableProperties(Container container, String category, boolean create) { return getWritableProperties(PropertyManager.SHARED_USER, container, category, create); } @Override - public PropertyMap getWritableProperties(String category, boolean create) + public WritablePropertyMap getWritableProperties(String category, boolean create) { return getWritableProperties(ContainerManager.getRoot(), category, create); } @@ -177,32 +181,7 @@ protected void validatePropertyMap(PropertyMap map) static { - NULL_MAP = new PropertyMap(0, PropertyManager.SHARED_USER, "NULL_MAP", PropertyManager.class.getName(), PropertyEncryption.None, null) - { - @Override - public String put(String key, String value) - { - throw new UnsupportedOperationException("Cannot modify NULL_MAP"); - } - - @Override - public void clear() - { - throw new UnsupportedOperationException("Cannot modify NULL_MAP"); - } - - @Override - public String remove(Object key) - { - throw new UnsupportedOperationException("Cannot modify NULL_MAP"); - } - - @Override - public void putAll(Map m) - { - throw new UnsupportedOperationException("Cannot modify NULL_MAP"); - } - }; + NULL_MAP = new PropertyMap(0, PropertyManager.SHARED_USER, "NULL_MAP", PropertyManager.class.getName(), PropertyEncryption.None, null, Collections.emptyMap()); } void clearCache(PropertyMap propertyMap) @@ -236,6 +215,8 @@ public PropertyMap load(@NotNull String key, Object argument) } } + private record PropertySet(int set, String encryption){} + @Nullable public PropertyMap getPropertyMapFromDatabase(User user, Container container, String category) { @@ -245,31 +226,32 @@ public PropertyMap getPropertyMapFromDatabase(User user, Container container, St SQLFragment sql = new SQLFragment("SELECT " + setSelectName + ", Encryption FROM " + _prop.getTableInfoPropertySets() + " WHERE UserId = ? AND ObjectId = ? AND Category = ?", user, container, category); - Map map = new SqlSelector(_prop.getSchema(), sql).getMap(); - if (map == null) + PropertySet propertySet = new SqlSelector(_prop.getSchema(), sql).getObject(PropertySet.class); + if (propertySet == null) { return null; } - PropertyEncryption propertyEncryption; - String encryptionName = (String) map.get("Encryption"); - propertyEncryption = PropertyEncryption.getBySerializedName(encryptionName); + PropertyEncryption propertyEncryption = PropertyEncryption.getBySerializedName(propertySet.encryption()); if (null == propertyEncryption) - throw new IllegalStateException("Unknown encryption name: " + encryptionName); + throw new IllegalStateException("Unknown encryption name: " + propertySet.encryption()); - // map should always contain the set number - int set = (Integer)map.get("Set"); + // Map-filling query needed only for existing property set + Filter filter = new SimpleFilter(setColumn.getFieldKey(), propertySet.set()); + TableInfo tinfo = _prop.getTableInfoProperties(); + TableSelector selector = new TableSelector(tinfo, tinfo.getColumns("Name", "Value"), filter, null); + Map map = new HashMap<>(); - PropertyMap m = new PropertyMap(set, user, container.getId(), category, propertyEncryption, AbstractPropertyStore.this); + selector.forEach(rs -> { + String value = decryptValue(rs.getString(2), propertyEncryption); + map.put(rs.getString(1), value); + }); + // Note: This unmodifiable map implementation is aligned with the map wrapper that PropertyMap extends + PropertyMap m = new PropertyMap(propertySet.set(), user, container.getId(), category, propertyEncryption, AbstractPropertyStore.this, UnmodifiableMap.unmodifiableMap(map)); validatePropertyMap(m); - // Map-filling query needed only for existing property set - Filter filter = new SimpleFilter(setColumn.getFieldKey(), set); - TableInfo tinfo = _prop.getTableInfoProperties(); - TableSelector selector = new TableSelector(tinfo, tinfo.getColumns("Name", "Value"), filter, null); - fillValueMap(selector, m); return m; } diff --git a/api/src/org/labkey/api/data/EncryptedPropertyStore.java b/api/src/org/labkey/api/data/EncryptedPropertyStore.java index c9a30755e3e..bb63774054c 100644 --- a/api/src/org/labkey/api/data/EncryptedPropertyStore.java +++ b/api/src/org/labkey/api/data/EncryptedPropertyStore.java @@ -75,17 +75,9 @@ protected String getSaveValue(PropertyMap props, @Nullable String value) } @Override - protected void fillValueMap(TableSelector selector, final PropertyMap props) + protected String decryptValue(@Nullable String value, PropertyEncryption encryption) { - final PropertyEncryption propertyEncryption = props.getEncryptionAlgorithm(); - - validatePropertyMap(props); - - selector.forEach(rs -> { - String encryptedValue = rs.getString(2); - String value = null == encryptedValue ? null : propertyEncryption.decrypt(Base64.decodeBase64(encryptedValue)); - props.put(rs.getString(1), value); - }); + return null == value ? null : encryption.decrypt(Base64.decodeBase64(value)); } @Override diff --git a/api/src/org/labkey/api/data/NormalPropertyStore.java b/api/src/org/labkey/api/data/NormalPropertyStore.java index 0f52417fa67..1c80ed52e5c 100644 --- a/api/src/org/labkey/api/data/NormalPropertyStore.java +++ b/api/src/org/labkey/api/data/NormalPropertyStore.java @@ -16,6 +16,7 @@ package org.labkey.api.data; import org.jetbrains.annotations.Nullable; +import org.labkey.api.data.PropertyManager.PropertyMap; /** * A PropertyStore that does not encrypt its values when persisted in the database. @@ -34,21 +35,19 @@ protected void validateStore() } @Override - protected boolean isValidPropertyMap(PropertyManager.PropertyMap props) + protected boolean isValidPropertyMap(PropertyMap props) { return props.getEncryptionAlgorithm() == PropertyEncryption.None; } @Override - protected void fillValueMap(TableSelector selector, PropertyManager.PropertyMap props) + protected String decryptValue(@Nullable String value, PropertyEncryption encryption) { - validatePropertyMap(props); - - selector.fillValueMap(props); + return value; } @Override - protected String getSaveValue(PropertyManager.PropertyMap props, @Nullable String value) + protected String getSaveValue(PropertyMap props, @Nullable String value) { if (props.getEncryptionAlgorithm() != PropertyEncryption.None) throw new IllegalStateException("NormalPropertyStore should not be saving a PropertyMap encrypted with " + props.getEncryptionAlgorithm()); diff --git a/api/src/org/labkey/api/data/PropertyManager.java b/api/src/org/labkey/api/data/PropertyManager.java index b7cc020f8aa..32edac036cc 100644 --- a/api/src/org/labkey/api/data/PropertyManager.java +++ b/api/src/org/labkey/api/data/PropertyManager.java @@ -16,14 +16,22 @@ package org.labkey.api.data; +import org.apache.commons.collections4.IterableMap; +import org.apache.commons.collections4.MapIterator; +import org.apache.commons.collections4.collection.UnmodifiableCollection; +import org.apache.commons.collections4.iterators.EntrySetMapIterator; +import org.apache.commons.collections4.iterators.UnmodifiableMapIterator; +import org.apache.commons.collections4.map.AbstractMapDecorator; +import org.apache.commons.collections4.map.UnmodifiableEntrySet; +import org.apache.commons.collections4.set.UnmodifiableSet; import org.apache.commons.lang3.StringUtils; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.junit.Assert; import org.junit.Test; import org.labkey.api.action.SpringActionController; +import org.labkey.api.collections.CollectionUtils; import org.labkey.api.data.DbScope.Transaction; import org.labkey.api.query.FieldKey; import org.labkey.api.security.Encryption.EncryptionMigrationHandler; @@ -33,6 +41,7 @@ import org.labkey.api.util.JunitUtil; 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; @@ -48,8 +57,8 @@ public class PropertyManager { public static final User SHARED_USER = User.guest; // Shared properties are saved with the guest user - private static final Logger _log = LogManager.getLogger(PropertyManager.class); - private static final PropertySchema prop = PropertySchema.getInstance(); + private static final Logger LOG = LogHelper.getLogger(PropertyManager.class, "Property map save and delete operations"); + private static final PropertySchema PROP_SCHEMA = PropertySchema.getInstance(); private static final NormalPropertyStore STORE = new NormalPropertyStore(); private static final PropertySchema SCHEMA = PropertySchema.getInstance(); private static final LockManager PROPERTY_MAP_LOCK_MANAGER = new LockManager<>(); @@ -109,25 +118,25 @@ private static boolean assertWritableProperties(@Nullable PropertyMap writablePr } /** For global system properties that get attached to the root container. */ - public static PropertyMap getWritableProperties(String category, boolean create) + public static WritablePropertyMap getWritableProperties(String category, boolean create) { - PropertyMap ret = STORE.getWritableProperties(category, create); + WritablePropertyMap ret = STORE.getWritableProperties(category, create); assert assertWritableProperties(ret, create); return ret; } - public static PropertyMap getWritableProperties(Container container, String category, boolean create) + public static WritablePropertyMap getWritableProperties(Container container, String category, boolean create) { - PropertyMap ret = STORE.getWritableProperties(container, category, create); + WritablePropertyMap ret = STORE.getWritableProperties(container, category, create); assert assertWritableProperties(ret, create); return ret; } - public static PropertyMap getWritableProperties(User user, Container container, String category, boolean create) + public static WritablePropertyMap getWritableProperties(User user, Container container, String category, boolean create) { - PropertyMap ret = STORE.getWritableProperties(user, container, category, create); + WritablePropertyMap ret = STORE.getWritableProperties(user, container, category, create); assert assertWritableProperties(ret, create); return ret; } @@ -149,7 +158,6 @@ public static void registerEncryptionMigrationHandler() * This is designed to coalesce up the container hierarchy, returning the first non-null value * If a userId is provided, it first traverses containers using that user. If no value is found, * it then defaults to all users (i.e. User.guest), then retry all containers - * * NOTE: this does not test permissions. Callers should ensure the requested user has the appropriate * permissions to read these properties */ @@ -157,7 +165,7 @@ public static String getCoalescedProperty(User user, Container c, String categor { String value = getCoalescedPropertyForContainer(user, c, category, name); - if(includeNullUser && value == null && user != SHARED_USER) + if (includeNullUser && value == null && user != SHARED_USER) value = getCoalescedPropertyForContainer(SHARED_USER, c, category, name); return value; @@ -202,12 +210,12 @@ public static Map> getPropertyValueAndAncestors( String value = getProperty(user, curContainer, category, name); Map containerMap = new HashMap<>(); - if(value != null) + if (value != null) containerMap.put(user.getUserId(), value); else if (includeNullContainers) containerMap.put(0, value); - if(!containerMap.isEmpty()) + if (!containerMap.isEmpty()) map.put(curContainer, containerMap); curContainer = curContainer.getParent(); @@ -226,9 +234,9 @@ public static String getProperty(User user, Container container, String category * Get a list of categories optionally filtered by user, container, and category prefix. * Returns entries from unencrypted store only. * - * @param user User of the property. If null properties for all users (NOT JUST THE NULL USER) will be found. - * @param container Container to search for. If null properties of all containers will be found - * @param categoryPrefix Prefix to search for. If null properties of all categories will be found + * @param user User of the property. If null, properties for all users (NOT JUST THE NULL USER) will be found. + * @param container Container to search for. If null, properties of all containers will be found + * @param categoryPrefix Prefix to search for. If null, properties of all categories will be found * @return list of categories array containing found properties */ public static List getCategoriesByPrefix(@Nullable User user, @Nullable Container container, @Nullable String categoryPrefix) @@ -243,7 +251,7 @@ public static List getCategoriesByPrefix(@Nullable User user, @Nullable Sort sort = new Sort("UserId,ObjectId,Category,Name"); - return new TableSelector(prop.getTableInfoPropertyEntries(), Collections.singleton("Category"), filter, sort).getArrayList(String.class); + return new TableSelector(PROP_SCHEMA.getTableInfoPropertyEntries(), Collections.singleton("Category"), filter, sort).getArrayList(String.class); } /** @@ -270,53 +278,65 @@ public static PropertyEntry[] findPropertyEntries(@Nullable User user, @Nullable Sort sort = new Sort("UserId,ObjectId,Category,Name"); - return new TableSelector(prop.getTableInfoPropertyEntries(), filter, sort).getArray(PropertyEntry.class); + return new TableSelector(PROP_SCHEMA.getTableInfoPropertyEntries(), filter, sort).getArray(PropertyEntry.class); } - public static class PropertyMap extends HashMap implements InitializingBean + // Instances of this specific class are guaranteed to be immutable; all mutating Map methods (put(), remove(), etc.) + // will throw exceptions. keySet(), values(), entrySet(), and mapIterator() return immutable data structures. + // Instances of subclass WritablePropertyMap ARE mutable and can be saved, deleted, etc. + public static class PropertyMap extends AbstractMapDecorator implements InitializingBean { private int _set; - @NotNull - private final User _user; - @NotNull - private final String _objectId; - @NotNull - private final String _category; + private final @NotNull User _user; + private final @NotNull String _objectId; + private final @NotNull String _category; private final PropertyEncryption _propertyEncryption; private final AbstractPropertyStore _store; private boolean _modified = false; - private Set removedKeys = null; + private Set _removedKeys = null; private boolean _locked = false; - /** Clone the existing map, creating our own copy of the underlying data */ - PropertyMap(PropertyMap copy) - { - super(copy); - _set = copy._set; - _user = copy._user; - _objectId = copy._objectId; - _category = copy._category; - _propertyEncryption = copy._propertyEncryption; - _store = copy._store; - } - - PropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store) + protected PropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store, Map map) { + super(map); _set = set; _user = user; _objectId = objectId; _category = category; _propertyEncryption = propertyEncryption; _store = store; + validate(map); } + protected void validate(Map map) + { + if (CollectionUtils.isModifiableCollectionMapOrArray(map)) + throw new IllegalStateException("Map is modifiable!"); + } int getSet() { return _set; } + @NotNull + public User getUser() + { + return _user; + } + + @NotNull + public String getObjectId() + { + return _objectId; + } + + @NotNull + public String getCategory() + { + return _category; + } PropertyEncryption getEncryptionAlgorithm() { @@ -327,11 +347,11 @@ PropertyEncryption getEncryptionAlgorithm() public String remove(Object key) { checkLocked(); - if (null == removedKeys) - removedKeys = new HashSet<>(); + if (null == _removedKeys) + _removedKeys = new HashSet<>(); if (containsKey(key)) { - removedKeys.add(key); + _removedKeys.add(key); _modified = true; } return super.remove(key); @@ -350,8 +370,8 @@ private void checkLocked() public String put(String key, @Nullable String value) { checkLocked(); - if (null != removedKeys) - removedKeys.remove(key); + if (null != _removedKeys) + _removedKeys.remove(key); if (!StringUtils.equals(value, get(key))) _modified = true; return super.put(key, value); @@ -360,7 +380,7 @@ public String put(String key, @Nullable String value) @Override public String toString() { - return "PropertyMap: " + _objectId + ", " + _category + ", " + _user.getDisplayName(null) + ": " + super.toString(); + return getClass().getSimpleName() + ": " + _objectId + ", " + _category + ", " + _user.getDisplayName(null) + ": " + super.toString(); } @Override @@ -376,35 +396,13 @@ public void putAll(Map m) public void clear() { checkLocked(); - if (null == removedKeys) - removedKeys = new HashSet<>(); - removedKeys.addAll(keySet()); + if (null == _removedKeys) + _removedKeys = new HashSet<>(); + _removedKeys.addAll(keySet()); _modified = true; super.clear(); } - - @NotNull - @Override - public Set keySet() - { - return Collections.unmodifiableSet(super.keySet()); - } - - @NotNull - @Override - public Collection values() - { - return Collections.unmodifiableCollection(super.values()); - } - - @NotNull - @Override - public Set> entrySet() - { - return Collections.unmodifiableSet(super.entrySet()); - } - @Override public boolean equals(Object o) { @@ -415,9 +413,8 @@ public boolean equals(Object o) if (!_user.equals(that._user)) return false; if (!_category.equals(that._category)) return false; - if (!_objectId.equals(that._objectId)) return false; - return true; + return _objectId.equals(that._objectId); } @Override @@ -478,7 +475,7 @@ public void save() ReentrantLock lock = PROPERTY_MAP_LOCK_MANAGER.getLock(this); try (Transaction transaction = SCHEMA.getSchema().getScope().ensureTransaction(DbScope.FINAL_COMMIT_UNLOCK_TRANSACTION_KIND, lock)) { - _log.debug(String.format("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString())); + LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); Container container = ContainerManager.getForId(_objectId); if (container == null) @@ -513,9 +510,9 @@ public void save() } // delete removed properties - if (null != removedKeys) + if (null != _removedKeys) { - for (Object removedKey : removedKeys) + for (Object removedKey : _removedKeys) { String name = toNullString(removedKey); saveValue(name, null); @@ -524,9 +521,8 @@ public void save() // set properties // we're not tracking modified or not, so set them all - for (Object entry : entrySet()) + for (Map.Entry e : entrySet()) { - Map.Entry e = (Map.Entry) entry; String name = toNullString(e.getKey()); String value = toNullString(e.getValue()); saveValue(name, value); @@ -535,7 +531,7 @@ public void save() transaction.addCommitTask(() -> _store.clearCache(PropertyMap.this), DbScope.CommitTaskOption.IMMEDIATE, DbScope.CommitTaskOption.POSTCOMMIT); transaction.commit(); - _log.debug(String.format("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString())); + LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); } } @@ -546,49 +542,86 @@ public void delete() final ReentrantLock lock = PROPERTY_MAP_LOCK_MANAGER.getLock(this); try (Transaction transaction = SCHEMA.getSchema().getScope().ensureTransaction(DbScope.FINAL_COMMIT_UNLOCK_TRANSACTION_KIND, lock)) { - _log.debug(String.format("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString())); + LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); 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.commit(); - _log.debug(String.format("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString())); + LOG.debug("", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString()); } } - @NotNull - public String getObjectId() + public void lock() { - return _objectId; + _locked = true; } - - @NotNull - public User getUser() + public boolean isLocked() { - return _user; + return _locked; } + } - @NotNull - public String getCategory() + /** + * This subclass is mutable and can be saved, deleted, etc. + */ + public static class WritablePropertyMap extends PropertyMap + { + /** Clone the existing map, creating our own copy of the underlying data */ + WritablePropertyMap(PropertyMap copy) { - return _category; + super(copy._set, copy._user, copy._objectId, copy._category, copy._propertyEncryption, copy._store, new HashMap<>(copy)); } - public void lock() + /** New empty, writable map */ + WritablePropertyMap(int set, @NotNull User user, @NotNull String objectId, @NotNull String category, PropertyEncryption propertyEncryption, AbstractPropertyStore store) { - _locked = true; + super(set, user, objectId, category, propertyEncryption, store, new HashMap<>()); } - public boolean isLocked() + @Override + protected void validate(Map map) { - return _locked; + // 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); } } /** - * In general, callers should always go through PropertyMap.delete(). This is exposed here for cases + * In general, callers should always go through WritablePropertyMap.delete(). This is exposed here for cases * where we can't create a PropertyMap because we fail to decrypt the values previously stored, and need * a direct way to delete the properties. See issue 18938 */ @@ -826,6 +859,12 @@ private void testProperties(PropertyStore store, User user, Container test, Stri assertEquals(map.get("foo"), "bar"); assertFalse(map.containsKey("this")); 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); } @Test diff --git a/api/src/org/labkey/api/data/PropertyStore.java b/api/src/org/labkey/api/data/PropertyStore.java index 3c1f666a9f2..44782b15401 100644 --- a/api/src/org/labkey/api/data/PropertyStore.java +++ b/api/src/org/labkey/api/data/PropertyStore.java @@ -17,6 +17,7 @@ import org.jetbrains.annotations.NotNull; import org.labkey.api.data.PropertyManager.PropertyMap; +import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.security.User; import java.util.stream.Stream; @@ -33,10 +34,10 @@ public interface PropertyStore @NotNull PropertyMap getProperties(String category); // If create == true, then never returns null. If create == false, will return null if property set doesn't exist. - PropertyMap getWritableProperties(User user, Container container, String category, boolean create); - PropertyMap getWritableProperties(User user, String category, boolean create); - PropertyMap getWritableProperties(Container container, String category, boolean create); - PropertyMap getWritableProperties(String category, boolean create); + WritablePropertyMap getWritableProperties(User user, Container container, String category, boolean create); + WritablePropertyMap getWritableProperties(User user, String category, boolean create); + WritablePropertyMap getWritableProperties(Container container, String category, boolean create); + WritablePropertyMap getWritableProperties(String category, boolean create); void deletePropertySet(User user, Container container, String category); void deletePropertySet(User user, String category); diff --git a/api/src/org/labkey/api/util/DateUtil.java b/api/src/org/labkey/api/util/DateUtil.java index 6c5b4fc5f58..1427f7ff0ab 100644 --- a/api/src/org/labkey/api/util/DateUtil.java +++ b/api/src/org/labkey/api/util/DateUtil.java @@ -1289,9 +1289,10 @@ public static String formatDateTime(@Nullable Date date, String pattern) } /** - * Get the default date format string to use in this Container + * Get the default date display format string to use in this Container * Note: The display format is specified by an admin; it could contain any characters, hence, it may not be safe. * Any value formatted by this pattern must be HTML filtered, if rendered to an HTML page. + * THIS IS A DISPLAY FORMAT STRING; DO NOT USE IT FOR PARSING! */ public static String getDateFormatString(Container c) { @@ -1299,15 +1300,22 @@ public static String getDateFormatString(Container c) } /** - * Get the default date/time format string set in this Container (or one of its parents) + * Get the default date/time display format string set in this Container (or one of its parents) * Note: The display format is specified by an admin; it could contain any characters, hence, it may not be safe. * Any value formatted by this pattern must be HTML filtered, if rendered to an HTML page. + * THIS IS A DISPLAY FORMAT STRING; DO NOT USE IT FOR PARSING! */ public static String getDateTimeFormatString(Container c) { return FolderSettingsCache.getDefaultDateTimeFormat(c); } + /** + * Get the default time display format string set in this Container (or one of its parents) + * Note: The display format is specified by an admin; it could contain any characters, hence, it may not be safe. + * Any value formatted by this pattern must be HTML filtered, if rendered to an HTML page. + * THIS IS A DISPLAY FORMAT STRING; DO NOT USE IT FOR PARSING! + */ public static String getTimeFormatString(Container c) { return FolderSettingsCache.getDefaultTimeFormat(c); diff --git a/api/src/org/labkey/api/util/PageFlowUtil.java b/api/src/org/labkey/api/util/PageFlowUtil.java index 16278d420e3..339d93c3916 100644 --- a/api/src/org/labkey/api/util/PageFlowUtil.java +++ b/api/src/org/labkey/api/util/PageFlowUtil.java @@ -173,7 +173,7 @@ private enum TransformFormat } private static final Logger _log = LogHelper.getLogger(PageFlowUtil.class, "HTML validation and file operation errors"); - private static final String _newline = System.getProperty("line.separator"); + private static final String _newline = System.lineSeparator(); private static final Pattern urlPatternStart = Pattern.compile("((http|https|ftp|mailto)://\\S+).*"); @@ -376,7 +376,6 @@ public static String filterQuote(Object value) /** * Creates a JavaScript string literal of an HTML escaped value. - * * Ext, for example, will use the 'id' config parameter as an attribute value in an XTemplate. * The string value is inserted directly into the dom and so should be HTML encoded. * @@ -1246,7 +1245,7 @@ public static List getStreamContentsAsList(InputStream is, boolean skipC public static boolean empty(String str) { - return null == str || str.trim().length() == 0; + return null == str || str.trim().isEmpty(); } @@ -1556,7 +1555,7 @@ public static String convertNodeToXml(Node node) throws TransformerException, IO return convertNodeToString(node, TransformFormat.xml); } - public static String convertNodeToString(Node node, TransformFormat format) throws TransformerException, IOException + private static String convertNodeToString(Node node, TransformFormat format) throws TransformerException, IOException { try { @@ -2691,11 +2690,6 @@ static public

P urlProvider(Class

inter, boolean chec return urlProvider(inter); } - static private String h(Object o) - { - return PageFlowUtil.filter(o); - } - /** * CONSOLIDATE ALL .lastFilter handling */