Improve confirmation messages and comments for removing deleted secrets

server/src/main/java/keywhiz/commands/DropDeletedSecretsCommand.java

@@ -11,8 +11,6 @@
 import net.sourceforge.argparse4j.inf.Subparser;
 import org.joda.time.DateTime;
 
-import static java.lang.String.format;
-
 /**
  * Secrets which are deleted from Keywhiz--whether through the admin client or the automation
  * interface--are not actually removed from Keywhiz' database.  Instead, the link between the
@@ -61,16 +59,25 @@ protected DropDeletedSecretsCommand(SecretDAO secretDAO) {
 
     Integer sleepMillis = namespace.getInt(INPUT_SLEEP_MILLIS);
     if (sleepMillis < 0) {
-      System.out.format("Milliseconds to sleep must be nonnegative; got %d", sleepMillis);
+      System.out.format("Milliseconds to sleep must be nonnegative; got %d\n", sleepMillis);
       return;
     }
 
     // determine how many secrets would be affected and get user confirmation
+    long totalDeletedCount = secretDAO.countDeletedSecrets();
     long affectedCount = secretDAO.countSecretsDeletedBeforeDate(deletedBefore);
 
+    if (affectedCount == 0) {
+      System.out.format(
+          "No secrets deleted before %s were found (out of %d deleted secrets); not altering the database.\n",
+          deletedBefore.toString(), totalDeletedCount);
+      return;
+    }
+
     System.out.format(
-        "WARNING: This will PERMANENTLY remove all secrets deleted before %s.  %d secrets will be removed.  Confirm? (y/n)\n",
-        deletedBefore.toString(), affectedCount);
+        "WARNING: This will PERMANENTLY remove all secrets deleted before %s.  "
+            + "%d secrets will be removed, out of %d deleted secrets.  Confirm? (y/n)\n",
+        deletedBefore.toString(), affectedCount, totalDeletedCount);
 
     String confirm;
     if (System.console() != null) {
@@ -118,9 +125,9 @@ private DateTime getDateIfValid(String deletedBeforeStr) {
 
     // the date must be in the past
     if (before.isAfterNow() || before.isEqualNow()) {
-      System.out.println(
-          format("Cutoff date for deletion must be before current time; input of %s was invalid",
-              before.toString()));
+      System.out.format(
+          "Cutoff date for deletion must be before current time; input of %s was invalid.\n",
+          before.toString());
       return null;
     }
 

server/src/main/java/keywhiz/service/daos/SecretDAO.java

@@ -403,6 +403,14 @@ public void deleteSecretsByName(String name) {
         .deleteSecretSeriesByName(name);
   }
 
+  /**
+   * Counts the total number of deleted secrets.
+   */
+  public int countDeletedSecrets() {
+    return secretSeriesDAOFactory.using(dslContext.configuration())
+        .countDeletedSecretSeries();
+  }
+
   /**
    * Counts the number of secrets deleted before the specified cutoff.
    *
@@ -424,17 +432,18 @@ public int countSecretsDeletedBeforeDate(DateTime deleteBefore) {
    * Unlike the "delete" endpoints above, THIS REMOVAL IS PERMANENT and cannot be undone by editing
    * the database to restore the "current" entries.
    *
-   * @param deleteBefore the cutoff date; secrets deleted before this date will be deleted
+   * @param deletedBefore the cutoff date; secrets deleted before this date will be removed from
+   * the database
    * @param sleepMillis how many milliseconds to sleep between each batch of removals
    */
-  public void dangerPermanentlyRemoveSecretsDeletedBeforeDate(DateTime deleteBefore,
+  public void dangerPermanentlyRemoveSecretsDeletedBeforeDate(DateTime deletedBefore,
       int sleepMillis) throws InterruptedException {
-    checkArgument(deleteBefore != null);
+    checkArgument(deletedBefore != null);
     SecretSeriesDAO secretSeriesDAO = secretSeriesDAOFactory.using(dslContext.configuration());
     SecretContentDAO secretContentDAO = secretContentDAOFactory.using(dslContext.configuration());
 
     // identify the secrets deleted before this date
-    List<Long> ids = secretSeriesDAO.getIdsForSecretSeriesDeletedBeforeDate(deleteBefore);
+    List<Long> ids = secretSeriesDAO.getIdsForSecretSeriesDeletedBeforeDate(deletedBefore);
 
     // batch the list of secrets to be removed, to reduce load on the database
     List<List<Long>> partitionedIds = Lists.partition(ids, MAX_ROWS_REMOVED_PER_TRANSACTION);

server/src/main/java/keywhiz/service/daos/SecretSeriesDAO.java

@@ -48,6 +48,7 @@
 import static keywhiz.jooq.tables.Groups.GROUPS;
 import static keywhiz.jooq.tables.Secrets.SECRETS;
 import static keywhiz.jooq.tables.SecretsContent.SECRETS_CONTENT;
+import static org.jooq.impl.DSL.count;
 import static org.jooq.impl.DSL.decode;
 import static org.jooq.impl.DSL.least;
 import static org.jooq.impl.DSL.val;
@@ -251,6 +252,17 @@ public void deleteSecretSeriesById(long id) {
     });
   }
 
