Check that client methods match API defined in the REST spec #31825

javanna · 2018-07-05T13:13:18Z

We have been encountering name mismatches between API defined in our
REST spec and method names that have been added to the high-level REST
client. We should check this automatically to prevent futher mismatches,
and correct all the current ones.

This commit adds a test for this and corrects the issues found by it.

We have been encountering name mismatches between API defined in our REST spec and method names that have been added to the high-level REST client. We should check this automatically to prevent futher mismatches, and correct all the current ones. This commit adds a test for this and corrects the issues found by it.

elasticmachine · 2018-07-05T13:13:19Z

Pinging @elastic/es-core-infra

javanna · 2018-07-05T13:15:23Z

client/rest-high-level/build.gradle

@@ -26,6 +27,9 @@ apply plugin: 'nebula.maven-scm'
 group = 'org.elasticsearch.client'
 archivesBaseName = 'elasticsearch-rest-high-level-client'

+Task copyRestSpec = RestIntegTestTask.createCopyRestSpecTask(project, false)
+test.dependsOn(copyRestSpec)


these build changes are temporary, I just hacked to make the new test work. The new test needs the spec in its classpath, yet it is not an integ test. Suggestions are welcome on how to improve this and make this cleaner.

javanna · 2018-07-05T13:16:39Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java

