diff --git a/build.gradle b/build.gradle index b419bf01e15..6ab00d73881 100644 --- a/build.gradle +++ b/build.gradle @@ -116,6 +116,7 @@ subprojects { "org.elasticsearch.distribution.tar:elasticsearch:${version}": ':distribution:tar', "org.elasticsearch.distribution.rpm:elasticsearch:${version}": ':distribution:rpm', "org.elasticsearch.distribution.deb:elasticsearch:${version}": ':distribution:deb', + "org.elasticsearch.test:logger-usage:${version}": ':test:logger-usage', ] configurations.all { resolutionStrategy.dependencySubstitution { DependencySubstitutions subs -> diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/LoggerUsageTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/LoggerUsageTask.groovy new file mode 100644 index 00000000000..b280a74db58 --- /dev/null +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/LoggerUsageTask.groovy @@ -0,0 +1,98 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.gradle.precommit + +import org.elasticsearch.gradle.LoggedExec +import org.gradle.api.file.FileCollection +import org.gradle.api.tasks.InputFiles +import org.gradle.api.tasks.OutputFile + +/** + * Runs LoggerUsageCheck on a set of directories. + */ +public class LoggerUsageTask extends LoggedExec { + + /** + * We use a simple "marker" file that we touch when the task succeeds + * as the task output. This is compared against the modified time of the + * inputs (ie the jars/class files). + */ + private File successMarker = new File(project.buildDir, 'markers/loggerUsage') + + private FileCollection classpath; + + private List classDirectories; + + public LoggerUsageTask() { + project.afterEvaluate { + dependsOn(classpath) + description = "Runs LoggerUsageCheck on ${classDirectories}" + executable = new File(project.javaHome, 'bin/java') + if (classDirectories == null) { + classDirectories = [] + if (project.sourceSets.findByName("main") && project.sourceSets.main.output.classesDir.exists()) { + classDirectories += [project.sourceSets.main.output.classesDir] + dependsOn project.tasks.classes + } + if (project.sourceSets.findByName("test") && project.sourceSets.test.output.classesDir.exists()) { + classDirectories += [project.sourceSets.test.output.classesDir] + dependsOn project.tasks.testClasses + } + } + doFirst({ + args('-cp', getClasspath().asPath, 'org.elasticsearch.test.loggerusage.ESLoggerUsageChecker') + getClassDirectories().each { + args it.getAbsolutePath() + } + }) + doLast({ + successMarker.parentFile.mkdirs() + successMarker.setText("", 'UTF-8') + }) + } + } + + @InputFiles + FileCollection getClasspath() { + return classpath + } + + void setClasspath(FileCollection classpath) { + this.classpath = classpath + } + + @InputFiles + List getClassDirectories() { + return classDirectories + } + + void setClassDirectories(List classDirectories) { + this.classDirectories = classDirectories + } + + @OutputFile + File getSuccessMarker() { + return successMarker + } + + void setSuccessMarker(File successMarker) { + this.successMarker = successMarker + } +} diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy index ab524351274..cbd72f2c7da 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/PrecommitTasks.groovy @@ -34,6 +34,7 @@ class PrecommitTasks { configureForbiddenApis(project), configureCheckstyle(project), configureNamingConventions(project), + configureLoggerUsage(project), project.tasks.create('forbiddenPatterns', ForbiddenPatternsTask.class), project.tasks.create('licenseHeaders', LicenseHeadersTask.class), project.tasks.create('jarHell', JarHellTask.class), @@ -64,20 +65,21 @@ class PrecommitTasks { internalRuntimeForbidden = true failOnUnsupportedJava = false bundledSignatures = ['jdk-unsafe', 'jdk-deprecated'] - signaturesURLs = [getClass().getResource('/forbidden/all-signatures.txt')] + signaturesURLs = [getClass().getResource('/forbidden/jdk-signatures.txt'), + getClass().getResource('/forbidden/es-all-signatures.txt')] suppressAnnotations = ['**.SuppressForbidden'] } Task mainForbidden = project.tasks.findByName('forbiddenApisMain') if (mainForbidden != null) { mainForbidden.configure { bundledSignatures += 'jdk-system-out' - signaturesURLs += getClass().getResource('/forbidden/core-signatures.txt') + signaturesURLs += getClass().getResource('/forbidden/es-core-signatures.txt') } } Task testForbidden = project.tasks.findByName('forbiddenApisTest') if (testForbidden != null) { testForbidden.configure { - signaturesURLs += getClass().getResource('/forbidden/test-signatures.txt') + signaturesURLs += getClass().getResource('/forbidden/es-test-signatures.txt') } } Task forbiddenApis = project.tasks.findByName('forbiddenApis') @@ -117,4 +119,18 @@ class PrecommitTasks { } return null } + + private static Task configureLoggerUsage(Project project) { + Task loggerUsageTask = project.tasks.create('loggerUsageCheck', LoggerUsageTask.class) + + project.configurations.create('loggerUsagePlugin') + project.dependencies.add('loggerUsagePlugin', + "org.elasticsearch.test:logger-usage:${org.elasticsearch.gradle.VersionProperties.elasticsearch}") + + loggerUsageTask.configure { + classpath = project.configurations.loggerUsagePlugin + } + + return loggerUsageTask + } } diff --git a/buildSrc/src/main/resources/forbidden/es-all-signatures.txt b/buildSrc/src/main/resources/forbidden/es-all-signatures.txt new file mode 100644 index 00000000000..d258c098911 --- /dev/null +++ b/buildSrc/src/main/resources/forbidden/es-all-signatures.txt @@ -0,0 +1,30 @@ +# Licensed to Elasticsearch under one or more contributor +# license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright +# ownership. Elasticsearch licenses this file to you under +# the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on +# an 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific +# language governing permissions and limitations under the License. + +java.nio.file.Paths @ Use org.elasticsearch.common.io.PathUtils.get() instead. +java.nio.file.FileSystems#getDefault() @ use org.elasticsearch.common.io.PathUtils.getDefaultFileSystem() instead. + +java.nio.file.Files#getFileStore(java.nio.file.Path) @ Use org.elasticsearch.env.Environment.getFileStore() instead, impacted by JDK-8034057 +java.nio.file.Files#isWritable(java.nio.file.Path) @ Use org.elasticsearch.env.Environment.isWritable() instead, impacted by JDK-8034057 + +@defaultMessage Use org.elasticsearch.common.Randomness#get for reproducible sources of randomness +java.util.Random#() +java.util.concurrent.ThreadLocalRandom + +java.security.MessageDigest#clone() @ use org.elasticsearch.common.hash.MessageDigests + +@defaultMessage this should not have been added to lucene in the first place +org.apache.lucene.index.IndexReader#getCombinedCoreAndDeletesKey() diff --git a/buildSrc/src/main/resources/forbidden/core-signatures.txt b/buildSrc/src/main/resources/forbidden/es-core-signatures.txt similarity index 100% rename from buildSrc/src/main/resources/forbidden/core-signatures.txt rename to buildSrc/src/main/resources/forbidden/es-core-signatures.txt diff --git a/buildSrc/src/main/resources/forbidden/test-signatures.txt b/buildSrc/src/main/resources/forbidden/es-test-signatures.txt similarity index 100% rename from buildSrc/src/main/resources/forbidden/test-signatures.txt rename to buildSrc/src/main/resources/forbidden/es-test-signatures.txt diff --git a/buildSrc/src/main/resources/forbidden/all-signatures.txt b/buildSrc/src/main/resources/forbidden/jdk-signatures.txt similarity index 85% rename from buildSrc/src/main/resources/forbidden/all-signatures.txt rename to buildSrc/src/main/resources/forbidden/jdk-signatures.txt index 9bc37005514..994b1ad3a4a 100644 --- a/buildSrc/src/main/resources/forbidden/all-signatures.txt +++ b/buildSrc/src/main/resources/forbidden/jdk-signatures.txt @@ -33,9 +33,6 @@ java.util.Formatter#(java.lang.String,java.lang.String,java.util.Locale) java.io.RandomAccessFile java.nio.file.Path#toFile() -java.nio.file.Paths @ Use org.elasticsearch.common.io.PathUtils.get() instead. -java.nio.file.FileSystems#getDefault() @ use org.elasticsearch.common.io.PathUtils.getDefaultFileSystem() instead. - @defaultMessage Specify a location for the temp file/directory instead. java.nio.file.Files#createTempDirectory(java.lang.String,java.nio.file.attribute.FileAttribute[]) java.nio.file.Files#createTempFile(java.lang.String,java.lang.String,java.nio.file.attribute.FileAttribute[]) @@ -48,9 +45,6 @@ java.io.ObjectInput java.nio.file.Files#isHidden(java.nio.file.Path) @ Dependent on the operating system, use FileSystemUtils.isHidden instead -java.nio.file.Files#getFileStore(java.nio.file.Path) @ Use org.elasticsearch.env.Environment.getFileStore() instead, impacted by JDK-8034057 -java.nio.file.Files#isWritable(java.nio.file.Path) @ Use org.elasticsearch.env.Environment.isWritable() instead, impacted by JDK-8034057 - @defaultMessage Resolve hosts explicitly to the address(es) you want with InetAddress. java.net.InetSocketAddress#(java.lang.String,int) java.net.Socket#(java.lang.String,int) @@ -89,9 +83,6 @@ java.lang.Class#getDeclaredMethods() @ Do not violate java's access system: Use java.lang.reflect.AccessibleObject#setAccessible(boolean) java.lang.reflect.AccessibleObject#setAccessible(java.lang.reflect.AccessibleObject[], boolean) -@defaultMessage this should not have been added to lucene in the first place -org.apache.lucene.index.IndexReader#getCombinedCoreAndDeletesKey() - @defaultMessage this method needs special permission java.lang.Thread#getAllStackTraces() @@ -112,8 +103,3 @@ java.util.Collections#EMPTY_MAP java.util.Collections#EMPTY_SET java.util.Collections#shuffle(java.util.List) @ Use java.util.Collections#shuffle(java.util.List, java.util.Random) with a reproducible source of randomness -@defaultMessage Use org.elasticsearch.common.Randomness#get for reproducible sources of randomness -java.util.Random#() -java.util.concurrent.ThreadLocalRandom - -java.security.MessageDigest#clone() @ use org.elasticsearch.common.hash.MessageDigests diff --git a/core/src/main/java/org/elasticsearch/common/SuppressLoggerChecks.java b/core/src/main/java/org/elasticsearch/common/SuppressLoggerChecks.java new file mode 100644 index 00000000000..c6f23f72f96 --- /dev/null +++ b/core/src/main/java/org/elasticsearch/common/SuppressLoggerChecks.java @@ -0,0 +1,33 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.common; + + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; +/** + * Annotation to suppress logging usage checks errors inside a whole class or a method. + */ +@Retention(RetentionPolicy.CLASS) +@Target({ ElementType.CONSTRUCTOR, ElementType.METHOD, ElementType.TYPE }) +public @interface SuppressLoggerChecks { + String reason(); +} diff --git a/settings.gradle b/settings.gradle index f2518e69b12..b1bb374fff1 100644 --- a/settings.gradle +++ b/settings.gradle @@ -11,6 +11,7 @@ List projects = [ 'test:framework', 'test:fixtures:example-fixture', 'test:fixtures:hdfs-fixture', + 'test:logger-usage', 'modules:ingest-grok', 'modules:lang-expression', 'modules:lang-groovy', diff --git a/test/build.gradle b/test/build.gradle index 7e1b5725147..fcf4f5bb761 100644 --- a/test/build.gradle +++ b/test/build.gradle @@ -30,8 +30,9 @@ subprojects { // the main files are actually test files, so use the appropriate forbidden api sigs forbiddenApisMain { bundledSignatures = ['jdk-unsafe', 'jdk-deprecated'] - signaturesURLs = [PrecommitTasks.getResource('/forbidden/all-signatures.txt'), - PrecommitTasks.getResource('/forbidden/test-signatures.txt')] + signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt'), + PrecommitTasks.getResource('/forbidden/es-signatures.txt'), + PrecommitTasks.getResource('/forbidden/es-test-signatures.txt')] } // TODO: should we have licenses for our test deps? diff --git a/test/logger-usage/build.gradle b/test/logger-usage/build.gradle new file mode 100644 index 00000000000..1a5815cf76e --- /dev/null +++ b/test/logger-usage/build.gradle @@ -0,0 +1,33 @@ +import org.elasticsearch.gradle.precommit.PrecommitTasks + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +dependencies { + compile 'org.ow2.asm:asm-debug-all:5.0.4' // use asm-debug-all as asm-all is broken + testCompile "org.elasticsearch.test:framework:${version}" +} + +loggerUsageCheck.enabled = false + +forbiddenApisMain.enabled = true // disabled by parent project +forbiddenApisMain { + signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt')] // does not depend on core, only jdk signatures +} +jarHell.enabled = true // disabled by parent project \ No newline at end of file diff --git a/test/logger-usage/src/main/java/org/elasticsearch/test/loggerusage/ESLoggerUsageChecker.java b/test/logger-usage/src/main/java/org/elasticsearch/test/loggerusage/ESLoggerUsageChecker.java new file mode 100644 index 00000000000..57ec37cb695 --- /dev/null +++ b/test/logger-usage/src/main/java/org/elasticsearch/test/loggerusage/ESLoggerUsageChecker.java @@ -0,0 +1,460 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.test.loggerusage; + +import org.objectweb.asm.AnnotationVisitor; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.IntInsnNode; +import org.objectweb.asm.tree.LdcInsnNode; +import org.objectweb.asm.tree.LineNumberNode; +import org.objectweb.asm.tree.MethodInsnNode; +import org.objectweb.asm.tree.MethodNode; +import org.objectweb.asm.tree.TypeInsnNode; +import org.objectweb.asm.tree.analysis.Analyzer; +import org.objectweb.asm.tree.analysis.AnalyzerException; +import org.objectweb.asm.tree.analysis.BasicInterpreter; +import org.objectweb.asm.tree.analysis.BasicValue; +import org.objectweb.asm.tree.analysis.Frame; + +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.Arrays; +import java.util.List; +import java.util.function.Consumer; +import java.util.function.Predicate; + +public class ESLoggerUsageChecker { + public static final String LOGGER_CLASS = "org.elasticsearch.common.logging.ESLogger"; + public static final String THROWABLE_CLASS = "java.lang.Throwable"; + public static final List LOGGER_METHODS = Arrays.asList("trace", "debug", "info", "warn", "error"); + public static final String IGNORE_CHECKS_ANNOTATION = "org.elasticsearch.common.SuppressLoggerChecks"; + + public static void main(String... args) throws Exception { + System.out.println("checking for wrong usages of ESLogger..."); + boolean[] wrongUsageFound = new boolean[1]; + checkLoggerUsage(wrongLoggerUsage -> { + System.err.println(wrongLoggerUsage.getErrorLines()); + wrongUsageFound[0] = true; + }, args); + if (wrongUsageFound[0]) { + throw new Exception("Wrong logger usages found"); + } else { + System.out.println("No wrong usages found"); + } + } + + private static void checkLoggerUsage(Consumer wrongUsageCallback, String... classDirectories) + throws IOException { + for (String classDirectory : classDirectories) { + Path root = Paths.get(classDirectory); + if (Files.isDirectory(root) == false) { + throw new IllegalArgumentException(root + " should be an existing directory"); + } + Files.walkFileTree(root, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + if (Files.isRegularFile(file) && file.endsWith(".class")) { + try (InputStream in = Files.newInputStream(file)) { + ESLoggerUsageChecker.check(wrongUsageCallback, in); + } + } + return super.visitFile(file, attrs); + } + }); + } + } + + public static void check(Consumer wrongUsageCallback, InputStream inputStream) throws IOException { + check(wrongUsageCallback, inputStream, s -> true); + } + + // used by tests + static void check(Consumer wrongUsageCallback, InputStream inputStream, Predicate methodsToCheck) + throws IOException { + ClassReader cr = new ClassReader(inputStream); + cr.accept(new ClassChecker(wrongUsageCallback, methodsToCheck), 0); + } + + public static class WrongLoggerUsage { + private final String className; + private final String methodName; + private final String logMethodName; + private final int line; + private final String errorMessage; + + public WrongLoggerUsage(String className, String methodName, String logMethodName, int line, String errorMessage) { + this.className = className; + this.methodName = methodName; + this.logMethodName = logMethodName; + this.line = line; + this.errorMessage = errorMessage; + } + + @Override + public String toString() { + return "WrongLoggerUsage{" + + "className='" + className + '\'' + + ", methodName='" + methodName + '\'' + + ", logMethodName='" + logMethodName + '\'' + + ", line=" + line + + ", errorMessage='" + errorMessage + '\'' + + '}'; + } + + /** + * Returns an error message that has the form of stack traces emitted by {@link Throwable#printStackTrace} + */ + public String getErrorLines() { + String fullClassName = Type.getObjectType(className).getClassName(); + String simpleClassName = fullClassName.substring(fullClassName.lastIndexOf(".") + 1, fullClassName.length()); + int innerClassIndex = simpleClassName.indexOf("$"); + if (innerClassIndex > 0) { + simpleClassName = simpleClassName.substring(0, innerClassIndex); + } + simpleClassName = simpleClassName + ".java"; + StringBuilder sb = new StringBuilder(); + sb.append("Bad usage of "); + sb.append(LOGGER_CLASS).append("#").append(logMethodName); + sb.append(": "); + sb.append(errorMessage); + sb.append("\n\tat "); + sb.append(fullClassName); + sb.append("."); + sb.append(methodName); + sb.append("("); + sb.append(simpleClassName); + sb.append(":"); + sb.append(line); + sb.append(")"); + return sb.toString(); + } + } + + private static class ClassChecker extends ClassVisitor { + private String className; + private boolean ignoreChecks; + private final Consumer wrongUsageCallback; + private final Predicate methodsToCheck; + + public ClassChecker(Consumer wrongUsageCallback, Predicate methodsToCheck) { + super(Opcodes.ASM5); + this.wrongUsageCallback = wrongUsageCallback; + this.methodsToCheck = methodsToCheck; + } + + @Override + public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { + this.className = name; + } + + @Override + public AnnotationVisitor visitAnnotation(String desc, boolean visible) { + if (IGNORE_CHECKS_ANNOTATION.equals(Type.getType(desc).getClassName())) { + ignoreChecks = true; + } + return super.visitAnnotation(desc, visible); + } + + @Override + public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { + if (ignoreChecks == false && methodsToCheck.test(name)) { + return new MethodChecker(this.className, access, name, desc, wrongUsageCallback); + } else { + return super.visitMethod(access, name, desc, signature, exceptions); + } + } + } + + private static class MethodChecker extends MethodVisitor { + private final String className; + private final Consumer wrongUsageCallback; + private boolean ignoreChecks; + + public MethodChecker(String className, int access, String name, String desc, Consumer wrongUsageCallback) { + super(Opcodes.ASM5, new MethodNode(access, name, desc, null, null)); + this.className = className; + this.wrongUsageCallback = wrongUsageCallback; + } + + @Override + public AnnotationVisitor visitAnnotation(String desc, boolean visible) { + if (IGNORE_CHECKS_ANNOTATION.equals(Type.getType(desc).getClassName())) { + ignoreChecks = true; + } + return super.visitAnnotation(desc, visible); + } + + @Override + public void visitEnd() { + if (ignoreChecks == false) { + findBadLoggerUsages((MethodNode) mv); + } + super.visitEnd(); + } + + public void findBadLoggerUsages(MethodNode methodNode) { + Analyzer stringPlaceHolderAnalyzer = new Analyzer<>(new PlaceHolderStringInterpreter()); + Analyzer arraySizeAnalyzer = new Analyzer<>(new ArraySizeInterpreter()); + try { + stringPlaceHolderAnalyzer.analyze(className, methodNode); + arraySizeAnalyzer.analyze(className, methodNode); + } catch (AnalyzerException e) { + throw new RuntimeException("Internal error: failed in analysis step", e); + } + Frame[] stringFrames = stringPlaceHolderAnalyzer.getFrames(); + Frame[] arraySizeFrames = arraySizeAnalyzer.getFrames(); + AbstractInsnNode[] insns = methodNode.instructions.toArray(); + int lineNumber = -1; + for (int i = 0; i < insns.length; i++) { + AbstractInsnNode insn = insns[i]; + if (insn instanceof LineNumberNode) { + LineNumberNode lineNumberNode = (LineNumberNode) insn; + lineNumber = lineNumberNode.line; + } + if (insn.getOpcode() == Opcodes.INVOKEVIRTUAL) { + MethodInsnNode methodInsn = (MethodInsnNode) insn; + if (Type.getObjectType(methodInsn.owner).getClassName().equals(LOGGER_CLASS) == false) { + continue; + } + if (LOGGER_METHODS.contains(methodInsn.name) == false) { + continue; + } + Type[] argumentTypes = Type.getArgumentTypes(methodInsn.desc); + BasicValue logMessageLengthObject = getStackValue(stringFrames[i], argumentTypes.length - 1); // first argument + if (logMessageLengthObject instanceof PlaceHolderStringBasicValue == false) { + wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, + "First argument must be a string constant so that we can statically ensure proper place holder usage")); + continue; + } + PlaceHolderStringBasicValue logMessageLength = (PlaceHolderStringBasicValue) logMessageLengthObject; + if (logMessageLength.minValue != logMessageLength.maxValue) { + wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, + "Multiple log messages with conflicting number of place holders")); + continue; + } + BasicValue varArgsSizeObject = getStackValue(arraySizeFrames[i], 0); // last argument + if (varArgsSizeObject instanceof ArraySizeBasicValue == false) { + wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, + "Could not determine size of varargs array")); + continue; + } + ArraySizeBasicValue varArgsSize = (ArraySizeBasicValue) varArgsSizeObject; + if (varArgsSize.minValue != varArgsSize.maxValue) { + wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, + "Multiple parameter arrays with conflicting sizes")); + continue; + } + assert logMessageLength.minValue == logMessageLength.maxValue && varArgsSize.minValue == varArgsSize.maxValue; + if (logMessageLength.minValue != varArgsSize.minValue) { + wrongUsageCallback.accept(new WrongLoggerUsage(className, methodNode.name, methodInsn.name, lineNumber, + "Expected " + logMessageLength.minValue + " arguments but got " + varArgsSize.minValue)); + continue; + } + } + } + } + } + + private static int calculateNumberOfPlaceHolders(String message) { + int count = 0; + for (int i = 1; i < message.length(); i++) { + if (message.charAt(i - 1) == '{' && message.charAt(i) == '}') { + count++; + i += 1; + } + } + return count; + } + + private static BasicValue getStackValue(Frame f, int index) { + int top = f.getStackSize() - 1; + return index <= top ? f.getStack(top - index) : null; + } + + private static class IntMinMaxTrackingBasicValue extends BasicValue { + protected final int minValue; + protected final int maxValue; + + public IntMinMaxTrackingBasicValue(Type type, int value) { + super(type); + this.minValue = value; + this.maxValue = value; + } + + public IntMinMaxTrackingBasicValue(Type type, int minValue, int maxValue) { + super(type); + this.minValue = minValue; + this.maxValue = maxValue; + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + if (!super.equals(o)) return false; + + IntMinMaxTrackingBasicValue that = (IntMinMaxTrackingBasicValue) o; + + if (minValue != that.minValue) return false; + return maxValue == that.maxValue; + + } + + @Override + public int hashCode() { + int result = super.hashCode(); + result = 31 * result + minValue; + result = 31 * result + maxValue; + return result; + } + + @Override + public String toString() { + return "IntMinMaxTrackingBasicValue{" + + "minValue=" + minValue + + ", maxValue=" + maxValue + + '}'; + } + } + + private static final class PlaceHolderStringBasicValue extends IntMinMaxTrackingBasicValue { + public static final Type STRING_OBJECT_TYPE = Type.getObjectType("java/lang/String"); + + public PlaceHolderStringBasicValue(int placeHolders) { + super(STRING_OBJECT_TYPE, placeHolders); + } + + public PlaceHolderStringBasicValue(int minPlaceHolders, int maxPlaceHolders) { + super(STRING_OBJECT_TYPE, minPlaceHolders, maxPlaceHolders); + } + } + + private static final class ArraySizeBasicValue extends IntMinMaxTrackingBasicValue { + public ArraySizeBasicValue(Type type, int minArraySize, int maxArraySize) { + super(type, minArraySize, maxArraySize); + } + } + + private static final class IntegerConstantBasicValue extends IntMinMaxTrackingBasicValue { + public IntegerConstantBasicValue(Type type, int constant) { + super(type, constant); + } + + public IntegerConstantBasicValue(Type type, int minConstant, int maxConstant) { + super(type, minConstant, maxConstant); + } + } + + private static final class PlaceHolderStringInterpreter extends BasicInterpreter { + @Override + public BasicValue newOperation(AbstractInsnNode insnNode) throws AnalyzerException { + if (insnNode.getOpcode() == Opcodes.LDC) { + Object constant = ((LdcInsnNode) insnNode).cst; + if (constant instanceof String) { + return new PlaceHolderStringBasicValue(calculateNumberOfPlaceHolders((String) constant)); + } + } + return super.newOperation(insnNode); + } + + @Override + public BasicValue merge(BasicValue value1, BasicValue value2) { + if (value1 instanceof PlaceHolderStringBasicValue && value2 instanceof PlaceHolderStringBasicValue + && value1.equals(value2) == false) { + PlaceHolderStringBasicValue c1 = (PlaceHolderStringBasicValue) value1; + PlaceHolderStringBasicValue c2 = (PlaceHolderStringBasicValue) value2; + return new PlaceHolderStringBasicValue(Math.min(c1.minValue, c2.minValue), Math.max(c1.maxValue, c2.maxValue)); + } + return super.merge(value1, value2); + } + } + + private static final class ArraySizeInterpreter extends BasicInterpreter { + @Override + public BasicValue newOperation(AbstractInsnNode insnNode) throws AnalyzerException { + switch (insnNode.getOpcode()) { + case ICONST_0: return new IntegerConstantBasicValue(Type.INT_TYPE, 0); + case ICONST_1: return new IntegerConstantBasicValue(Type.INT_TYPE, 1); + case ICONST_2: return new IntegerConstantBasicValue(Type.INT_TYPE, 2); + case ICONST_3: return new IntegerConstantBasicValue(Type.INT_TYPE, 3); + case ICONST_4: return new IntegerConstantBasicValue(Type.INT_TYPE, 4); + case ICONST_5: return new IntegerConstantBasicValue(Type.INT_TYPE, 5); + case BIPUSH: + case SIPUSH: return new IntegerConstantBasicValue(Type.INT_TYPE, ((IntInsnNode)insnNode).operand); + case Opcodes.LDC: { + Object constant = ((LdcInsnNode)insnNode).cst; + if (constant instanceof Integer) { + return new IntegerConstantBasicValue(Type.INT_TYPE, (Integer)constant); + } else { + return super.newOperation(insnNode); + } + } + default: return super.newOperation(insnNode); + } + } + + @Override + public BasicValue merge(BasicValue value1, BasicValue value2) { + if (value1 instanceof IntegerConstantBasicValue && value2 instanceof IntegerConstantBasicValue) { + IntegerConstantBasicValue c1 = (IntegerConstantBasicValue) value1; + IntegerConstantBasicValue c2 = (IntegerConstantBasicValue) value2; + return new IntegerConstantBasicValue(Type.INT_TYPE, Math.min(c1.minValue, c2.minValue), Math.max(c1.maxValue, c2.maxValue)); + } else if (value1 instanceof ArraySizeBasicValue && value2 instanceof ArraySizeBasicValue) { + ArraySizeBasicValue c1 = (ArraySizeBasicValue) value1; + ArraySizeBasicValue c2 = (ArraySizeBasicValue) value2; + return new ArraySizeBasicValue(Type.INT_TYPE, Math.min(c1.minValue, c2.minValue), Math.max(c1.maxValue, c2.maxValue)); + } + return super.merge(value1, value2); + } + + @Override + public BasicValue unaryOperation(AbstractInsnNode insnNode, BasicValue value) throws AnalyzerException { + if (insnNode.getOpcode() == Opcodes.ANEWARRAY && value instanceof IntegerConstantBasicValue) { + IntegerConstantBasicValue constantBasicValue = (IntegerConstantBasicValue) value; + String desc = ((TypeInsnNode) insnNode).desc; + return new ArraySizeBasicValue(Type.getType("[" + Type.getObjectType(desc)), constantBasicValue.minValue, + constantBasicValue.maxValue); + } + return super.unaryOperation(insnNode, value); + } + + @Override + public BasicValue ternaryOperation(AbstractInsnNode insnNode, BasicValue value1, BasicValue value2, BasicValue value3) + throws AnalyzerException { + if (insnNode.getOpcode() == Opcodes.AASTORE && value1 instanceof ArraySizeBasicValue) { + return value1; + } + return super.ternaryOperation(insnNode, value1, value2, value3); + } + } +} diff --git a/test/logger-usage/src/test/java/org/elasticsearch/test/loggerusage/ESLoggerUsageTests.java b/test/logger-usage/src/test/java/org/elasticsearch/test/loggerusage/ESLoggerUsageTests.java new file mode 100644 index 00000000000..ab07ecbf45e --- /dev/null +++ b/test/logger-usage/src/test/java/org/elasticsearch/test/loggerusage/ESLoggerUsageTests.java @@ -0,0 +1,165 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.test.loggerusage; + +import org.elasticsearch.common.SuppressLoggerChecks; +import org.elasticsearch.common.logging.ESLogger; +import org.elasticsearch.test.loggerusage.ESLoggerUsageChecker.WrongLoggerUsage; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.io.InputStream; +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Predicate; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.notNullValue; + +public class ESLoggerUsageTests extends ESTestCase { + + public void testLoggerUsageChecks() throws IOException { + for (Method method : getClass().getMethods()) { + if (method.getDeclaringClass().equals(getClass())) { + if (method.getName().startsWith("check")) { + logger.info("Checking logger usage for method {}", method.getName()); + InputStream classInputStream = getClass().getResourceAsStream(getClass().getSimpleName() + ".class"); + List errors = new ArrayList<>(); + ESLoggerUsageChecker.check(errors::add, classInputStream, Predicate.isEqual(method.getName())); + if (method.getName().startsWith("checkFail")) { + assertFalse("Expected " + method.getName() + " to have wrong ESLogger usage", errors.isEmpty()); + } else { + assertTrue("Method " + method.getName() + " has unexpected ESLogger usage errors: " + errors, errors.isEmpty()); + } + } else { + assertTrue("only allow methods starting with test or check in this class", method.getName().startsWith("test")); + } + } + } + } + + public void testLoggerUsageCheckerCompatibilityWithESLogger() throws NoSuchMethodException { + assertThat(ESLoggerUsageChecker.LOGGER_CLASS, equalTo(ESLogger.class.getName())); + assertThat(ESLoggerUsageChecker.THROWABLE_CLASS, equalTo(Throwable.class.getName())); + int varargsMethodCount = 0; + for (Method method : ESLogger.class.getMethods()) { + if (method.isVarArgs()) { + // check that logger usage checks all varargs methods + assertThat(ESLoggerUsageChecker.LOGGER_METHODS, hasItem(method.getName())); + varargsMethodCount++; + } + } + // currently we have two overloaded methods for each of debug, info, ... + // if that changes, we might want to have another look at the usage checker + assertThat(varargsMethodCount, equalTo(ESLoggerUsageChecker.LOGGER_METHODS.size() * 2)); + + // check that signature is same as we expect in the usage checker + for (String methodName : ESLoggerUsageChecker.LOGGER_METHODS) { + assertThat(ESLogger.class.getMethod(methodName, String.class, Object[].class), notNullValue()); + assertThat(ESLogger.class.getMethod(methodName, String.class, Throwable.class, Object[].class), notNullValue()); + } + } + + public void checkNumberOfArguments1() { + logger.info("Hello {}", "world"); + } + + public void checkFailNumberOfArguments1() { + logger.info("Hello {}"); + } + + @SuppressLoggerChecks(reason = "test ignore functionality") + public void checkIgnoreWhenAnnotationPresent() { + logger.info("Hello {}"); + } + + public void checkNumberOfArguments2() { + logger.info("Hello {}, {}, {}", "world", 2, "third argument"); + } + + public void checkFailNumberOfArguments2() { + logger.info("Hello {}, {}", "world", 2, "third argument"); + } + + public void checkNumberOfArguments3() { + // long argument list (> 5), emits different bytecode + logger.info("Hello {}, {}, {}, {}, {}, {}, {}", "world", 2, "third argument", 4, 5, 6, new String("last arg")); + } + + public void checkFailNumberOfArguments3() { + logger.info("Hello {}, {}, {}, {}, {}, {}, {}", "world", 2, "third argument", 4, 5, 6, 7, new String("last arg")); + } + + public void checkOrderOfExceptionArgument() { + logger.info("Hello", new Exception()); + } + + public void checkOrderOfExceptionArgument1() { + logger.info("Hello {}", new Exception(), "world"); + } + + public void checkFailOrderOfExceptionArgument1() { + logger.info("Hello {}", "world", new Exception()); + } + + public void checkOrderOfExceptionArgument2() { + logger.info("Hello {}, {}", new Exception(), "world", 42); + } + + public void checkFailOrderOfExceptionArgument2() { + logger.info("Hello {}, {}", "world", 42, new Exception()); + } + + public void checkFailNonConstantMessage(boolean b) { + logger.info(Boolean.toString(b)); + } + + public void checkComplexUsage(boolean b) { + String message = "Hello {}, {}"; + Object[] args = new Object[] { "world", 42 }; + if (b) { + message = "also two args {}{}"; + args = new Object[] { "world", 43 }; + } + logger.info(message, args); + } + + public void checkFailComplexUsage1(boolean b) { + String message = "Hello {}, {}"; + Object[] args = new Object[] { "world", 42 }; + if (b) { + message = "just one arg {}"; + args = new Object[] { "world", 43 }; + } + logger.info(message, args); + } + + public void checkFailComplexUsage2(boolean b) { + String message = "Hello {}, {}"; + Object[] args = new Object[] { "world", 42 }; + if (b) { + message = "also two args {}{}"; + args = new Object[] { "world", 43, "another argument" }; + } + logger.info(message, args); + } +}