CNDB-15280: Address review feedback on redaction

Author: adelapena

src/java/org/apache/cassandra/db/AbstractReadQuery.java

@@ -64,7 +64,7 @@ public TableMetadata metadata()
     // Monitorable interface
     public String name()
     {
-        return toCQLString();
+        return toRedactedCQLString();
     }
 
     @Override
@@ -100,18 +100,24 @@ public ColumnFilter columnFilter()
     /**
      * Recreates the CQL string corresponding to this query, representing any specific values with '?',
      * to prevent leaking sensitive data.
-     * <p>
-     * Note that in general the returned string will not be exactly the original user string, first
-     * because there isn't always a single syntax for a given query, but also because we don't have
-     * all the information needed (we know the non-PK columns queried but not the PK ones as internally
-     * we query them all). So this shouldn't be relied upon too strongly, but this should be good enough for
-     * monitoring purposes which is what this is for.
+     * @see #toCQLString(boolean)
      */
-    public String toCQLString()
+    public String toRedactedCQLString()
     {
         return toCQLString(true);
     }
 
+    /**
+     * Recreates the CQL string corresponding to this query, printing specific values without any redaction.
+     * This might leak sensitive data if the query string ends up in logs or any other unprotected place, so this only
+     * should be used for debugging purposes or to present the query string to the same end user that created the query.
+     * @see #toCQLString(boolean)
+     */
+    public String toUnredactedCQLString()
+    {
+        return toCQLString(false);
+    }
+
     /**
      * Recreates the CQL string corresponding to this query.
      * </p>
@@ -130,7 +136,7 @@ public String toCQLString()
      * @param redact whether to redact the queried column values.
      */
     @VisibleForTesting
-    public String toCQLString(boolean redact)
+    protected String toCQLString(boolean redact)
     {
         CqlBuilder builder = new CqlBuilder();
         builder.append("SELECT ").append(columnFilter().toCQLString(redact));

src/java/org/apache/cassandra/db/ReadCommand.java

@@ -607,7 +607,7 @@ private Threshold.GuardedCounter createTombstoneCounter()
                 Threshold guardrail = shouldRespectTombstoneThresholds()
                                                 ? Guardrails.scannedTombstones
                                                 : DefaultGuardrail.DefaultThreshold.NEVER_TRIGGERED;
-                return guardrail.newCounter(ReadCommand.this::toCQLString, false, null);
+                return guardrail.newCounter(ReadCommand.this::toRedactedCQLString, false, null);
             }
 
             private MetricRecording()
