Repository: OpenRefine/OpenRefine (java)

Issue:
Not all test projects created during ui_testing are getting scraped
<!-- Describe the bug - Please add a clear and concise description of the bug above this line. You can delete this line if you want. It will be hidden in the final bug report -->
We already have an afterEach hook that deletes all the projects created during the test but due to some bug, not all projects are being deleted.
### To Reproduce
Steps to reproduce the behavior:
1. Open Cypress application
2. Run a Spec
3. See a few outstanding projects remaining



### Current Results
<!-- What results occurred or were shown. -->
Few projects remains after successful afterEach hook.
### Expected Behavior
<!-- A clear and concise description of what you expected to happen or to show. -->
All projects should be flushed out


Pull request description:
Fix DeleteProject NPE during e2e tests. Fixes #3479.
Fixes #3479 

- refactor & DRY up tag management code
- add some tests for project related commands
- add tests for tag commands


Patch:
diff --git a/main/pom.xml b/main/pom.xml
index 9a9e2c4e8..033738f26 100644
--- a/main/pom.xml
+++ b/main/pom.xml
@@ -228,8 +228,8 @@
       <version>${commons-validator.version}</version>
     </dependency>
     <dependency>
-      <groupId>velocity</groupId>
-      <artifactId>velocity</artifactId>
+      <groupId>org.apache.velocity</groupId>
+      <artifactId>velocity-engine-core</artifactId>
       <version>${velocity.version}</version>
     </dependency>
     <dependency>
diff --git a/main/src/com/google/refine/ProjectManager.java b/main/src/com/google/refine/ProjectManager.java
index 40530adbc..4ce487239 100644
--- a/main/src/com/google/refine/ProjectManager.java
+++ b/main/src/com/google/refine/ProjectManager.java
@@ -38,6 +38,7 @@ import java.io.InputStream;
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -76,7 +77,10 @@ public abstract class ProjectManager {
     static protected final Duration QUICK_SAVE_MAX_TIME = Duration.ofSeconds(30);
 
     protected Map<Long, ProjectMetadata> _projectsMetadata;
-    protected Map<String, Integer> _projectsTags;// TagName, number of projects having that tag
+    /**
+     * Count of projects which have the given tag
+     */
+    private Map<String, Integer> _projectsTags;
     protected PreferenceStore _preferenceStore;
 
     final Logger logger = LoggerFactory.getLogger(this.getClass());
@@ -104,7 +108,7 @@ public abstract class ProjectManager {
         _projectsMetadata = new HashMap<Long, ProjectMetadata>();
         _preferenceStore = new PreferenceStore();
         _projects = new HashMap<Long, Project>();
-        _projectsTags = new HashMap<String, Integer>();
+        _projectsTags = new HashMap<>();
 
         preparePreferenceStore(_preferenceStore);
     }
@@ -134,18 +138,7 @@ public abstract class ProjectManager {
             projectMetadata.setRowCount(project.rows.size());
             _projects.put(project.id, project);
             _projectsMetadata.put(project.id, projectMetadata);
-            if (_projectsTags == null)
-                _projectsTags = new HashMap<>();
-            String[] tags = projectMetadata.getTags();
-            if (tags != null) {
-                for (String tag : tags) {
-                    if (_projectsTags.containsKey(tag)) {
-                        _projectsTags.put(tag, _projectsTags.get(tag) + 1);
-                    } else {
-                        _projectsTags.put(tag, 1);
-                    }
-                }
-            }
+            addProjectTags(projectMetadata.getTags());
         }
     }
 