@@ -173,7 +173,7 @@ public void putMappingAsync(PutMappingRequest putMappingRequest, RequestOptions
     * @return the response
     * @throws IOException in case there is a problem sending the request or parsing back the response
     */
-    public GetMappingsResponse getMappings(GetMappingsRequest getMappingsRequest, RequestOptions options) throws IOException {
+    public GetMappingsResponse getMapping(GetMappingsRequest getMappingsRequest, RequestOptions options) throws IOException {


All these changes are breaking. I guess we should deprecate the old methods in 6.x and have the test ignore those? It is pretty annoying that we just released some of these (some haven't been released yet so we are lucky there) and we already need to rename them...

I agree we should deprecate the old names and remove in 7.0. Certainly annoying but we really are trying to be better about breaking the client.

javanna · 2018-07-05T13:19:03Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+
+        assertThat("Some client method doesn't match a corresponding API defined in the REST spec: " + apiNotFound,
+            apiNotFound.size(), equalTo(0));
+        assertThat(apiSpec, equalTo(Arrays.stream(notSupportedApi).collect(Collectors.toSet())));


Still conflicted on whether this is a good check or not, given that the client is not complete yet and 60 API are missing. yet it helps listing what is missing and possibly finding inconsistencies, for instance render search template should be already supported through search template but it does not have its own method (just a parameter in the request).

I think the test is fine. I think it'd be nicer to remove the notsupportedApi from the apiSpec and check that it is empty. You could even make it into a list and sort it so the results are easier to read.

sure will do

javanna · 2018-07-05T13:32:00Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

@@ -632,6 +640,108 @@ public void testProvidedNamedXContents() {
        assertTrue(names.contains(DiscountedCumulativeGain.NAME));
    }

+    public void testApiNamingConventions() throws Exception {


this test should probably be moved to a different class, as it needs rest spec in the classpath which is currently done from gradle. not sure if it should belong to unit tests or IT tests. it is kind of in between.

Since it doesn't need a running Elasticsearch server I'd keep it in unit tests. Moving it to another file is fine by me. I'm hoping that we can make the gradle copying thing not required though.....

ok let's try to make the copy not required then.

nik9000 · 2018-07-05T13:54:31Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java

@@ -173,7 +173,7 @@ public void putMappingAsync(PutMappingRequest putMappingRequest, RequestOptions
     * @return the response
     * @throws IOException in case there is a problem sending the request or parsing back the response
     */
-    public GetMappingsResponse getMappings(GetMappingsRequest getMappingsRequest, RequestOptions options) throws IOException {
+    public GetMappingsResponse getMapping(GetMappingsRequest getMappingsRequest, RequestOptions options) throws IOException {


I agree we should deprecate the old names and remove in 7.0. Certainly annoying but we really are trying to be better about breaking the client.

nik9000 · 2018-07-05T13:54:43Z

client/rest-high-level/src/main/java/org/elasticsearch/client/IndicesClient.java

@@ -446,7 +446,7 @@ public void getSettingsAsync(GetSettingsRequest getSettingsRequest, RequestOptio
     * @return the response
     * @throws IOException in case there is a problem sending the request or parsing back the response
     */
-    public ForceMergeResponse forceMerge(ForceMergeRequest forceMergeRequest, RequestOptions options) throws IOException {
+    public ForceMergeResponse forcemerge(ForceMergeRequest forceMergeRequest, RequestOptions options) throws IOException {


nik9000 · 2018-07-05T13:56:43Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

@@ -632,6 +640,108 @@ public void testProvidedNamedXContents() {
        assertTrue(names.contains(DiscountedCumulativeGain.NAME));
    }

+    public void testApiNamingConventions() throws Exception {


Since it doesn't need a running Elasticsearch server I'd keep it in unit tests. Moving it to another file is fine by me. I'm hoping that we can make the gradle copying thing not required though.....

nik9000 · 2018-07-05T13:57:47Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

@@ -632,6 +640,108 @@ public void testProvidedNamedXContents() {
        assertTrue(names.contains(DiscountedCumulativeGain.NAME));
    }

+    public void testApiNamingConventions() throws Exception {
+        String[] notSupportedApi = new String[]{
+            "cat.aliases", "cat.allocation", "cat.count", "cat.fielddata", "cat.health", "cat.help", "cat.indices", "cat.master",


I think making these one per line is going to make merges so much easier when we remove things from it.

I think you should remove cat.* via a hard coded pattern because we're not going to do it.

I think for the APIs we've decided not to implement we should have a line comment saying that.

nik9000 · 2018-07-05T14:00:51Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+                    Modifier.isFinal(method.getClass().getModifiers()) || Modifier.isFinal(method.getModifiers()));
+            assertTrue(Modifier.isPublic(method.getModifiers()));
+
+            if (apiName.endsWith("_async")) {


Where does _async come from. Like with the _ and all.

I see! So you snake_caseify the method name to line up with the spec and you make sure the async version pairs up.

Maybe worth a comment.

yep, I will add a comment

nik9000 · 2018-07-05T14:02:49Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+
+        assertThat("Some client method doesn't match a corresponding API defined in the REST spec: " + apiNotFound,
+            apiNotFound.size(), equalTo(0));
+        assertThat(apiSpec, equalTo(Arrays.stream(notSupportedApi).collect(Collectors.toSet())));


I think the test is fine. I think it'd be nicer to remove the notsupportedApi from the apiSpec and check that it is empty. You could even make it into a list and sort it so the results are easier to read.

nik9000 · 2018-07-05T14:05:59Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+    }
+
+    private static String toSnakeCase(String camelCase) {
+        List<Character> chars = new ArrayList<>();


StringBuilder instead?

nik9000 · 2018-07-05T14:07:47Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+            }
+        }
+
+        assertThat("Some client method doesn't match a corresponding API defined in the REST spec: " + apiNotFound,


This one is going to fail if we add APIs for plugins and when we add them for xpack. But we can cross that bridge when we come to it.

yes at the moment I think even modules are not considered. We should improve that, it can also be done later I think.

I think we stuff the spec for modules into the main REST spec for historical reasons.

javanna · 2018-07-06T15:08:20Z

client/rest-high-level/build.gradle

@@ -47,6 +47,7 @@ dependencies {
  testCompile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
  testCompile "junit:junit:${versions.junit}"
  testCompile "org.hamcrest:hamcrest-all:${versions.hamcrest}"
+  testCompile "org.elasticsearch:rest-api-spec:${version}"


funnily, this makes it work from intellij, as stuff is found on filesystem, extracted from jars. not sure what this does in Eclipse.

javanna · 2018-07-06T15:09:34Z

...amework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java

+            while (e.hasMoreElements()) {
+                JarEntry entry = e.nextElement();
+                if (entry.getName().startsWith( classpathPrefix) && entry.getName().endsWith(".json")) {
+                    parseSpecFile(restApiParser, null/*entry.getName()?*/, restSpec);


this side of the branch does not quite work yet, currently stuck at java.security.AccessControlException: access denied ("java.io.FilePermission" "file:/home/javanna/elastic/elasticsearch/rest-api-spec/build/distributions/rest-api-spec-7.0.0-alpha1-SNAPSHOT.jar" "read") at line 98 above.

javanna · 2018-07-06T15:10:17Z

...amework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java

-            });
+            }
+        } else {
+            Path dir = PathUtils.get(ClientYamlSuiteRestSpec.class.getResource(classpathPrefix).toURI());


this one is still exercised in the new unit test when run from idea, I guess that once copying is not required it should not be exercised anymore by the integ test stuff.

javanna · 2018-07-06T15:11:13Z

@nik9000 I addressed all comments besides the part around not copying spec files around.

nik9000 · 2018-07-06T21:37:06Z

@nik9000 I addressed all comments besides the part around not copying spec files around.

I'll have a look at the spec copying thing on Monday I think.

javanna · 2018-07-16T13:37:52Z

@nik9000 I made the copying work like we discussed. The change should not be disruptive, all the rest should work this time. I have tried things and the new test works from both IntelliJ and command line. Would you mind checking what Eclipse does? I have left a TODO for the xpack API, I ignore them for now and I think it should be a followup.

javanna · 2018-07-16T13:38:09Z

@hub-cap feel free to have a look as well!

hub-cap · 2018-07-16T15:58:51Z

client/rest-high-level/src/test/java/org/elasticsearch/client/RestHighLevelClientTests.java

+
+                boolean remove = apiSpec.remove(apiName);
+                if (remove == false && deprecatedMethods.contains(apiName) == false) {
+                    //TODO xpack api are currently ignored, we need to load xpack yaml spec too


Can you create a GH issue and assign it to me to finish up the xpack.* here?

hub-cap · 2018-07-16T16:01:19Z

...amework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java

        ClientYamlSuiteRestSpec restSpec = new ClientYamlSuiteRestSpec();
        ClientYamlSuiteRestApiParser restApiParser = new ClientYamlSuiteRestApiParser();
+        Path dir = PathUtils.get(ClientYamlSuiteRestSpec.class.getResource(classpathPrefix).toURI());


any reason these got moved around here? Seems like the imports and the dir are exactly the same, albeit just moved a few lines.

I believe this is because he tried to make a change to this file but it didn't work out and he reversed it by hand. @javanna , can you entirely revert the changes to this file?

hub-cap

Minor Q. Id love to see this merged asap so I can do the work for the xpack.* spec methods before we go crazy adding them all!

nik9000

LGTM and it works in Eclipse.

nik9000 · 2018-07-16T16:57:10Z

...amework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestSpec.java

        ClientYamlSuiteRestSpec restSpec = new ClientYamlSuiteRestSpec();
        ClientYamlSuiteRestApiParser restApiParser = new ClientYamlSuiteRestApiParser();
+        Path dir = PathUtils.get(ClientYamlSuiteRestSpec.class.getResource(classpathPrefix).toURI());


I believe this is because he tried to make a change to this file but it didn't work out and he reversed it by hand. @javanna , can you entirely revert the changes to this file?

javanna · 2018-07-16T19:11:19Z

I addressed the latest comments, this should be ready, I will merge once I get a green build. Thanks for the reviews!

@return

* master: Painless: Simplify Naming in Lookup Package (#32177) Handle missing values in painless (#32207) add support for write index resolution when creating/updating documents (#31520) ECS Task IAM profile credentials ignored in repository-s3 plugin (#31864) Remove indication of future multi-homing support (#32187) Rest test - allow for snapshots to take 0 milliseconds Make x-pack-core generate a pom file Rest HL client: Add put watch action (#32026) Build: Remove pom generation for plugin zip files (#32180) Fix comments causing errors with Java 11 Fix rollup on date fields that don't support epoch_millis (#31890) Detect and prevent configuration that triggers a Gradle bug (#31912) [test] port linux package packaging tests (#31943) Revert "Introduce a Hashing Processor (#31087)" (#32178) Remove empty @return from JavaDoc Adjust SSLDriver behavior for JDK11 changes (#32145) [test] use randomized runner in packaging tests (#32109) Add support for field aliases. (#32172) Painless: Fix caching bug and clean up addPainlessClass. (#32142) Call setReferences() on custom referring tokenfilters in _analyze (#32157) Fix BwC Tests looking for UUID Pre 6.4 (#32158) Improve docs for search preferences (#32159) use before instead of onOrBefore Add more contexts to painless execute api (#30511) Add EC2 credential test for repository-s3 (#31918) A replica can be promoted and started in one cluster state update (#32042) Fix Java 11 javadoc compile problem Fix CP for namingConventions when gradle home has spaces (#31914) Fix `range` queries on `_type` field for singe type indices (#31756) [DOCS] Update TLS on Docker for 6.3 (#32114) ESIndexLevelReplicationTestCase doesn't support replicated failures but it's good to know what they are Remove versionType from translog (#31945) Switch distribution to new style Requests (#30595) Build: Skip jar tests if jar disabled Painless: Add PainlessClassBuilder (#32141) Build: Make additional test deps of check (#32015) Disable C2 from using AVX-512 on JDK 10 (#32138) Build: Move shadow customizations into common code (#32014) Painless: Fix Bug with Duplicate PainlessClasses (#32110) Remove empty @param from Javadoc Re-disable packaging tests on suse boxes Docs: Fix missing example script quote (#32010) [ML] Wait for aliases in multi-node tests (#32086) [ML] Move analyzer dependencies out of categorization config (#32123) Ensure to release translog snapshot in primary-replica resync (#32045) Handle TokenizerFactory TODOs (#32063) Relax TermVectors API to work with textual fields other than TextFieldType (#31915) Updates the build to gradle 4.9 (#32087) Mute :qa:mixed-cluster indices.stats/10_index/Index - all’ Check that client methods match API defined in the REST spec (#31825) Enable testing in FIPS140 JVM (#31666) Fix put mappings java API documentation (#31955) Add exclusion option to `keep_types` token filter (#32012) [Test] Modify assert statement for ssl handshake (#32072)

Moves the customizations to the build to produce nice shadow jars and javadocs into common build code, mostly BuildPlugin with a little into the root build.gradle file. This means that any project that applies the shadow plugin will automatically be set up just like the high level rest client: * The non-shadow jar will not be built * The shadow jar will not have a "classifier" * Tests will run against the shadow jar * Javadoc will include all of the shadowed classes * Service files in `META-INF/services` will be merged

We have been encountering name mismatches between API defined in our REST spec and method names that have been added to the high-level REST client. We should check this automatically to prevent furher mismatches, and correct all the current ones. This commit adds a test for this and corrects the issues found by it.

* 6.x: Security: revert to old way of merging automata (#32254) Fix a test bug in RangeQueryBuilderTests introduced in the field aliases backport. Introduce Application Privileges with support for Kibana RBAC (#32309) Undo a debugging change that snuck in during the field aliases merge. [test] port linux package packaging tests (#31943) Painless: Update More Methods to New Naming Scheme (#32305) Tribe: Add error with secure settings copied to tribe (#32298) Add V_6_3_3 version constant Add ERR to ranking evaluation documentation (#32314) [DOCS] Added link to 6.3.2 RNs [DOCS] Updates 6.3.2 release notes with PRs from ml-cpp repo (#32334) [Kerberos] Add Kerberos authentication support (#32263) [ML] Extract persistent task methods from MlMetadata (#32319) Backport - Add Snapshots Status API to High Level Rest Client (#32295) Make release notes ignore the `>test-failure` label. (#31309) [DOCS] Adds release highlights for search for 6.4 (#32095) Allow Integ Tests to run in a FIPS-140 JVM (#32316) Add support for field aliases to 6.x. (#32184) Register ERR metric with NamedXContentRegistry (#32320) fixes broken build for third-party-tests (#32315) Relates #31918 / Closes infra/issues/6085 [DOCS] Rollup Caps API incorrectly mentions GET Jobs API (#32280) Rest HL client: Add put watch action (#32026) (#32191) Add WeightedAvg metric aggregation (#31037) Consistent encoder names (#29492) Switch monitoring to new style Requests (#32255) specify subdirs of lib, bin, modules in package (#32253) Rename ranking evaluation `quality_level` to `metric_score` (#32168) Add new permission for JDK11 to load JAAS libraries (#32132) Switch x-pack:core to new style Requests (#32252) Watcher: Store username on watch execution (#31873) Silence SSL reload test that fails on JDK 11 Painless: Clean up add methods in PainlessLookup (#32258) CCE when re-throwing "shard not available" exception in TransportShardMultiGetAction (#32185) Fail shard if IndexShard#storeStats runs into an IOException (#32241) Fix `range` queries on `_type` field for singe type indices (#31756) (#32161) AwaitsFix RecoveryIT#testHistoryUUIDIsGenerated Add new fields to monitoring template for Beats state (#32085) (#32273) [TEST] improve REST high-level client naming conventions check (#32244) Check that client methods match API defined in the REST spec (#31825)

javanna added >enhancement WIP v7.0.0 :Core/Java High Level REST Client v6.4.0 labels Jul 5, 2018

javanna requested a review from nik9000 July 5, 2018 13:13

javanna commented Jul 5, 2018

View reviewed changes

remove needless dependency

33d86a6

nik9000 reviewed Jul 5, 2018

View reviewed changes

javanna added 4 commits July 6, 2018 12:52

Merge branch 'master' into test/hl_client_spec

bfc28a2

address part of the review comments

4e035aa

deprecate old methods

16ec7a2

remove empty lines and unused import

fba6b12

javanna commented Jul 6, 2018

View reviewed changes

javanna mentioned this pull request Jul 9, 2018

HLRest: Move xPackInfo() to xPack().info() #31905

Merged

javanna added 3 commits July 16, 2018 14:26

Merge branch 'master' into test/hl_client_spec

0dee911

revert to copying spec files, adapt to changes upstream

e0d6635

add todo on xpack api

7086d08

javanna removed the WIP label Jul 16, 2018

hub-cap reviewed Jul 16, 2018

View reviewed changes

hub-cap approved these changes Jul 16, 2018

View reviewed changes

nik9000 approved these changes Jul 16, 2018

View reviewed changes

javanna added 2 commits July 16, 2018 21:05

revert some of the changes

72e9fa4

add comments

dc3c55e

javanna merged commit 049966a into elastic:master Jul 17, 2018

javanna added the backport pending label Jul 17, 2018

spinscale mentioned this pull request Jul 20, 2018

Rest HL client: Add put watch action (#32026) #32191

Merged

javanna mentioned this pull request Jul 20, 2018

REST high-level client: check naming conventions for xpack API #32245

Closed

javanna removed the backport pending label Jul 22, 2018

javanna mentioned this pull request Jul 22, 2018

Add Snapshots Status API to High Level Rest Client #31515

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Check that client methods match API defined in the REST spec #31825

Check that client methods match API defined in the REST spec #31825

Conversation

javanna commented Jul 5, 2018

elasticmachine commented Jul 5, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jul 6, 2018

nik9000 commented Jul 6, 2018

javanna commented Jul 16, 2018

javanna commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap left a comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Jul 16, 2018