services: disallow duplicate rules for a binlogging services or methods (#4868)

Let's simplify the impementation to just disallow duplicates. This
makes it easier to maintain.

Background info: https://github.com/grpc/proposal/pull/104

2 files changed, 77 insertions, 58 deletions

diff --git a/services/src/main/java/io/grpc/services/BinlogHelper.java b/services/src/main/java/io/grpc/services/BinlogHelper.java
index 260016756..7ae26d88f 100644
--- a/services/src/main/java/io/grpc/services/BinlogHelper.java
+++ b/services/src/main/java/io/grpc/services/BinlogHelper.java
@@ -18,6 +18,7 @@ package io.grpc.services;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
 import static io.grpc.services.BinaryLogProvider.BYTEARRAY_MARSHALLER;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -507,47 +508,50 @@ final class BinlogHelper {
         for (String configuration : Splitter.on(',').split(configurationString)) {
           Matcher configMatcher = configRe.matcher(configuration);
           if (!configMatcher.matches()) {
-            throw new IllegalArgumentException("Bad input: " + configuration);
+            throw new IllegalArgumentException("Illegal log config pattern: " + configuration);
           }
           String methodOrSvc = configMatcher.group(1);
           String binlogOptionStr = configMatcher.group(2);
-          BinlogHelper binLog = createBinaryLog(sink, binlogOptionStr);
-          if (binLog == null) {
-            continue;
-          }
           if (methodOrSvc.equals("*")) {
-            if (globalLog != null) {
-              logger.log(Level.SEVERE, "Ignoring duplicate entry: {0}", configuration);
-              continue;
-            }
-            globalLog = binLog;
+            // parse config for "*"
+            checkState(
+                globalLog == null,
+                "Duplicate entry, this is fatal: " + configuration);
+            globalLog = createBinaryLog(sink, binlogOptionStr);
             logger.log(Level.INFO, "Global binlog: {0}", binlogOptionStr);
           } else if (isServiceGlob(methodOrSvc)) {
+            // parse config for a service, e.g. "service/*"
             String service = MethodDescriptor.extractFullServiceName(methodOrSvc);
-            if (perServiceLogs.containsKey(service)) {
-              logger.log(Level.SEVERE, "Ignoring duplicate entry: {0}", configuration);
-              continue;
-            }
-            perServiceLogs.put(service, binLog);
+            checkState(
+                !perServiceLogs.containsKey(service),
+                "Duplicate entry, this is fatal: " + configuration);
+            perServiceLogs.put(service, createBinaryLog(sink, binlogOptionStr));
             logger.log(
                 Level.INFO,
                 "Service binlog: service={0} config={1}",
                 new Object[] {service, binlogOptionStr});
           } else if (methodOrSvc.startsWith("-")) {
+            // parse config for a method, e.g. "-service/method"
             String blacklistedMethod = methodOrSvc.substring(1);
             if (blacklistedMethod.length() == 0) {
               continue;
             }
-            if (!blacklistedMethods.add(blacklistedMethod)) {
-              logger.log(Level.SEVERE, "Ignoring duplicate entry: {0}", configuration);
-            }
+            checkState(
+                !blacklistedMethods.contains(blacklistedMethod),
+                "Duplicate entry, this is fatal: " + configuration);
+            checkState(
+                !perMethodLogs.containsKey(blacklistedMethod),
+                "Duplicate entry, this is fatal: " + configuration);
+            blacklistedMethods.add(blacklistedMethod);
           } else {
-            // assume fully qualified method name
-            if (perMethodLogs.containsKey(methodOrSvc)) {
-              logger.log(Level.SEVERE, "Ignoring duplicate entry: {0}", configuration);
-              continue;
-            }
-            perMethodLogs.put(methodOrSvc, binLog);
+            // parse config for a fully qualified method, e.g "serice/method"
+            checkState(
+                !perMethodLogs.containsKey(methodOrSvc),
+                "Duplicate entry, this is fatal: " + configuration);
+            checkState(
+                !blacklistedMethods.contains(methodOrSvc),
+                "Duplicate entry, this method was blacklisted: " + configuration);
+            perMethodLogs.put(methodOrSvc, createBinaryLog(sink, binlogOptionStr));
             logger.log(
                 Level.INFO,
                 "Method binlog: method={0} config={1}",
@@ -619,13 +623,11 @@ final class BinlogHelper {
               maxHeaderStr != null ? Integer.parseInt(maxHeaderStr) : Integer.MAX_VALUE;
           maxMsgBytes = maxMsgStr != null ? Integer.parseInt(maxMsgStr) : Integer.MAX_VALUE;
         } else {
-          logger.log(Level.SEVERE, "Illegal log config pattern: " + logConfig);
-          return null;
+          throw new IllegalArgumentException("Illegal log config pattern");
         }
         return new BinlogHelper(new SinkWriterImpl(sink, maxHeaderBytes, maxMsgBytes));
       } catch (NumberFormatException e) {
-        logger.log(Level.SEVERE, "Illegal log config pattern: " + logConfig);
-        return null;
+        throw new IllegalArgumentException("Illegal log config pattern");
       }
     }
 
diff --git a/services/src/test/java/io/grpc/services/BinlogHelperTest.java b/services/src/test/java/io/grpc/services/BinlogHelperTest.java
index cdaa83691..0b5961cea 100644
--- a/services/src/test/java/io/grpc/services/BinlogHelperTest.java
+++ b/services/src/test/java/io/grpc/services/BinlogHelperTest.java
@@ -25,6 +25,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Matchers.isNull;
@@ -219,18 +220,37 @@ public final class BinlogHelperTest {
         makeLog("{h:256;m}"));
   }
 
+  private void assertIllegalPatternDetected(String perSvcOrMethodConfig) {
+    try {
+      FactoryImpl.createBinaryLog(sink, perSvcOrMethodConfig);
+      fail();
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected).hasMessageThat().startsWith("Illegal log config pattern");
+    }
+  }
+
+  @Test
+  public void badFactoryConfigStrDetected() throws Exception {
+    try {
+      new FactoryImpl(sink, "obviouslybad{");
+      fail();
+    } catch (IllegalArgumentException expected) {
+      assertThat(expected).hasMessageThat().startsWith("Illegal log config pattern");
+    }
+  }
+
   @Test
   public void createLogFromOptionString_malformed() throws Exception {
-    assertNull(makeLog("bad"));
-    assertNull(makeLog("{bad}"));
-    assertNull(makeLog("{x;y}"));
-    assertNull(makeLog("{h:abc}"));
-    assertNull(makeLog("{2}"));
-    assertNull(makeLog("{2;2}"));
+    assertIllegalPatternDetected("bad");
+    assertIllegalPatternDetected("{bad}");
+    assertIllegalPatternDetected("{x;y}");
+    assertIllegalPatternDetected("{h:abc}");
+    assertIllegalPatternDetected("{2}");
+    assertIllegalPatternDetected("{2;2}");
     // The grammar specifies that if both h and m are present, h comes before m
-    assertNull(makeLog("{m:123;h:123}"));
+    assertIllegalPatternDetected("{m:123;h:123}");
     // NumberFormatException
-    assertNull(makeLog("{h:99999999999999}"));
+    assertIllegalPatternDetected("{h:99999999999999}");
   }
 
   @Test
@@ -289,35 +309,32 @@ public final class BinlogHelperTest {
     assertNotNull(makeLog("-p.s/blacklisted,p.s/*", "p.s/allowed"));
   }
 
+  private void assertDuplicatelPatternDetected(String factoryConfigStr) {
+    try {
+      new BinlogHelper.FactoryImpl(sink, factoryConfigStr);
+      fail();
+    } catch (IllegalStateException expected) {
+      assertThat(expected).hasMessageThat().startsWith("Duplicate entry");
+    }
+  }
+
   @Test
-  public void configBinLog_ignoreDuplicates_global() throws Exception {
-    String configStr = "*{h},p.s/m,*{h:256}";
-    // The duplicate
-    assertSameLimits(HEADER_FULL, makeLog(configStr, "p.other1/m"));
-    assertSameLimits(HEADER_FULL, makeLog(configStr, "p.other2/m"));
-    // Other
-    assertSameLimits(BOTH_FULL, makeLog(configStr, "p.s/m"));
+  public void configBinLog_duplicates_global() throws Exception {
+    assertDuplicatelPatternDetected("*{h},*{h:256}");
   }
 
   @Test
-  public void configBinLog_ignoreDuplicates_service() throws Exception {
-    String configStr = "p.s/*,*{h:256},p.s/*{h}";
-    // The duplicate
-    assertSameLimits(BOTH_FULL, makeLog(configStr, "p.s/m1"));
-    assertSameLimits(BOTH_FULL, makeLog(configStr, "p.s/m2"));
-    // Other
-    assertSameLimits(HEADER_256, makeLog(configStr, "p.other1/m"));
-    assertSameLimits(HEADER_256, makeLog(configStr, "p.other2/m"));
+  public void configBinLog_duplicates_service() throws Exception {
+    assertDuplicatelPatternDetected("p.s/*,p.s/*{h}");
+
   }
 
   @Test
-  public void configBinLog_ignoreDuplicates_method() throws Exception {
-    String configStr = "p.s/m,*{h:256},p.s/m{h}";
-    // The duplicate
-    assertSameLimits(BOTH_FULL, makeLog(configStr, "p.s/m"));
-    // Other
-    assertSameLimits(HEADER_256, makeLog(configStr, "p.other1/m"));
-    assertSameLimits(HEADER_256, makeLog(configStr, "p.other2/m"));
+  public void configBinLog_duplicates_method() throws Exception {
+    assertDuplicatelPatternDetected("p.s/*,p.s/*{h:1;m:2}");
+    assertDuplicatelPatternDetected("p.s/m,-p.s/m");
+    assertDuplicatelPatternDetected("-p.s/m,p.s/m");
+    assertDuplicatelPatternDetected("-p.s/m,-p.s/m");
   }
 
   @Test