diff --git a/dspace-api/src/main/java/org/dspace/content/BundleServiceImpl.java b/dspace-api/src/main/java/org/dspace/content/BundleServiceImpl.java index 7ef6d0b9f1..ca6d034e1b 100644 --- a/dspace-api/src/main/java/org/dspace/content/BundleServiceImpl.java +++ b/dspace-api/src/main/java/org/dspace/content/BundleServiceImpl.java @@ -269,29 +269,60 @@ public class BundleServiceImpl extends DSpaceObjectServiceImpl implement public void setOrder(Context context, Bundle bundle, UUID[] bitstreamIds) throws AuthorizeException, SQLException { authorizeService.authorizeAction(context, bundle, Constants.WRITE); - bundle.getBitstreams().clear(); + List currentBitstreams = bundle.getBitstreams(); + List updatedBitstreams = new ArrayList(); + + // Loop through and ensure these Bitstream IDs are all valid. Add them to list of updatedBitstreams. for (int i = 0; i < bitstreamIds.length; i++) { UUID bitstreamId = bitstreamIds[i]; Bitstream bitstream = bitstreamService.find(context, bitstreamId); - if(bitstream == null){ + + // If we have an invalid Bitstream ID, just ignore it, but log a warning + if(bitstream == null) { //This should never occur but just in case log.warn(LogManager.getHeader(context, "Invalid bitstream id while changing bitstream order", "Bundle: " + bundle.getID() + ", bitstream id: " + bitstreamId)); continue; } - bitstream.getBundles().remove(bundle); - bundle.getBitstreams().add(bitstream); - bitstream.getBundles().add(bundle); - bitstreamService.update(context, bitstream); + // If we have a Bitstream not in the current list, log a warning & exit immediately + if(!currentBitstreams.contains(bitstream)) + { + log.warn(LogManager.getHeader(context, "Encountered a bitstream not in this bundle while changing bitstream order. Bitstream order will not be changed.", "Bundle: " + bundle.getID() + ", bitstream id: " + bitstreamId)); + return; + } + updatedBitstreams.add(bitstream); } - //The order of the bitstreams has changed, ensure that we update the last modified of our item - Item owningItem = (Item) getParentObject(context, bundle); - if(owningItem != null) + // If our lists are different sizes, exit immediately + if(updatedBitstreams.size()!=currentBitstreams.size()) { - itemService.updateLastModified(context, owningItem); - itemService.update(context, owningItem); + log.warn(LogManager.getHeader(context, "Size of old list and new list do not match. Bitstream order will not be changed.", "Bundle: " + bundle.getID())); + return; + } + // As long as the order has changed, update it + if(CollectionUtils.isNotEmpty(updatedBitstreams) && !updatedBitstreams.equals(currentBitstreams)) + { + //First clear out the existing list of bitstreams + bundle.getBitstreams().clear(); + + // Now add them back in the proper order + for (Bitstream bitstream : updatedBitstreams) + { + bitstream.getBundles().remove(bundle); + bundle.getBitstreams().add(bitstream); + bitstream.getBundles().add(bundle); + bitstreamService.update(context, bitstream); + } + + //The order of the bitstreams has changed, ensure that we update the last modified of our item + Item owningItem = (Item) getParentObject(context, bundle); + if(owningItem != null) + { + itemService.updateLastModified(context, owningItem); + itemService.update(context, owningItem); + + } } } diff --git a/dspace-api/src/test/java/org/dspace/content/BundleTest.java b/dspace-api/src/test/java/org/dspace/content/BundleTest.java index f085a27114..aebff76661 100644 --- a/dspace-api/src/test/java/org/dspace/content/BundleTest.java +++ b/dspace-api/src/test/java/org/dspace/content/BundleTest.java @@ -719,26 +719,42 @@ public class BundleTest extends AbstractDSpaceObjectTest bundleService.addBitstream(context, b, bs3); // Assert Bitstreams are in the order added - List bitstreams = b.getBitstreams(); - assertTrue("testSetOrder: starting count correct", bitstreams.size() == 3); - assertThat("testSetOrder: Bitstream 1 is first", bitstreams.get(0).getName(), equalTo(bs.getName())); - assertThat("testSetOrder: Bitstream 2 is second", bitstreams.get(1).getName(), equalTo(bs2.getName())); - assertThat("testSetOrder: Bitstream 3 is third", bitstreams.get(2).getName(), equalTo(bs3.getName())); + Bitstream[] bitstreams = b.getBitstreams().toArray(new Bitstream[b.getBitstreams().size()]); + assertTrue("testSetOrder: starting count correct", bitstreams.length == 3); + assertThat("testSetOrder: Bitstream 1 is first", bitstreams[0].getName(), equalTo(bs.getName())); + assertThat("testSetOrder: Bitstream 2 is second", bitstreams[1].getName(), equalTo(bs2.getName())); + assertThat("testSetOrder: Bitstream 3 is third", bitstreams[2].getName(), equalTo(bs3.getName())); UUID bsID1 = bs.getID(); UUID bsID2 = bs2.getID(); UUID bsID3 = bs3.getID(); // Now define a new order and call setOrder() - UUID[] newBitstreamOrder = { bsID3, bsID1, bsID2 }; + UUID[] newBitstreamOrder = new UUID[] { bsID3, bsID1, bsID2 }; bundleService.setOrder(context, b, newBitstreamOrder); // Assert Bitstreams are in the new order - bitstreams = b.getBitstreams(); - assertTrue("testSetOrder: new count correct", bitstreams.size() == 3); - assertThat("testSetOrder: Bitstream 3 is now first", bitstreams.get(0).getName(), equalTo(bs3.getName())); - assertThat("testSetOrder: Bitstream 1 is now second", bitstreams.get(1).getName(), equalTo(bs.getName())); - assertThat("testSetOrder: Bitstream 2 is now third", bitstreams.get(2).getName(), equalTo(bs2.getName())); + bitstreams = b.getBitstreams().toArray(new Bitstream[b.getBitstreams().size()]); + assertTrue("testSetOrder: new count correct", bitstreams.length == 3); + assertThat("testSetOrder: Bitstream 3 is now first", bitstreams[0].getName(), equalTo(bs3.getName())); + assertThat("testSetOrder: Bitstream 1 is now second", bitstreams[1].getName(), equalTo(bs.getName())); + assertThat("testSetOrder: Bitstream 2 is now third", bitstreams[2].getName(), equalTo(bs2.getName())); + + // Now give only a partial list of bitstreams + newBitstreamOrder = new UUID[] { bsID1, bsID2 }; + bundleService.setOrder(context, b, newBitstreamOrder); + + // Assert Bitstream order is unchanged + Bitstream[] bitstreamsAfterPartialData = b.getBitstreams().toArray(new Bitstream[b.getBitstreams().size()]); + assertThat("testSetOrder: Partial data doesn't change order", bitstreamsAfterPartialData, equalTo(bitstreams)); + + // Now give bad data in the list of bitstreams + newBitstreamOrder = new UUID[] { bsID1, null, bsID2 }; + bundleService.setOrder(context, b, newBitstreamOrder); + + // Assert Bitstream order is unchanged + Bitstream[] bitstreamsAfterBadData = b.getBitstreams().toArray(new Bitstream[b.getBitstreams().size()]); + assertThat("testSetOrder: Partial data doesn't change order", bitstreamsAfterBadData, equalTo(bitstreams)); } /**