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

rest-api-spec : setting path parts as required to reflect the code base #27263

Merged
merged 1 commit into from
Nov 6, 2017
Merged

rest-api-spec : setting path parts as required to reflect the code base #27263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/api/indices.exists_alias.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
},
"name": {
"type" : "list",
"required" : true,
"description" : "A comma-separated list of alias names to return"
}
},
Expand Down
1 change: 1 addition & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/api/reindex_rethrottle.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"parts": {
"task_id": {
"type": "string",
"required" : true,
"description": "The task id to rethrottle"
}
},
Expand Down
1 change: 1 addition & 0 deletions rest-api-spec/src/main/resources/rest-api-spec/api/tasks.get.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"parts": {
"task_id": {
"type": "string",
"required" : true,
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually required, you can do GET /_tasks no problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dakrone you are correct! But when you call GET /_tasks without task_id, it is mapped to :

elasticsearch/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestListTasksAction.java

Lines 52 to 56 in 3deba0e

public RestListTasksAction(Settings settings, RestController controller, Supplier<DiscoveryNodes> nodesInCluster) {
super(settings);
this.nodesInCluster = nodesInCluster;
controller.registerHandler(GET, "/_tasks", this);
}
defined by tasks.list in the rest-api-specs.

tasks.get is for :

elasticsearch/core/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestGetTaskAction.java

Lines 37 to 40 in 3deba0e

public RestGetTaskAction(Settings settings, RestController controller) {
super(settings);
controller.registerHandler(GET, "/_tasks/{task_id}", this);
}

Copy link
Member

Choose a reason for hiding this comment

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

That's true, and there is the other .json spec file that mentions solely /_tasks

Hmm... I'm not sure what the best thing for this situation is, perhaps leaving it as required is the best since it only pertains to this particular API (not tasks.list.json)

Copy link
Member

Choose a reason for hiding this comment

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

are there downsides for this change in the spec? I think it makes sense in the context of the get task API.

Copy link
Member

Choose a reason for hiding this comment

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

@javanna yeah I think it does too, I just wanted to think about it for a little bit. I'll merge this.

"description": "Return the task with specified id (node_id:task_number)"
}
},
Expand Down