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

Handle multiple aliases in _cat/aliases api #23698

Merged
merged 14 commits into from Apr 28, 2017

Conversation

@glefloch
Contributor

glefloch commented Mar 22, 2017

This pull request aims to handle a list of alias in GetAliasRequest instead of a single one.

Closes #23661

@elasticmachine

This comment has been minimized.

elasticmachine commented Mar 22, 2017

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jasontedor

This comment has been minimized.

Member

jasontedor commented Mar 22, 2017

Thanks for the PR, but we'll need tests here; namely, tests that would reveal the broken behavior before your change (which at a superficial glance looks right), and tests that will pass after your change.

@jasontedor jasontedor self-requested a review Mar 22, 2017

@glefloch

This comment has been minimized.

Contributor

glefloch commented Mar 22, 2017

Yes for sure, I 'am gonna add that to this pull request

@glefloch

This comment has been minimized.

Contributor

glefloch commented Mar 28, 2017

Hi, I looked at the code in order to develop some tests. I'm kind of stuck, I would like to assert the result of the doCatRequest wich return a RestChannelConsumer object. But I d'ont have access to that class. Are there any available test classes that allow to assert Rest response?

@jasontedor

This comment has been minimized.

Member

jasontedor commented Mar 28, 2017

The simplest option would be to write a REST test; look in rest-api-spec where there are tests for similar endpoints that you can base your work from.

@jasontedor

Thanks @glefloch, this is a good start, I left some comments.

@@ -78,7 +78,8 @@ protected Table getTableWithHeader(RestRequest request) {
return table;
}
private Table buildTable(RestRequest request, GetAliasesResponse response) {
// package private for testing
Table buildTable(RestRequest request, GetAliasesResponse response) {

This comment has been minimized.

@jasontedor

jasontedor Apr 4, 2017

Member

Can you revert this change, I do not see any tests added that rely on the visibility being changed?

This comment has been minimized.

@glefloch

glefloch Apr 4, 2017

Contributor

Yes I'm gonna revert this change

If you only want to get information about a single alias, you can specify
the alias in the URL, for example `/_cat/aliases/alias1`.
If you only want to get information about a single or multiple aliases, you can specify
the alias in the URL, for example `/_cat/aliases/alias1` or `/_cat/aliases/alias1,alias2`.

This comment has been minimized.

@jasontedor

jasontedor Apr 4, 2017

Member

I wonder if rewording this to the following is simpler?

If you only want to get information about specific aliases, you can specify the aliases in comma-delimited format as the last component of the URL path, e.g., /_cat/aliases/aliases/alias1,alias2.

- match:
$body: /(^|\n)bar\s+test2 /

This comment has been minimized.

@jasontedor

jasontedor Apr 4, 2017

Member

I think that this test should add a third index, and a third alias, and not request that alias (as you've already done) just so that we can be sure that a non-requested alias is not returned.

This comment has been minimized.

@glefloch

glefloch Apr 4, 2017

Contributor

I will do that

glefloch added some commits Apr 4, 2017

@@ -46,7 +46,7 @@ public RestAliasAction(Settings settings, RestController controller) {
@Override
protected RestChannelConsumer doCatRequest(final RestRequest request, final NodeClient client) {
final GetAliasesRequest getAliasesRequest = request.hasParam("alias") ?
new GetAliasesRequest(request.param("alias")) :
new GetAliasesRequest(request.param("alias").split(",")) :

This comment has been minimized.

@olcbean

olcbean Apr 11, 2017

Contributor

I think we should consider changing the parameter from alias to aliases as it is misleading to pack a list in alias.

@@ -46,7 +46,7 @@ public RestAliasAction(Settings settings, RestController controller) {
@Override
protected RestChannelConsumer doCatRequest(final RestRequest request, final NodeClient client) {
final GetAliasesRequest getAliasesRequest = request.hasParam("alias") ?
new GetAliasesRequest(request.param("alias")) :
new GetAliasesRequest(request.param("alias").split(",")) :

This comment has been minimized.

@javanna

javanna Apr 18, 2017

Member

you could use Strings.commaDelimitedListToStringArray here as we do in other places.

This comment has been minimized.

@glefloch

glefloch Apr 18, 2017

Contributor

I didn't knew about this class. I will use it.

@javanna

This comment has been minimized.

Member

javanna commented Apr 18, 2017

jenkins please test this

@javanna

This comment has been minimized.

Member

javanna commented Apr 18, 2017

retest this please

@glefloch

This comment has been minimized.

Contributor

glefloch commented Apr 19, 2017

@javanna the build has failed, but it seems like not tests are failed. Is there anything I can do to fix this?

@javanna javanna changed the title from Handle multiple aliases in GetAliasRequest to Handle multiple aliases in _cat/aliases api Apr 19, 2017

@javanna

This comment has been minimized.

Member

javanna commented Apr 19, 2017

@glefloch I think there may be a problem with backwards compatibility tests, as the new yaml test will not work against previous versions. It would need a skip section. I think we can take care of it when merging this in. @jasontedor are you ok with the state of this PR?

@jasontedor

This comment has been minimized.

Member

jasontedor commented Apr 19, 2017

@javanna We saw similar build failures when we bumped versions in 5.x but not in master. I think that master needs to be merged into this PR branch to pick that up and then the PR CI can run again.

@javanna

This comment has been minimized.

Member

javanna commented Apr 19, 2017

ok @glefloch can you merge master in please?

@glefloch

This comment has been minimized.

Contributor

glefloch commented Apr 19, 2017

@javanna I merged it

@javanna

This comment has been minimized.

Member

javanna commented Apr 19, 2017

jenkins retest this please

@javanna

This comment has been minimized.

Member

javanna commented Apr 19, 2017

@glefloch there's one test failure, as I mentioned above, around backwards compatibility. We run the yaml tests against previous versions to test backwards compatibility on the REST layer. Your updated test doesn't lead to the same result when run against previous versions that don't contain your fix. This is expected, could you add for now the following skip section to your newly added test please?

  - skip:
      version: " - 5.99.99"
      reason:  multiple aliases are supported only from 6.0.0 on

This way tests should go green and we can merge the PR. Later we will backport your fix and we will have to update the version in the skip section, both in master and 5.x. Let me know if you have any question about this.

@glefloch

This comment has been minimized.

Contributor

glefloch commented Apr 19, 2017

@javanna, I add the skip section. All tests passed locally.

@javanna

This comment has been minimized.

Member

javanna commented Apr 19, 2017

jenkins retest this please

@jasontedor

Almost there, thanks for all your work on this.

$body: /(^|\n)bar\s+test2 /
- match:
$body: /(^|\n)[^bar\s+test3] /

This comment has been minimized.

@jasontedor

jasontedor Apr 20, 2017

Member

I think we can and should make a stronger assertion here. We can request only the alias and index fields, request that the cat API sort the output on the index field, and then assert the exact expected output:

foo test
bar test2
@glefloch

This comment has been minimized.

Contributor

glefloch commented Apr 20, 2017

@jasontedor No problem ;) I'm going to do it.

@jasontedor

One more thing. 😄

test
\n
bar \s+
test2

This comment has been minimized.

@jasontedor

jasontedor Apr 20, 2017

Member

I think this would be clearer if you put all the pieces that belong on a line together on the same line.

@jasontedor

This comment has been minimized.

Member

jasontedor commented Apr 21, 2017

retest this please

@jasontedor

LGTM.

@javanna javanna merged commit 382a617 into elastic:master Apr 28, 2017

2 checks passed

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

javanna added a commit that referenced this pull request Apr 28, 2017

Handle multiple aliases in _cat/aliases api (#23698)
The alias parameter was documented as a list in our rest-spec, yet only the first value out of a list was getting read and processed. This commit adds support for multiple aliases to _cat/aliases

Closes #23661
@javanna

This comment has been minimized.

Member

javanna commented Apr 28, 2017

Merged to master and cherry-picked to 5.x. Thanks a lot @glefloch

@jasontedor

This comment has been minimized.

Member

jasontedor commented Apr 28, 2017

Indeed, thank you @glefloch and thank you @javanna for managing the integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment