From 1b1d337d4c82cfa52ac3777fc99d88de1b2d45b8 Mon Sep 17 00:00:00 2001 From: GPortas Date: Wed, 23 Oct 2024 14:18:28 +0100 Subject: [PATCH 1/3] Changed: using new UpdateDataverseAttributeCommand from updateAttribute endpoint --- .../harvard/iq/dataverse/api/Dataverses.java | 62 ++-------- .../impl/UpdateDataverseAttributeCommand.java | 110 ++++++++++++++++++ 2 files changed, 121 insertions(+), 51 deletions(-) create mode 100644 src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseAttributeCommand.java diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java b/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java index 0ee146ed99b..110adb60577 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java +++ b/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(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") diff --git a/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseAttributeCommand.java b/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/UpdateDataverseAttributeCommand.java new file mode 100644 index 00000000000..57ac20fcee6 --- /dev/null +++ b/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 { + + 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); + } +} From 45cab7502a264108802c5462e46b48a27f118459 Mon Sep 17 00:00:00 2001 From: GPortas Date: Wed, 23 Oct 2024 14:38:33 +0100 Subject: [PATCH 2/3] Changed: recovered response message in updateAttribute endpoint --- src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java b/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java index 110adb60577..2be6b1e51c2 100644 --- a/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java +++ b/src/main/java/edu/harvard/iq/dataverse/api/Dataverses.java @@ -624,7 +624,7 @@ public Response updateAttribute(@Context ContainerRequestContext crc, @PathParam Dataverse dataverse = findDataverseOrDie(identifier); Object formattedValue = formatAttributeValue(attribute, value); dataverse = execCommand(new UpdateDataverseAttributeCommand(createDataverseRequest(getRequestUser(crc)), dataverse, attribute, formattedValue)); - return ok(JsonPrinter.json(dataverse)); + return ok("Update successful", JsonPrinter.json(dataverse)); } catch (WrappedResponse e) { return e.getResponse(); } From e98ae05da45d2d4680807c257c6b95bfda81b8b5 Mon Sep 17 00:00:00 2001 From: GPortas Date: Wed, 23 Oct 2024 14:39:00 +0100 Subject: [PATCH 3/3] Added: IT cases for testAttributesApi --- .../iq/dataverse/api/DataversesIT.java | 75 ++++++++++++++----- 1 file changed, 57 insertions(+), 18 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java index 8c6a8244af1..6a040f27786 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java +++ b/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