Merge pull request #8165 from atmire/w2p-87384_Fix-null-metadata-values-being-added

Fix Null metadata values being added

dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java

@@ -243,67 +243,64 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
 
         boolean authorityControlled = metadataAuthorityService.isAuthorityControlled(metadataField);
         boolean authorityRequired = metadataAuthorityService.isAuthorityRequired(metadataField);
-        List<MetadataValue> newMetadata = new ArrayList<>(values.size());
+        List<MetadataValue> 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("\u0009") &&
-                        !String.valueOf(dcvalue[charPos]).equals("\n") &&
+                            !String.valueOf(dcvalue[charPos]).equals("\n") &&
-                        !String.valueOf(dcvalue[charPos]).equals("\r")) {
+                            !String.valueOf(dcvalue[charPos]).equals("\r")) {
                         dcvalue[charPos] = ' ';
                     }
                 }
                 metadataValue.setValue(String.valueOf(dcvalue));
-            } else {
+                //An update here isn't needed, this is persited upon the merge of the owning object
-                metadataValue.setValue(null);
-            }
-            //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;

dspace-server-webapp/src/test/java/org/dspace/app/rest/WorkspaceItemRestRepositoryIT.java

@@ -43,6 +43,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.time.DateUtils;
 import org.dspace.app.rest.matcher.CollectionMatcher;
@@ -2515,6 +2516,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<Operation> operations = new ArrayList<Operation>();
+        // create a list of values to use in add operation
+        List<Map<String, String>> titelValues = new ArrayList<Map<String, String>>();
+        List<Map<String, String>> uriValues = new ArrayList<Map<String, String>>();
+        Map<String, String> value = new HashMap<String, String>();
+        Map<String, String> value2 = new HashMap<String, String>();
+        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();