Validate REST specs against schema #55117

jakelandis · 2020-04-13T15:45:19Z

A JSON schema was recently introduced for the REST API specification. #54252
This PR introduces a 3rd party validation tool to ensure that the
REST specification conforms the schema.

The task is applied to the 3 projects that contain REST API specifications.
The plugin wires this task into the precommit commit task, and should be
considered as part of the public API for the build tools for any plugin
developer to contribute their plugin's specification.

An ignore parameter has been introduced for the task to allow specific
file to be ignored from the validation. The ignored files in this PR
will soon get issues logged and a link so they can be fixed.

Closes #54314

A JSON schema was recently introduced for the REST API specification. elastic#54252 This PR introduces a 3rd party validation tool to ensure that the REST specification conforms the schema. The task is applied to the 3 projects that contain REST API specifications. The plugin wires this task into the precommit commit task, and should be considered as part of the public API for the build tools for any plugin developer to contribute their plugin's specification. An ignore parameter has been introduced for the task to allow specific file to be ignored from the validation. The ignored files in this PR will soon get issues logged and a link so they can be fixed. Closes elastic#54314

elasticmachine · 2020-04-13T15:45:22Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

jakelandis · 2020-04-13T18:01:30Z

@elasticmachine update branch

jakelandis · 2020-04-13T20:14:54Z

@elasticmachine run elasticsearch-ci/1

mark-vieira

Left some comments.

Also, one issue we have here is that this is all very coupled to the notion of at most two source sets (main and test). Once we start splitting these things up this assumption is going to break. We already have plans to split out YAML tests into their own source set, how will that affect this?

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

mark-vieira · 2020-04-13T21:05:37Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

+    public void validate() throws IOException {
+        // load schema
+        JsonSchema jsonSchema;
+        FileTree jsonSchemas = Util.getJavaMainSourceResources(


We should not directly access the schema from the task implementation. We should do this in the plugin and pass them in as an input to the task. Two reasons for this:

This logic simply shouldn't exist in the task. This "wiring" should be done by plugins. This makes the tasks more reusable and composable.

The schema isn't tracked as an input. Which means, if it changes we won't correctly flag this task as out of date.

Done, and thanks for the feedback. I think the result is much cleaner.

I changed the task so that it is generic Json validator and moved the ES specific location of stuff to the plugin.

mark-vieira · 2020-04-13T21:06:34Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

+
+    private static final String SCHEMA_PROJECT = ":rest-api-spec";
+    private static final String DOUBLE_STARS = "**"; // checkstyle thinks this is an empty javadoc statement, so string concat instead
+    private static final String JSON_SPEC_PATTERN_INCLUDE = DOUBLE_STARS + "/rest-api-spec/api/" + DOUBLE_STARS + "/*.json";


All this stuff should be in the plugin. These are conventions specific to our project. We should be able to easily reuse this task to validate something else. Right now that would be impossible do to these conventions being hard-coded into the task implementation.

Agreed, and done.

mark-vieira · 2020-04-13T21:08:18Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

+    @InputFiles
+    public FileTree getInputDir() {
+        // if null results in in NO-SOURCE
+        return Util.getJavaTestAndMainSourceResources(


Again, this should be in the plugin. We should be just add a setInputFiles(FileCollection files) that we pass exactly what we want to validate. Then the plugin can do all this source set stuff. Also the plugin should confirm these source sets exist (or apply the plugin that creates them) so things don't explode.

mark-vieira · 2020-04-13T21:19:06Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

+                    sb.append(System.lineSeparator());
+                });
+            });
+            throw new JsonSchemaException(sb.toString());


Let's avoid smashing the output of this task in an exception message. Instead let's follow the example of other core Gradle tasks which is to simply log the errors in the task using logger and then throw an exception message that says something like "There were errors, refer to task output.". We can (and should) include a summary in the exception (ex: "Validation failed: 3 files contained 8 violations"), but details should be in task output.

For example, here's a checkstyle error: https://gradle-enterprise.elastic.co/s/ixyvqw455k6m6/failure?openFailures=WzBd&openStackTraces=WzFd#top=0

Done. The a validation error looks like:

