From bcb451583264e79ff26b32c65c717b5cfb4718e2 Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Tue, 9 Apr 2019 08:35:26 -0700 Subject: [PATCH] services: Remove dependency on re2j re2j is a fairly unnecessary dependency. Our usage of Pattern is quite limited and isn't all that hard to do manually. Our usage would be safe to use normal java.util.regex, but it's sort of annoying to keep re-explaining to others who are (rightly) concerned with java.util.regex's poor pathological behavior. --- repositories.bzl | 12 --- services/BUILD.bazel | 2 - services/build.gradle | 1 - .../java/io/grpc/services/BinlogHelper.java | 88 +++++++++++-------- .../io/grpc/services/BinlogHelperTest.java | 44 ++++++---- 5 files changed, 80 insertions(+), 67 deletions(-) diff --git a/repositories.bzl b/repositories.bzl index abb2af0383..523e0c7a0a 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -19,7 +19,6 @@ def grpc_java_repositories( omit_com_google_protobuf_java = False, omit_com_google_protobuf_javalite = False, omit_com_google_protobuf_nano_protobuf_javanano = False, - omit_com_google_re2j = False, omit_com_google_truth_truth = False, omit_com_squareup_okhttp = False, omit_com_squareup_okio = False, @@ -73,8 +72,6 @@ def grpc_java_repositories( com_google_protobuf_javalite() if not omit_com_google_protobuf_nano_protobuf_javanano: com_google_protobuf_nano_protobuf_javanano() - if not omit_com_google_re2j: - com_google_re2j() if not omit_com_google_truth_truth: com_google_truth_truth() if not omit_com_squareup_okhttp: @@ -265,15 +262,6 @@ def com_google_protobuf_nano_protobuf_javanano(): licenses = ["notice"], # BSD 2-clause ) -def com_google_re2j(): - jvm_maven_import_external( - name = "com_google_re2j", - artifact = "com.google.re2j:re2j:1.2", - server_urls = ["http://central.maven.org/maven2"], - artifact_sha256 = "e9dc705fd4c570344b54a7146b2e3a819cdc271a29793f4acc1a93b56a388e59", - licenses = ["notice"], # Go License - ) - def com_google_truth_truth(): jvm_maven_import_external( name = "com_google_truth_truth", diff --git a/services/BUILD.bazel b/services/BUILD.bazel index 04cdb83950..896e8015f8 100644 --- a/services/BUILD.bazel +++ b/services/BUILD.bazel @@ -19,7 +19,6 @@ java_library( "@com_google_guava_guava//jar", "@com_google_protobuf//:protobuf_java", "@com_google_protobuf//:protobuf_java_util", - "@com_google_re2j//jar", "@io_grpc_grpc_proto//:binarylog_java_proto", ], ) @@ -60,7 +59,6 @@ java_library( "@com_google_guava_guava//jar", "@com_google_protobuf//:protobuf_java", "@com_google_protobuf//:protobuf_java_util", - "@com_google_re2j//jar", "@io_grpc_grpc_proto//:reflection_java_proto_deprecated", "@javax_annotation_javax_annotation_api//jar", ], diff --git a/services/build.gradle b/services/build.gradle index 3b41c8f3ba..9fe6c87bca 100644 --- a/services/build.gradle +++ b/services/build.gradle @@ -24,7 +24,6 @@ dependencies { // prefer 26.0-android from libraries instead of 20.0 exclude group: 'com.google.guava', module: 'guava' } - compile libraries.re2j compileOnly libraries.javax_annotation testCompile project(':grpc-testing'), diff --git a/services/src/main/java/io/grpc/services/BinlogHelper.java b/services/src/main/java/io/grpc/services/BinlogHelper.java index ab503ca3a3..e38524d327 100644 --- a/services/src/main/java/io/grpc/services/BinlogHelper.java +++ b/services/src/main/java/io/grpc/services/BinlogHelper.java @@ -29,8 +29,6 @@ import com.google.protobuf.ByteString; import com.google.protobuf.Duration; import com.google.protobuf.util.Durations; import com.google.protobuf.util.Timestamps; -import com.google.re2j.Matcher; -import com.google.re2j.Pattern; import io.grpc.Attributes; import io.grpc.CallOptions; import io.grpc.Channel; @@ -601,20 +599,6 @@ final class BinlogHelper { } static final class FactoryImpl implements Factory { - // '*' for global, 'service/*' for service glob, or 'service/method' for fully qualified. - private static final Pattern logPatternRe = Pattern.compile("[^{]+"); - // A curly brace wrapped expression. Will be further matched with the more specified REs below. - private static final Pattern logOptionsRe = Pattern.compile("\\{[^}]+}"); - private static final Pattern configRe = Pattern.compile( - String.format("^(%s)(%s)?$", logPatternRe.pattern(), logOptionsRe.pattern())); - // Regexes to extract per-binlog options - // The form: {m:256} - private static final Pattern msgRe = Pattern.compile("\\{m(?::(\\d+))?}"); - // The form: {h:256} - private static final Pattern headerRe = Pattern.compile("\\{h(?::(\\d+))?}"); - // The form: {h:256,m:256} - private static final Pattern bothRe = Pattern.compile("\\{h(?::(\\d+))?;m(?::(\\d+))?}"); - private final BinlogHelper globalLog; private final Map perServiceLogs; private final Map perMethodLogs; @@ -632,12 +616,26 @@ final class BinlogHelper { Set blacklistedMethods = new HashSet<>(); if (configurationString != null && configurationString.length() > 0) { for (String configuration : Splitter.on(',').split(configurationString)) { - Matcher configMatcher = configRe.matcher(configuration); - if (!configMatcher.matches()) { + int leftCurly = configuration.indexOf('{'); + // '*' for global, 'service/*' for service glob, or 'service/method' for fully qualified + String methodOrSvc; + // An expression originally wrapped in curly braces; like {m:256,h:256}, {m:256}, {h:256} + String binlogOptionStr; + if (leftCurly == -1) { + methodOrSvc = configuration; + binlogOptionStr = null; + } else { + int rightCurly = configuration.indexOf('}', leftCurly); + if (rightCurly != configuration.length() - 1) { + throw new IllegalArgumentException("Illegal log config pattern: " + configuration); + } + methodOrSvc = configuration.substring(0, leftCurly); + // option without the curly braces + binlogOptionStr = configuration.substring(leftCurly + 1, configuration.length() - 1); + } + if (methodOrSvc.isEmpty()) { throw new IllegalArgumentException("Illegal log config pattern: " + configuration); } - String methodOrSvc = configMatcher.group(1); - String binlogOptionStr = configMatcher.group(2); if (methodOrSvc.equals("*")) { // parse config for "*" checkState( @@ -729,26 +727,21 @@ final class BinlogHelper { sink, TimeProvider.SYSTEM_TIME_PROVIDER, Integer.MAX_VALUE, Integer.MAX_VALUE)); } try { - Matcher headerMatcher; - Matcher msgMatcher; - Matcher bothMatcher; final int maxHeaderBytes; final int maxMsgBytes; - if ((headerMatcher = headerRe.matcher(logConfig)).matches()) { - String maxHeaderStr = headerMatcher.group(1); - maxHeaderBytes = - maxHeaderStr != null ? Integer.parseInt(maxHeaderStr) : Integer.MAX_VALUE; + String[] parts = logConfig.split(";", 2); + if (parts.length == 2) { + if (!(parts[0].startsWith("h") && parts[1].startsWith("m"))) { + throw new IllegalArgumentException("Illegal log config pattern"); + } + maxHeaderBytes = optionalInt(parts[0].substring(1)); + maxMsgBytes = optionalInt(parts[1].substring(1)); + } else if (parts[0].startsWith("h")) { + maxHeaderBytes = optionalInt(parts[0].substring(1)); maxMsgBytes = 0; - } else if ((msgMatcher = msgRe.matcher(logConfig)).matches()) { + } else if (parts[0].startsWith("m")) { maxHeaderBytes = 0; - String maxMsgStr = msgMatcher.group(1); - maxMsgBytes = maxMsgStr != null ? Integer.parseInt(maxMsgStr) : Integer.MAX_VALUE; - } else if ((bothMatcher = bothRe.matcher(logConfig)).matches()) { - String maxHeaderStr = bothMatcher.group(1); - String maxMsgStr = bothMatcher.group(2); - maxHeaderBytes = - maxHeaderStr != null ? Integer.parseInt(maxHeaderStr) : Integer.MAX_VALUE; - maxMsgBytes = maxMsgStr != null ? Integer.parseInt(maxMsgStr) : Integer.MAX_VALUE; + maxMsgBytes = optionalInt(parts[0].substring(1)); } else { throw new IllegalArgumentException("Illegal log config pattern"); } @@ -760,6 +753,29 @@ final class BinlogHelper { } } + /** Returns {@code s}, after verifying it contains only digits. */ + static String checkDigits(String s) { + for (int i = 0; i < s.length(); i++) { + char c = s.charAt(i); + if (c < '0' || '9' < c) { + throw new IllegalArgumentException("Illegal log config pattern"); + } + } + return s; + } + + /** Parses the optional int of the form "" (max int) or ":123" (123). */ + static int optionalInt(String s) { + if (s.isEmpty()) { + return Integer.MAX_VALUE; + } + if (!s.startsWith(":")) { + throw new IllegalArgumentException("Illegal log config pattern"); + } + s = checkDigits(s.substring(1)); + return Integer.parseInt(s); + } + /** * Returns true if the input string is a glob of the form: {@code /*}. */ diff --git a/services/src/test/java/io/grpc/services/BinlogHelperTest.java b/services/src/test/java/io/grpc/services/BinlogHelperTest.java index a707491fc5..8162143791 100644 --- a/services/src/test/java/io/grpc/services/BinlogHelperTest.java +++ b/services/src/test/java/io/grpc/services/BinlogHelperTest.java @@ -226,18 +226,18 @@ public final class BinlogHelperTest { @Test public void createLogFromOptionString() throws Exception { - assertSameLimits(BOTH_FULL, makeLog(null)); - assertSameLimits(HEADER_FULL, makeLog("{h}")); - assertSameLimits(MSG_FULL, makeLog("{m}")); - assertSameLimits(HEADER_256, makeLog("{h:256}")); - assertSameLimits(MSG_256, makeLog("{m:256}")); - assertSameLimits(BOTH_256, makeLog("{h:256;m:256}")); + assertSameLimits(BOTH_FULL, makeOptions(null)); + assertSameLimits(HEADER_FULL, makeOptions("h")); + assertSameLimits(MSG_FULL, makeOptions("m")); + assertSameLimits(HEADER_256, makeOptions("h:256")); + assertSameLimits(MSG_256, makeOptions("m:256")); + assertSameLimits(BOTH_256, makeOptions("h:256;m:256")); assertSameLimits( new Builder().header(Integer.MAX_VALUE).msg(256).build(), - makeLog("{h;m:256}")); + makeOptions("h;m:256")); assertSameLimits( new Builder().header(256).msg(Integer.MAX_VALUE).build(), - makeLog("{h:256;m}")); + makeOptions("h:256;m")); } private void assertIllegalPatternDetected(String perSvcOrMethodConfig) { @@ -259,18 +259,30 @@ public final class BinlogHelperTest { } } + @Test + public void badFactoryConfigStrDetected_empty() throws Exception { + try { + new FactoryImpl(sink, "*,"); + fail(); + } catch (IllegalArgumentException expected) { + assertThat(expected).hasMessageThat().startsWith("Illegal log config pattern"); + } + } + @Test public void createLogFromOptionString_malformed() throws Exception { + assertIllegalPatternDetected(""); assertIllegalPatternDetected("bad"); - assertIllegalPatternDetected("{bad}"); - assertIllegalPatternDetected("{x;y}"); - assertIllegalPatternDetected("{h:abc}"); - assertIllegalPatternDetected("{2}"); - assertIllegalPatternDetected("{2;2}"); + assertIllegalPatternDetected("mad"); + assertIllegalPatternDetected("x;y"); + assertIllegalPatternDetected("h:abc"); + assertIllegalPatternDetected("h:1e8"); + assertIllegalPatternDetected("2"); + assertIllegalPatternDetected("2;2"); // The grammar specifies that if both h and m are present, h comes before m - assertIllegalPatternDetected("{m:123;h:123}"); + assertIllegalPatternDetected("m:123;h:123"); // NumberFormatException - assertIllegalPatternDetected("{h:99999999999999}"); + assertIllegalPatternDetected("h:99999999999999"); } @Test @@ -1530,7 +1542,7 @@ public final class BinlogHelperTest { return new BinlogHelper.FactoryImpl(sink, factoryConfigStr).getLog(lookup); } - private BinlogHelper makeLog(String logConfigStr) { + private BinlogHelper makeOptions(String logConfigStr) { return FactoryImpl.createBinaryLog(sink, logConfigStr); }