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

Make yaml test runner stricter by enforcing required for paths and parameters #27035

Merged
merged 4 commits into from
Oct 25, 2017
Merged

Make yaml test runner stricter by enforcing required for paths and parameters #27035

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83560ea
enforce 'required' in yaml test runner for paths and parameters
olcbean Oct 9, 2017
30ecc4f
addressing the reviewers comments
olcbean Oct 18, 2017
744b2fa
renaming inner class
olcbean Oct 19, 2017
94684c2
ObjectParser set to static
olcbean Oct 20, 2017
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
36 changes: 28 additions & 8 deletions test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java
View file
Open in desktop
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,20 +84,36 @@ 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 (!apiRequiredPathParts.isEmpty()) {
Copy link
Member

Choose a reason for hiding this comment

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

Funny story: we had a contributor for a long time that always wanted false == blah rather than !blah because, he claimed, it is harder to miss the false == part then the ! part. I kind of agree with him but I think we were in the minority. But that is why you have both patterns all over our code base. Either way is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope you don't mind that I change all occurrences of ! to false == (consistency on file level)

throw new IllegalArgumentException(
"missing required path part: " + apiRequiredPathParts + " by [" + restApi.getName() + "] api");
}
if (!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) {
Expand Down
25 changes: 17 additions & 8 deletions ...ework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApi.java
View file
Open in desktop
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() {
Copy link
Member

Choose a reason for hiding this comment

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

Probably should have javadoc because it isn't obvious what the boolean reads unless you read it as part of this PR.

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
31 changes: 24 additions & 7 deletions ...src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java
View file
Open in desktop
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 Down Expand Up @@ -57,7 +59,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 +72,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, getRequired(parser));
}
}

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, getRequired(parser));
}
}

Expand Down Expand Up @@ -146,4 +147,20 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro

return restApi;
}

class YamlElement {
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be more reasonable to name this Parameter or something.

private boolean required;
public boolean isRequired() {
return required;
}
public void setRequired(boolean required) {
this.required = required;
}
}

private boolean getRequired(XContentParser parser) throws IOException {
ObjectParser<YamlElement, Void> objParser = new ObjectParser<>("", true, YamlElement::new);
Copy link
Member

Choose a reason for hiding this comment

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

We tend to make these static.

objParser.declareBoolean(YamlElement::setRequired, new ParseField("required"));
return objParser.parse(parser, null).isRequired();
}
}
21 changes: 13 additions & 8 deletions ...est/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserTests.java
View file
Open in desktop
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