@@ -348,7 +341,7 @@ public abstract class ProjectManager {
     }
 
     /**
-     * Gets the project metadata from memory Requires that the metadata has already been loaded from the data store
+     * Gets the project metadata from memory. Requires that the metadata has already been loaded from the data store
      * 
      * @param id
      * @return
@@ -358,7 +351,7 @@ public abstract class ProjectManager {
     }
 
     /**
-     * Gets the project metadata from memory Requires that the metadata has already been loaded from the data store
+     * Gets the project metadata from memory. Requires that the metadata has already been loaded from the data store
      * 
      * @param name
      * @return
@@ -488,11 +481,59 @@ public abstract class ProjectManager {
         return _projectsMetadata;
     }
 
+    /**
+     * Increment usage count for all tags in the list.
+     *
+     * @param tags
+     *            String[] array containing all tag names to be updated
+     */
+    public void addProjectTags(String[] tags) {
+        if (tags != null) {
+            for (String tag : tags) {
+                if (_projectsTags.containsKey(tag)) {
+                    _projectsTags.put(tag, _projectsTags.get(tag) + 1);
+                } else {
+                    _projectsTags.put(tag, 1);
+                }
+            }
+        }
+    }
+
+    /**
+     * Decrement usage count for all tags in the list and remove any which are no longer used at all.
+     *
+     * @param tags
+     *            String[] array containing all tag names to be updated
+     */
+    public void removeProjectTags(String[] tags) {
+        for (String tag : tags) {
+            if (_projectsTags.containsKey(tag)) {
+                int occurrence = _projectsTags.get(tag);
+                if (occurrence <= 1)
+                    _projectsTags.remove(tag);
+                else {
+                    _projectsTags.put(tag, occurrence - 1);
+                }
+            }
+        }
+    }
+
+    /**
+     * Get tag names and usage counts for all projects
+     *
+     * @return unmodifiable map keyed by tag name with usage counts as the values
+     */
+    @JsonIgnore
+    public Map<String, Integer> getAllProjectsTags() {
+        return Collections.unmodifiableMap(_projectsTags);
+    }
+
     /**
      * Gets all the project tags currently held in memory
-     * 
-     * @return
+     *
+     * @deprecated Deprecated for v3.8. Use {@link #getAllProjectsTags()}
      */
