Skip to content

Commit

Permalink
Make yaml test runner stricter by enforcing required for paths and …
Browse files Browse the repository at this point in the history 
…parameters (#27035)

Till now the yaml test runner was verifying that the provided path parts and parameters are supported.
With this PR, yaml test runner also checks that all required path parts and parameters are provided.
  • Loading branch information
@olcbean @nik9000
olcbean authored and nik9000 committed Oct 25, 2017
1 parent 6533b16 commit 981b7f4
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 33 deletions.
38 changes: 29 additions & 9 deletions test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.elasticsearch.test.rest.yaml;

import com.carrotsearch.randomizedtesting.RandomizedTest;

import org.apache.http.Header;
import org.apache.http.HttpEntity;
import org.apache.http.HttpHost;
Expand All @@ -42,6 +43,9 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.stream.Collectors;

/**
* Used by {@link ESClientYamlSuiteTestCase} to execute REST requests according to the tests written in yaml suite files. Wraps a
Expand Down Expand Up @@ -80,24 +84,40 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> params
//divide params between ones that go within query string and ones that go within path
Map<String, String> pathParts = new HashMap<>();
Map<String, String> queryStringParams = new HashMap<>();

Set<String> apiRequiredPathParts = restApi.getPathParts().entrySet().stream().filter(e -> e.getValue() == true).map(Entry::getKey)
.collect(Collectors.toSet());
Set<String> apiRequiredParameters = restApi.getParams().entrySet().stream().filter(e -> e.getValue() == true).map(Entry::getKey)
.collect(Collectors.toSet());

for (Map.Entry<String, String> entry : params.entrySet()) {
if (restApi.getPathParts().contains(entry.getKey())) {
if (restApi.getPathParts().containsKey(entry.getKey())) {
pathParts.put(entry.getKey(), entry.getValue());
apiRequiredPathParts.remove(entry.getKey());
} else if (restApi.getParams().containsKey(entry.getKey())
|| restSpec.isGlobalParameter(entry.getKey())
|| restSpec.isClientParameter(entry.getKey())) {
queryStringParams.put(entry.getKey(), entry.getValue());
apiRequiredParameters.remove(entry.getKey());
} else {
if (restApi.getParams().contains(entry.getKey()) || restSpec.isGlobalParameter(entry.getKey())
|| restSpec.isClientParameter(entry.getKey())) {
queryStringParams.put(entry.getKey(), entry.getValue());
} else {
throw new IllegalArgumentException("param [" + entry.getKey() + "] not supported in ["
+ restApi.getName() + "] " + "api");
}
throw new IllegalArgumentException(
"path/param [" + entry.getKey() + "] not supported by [" + restApi.getName() + "] " + "api");
}
}

if (false == apiRequiredPathParts.isEmpty()) {
throw new IllegalArgumentException(
"missing required path part: " + apiRequiredPathParts + " by [" + restApi.getName() + "] api");
}
if (false == apiRequiredParameters.isEmpty()) {
throw new IllegalArgumentException(
"missing required parameter: " + apiRequiredParameters + " by [" + restApi.getName() + "] api");
}

List<String> supportedMethods = restApi.getSupportedMethods(pathParts.keySet());
String requestMethod;
if (entity != null) {
if (!restApi.isBodySupported()) {
if (false == restApi.isBodySupported()) {
throw new IllegalArgumentException("body is not supported by [" + restApi.getName() + "] api");
}
String contentType = entity.getContentType().getValue();
Expand Down
25 changes: 17 additions & 8 deletions ...ework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApi.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.apache.http.client.methods.HttpPut;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -35,8 +36,8 @@ public class ClientYamlSuiteRestApi {
private final String name;
private List<String> methods = new ArrayList<>();
private List<String> paths = new ArrayList<>();
private List<String> pathParts = new ArrayList<>();
private List<String> params = new ArrayList<>();
private Map<String, Boolean> pathParts = new HashMap<>();
private Map<String, Boolean> params = new HashMap<>();
private Body body = Body.NOT_SUPPORTED;

public enum Body {
Expand Down Expand Up @@ -98,20 +99,28 @@ void addPath(String path) {
this.paths.add(path);
}

public List<String> getPathParts() {
/**
* Gets all path parts supported by the api. For every path part defines if it
* is required or optional.
*/
public Map<String, Boolean> getPathParts() {
return pathParts;
}

void addPathPart(String pathPart) {
this.pathParts.add(pathPart);
void addPathPart(String pathPart, boolean required) {
this.pathParts.put(pathPart, required);
}

public List<String> getParams() {
/**
* Gets all parameters supported by the api. For every parameter defines if it
* is required or optional.
*/
public Map<String, Boolean> getParams() {
return params;
}

void addParam(String param) {
this.params.add(param);
void addParam(String param, boolean required) {
this.params.put(param, required);
}

void setBodyOptional() {
Expand Down
32 changes: 24 additions & 8 deletions ...src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.elasticsearch.test.rest.yaml.restspec;

import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;
Expand All @@ -27,6 +29,11 @@
*/
public class ClientYamlSuiteRestApiParser {

private static final ObjectParser<Parameter, Void> PARAMETER_PARSER = new ObjectParser<>("parameter", true, Parameter::new);
static {
PARAMETER_PARSER.declareBoolean(Parameter::setRequired, new ParseField("required"));
}

public ClientYamlSuiteRestApi parse(String location, XContentParser parser) throws IOException {

while ( parser.nextToken() != XContentParser.Token.FIELD_NAME ) {
Expand Down Expand Up @@ -57,7 +64,6 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro
if (parser.currentToken() == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
}

if (parser.currentToken() == XContentParser.Token.START_ARRAY && "paths".equals(currentFieldName)) {
while (parser.nextToken() == XContentParser.Token.VALUE_STRING) {
String path = parser.text();
Expand All @@ -71,30 +77,30 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro
if (parser.currentToken() == XContentParser.Token.START_OBJECT && "parts".equals(currentFieldName)) {
while (parser.nextToken() == XContentParser.Token.FIELD_NAME) {
String part = parser.currentName();
if (restApi.getPathParts().contains(part)) {
if (restApi.getPathParts().containsKey(part)) {
throw new IllegalArgumentException("Found duplicate part [" + part + "]");
}
restApi.addPathPart(part);
parser.nextToken();
if (parser.currentToken() != XContentParser.Token.START_OBJECT) {
throw new IllegalArgumentException("Expected parts field in rest api definition to contain an object");
}
parser.skipChildren();
restApi.addPathPart(part, PARAMETER_PARSER.parse(parser, null).isRequired());
}
}

if (parser.currentToken() == XContentParser.Token.START_OBJECT && "params".equals(currentFieldName)) {
while (parser.nextToken() == XContentParser.Token.FIELD_NAME) {

String param = parser.currentName();
if (restApi.getParams().contains(param)) {
if (restApi.getParams().containsKey(param)) {
throw new IllegalArgumentException("Found duplicate param [" + param + "]");
}
restApi.addParam(parser.currentName());

parser.nextToken();
if (parser.currentToken() != XContentParser.Token.START_OBJECT) {
throw new IllegalArgumentException("Expected params field in rest api definition to contain an object");
}
parser.skipChildren();
restApi.addParam(param, PARAMETER_PARSER.parse(parser, null).isRequired());
}
}

Expand Down Expand Up @@ -124,7 +130,7 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro
}
}
}
if (!requiredFound) {
if (false == requiredFound) {
restApi.setBodyOptional();
}
}
Expand All @@ -146,4 +152,14 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro

return restApi;
}

private static class Parameter {
private boolean required;
public boolean isRequired() {
return required;
}
public void setRequired(boolean required) {
this.required = required;
}
}
}
21 changes: 13 additions & 8 deletions ...est/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import org.elasticsearch.test.rest.yaml.section.AbstractClientYamlTestFragmentParserTestCase;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.notNullValue;

public class ClientYamlSuiteRestApiParserTests extends AbstractClientYamlTestFragmentParserTestCase {
Expand All @@ -39,11 +41,13 @@ public void testParseRestSpecIndexApi() throws Exception {
assertThat(restApi.getPaths().get(0), equalTo("/{index}/{type}"));
assertThat(restApi.getPaths().get(1), equalTo("/{index}/{type}/{id}"));
assertThat(restApi.getPathParts().size(), equalTo(3));
assertThat(restApi.getPathParts().get(0), equalTo("id"));
assertThat(restApi.getPathParts().get(1), equalTo("index"));
assertThat(restApi.getPathParts().get(2), equalTo("type"));
assertThat(restApi.getPathParts().keySet(), containsInAnyOrder("id", "index", "type"));
assertThat(restApi.getPathParts(), hasEntry("index", true));
assertThat(restApi.getPathParts(), hasEntry("type", true));
assertThat(restApi.getPathParts(), hasEntry("id", false));
assertThat(restApi.getParams().size(), equalTo(4));
assertThat(restApi.getParams(), contains("wait_for_active_shards", "op_type", "parent", "refresh"));
assertThat(restApi.getParams().keySet(), containsInAnyOrder("wait_for_active_shards", "op_type", "parent", "refresh"));
restApi.getParams().entrySet().stream().forEach(e -> assertThat(e.getValue(), equalTo(false)));
assertThat(restApi.isBodySupported(), equalTo(true));
assertThat(restApi.isBodyRequired(), equalTo(true));
}
Expand All @@ -59,7 +63,7 @@ public void testParseRestSpecGetTemplateApi() throws Exception {
assertThat(restApi.getPaths().get(0), equalTo("/_template"));
assertThat(restApi.getPaths().get(1), equalTo("/_template/{name}"));
assertThat(restApi.getPathParts().size(), equalTo(1));
assertThat(restApi.getPathParts().get(0), equalTo("name"));
assertThat(restApi.getPathParts(), hasEntry("name", false));
assertThat(restApi.getParams().size(), equalTo(0));
assertThat(restApi.isBodySupported(), equalTo(false));
assertThat(restApi.isBodyRequired(), equalTo(false));
Expand All @@ -78,10 +82,11 @@ public void testParseRestSpecCountApi() throws Exception {
assertThat(restApi.getPaths().get(1), equalTo("/{index}/_count"));
assertThat(restApi.getPaths().get(2), equalTo("/{index}/{type}/_count"));
assertThat(restApi.getPathParts().size(), equalTo(2));
assertThat(restApi.getPathParts().get(0), equalTo("index"));
assertThat(restApi.getPathParts().get(1), equalTo("type"));
assertThat(restApi.getPathParts().keySet(), containsInAnyOrder("index", "type"));
restApi.getPathParts().entrySet().stream().forEach(e -> assertThat(e.getValue(), equalTo(false)));
assertThat(restApi.getParams().size(), equalTo(1));
assertThat(restApi.getParams().get(0), equalTo("ignore_unavailable"));
assertThat(restApi.getParams().keySet(), contains("ignore_unavailable"));
assertThat(restApi.getParams(), hasEntry("ignore_unavailable", false));
assertThat(restApi.isBodySupported(), equalTo(true));
assertThat(restApi.isBodyRequired(), equalTo(false));
}
Expand Down

0 comments on commit 981b7f4

Please sign in to comment.