Skip to content

Commit

Permalink
Merge pull request DSpace#9509 from tdonohue/empty_metadata_bug
Browse files Browse the repository at this point in the history 
Fix bug where empty metadata List can result in "Index 0 out of bounds for length 0" exceptions in several scenarios
  • Loading branch information
@tdonohue
tdonohue authored Jun 14, 2024
2 parents 099539e + 2eb7dbc commit 2af0509
Show file tree
Hide file tree
Showing 4 changed files with 164 additions and 31 deletions.
10 changes: 6 additions & 4 deletions dspace-api/src/main/java/org/dspace/app/bulkedit/MetadataImport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,10 @@ protected void compareAndUpdate(Context c, Item item, String[] fromCSV, boolean
addRelationships(c, item, element, values);
} else {
itemService.clearMetadata(c, item, schema, element, qualifier, language);
itemService.addMetadata(c, item, schema, element, qualifier,
language, values, authorities, confidences);
if (!values.isEmpty()) {
itemService.addMetadata(c, item, schema, element, qualifier,
language, values, authorities, confidences);
}
itemService.update(c, item);
}
}
Expand Down Expand Up @@ -1121,8 +1123,8 @@ protected void add(Context c, String[] fromCSV, String md, BulkEditChange change
.getAuthoritySeparator() + dcv.getConfidence();
}

// Add it
if ((value != null) && (!"".equals(value))) {
// Add it, if value is not blank
if (value != null && StringUtils.isNotBlank(value)) {
changes.registerAdd(dcv);
}
}
Expand Down
35 changes: 31 additions & 4 deletions dspace-api/src/main/java/org/dspace/content/DSpaceObjectServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,10 +242,31 @@ public List<MetadataValue> addMetadata(Context context, T dso, MetadataField met

}

/**
* 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<>();
Expand Down Expand Up @@ -314,20 +335,26 @@ public List<MetadataValue> addMetadata(Context context, T dso, MetadataField met
@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),
Arrays.asList(confidence)).get(0);
List<MetadataValue> metadataValues =
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
Expand Down
27 changes: 15 additions & 12 deletions dspace-api/src/test/java/org/dspace/app/bulkedit/MetadataImportIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@

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;
Expand Down Expand Up @@ -218,9 +218,10 @@ public void personMetadataImportTest() throws Exception {

@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();

Expand All @@ -232,19 +233,21 @@ public void metadataImportRemovingValueTest() throws Exception {
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 {
Item importedItem = null;
List<Item> allItems = IteratorUtils.toList(itemService.findAll(context));
for (Item item : allItems) {
if (item.getName().equals(name)) {
importedItem = item;
}
private Item findItemByName(String name) throws Exception {
List<Item> items =
IteratorUtils.toList(itemService.findByMetadataField(context, "dc", "title", null, name));

if (items != null && !items.isEmpty()) {
// 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 {
Expand Down
123 changes: 112 additions & 11 deletions dspace-api/src/test/java/org/dspace/content/ItemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
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;
Expand Down Expand Up @@ -537,6 +539,17 @@ public void testAddMetadata_5args_1() throws SQLException {
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.
*/
Expand Down Expand Up @@ -614,33 +627,85 @@ public void testAddMetadata_7args_1_noauthority() throws SQLException {
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 {
String schema = "dc";
String element = "contributor";
String qualifier = "author";
String lang = Item.ANY;
List<String> values = Arrays.asList("value0", "value1");
itemService.addMetadata(context, it, schema, element, qualifier, lang, values);
String value = "value0";
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 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)));
assertThat("testAddMetadata_5args_2 6", dc.get(0).getValue(), equalTo(value));
}

/**
Expand Down Expand Up @@ -702,6 +767,42 @@ public void testAddMetadata_7args_2_noauthority() throws SQLException {
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.
*/
Expand Down

0 comments on commit 2af0509

Please sign in to comment.