From dad2baf541db391c50d3fcf354c1d0a1b9eb084e Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Mon, 3 Dec 2018 22:17:49 -0600 Subject: [PATCH 01/13] converting ForbiddenPatternsTask w java impl & unit tests --- buildSrc/build.gradle | 2 +- .../precommit/ForbiddenPatternsTask.groovy | 130 -------------- .../precommit/ForbiddenPatternsTask.java | 165 ++++++++++++++++++ .../precommit/ForbiddenPatternsTaskTests.java | 127 ++++++++++++++ 4 files changed, 293 insertions(+), 131 deletions(-) delete mode 100644 buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.groovy create mode 100644 buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java create mode 100644 buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java diff --git a/buildSrc/build.gradle b/buildSrc/build.gradle index 177a128d31e46..4da26da69a5a5 100644 --- a/buildSrc/build.gradle +++ b/buildSrc/build.gradle @@ -206,7 +206,7 @@ if (project != rootProject) { forbiddenPatterns { exclude '**/*.wav' // the file that actually defines nocommit - exclude '**/ForbiddenPatternsTask.groovy' + exclude '**/ForbiddenPatternsTask.java' } namingConventions { diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.groovy deleted file mode 100644 index e574d67f2ace1..0000000000000 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.groovy +++ /dev/null @@ -1,130 +0,0 @@ -/* - * 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.gradle.api.DefaultTask -import org.gradle.api.GradleException -import org.gradle.api.InvalidUserDataException -import org.gradle.api.file.FileCollection -import org.gradle.api.tasks.InputFiles -import org.gradle.api.tasks.OutputFile -import org.gradle.api.tasks.SourceSet -import org.gradle.api.tasks.TaskAction -import org.gradle.api.tasks.util.PatternFilterable -import org.gradle.api.tasks.util.PatternSet - -import java.util.regex.Pattern - -/** - * Checks for patterns in source files for the project which are forbidden. - */ -public class ForbiddenPatternsTask extends DefaultTask { - - /** The rules: a map from the rule name, to a rule regex pattern. */ - private Map patterns = new LinkedHashMap<>() - /** A pattern set of which files should be checked. */ - private PatternFilterable filesFilter = new PatternSet() - - @OutputFile - File outputMarker = new File(project.buildDir, "markers/forbiddenPatterns") - - public ForbiddenPatternsTask() { - description = 'Checks source files for invalid patterns like nocommits or tabs' - - // we always include all source files, and exclude what should not be checked - filesFilter.include('**') - // exclude known binary extensions - filesFilter.exclude('**/*.gz') - filesFilter.exclude('**/*.ico') - filesFilter.exclude('**/*.jar') - filesFilter.exclude('**/*.zip') - filesFilter.exclude('**/*.jks') - filesFilter.exclude('**/*.crt') - filesFilter.exclude('**/*.png') - - // add mandatory rules - patterns.put('nocommit', /nocommit|NOCOMMIT/) - patterns.put('nocommit should be all lowercase or all uppercase', - /((?i)nocommit)(? props) { - String name = props.remove('name') - if (name == null) { - throw new InvalidUserDataException('Missing [name] for invalid pattern rule') - } - String pattern = props.remove('pattern') - if (pattern == null) { - throw new InvalidUserDataException('Missing [pattern] for invalid pattern rule') - } - if (props.isEmpty() == false) { - throw new InvalidUserDataException("Unknown arguments for ForbiddenPatterns rule mapping: ${props.keySet()}") - } - // TODO: fail if pattern contains a newline, it won't work (currently) - patterns.put(name, pattern) - } - - /** Returns the files this task will check */ - @InputFiles - FileCollection files() { - List collections = new ArrayList<>() - for (SourceSet sourceSet : project.sourceSets) { - collections.add(sourceSet.allSource.matching(filesFilter)) - } - return project.files(collections.toArray()) - } - - @TaskAction - void checkInvalidPatterns() { - Pattern allPatterns = Pattern.compile('(' + patterns.values().join(')|(') + ')') - List failures = new ArrayList<>() - for (File f : files()) { - f.eachLine('UTF-8') { String line, int lineNumber -> - if (allPatterns.matcher(line).find()) { - addErrorMessages(failures, f, line, lineNumber) - } - } - } - if (failures.isEmpty() == false) { - throw new GradleException('Found invalid patterns:\n' + failures.join('\n')) - } - outputMarker.setText('done', 'UTF-8') - } - - // iterate through patterns to find the right ones for nice error messages - void addErrorMessages(List failures, File f, String line, int lineNumber) { - String path = project.getRootProject().projectDir.toURI().relativize(f.toURI()).toString() - for (Map.Entry pattern : patterns.entrySet()) { - if (Pattern.compile(pattern.value).matcher(line).find()) { - failures.add('- ' + pattern.key + ' on line ' + lineNumber + ' of ' + path) - } - } - } -} diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java new file mode 100644 index 0000000000000..e16e8e9f5dc6b --- /dev/null +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.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.gradle.precommit; + +import org.gradle.api.DefaultTask; +import org.gradle.api.GradleException; +import org.gradle.api.InvalidUserDataException; +import org.gradle.api.file.FileCollection; +import org.gradle.api.file.FileTree; +import org.gradle.api.plugins.JavaPluginConvention; +import org.gradle.api.tasks.InputFiles; +import org.gradle.api.tasks.OutputFile; +import org.gradle.api.tasks.SkipWhenEmpty; +import org.gradle.api.tasks.TaskAction; +import org.gradle.api.tasks.util.PatternFilterable; +import org.gradle.api.tasks.util.PatternSet; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.AbstractMap; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +/** + * Checks for patterns in source files for the project which are forbidden. + */ +public class ForbiddenPatternsTask extends DefaultTask { + + /* + * A pattern set of which files should be checked. + */ + private final PatternFilterable filesFilter = new PatternSet() + // we always include all source files, and exclude what should not be checked + .include("**") + // exclude known binary extensions + .exclude("**/*.gz") + .exclude("**/*.ico") + .exclude("**/*.jar") + .exclude("**/*.zip") + .exclude("**/*.jks") + .exclude("**/*.crt") + .exclude("**/*.png"); + + /* + * The rules: a map from the rule name, to a rule regex pattern. + */ + private static final Map patterns = new HashMap<>(); + static { + // add mandatory rules + patterns.put("nocommit", "nocommit|NOCOMMIT"); + patterns.put("nocommit should be all lowercase or all uppercase", "((?i)nocommit)(? sourceSet.getAllSource().matching(filesFilter)) + .reduce(FileTree::plus) + .orElse(getProject().files().getAsFileTree()); + } + + @TaskAction + public void checkInvalidPatterns() throws IOException { + Pattern allPatterns = Pattern.compile("(" + String.join(")|(", patterns.values()) + ")"); + List failures = new ArrayList<>(); + for (File f : files()) { + List lines = getLines(f.toPath()); + List invalidLines = IntStream.range(0, lines.size()) + .filter(i -> allPatterns.matcher(lines.get(i)).find()) + .boxed() + .collect(Collectors.toList()); + + String path = getProject().getRootProject().getProjectDir().toURI().relativize(f.toURI()).toString(); + failures = invalidLines.stream() + .map(l -> new AbstractMap.SimpleEntry<>(l+1, lines.get(l))) + .flatMap(kv -> patterns.entrySet().stream() + .filter(p -> Pattern.compile(p.getValue()).matcher(kv.getValue()).find()) + .map(p -> "- " + p.getKey() + " on line " + kv.getKey() + " of " + path) + ) + .collect(Collectors.toList()); + } + if (failures.isEmpty() == false) { + throw new GradleException("Found invalid patterns:\n" + String.join("\n", failures)); + } + + outputMarker.getParentFile().mkdirs(); + Files.write(outputMarker.toPath(), "done".getBytes(StandardCharsets.UTF_8)); + } + + @OutputFile + public File getOutputMarker() { + return outputMarker; + } + + public void exclude(String... excludes) { + filesFilter.exclude(excludes); + } + + public void rule(Map props) { + String name = props.remove("name"); + if (name == null) { + throw new InvalidUserDataException("Missing [name] for invalid pattern rule"); + } + String pattern = props.remove("pattern"); + if (pattern == null) { + throw new InvalidUserDataException("Missing [pattern] for invalid pattern rule"); + } + if (props.isEmpty() == false) { + throw new InvalidUserDataException("Unknown arguments for ForbiddenPatterns rule mapping: " + + props.keySet().toString()); + } + // TODO: fail if pattern contains a newline, it won't work (currently) + patterns.put(name, pattern); + } + + private List getLines(Path path) throws IOException { + for (Charset encoding : Arrays.asList(StandardCharsets.UTF_8, StandardCharsets.ISO_8859_1)) { + try (Stream stream = Files.lines(path, encoding)) { + return stream.collect(Collectors.toList()); + } catch (Exception e){ + continue; + } + } + + throw new IOException("Unable to read lines from source file: " + path); + } +} diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java new file mode 100644 index 0000000000000..7fc0b47998fed --- /dev/null +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -0,0 +1,127 @@ +package org.elasticsearch.gradle.precommit; + +import org.elasticsearch.gradle.test.GradleUnitTestCase; +import org.gradle.api.GradleException; +import org.gradle.api.Project; +import org.gradle.api.plugins.JavaPlugin; +import org.gradle.testfixtures.ProjectBuilder; +import org.junit.Rule; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Arrays; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; + +public class ForbiddenPatternsTaskTests extends GradleUnitTestCase { + @Rule + public TemporaryFolder temporaryFolder = new TemporaryFolder(); + + public void testCheckInvalidPatternsWhenNoSourceFilesExist() throws Exception { + Project project = createProject(); + ForbiddenPatternsTask task = createTask(project); + + task.checkInvalidPatterns(); + + File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); + Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); + + assertTrue(result.isPresent()); + assertEquals("done", result.get()); + } + + public void testCheckInvalidPatternsWhenSourceFilesExistNoViolation() throws Exception { + Project project = createProject(); + ForbiddenPatternsTask task = createTask(project); + + String line = "public void bar() {}"; + File file = new File(project.getProjectDir(), "src/main/java/Foo.java"); + file.getParentFile().mkdirs(); + file.createNewFile(); + Files.write(file.toPath(), line.getBytes()); + + task.checkInvalidPatterns(); + + File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); + Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); + + assertTrue(result.isPresent()); + assertEquals("done", result.get()); + } + + public void testCheckInvalidPatternsWhenSourceFilesExistHavingTab() throws Exception { + Project project = createProject(); + ForbiddenPatternsTask task = createTask(project); + + String line = "\tpublic void bar() {}"; + File file = new File(project.getProjectDir(), "src/main/java/Bar.java"); + file.getParentFile().mkdirs(); + file.createNewFile(); + Files.write(file.toPath(), line.getBytes()); + + try { + task.checkInvalidPatterns(); + fail("GradleException was expected to be thrown in this case!"); + } catch (GradleException e) { + assertTrue(e.getMessage().startsWith("Found invalid patterns")); + } + } + + public void testCheckInvalidPatternsWithCustomRule() throws Exception { + Map rule = new HashMap<>(); + rule.put("name", "TODO comments are not allowed"); + rule.put("pattern", "\\/\\/.*(?i)TODO"); + + Project project = createProject(); + ForbiddenPatternsTask task = createTask(project); + task.rule(rule); + + List lines = Arrays.asList("GOOD LINE", "//todo", "// some stuff, toDo"); + File file = new File(project.getProjectDir(), "src/main/java/Moot.java"); + file.getParentFile().mkdirs(); + file.createNewFile(); + Files.write(file.toPath(), lines, StandardCharsets.UTF_8); + + try { + task.checkInvalidPatterns(); + fail("GradleException was expected to be thrown in this case!"); + } catch (GradleException e) { + assertTrue(e.getMessage().startsWith("Found invalid patterns")); + } + } + + public void testCheckInvalidPatternsWhenExcludingFiles() throws Exception { + Project project = createProject(); + ForbiddenPatternsTask task = createTask(project); + task.exclude("**/*.java"); + + File file = new File(project.getProjectDir(), "src/main/java/FooBarMoot.java"); + file.getParentFile().mkdirs(); + file.createNewFile(); + Files.write(file.toPath(), "\t".getBytes()); + + task.checkInvalidPatterns(); + + File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); + Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); + + assertTrue(result.isPresent()); + assertEquals("done", result.get()); + } + + private Project createProject() throws IOException { + Project project = ProjectBuilder.builder().withProjectDir(temporaryFolder.newFolder()).build(); + project.getPlugins().apply(JavaPlugin.class); + + return project; + } + + private ForbiddenPatternsTask createTask(Project project) { + return project.getTasks().create("forbiddenPatterns", ForbiddenPatternsTask.class); + } +} From 3972bec641c1e1225004f84fa59960919e20006d Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 20:23:24 -0600 Subject: [PATCH 02/13] taking approach of excluding non-UTF-8 files in server project --- .../precommit/ForbiddenPatternsTask.java | 18 +----------------- server/build.gradle | 3 +++ 2 files changed, 4 insertions(+), 17 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index e16e8e9f5dc6b..1847e76324b01 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -33,20 +33,16 @@ import java.io.File; import java.io.IOException; -import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; import java.nio.file.Files; -import java.nio.file.Path; import java.util.AbstractMap; import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.IntStream; -import java.util.stream.Stream; /** * Checks for patterns in source files for the project which are forbidden. @@ -102,7 +98,7 @@ public void checkInvalidPatterns() throws IOException { Pattern allPatterns = Pattern.compile("(" + String.join(")|(", patterns.values()) + ")"); List failures = new ArrayList<>(); for (File f : files()) { - List lines = getLines(f.toPath()); + List lines = Files.lines(f.toPath(), StandardCharsets.UTF_8).collect(Collectors.toList()); List invalidLines = IntStream.range(0, lines.size()) .filter(i -> allPatterns.matcher(lines.get(i)).find()) .boxed() @@ -150,16 +146,4 @@ public void rule(Map props) { // TODO: fail if pattern contains a newline, it won't work (currently) patterns.put(name, pattern); } - - private List getLines(Path path) throws IOException { - for (Charset encoding : Arrays.asList(StandardCharsets.UTF_8, StandardCharsets.ISO_8859_1)) { - try (Stream stream = Files.lines(path, encoding)) { - return stream.collect(Collectors.toList()); - } catch (Exception e){ - continue; - } - } - - throw new IOException("Unable to read lines from source file: " + path); - } } diff --git a/server/build.gradle b/server/build.gradle index 39579bed28866..5465d9f1bb3a7 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -156,6 +156,9 @@ compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-tr forbiddenPatterns { exclude '**/*.json' exclude '**/*.jmx' + exclude '**/*.dic' + exclude '**/*.binary' + exclude '**/*.st' } task generateModulesList { From 5bc17f43aa686b3339e3042a229898374743653e Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 21:38:26 -0600 Subject: [PATCH 03/13] not needing to use temp folder for ProjectBuilder --- .../gradle/precommit/ForbiddenPatternsTaskTests.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java index 7fc0b47998fed..445bf3706e0ed 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -9,7 +9,6 @@ import org.junit.rules.TemporaryFolder; import java.io.File; -import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.Arrays; @@ -114,8 +113,8 @@ public void testCheckInvalidPatternsWhenExcludingFiles() throws Exception { assertEquals("done", result.get()); } - private Project createProject() throws IOException { - Project project = ProjectBuilder.builder().withProjectDir(temporaryFolder.newFolder()).build(); + private Project createProject() { + Project project = ProjectBuilder.builder().build(); project.getPlugins().apply(JavaPlugin.class); return project; From 881022704d80579a5f4e35faf2363d7571337601 Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 22:04:55 -0600 Subject: [PATCH 04/13] consolidating redundant test/assertions in ForbiddenPatternsTaskTest, per suggestion --- .../precommit/ForbiddenPatternsTaskTests.java | 62 +++++++++---------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java index 445bf3706e0ed..41ac9a05954d9 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -9,6 +9,7 @@ import org.junit.rules.TemporaryFolder; import java.io.File; +import java.io.IOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.Arrays; @@ -25,13 +26,7 @@ public void testCheckInvalidPatternsWhenNoSourceFilesExist() throws Exception { Project project = createProject(); ForbiddenPatternsTask task = createTask(project); - task.checkInvalidPatterns(); - - File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); - Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); - - assertTrue(result.isPresent()); - assertEquals("done", result.get()); + checkAndAssertTaskSuccessful(task); } public void testCheckInvalidPatternsWhenSourceFilesExistNoViolation() throws Exception { @@ -44,13 +39,7 @@ public void testCheckInvalidPatternsWhenSourceFilesExistNoViolation() throws Exc file.createNewFile(); Files.write(file.toPath(), line.getBytes()); - task.checkInvalidPatterns(); - - File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); - Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); - - assertTrue(result.isPresent()); - assertEquals("done", result.get()); + checkAndAssertTaskSuccessful(task); } public void testCheckInvalidPatternsWhenSourceFilesExistHavingTab() throws Exception { @@ -63,12 +52,7 @@ public void testCheckInvalidPatternsWhenSourceFilesExistHavingTab() throws Excep file.createNewFile(); Files.write(file.toPath(), line.getBytes()); - try { - task.checkInvalidPatterns(); - fail("GradleException was expected to be thrown in this case!"); - } catch (GradleException e) { - assertTrue(e.getMessage().startsWith("Found invalid patterns")); - } + checkAndAssertTaskThrowsException(task); } public void testCheckInvalidPatternsWithCustomRule() throws Exception { @@ -86,12 +70,7 @@ public void testCheckInvalidPatternsWithCustomRule() throws Exception { file.createNewFile(); Files.write(file.toPath(), lines, StandardCharsets.UTF_8); - try { - task.checkInvalidPatterns(); - fail("GradleException was expected to be thrown in this case!"); - } catch (GradleException e) { - assertTrue(e.getMessage().startsWith("Found invalid patterns")); - } + checkAndAssertTaskThrowsException(task); } public void testCheckInvalidPatternsWhenExcludingFiles() throws Exception { @@ -104,13 +83,7 @@ public void testCheckInvalidPatternsWhenExcludingFiles() throws Exception { file.createNewFile(); Files.write(file.toPath(), "\t".getBytes()); - task.checkInvalidPatterns(); - - File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); - Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); - - assertTrue(result.isPresent()); - assertEquals("done", result.get()); + checkAndAssertTaskSuccessful(task); } private Project createProject() { @@ -123,4 +96,27 @@ private Project createProject() { private ForbiddenPatternsTask createTask(Project project) { return project.getTasks().create("forbiddenPatterns", ForbiddenPatternsTask.class); } + + private void checkAndAssertTaskSuccessful(ForbiddenPatternsTask task) throws IOException { + task.checkInvalidPatterns(); + assertTaskSuccessful(task.getProject()); + } + + private void checkAndAssertTaskThrowsException(ForbiddenPatternsTask task) throws IOException { + try { + task.checkInvalidPatterns(); + fail("GradleException was expected to be thrown in this case!"); + } catch (GradleException e) { + assertTrue(e.getMessage().startsWith("Found invalid patterns")); + } + } + + private void assertTaskSuccessful(Project project) throws IOException { + File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); + assertTrue(outputMarker.exists()); + + Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); + assertTrue(result.isPresent()); + assertEquals("done", result.get()); + } } From cc654691b271bc2dd8b2e656ecdd245857ac3055 Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 22:35:25 -0600 Subject: [PATCH 05/13] adding writeSourceFile helper, per suggestion --- .../precommit/ForbiddenPatternsTaskTests.java | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java index 41ac9a05954d9..2543462afadb8 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -14,7 +14,6 @@ import java.nio.file.Files; import java.util.Arrays; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Optional; @@ -33,12 +32,7 @@ public void testCheckInvalidPatternsWhenSourceFilesExistNoViolation() throws Exc Project project = createProject(); ForbiddenPatternsTask task = createTask(project); - String line = "public void bar() {}"; - File file = new File(project.getProjectDir(), "src/main/java/Foo.java"); - file.getParentFile().mkdirs(); - file.createNewFile(); - Files.write(file.toPath(), line.getBytes()); - + writeSourceFile(project, "src/main/java/Foo.java", "public void bar() {}"); checkAndAssertTaskSuccessful(task); } @@ -46,12 +40,7 @@ public void testCheckInvalidPatternsWhenSourceFilesExistHavingTab() throws Excep Project project = createProject(); ForbiddenPatternsTask task = createTask(project); - String line = "\tpublic void bar() {}"; - File file = new File(project.getProjectDir(), "src/main/java/Bar.java"); - file.getParentFile().mkdirs(); - file.createNewFile(); - Files.write(file.toPath(), line.getBytes()); - + writeSourceFile(project, "src/main/java/Bar.java", "\tpublic void bar() {}"); checkAndAssertTaskThrowsException(task); } @@ -64,12 +53,7 @@ public void testCheckInvalidPatternsWithCustomRule() throws Exception { ForbiddenPatternsTask task = createTask(project); task.rule(rule); - List lines = Arrays.asList("GOOD LINE", "//todo", "// some stuff, toDo"); - File file = new File(project.getProjectDir(), "src/main/java/Moot.java"); - file.getParentFile().mkdirs(); - file.createNewFile(); - Files.write(file.toPath(), lines, StandardCharsets.UTF_8); - + writeSourceFile(project, "src/main/java/Moot.java", "GOOD LINE", "//todo", "// some stuff, toDo"); checkAndAssertTaskThrowsException(task); } @@ -78,11 +62,7 @@ public void testCheckInvalidPatternsWhenExcludingFiles() throws Exception { ForbiddenPatternsTask task = createTask(project); task.exclude("**/*.java"); - File file = new File(project.getProjectDir(), "src/main/java/FooBarMoot.java"); - file.getParentFile().mkdirs(); - file.createNewFile(); - Files.write(file.toPath(), "\t".getBytes()); - + writeSourceFile(project, "src/main/java/FooBarMoot.java", "\t"); checkAndAssertTaskSuccessful(task); } @@ -97,6 +77,15 @@ private ForbiddenPatternsTask createTask(Project project) { return project.getTasks().create("forbiddenPatterns", ForbiddenPatternsTask.class); } + private void writeSourceFile(Project project, String name, String... lines) throws IOException { + File file = new File(project.getProjectDir(), name); + file.getParentFile().mkdirs(); + file.createNewFile(); + + if (lines.length != 0) + Files.write(file.toPath(), Arrays.asList(lines), StandardCharsets.UTF_8); + } + private void checkAndAssertTaskSuccessful(ForbiddenPatternsTask task) throws IOException { task.checkInvalidPatterns(); assertTaskSuccessful(task.getProject()); From 5c9f93a6c350737a5ddf12392df2ce3b41240a0d Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 22:49:41 -0600 Subject: [PATCH 06/13] removing need for class-level var for outputMarker, also updating test to assert file marker name = task name --- .../gradle/precommit/ForbiddenPatternsTask.java | 5 ++--- .../gradle/precommit/ForbiddenPatternsTaskTests.java | 10 +++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index 1847e76324b01..3fd761abb1ee0 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -75,8 +75,6 @@ public class ForbiddenPatternsTask extends DefaultTask { patterns.put("tab", "\t"); } - private File outputMarker = new File(getProject().getBuildDir(), "markers/forbiddenPatterns"); - public ForbiddenPatternsTask() { setDescription("Checks source files for invalid patterns like nocommits or tabs"); getInputs().property("excludes", filesFilter.getExcludes()); @@ -117,13 +115,14 @@ public void checkInvalidPatterns() throws IOException { throw new GradleException("Found invalid patterns:\n" + String.join("\n", failures)); } + File outputMarker = getOutputMarker(); outputMarker.getParentFile().mkdirs(); Files.write(outputMarker.toPath(), "done".getBytes(StandardCharsets.UTF_8)); } @OutputFile public File getOutputMarker() { - return outputMarker; + return new File(getProject().getBuildDir(), "markers/" + getName()); } public void exclude(String... excludes) { diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java index 2543462afadb8..88d101409310b 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -77,6 +77,10 @@ private ForbiddenPatternsTask createTask(Project project) { return project.getTasks().create("forbiddenPatterns", ForbiddenPatternsTask.class); } + private ForbiddenPatternsTask createTask(Project project, String taskName) { + return project.getTasks().create(taskName, ForbiddenPatternsTask.class); + } + private void writeSourceFile(Project project, String name, String... lines) throws IOException { File file = new File(project.getProjectDir(), name); file.getParentFile().mkdirs(); @@ -88,7 +92,7 @@ private void writeSourceFile(Project project, String name, String... lines) thro private void checkAndAssertTaskSuccessful(ForbiddenPatternsTask task) throws IOException { task.checkInvalidPatterns(); - assertTaskSuccessful(task.getProject()); + assertTaskSuccessful(task.getProject(), task.getName()); } private void checkAndAssertTaskThrowsException(ForbiddenPatternsTask task) throws IOException { @@ -100,8 +104,8 @@ private void checkAndAssertTaskThrowsException(ForbiddenPatternsTask task) throw } } - private void assertTaskSuccessful(Project project) throws IOException { - File outputMarker = new File(project.getBuildDir(), "markers/forbiddenPatterns"); + private void assertTaskSuccessful(Project project, String fileName) throws IOException { + File outputMarker = new File(project.getBuildDir(), "markers/" + fileName); assertTrue(outputMarker.exists()); Optional result = Files.readAllLines(outputMarker.toPath(), StandardCharsets.UTF_8).stream().findFirst(); From c665e4aad9d9ee98fc7a62136958addbf09da27c Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Wed, 5 Dec 2018 22:59:27 -0600 Subject: [PATCH 07/13] patterns no longer static --- .../gradle/precommit/ForbiddenPatternsTask.java | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index 3fd761abb1ee0..626f66bd5a765 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -67,18 +67,17 @@ public class ForbiddenPatternsTask extends DefaultTask { /* * The rules: a map from the rule name, to a rule regex pattern. */ - private static final Map patterns = new HashMap<>(); - static { - // add mandatory rules - patterns.put("nocommit", "nocommit|NOCOMMIT"); - patterns.put("nocommit should be all lowercase or all uppercase", "((?i)nocommit)(? patterns = new HashMap<>(); public ForbiddenPatternsTask() { setDescription("Checks source files for invalid patterns like nocommits or tabs"); getInputs().property("excludes", filesFilter.getExcludes()); getInputs().property("rules", patterns); + + // add mandatory rules + patterns.put("nocommit", "nocommit|NOCOMMIT"); + patterns.put("nocommit should be all lowercase or all uppercase", "((?i)nocommit)(? Date: Wed, 5 Dec 2018 23:02:50 -0600 Subject: [PATCH 08/13] adding @Input-annotated getter for patterns var --- .../gradle/precommit/ForbiddenPatternsTask.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index 626f66bd5a765..f723455a70441 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -24,6 +24,7 @@ import org.gradle.api.file.FileCollection; import org.gradle.api.file.FileTree; import org.gradle.api.plugins.JavaPluginConvention; +import org.gradle.api.tasks.Input; import org.gradle.api.tasks.InputFiles; import org.gradle.api.tasks.OutputFile; import org.gradle.api.tasks.SkipWhenEmpty; @@ -92,7 +93,7 @@ public FileCollection files() { @TaskAction public void checkInvalidPatterns() throws IOException { - Pattern allPatterns = Pattern.compile("(" + String.join(")|(", patterns.values()) + ")"); + Pattern allPatterns = Pattern.compile("(" + String.join(")|(", getPatterns().values()) + ")"); List failures = new ArrayList<>(); for (File f : files()) { List lines = Files.lines(f.toPath(), StandardCharsets.UTF_8).collect(Collectors.toList()); @@ -124,6 +125,11 @@ public File getOutputMarker() { return new File(getProject().getBuildDir(), "markers/" + getName()); } + @Input + public Map getPatterns() { + return patterns; + } + public void exclude(String... excludes) { filesFilter.exclude(excludes); } From c42b799071fccf766acce4a672e641b84f932cf8 Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Thu, 6 Dec 2018 21:51:19 -0600 Subject: [PATCH 09/13] unused TemporaryFolder --- .../gradle/precommit/ForbiddenPatternsTaskTests.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java index 88d101409310b..7d61cb46e77ea 100644 --- a/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java +++ b/buildSrc/src/test/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTaskTests.java @@ -5,8 +5,6 @@ import org.gradle.api.Project; import org.gradle.api.plugins.JavaPlugin; import org.gradle.testfixtures.ProjectBuilder; -import org.junit.Rule; -import org.junit.rules.TemporaryFolder; import java.io.File; import java.io.IOException; @@ -18,8 +16,6 @@ import java.util.Optional; public class ForbiddenPatternsTaskTests extends GradleUnitTestCase { - @Rule - public TemporaryFolder temporaryFolder = new TemporaryFolder(); public void testCheckInvalidPatternsWhenNoSourceFilesExist() throws Exception { Project project = createProject(); From d1c3582334178063a26dc78a8db5377e109fc9d9 Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Fri, 7 Dec 2018 06:30:06 -0600 Subject: [PATCH 10/13] unmodifiable map --- .../elasticsearch/gradle/precommit/ForbiddenPatternsTask.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index f723455a70441..8bd797a5f4ae9 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -38,6 +38,7 @@ import java.nio.file.Files; import java.util.AbstractMap; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -127,7 +128,7 @@ public File getOutputMarker() { @Input public Map getPatterns() { - return patterns; + return Collections.unmodifiableMap(patterns); } public void exclude(String... excludes) { From 57da91ceff9db859dbe83aaa1a556e93417042bc Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Sun, 9 Dec 2018 21:57:39 -0600 Subject: [PATCH 11/13] adding rolling-upgrade exclusion for system_key file --- x-pack/qa/rolling-upgrade/build.gradle | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/x-pack/qa/rolling-upgrade/build.gradle b/x-pack/qa/rolling-upgrade/build.gradle index 68270032c8998..5e2c40564abdd 100644 --- a/x-pack/qa/rolling-upgrade/build.gradle +++ b/x-pack/qa/rolling-upgrade/build.gradle @@ -72,6 +72,10 @@ Project mainProject = project compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-try,-unchecked" +forbiddenPatterns { + exclude '**/system_key' +} + /** * Subdirectories of this project are test rolling upgrades with various * configuration options based on their name. @@ -97,6 +101,10 @@ subprojects { } } + forbiddenPatterns { + exclude '**/system_key' + } + String outputDir = "${buildDir}/generated-resources/${project.name}" // This is a top level task which we will add dependencies to below. From da8575101b004ecd47b2d55b48e65d4a07225b25 Mon Sep 17 00:00:00 2001 From: Christian Schneider Date: Sun, 9 Dec 2018 22:03:11 -0600 Subject: [PATCH 12/13] adding full-cluster-restart exclusion for system_key file --- x-pack/qa/full-cluster-restart/build.gradle | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x-pack/qa/full-cluster-restart/build.gradle b/x-pack/qa/full-cluster-restart/build.gradle index 9504f527cdd63..c9bbce0701954 100644 --- a/x-pack/qa/full-cluster-restart/build.gradle +++ b/x-pack/qa/full-cluster-restart/build.gradle @@ -86,6 +86,9 @@ licenseHeaders { approvedLicenses << 'Apache' } +forbiddenPatterns { + exclude '**/system_key' +} /** * Subdirectories of this project are test rolling upgrades with various * configuration options based on their name. @@ -115,6 +118,10 @@ subprojects { approvedLicenses << 'Apache' } + forbiddenPatterns { + exclude '**/system_key' + } + String outputDir = "${buildDir}/generated-resources/${project.name}" // This is a top level task which we will add dependencies to below. From 4af0c0693e31c111e5ae6a22b633cf0487a47ae5 Mon Sep 17 00:00:00 2001 From: Alpar Torok Date: Tue, 11 Dec 2018 14:10:58 +0200 Subject: [PATCH 13/13] Improove exception for non UTF8 files --- .../gradle/precommit/ForbiddenPatternsTask.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java index 8bd797a5f4ae9..d68985ff17ab6 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ForbiddenPatternsTask.java @@ -34,6 +34,7 @@ import java.io.File; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.util.AbstractMap; @@ -45,6 +46,7 @@ import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.IntStream; +import java.util.stream.Stream; /** * Checks for patterns in source files for the project which are forbidden. @@ -97,7 +99,12 @@ public void checkInvalidPatterns() throws IOException { Pattern allPatterns = Pattern.compile("(" + String.join(")|(", getPatterns().values()) + ")"); List failures = new ArrayList<>(); for (File f : files()) { - List lines = Files.lines(f.toPath(), StandardCharsets.UTF_8).collect(Collectors.toList()); + List lines; + try(Stream stream = Files.lines(f.toPath(), StandardCharsets.UTF_8)) { + lines = stream.collect(Collectors.toList()); + } catch (UncheckedIOException e) { + throw new IllegalArgumentException("Failed to read " + f + " as UTF_8", e); + } List invalidLines = IntStream.range(0, lines.size()) .filter(i -> allPatterns.matcher(lines.get(i)).find()) .boxed()