+  /**
+   * Count the number of deleted secret series
+   */
+  public int countDeletedSecretSeries() {
+    return dslContext.selectCount()
+        .from(SECRETS)
+        .where(SECRETS.CURRENT.isNull())
+        .fetchOne()
+        .value1();
+  }
+
   /**
    * Identify all secret series which were deleted before the given date.
    *

server/src/test/java/keywhiz/commands/DropDeletedSecretsCommandTest.java

@@ -168,7 +168,7 @@ public void setUp() throws Exception {
   public void testSecretDeletion_allDeletedSecrets() {
     runCommandWithConfirmationAndDate("yes", "2019-02-01T00:00:00Z", 0);
 
-    // check the database state; secret2 and secret3 should have been deleted
+    // check the database state; secret2 and secret3 should have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1), ImmutableList.of(series2, series3));
     checkExpectedSecretContents(ImmutableList.of(content1),
         ImmutableList.of(content2a, content2b, content3));
@@ -178,7 +178,7 @@ public void testSecretDeletion_allDeletedSecrets() {
   public void testSecretDeletion_filterByDate_someSecretsRemoved() {
     runCommandWithConfirmationAndDate("yes", "2018-02-01T00:00:00Z", 0);
 
-    // check the database state; only secret2 should have been deleted
+    // check the database state; only secret2 should have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series3), ImmutableList.of(series2));
     checkExpectedSecretContents(ImmutableList.of(content1, content3),
         ImmutableList.of(content2a, content2b));
@@ -188,7 +188,7 @@ public void testSecretDeletion_filterByDate_someSecretsRemoved() {
   public void testSecretDeletion_filterByDate_noSecretsRemoved() {
     runCommandWithConfirmationAndDate("yes", "2017-02-01T00:00:00Z", 0);
 
-    // check the database state; no secrets should have been deleted
+    // check the database state; no secrets should have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series2, series3), ImmutableList.of());
     checkExpectedSecretContents(ImmutableList.of(content1, content2a, content2b, content3),
         ImmutableList.of());
@@ -198,7 +198,7 @@ public void testSecretDeletion_filterByDate_noSecretsRemoved() {
   public void testSecretDeletion_futureDate() {
     runCommandWithConfirmationAndDate("yes", "5000-02-01T00:00:00Z", 0);
 
-    // check the database state; secrets should NOT have been deleted
+    // check the database state; secrets should NOT have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series2, series3), ImmutableList.of());
     checkExpectedSecretContents(ImmutableList.of(content1, content2a, content2b, content3),
         ImmutableList.of());
@@ -208,7 +208,7 @@ public void testSecretDeletion_futureDate() {
   public void testSecretDeletion_invalidDate() {
     runCommandWithConfirmationAndDate("yes", "notadate", 0);
 
-    // check the database state; secrets should NOT have been deleted
+    // check the database state; secrets should NOT have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series2, series3), ImmutableList.of());
     checkExpectedSecretContents(ImmutableList.of(content1, content2a, content2b, content3),
         ImmutableList.of());
@@ -218,7 +218,7 @@ public void testSecretDeletion_invalidDate() {
   public void testSecretDeletion_noConfirmation() {
     runCommandWithConfirmationAndDate("no", "2019-02-01T00:00:00Z", 0);
 
-    // check the database state; secrets should NOT have been deleted
+    // check the database state; secrets should NOT have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series2, series3), ImmutableList.of());
     checkExpectedSecretContents(ImmutableList.of(content1, content2a, content2b, content3),
         ImmutableList.of());
@@ -228,7 +228,7 @@ public void testSecretDeletion_noConfirmation() {
   public void testSecretDeletion_negativeSleep() {
     runCommandWithConfirmationAndDate("no", "2019-02-01T00:00:00Z", -10);
 
-    // check the database state; secrets should NOT have been deleted
+    // check the database state; secrets should NOT have been removed
     checkExpectedSecretSeries(ImmutableList.of(series1, series2, series3), ImmutableList.of());
     checkExpectedSecretContents(ImmutableList.of(content1, content2a, content2b, content3),
         ImmutableList.of());

server/src/test/java/keywhiz/service/daos/SecretDAOTest.java

@@ -513,6 +513,16 @@ public void getSecretByIdOneThrowsExceptionIfCurrentVersionIsInvalid() {
   //---------------------------------------------------------------------------------------
   // permanentlyRemoveSecret
   //---------------------------------------------------------------------------------------
+  @Test
+  public void countDeletedSecrets() {
+    // one secret was deleted in the initial test setup
+    assertThat(secretDAO.countDeletedSecrets()).isEqualTo(1);
+
+    // delete a secret and recount
+    secretDAO.deleteSecretsByName(series2.name());
+    assertThat(secretDAO.countDeletedSecrets()).isEqualTo(2);
+  }
+
   @Test
   public void countSecretsDeletedBeforeDate() {
     // some secrets may have been deleted in test setup