diff --git a/dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java index c34291c3dd..34b28d31b8 100644 --- a/dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java @@ -257,67 +257,64 @@ public abstract class DSpaceObjectServiceImpl implements boolean authorityControlled = metadataAuthorityService.isAuthorityControlled(metadataField); boolean authorityRequired = metadataAuthorityService.isAuthorityRequired(metadataField); - List newMetadata = new ArrayList<>(values.size()); + List newMetadata = new ArrayList<>(); // We will not verify that they are valid entries in the registry // until update() is called. for (int i = 0; i < values.size(); i++) { - - if (authorities != null && authorities.size() >= i) { - if (StringUtils.startsWith(authorities.get(i), Constants.VIRTUAL_AUTHORITY_PREFIX)) { - continue; - } - } - MetadataValue metadataValue = metadataValueService.create(context, dso, metadataField); - newMetadata.add(metadataValue); - - metadataValue.setPlace(placeSupplier.get()); - - metadataValue.setLanguage(lang == null ? null : lang.trim()); - - // Logic to set Authority and Confidence: - // - normalize an empty string for authority to NULL. - // - if authority key is present, use given confidence or NOVALUE if not given - // - otherwise, preserve confidence if meaningful value was given since it may document a failed - // authority lookup - // - CF_UNSET signifies no authority nor meaningful confidence. - // - it's possible to have empty authority & CF_ACCEPTED if e.g. user deletes authority key - if (authorityControlled) { - if (authorities != null && authorities.get(i) != null && authorities.get(i).length() > 0) { - metadataValue.setAuthority(authorities.get(i)); - metadataValue.setConfidence(confidences == null ? Choices.CF_NOVALUE : confidences.get(i)); - } else { - metadataValue.setAuthority(null); - metadataValue.setConfidence(confidences == null ? Choices.CF_UNSET : confidences.get(i)); - } - // authority sanity check: if authority is required, was it supplied? - // XXX FIXME? can't throw a "real" exception here without changing all the callers to expect it, so - // use a runtime exception - if (authorityRequired && (metadataValue.getAuthority() == null || metadataValue.getAuthority() - .length() == 0)) { - throw new IllegalArgumentException("The metadata field \"" + metadataField - .toString() + "\" requires an authority key but none was provided. Value=\"" + values - .get(i) + "\""); - } - } if (values.get(i) != null) { + if (authorities != null && authorities.size() >= i) { + if (StringUtils.startsWith(authorities.get(i), Constants.VIRTUAL_AUTHORITY_PREFIX)) { + continue; + } + } + MetadataValue metadataValue = metadataValueService.create(context, dso, metadataField); + newMetadata.add(metadataValue); + + metadataValue.setPlace(placeSupplier.get()); + + metadataValue.setLanguage(lang == null ? null : lang.trim()); + + // Logic to set Authority and Confidence: + // - normalize an empty string for authority to NULL. + // - if authority key is present, use given confidence or NOVALUE if not given + // - otherwise, preserve confidence if meaningful value was given since it may document a failed + // authority lookup + // - CF_UNSET signifies no authority nor meaningful confidence. + // - it's possible to have empty authority & CF_ACCEPTED if e.g. user deletes authority key + if (authorityControlled) { + if (authorities != null && authorities.get(i) != null && authorities.get(i).length() > 0) { + metadataValue.setAuthority(authorities.get(i)); + metadataValue.setConfidence(confidences == null ? Choices.CF_NOVALUE : confidences.get(i)); + } else { + metadataValue.setAuthority(null); + metadataValue.setConfidence(confidences == null ? Choices.CF_UNSET : confidences.get(i)); + } + // authority sanity check: if authority is required, was it supplied? + // XXX FIXME? can't throw a "real" exception here without changing all the callers to expect it, so + // use a runtime exception + if (authorityRequired && (metadataValue.getAuthority() == null || metadataValue.getAuthority() + .length() == 0)) { + throw new IllegalArgumentException("The metadata field \"" + metadataField + .toString() + "\" requires an authority key but none was provided. Value=\"" + values + .get(i) + "\""); + } + } // remove control unicode char String temp = values.get(i).trim(); char[] dcvalue = temp.toCharArray(); for (int charPos = 0; charPos < dcvalue.length; charPos++) { if (Character.isISOControl(dcvalue[charPos]) && - !String.valueOf(dcvalue[charPos]).equals("\u0009") && - !String.valueOf(dcvalue[charPos]).equals("\n") && - !String.valueOf(dcvalue[charPos]).equals("\r")) { + !String.valueOf(dcvalue[charPos]).equals("\u0009") && + !String.valueOf(dcvalue[charPos]).equals("\n") && + !String.valueOf(dcvalue[charPos]).equals("\r")) { dcvalue[charPos] = ' '; } } metadataValue.setValue(String.valueOf(dcvalue)); - } else { - metadataValue.setValue(null); - } - //An update here isn't needed, this is persited upon the merge of the owning object + //An update here isn't needed, this is persited upon the merge of the owning object // metadataValueService.update(context, metadataValue); - dso.addDetails(metadataField.toString()); + dso.addDetails(metadataField.toString()); + } } setMetadataModified(dso); return newMetadata; diff --git a/dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkspaceItemRestRepositoryIT.java b/dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkspaceItemRestRepositoryIT.java index ffaf6f2da7..5fa1bbbf64 100644 --- a/dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkspaceItemRestRepositoryIT.java +++ b/dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkspaceItemRestRepositoryIT.java @@ -37,6 +37,7 @@ import java.util.concurrent.atomic.AtomicReference; import javax.ws.rs.core.MediaType; import com.fasterxml.jackson.databind.ObjectMapper; +import com.jayway.jsonpath.matchers.JsonPathMatchers; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.CharEncoding; import org.apache.commons.lang3.time.DateUtils; @@ -2494,6 +2495,76 @@ public class WorkspaceItemRestRepositoryIT extends AbstractControllerIntegration ; } + @Test + /** + * Test the addition of metadata of a null value + * + * @throws Exception + */ + public void patchAddMetadataNullValueTest() throws Exception { + context.turnOffAuthorisationSystem(); + + //** GIVEN ** + //1. A community-collection structure with one parent community with sub-community and two collections. + parentCommunity = CommunityBuilder.createCommunity(context) + .withName("Parent Community") + .build(); + Community child1 = CommunityBuilder.createSubCommunity(context, parentCommunity) + .withName("Sub Community") + .build(); + Collection col1 = CollectionBuilder.createCollection(context, child1).withName("Collection 1").build(); + String authToken = getAuthToken(eperson.getEmail(), password); + + WorkspaceItem witem = WorkspaceItemBuilder.createWorkspaceItem(context, col1) + .withIssueDate("2017-10-17") + .withSubject("ExtraEntry") + .build(); + + //disable file upload mandatory + configurationService.setProperty("webui.submit.upload.required", false); + + context.restoreAuthSystemState(); + + // try to add the title + List operations = new ArrayList(); + // create a list of values to use in add operation + List> titelValues = new ArrayList>(); + List> uriValues = new ArrayList>(); + Map value = new HashMap(); + Map value2 = new HashMap(); + value.put("value", "New Title"); + value2.put("value", null); + titelValues.add(value); + uriValues.add(value2); + operations.add(new AddOperation("/sections/traditionalpageone/dc.title", titelValues)); + operations.add(new AddOperation("/sections/traditionalpageone/dc.identifier.uri", uriValues)); + + String patchBody = getPatchContent(operations); + getClient(authToken).perform(patch("/api/submission/workspaceitems/" + witem.getID()) + .content(patchBody) + .contentType(MediaType.APPLICATION_JSON_PATCH_JSON)) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.errors").doesNotExist()) + .andExpect(jsonPath("$", + // check if the new title if back and the other values untouched + Matchers.is(WorkspaceItemMatcher.matchItemWithTitleAndDateIssuedAndSubject(witem, + "New Title", "2017-10-17", "ExtraEntry")))) + .andExpect(jsonPath("$", JsonPathMatchers + .hasNoJsonPath("$.sections.traditionalpageone['dc.identifier.uri']"))); + + + // verify that the patch changes have been persisted + getClient(authToken).perform(get("/api/submission/workspaceitems/" + witem.getID())) + .andExpect(status().isOk()) + .andExpect(jsonPath("$.errors").doesNotExist()) + .andExpect(jsonPath("$", + Matchers.is(WorkspaceItemMatcher.matchItemWithTitleAndDateIssuedAndSubject(witem, + "New Title", "2017-10-17", "ExtraEntry")))) + .andExpect(jsonPath("$", JsonPathMatchers + .hasNoJsonPath("$.sections.traditionalpageone['dc.identifier.uri']"))) + ; + } + @Test public void patchAddMetadataUpdateExistValueTest() throws Exception { context.turnOffAuthorisationSystem();