> Task :rest-api-spec:validateRestSpec FAILED [validate JSON][ERROR][cat.thread_pool.json][$.cat.thread_pool.params.size.options[0]: does not match the regex pattern ^[a-zA-Z_]+$] FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':rest-api-spec:validateRestSpec'. > Error validating JSON. See the report at: file:/Users/jakelandis/workspace/8x/elasticsearch/rest-api-spec/build/reports/validateJson.txt JSON validation failed: 1 files contained 1 violations

The report looks like:

Schema: /Users/jakelandis/workspace/8x/elasticsearch/rest-api-spec/src/main/resources/schema.json ----------Validation Errors----------- /Users/jakelandis/workspace/8x/elasticsearch/rest-api-spec/src/main/resources/rest-api-spec/api/cat.thread_pool.json: $.cat.thread_pool.params.size.options[0]: does not match the regex pattern ^[a-zA-Z_]+$

I'm not sure we need the report file and the log output, what do you think? To me the log output is the priority, as that's the only convenient way to debug errors in CI. And if we have all the info we need in the console, do we need to create the report file?

Sorry if my example sent you down that path. I was mainly getting at the fact that individual errors are reported in the task output and the exception message simply refers to that output.

I think there is marginal benefit to having a report. It allows me to use the fqn for the files without polluting the log output with long names. Also, this validation as-is should feel familiar since it like checkstyle (not sure if that is good or bad)

I have a minor preference to keep the report.

I have a minor preference to keep the report.

SGTM 👍

If we are going to keep the report, let's just go ahead and ditch the success marker then. No point in having two different outputs. In the case of scenario where we found no errors, we can just say as much in the report file. So let's get rid of that and the need to extend PrecommitTask.

done. 6090fba

buildSrc/src/main/java/org/elasticsearch/gradle/util/Util.java

mark-vieira · 2020-04-13T21:29:06Z

buildSrc/src/main/java/org/elasticsearch/gradle/util/Util.java

@@ -75,4 +84,91 @@ public static URI getBuildSrcCodeSource() {
            throw new GradleException("Error determining build tools JAR location", e);
        }
    }