@@ -671,7 +671,7 @@ private void countTombstone(ClusteringPrefix<?> clustering)
                 {
                     metric.tombstoneFailures.inc();
                     throw new TombstoneOverwhelmingException(tombstones.get(),
-                                                             ReadCommand.this.toCQLString(),
+                                                             ReadCommand.this.toRedactedCQLString(),
                                                              ReadCommand.this.metadata(),
                                                              currentKey,
                                                              clustering);

src/java/org/apache/cassandra/db/ReadExecutionController.java

@@ -112,7 +112,7 @@ int oldestUnrepairedTombstone()
     {
         return oldestUnrepairedTombstone;
     }
-    
+
     void updateMinOldestUnrepairedTombstone(int candidate)
     {
         oldestUnrepairedTombstone = Math.min(oldestUnrepairedTombstone, candidate);
@@ -254,15 +254,15 @@ public boolean isRepairedDataDigestConclusive()
     {
         return repairedDataInfo.isConclusive();
     }
-    
+
     public RepairedDataInfo getRepairedDataInfo()
     {
         return repairedDataInfo;
     }
 
     private void addSample()
     {
-        String cql = command.toCQLString();
+        String cql = command.toRedactedCQLString();
         int timeMicros = (int) Math.min(TimeUnit.NANOSECONDS.toMicros(clock.now() - createdAtNanos), Integer.MAX_VALUE);
         ColumnFamilyStore cfs = ColumnFamilyStore.getIfExists(baseMetadata.id);
         if (cfs != null)

src/java/org/apache/cassandra/db/filter/RowFilter.java

@@ -1423,18 +1423,17 @@ public String toCQLString(boolean redact)
         private String likeToCQLString(String pattern, AbstractType<?> type, boolean redact)
         {
             if (redact)
-                return String.format("%s LIKE ?", column.name);
+                return String.format("%s LIKE ?", column.name.toCQLString());
 
             String stringValue = String.format(pattern, type.getString(value));
-            return String.format("%s LIKE %s", column.name, truncateValue(stringValue));
+            return String.format("%s LIKE %s", column.name.toCQLString(), truncateValue(stringValue));
         }
 
         private static String truncateValue(String value)
         {
             return value.length() > 9 ? value.substring(0, 6) + "..." : value;
         }
 
-
         @Override
         protected Kind kind()
         {

src/java/org/apache/cassandra/db/marshal/AbstractType.java

@@ -183,7 +183,7 @@ public ByteBuffer decompose(T value)
     }
 
     /**
-     * Generates a CQL literal representing the specified binary value.
+     * Returns a CQL literal representing the specified binary value, or "?" if redaction is requested.
      *
      * @param bytes the value to convert to a CQL literal.
      * @param redact whether to mask the value with '?' (for redaction purposes)

src/java/org/apache/cassandra/index/sai/plan/Plan.java

@@ -284,31 +284,40 @@ enum ControlFlow { Continue, Break }
     protected abstract double estimateSelectivity();
 
     /**
-     * Formats the whole plan as a pretty tree
+     * Formats the whole plan as a pretty tree, redacting the queried column values.
      */
-    public final String toStringRecursive()
+    public final String toRedactedStringRecursive()
     {
         return toStringRecursive(true);
     }
 
+    /**
+     * Formats the whole plan as a pretty tree, redacting the queried column values.
+     */
+    public final String toUnredactedStringRecursive()
+    {
+        return toStringRecursive(false);
+    }
+
     /**
      * Formats the whole plan as a pretty tree
      *
      * @param redact whether to redact the queried column values.
      */
-    public final String toStringRecursive(boolean redact)
+    private final String toStringRecursive(boolean redact)
     {
         TreeFormatter<Plan> formatter = new TreeFormatter<>(plan -> plan.toString(redact), Plan::subplans);
         return formatter.format(this);
     }
 
     /**
-     * Returns the string representation of this node only
+     * Returns the string representation of this node only, without redacting the queried column values.
+     * @see #toString(boolean)
      */
     @Override
     public final String toString()
     {
-        return toString(true);
+        return toString(false);
     }
 
     /**
@@ -327,7 +336,7 @@ public final String toString(boolean redact)
 
     /**
      * Returns additional information specific to the node displayed in the first line.
-     * The information is included in the output of {@link #toString()} and {@link #toStringRecursive()}.
+     * The information is included in the output of {@link #toString()} and {@link #toRedactedStringRecursive()}.
      * It is up to subclasses to implement it.
      */
     protected String title(boolean redact)
@@ -337,7 +346,7 @@ protected String title(boolean redact)
 
     /**
      * Returns additional information specific to the node, displayed below the title.
-     * The information is included in the output of {@link #toString()} and {@link #toStringRecursive()}.
+     * The information is included in the output of {@link #toString()} and {@link #toRedactedStringRecursive()}.
      * It is up to subclasses to implement it.
      */
     protected String description()
@@ -366,7 +375,7 @@ protected String description()
     public final Plan optimize()
     {
         if (logger.isTraceEnabled())
-            logger.trace("Optimizing plan:\n{}", toStringRecursive());
+            logger.trace("Optimizing plan:\n{}", toRedactedStringRecursive());
 
         Plan bestPlanSoFar = this;
         List<Leaf> leaves = nodesOfType(Leaf.class);
@@ -381,14 +390,14 @@ public final Plan optimize()
 
             Plan candidate = bestPlanSoFar.removeRestriction(leaf.id);
             if (logger.isTraceEnabled())
-                logger.trace("Candidate query plan:\n{}", candidate.toStringRecursive());
+                logger.trace("Candidate query plan:\n{}", candidate.toRedactedStringRecursive());
 
             if (candidate.fullCost() <= bestPlanSoFar.fullCost())
                 bestPlanSoFar = candidate;
         }
 
         if (logger.isTraceEnabled())
-            logger.trace("Optimized plan:\n{}", bestPlanSoFar.toStringRecursive());
+            logger.trace("Optimized plan:\n{}", bestPlanSoFar.toRedactedStringRecursive());
         return bestPlanSoFar;
     }
 

src/java/org/apache/cassandra/index/sai/plan/QueryController.java

@@ -415,11 +415,11 @@ Plan buildPlan()
         updateIndexMetricsQueriesCount(plan);
 
         if (logger.isTraceEnabled())
-            logger.trace("Query execution plan:\n" + plan.toStringRecursive());
+            logger.trace("Query execution plan:\n" + plan.toRedactedStringRecursive());
 
         if (Tracing.isTracing())
         {
-            Tracing.trace("Query execution plan:\n" + plan.toStringRecursive(false));
+            Tracing.trace("Query execution plan:\n" + plan.toUnredactedStringRecursive());
             List<Plan.IndexScan> origIndexScans = keysIterationPlan.nodesOfType(Plan.IndexScan.class);
             List<Plan.IndexScan> selectedIndexScans = plan.nodesOfType(Plan.IndexScan.class);
             Tracing.trace("Selecting {} {} of {} out of {} indexes",

src/java/org/apache/cassandra/service/context/DefaultOperationContext.java

@@ -58,7 +58,7 @@ static class Factory implements OperationContext.Factory
         @Override
         public OperationContext forRead(ReadCommand command, ColumnFamilyStore cfs)
         {
-            return new DefaultOperationContext(command::toCQLString);
+            return new DefaultOperationContext(command::toRedactedCQLString);
         }
     }
 }

src/java/org/apache/cassandra/service/reads/ReplicaFilteringProtection.java

@@ -199,7 +199,7 @@ public void close()
                     String message = String.format("Replica filtering protection has cached up to %d rows during query %s, " +
                                                    "which is over the warning threshold of %d rows defined by " +
                                                    "'cached_replica_rows_warn_threshold' in cassandra.yaml.",
-                                                   maxRowsCached, command.toCQLString(), cachedRowsWarnThreshold);
+                                                   maxRowsCached, command.toRedactedCQLString(), cachedRowsWarnThreshold);
 
                     ClientWarn.instance.warn(message);
                     oneMinuteLogger.warn(message);
@@ -289,7 +289,7 @@ private void incrementCachedRows()
             String message = String.format("Replica filtering protection has cached %d rows during query %s, " +
                                            "which is over the failure threshold of %d rows defined by " +
                                            "'cached_replica_rows_fail_threshold' in cassandra.yaml.",
-                                           currentRowsCached, command.toCQLString(), cachedRowsFailThreshold);
+                                           currentRowsCached, command.toRedactedCQLString(), cachedRowsFailThreshold);
 
             logger.error(message);
             Tracing.trace(message);

src/java/org/apache/cassandra/service/reads/repair/ReadRepairEvent.java

@@ -22,7 +22,6 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -66,7 +65,7 @@ enum ReadRepairEventType
     {
         this.keyspace = readRepair.cfs.keyspace;
         this.tableName = readRepair.cfs.getTableName();
-        this.cqlCommand = readRepair.command.toCQLString();
+        this.cqlCommand = readRepair.command.toRedactedCQLString();
         this.consistency = readRepair.replicaPlan().consistencyLevel();
         this.speculativeRetry = readRepair.cfs.metadata().params.speculativeRetry.kind();
         this.destinations = destinations;