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

Removing query-string parameters in `_analyze` API #20704

Merged
merged 4 commits into from Oct 27, 2016

Conversation

Projects
None yet
3 participants
@johtani
Copy link
Member

commented Sep 30, 2016

Remove request params in _analyze API without index param
Change rest-api-test using JSON
Change docs using JSON

Closes #20246

@johtani

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2016

I will add breaking changes in migration doc.

@dadoonet
Copy link
Member

left a comment

I left a small comment about imports.

BTW, thinking out loud and not really related to your PR, I wonder if we should document the GET verb instead of using POST as we are doing for _search endpoint. We don't change the server state when calling _analyze so GET makes more sense to me from a REST semantic point of view.

core/src/test/java/org/elasticsearch/rest/action/admin/indices/RestAnalyzeActionTests.java Outdated

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.startsWith;
import static org.mockito.Mockito.doCallRealMethod;

This comment has been minimized.

Copy link
@dadoonet

dadoonet Sep 30, 2016

Member

Lot of imports have been added here. Are they really needed?

This comment has been minimized.

Copy link
@johtani

johtani Sep 30, 2016

Author Member

Good catch! That is no need. I will check and fix.

@johtani

This comment has been minimized.

Copy link
Member Author

commented Sep 30, 2016

@dadoonet Thx for reviewing. I fixed the issues you mentioned and add breaking changes.

@dadoonet
Copy link
Member

left a comment

Left a minor comment. It looks good to me.

docs/reference/migration/migrate_6_0/rest.asciidoc Outdated

==== Analyze API changes

The deprecated request parameters and plain text in request body has been removed, use JSON in request body.

This comment has been minimized.

Copy link
@dadoonet

dadoonet Sep 30, 2016

Member

"...have been removed. Use..." instead.

This comment has been minimized.

Copy link
@javanna

javanna Oct 3, 2016

Member

can we also be more generic when mentioning the request body? We don't support only the json format.

@johtani

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2016

@clintongormley Question about rest-api-spec. In this PR, I have removed request params. Should I remove the params in rest-api-spec ?
I'm not sure the difference between JSON param and request params in the spec.

@javanna

This comment has been minimized.

Copy link
Member

commented Oct 3, 2016

@johtani we don't document in our spec what should go within the request body, we only define whether a body is required or not. The params section in the spec is for query_string parameters, which are what you removed, hence that section should be adapted ;)

@johtani

This comment has been minimized.

Copy link
Member Author

commented Oct 5, 2016

@javanna Thanks for you comment. I removed params in spec json file and updated REST changes message.

johtani added some commits Sep 22, 2016

Removing request parameters in _analyze API
Remove request params in _analyze API without index param
Change rest-api-test using JSON
Change docs using JSON

Closes #20246
Removing request parameters in _analyze API
Remove unused imports
Replace POST method by GET method in docs
 Add breaking changes explanation
 Fix small issue in Kuromoji docs

Closes #20246
Removing request parameters in _analyze API
Fix small English issue in breaking changes

Closes #20246
Deprecating request parameters in _analyze API
Remove params in indices.analyze.json
Fix REST changes

Closes #20246

@johtani johtani force-pushed the johtani:remove_request_params_in_analyze_api branch to 945fa49 Oct 7, 2016

@johtani

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2016

Rebased master

@javanna
Copy link
Member

left a comment

LGTM

@johtani johtani merged commit a66c76e into elastic:master Oct 27, 2016

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details
@johtani

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2016

@javanna @dadoonet Thx for the review!

@clintongormley clintongormley changed the title Removing request parameters in _analyze API Removing query-string parameters in `_analyze` API May 5, 2017

@estolfo estolfo referenced this pull request Oct 24, 2018

Closed

url path need encode #528

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.