Removes typed calls from YAML REST tests #37611

colings86 · 2019-01-18T14:24:48Z

This PR attempts to remove all typed calls from our YAML REST tests. The PR adds include_type_name: false to create index requests that use a mapping and also to put mapping requests. It also removes _type from index requests where they haven't already been removed. The PR ignores tests named *_with_types.yml since this are specifically testing typed API behaviour.

The change also includes changing the test harness to add the type _doc to index, update, get and bulk requests that do not specify the document type when the test is running against a mixed 7.x/6.x cluster.

elasticmachine · 2019-01-18T14:24:49Z

Pinging @elastic/es-search

markharwood · 2019-01-18T16:28:46Z

...api-spec/src/main/resources/rest-api-spec/test/indices.get_mapping/50_wildcard_expansion.yml

-              type_2: {}
+              properties:
+                foo2:
+                  type: keyword


Curious: why the need to introduce new properties in this test?

The test failed if I added an empty mappings object here. I think it might be because in the YAML framework we might evaluate {} as falsey but I could be wrong. If you feel strongly against doing this I could probably modify the checks below to check explicitly for an empty mappings object

colings86 · 2019-01-18T18:05:09Z

@elasticmachine test this please

jtibshirani · 2019-01-18T18:09:46Z

Thanks @colings86 for this PR!

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

For this API, we accidentally deviated from our usual strategy of creating a new file called 11_basic_with_types.yml. I think we should move the test "Test include_type_name" into a new file called 11_basic_with_types with the typed index set-up, and remove the mention of types in 10_basic.

indices.get_field_mapping/30_missing_type.yml
indices.get_mapping/20_missing_type.yml
indices.stats/15_types.yml

You're right in that these tests are explicitly testing typed behavior, and shouldn't need much modification. Instead of skipping 7.0 completely for these tests, I think we should just make sure the index creation calls specify include_type_name=true. That way we can continue to test both 7.0 and mixed-version clusters, as we do for the tests named *_with_types.yml.

colings86 · 2019-01-21T14:30:52Z

@jtibshirani I push a commit that should address your comments

colings86 · 2019-01-21T14:52:23Z

@elasticmachine run elasticsearch-ci-2

colings86 · 2019-01-21T16:12:50Z

@elasticmachine test this please

jtibshirani

These changes are looking good to me, I just had one additional comment: should we remove the change to the test harness as part of this PR, to make sure that we've covered all the tests that need updating? Here is the change, for reference: https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java#L101-L107

I also just had a couple thoughts/ clarifications:

I noticed that in many places we still check the value of the _type field in responses. This makes sense to me for the current PR, and I assume we will remove these checks in 8.0 when we stop returning the _type field altogether.
After this merges, backports from 7.0 to 6.x may encounter more conflicts. However, I'm assuming this will not be a big issue given where we are in terms of the 6.x release cycles.

colings86 · 2019-01-22T08:57:25Z

should we remove the change to the test harness as part of this PR

I think we should do this as a follow up PR since this PR is already pretty big if thats ok with you. The follow up can change the test harness back and then clean up any remaining tests

I noticed that in many places we still check the value of the _type field in responses. This makes sense to me for the current PR, and I assume we will remove these checks in 8.0 when we stop returning the _type field altogether.

Yeah I think still checking _type is important in 7.x because we want to ensure it is always emitting _doc.

After this merges, backports from 7.0 to 6.x may encounter more conflicts. However, I'm assuming this will not be a big issue given where we are in terms of the 6.x release cycles.

I think any conflicts should be pretty simple to resolve since the vast majority of these changes are fairly minor.. Also most of the changes are in the setup of the tests which are less likely to need to be modified in backported PRs, especially given we are so late in the 6.x series

colings86 · 2019-01-22T08:58:16Z

@elasticmachine test this please

jpountz · 2019-01-22T17:05:20Z

Is the WIP label still relevant?

colings86 · 2019-01-22T17:20:14Z

@jpountz I was leaving the WIP label on it until I could get the tests to all pass on CI since there are still some tests failing because of things I have missed. Feel free to review what I have though. I can also remove WIP if you think having it on here confuses things?

jpountz · 2019-01-22T17:23:10Z

I don't have a preference, I just had a look and it looks really great. We certainly owe you beers (yes, plural) for that work!

jtibshirani

Thanks again for this PR, looks good to me assuming the tests pass!

colings86 · 2019-01-23T14:11:08Z

@elasticmachine test this please

colings86 · 2019-01-23T14:11:51Z

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

colings86 · 2019-01-24T18:51:39Z

