Yaml Integration tests use deprecated paths

[brusic]

This issue revolves around the actual code against the master branch and not using Elasticsearch in general.

While working on a PR, I kept on experiencing errors during integration tests with tests involving action paths that were deprecated as part of the PR.

Here is an example of a deprecated path

{
  "get_script":{
    "url":{
      "paths":[
        {
          "path":"/_script/{id}",
          ...
        },
        {
          "path":"/_scripts/{id}",
          ...
          "deprecated":{
            "version":"8.0.0",
            "description":"The path is now singular"
          }
        }
      ]
    }
  }
}

Using LangPainlessClientYamlTestSuiteIT/painless/16_update2 as example. The test returns:

Failure at [painless/16_update2:12]: got unexpected warning header [
  299 Elasticsearch-8.0.0-SNAPSHOT-7610ef3242c5e63f8bd7545be5e2addb0feac688 "[GET /_scripts/{name}] is deprecated! Use [GET /_script/{id}] instead."
]

Removing the deprecated path in the json definition will fix the issue. Took me a while to figure out the issue. :) The deprecation definitions are useful and I would like to submit complete PRs with those definitions. Might be something I could fix, but Elastic PRs are never quick and I would like to finish the original PR first.

The JSON schema was updated today, but this issue occurred even before the change.

[brusic]

Perhaps not related, but the RestController is just screwy under integration tests.

"Create things in the cluster state that we'll validate are there after the upgrade":
  - do:
      put_script:
        id: test_search_template
        body:
          script:
            lang: mustache
            source:
              query:
                match:
                  f1: "{{f1}}"

results in:

WARNING: request [POST http://[::1]:55440/_script/test_search_template?error_trace=true] returned 1 warnings: [299 Elasticsearch-7.4.0-SNAPSHOT-a635eca5f83a7bbddc87f3640ad827294aabeeb0 "[types removal] Specifying types in document index requests is deprecated, use the typeless endpoints instead (/{index}/_doc/{id}, /{index}/_doc, or /{index}/_create/{id})."]

InvalidIndexNameException[Invalid index name [_script], must not start with '_', '-', or '+']

No issues with those endpoints running the server.

[javanna]

cc @elastic/es-clients

Howdy @brusic , what do you suggest we do about this problem you encountered with deprecated paths? Deprecated paths are still there and should be tested, what gets annoying is that we return deprecation warnings and in our tests we assert that there are no warnings, hence you need to kind of catch the warnings in your test. Would that work?

[elasticmachine]

Pinging @elastic/es-core-infra

[brusic]

I am in complete agreement that even deprecated paths should be tested, which is why I added the deprecation in the definition.

hence you need to kind of catch the warnings in your test

These are not my tests, but the existing tests in the elasticsearch codebase. The example above uses LangPainlessClientYamlTestSuiteIT. Any existing spec that puts a script will fail

  - do:
      put_script:
        id: "1"
        body: { "script": {"lang": "painless", "source": "_score * doc['myParent.weight'].value" } }
- match: { acknowledged: true }

I have not looked into the testing code or the way the REST endpoints are accumulated. When the specs look up the correct path for the appropriate action such as put_script, it seems multiple can be returned using the arguments provided. For id and body, it can be either /_script/{id} or /_scripts/{id}. If two or more are defined, drop the deprecated ones. If only one is defined and is deprecated, continue and catch the warning farther down, as it is occurring now.

[brusic]

Said I was not going to write a fix, but I'm doing it. The issue is mostly here

ClientYamlSuiteRestApi.Path path = RandomizedTest.randomFrom(bestPaths);

The best path should be among the non-deprecated ones. The integration tests are not attempting every path, just the "best" random one.

[javanna]

The integration tests are randomizing between all of the matching paths, based on the parameters that are provided. If we remove the deprecated ones from there, we end up not testing deprecated paths, it's like if they are not there?

[brusic]

Shouldn't there be isolated tests against the paths? In this case, the paths are not the focus of the tests, but the painless scripts are.

A better question would be: how can we can deprecate a path without breaking tests?

[brusic]

Will not create a PR since it sounds like the use of deprecated paths is wanted, but this makes the tests work

https://github.com/elastic/elasticsearch/compare/master...brusic:fix/best-non-deprecated-path?expand=1

Perhaps an allowed deprecated context can be added to the YAML Test client. Not going to work on this issue more for now.

[brusic]

At first I thought the issue was failing on tests whose code did not change. Since the GET/SCRIPT endpoints changed, they are used by multiple tests. But how come the removal of types do not trigger errors. It is because they are not marked as deprecated! In the original PR, I am fine with marking things as deprecated in the documentation and Javadoc, but leave the endpoints alone.

[jaymode]

The Core/Infra team discussed this issue in a team meeting. If the test client picks a deprecated path, then we need to ensure that the deprecation warning is asserted in the test. This validates that the path is actually deprecated and returning a warning while preventing developers from having to deal with issues like this one.