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.

Sign up for GitHub

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

Jump to bottom

exists template needs a template name #25988

Merged
merged 1 commit into from
Oct 27, 2017
Merged

exists template needs a template name #25988

merged 1 commit into from
Oct 27, 2017

Conversation

honzakral
Copy link
Contributor

running curl -X HEAD localhost:9200/_template results in 405 Method Not Allowed

@honzakral honzakral added the :Core/Infra/REST API REST infrastructure and utilities label Jul 31, 2017
jasontedor
jasontedor requested changes Aug 1, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The flat_settings parameter should remain, it is supported on HEAD template requests.

rest-api-spec/src/main/resources/rest-api-spec/api/indices.exists_template.json Outdated
"description": "The comma separated names of the index templates"
}
},
"params": {
"flat_settings": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flat_settings is supported on HEAD requests; the behavior of GET and HEAD are aligned when both methods are supported.

rest-api-spec/src/main/resources/rest-api-spec/api/indices.exists_template.json
@@ -8,15 +8,11 @@
"parts": {
"name": {
"type": "list",
"required": false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is correct.

@honzakral
Copy link
Contributor Author

Thanks for the review, updated the commit. Out of curiosity, what does flat_settings do for HEAD request?

@jasontedor
Copy link
Member

Out of curiosity, what does flat_settings do for HEAD request?

Per the HTTP specification, when an endpoint supports both the HEAD and GET methods, the response to a HEAD request should be the same as a GET request to the same endpoint (including the same parameters) except that the body should be empty (to be explicit: this means that the Content-Length header should be the same as if a HEAD request had been sent). The way that we implement HEAD requests on endpoints that support them is to treat the request as if it was a GET request, and then before sending the request to the client, strip the body from the response. This means that a parameter like flat_settings on a HEAD request will change the content of the reply that would have come if the method was a GET method, thus changing the content length, thus changing the value of the Content-Length header that is included in the reply to the HEAD request.

jasontedor
jasontedor approved these changes Aug 1, 2017
View reviewed changes
Copy link
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@cbuescher
Copy link
Member

@honzakral just going through older open PRs, seems like this one is ready to merge. Do you want to go ahead or should we do it?

@honzakral
Copy link
Contributor Author

@cbuescher, please go ahead, I am not up-to-date with all the necessary backporting policies and other steps.

Thank you!

@cbuescher cbuescher merged commit 1dd6aee into elastic:master Oct 27, 2017
@cbuescher
Copy link
Member

@honzakral not entirely sure how rest-api-specs are used, but I suppose this change should be applied to all active branches down to 5.6?

@cbuescher cbuescher removed the >bug label Oct 27, 2017
@honzakral
Copy link
Contributor Author

I'd say yes, but not sure what is the current status of freezes/branch policies etc. That is why I try to stay away from merging patches to es myself since I am no longer familiar enough with the process...

cbuescher pushed a commit that referenced this pull request Oct 27, 2017
@honzakral @cbuescher
Exists template needs a template name (#25988)
4bacb60
cbuescher pushed a commit that referenced this pull request Oct 27, 2017
@honzakral @cbuescher
Exists template needs a template name (#25988)
119515d
cbuescher pushed a commit that referenced this pull request Oct 27, 2017
@honzakral @cbuescher
Exists template needs a template name (#25988)
dbc7f22
@olcbean olcbean mentioned this pull request Oct 28, 2017
Closed
@lcawl lcawl added v6.0.0-rc2 and removed v6.0.0 labels Oct 30, 2017
jasontedor added a commit to olcbean/elasticsearch that referenced this pull request Oct 30, 2017
@jasontedor
Merge branch 'master' into pr/23767
0744b44 
* master: (63 commits)
  [Docs] Fix note in bucket_selector
  [Docs] Fix indentation of examples (elastic#27168)
  [Docs] Clarify `span_not` query behavior for non-overlapping matches (elastic#27150)
  [Docs] Remove first person "I" from getting started (elastic#27155)
  [Docs] Correct link target for datatype murmur3 (elastic#27143)
  Fix division by zero in phrase suggester that causes assertion to fail
  Enable Docstats with totalSizeInBytes for 6.1.0
  Adds average document size to DocsStats (elastic#27117)
  Upgrade Painless from ANTLR 4.5.1-1 to  ANTLR 4.5.3. (elastic#27153)
  Exists template needs a template name (elastic#25988)
  [Tests] Fix occasional test failure due to two random values being the same
  Fix beidermorse phonetic token filter for unspecified `languageset` (elastic#27112)
  Fix max score tracking with field collapsing (elastic#27122)
  [Doc] Add Ingest CSV Processor Plugin to plugin as a community plugin (elastic#27105)
  Removed the beta tag from cross-cluster search
  fixed typo in ConstructingObjectParse (elastic#27129)
  Allow for the Painless Definition to have multiple instances (elastic#27096)
  Apply missing request options to the expand phase (elastic#27118)
  Only pull SegmentReader once in getSegmentInfo (elastic#27121)
  Fix BWC for discovery stats
  ...
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasontedor jasontedor jasontedor approved these changes

Assignees
No one assigned
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities v5.6.4 v6.0.0-rc2 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@honzakral @jasontedor @cbuescher @clintongormley @jimczi @lcawl