Merge pull request IQSS#10958 from IQSS/fix/updateAttributes

Fixes updateAttributes endpoint

src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java

@@ -621,62 +621,22 @@ public Response deleteDataverse(@Context ContainerRequestContext crc, @PathParam
     public Response updateAttribute(@Context ContainerRequestContext crc, @PathParam("identifier") String identifier,
                                     @PathParam("attribute") String attribute, @QueryParam("value") String value) {
         try {
-            Dataverse collection = findDataverseOrDie(identifier);
-            User user = getRequestUser(crc);
-            DataverseRequest dvRequest = createDataverseRequest(user);
-
-            // TODO: The cases below use hard coded strings, because we have no place for definitions of those!
-            //       They are taken from util.json.JsonParser / util.json.JsonPrinter. This shall be changed.
-            //       This also should be extended to more attributes, like the type, theme, contacts, some booleans, etc.
-            switch (attribute) {
-                case "alias":
-                    collection.setAlias(value);
-                    break;
-                case "name":
-                    collection.setName(value);
-                    break;
-                case "description":
-                    collection.setDescription(value);
-                    break;
-                case "affiliation":
-                    collection.setAffiliation(value);
-                    break;
-                /* commenting out the code from the draft pr #9462:
-                case "versionPidsConduct":
-                    CollectionConduct conduct = CollectionConduct.findBy(value);
-                    if (conduct == null) {
-                        return badRequest("'" + value + "' is not one of [" +
-                            String.join(",", CollectionConduct.asList()) + "]");
-                    }
-                    collection.setDatasetVersionPidConduct(conduct);
-                    break;
-                 */
-                case "filePIDsEnabled":
-                    if(!user.isSuperuser()) {
-                        return forbidden("You must be a superuser to change this setting");
-                    }
-                    if(!settingsService.isTrueForKey(SettingsServiceBean.Key.AllowEnablingFilePIDsPerCollection, false)) {
-                        return forbidden("Changing File PID policy per collection is not enabled on this server");
-                    }
-                    collection.setFilePIDsEnabled(parseBooleanOrDie(value));
-                    break;
-                default:
-                    return badRequest("'" + attribute + "' is not a supported attribute");
-            }
-
-            // Off to persistence layer
-            execCommand(new UpdateDataverseCommand(collection, null, null, dvRequest, null));
-
-            // Also return modified collection to user
-            return ok("Update successful", JsonPrinter.json(collection));
-
-        // TODO: This is an anti-pattern, necessary due to this bean being an EJB, causing very noisy and unnecessary
-        //       logging by the EJB container for bubbling exceptions. (It would be handled by the error handlers.)
+            Dataverse dataverse = findDataverseOrDie(identifier);
+            Object formattedValue = formatAttributeValue(attribute, value);
+            dataverse = execCommand(new UpdateDataverseAttributeCommand(createDataverseRequest(getRequestUser(crc)), dataverse, attribute, formattedValue));
+            return ok("Update successful", JsonPrinter.json(dataverse));
         } catch (WrappedResponse e) {
             return e.getResponse();
         }
     }
 
+    private Object formatAttributeValue(String attribute, String value) throws WrappedResponse {
+        if (attribute.equals("filePIDsEnabled")) {
+            return parseBooleanOrDie(value);
+        }
+        return value;
+    }
+
     @GET
     @AuthRequired
     @Path("{identifier}/inputLevels")

src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseAttributeCommand.java

@@ -0,0 +1,110 @@
+package edu.harvard.iq.dataverse.engine.command.impl;
+
+import edu.harvard.iq.dataverse.Dataverse;
+import edu.harvard.iq.dataverse.authorization.Permission;
+import edu.harvard.iq.dataverse.engine.command.AbstractCommand;
+import edu.harvard.iq.dataverse.engine.command.CommandContext;
+import edu.harvard.iq.dataverse.engine.command.DataverseRequest;
+import edu.harvard.iq.dataverse.engine.command.RequiredPermissions;
+import edu.harvard.iq.dataverse.engine.command.exception.CommandException;
+import edu.harvard.iq.dataverse.engine.command.exception.IllegalCommandException;
+import edu.harvard.iq.dataverse.engine.command.exception.PermissionException;
+import edu.harvard.iq.dataverse.settings.SettingsServiceBean;
+
+import java.util.Collections;
+
+/**
+ * Command to update an existing Dataverse attribute.
+ */
+@RequiredPermissions(Permission.EditDataverse)
+public class UpdateDataverseAttributeCommand extends AbstractCommand<Dataverse> {
+
+    private static final String ATTRIBUTE_ALIAS = "alias";
+    private static final String ATTRIBUTE_NAME = "name";
+    private static final String ATTRIBUTE_DESCRIPTION = "description";
+    private static final String ATTRIBUTE_AFFILIATION = "affiliation";
+    private static final String ATTRIBUTE_FILE_PIDS_ENABLED = "filePIDsEnabled";
+
+    private final Dataverse dataverse;
+    private final String attributeName;
+    private final Object attributeValue;
+
+    public UpdateDataverseAttributeCommand(DataverseRequest request, Dataverse dataverse, String attributeName, Object attributeValue) {
+        super(request, dataverse);
+        this.dataverse = dataverse;
+        this.attributeName = attributeName;
+        this.attributeValue = attributeValue;
+    }
+
+    @Override
+    public Dataverse execute(CommandContext ctxt) throws CommandException {
+        switch (attributeName) {
+            case ATTRIBUTE_ALIAS:
+            case ATTRIBUTE_NAME:
+            case ATTRIBUTE_DESCRIPTION:
+            case ATTRIBUTE_AFFILIATION:
+                setStringAttribute(attributeName, attributeValue);
+                break;
+            case ATTRIBUTE_FILE_PIDS_ENABLED:
+                setBooleanAttributeForFilePIDs(ctxt);
+                break;
+            default:
+                throw new IllegalCommandException("'" + attributeName + "' is not a supported attribute", this);
+        }
+
+        return ctxt.engine().submit(new UpdateDataverseCommand(dataverse, null, null, getRequest(), null));
+    }
+
+    /**
+     * Helper method to set a string attribute.
+     *
+     * @param attributeName  The name of the attribute.
+     * @param attributeValue The value of the attribute (must be a String).
+     * @throws IllegalCommandException if the provided attribute value is not of String type.
+     */
+    private void setStringAttribute(String attributeName, Object attributeValue) throws IllegalCommandException {
+        if (!(attributeValue instanceof String stringValue)) {
+            throw new IllegalCommandException("'" + attributeName + "' requires a string value", this);
+        }
+
+        switch (attributeName) {
+            case ATTRIBUTE_ALIAS:
+                dataverse.setAlias(stringValue);
+                break;
+            case ATTRIBUTE_NAME:
+                dataverse.setName(stringValue);
+                break;
+            case ATTRIBUTE_DESCRIPTION:
+                dataverse.setDescription(stringValue);
+                break;
+            case ATTRIBUTE_AFFILIATION:
+                dataverse.setAffiliation(stringValue);
+                break;
+            default:
+                throw new IllegalCommandException("Unsupported string attribute: " + attributeName, this);
+        }
+    }
+
+    /**
+     * Helper method to handle the "filePIDsEnabled" boolean attribute.
+     *
+     * @param ctxt The command context.
+     * @throws PermissionException if the user doesn't have permission to modify this attribute.
+     */
+    private void setBooleanAttributeForFilePIDs(CommandContext ctxt) throws CommandException {
+        if (!getRequest().getUser().isSuperuser()) {
+            throw new PermissionException("You must be a superuser to change this setting",
+                    this, Collections.singleton(Permission.EditDataset), dataverse);
+        }
+        if (!ctxt.settings().isTrueForKey(SettingsServiceBean.Key.AllowEnablingFilePIDsPerCollection, false)) {
+            throw new PermissionException("Changing File PID policy per collection is not enabled on this server",
+                    this, Collections.singleton(Permission.EditDataset), dataverse);
+        }
+
+        if (!(attributeValue instanceof Boolean)) {
+            throw new IllegalCommandException("'" + ATTRIBUTE_FILE_PIDS_ENABLED + "' requires a boolean value", this);
+        }
+
+        dataverse.setFilePIDsEnabled((Boolean) attributeValue);
+    }
+}

src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java

@@ -819,10 +819,9 @@ public void testImport() throws IOException, InterruptedException {
         Response deleteUserResponse = UtilIT.deleteUser(username);
         assertEquals(200, deleteUserResponse.getStatusCode());
     }
-
-    @Test
-    public void testAttributesApi() throws Exception {
 
+    @Test
+    public void testAttributesApi() {
         Response createUser = UtilIT.createRandomUser();
         String apiToken = UtilIT.getApiTokenFromResponse(createUser);
 
@@ -837,30 +836,70 @@ public void testAttributesApi() throws Exception {
 
         String collectionAlias = UtilIT.getAliasFromResponse(createDataverseResponse);
         String newCollectionAlias = collectionAlias + "RENAMED";
-
-        // Change the alias of the collection: 
-
-        Response changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "alias", newCollectionAlias, apiToken);
-        changeAttributeResp.prettyPrint();
-
+
+        // Change the name of the collection:
+
+        String newCollectionName = "Renamed Name";
+        Response changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "name", newCollectionName, apiToken);
         changeAttributeResp.then().assertThat()
                 .statusCode(OK.getStatusCode())
                 .body("message.message", equalTo("Update successful"));
-
-        // Check on the collection, under the new alias: 
-
+
+        // Change the description of the collection:
+
+        String newDescription = "Renamed Description";
+        changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "description", newDescription, apiToken);
+        changeAttributeResp.then().assertThat()
+                .statusCode(OK.getStatusCode())
+                .body("message.message", equalTo("Update successful"));
+
+        // Change the affiliation of the collection:
+
+        String newAffiliation = "Renamed Affiliation";
+        changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "affiliation", newAffiliation, apiToken);
+        changeAttributeResp.then().assertThat()
+                .statusCode(OK.getStatusCode())
+                .body("message.message", equalTo("Update successful"));
+
+        // Cannot update filePIDsEnabled from a regular user:
+
+        changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "filePIDsEnabled", "true", apiToken);
+        changeAttributeResp.then().assertThat()
+                .statusCode(UNAUTHORIZED.getStatusCode());
+
+        // Change the alias of the collection:
+
+        changeAttributeResp = UtilIT.setCollectionAttribute(collectionAlias, "alias", newCollectionAlias, apiToken);
+        changeAttributeResp.then().assertThat()
+                .statusCode(OK.getStatusCode())
+                .body("message.message", equalTo("Update successful"));
+
+        // Check on the collection, under the new alias:
+
         Response collectionInfoResponse = UtilIT.exportDataverse(newCollectionAlias, apiToken);
