Fix config items being deleted when they shouldnt

connexta · Apr 1, 2019 · dcc7c17 · dcc7c17
1 parent e209fbc
commit dcc7c17
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 51 deletions.
diff --git a/...-impl/src/main/java/org/codice/ditto/replication/api/impl/ReplicationItemManagerImpl.java b/...-impl/src/main/java/org/codice/ditto/replication/api/impl/ReplicationItemManagerImpl.java
@@ -20,6 +20,7 @@
 import java.util.Optional;
 import java.util.stream.Collectors;
 import org.apache.commons.collections4.CollectionUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.codice.ddf.persistence.PersistenceException;
 import org.codice.ddf.persistence.PersistentItem;
 import org.codice.ddf.persistence.PersistentStore;
@@ -115,7 +116,12 @@ public void deleteAllItems() throws PersistenceException {
   @Override
   public List<ReplicationItem> getItemsForConfig(String configId, int startIndex, int pageSize)
       throws PersistenceException {
-    String cql = String.format("'%s' = '%s'", CONFIGURATION_ID_KEY, configId);
+    String cql;
+    if (StringUtils.isNotEmpty(configId)) {
+      cql = String.format("'%s' = '%s'", CONFIGURATION_ID_KEY, configId);
+    } else {
+      cql = "";
+    }
     List<Map<String, Object>> matchingPersistentItems;
 
     matchingPersistentItems = persistentStore.get(PERSISTENCE_TYPE, cql, startIndex, pageSize);

diff --git a/...-impl/src/main/java/org/codice/ditto/replication/api/impl/ScheduledReplicatorDeleter.java b/...-impl/src/main/java/org/codice/ditto/replication/api/impl/ScheduledReplicatorDeleter.java
@@ -184,7 +184,7 @@ private void cleanupOrphanedReplicationItems(List<ReplicatorConfig> replicatorCo
   }
 
   private boolean itemNotInCatalog(ReplicationItem item, Set<String> idsInCatalog) {
-    return !idsInCatalog.contains(item.getId());
+    return !idsInCatalog.contains(item.getMetacardId());
   }
 
   private void cleanupDeletedConfigs(List<ReplicatorConfig> replicatorConfigs) {
@@ -213,27 +213,22 @@ private void cleanupDeletedConfigs(List<ReplicatorConfig> replicatorConfigs) {
         }
       }
 
-      try {
-        replicationItemManager.deleteItemsForConfig(configId);
-      } catch (PersistenceException e) {
-        LOGGER.debug(
-            "Failed to delete replication items for config: {}. Deletion will be retried next poll interval",
-            configName,
-            e);
-        return;
-      }
-
-      try {
-        deleteReplicatorHistory(config);
-      } catch (ReplicationPersistenceException e) {
-        LOGGER.debug(
-            "History for replicator configuration {} could not be deleted. Deletion will be retried next polling interval.",
-            configName,
-            e);
-        return;
+      if (deleteHistory(config)) {
+        replicatorConfigManager.remove(configId);
       }
+    }
+  }
 
-      replicatorConfigManager.remove(configId);
+  private boolean deleteHistory(ReplicatorConfig config) {
+    try {
+      deleteReplicatorHistory(config);
+      return true;
+    } catch (ReplicationPersistenceException e) {
+      LOGGER.debug(
+          "History for replicator configuration {} could not be deleted. Deletion will be retried next polling interval.",
+          config.getName(),
+          e);
+      return false;
     }
   }
 

diff --git a/...l/src/test/java/org/codice/ditto/replication/api/impl/ScheduledReplicatorDeleterTest.java b/...l/src/test/java/org/codice/ditto/replication/api/impl/ScheduledReplicatorDeleterTest.java
@@ -129,7 +129,7 @@ public void testCleanupOrphanedItems() throws PersistenceException {
     when(rep2.getConfigurationId()).thenReturn(configId);
     when(rep3.getConfigurationId()).thenReturn(configId);
     when(rep4.getConfigurationId()).thenReturn(configId);
-    when(rep5.getConfigurationId()).thenReturn(configId);
+    when(rep5.getConfigurationId()).thenReturn("noExistingConfigId");
     when(rep6.getConfigurationId()).thenReturn("noExistingConfigId");
 
     when(replicationItemManager.getItemsForConfig("", 0, pageSize))
@@ -140,9 +140,9 @@ public void testCleanupOrphanedItems() throws PersistenceException {
         .thenReturn(ImmutableList.of(rep5, rep6));
 
     when(metacards.getIdsOfMetacardsInCatalog(ImmutableSet.of(metacardId1)))
-        .thenReturn(Collections.singleton(metacardId1));
-    when(metacards.getIdsOfMetacardsInCatalog(ImmutableSet.of(metacardId6)))
-        .thenReturn(Collections.singleton(metacardId6));
+        .thenReturn(Collections.emptySet());
+    when(metacards.getIdsOfMetacardsInCatalog(ImmutableSet.of(metacardId5, metacardId6)))
+        .thenReturn(Collections.singleton(metacardId5));
 
     scheduledReplicatorDeleter.cleanup();
 
@@ -289,32 +289,6 @@ public void testFailDeleteMetacardsDoesntRemoveItemsOrConfig()
     verify(replicatorConfigManager, never()).remove(configId);
   }
 
-  @Test
-  public void testFailCleanupReplicationItemsDoesntRemoveConfig()
-      throws PersistenceException, SourceUnavailableException {
-    final String configId = "configId";
-    final String metacardId1 = "mId1";
-    ReplicatorConfig config = mockConfig(configId, "name", true, true);
-    when(replicatorConfigManager.objects()).thenReturn(Stream.of(config));
-
-    List<ReplicationItem> mockItems = new ArrayList<>();
-    mockItems.add(mockRepItem("id", metacardId1, SITE_NAME));
-    when(replicationItemManager.getItemsForConfig(configId, 0, pageSize)).thenReturn(mockItems);
-
-    doThrow(PersistenceException.class).when(replicationItemManager).deleteItemsForConfig(configId);
-
-    Set<String> itemMetacardIds = Collections.singleton(metacardId1);
-    Set<String> idsInCatalog = Collections.singleton(metacardId1);
-    when(metacards.getIdsOfMetacardsInCatalog(itemMetacardIds)).thenReturn(idsInCatalog);
-
-    scheduledReplicatorDeleter.cleanup();
-
-    verify(metacards, times(1)).doDelete(idsInCatalog.toArray(new String[0]));
-    verify(replicatorHistory, never()).getReplicationEvents(configId);
-    verify(replicatorHistory, never()).removeReplicationEvents(anySet());
-    verify(replicatorConfigManager, never()).remove(configId);
-  }
-
   @Test
   public void testFailCleanupHistoryDoesntRemoveConfig()
       throws PersistenceException, SourceUnavailableException {