Merge pull request #9509 from tdonohue/empty_metadata_bug

Fix bug where empty metadata List can result in "Index 0 out of bounds for length 0" exceptions in several scenarios

dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java

@@ -825,8 +825,10 @@ public class MetadataImport extends DSpaceRunnable<MetadataImportScriptConfigura
                 addRelationships(c, item, element, values);
             } else {
                 itemService.clearMetadata(c, item, schema, element, qualifier, language);
+                if (!values.isEmpty()) {
                     itemService.addMetadata(c, item, schema, element, qualifier,
                                             language, values, authorities, confidences);
+                }
                 itemService.update(c, item);
             }
         }
@@ -1121,8 +1123,8 @@ public class MetadataImport extends DSpaceRunnable<MetadataImportScriptConfigura
                     .getAuthoritySeparator() + dcv.getConfidence();
             }
 
-            // Add it
+            // Add it, if value is not blank
-            if ((value != null) && (!"".equals(value))) {
+            if (value != null && StringUtils.isNotBlank(value)) {
                 changes.registerAdd(dcv);
             }
         }

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

@@ -242,10 +242,31 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
 
     }
 
+    /**
+     * Add metadata value(s) to a MetadataField of a DSpace Object
+     * @param context current DSpace context
+     * @param dso DSpaceObject to modify
+     * @param metadataField MetadataField to add values to
+     * @param lang Language code to add
+     * @param values One or more metadata values to add
+     * @param authorities One or more authorities to add
+     * @param confidences One or more confidences to add (for authorities)
+     * @param placeSupplier Supplier of "place" for new metadata values
+     * @return List of newly added metadata values
+     * @throws SQLException if database error occurs
+     * @throws IllegalArgumentException for an empty list of values
+     */
     public List<MetadataValue> addMetadata(Context context, T dso, MetadataField metadataField, String lang,
             List<String> values, List<String> authorities, List<Integer> confidences, Supplier<Integer> placeSupplier)
                     throws SQLException {
 
+        // Throw an error if we are attempting to add empty values
+        if (values == null || values.isEmpty()) {
+            throw new IllegalArgumentException("Cannot add empty values to a new metadata field " +
+                                                   metadataField.toString() + " on object with uuid = " +
+                                                   dso.getID().toString() + " and type = " + getTypeText(dso));
+        }
+
         boolean authorityControlled = metadataAuthorityService.isAuthorityControlled(metadataField);
         boolean authorityRequired = metadataAuthorityService.isAuthorityRequired(metadataField);
         List<MetadataValue> newMetadata = new ArrayList<>();
@@ -314,20 +335,26 @@ public abstract class DSpaceObjectServiceImpl<T extends DSpaceObject> implements
     @Override
     public MetadataValue addMetadata(Context context, T dso, MetadataField metadataField, String language,
                             String value, String authority, int confidence) throws SQLException {
-        return addMetadata(context, dso, metadataField, language, Arrays.asList(value), Arrays.asList(authority),
+        List<MetadataValue> metadataValues =
-                    Arrays.asList(confidence)).get(0);
+            addMetadata(context, dso, metadataField, language, Arrays.asList(value), Arrays.asList(authority),
+                        Arrays.asList(confidence));
+        return CollectionUtils.isNotEmpty(metadataValues) ? metadataValues.get(0) : null;
     }
 
     @Override
     public MetadataValue addMetadata(Context context, T dso, String schema, String element, String qualifier,
                              String lang, String value) throws SQLException {
-        return addMetadata(context, dso, schema, element, qualifier, lang, Arrays.asList(value)).get(0);
+        List<MetadataValue> metadataValues =
+            addMetadata(context, dso, schema, element, qualifier, lang, Arrays.asList(value));
+        return CollectionUtils.isNotEmpty(metadataValues) ? metadataValues.get(0) : null;
     }
 
     @Override
     public MetadataValue addMetadata(Context context, T dso, MetadataField metadataField, String language, String value)
         throws SQLException {
-        return addMetadata(context, dso, metadataField, language, Arrays.asList(value)).get(0);
+        List<MetadataValue> metadataValues =
+            addMetadata(context, dso, metadataField, language, Arrays.asList(value));
+        return CollectionUtils.isNotEmpty(metadataValues) ? metadataValues.get(0) : null;
     }
 
     @Override

dspace-api/src/test/java/org/dspace/app/bulkedit/MetadataImportIT.java

@@ -9,12 +9,12 @@ package org.dspace.app.bulkedit;
 
 import static junit.framework.TestCase.assertEquals;
 import static junit.framework.TestCase.assertTrue;
+import static junit.framework.TestCase.fail;
 
 import java.io.BufferedWriter;
 import java.io.File;
 import java.io.FileOutputStream;
 import java.io.OutputStreamWriter;
-import java.sql.SQLException;
 import java.util.List;
 
 import org.apache.commons.cli.ParseException;
@@ -218,9 +218,10 @@ public class MetadataImportIT extends AbstractIntegrationTestWithDatabase {
 
     @Test
     public void metadataImportRemovingValueTest() throws Exception {
-
         context.turnOffAuthorisationSystem();
-        Item item = ItemBuilder.createItem(context,personCollection).withAuthor("TestAuthorToRemove").withTitle("title")
+        String itemTitle = "Testing removing author";
+        Item item = ItemBuilder.createItem(context,personCollection).withAuthor("TestAuthorToRemove")
+                               .withTitle(itemTitle)
                                .build();
         context.restoreAuthSystemState();
 
@@ -232,20 +233,22 @@ public class MetadataImportIT extends AbstractIntegrationTestWithDatabase {
         String[] csv = {"id,collection,dc.title,dc.contributor.author[*]",
             item.getID().toString() + "," + personCollection.getHandle() + "," + item.getName() + ","};
         performImportScript(csv);
-        item = findItemByName("title");
+        item = findItemByName(itemTitle);
         assertEquals(0, itemService.getMetadata(item, "dc", "contributor", "author", Item.ANY).size());
     }
 
-    private Item findItemByName(String name) throws SQLException {
+    private Item findItemByName(String name) throws Exception {
-        Item importedItem = null;
+        List<Item> items =
-        List<Item> allItems = IteratorUtils.toList(itemService.findAll(context));
+            IteratorUtils.toList(itemService.findByMetadataField(context, "dc", "title", null, name));
-        for (Item item : allItems) {
+
-            if (item.getName().equals(name)) {
+        if (items != null && !items.isEmpty()) {
-                importedItem = item;
+            // Just return first matching Item. Tests should ensure name/title is unique.
+            return items.get(0);
+        } else {
+            fail("Could not find expected Item with dc.title = '" + name + "'");
+            return null;
         }
     }
-        return importedItem;
-    }
 
     public void performImportScript(String[] csv) throws Exception {
         performImportScript(csv, false);

dspace-api/src/test/java/org/dspace/content/ItemTest.java

@@ -13,6 +13,8 @@ import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
@@ -537,6 +539,17 @@ public class ItemTest extends AbstractDSpaceObjectTest {
         assertThat("testAddMetadata_5args_1 11", dc.get(1).getValue(), equalTo(values[1]));
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void testAddMetadata_5args_no_values() throws Exception {
+        String schema = "dc";
+        String element = "contributor";
+        String qualifier = "author";
+        String lang = Item.ANY;
+        String[] values = {};
+        itemService.addMetadata(context, it, schema, element, qualifier, lang, Arrays.asList(values));
+        fail("IllegalArgumentException expected");
+    }
+
     /**
      * Test of addMetadata method, of class Item.
      */
@@ -614,8 +627,66 @@ public class ItemTest extends AbstractDSpaceObjectTest {
         assertThat("testAddMetadata_7args_1 15", dc.get(1).getConfidence(), equalTo(-1));
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void testAddMetadata_7args_no_values() throws Exception {
+        String schema = "dc";
+        String element = "contributor";
+        String qualifier = "author";
+        String lang = Item.ANY;
+        List<String> values = new ArrayList();
+        List<String> authorities = new ArrayList();
+        List<Integer> confidences = new ArrayList();
+        itemService.addMetadata(context, it, schema, element, qualifier, lang, values, authorities, confidences);
+        fail("IllegalArgumentException expected");
+    }
+
+    @Test
+    public void testAddMetadata_list_with_virtual_metadata() throws Exception {
+        String schema = "dc";
+        String element = "contributor";
+        String qualifier = "author";
+        String lang = Item.ANY;
+        // Create two fake virtual metadata ("virtual::[relationship-id]") values
+        List<String> values = new ArrayList<>(Arrays.asList("uuid-1", "uuid-2"));
+        List<String> authorities = new ArrayList<>(Arrays.asList(Constants.VIRTUAL_AUTHORITY_PREFIX + "relationship-1",
+                                                 Constants.VIRTUAL_AUTHORITY_PREFIX + "relationship-2"));
+        List<Integer> confidences = new ArrayList<>(Arrays.asList(-1, -1));
+
+        // Virtual metadata values will be IGNORED. No metadata should be added as we are calling addMetadata()
+        // with two virtual metadata values.
+        List<MetadataValue> valuesAdded = itemService.addMetadata(context, it, schema, element, qualifier, lang,
+                                                                  values, authorities, confidences);
+        assertNotNull(valuesAdded);
+        assertTrue(valuesAdded.isEmpty());
+
+        // Now, update tests values to append a third value which is NOT virtual metadata
+        String newValue = "new-metadata-value";
+        String newAuthority = "auth0";
+        Integer newConfidence = 0;
+        values.add(newValue);
+        authorities.add(newAuthority);
+        confidences.add(newConfidence);
+
+        // Call addMetadata again, and this time only one value (the new, non-virtual metadata) should be added
+        valuesAdded = itemService.addMetadata(context, it, schema, element, qualifier, lang,
+                                              values, authorities, confidences);
+        assertNotNull(valuesAdded);
+        assertEquals(1, valuesAdded.size());
+
+        // Get metadata and ensure new value is the ONLY ONE for this metadata field
+        List<MetadataValue> dc = itemService.getMetadata(it, schema, element, qualifier, lang);
+        assertNotNull(dc);
+        assertEquals(1, dc.size());
+        assertEquals(schema, dc.get(0).getMetadataField().getMetadataSchema().getName());
+        assertEquals(element, dc.get(0).getMetadataField().getElement());
+        assertEquals(qualifier, dc.get(0).getMetadataField().getQualifier());
+        assertEquals(newValue, dc.get(0).getValue());
+        assertNull(dc.get(0).getAuthority());
+        assertEquals(-1, dc.get(0).getConfidence());
+    }
+
     /**
-     * Test of addMetadata method, of class Item.
+     * This is the same as testAddMetadata_5args_1 except it is adding a *single* value as a String, not a List.
      */
     @Test
     public void testAddMetadata_5args_2() throws SQLException {
@@ -623,24 +694,18 @@ public class ItemTest extends AbstractDSpaceObjectTest {
         String element = "contributor";
         String qualifier = "author";
         String lang = Item.ANY;
-        List<String> values = Arrays.asList("value0", "value1");
+        String value = "value0";
-        itemService.addMetadata(context, it, schema, element, qualifier, lang, values);
+        itemService.addMetadata(context, it, schema, element, qualifier, lang, value);
 
         List<MetadataValue> dc = itemService.getMetadata(it, schema, element, qualifier, lang);
         assertThat("testAddMetadata_5args_2 0", dc, notNullValue());
-        assertTrue("testAddMetadata_5args_2 1", dc.size() == 2);
+        assertTrue("testAddMetadata_5args_2 1", dc.size() == 1);
         assertThat("testAddMetadata_5args_2 2", dc.get(0).getMetadataField().getMetadataSchema().getName(),
                    equalTo(schema));
         assertThat("testAddMetadata_5args_2 3", dc.get(0).getMetadataField().getElement(), equalTo(element));
         assertThat("testAddMetadata_5args_2 4", dc.get(0).getMetadataField().getQualifier(), equalTo(qualifier));
         assertThat("testAddMetadata_5args_2 5", dc.get(0).getLanguage(), equalTo(lang));
-        assertThat("testAddMetadata_5args_2 6", dc.get(0).getValue(), equalTo(values.get(0)));
+        assertThat("testAddMetadata_5args_2 6", dc.get(0).getValue(), equalTo(value));
-        assertThat("testAddMetadata_5args_2 7", dc.get(1).getMetadataField().getMetadataSchema().getName(),
-                   equalTo(schema));
-        assertThat("testAddMetadata_5args_2 8", dc.get(1).getMetadataField().getElement(), equalTo(element));
-        assertThat("testAddMetadata_5args_2 9", dc.get(1).getMetadataField().getQualifier(), equalTo(qualifier));
-        assertThat("testAddMetadata_5args_2 10", dc.get(1).getLanguage(), equalTo(lang));
-        assertThat("testAddMetadata_5args_2 11", dc.get(1).getValue(), equalTo(values.get(1)));
     }
 
     /**
@@ -702,6 +767,42 @@ public class ItemTest extends AbstractDSpaceObjectTest {
         assertThat("testAddMetadata_7args_2 8", dc.get(0).getConfidence(), equalTo(-1));
     }
 
+    @Test
+    public void testAddMetadata_single_virtual_metadata() throws Exception {
+        String schema = "dc";
+        String element = "contributor";
+        String qualifier = "author";
+        String lang = Item.ANY;
+        // Create a single fake virtual metadata ("virtual::[relationship-id]") value
+        String value = "uuid-1";
+        String authority = Constants.VIRTUAL_AUTHORITY_PREFIX + "relationship-1";
+        Integer confidence = -1;
+
+        // Virtual metadata values will be IGNORED. No metadata should be added as we are calling addMetadata()
+        // with a virtual metadata value.
+        MetadataValue valuesAdded = itemService.addMetadata(context, it, schema, element, qualifier, lang,
+                                                            value, authority, confidence);
+        // Returned object will be null when no metadata was added
+        assertNull(valuesAdded);
+
+        // Verify this metadata field does NOT exist on the item
+        List<MetadataValue> mv = itemService.getMetadata(it, schema, element, qualifier, lang);
+        assertNotNull(mv);
+        assertTrue(mv.isEmpty());
+
+        // Also try calling addMetadata() with MetadataField object
+        MetadataField metadataField = metadataFieldService.findByElement(context, schema, element, qualifier);
+        valuesAdded = itemService.addMetadata(context, it, metadataField, lang, value, authority, confidence);
+        // Returned object should still be null
+        assertNull(valuesAdded);
+
+        // Verify this metadata field does NOT exist on the item
+        mv = itemService.getMetadata(it, schema, element, qualifier, lang);
+        assertNotNull(mv);
+        assertTrue(mv.isEmpty());
+    }
+
+
     /**
      * Test of clearMetadata method, of class Item.
      */