+    @Deprecated
     @JsonIgnore
     public Map<String, Integer> getAllProjectTags() {
         return _projectsTags;
diff --git a/main/src/com/google/refine/commands/project/DeleteProjectCommand.java b/main/src/com/google/refine/commands/project/DeleteProjectCommand.java
index efc502625..85293fff7 100644
--- a/main/src/com/google/refine/commands/project/DeleteProjectCommand.java
+++ b/main/src/com/google/refine/commands/project/DeleteProjectCommand.java
@@ -34,14 +34,12 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 package com.google.refine.commands.project;
 
 import java.io.IOException;
-import java.util.Map;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import com.google.refine.ProjectManager;
-import com.google.refine.ProjectMetadata;
 import com.google.refine.commands.Command;
 
 public class DeleteProjectCommand extends Command {
@@ -58,21 +56,6 @@ public class DeleteProjectCommand extends Command {
         try {
             long projectID = Long.parseLong(request.getParameter("project"));
 
-            // Remove the project tags from the general map
-            Map<String, Integer> allProjectTags = ProjectManager.singleton.getAllProjectTags();
-            ProjectMetadata metadata = ProjectManager.singleton.getProjectMetadata(projectID);
-            for (String tag : metadata.getTags()) {
-                if (allProjectTags.containsKey(tag)) {
-                    int occurrence = allProjectTags.get(tag);
-
-                    if (occurrence == 1)
-                        allProjectTags.remove(tag);
-                    else {
-                        allProjectTags.put(tag, occurrence - 1);
-                    }
-                }
-            }
-
             ProjectManager.singleton.deleteProject(projectID);
 
             respond(response, "{ \"code\" : \"ok\" }");
@@ -81,4 +64,5 @@ public class DeleteProjectCommand extends Command {
             respondException(response, e);
         }
     }
+
 }
diff --git a/main/src/com/google/refine/commands/project/RenameProjectCommand.java b/main/src/com/google/refine/commands/project/RenameProjectCommand.java
index 4dad20473..a0fe53c70 100644
--- a/main/src/com/google/refine/commands/project/RenameProjectCommand.java
+++ b/main/src/com/google/refine/commands/project/RenameProjectCommand.java
@@ -58,7 +58,7 @@ public class RenameProjectCommand extends Command {
 
             pm.setName(name);
 
-            respond(response, "{ \"code\" : \"ok\" }");
+            respond(response, "{\"code\":\"ok\"}");
         } catch (Exception e) {
             respondException(response, e);
         }
diff --git a/main/src/com/google/refine/commands/project/SetProjectMetadataCommand.java b/main/src/com/google/refine/commands/project/SetProjectMetadataCommand.java
index 09205950f..3cf45c486 100644
--- a/main/src/com/google/refine/commands/project/SetProjectMetadataCommand.java
+++ b/main/src/com/google/refine/commands/project/SetProjectMetadataCommand.java
@@ -54,7 +54,7 @@ public class SetProjectMetadataCommand extends Command {
         ProjectMetadata meta = null;
 
         if (project == null) {
-            respond(response, "{ \"code\" : \"error\", \"message\" : \"Project cannot be found\" }");
+            respond(response, "{\"code\":\"error\",\"message\":\"Project cannot be found\"}");
             return;
         }
 
diff --git a/main/src/com/google/refine/commands/project/SetProjectTagsCommand.java b/main/src/com/google/refine/commands/project/SetProjectTagsCommand.java
index 923ce2faf..5e726659c 100644
--- a/main/src/com/google/refine/commands/project/SetProjectTagsCommand.java
+++ b/main/src/com/google/refine/commands/project/SetProjectTagsCommand.java
@@ -26,16 +26,12 @@
 package com.google.refine.commands.project;
 
 import java.io.IOException;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Map;
+import java.util.stream.Stream;
 
 import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import com.google.common.base.CharMatcher;
-
 import com.google.refine.ProjectManager;
 import com.google.refine.ProjectMetadata;
 import com.google.refine.commands.Command;
@@ -66,43 +62,16 @@ public class SetProjectTagsCommand extends Command {
         String oldT = request.getParameter("old");
         String newT = request.getParameter("new");
 
-        Map<String, Integer> allProjectTags = ProjectManager.singleton.getAllProjectTags();
-
-        // Lets remove the old tags from the general map
-        String[] oldTags = oldT.split(",");
-        for (String tag : oldTags) {
-            if (allProjectTags != null && allProjectTags.containsKey(tag)) {
-                int occurrence = allProjectTags.get(tag);
+        String[] oldTags = oldT == null ? new String[0] : oldT.split(",");
+        ProjectManager.singleton.removeProjectTags(oldTags);
 
-                if (occurrence == 1) {
-                    allProjectTags.remove(tag);
-                } else {
-                    allProjectTags.put(tag, occurrence - 1);
-                }
-            }
-        }
+        String[] newTags = newT == null ? new String[0]
+                : Stream.of(newT.split(" |\\,")).map(String::trim).filter(t -> !t.isEmpty()).toArray(String[]::new);
+        ProjectManager.singleton.addProjectTags(newTags);
 
-        // Lets add the new tags to the general map
-        String[] newTags = newT.split(" |\\,");
-        List<String> polishedTags = new ArrayList<String>(newTags.length);
-        for (String tag : newTags) {
-            tag = CharMatcher.whitespace().trimFrom(tag);
+        metadata.setTags(newTags);
 
-            if (!tag.isEmpty()) {
-                if (allProjectTags != null) {
-                    if (allProjectTags.containsKey(tag)) {
-                        allProjectTags.put(tag, allProjectTags.get(tag) + 1);
-                    } else {
-                        allProjectTags.put(tag, 1);
-                    }
-                }
-                polishedTags.add(tag);
-            }
-        }
-
-        // Let's update the project tags
-        metadata.setTags(polishedTags.toArray(new String[polishedTags.size()]));
-
-        respond(response, "{ \"code\" : \"ok\" }");
+        respond(response, "{\"code\":\"ok\"}");
     }
+
 }
diff --git a/main/src/com/google/refine/commands/workspace/GetAllProjectTagsCommand.java b/main/src/com/google/refine/commands/workspace/GetAllProjectTagsCommand.java
index 44000c74e..0fdd801b3 100644
--- a/main/src/com/google/refine/commands/workspace/GetAllProjectTagsCommand.java
+++ b/main/src/com/google/refine/commands/workspace/GetAllProjectTagsCommand.java
@@ -55,7 +55,7 @@ public class GetAllProjectTagsCommand extends Command {
     public void doGet(HttpServletRequest request, HttpServletResponse response)
             throws ServletException, IOException {
 
-        Map<String, Integer> tagMap = ProjectManager.singleton.getAllProjectTags();
+        Map<String, Integer> tagMap = ProjectManager.singleton.getAllProjectsTags();
         Set<String> tags = tagMap == null ? Collections.emptySet() : tagMap.keySet();
         respondJSON(response, new AllProjectsTags(tags));
     }
diff --git a/main/src/com/google/refine/io/FileProjectManager.java b/main/src/com/google/refine/io/FileProjectManager.java
index d950766a4..09d175538 100644
--- a/main/src/com/google/refine/io/FileProjectManager.java
+++ b/main/src/com/google/refine/io/FileProjectManager.java
@@ -39,7 +39,6 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map.Entry;
 import java.util.Set;
@@ -130,19 +129,7 @@ public class FileProjectManager extends ProjectManager {
 
             if (metadata != null) {
                 _projectsMetadata.put(projectID, metadata);
-                if (_projectsTags == null) {
-                    _projectsTags = new HashMap<String, Integer>();
-                }
-
-                if (metadata != null && metadata.getTags() != null) {
-                    for (String tag : metadata.getTags()) {
-                        if (_projectsTags.containsKey(tag)) {
-                            _projectsTags.put(tag, _projectsTags.get(tag) + 1);
-                        } else {
-                            _projectsTags.put(tag, 1);
-                        }
-                    }
-                }
+                addProjectTags(metadata.getTags());
                 return true;
             } else {
                 return false;
@@ -341,6 +328,16 @@ public class FileProjectManager extends ProjectManager {
     @Override
     public void deleteProject(long projectID) {
         synchronized (this) {
+
+            // Remove this project's tags from the overall map
+            ProjectMetadata metadata = getProjectMetadata(projectID);
+            if (metadata != null) {
+                String[] tags = metadata.getTags();
+                if (tags != null) { // should only ever happen during tests with mocked ProjectMetadata
+                    removeProjectTags(tags);
+                }
+            }
+
             removeProject(projectID);
 
             File dir = getProjectDir(projectID);
@@ -470,14 +467,8 @@ public class FileProjectManager extends ProjectManager {
 
             _projectsMetadata.put(id, metadata);
 
-            if (metadata != null && metadata.getTags() != null) {
-                for (String tag : metadata.getTags()) {
-                    if (_projectsTags.containsKey(tag)) {
-                        _projectsTags.put(tag, _projectsTags.get(tag) + 1);
-                    } else {
-                        _projectsTags.put(tag, 1);
-                    }
-                }
+            if (metadata != null) {
+                addProjectTags(metadata.getTags());
             }
         }
     }
diff --git a/main/webapp/WEB-INF/velocity.properties b/main/webapp/WEB-INF/velocity.properties
index 5e78fcb88..8e9747099 100644
--- a/main/webapp/WEB-INF/velocity.properties
+++ b/main/webapp/WEB-INF/velocity.properties
@@ -7,5 +7,11 @@ velocimacro.context.localscope=false
 velocimacro.library.autoreload=false
 velocimacro.permissions.allow.inline.local.scope=true
 
+# UTF-8 is the default encoding for 2.x, so these might be superfluous
 input.encoding=UTF-8
 output.encoding=UTF-8
+
+# Settings to maximize backward compatibility with 1.x
+runtime.conversion.handler = none
+space.gobbling = bc
+directive.if.emptycheck = false
diff --git a/pom.xml b/pom.xml
index 62e6629ad..0e4098cc3 100644
--- a/pom.xml
+++ b/pom.xml
@@ -80,7 +80,7 @@
     <jetty.version>10.0.16</jetty.version>
     <okhttp.version>4.10.0</okhttp.version>
     <jena.version>4.10.0</jena.version>
-    <poi.version>5.2.4</poi.version>
+    <poi.version>5.2.5</poi.version>
     <commons-lang3.version>3.14.0</commons-lang3.version>
     <commons-fileupload.version>1.5</commons-fileupload.version>
     <commons-collections.version>3.2.2</commons-collections.version>
@@ -90,7 +90,7 @@
     <!-- v1.15 of commons-codec is needed for Jena 3.14+ - copied from org.apache.jena:jena:pom -->
     <commons-codec.version>1.16.0</commons-codec.version>
     <commons-compress.version>1.25.0</commons-compress.version>
-    <velocity.version>1.5</velocity.version>
+    <velocity.version>2.3</velocity.version>
     <marc4j.version>2.9.5</marc4j.version>
     <jsoup.version>1.15.3</jsoup.version>
     <odfdom-java.version>0.9.0</odfdom-java.version> <!-- do not update to 0.10.0, see issue #4397 -->


Test patch:
diff --git a/main/tests/server/src/com/google/refine/ProjectManagerTests.java b/main/tests/server/src/com/google/refine/ProjectManagerTests.java
index 740ab7e91..3204fd76c 100644
--- a/main/tests/server/src/com/google/refine/ProjectManagerTests.java
+++ b/main/tests/server/src/com/google/refine/ProjectManagerTests.java
@@ -41,10 +41,12 @@ import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
+import static org.testng.AssertJUnit.assertEquals;
 
 import java.lang.reflect.Field;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Set;
 
 import org.mockito.Mockito;
 import org.slf4j.LoggerFactory;
@@ -63,6 +65,7 @@ public class ProjectManagerTests extends RefineTest {
 
     private static final Instant BASE_DATE = Instant.parse("1970-01-02T00:30:00Z");
     private static final int ROW_COUNT = 3;
+    private static final String[] TAGS = { "testtag1", "testtag2" };
     ProjectManagerStub pm;
     ProjectManagerStub SUT;
     Project project;
@@ -88,6 +91,7 @@ public class ProjectManagerTests extends RefineTest {
         addRows(project);
 
         metadata = mock(ProjectMetadata.class);
+        when(metadata.getTags()).thenReturn(TAGS);
         procmgr = mock(ProcessManager.class);
         when(project.getProcessManager()).thenReturn(procmgr);
         when(procmgr.hasPending()).thenReturn(false); // always false for now, but should test separately
@@ -118,6 +122,7 @@ public class ProjectManagerTests extends RefineTest {
 
         verifyNoMoreInteractions(project);
         verifyNoMoreInteractions(metadata);
+        assertEquals(SUT.getAllProjectsTags().keySet(), Set.of(TAGS));
     }
 
     // TODO test registerProject in race condition
diff --git a/main/tests/server/src/com/google/refine/commands/CommandTestBase.java b/main/tests/server/src/com/google/refine/commands/CommandTestBase.java
index 7c90339c1..8ee589834 100644
--- a/main/tests/server/src/com/google/refine/commands/CommandTestBase.java
+++ b/main/tests/server/src/com/google/refine/commands/CommandTestBase.java
@@ -3,6 +3,8 @@ package com.google.refine.commands;
 
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 
 import java.io.IOException;
 import java.io.PrintWriter;
@@ -11,11 +13,15 @@ import java.io.StringWriter;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.JsonNode;
 import org.testng.annotations.BeforeMethod;
 
+import com.google.refine.RefineTest;
+import com.google.refine.util.ParsingUtilities;
 import com.google.refine.util.TestUtils;
 
-public class CommandTestBase {
+public class CommandTestBase extends RefineTest {
 
     protected HttpServletRequest request = null;
     protected HttpServletResponse response = null;
@@ -40,4 +46,14 @@ public class CommandTestBase {
     protected void assertCSRFCheckFailed() {
         TestUtils.assertEqualsAsJson(writer.toString(), "{\"code\":\"error\",\"message\":\"Missing or invalid csrf_token parameter\"}");
     }
+
+    /**
+     * Convenience method to check that CSRF protection was NOT triggered
+     */
+    protected void assertErrorNotCSRF() throws JsonProcessingException {
+        String response = writer.toString();
+        JsonNode node = ParsingUtilities.mapper.readValue(response, JsonNode.class);
+        assertEquals(node.get("code").toString(), "\"error\"");
+        assertFalse(response.contains("Missing or invalid csrf_token parameter"));
+    }
 }
diff --git a/main/tests/server/src/com/google/refine/commands/project/RenameProjectCommandTests.java b/main/tests/server/src/com/google/refine/commands/project/RenameProjectCommandTests.java
index 4dbaa3c56..5b583ff23 100644
--- a/main/tests/server/src/com/google/refine/commands/project/RenameProjectCommandTests.java
+++ b/main/tests/server/src/com/google/refine/commands/project/RenameProjectCommandTests.java
@@ -1,6 +1,8 @@
 
 package com.google.refine.commands.project;
 
+import static org.mockito.Mockito.when;
+
 import java.io.IOException;
 
 import javax.servlet.ServletException;
@@ -8,6 +10,7 @@ import javax.servlet.ServletException;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import com.google.refine.commands.Command;
 import com.google.refine.commands.CommandTestBase;
 
 public class RenameProjectCommandTests extends CommandTestBase {
@@ -22,4 +25,12 @@ public class RenameProjectCommandTests extends CommandTestBase {
         command.doPost(request, response);
         assertCSRFCheckFailed();
     }
+
+    @Test
+    public void testNoProjectId() throws ServletException, IOException {
+        when(request.getParameter("csrf_token")).thenReturn(Command.csrfFactory.getFreshToken());
+        command.doPost(request, response);
+        assertErrorNotCSRF();
+    }
+
 }
diff --git a/main/tests/server/src/com/google/refine/commands/project/SetProjectMetadataCommandTests.java b/main/tests/server/src/com/google/refine/commands/project/SetProjectMetadataCommandTests.java
index ccfb38f05..aae52672b 100644
--- a/main/tests/server/src/com/google/refine/commands/project/SetProjectMetadataCommandTests.java
+++ b/main/tests/server/src/com/google/refine/commands/project/SetProjectMetadataCommandTests.java
@@ -33,6 +33,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 package com.google.refine.commands.project;
 
+import static com.google.refine.util.TestUtils.assertEqualsAsJson;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -41,6 +42,7 @@ import static org.mockito.Mockito.when;
 
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.io.StringWriter;
 import java.util.List;
 import java.util.Map;
 
@@ -59,12 +61,12 @@ import org.testng.annotations.Test;
 
 import com.google.refine.ProjectManager;
 import com.google.refine.ProjectMetadata;
-import com.google.refine.RefineTest;
 import com.google.refine.commands.Command;
+import com.google.refine.commands.CommandTestBase;
 import com.google.refine.model.Project;
 import com.google.refine.util.ParsingUtilities;
 
-public class SetProjectMetadataCommandTests extends RefineTest {
+public class SetProjectMetadataCommandTests extends CommandTestBase {
 
     @Override
     @BeforeTest
@@ -77,22 +79,19 @@ public class SetProjectMetadataCommandTests extends RefineTest {
 
     // variables
     long PROJECT_ID_LONG = 1234;
-    String PROJECT_ID = "1234";
+    String PROJECT_ID = Long.toString(PROJECT_ID_LONG);
     String SUBJECT = "subject for project";
 
     // mocks
-    HttpServletRequest request = null;
-    HttpServletResponse response = null;
     ProjectManager projMan = null;
     Project proj = null;
-    PrintWriter pw = null;
 
     @BeforeMethod
     public void SetUp() throws IOException {
         projMan = mock(ProjectManager.class);
         ProjectManager.singleton = projMan;
         proj = mock(Project.class);
-        pw = mock(PrintWriter.class);
+        writer = new StringWriter();
 
         request = mock(HttpServletRequest.class);
         response = mock(HttpServletResponse.class);
@@ -108,7 +107,7 @@ public class SetProjectMetadataCommandTests extends RefineTest {
         when(proj.getMetadata()).thenReturn(metadata);
 
         try {
-            when(response.getWriter()).thenReturn(pw);
+            when(response.getWriter()).thenReturn(new PrintWriter(writer));
         } catch (IOException e1) {
             Assert.fail();
         }
@@ -121,7 +120,7 @@ public class SetProjectMetadataCommandTests extends RefineTest {
         projMan = null;
         ProjectManager.singleton = null;
         proj = null;
-        pw = null;
+        writer = null;
         request = null;
         response = null;
     }
@@ -155,15 +154,13 @@ public class SetProjectMetadataCommandTests extends RefineTest {
         } catch (IOException e) {
             Assert.fail();
         }
-        verify(pw, times(1)).write("{ \"code\" : \"ok\" }");
+        assertEqualsAsJson(writer.toString(), "{\"code\":\"ok\"}");
 
         Assert.assertEquals(proj.getMetadata().getSubject(), SUBJECT);
     }
 
     /**
      * set a user defined metadata field
-     * 
-     * @throws JSONException
      */
     @Test
     public void setUserMetadataFieldTest() {
@@ -191,7 +188,7 @@ public class SetProjectMetadataCommandTests extends RefineTest {
         } catch (IOException e) {
             Assert.fail();
         }
-        verify(pw, times(1)).write("{ \"code\" : \"ok\" }");
+        assertEqualsAsJson(writer.toString(), "{\"code\":\"ok\"}");
 
         ObjectNode obj = (ObjectNode) proj.getMetadata().getUserMetadata().get(0);
         Assert.assertEquals(obj.get("name").asText(), "clientID");
@@ -235,6 +232,20 @@ public class SetProjectMetadataCommandTests extends RefineTest {
         Assert.assertThrows(() -> meta.setAnyField("customMetadata", customMetadata));
     }
 
+    @Test
+    public void testCSRFProtection() throws ServletException, IOException {
+        when(request.getParameter("csrf_token")).thenReturn("");
+        SUT.doPost(request, response);
+        assertCSRFCheckFailed();
+    }
+
+    @Test
+    public void testNoProjectId() throws ServletException, IOException {
+        when(request.getParameter("project")).thenReturn(null);
+        SUT.doPost(request, response);
+        assertErrorNotCSRF();
+    }
+
     private String getValidCustomMetadata() {
         return "{\n" + //
                 "  \"name\":\"test\",\n" + //
diff --git a/main/tests/server/src/com/google/refine/commands/project/SetProjectTagsCommandTests.java b/main/tests/server/src/com/google/refine/commands/project/SetProjectTagsCommandTests.java
index bd2027755..3d72db5a4 100644
--- a/main/tests/server/src/com/google/refine/commands/project/SetProjectTagsCommandTests.java
+++ b/main/tests/server/src/com/google/refine/commands/project/SetProjectTagsCommandTests.java
@@ -1,24 +1,114 @@
 
 package com.google.refine.commands.project;
 
+import static com.google.refine.util.TestUtils.assertEqualsAsJson;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.testng.Assert.assertEquals;
+
 import java.io.IOException;
+import java.io.PrintWriter;
+import java.util.Arrays;
+import java.util.Map;
 
 import javax.servlet.ServletException;
 
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.node.ArrayNode;
+import org.testng.Assert;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import com.google.refine.ProjectManager;
+import com.google.refine.ProjectManagerStub;
+import com.google.refine.ProjectMetadata;
+import com.google.refine.commands.Command;
 import com.google.refine.commands.CommandTestBase;
+import com.google.refine.model.Project;
+import com.google.refine.model.ProjectStub;
+import com.google.refine.util.ParsingUtilities;
 
 public class SetProjectTagsCommandTests extends CommandTestBase {
 
+    private static long PROJECT_ID = 1234;
+    private String[] TAGS = new String[] { "tag1", "tag2" };
+    ProjectManager projectManager = null;
+    Project project = null;
+    PrintWriter pw = null;
+
     @BeforeMethod
-    public void setUpCommand() {
+    public void setUpCommand() throws JsonProcessingException {
         command = new SetProjectTagsCommand();
+
+        pw = new PrintWriter(writer);
+
+        ProjectMetadata metadata = new ProjectMetadata();
+        metadata.setUserMetadata((ArrayNode) ParsingUtilities.mapper.readTree("[ {name: \"clientID\", display: true} ]"));
+        metadata.setTags(TAGS);
+
+        projectManager = new ProjectManagerStub();
+        ProjectManager.singleton = projectManager;
+        project = new ProjectStub(PROJECT_ID);
+        ProjectManager.singleton.registerProject(project, metadata);
+
+        // mock dependencies
+        when(request.getParameter("project")).thenReturn(Long.toString(PROJECT_ID));
+        when(request.getParameter("csrf_token")).thenReturn(Command.csrfFactory.getFreshToken());
+
+        try {
+            when(response.getWriter()).thenReturn(pw);
+        } catch (IOException e1) {
+            Assert.fail();
+        }
+    }
+
+    @Test
+    public void setProjectTagsTest() {
+        // Check that our tags got set up correctly at project registration time.
+        assertEquals(ProjectManager.singleton.getAllProjectsTags().keySet().stream().sorted().toArray(String[]::new), TAGS);
+
+        when(request.getParameter("old")).thenReturn(String.join(",", TAGS));
+        when(request.getParameter("new")).thenReturn("a b c");
+        // run
+        try {
+            command.doPost(request, response);
+        } catch (ServletException e) {
+            Assert.fail();
+        } catch (IOException e) {
+            Assert.fail();
+        }
+
+        // verify
+        verify(request, times(1)).getParameter("csrf_token");
+        verify(request, times(1)).getParameter("project");
+
+        verify(response, times(1))
+                .setHeader("Content-Type", "application/json");
+        try {
+            verify(response, times(1)).getWriter();
+        } catch (IOException e) {
+            Assert.fail();
+        }
+        assertEqualsAsJson(writer.toString(), "{\"code\":\"ok\"}");
+
+        String[] tags = project.getMetadata().getTags();
+        assertEquals(tags.length, 3);
+        Arrays.sort(tags);
+        assertEquals(tags[1], "b");
+
+        Map<String, Integer> tagCounts = ProjectManager.singleton.getAllProjectsTags();
+        assertEquals(tagCounts.size(), 3);
+        assertEquals(ProjectManager.singleton.getAllProjectsTags().keySet().stream().sorted().toArray(String[]::new)[1], "b");
+
+        projectManager.deleteProject(PROJECT_ID);
+        // TODO: ProjectManagerStub doesn't manage tags on project delete, so this fails
+//        assertEquals(ProjectManager.singleton.getAllProjectsTags().size(), 0);
     }
 
     @Test
     public void testCSRFProtection() throws ServletException, IOException {
+        when(request.getParameter("csrf_token")).thenReturn(null);
         command.doPost(request, response);
         assertCSRFCheckFailed();
     }
diff --git a/main/tests/server/src/com/google/refine/io/FileProjectManagerTests.java b/main/tests/server/src/com/google/refine/io/FileProjectManagerTests.java
index a03583dba..dce133832 100644
--- a/main/tests/server/src/com/google/refine/io/FileProjectManagerTests.java
+++ b/main/tests/server/src/com/google/refine/io/FileProjectManagerTests.java
@@ -66,7 +66,6 @@ public class FileProjectManagerTests {
         protected FileProjectManagerStub(File dir) {
             super(dir);
             _projectsMetadata.put(5555L, mock(ProjectMetadata.class));
-
         }
     }
 
@@ -115,6 +114,7 @@ public class FileProjectManagerTests {
     public void deleteProjectAndSaveWorkspace() throws IOException {
         FileProjectManager manager = new FileProjectManagerStub(workspaceDir);
         manager.saveWorkspace();
+        // TODO: Test tag updating on project deletion
         manager.deleteProject(5555);
         manager.saveWorkspace();