+
+    public static Function<FileTree, FileTree> filterByPatterns(


This is a bit of an awkward way of defining a filter. Instead of creating a function that takes a FileTree and returns another FileTree why not model the filter as an Action<? super PatternFilterable>? Then your methods that take this could just look like:

getJavaMainSourceResources(project, spec -> spec.include(**/foo/**))

This also avoids the need to create a PatternSet via PatternSetFactory which is technically an internal API.

Also would alleviate the need for a bunch of null-checking logic on the filter.

Cool, thanks for the pointer. Updated to use Action<? super PatternFilterable> and think it is much cleaner now.

mark-vieira · 2020-04-13T21:34:33Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecTask.java

+    private static final String SCHEMA_PROJECT = ":rest-api-spec";
+    private static final String DOUBLE_STARS = "**"; // checkstyle thinks this is an empty javadoc statement, so string concat instead
+    private static final String JSON_SPEC_PATTERN_INCLUDE = DOUBLE_STARS + "/rest-api-spec/api/" + DOUBLE_STARS + "/*.json";
+    private static final String JSON_SPEC_PATTERN_EXCLUDE = DOUBLE_STARS + "/_common.json";


Does this file ever get validated?

Nope. this is a hard coded exception to the JSON schema validation.

jakelandis · 2020-04-19T23:17:33Z

thanks @mark-vieira for the review. This should be good to go again.

Also, one issue we have here is that this is all very coupled to the notion of at most two source sets (main and test). Once we start splitting these things up this assumption is going to break. We already have plans to split out YAML tests into their own source set, how will that affect this?

With the latest round of changes, I have separated the concerns of validation and setup, where the setup is handled by the plugin, and the validation the task. When YAML test source sets are introduced, the plugin will add that source set to the set of inputs to look at and no changes should be required from the task. So instead of just main, and test, the plugin will also include yaml tests (or what ever they will be called). FWIW, splitting the YAML tests to their own source sets is up next on my todo list.

jakelandis · 2020-04-20T13:25:20Z

Here are the relevant build scans :

https://gradle-enterprise.elastic.co/s/gy5kw7vrmupxy/timeline?typeFilter=org.elasticsearch.gradle.precommit.ValidateJsonAgainstSchemaTask
https://gradle-enterprise.elastic.co/s/qnretkunosevw/timeline?typeFilter=org.elasticsearch.gradle.precommit.ValidateJsonAgainstSchemaTask

jakelandis · 2020-04-20T15:32:05Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+    private File jsonSchema;
+    private FileTree inputFiles;
+    // no need to rebuild the JsonSchema multiple times per build
+    private static ConcurrentHashMap<File, JsonSchema> cache = new ConcurrentHashMap<>(1);


oops... this isn't effective across multiple tasks. Will remove this, or move out to the plugin.

Let's just ditch it. These tasks are already pretty quick and the risk of daemon memory leaks here are probably not worth the performance benefit. We can come back later and micro-optimize if we get some time.

done e06e434

mark-vieira

Left some more comments.

mark-vieira · 2020-04-20T19:00:44Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+ */
+public class ValidateJsonAgainstSchemaTask extends PrecommitTask {
+
+    private static final ObjectMapper mapper = new ObjectMapper();


Let's remove static here. I realize there's some overhead in allocating these but the potential for weird memory leaks in the daemon is probalby not worth it. The Gradle daemon is long-lived and also caches classloaders. Static references can cause weird issues with leaking classloaders which can cause daemons to eventually OOM.

mark-vieira · 2020-04-20T19:05:54Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+                throw new UncheckedIOException(e);
+            }
+        });
+        ConcurrentHashMap<File, Set<String>> errors = new ConcurrentHashMap<>();


There's no concurrency that I can see here. We are simply validating each input file serially on the task execution thread. Also, we should probably use a map implementation that maintains insertion order like LinkedHashMap so that in cases with multiple errors the ordering makes sense (all errors for the same file in the same place in line order).

mark-vieira · 2020-04-20T19:07:14Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+        ConcurrentHashMap<File, Set<String>> errors = new ConcurrentHashMap<>();
+        // incrementally evaluate input files
+        inputChanges.getFileChanges(getInputFiles()).forEach(fileChange -> {
+            File file = fileChange.getFile();


Keep in mind here that fileChange could be a removed file. In which case we'd fail here as we'd try to validate a file that has been deleted. We should filter those out.

mark-vieira · 2020-04-20T19:09:50Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+
+    @Incremental
+    @InputFiles
+    public FileTree getInputFiles() {


I think we might be able to simplify things by just making this a FileCollection instead of a FileTree. There's no reason to keep a tree structure here as ignores are done of a file name basis and file paths are completely irrelevant for the purposes of validation.

This considerably simplifies things like adding input files from completely disparate directories. You can do this with file trees but it ends up creating a UnionFileTree which can have some funky behavior.

mark-vieira · 2020-04-20T19:12:04Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+
+    private void maybeLogAndCollectError(Set<ValidationMessage> messages, ConcurrentHashMap<File, Set<String>> errors, File file) {
+        for (ValidationMessage message : messages) {
+            String error = String.format("[validate JSON][ERROR][%s][%s]", file.getName(), message.toString());


There's no need to use String.format here as the Logger.error() method supports formatting strings using {} as a placeholder.

mark-vieira · 2020-04-20T19:13:45Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+            String error = String.format("[validate JSON][ERROR][%s][%s]", file.getName(), message.toString());
+            getLogger().error(error);
+            try {
+                errors.computeIfAbsent(file, k -> new HashSet<>())


Same as above, let's use a set implementation that maintains ordering so we don't potentially print an error for line 50 before one on line 30 in the report.

mark-vieira · 2020-04-20T19:16:34Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+            Files.write(getSuccessMarker().toPath(), new byte[] {}, StandardOpenOption.CREATE);
+        } else {
+            // build output and throw exception
+            Files.writeString(getErrorReport().toPath(), String.format("Schema: %s", jsonSchemaOnDisk), StandardOpenOption.CREATE);


I feel like we should probably just create a single PrintWriter and do all this stuff in one go. Each call to Files.write* is going to have to open and then close a stream on that file. Let's just do this once.

mark-vieira · 2020-04-20T19:18:54Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+            getLogger().error(error);
+            try {
+                errors.computeIfAbsent(file, k -> new HashSet<>())
+                    .add(String.format("%s: %s", file.getCanonicalFile(), message.toString()));


Is it strictly necessary to resolve the canonical file path here? I mean, if folks are working in some kind linked workspace folder wouldn't it be friendlier to report that path instead of the resolved one?

i don't think it matters too much. are you advocating for absolute path, or relative path ?

Absolute path, but we don't have to resolve the canonical path. It's costly, and requires catching an IOException vs just using the given absolute path.

mark-vieira · 2020-04-20T20:50:56Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateRestSpecPlugin.java

+
+    @Override
+    public void apply(Project project) {
+        if (jsonSchema == null) {


Firstly, project configuration is never done in parallel, so there's no need for the thread safety stuff here. Second, I think this is overly complex. Instead of all this nastiness to try and find the file, let's just go ahead and have the project configure its location. It's one file, and it's rarely if every going to move. Doing a bunch of work at configuraiton time to scan for this file seems unnecessary.

I moved the work into the task configuration ... is that what you meant ? e9dd891

(I think this is still configuration time work... but since it is a registered task, won't do any work unless needed ?)

Not exactly, although this is better since we now defer the work unless the task is actually executed. I mean we should remove this "search" altogether and just do something along the lines of validateJson.jsonSchema = file('rest-api-spec/src/main/resources/schema.json') in the project build script or even hard-code in the plugin for now (since it's already tightly coupled to our project).

My reasoning being, there is exactly one file, we always know where it'll be and it rarely moves. Why all the work to dynamically locate it?

OK .. i understand now. I hard coded it in the plugin. 1768c6d

mark-vieira · 2020-04-20T20:51:44Z

x-pack/plugin/build.gradle

@@ -109,3 +110,15 @@ testClusters.integTest {
  extraConfigFile nodeCert.name, nodeCert
  extraConfigFile 'roles.yml', file('src/test/resources/roles.yml')
 }
+
+validateRestSpec {
+  ignore 'searchable_snapshots.mount.json'


What's the reason to ignore these files? Do they fail validation? If so, shouldn't we fix the failures so it doesn't break the clients team?

yeah, these will need to be fixed. I plan to log issues to fix once this get merged.

Once we get this fixed I'd argue for removing the ability to ignore. The purpose of doing validation here is so that the clients folks don't have to do it downstream. Not much point in validating these twice, both the producer and consumer sides.

Going forward we need to make it such that a change that doesn't conform to the schema fails checks and effectively cannot be merged. This way the clients team can ditch this on their end, simplifying their process.

jakelandis · 2020-04-20T22:18:05Z

implemented requested changes on e9dd891 ... However, have one question I wasn't sure about .

mark-vieira

Two minor comments, othewise latest changes LGTM 👍

mark-vieira · 2020-04-20T22:38:33Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+        Map<File, Set<String>> errors = new LinkedHashMap<>();
+        // incrementally evaluate input files
+        StreamSupport.stream(inputChanges.getFileChanges(getInputFiles()).spliterator(), false)
+            .filter(f -> ChangeType.REMOVED.equals(f.getChangeType()) == false)


nit: I think just writing this as f.getChangeType() != ChangeType.REMOVED is simpler yes? Does our stance on using == false in the codebase mean avoid != as well in cases where using .equals() is unnecessary?

not sure ... i think i am just used to == false now :) ... changed it to !=

mark-vieira · 2020-04-20T22:40:19Z

buildSrc/src/main/java/org/elasticsearch/gradle/precommit/ValidateJsonAgainstSchemaTask.java

+        return inputFiles;
+    }
+
+    public void setInputFiles(FileTree inputFiles) {


Setter should be FileCollection as well. Also the field should be updated.

jakelandis · 2020-04-20T22:56:15Z

@mark-vieira - thanks so much for the thorough review! I am learning so much about Gradle and how it should be modeled.

jakelandis · 2020-04-21T16:32:44Z

@elasticmachine update branch

jakelandis · 2020-04-21T20:12:13Z

@elasticmachine update branch

russcam · 2020-04-21T23:31:31Z

Thank you for the work and review that went into this PR, @jakelandis and @mark-vieira!

A JSON schema was recently introduced for the REST API specification. elastic#54252 This PR introduces a 3rd party validation tool to ensure that the REST specification conforms to the schema. The task is applied to the 3 projects that contain REST API specifications. The plugin wires this task into the precommit commit task, and should be considered as part of the public API for the build tools for any plugin developer to contribute their plugin's specification. An ignore parameter has been introduced for the task to allow specific file to be ignored from the validation. The ignored files in this PR will soon get issues logged and a link so they can be fixed. Closes elastic#54314

A JSON schema was recently introduced for the REST API specification. #54252 This PR introduces a 3rd party validation tool to ensure that the REST specification conforms to the schema. The task is applied to the 3 projects that contain REST API specifications. The plugin wires this task into the precommit commit task, and should be considered as part of the public API for the build tools for any plugin developer to contribute their plugin's specification. An ignore parameter has been introduced for the task to allow specific file to be ignored from the validation. The ignored files in this PR will soon get issues logged and a link so they can be fixed. Closes #54314

jakelandis added >non-issue :Delivery/Build Build or test infrastructure v8.0.0 v7.6.3 v7.8.0 v7.7.1 labels Apr 13, 2020

jakelandis requested review from russcam and mark-vieira April 13, 2020 15:45

fix javadoc

7b8bdf0

Merge branch 'master' into validate_rest_spec

b34ba76

mark-vieira requested changes Apr 13, 2020

View reviewed changes

jakelandis removed the v7.6.3 label Apr 16, 2020

jakelandis added 5 commits April 19, 2020 17:50

review updates (still missing report)

4b2fadc

checkstyle/spotless and new report

9d89f40

Merge remote-tracking branch 'upstream/master' into validate_rest_spec

6c7e1a3

minor wording changes

86b8a77

checkstyle

0918c9a

add another ignore

6a25b26

jakelandis commented Apr 20, 2020

View reviewed changes

jakelandis added 2 commits April 20, 2020 14:48

remove unecessary 'cache'

e06e434

use default task instead of precommit task

6090fba

mark-vieira reviewed Apr 20, 2020

View reviewed changes

review updates

e9dd891

jakelandis added 2 commits April 20, 2020 17:25

remove unnecessary FileWriter

323e4d0

hard code schema location

1768c6d

mark-vieira approved these changes Apr 20, 2020

View reviewed changes

review changes

b11efbd

Merge branch 'master' into validate_rest_spec

1431191

jakelandis changed the title ~~new validateRestSpec task and plugin~~ Validate REST specs against schema Apr 21, 2020

jakelandis merged commit 2b0900d into elastic:master Apr 21, 2020

jakelandis deleted the validate_rest_spec branch April 21, 2020 23:18

jakelandis added the backport pending label Apr 21, 2020

jakelandis mentioned this pull request Apr 21, 2020

[7.7] Validate REST specs against schema (#55117) #55562

Merged

jakelandis mentioned this pull request Apr 21, 2020

[7.x] Validate REST specs against schema (#55117) #55563

Merged

jakelandis removed the backport pending label Apr 22, 2020

delvedor mentioned this pull request Jun 30, 2020

Validate URLs in ES JSON spec elastic/docs#1884

Closed

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Validate REST specs against schema #55117

Validate REST specs against schema #55117

Conversation

jakelandis commented Apr 13, 2020

elasticmachine commented Apr 13, 2020

jakelandis commented Apr 13, 2020

jakelandis commented Apr 13, 2020

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Apr 19, 2020

jakelandis commented Apr 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-vieira Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

jakelandis commented Apr 20, 2020

mark-vieira left a comment

Choose a reason for hiding this comment

mark-vieira Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Apr 20, 2020 • edited Loading

jakelandis commented Apr 21, 2020

jakelandis commented Apr 21, 2020

russcam commented Apr 21, 2020

mark-vieira Apr 20, 2020 •

edited

Loading

jakelandis Apr 20, 2020 •

edited

Loading

mark-vieira Apr 20, 2020 •

edited

Loading

mark-vieira Apr 20, 2020 •

edited

Loading

jakelandis commented Apr 20, 2020 •

edited

Loading