-        collectionInfoResponse.prettyPrint();
-
         collectionInfoResponse.then().assertThat()
                 .statusCode(OK.getStatusCode())
-                .body("data.alias", equalTo(newCollectionAlias));
-
+                .body("data.alias", equalTo(newCollectionAlias))
+                .body("data.name", equalTo(newCollectionName))
+                .body("data.description", equalTo(newDescription))
+                .body("data.affiliation", equalTo(newAffiliation));
+
         // Delete the collection (again, using its new alias):
-        
+
         Response deleteCollectionResponse = UtilIT.deleteDataverse(newCollectionAlias, apiToken);
-        deleteCollectionResponse.prettyPrint();
         assertEquals(OK.getStatusCode(), deleteCollectionResponse.getStatusCode());
+
+        // Cannot update root collection from a regular user:
+
+        changeAttributeResp = UtilIT.setCollectionAttribute("root", "name", newCollectionName, apiToken);
+        changeAttributeResp.then().assertThat()
+                .statusCode(UNAUTHORIZED.getStatusCode());
+
+        collectionInfoResponse = UtilIT.exportDataverse("root", apiToken);
+
+        collectionInfoResponse.then().assertThat()
+                .statusCode(OK.getStatusCode())
+                .body("data.name", equalTo("Root"));
     }
 
     @Test