Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issues with the json description of Snapshot Create API #11897

Closed
szroland opened this issue Jun 26, 2015 · 5 comments · Fixed by #11928
Closed

Issues with the json description of Snapshot Create API #11897

szroland opened this issue Jun 26, 2015 · 5 comments · Fixed by #11928

Comments

@szroland
Copy link
Contributor

In general, Rest test code picks the HTTP method and path randomly from the API json spec. Not sure what other uses of this specification exist, but this indicates an assumption that if there are multiple methods available, they are all available on all urls (that match the parameters) with the same semantics.

However, this breaks for snapshot create, where the POST and PUT URLs in the API are different, there is an extra _create for POST only:

        controller.registerHandler(PUT, "/_snapshot/{repository}/{snapshot}", this);
        controller.registerHandler(POST, "/_snapshot/{repository}/{snapshot}/_create", this);

The API spec in json:

    "methods": ["PUT", "POST"],
    "paths": [
      "/_snapshot/{repository}/{snapshot}", 
      "/_snapshot/{repository}/{snapshot}/_create"],

So if you tried to use this API in rest tests (which might explain why no such tests currently exist), sometimes RestClient actually picks the wrong combination and does a PUT to /_snapshot/{repository}/{snapshot}/_create which happens to get parsed as a create index request (and fails because of the invalid index name _snapshot, which could indicate some issue in the dispatch code as well).

So there are a couple of things that could be done:

  • Change the Snapshot Create API to have PUT and POST both on all the same paths, in line with other APIs (e.g. add PUT and POST to both, assuming request dispatching could support that, or drop "_create" from the POST path)
  • Change the json API spec format to allow the linking of method to path
  • Create 2 separate json files for snapshot create, one describing the PUT api and the other the POST api (you would lose the random selection between the methods in tests, and would pick e.g. between snapshot.create or snapshot.create.post in your test descriptor)

I guess the best course of action depends on what uses of the json specification there are, what it is really intended for.

Dropping POST/_create, at least from the spec file, seems to be an interesting option, since I see this has been done on other APIs as well (e.g. search_template and put_script), which have a POST/_create mapping in code but not in the json spec. I guess the issue might be that the path also matches the create index action path /{index}/{type}/{id}/_create, not sure if the dispatch logic deterministically considers the create index option last (e.g. prioritizes more specific paths over less specific ones).

szroland added a commit to szroland/elasticsearch that referenced this issue Jun 27, 2015
…o address elastic#11897. This allows using this API in Rest Tests. There is also precedent for the spec file not including the POST / _create option, like in search_template, put_script.
@bleskes
Copy link
Contributor

bleskes commented Jun 29, 2015

Change the Snapshot Create API to have PUT and POST both on all the same paths, in line with other APIs (e.g. add PUT and POST to both, assuming request dispatching could support that, or drop "_create" from the POST path)

Let's do this. As you say it's consistent with other api (for example, search templates)

@clintongormley
Copy link

.../_create isn't even documented. I'd go for just dropping that URL from the spec.

@bleskes
Copy link
Contributor

bleskes commented Jun 29, 2015

@clintongormley works for me, but let's remove it from the code as well (2.0 only). We should still support POST (at least in the code level) for /_snapshot/{repository}/{snapshot} .

@clintongormley
Copy link

To be clear: support PUT and POST, but drop the _create endpoint.

@szroland
Copy link
Contributor Author

Added a pull request, please see #11928

bleskes added a commit that referenced this issue Jun 30, 2015
Create Snapshot: remove _create from POST path to match PUT

Closes #11928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants