Remove comma-separated feature parsing for GetIndicesAction #24723

dakrone · 2017-05-16T21:50:57Z

This removes the parsing of things like GET /idx/_aliases,_mappings, instead,
a user must choose between retriving all index metadata with GET /idx, or only
a specific form such as GET /idx/_settings.

Relates to (and is a prerequisite of) #24437

dakrone · 2017-05-30T14:58:17Z

@jasontedor could you take a look at this? This is the PR you asked me to separate from #24437

pickypg · 2017-06-02T15:15:26Z

docs/reference/indices/get-index.asciidoc

-
-[source,js]
--------------------------------------------------
-GET twitter/_settings,_mappings


This could just be changed to

GET twitter?filter_path=*.settings,*.mappings

I think we could, but not sure it's worth it since I think this is a very minor edge case that people use.

abeyad

@dakrone I left some comments

abeyad · 2017-06-02T15:15:27Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllAliasesAction.java

+                            writeAliases(response.aliases().get(index), builder, request);
+                        }
+                        builder.endObject();
+


nit: extra newline

abeyad · 2017-06-02T15:15:46Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllMappingsAction.java

+                            writeMappings(response.mappings().get(index), builder);
+                        }
+                        builder.endObject();
+


nit: extra new line

abeyad · 2017-06-02T15:16:29Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllSettingsAction.java

+                            writeSettings(response.settings().get(index), builder, request, defaults);
+                        }
+                        builder.endObject();
+


nit: extra newline

abeyad · 2017-06-02T15:23:05Z

docs/reference/migration/migrate_6_0/rest.asciidoc

+
+==== Indices information APIs
+
+Previously it was possible to do `GET /_aliases,_mappings` or `GET


/s/do/execute

abeyad · 2017-06-02T15:36:34Z

docs/reference/migration/migrate_6_0/rest.asciidoc

+Previously it was possible to do `GET /_aliases,_mappings` or `GET
+/myindex/_settings,_alias` by separating mulitple types of requests with commas
+in order to retrieve multiple types of information about one or more indices.
+This comma-separation for multiple information retrieval has been removed. `GET


/s/multiple information retrieval/retrieving multiple pieces of information

abeyad · 2017-06-02T16:00:33Z

core/src/main/java/org/elasticsearch/rest/action/admin/indices/RestGetAllMappingsAction.java

+                                       final XContentBuilder builder) throws IOException {
+                builder.startObject("mappings");
+                {
+                    if (mappings != null) {


Under what circumstances would the mappings for an index be null (as opposed to an empty map)? It seems the default for GetIndexResponse is to always have an empty map for mappings (and aliases and settings) and it would only get assigned to a non-null map.

I don't think they ever would be, this is a carryover from when I copied this from the other method. I'll remove it.

abeyad · 2017-06-02T16:06:53Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

-  - do:
-      indices.get:
-        index: test_index
-        feature: _mapping


I'm not clear on why this test was eliminated, because we still support {index}/_mapping by its own without the comma-separated multi values?

This test was eliminated because it was for the indices.get API, whereas we already have tests for the indices.get_mapping API (in another file), so this was just a duplicate test.

abeyad · 2017-06-02T16:07:03Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml

-  - do:
-      indices.get:
-        index: test_blias
-        feature: _mapping


Ditto to the comment above

Same about the duplicated test

abeyad · 2017-06-02T16:07:12Z

rest-api-spec/src/main/resources/rest-api-spec/test/indices.get/10_basic.yml


 ---
 "Get index infos with human settings should return index creation date and version in readable format":

  - do:
      indices.get:
        index: test_index
-        feature: _settings


Ditto to the comment above

Same about the duplicated test :)

abeyad · 2017-06-02T16:08:56Z

rest-api-spec/src/main/resources/rest-api-spec/api/indices.get.json

@@ -4,17 +4,12 @@
    "methods":[ "GET" ],
    "url":{
      "path":"/{index}",
-      "paths":[ "/{index}", "/{index}/{feature}" ],


Should we also add rest specs and yaml tests for the new REST endpoints introduced in this PR?

Interestingly enough, we already do have these endpoints in the spec, they just weren't in this file :)

See: https://github.com/dakrone/elasticsearch/blob/remove-comma-parsing-of-features/rest-api-spec/src/main/resources/rest-api-spec/api/indices.get_mapping.json#L7

dakrone · 2017-06-02T17:05:43Z

Thanks @abeyad, I pushed some commits addressing your feedback

abeyad

LGTM, thanks @dakrone

This removes the parsing of things like `GET /idx/_aliases,_mappings`, instead, a user must choose between retriving all index metadata with `GET /idx`, or only a specific form such as `GET /idx/_settings`. Relates to (and is a prerequisite of) elastic#24437

dakrone · 2017-06-02T20:48:12Z

thank you @abeyad :)

javanna · 2017-06-02T22:04:26Z

Are we going to also deprecate this in 5.x?

dakrone · 2017-06-02T22:08:33Z

Are we going to also deprecate this in 5.x?

Good idea, I'll open a PR

This is the deprecation logging and documentation for 5.x related to elastic#24723. It logs when a user does a request like: ``` GET /index/_alias,_mapping ```

Relates to #24723

* Add deprecation logging for comma-separated feature parsing This is the deprecation logging and documentation for 5.x related to #24723. It logs when a user does a request like: ``` GET /index/_alias,_mapping ```

Previously this would output: ``` GET /test-1/_mappings { } ``` And after this change: ``` GET /test-1/_mappings { "test-1": { "mappings": {} } } ``` To bring parity back to the REST output after elastic#24723. Relates to elastic#25090

Previously in elastic#24723 we changed the `_alias` API to not go through the `RestGetIndicesAction` endpoint, instead creating a `RestGetAliasesAction` that did the same thing. This changes the formatting so that it matches the old formatting of the endpoint, before: ``` GET /test-1/_alias { } ``` And after this change: ``` GET /test-1/_alias { "test-1": { "aliases": {} } } ``` This is related to elastic#25090

Previously this would output: ``` GET /test-1/_mappings { } ``` And after this change: ``` GET /test-1/_mappings { "test-1": { "mappings": {} } } ``` To bring parity back to the REST output after #24723. Relates to #25090

Follow-up of elastic#24723

Removes dead code. Follow-up of #24723

dakrone added :Core/Infra/REST API REST infrastructure and utilities >breaking v6.0.0 labels May 16, 2017

dakrone requested a review from jasontedor May 16, 2017 21:50

dakrone force-pushed the remove-comma-parsing-of-features branch 3 times, most recently from 8211d12 to 3ac55ad Compare May 18, 2017 20:15

abeyad self-requested a review June 2, 2017 15:09

pickypg reviewed Jun 2, 2017

View reviewed changes

abeyad suggested changes Jun 2, 2017

View reviewed changes

abeyad approved these changes Jun 2, 2017

View reviewed changes

dakrone force-pushed the remove-comma-parsing-of-features branch 2 times, most recently from 7577dc9 to 3581e37 Compare June 2, 2017 20:40

dakrone force-pushed the remove-comma-parsing-of-features branch from 3581e37 to a32d1b9 Compare June 2, 2017 20:43

dakrone merged commit a32d1b9 into elastic:master Jun 2, 2017

dakrone mentioned this pull request Jun 2, 2017

Add deprecation logging for comma-separated feature parsing #25040

Merged

dakrone added a commit that referenced this pull request Jun 3, 2017

[TEST] Skip wildcard expansion test due to breaking change

134b0d5

Relates to #24723

spalger mentioned this pull request Jun 7, 2017

_alias API no longer accepts index wildcards #25090

Closed

dakrone mentioned this pull request Jun 7, 2017

Return index name and empty map for /{index}/_alias with no aliases #25114

Merged

dakrone mentioned this pull request Jun 7, 2017

Include empty mappings in GET /{index}/_mappings requests #25118

Merged

clintongormley added v6.0.0-beta1 and removed v6.0.0 labels Jul 25, 2017

dakrone deleted the remove-comma-parsing-of-features branch December 13, 2017 20:37

ywelsch added a commit to ywelsch/elasticsearch that referenced this pull request Feb 6, 2018

Remove feature parsing for GetIndicesAction

3c882f5

Follow-up of elastic#24723

ywelsch mentioned this pull request Feb 6, 2018

Remove feature parsing code #28535

Merged

ywelsch added a commit that referenced this pull request Feb 6, 2018

Remove feature parsing for GetIndicesAction (#28535)

e6f873c

Removes dead code. Follow-up of #24723

ywelsch added a commit that referenced this pull request Feb 6, 2018

Remove feature parsing for GetIndicesAction (#28535)

b225ed5

Removes dead code. Follow-up of #24723

javanna mentioned this pull request Feb 20, 2018

Refactor GET {index} vs GET {index}/_settings etc #16053

Closed


		==== Indices information APIs

		Previously it was possible to do `GET /_aliases,_mappings` or `GET

Remove comma-separated feature parsing for GetIndicesAction #24723

Remove comma-separated feature parsing for GetIndicesAction #24723

Uh oh!

Conversation

dakrone commented May 16, 2017

Uh oh!

dakrone commented May 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dakrone commented Jun 2, 2017

Uh oh!

abeyad left a comment

Choose a reason for hiding this comment

Uh oh!

dakrone commented Jun 2, 2017

Uh oh!

javanna commented Jun 2, 2017

Uh oh!

dakrone commented Jun 2, 2017

Uh oh!

Uh oh!