Fix bug where empty metadata List can cause AIP import/restore to fail

[tdonohue]

Description

While testing 8.0, I ran into a scenario where an AIP import threw this error:

Replacing DSpace object(s) with package located at AIPs/COLLECTION@123456789-1145.zip
java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
        at java.base/java.util.Objects.checkIndex(Objects.java:361)
        at java.base/java.util.ArrayList.get(ArrayList.java:427)
        at org.dspace.content.DSpaceObjectServiceImpl.addMetadata(DSpaceObjectServiceImpl.java:318)
        at org.dspace.content.crosswalk.XSLTIngestionCrosswalk.applyDimField(XSLTIngestionCrosswalk.java:115)
        at org.dspace.content.crosswalk.XSLTIngestionCrosswalk.applyDim(XSLTIngestionCrosswalk.java:81)
        at org.dspace.content.crosswalk.XSLTIngestionCrosswalk.applyDim(XSLTIngestionCrosswalk.java:84)
        at org.dspace.content.crosswalk.XSLTIngestionCrosswalk.ingestDIM(XSLTIngestionCrosswalk.java:231)
        at org.dspace.content.crosswalk.AIPDIMCrosswalk.ingest(AIPDIMCrosswalk.java:185)
        at org.dspace.content.packager.METSManifest.crosswalkXmd(METSManifest.java:1128)
        at org.dspace.content.packager.METSManifest.crosswalkItemDmd(METSManifest.java:1018)
        at org.dspace.content.packager.DSpaceAIPIngester.crosswalkObjectDmd(DSpaceAIPIngester.java:155)
        at org.dspace.content.packager.AbstractMETSIngester.ingestObject(AbstractMETSIngester.java:451)
        at org.dspace.content.packager.AbstractMETSIngester.replace(AbstractMETSIngester.java:1075)
        at org.dspace.content.packager.AbstractPackageIngester.replaceAll(AbstractPackageIngester.java:275)
        at org.dspace.content.packager.AbstractPackageIngester.replaceAll(AbstractPackageIngester.java:303)
        at org.dspace.app.packager.Packager.replace(Packager.java:684)

The code which is referenced is obviously buggy. If the List in question is ever empty (because code called it with an empty metadata field or some other error occurs), then it fails entirely.

This PR is a small fix to avoid the entire import failing. All it does is ensure the List is not empty before returning first value

Instructions for Reviewers

• The scenario here may be difficult to reproduce. However, the code should be very self explanatory, and it matches the code from the getMetadataFirstValue() methods in the same DSpaceObjectServiceImpl
• With this code in place, I was able to complete a successful AIP restoration (with no metadata loss). Without it, I could not restore from AIPs.

[abollini]

Hi @tdonohue,
IMHO the issue is caused by this hack org.dspace.content.DSpaceObjectServiceImpl.addMetadata(Context, T, MetadataField, String, List<String>, List<String>, List<Integer>, Supplier<Integer>) around line 255

 if (authorities != null && authorities.size() >= i) {
                    if (StringUtils.startsWith(authorities.get(i), Constants.VIRTUAL_AUTHORITY_PREFIX)) {
                        continue;
                    }
                }

that was obviously introduced to deal with management of relationship and virtual field.

I argue that it was a hack because it looks odd to me that a method named addMetadata should eventually return an empty list (i.e. don't add anything).

It is clear that modify the addMetadata method was an easier solution compared to rethink the way that virtual metadata are managed in dspace or to check all the places that would potentially pass a virtual metadata to the ItemServiceImpl but, IMHO, this is a clear sign of bad design that we should note as an "architecture technical debt".

That said, I'm ok with the changes that you have provided but I would suggest two improvements:

• we need to add a test that demonstrate the bug and can be used to avoid regression in future refactoring (if my analysis is right an AIP containing virtual metadata should be enough)
• we should check in the addMetadata method that the list received are not empty, otherwise we should throw an IllegalArgumentException. This to avoid that the changes would hide future bugs where the calling code is populating the list wrongly