@elasticmachine run elasticsearch-ci/1

colings86 · 2019-01-25T14:35:52Z

@elasticmachine test this please

colings86 · 2019-01-25T16:24:59Z

@elasticmachine test this please

colings86 · 2019-01-28T13:51:43Z

@elasticmachine test this please

colings86 · 2019-01-29T11:05:49Z

@elasticmachine run elasticsearch-ci/default-distro

colings86 · 2019-01-29T11:45:23Z

@elasticmachine run elasticsearch-ci/2

colings86 · 2019-01-29T13:56:21Z

@elasticmachine run elasticsearch-ci/2

colings86 · 2019-01-29T13:56:36Z

@elasticmachine run elasticsearch-ci/packaging-sample

jpountz

Changes to the test harness look good to me.

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/11_basic_with_types.yml

...framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java

jtibshirani

I left one question about the test harness change. It would also be good if we updated the description of the PR to describe this harness change.

jtibshirani · 2019-01-29T19:17:25Z

...framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java

+        // When running tests against a mixed 7.x/6.x cluster we need to add the type to the bulk API requests
+        // if its not already included. The type can either be on the request parameters or in the action metadata
+        // in the body of the request so we need to be sensitive to both scenarios
+        if (apiName.equals("bulk") && esVersion().before(Version.V_7_0_0) && requestParams.containsKey("type") == false) {


Sorry if I'm missing something, but instead of this list of checks, could we just take the simpler approach of always doing requestParams.put("type", "_doc") when the request param is not already there? In particular, I'm not sure why we would only put the type parameter if index is also specified.

We have to do this because in our tests we use both methods of specifying the index (and type) in a bulk request. Sometimes we set the index (and type) on the request params so its applied as a default and then the action metadata just contains {index:{}} or {index:{ _id: foo} and sometimes we don't specify the index (or type) on the request params and explicitly define them in the action metadata on each document we want to index. The problem with always setting the type on the request params is that if the index is not set on the request params the request will fail since its not valid to specify the type but not the index here.

+1 to the current approach of this PR

The way our request parsing works, you can actually specify the type without the index. In particular POST _bulk?type=_doc { ... } is a valid call.

It sounds like you and @jpountz are happy with this though, and this approach is sensible + straightforward, so I'm also happy if we want to merge.

I think the issue here though is that the type and index are actually used in the path rather than queryParams on the URL (it seems in this class the two are put in the same map). So specifying a type without an index would try the endpoint {type}/_bulk which is not valid. I actually did it the way you suggested initially and found I got errors when I specified the type without the test specifying the index.

As you are happy for me to merge I'll merge this now and have a closer look into this as a follow up

colings86 · 2019-01-30T10:37:00Z

@jpountz @jtibshirani thanks for the reviews. I pushed a fix for the code style issue and updated the PR description. I also replied to the question. Could you take another look?

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYaml TestExecutionContext.java

* master: Expose retention leases in shard stats (elastic#37991) Make primary terms fields private in index shard (elastic#38036) ML: Add reason field in JobTaskState (elastic#38029) Log flush_stats and commit_stats in testMaybeFlush HLRC: Fix strict setting exception handling (elastic#37247) Test: Enable strict deprecation on all tests (elastic#36558) Removes typed calls from YAML REST tests (elastic#37611) Switch default time format for ingest from Joda to Java for v7 (elastic#37934) Remove deprecated Plugin#onModule extension points (elastic#37866) Geo: Fix Empty Geometry Collection Handling (elastic#37978)

This PR removes the temporary change we made to the yml test harness in #37285 to automatically set `include_type_name` to `true` in index creation requests if it's not already specified. This is possible now that the vast majority of index creation requests were updated to be typeless in #37611. A few additional tests also needed updating here. Additionally, this PR updates the test harness to set `include_type_name` to `false` in index creation requests when communicating with 6.x nodes. This mirrors the logic added in #37611 to allow for typeless document write requests in test set-up code. With this update in place, we can remove many references to `include_type_name: false` from the yml tests.

colings86 added WIP :Search/Search Search-related issues that do not fall into other categories v7.0.0 >refactoring labels Jan 18, 2019

markharwood reviewed Jan 18, 2019

View reviewed changes

jtibshirani reviewed Jan 21, 2019

View reviewed changes

jtibshirani approved these changes Jan 22, 2019

View reviewed changes

jtibshirani mentioned this pull request Jan 22, 2019

Implementation tracking for 7.0 types deprecation. #35190

Closed

48 tasks

colings86 removed the WIP label Jan 23, 2019

jpountz approved these changes Jan 29, 2019

View reviewed changes

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/11_basic_with_types.yml Show resolved Hide resolved

...framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java Outdated Show resolved Hide resolved

jtibshirani reviewed Jan 29, 2019

View reviewed changes

talevy mentioned this pull request Jan 30, 2019

add basic REST test for geohash_grid #37996

Merged

colings86 added 18 commits January 30, 2019 11:06

Fixes tests in directory starting a to search

c537b98

the rest of the tests

82def67

addresses tests in plugins

b73f9ee

Addresses module rest tests

8c87b2a

review comments

7ab3edc

Fixes Tests

da8f57e

Fix tests

144c09b

Fixes tests

1c5d5a8

Trying to fix test

354a5b6

Fixes tests

e5c0cce

Fixes tests

0369bc1

Makes test harness insert type in mixed version tests

a89354e

Fixes tests and adds types to get requests

0cea5f7

Fixes tests

ae27a70

Fix Test

52f9252

Fix Test

0cc6bb0

fixup! Makes test harness insert type in mixed version tests

a20c1af

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYaml TestExecutionContext.java

Fixes style

9c8cbb9

colings86 merged commit 21e392e into elastic:master Jan 30, 2019

colings86 deleted the types-removal/yaml-tests branch January 30, 2019 16:33

jtibshirani mentioned this pull request Jan 30, 2019

Default include_type_name to false in the yml test harness. #38058

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes typed calls from YAML REST tests #37611

Removes typed calls from YAML REST tests #37611

colings86 commented Jan 18, 2019 •

edited

Loading

elasticmachine commented Jan 18, 2019

markharwood Jan 18, 2019

colings86 Jan 18, 2019

colings86 commented Jan 18, 2019

jtibshirani commented Jan 18, 2019

colings86 commented Jan 21, 2019

colings86 commented Jan 21, 2019

colings86 commented Jan 21, 2019

jtibshirani left a comment

colings86 commented Jan 22, 2019

colings86 commented Jan 22, 2019

jpountz commented Jan 22, 2019

colings86 commented Jan 22, 2019

jpountz commented Jan 22, 2019

jtibshirani left a comment

colings86 commented Jan 23, 2019

colings86 commented Jan 23, 2019

colings86 commented Jan 24, 2019

colings86 commented Jan 25, 2019

colings86 commented Jan 25, 2019

colings86 commented Jan 28, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

jpountz left a comment

jtibshirani left a comment •

edited

Loading

jtibshirani Jan 29, 2019 •

edited

Loading

colings86 Jan 30, 2019

jpountz Jan 30, 2019

jtibshirani Jan 30, 2019 •

edited

Loading

colings86 Jan 30, 2019

colings86 commented Jan 30, 2019

Removes typed calls from YAML REST tests #37611

Removes typed calls from YAML REST tests #37611

Conversation

colings86 commented Jan 18, 2019 • edited Loading

elasticmachine commented Jan 18, 2019

markharwood Jan 18, 2019

Choose a reason for hiding this comment

colings86 Jan 18, 2019

Choose a reason for hiding this comment

colings86 commented Jan 18, 2019

jtibshirani commented Jan 18, 2019

colings86 commented Jan 21, 2019

colings86 commented Jan 21, 2019

colings86 commented Jan 21, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

colings86 commented Jan 22, 2019

colings86 commented Jan 22, 2019

jpountz commented Jan 22, 2019

colings86 commented Jan 22, 2019

jpountz commented Jan 22, 2019

jtibshirani left a comment

Choose a reason for hiding this comment

colings86 commented Jan 23, 2019

colings86 commented Jan 23, 2019

colings86 commented Jan 24, 2019

colings86 commented Jan 25, 2019

colings86 commented Jan 25, 2019

colings86 commented Jan 28, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

colings86 commented Jan 29, 2019

jpountz left a comment

Choose a reason for hiding this comment

jtibshirani left a comment • edited Loading

Choose a reason for hiding this comment

jtibshirani Jan 29, 2019 • edited Loading

Choose a reason for hiding this comment

colings86 Jan 30, 2019

Choose a reason for hiding this comment

jpountz Jan 30, 2019

Choose a reason for hiding this comment

jtibshirani Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

colings86 Jan 30, 2019

Choose a reason for hiding this comment

colings86 commented Jan 30, 2019

colings86 commented Jan 18, 2019 •

edited

Loading

jtibshirani left a comment •

edited

Loading

jtibshirani Jan 29, 2019 •

edited

Loading

jtibshirani Jan 30, 2019 •

edited

Loading