Skip to content

Commit

Permalink
Merge pull request #694 from MrCreosote/develop
Browse files Browse the repository at this point in the history 
Simplify setting workspace metadata & add tests
  • Loading branch information
@MrCreosote
MrCreosote authored Oct 13, 2023
2 parents fc6817c + 3f3a505 commit b02d1b1
Show file tree
Hide file tree
Showing 3 changed files with 281 additions and 72 deletions.
123 changes: 52 additions & 71 deletions src/us/kbase/workspace/database/mongo/MongoWorkspaceDB.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,86 +699,67 @@ private void setCreatedWorkspacePermissions(
private static final Set<String> FLDS_WS_META = newHashSet(Fields.WS_META);

@Override
public Instant setWorkspaceMeta(final ResolvedWorkspaceID rwsi,
public Instant setWorkspaceMeta(
final ResolvedWorkspaceID rwsi,
final WorkspaceUserMetadata newMeta)
throws WorkspaceCommunicationException,
CorruptWorkspaceDBException {

throws WorkspaceCommunicationException, CorruptWorkspaceDBException {
// TODO NOW CODE integrate keys to remove (below) into this method and do it all in
// one shot
if (newMeta == null || newMeta.isEmpty()) {
throw new IllegalArgumentException("Metadata cannot be null or empty");
}
final Map<String, Object> ws = query.queryWorkspace(rwsi, FLDS_WS_META);
@SuppressWarnings("unchecked")
final Map<String, String> currMeta = metaMongoArrayToHash(
(List<Object>) ws.get(Fields.WS_META));
currMeta.putAll(newMeta.getMetadata());
try {
WorkspaceUserMetadata.checkMetadataSize(currMeta);
} catch (MetadataException me) {
throw new IllegalArgumentException(String.format(
"Updated metadata exceeds allowed size of %sB",
WorkspaceUserMetadata.MAX_METADATA_SIZE));
}

/* it's possible if this is running at the same time on the same object
* that the metadata size could exceed 16k since the size check
* happens once at the beginning of the method. That has virtually no
* repercussions whatsoever, so meh.
*/
// 2023/09/21: this seem overcomplicated and inefficient although it works.
// Look for a better way.
int attempts = 1;
Instant time = null;
final String mkey = Fields.WS_META + Fields.FIELD_SEP + Fields.META_KEY;
final String mval = Fields.WS_META + Fields.FIELD_SEP + "$" + Fields.FIELD_SEP +
Fields.META_VALUE;
for (final Entry<String, String> e: newMeta.getMetadata().entrySet()) {
final String key = e.getKey();
final String value = e.getValue();
boolean success = false;
while (!success) { //Danger, Will Robinson! Danger!
//replace the value if it exists already
UpdateResult ur;
try {
time = Instant.now();
ur = wsmongo.getCollection(COL_WORKSPACES).updateOne(
new Document(Fields.WS_ID, rwsi.getID()).append(mkey, key),
new Document("$set", new Document(mval, value)
.append(Fields.WS_MODDATE, Date.from(time))));
} catch (MongoException me) {
throw new WorkspaceCommunicationException(ERR_DB_COMM, me);
}
if (ur.getModifiedCount() == 1) { //ok, it worked
success = true;
continue;
}
//add the key/value pair to the array
time = Instant.now();
try {
ur = wsmongo.getCollection(COL_WORKSPACES).updateOne(
new Document(Fields.WS_ID, rwsi.getID())
.append(mkey, new Document("$nin", Arrays.asList(key))),
new Document(
"$push", new Document(Fields.WS_META,
new Document(Fields.META_KEY, key)
.append(Fields.META_VALUE, value)))
.append("$set", new Document(
Fields.WS_MODDATE, Date.from(time))));

} catch (MongoException me) {
throw new WorkspaceCommunicationException(ERR_DB_COMM, me);
}
if (ur.getModifiedCount() == 1) { //ok, it worked
success = true;
}
/* amazingly, someone added that key to the metadata between the
two calls above, so here we go again on our own
Should be impossible to get stuck in a loop, but if so add
counter and throw error if > 3 or something
*/
while (time == null) {
final Map<String, Object> ws = query.queryWorkspace(rwsi, FLDS_WS_META);
@SuppressWarnings("unchecked")
final List<Map<String, String>> mlist = (List<Map<String, String>>)
ws.get(Fields.WS_META);
final Map<String, String> updatedMeta = metaMongoArrayToHash(mlist);
updatedMeta.putAll(newMeta.getMetadata());
try {
WorkspaceUserMetadata.checkMetadataSize(updatedMeta);
} catch (MetadataException me) {
throw new IllegalArgumentException(String.format(
"Updated metadata exceeds allowed size of %sB",
WorkspaceUserMetadata.MAX_METADATA_SIZE));
}
final Document query = new Document(Fields.WS_ID, rwsi.getID())
.append(Fields.WS_META, mlist);
final Document metaUpdate = new Document(
Fields.WS_META, metaHashToMongoArray(updatedMeta));
time = _internal_setWorkspaceMeta(attempts, 5, query, metaUpdate);
attempts++;
}
return time;
}

// split the method for testing purposes
private Instant _internal_setWorkspaceMeta(
final int attempts,
final int maxattempts,
final Document query,
final Document metaUpdate)
throws WorkspaceCommunicationException {
try {
final Instant time = clock.instant();
// only match if the metadata we pulled from the db is stil the same, so we don't
// clobber any interleaving changes
final UpdateResult ur = wsmongo.getCollection(COL_WORKSPACES).updateOne(
query,
new Document("$set", metaUpdate.append(Fields.WS_MODDATE, Date.from(time))));
if (ur.getModifiedCount() == 1) { //ok, it worked
return time;
} else if (attempts >= maxattempts) {
throw new WorkspaceCommunicationException(
String.format("Failed to update metadata %s times", attempts));
} else {
return null;
}
} catch (MongoException me) { /// very difficult to test
throw new WorkspaceCommunicationException(ERR_DB_COMM, me);
}
}

@Override
public Instant removeWorkspaceMetaKey(
Expand Down
103 changes: 103 additions & 0 deletions src/us/kbase/workspace/test/database/mongo/MongoInternalsTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package us.kbase.workspace.test.database.mongo;

import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static us.kbase.common.test.TestCommon.now;
import static us.kbase.common.test.TestCommon.assertCloseToNow;
import static us.kbase.common.test.TestCommon.assertExceptionCorrect;
import static us.kbase.workspace.test.WorkspaceTestCommon.basicProv;

import java.io.File;
Expand All @@ -14,6 +17,7 @@
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.nio.file.Paths;
import java.time.Instant;
import java.util.Arrays;
import java.util.Date;
import java.util.HashMap;
Expand Down Expand Up @@ -78,10 +82,22 @@
import us.kbase.workspace.database.provenance.Provenance;
import us.kbase.workspace.test.workspace.WorkspaceTester;

import com.google.common.collect.ImmutableMap;
import com.mongodb.MongoClient;
import com.mongodb.client.MongoCollection;
import com.mongodb.client.MongoDatabase;

/*
* These tests are naughty because they bypass the public API and test the internals directly.
* In general, you shouldn't do that. For some of these tests, there's no good way to
* test some of the code without sticking our filthy paws into the class guts (like
* code that pulls DB data, modifies it, and saves it back while atomically checking it
* hasn't changed since the pull and looping if it has), but some code was written too
* protectively and probably could be refactored so the testing accessing internals aren't
* necessary.
*/


public class MongoInternalsTest {

private static MongoDatabase db;
Expand Down Expand Up @@ -1202,5 +1218,92 @@ public void typeFieldsOnClone() throws Exception {

checkTypeFields(2, 1, 2);
}

private Method getSetWorkspaceMetadataInternalMethod() throws Exception {
final Method m = MongoWorkspaceDB.class.getDeclaredMethod(
"_internal_setWorkspaceMeta",
int.class,
int.class,
Document.class,
Document.class);
m.setAccessible(true);
return m;
}

private ResolvedWorkspaceID setWorkspaceMetadataCreateWorkspace(
Map<String, String> meta) throws Exception {
final WorkspaceUser user = new WorkspaceUser("foo");
final ResolvedWorkspaceID rwsi = new ResolvedWorkspaceID(1, "foo", false, false);
ws.createWorkspace(user, rwsi.getName(), false, null, new WorkspaceUserMetadata(meta));
return rwsi;
}

@Test
public void setWorkspaceMetadataInternalsPassAfterPreviousAttempts() throws Exception {
final Method m = getSetWorkspaceMetadataInternalMethod();
final ResolvedWorkspaceID rwsi = setWorkspaceMetadataCreateWorkspace(
ImmutableMap.of("a", "b"));

final Instant result = (Instant) m.invoke(
mwdb,
3,
5,
new Document("ws", 1).append("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "b"))),
new Document("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "x"), ImmutableMap.of("k", "y", "v", "z")))
);
assertCloseToNow(result);
final Map<String, String> meta = mwdb.getWorkspaceInformation(
new WorkspaceUser("foo"), rwsi).getUserMeta().getMetadata();
assertThat("incorrect meta", meta, is(ImmutableMap.of("a", "x", "y", "z")));
}

@Test
public void setWorkspaceMetadataInternalsNoUpdateAfterPreviousAttempts() throws Exception {
final Method m = getSetWorkspaceMetadataInternalMethod();
final ResolvedWorkspaceID rwsi = setWorkspaceMetadataCreateWorkspace(
ImmutableMap.of("a", "b"));

final Instant result = (Instant) m.invoke(
mwdb,
3,
5,
new Document("ws", 1).append("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "c"))),
new Document("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "x"), ImmutableMap.of("k", "y", "v", "z")))
);
assertThat("incorrect time", result, is(nullValue()));
final Map<String, String> meta = mwdb.getWorkspaceInformation(
new WorkspaceUser("foo"), rwsi).getUserMeta().getMetadata();
assertThat("incorrect meta", meta, is(ImmutableMap.of("a", "b")));
}

@Test
public void setWorkspaceMetadataInternalsFailAfterPreviousAttempts() throws Exception {
final Method m = getSetWorkspaceMetadataInternalMethod();
final ResolvedWorkspaceID rwsi = setWorkspaceMetadataCreateWorkspace(
ImmutableMap.of("a", "b"));
try {
m.invoke(
mwdb,
5,
5,
new Document("ws", 1).append("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "c"))),
new Document("meta", Arrays.asList(
ImmutableMap.of("k", "a", "v", "x"),
ImmutableMap.of("k", "y", "v", "z")))
);
} catch (InvocationTargetException e) {
assertExceptionCorrect(e.getCause(), new WorkspaceCommunicationException(
"Failed to update metadata 5 times"));
}
final Map<String, String> meta = mwdb.getWorkspaceInformation(
new WorkspaceUser("foo"), rwsi).getUserMeta().getMetadata();
assertThat("incorrect meta", meta, is(ImmutableMap.of("a", "b")));
}


}
Loading

0 comments on commit b02d1b1

Please sign in to comment.