Skip to content

PropertyMaps can no longer be saved or deleted #5909

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
labkey-adam merged 2 commits into from
Oct 8, 2024
Merged

PropertyMaps can no longer be saved or deleted #5909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8dfd543
PropertyMaps can no longer be saved or deleted
labkey-adam Oct 2, 2024
9fa84df
Adjust expected exception
labkey-adam Oct 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions api/src/org/labkey/api/collections/CollectionUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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))
{
Expand All @@ -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)))
Expand Down
8 changes: 2 additions & 6 deletions api/src/org/labkey/api/data/AbstractPropertyStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
}
}

Expand Down
181 changes: 73 additions & 108 deletions api/src/org/labkey/api/data/PropertyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, String> implements InitializingBean
public static class PropertyMap extends AbstractMapDecorator<String, String>
{
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<String, String> map)
{
Expand Down Expand Up @@ -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<Object> _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<String, String> 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<String, String> mapIterator() {
Map<String, String> map = decorated();
if (map instanceof IterableMap) {
final MapIterator<String, String> it = ((IterableMap<String, String>) decorated()).mapIterator();
return UnmodifiableMapIterator.unmodifiableMapIterator(it);
}
final MapIterator<String, String> it = new EntrySetMapIterator<>(map);
return UnmodifiableMapIterator.unmodifiableMapIterator(it);
}

@Override
public @NotNull Set<Map.Entry<String, String>> entrySet() {
final Set<Map.Entry<String, String>> set = super.entrySet();
return UnmodifiableEntrySet.unmodifiableEntrySet(set);
}

@Override
public @NotNull Set<String> keySet() {
final Set<String> set = super.keySet();
return UnmodifiableSet.unmodifiableSet(set);
}

@Override
public @NotNull Collection<String> values() {
final Collection<String> 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))
Expand All @@ -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)))
Expand All @@ -414,36 +454,27 @@ public String put(String key, @Nullable String value)
@Override
public void putAll(Map<? extends String, ? extends String> 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());
_modified = true;
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);
Expand Down Expand Up @@ -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("</PropertyMap.save() [{}, {}, {}, {}, map.hashCode: {}, lock: {}]>", _user.getUserId(), _objectId, _category, _set, hashCode(), lock.toString());
Expand All @@ -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("</PropertyMap.delete() [{}, {}, {}, {}, map.hashCode: {}, lock: {}]>", _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<String, String> 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<String, String> mapIterator() {
Map<String, String> map = decorated();
if (map instanceof IterableMap) {
final MapIterator<String, String> it = ((IterableMap<String, String>) decorated()).mapIterator();
return UnmodifiableMapIterator.unmodifiableMapIterator(it);
}
final MapIterator<String, String> it = new EntrySetMapIterator<>(map);
return UnmodifiableMapIterator.unmodifiableMapIterator(it);
}

@Override
public @NotNull Set<Map.Entry<String, String>> entrySet() {
final Set<Map.Entry<String, String>> set = super.entrySet();
return UnmodifiableEntrySet.unmodifiableEntrySet(set);
}

@Override
public @NotNull Set<String> keySet() {
final Set<String> set = super.keySet();
return UnmodifiableSet.unmodifiableSet(set);
}

@Override
public @NotNull Collection<String> values() {
final Collection<String> coll = super.values();
return UnmodifiableCollection.unmodifiableCollection(coll);
}
}

/**
Expand Down Expand Up @@ -866,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
Expand Down