BXC-4532 - Fix destroy file operation (#1723)

* Inject missing dependency, prevent indexing error when a tombstone is supposed to be indexed, improve test coverage in SolrUpdateProcessor, address codeclimate warning

* Skip processing order if its an empty string

* Reverse name/logic of method for clarity

model-fcrepo/src/main/java/edu/unc/lib/boxc/model/fcrepo/objects/WorkObjectImpl.java

@@ -194,7 +194,7 @@ public FileObject addDataFile(PID filePid, URI storageUri, String filename,
     @Override
     public List<PID> getMemberOrder() {
         var memberOrder = getResource().getProperty(Cdr.memberOrder);
-        if (memberOrder == null) {
+        if (memberOrder == null || memberOrder.getString().isEmpty()) {
             return Collections.emptyList();
         }
         // Split the value from fedora up by pipes, convert into PID objects, return as a list

operations/src/main/java/edu/unc/lib/boxc/operations/impl/destroy/DestroyObjectsJob.java

@@ -257,6 +257,7 @@ private void sendRemoveOrderRequest(String parentId, String childId) throws IOEx
         var request = new MultiParentOrderRequest();
         request.setParentToOrdered(Map.of(parentId, Collections.singletonList(childId)));
         request.setOperation(OrderOperationType.REMOVE_FROM);
+        request.setAgent(agent);
         memberOrderRequestSender.sendToQueue(request);
     }
 

services-camel-app/src/main/java/edu/unc/lib/boxc/services/camel/destroy/DestroyObjectsProcessor.java

@@ -1,5 +1,6 @@
 package edu.unc.lib.boxc.services.camel.destroy;
 
+import edu.unc.lib.boxc.operations.jms.order.MemberOrderRequestSender;
 import org.apache.camel.Exchange;
 import org.apache.camel.Message;
 import org.apache.camel.Processor;
@@ -50,6 +51,7 @@ public class DestroyObjectsProcessor implements Processor {
     private IndexingMessageSender indexingMessageSender;
     private MessageSender binaryDestroyedMessageSender;
     private PremisLoggerFactory premisLoggerFactory;
+    private MemberOrderRequestSender memberOrderRequestSender;
 
     @Override
     public void process(Exchange exchange) throws Exception {
@@ -89,6 +91,7 @@ private AbstractDestroyObjectsJob createJob(DestroyObjectsRequest request) {
             job.setIndexingMessageSender(indexingMessageSender);
             job.setBinaryDestroyedMessageSender(binaryDestroyedMessageSender);
             job.setPremisLoggerFactory(premisLoggerFactory);
+            job.setMemberOrderRequestSender(memberOrderRequestSender);
             return job;
         }
     }
@@ -140,4 +143,8 @@ public void setBinaryDestroyedMessageSender(MessageSender binaryDestroyedMessage
     public void setPremisLoggerFactory(PremisLoggerFactory premisLoggerFactory) {
         this.premisLoggerFactory = premisLoggerFactory;
     }
+
+    public void setMemberOrderRequestSender(MemberOrderRequestSender memberOrderRequestSender) {
+        this.memberOrderRequestSender = memberOrderRequestSender;
+    }
 }

services-camel-app/src/main/java/edu/unc/lib/boxc/services/camel/solrUpdate/SolrUpdateProcessor.java

@@ -8,6 +8,7 @@
 import edu.unc.lib.boxc.model.api.objects.FileObject;
 import edu.unc.lib.boxc.model.api.objects.RepositoryObject;
 import edu.unc.lib.boxc.model.api.objects.RepositoryObjectLoader;
+import edu.unc.lib.boxc.model.api.objects.Tombstone;
 import edu.unc.lib.boxc.model.api.objects.WorkObject;
 import edu.unc.lib.boxc.model.fcrepo.ids.PIDs;
 import edu.unc.lib.boxc.operations.jms.MessageSender;
@@ -28,6 +29,7 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
 import java.util.Map;
@@ -89,7 +91,7 @@ public void process(Exchange exchange) throws Exception {
             }
 
             SolrUpdateRequest updateRequest;
-            if (children == null) {
+            if (children.isEmpty()) {
                 updateRequest = new SolrUpdateRequest(pid, actionType, null, author);
             } else {
                 updateRequest = new ChildSetRequest(pid, children, actionType, author);
@@ -102,6 +104,10 @@ public void process(Exchange exchange) throws Exception {
             }
             var targetPid = PIDs.get(pid);
             var targetObj = repoObjLoader.getRepositoryObject(targetPid);
+            if (targetObj instanceof Tombstone && !actionSupportsTombstones(actionType)) {
+                log.info("Ignoring action {} on tombstone {}", action, pid);
+                return;
+            }
             String previousTitle = null;
             if (needsUpdateOfChildrenPathInfo(targetObj, actionType)) {
                 previousTitle = titleRetrievalService.retrieveCachedTitle(targetPid);
@@ -113,6 +119,12 @@ public void process(Exchange exchange) throws Exception {
         }
     }
 
+    private static boolean actionSupportsTombstones(IndexingActionType actionType) {
+        return actionType == IndexingActionType.DELETE ||
+                actionType == IndexingActionType.DELETE_SOLR_TREE ||
+                actionType == IndexingActionType.DELETE_CHILDREN_PRIOR_TO_TIMESTAMP;
+    }
+
     private void triggerFollowupActions(RepositoryObject targetObj, IndexingActionType actionType,
                                         String previousTitle) {
         // Trigger update of parent work obj for files if the action requires it
@@ -152,7 +164,7 @@ private boolean needsUpdateOfChildrenPathInfo(RepositoryObject targetObj, Indexi
     private List<String> extractChildren(Element body) {
         Element childrenEl = body.getChild("children", CDR_MESSAGE_NS);
         if (childrenEl == null) {
-            return null;
+            return Collections.EMPTY_LIST;
         }
         return childrenEl.getChildren("pid", CDR_MESSAGE_NS).stream()
                 .map(c -> c.getTextTrim())

services-camel-app/src/main/webapp/WEB-INF/service-context.xml

@@ -266,6 +266,16 @@
 
     <bean id="indexingMessageProcessor" class="edu.unc.lib.boxc.services.camel.triplesReindexing.IndexingMessageProcessor">
     </bean>
+
+    <bean id="importMemberOrderJmsTemplate" class="org.springframework.jms.core.JmsTemplate">
+        <property name="connectionFactory" ref="pooledAmqConnectionFactory" />
+        <property name="defaultDestinationName" value="${cdr.ordermembers.stream}" />
+        <property name="pubSubDomain" value="false" />
+    </bean>
+
+    <bean id="memberOrderRequestSender" class="edu.unc.lib.boxc.operations.jms.order.MemberOrderRequestSender">
+        <property name="jmsTemplate" ref="importMemberOrderJmsTemplate"/>
+    </bean>
 
     <bean id="destroyObjectsProcessor" class="edu.unc.lib.boxc.services.camel.destroy.DestroyObjectsProcessor">
         <property name="aclService" ref="aclService" />
@@ -280,6 +290,7 @@
         <property name="indexingMessageSender" ref="indexingMessageSender" />
         <property name="binaryDestroyedMessageSender" ref="binaryDestroyedMessageSender" />
         <property name="premisLoggerFactory" ref="premisLoggerFactory" />
+        <property name="memberOrderRequestSender" ref="memberOrderRequestSender" />
     </bean>
 
     <bean id="fcrepo" class="org.fcrepo.camel.FcrepoComponent">

services-camel-app/src/test/java/edu/unc/lib/boxc/services/camel/solrUpdate/SolrUpdateProcessorTest.java

@@ -4,7 +4,10 @@
 import static edu.unc.lib.boxc.model.api.xml.JDOMNamespaceUtil.CDR_MESSAGE_NS;
 import static edu.unc.lib.boxc.operations.jms.indexing.IndexingActionType.ADD;
 import static edu.unc.lib.boxc.operations.jms.indexing.IndexingActionType.DELETE;
+import static edu.unc.lib.boxc.operations.jms.indexing.IndexingActionType.UPDATE_DESCRIPTION;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.mock;
@@ -19,13 +22,18 @@
 import java.util.Map;
 import java.util.UUID;
 
+import edu.unc.lib.boxc.model.api.objects.CollectionObject;
 import edu.unc.lib.boxc.model.api.objects.FileObject;
 import edu.unc.lib.boxc.model.api.objects.RepositoryObjectLoader;
+import edu.unc.lib.boxc.model.api.objects.Tombstone;
 import edu.unc.lib.boxc.model.api.objects.WorkObject;
 import edu.unc.lib.boxc.operations.jms.MessageSender;
+import edu.unc.lib.boxc.operations.jms.indexing.IndexingMessageSender;
+import edu.unc.lib.boxc.search.solr.services.TitleRetrievalService;
 import edu.unc.lib.boxc.services.camel.ProcessorTestHelper;
 import org.apache.camel.Exchange;
 import org.apache.camel.Message;
+import org.apache.solr.client.solrj.SolrClient;
 import org.jdom2.Document;
 import org.jdom2.Element;
 import org.junit.Before;
@@ -63,29 +71,40 @@ public class SolrUpdateProcessorTest {
     private Message msg;
 
     @Mock
-    private IndexingAction mockAddAction;
+    private IndexingAction mockAction;
     @Mock
     private IndexingAction mockUpdateAccessAction;
     @Mock
     private RepositoryObjectLoader repositoryObjectLoader;
     @Mock
     private MessageSender messageSender;
+    @Mock
+    private TitleRetrievalService titleRetrievalService;
+    @Mock
+    private IndexingMessageSender indexingMessageSender;
+    @Mock
+    private SolrClient solrClient;
 
     @Captor
-    private ArgumentCaptor<ChildSetRequest> childSetCaptor;
+    private ArgumentCaptor<SolrUpdateRequest> requestCaptor;
 
     @Before
     public void init() {
         closeable = openMocks(this);
 
         indexingActionMap = new HashMap<>();
-        indexingActionMap.put(ADD, mockAddAction);
+        indexingActionMap.put(ADD, mockAction);
         indexingActionMap.put(IndexingActionType.UPDATE_ACCESS, mockUpdateAccessAction);
+        indexingActionMap.put(DELETE, mockAction);
+        indexingActionMap.put(UPDATE_DESCRIPTION, mockAction);
 
         processor = new SolrUpdateProcessor();
         processor.setSolrIndexingActionMap(indexingActionMap);
         processor.setRepositoryObjectLoader(repositoryObjectLoader);
         processor.setUpdateWorkSender(messageSender);
+        processor.setTitleRetrievalService(titleRetrievalService);
+        processor.setIndexingMessageSender(indexingMessageSender);
+        processor.setSolrClient(solrClient);
 
         bodyDoc = new Document();
         when(exchange.getIn()).thenReturn(msg);
@@ -105,7 +124,7 @@ public void testMessageNoChildren() throws Exception {
 
         processor.process(exchange);
 
-        verify(mockAddAction).performAction(any(SolrUpdateRequest.class));
+        verify(mockAction).performAction(any(SolrUpdateRequest.class));
     }
 
     @Test
@@ -115,20 +134,20 @@ public void testMessageWithChildren() throws Exception {
 
         processor.process(exchange);
 
-        verify(mockAddAction).performAction(childSetCaptor.capture());
-        ChildSetRequest childSetRequest = childSetCaptor.getValue();
+        verify(mockAction).performAction(requestCaptor.capture());
+        ChildSetRequest childSetRequest = (ChildSetRequest) requestCaptor.getValue();
 
         assertTrue(childSetRequest.getChildren().containsAll(childrenPaths));
     }
 
     @Test
     public void testInvalidIndexingAction() throws Exception {
         // This action is not mapped
-        populateEntry(DELETE);
+        populateEntry(IndexingActionType.UPDATE_TYPE);
 
         processor.process(exchange);
 
-        verify(mockAddAction, never()).performAction(any());
+        verify(mockAction, never()).performAction(any());
     }
 
     @Test
@@ -146,7 +165,7 @@ public void testFileMessageUpdateWork() throws Exception {
 
         verify(messageSender).sendMessage(workPid.getQualifiedId());
         // Regular indexing should also happen
-        verify(mockAddAction).performAction(any(SolrUpdateRequest.class));
+        verify(mockAction).performAction(any(SolrUpdateRequest.class));
     }
 
     @Test
@@ -166,18 +185,81 @@ public void testFileMessageNotNeedWorkUpdate() throws Exception {
         verify(mockUpdateAccessAction).performAction(any(SolrUpdateRequest.class));
     }
 
+    @Test
+    public void testTombstoneUpdate() throws Exception {
+        populateEntry(IndexingActionType.ADD);
+        var tombstone = mock(Tombstone.class);
+        when(tombstone.getPid()).thenReturn(targetPid);
+        when(repositoryObjectLoader.getRepositoryObject(targetPid)).thenReturn(tombstone);
+
+        processor.process(exchange);
+
+        // Tombstones should not be indexed for update action
+        verify(mockAction, never()).performAction(any());
+    }
+
+    @Test
+    public void testTombstoneDelete() throws Exception {
+        populateEntry(IndexingActionType.DELETE);
+        var tombstone = mock(Tombstone.class);
+        when(tombstone.getPid()).thenReturn(targetPid);
+        when(repositoryObjectLoader.getRepositoryObject(targetPid)).thenReturn(tombstone);
+
+        processor.process(exchange);
+
+        // Tombstones should not be indexed for update action
+        verify(mockAction).performAction(any());
+    }
+
     private Element populateEntry(IndexingActionType type) {
         Element entry = new Element("entry", ATOM_NS);
         bodyDoc.addContent(entry);
 
         entry.addContent(new Element("pid", ATOM_NS).setText(targetPid.getRepositoryPath()));
+        entry.addContent(new Element("author", ATOM_NS).setText("someone"));
 
         entry.addContent(new Element("actionType", ATOM_NS)
                 .setText(type.getURI().toString()));
 
         return entry;
     }
 
+    @Test
+    public void testUpdateCollectionDescription() throws Exception {
+        populateEntry(UPDATE_DESCRIPTION);
+        var targetCollection = mock(CollectionObject.class);
+        when(targetCollection.getPid()).thenReturn(targetPid);
+        when(repositoryObjectLoader.getRepositoryObject(targetPid)).thenReturn(targetCollection);
+        when(titleRetrievalService.retrieveCachedTitle(targetPid)).thenReturn("previous title");
+        when(titleRetrievalService.retrieveTitle(targetPid)).thenReturn("new title");
+
+        processor.process(exchange);
+
+        // Regular indexing should also happen
+        verify(mockAction).performAction(any(SolrUpdateRequest.class));
+        verify(indexingMessageSender).sendIndexingOperation("", targetPid, IndexingActionType.UPDATE_PARENT_PATH_TREE);
+    }
+
+    @Test
+    public void testAddWithParams() throws Exception {
+        populateEntry(ADD);
+        bodyDoc.getRootElement()
+                .addContent(new Element("params", CDR_MESSAGE_NS)
+                        .addContent(new Element("param", CDR_MESSAGE_NS)
+                                .setAttribute("name", "key1")
+                                .setText("value1")));
+        var targetWork = mock(CollectionObject.class);
+        when(targetWork.getPid()).thenReturn(targetPid);
+        when(repositoryObjectLoader.getRepositoryObject(targetPid)).thenReturn(targetWork);
+
+        processor.process(exchange);
+
+        // Regular indexing should also happen
+        verify(mockAction).performAction(requestCaptor.capture());
+        assertNotNull(requestCaptor.getValue().getParams());
+        assertEquals("value1", requestCaptor.getValue().getParams().get("key1"));
+    }
+
     private List<PID> addChildren(int count) {
         Element entry = bodyDoc.getRootElement();
         Element children = new Element("children", CDR_MESSAGE_NS);

services-camel-app/src/test/resources/destroy-objects-context.xml

@@ -24,6 +24,11 @@
         <property name="indexingMessageSender" ref="indexingMessageSender" />
         <property name="binaryDestroyedMessageSender" ref="binaryDestroyedMessageSender" />
         <property name="premisLoggerFactory" ref="premisLoggerFactory" />
+        <property name="memberOrderRequestSender" ref="memberOrderRequestSender" />
+    </bean>
+
+    <bean id="memberOrderRequestSender" class="org.mockito.Mockito" factory-method="mock">
+        <constructor-arg type="java.lang.Class" value="edu.unc.lib.boxc.operations.jms.order.MemberOrderRequestSender" />
     </bean>
 
     <bean id="premisLoggerFactory" class="org.mockito.Mockito" factory-method="mock">