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

[Test] ClientYamlSuiteRestApiParser parses specs without path parts #33720

Merged
merged 2 commits into from Sep 21, 2018
Jump to file or symbol
Failed to load files and symbols.
+26 −9
Diff settings

Always

Just for now

Next

Test: ClientYamlSuiteRestApiParser parses specs without path parts

Previously ClientYamlSuiteRestApiParser threw an exception when an api
spec contained neither path parts nor url parameter sections.
  • Loading branch information...
lipsill committed Sep 14, 2018
commit 76a2e89f4966ef3f3d3851be82b32ce86b68a834
@@ -64,8 +64,7 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro
if ("url".equals(parser.currentName())) {
String currentFieldName = "url";
int innerLevel = -1;
while(parser.nextToken() != XContentParser.Token.END_OBJECT || innerLevel >= 0) {
while(parser.nextToken() != XContentParser.Token.END_OBJECT) {
if (parser.currentToken() == XContentParser.Token.FIELD_NAME) {
currentFieldName = parser.currentName();
}
@@ -108,13 +107,6 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro
restApi.addParam(param, PARAMETER_PARSER.parse(parser, null).isRequired());
}
}
if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
innerLevel++;
}
if (parser.currentToken() == XContentParser.Token.END_OBJECT) {
innerLevel--;
}

This comment has been minimized.

@javanna

javanna Sep 17, 2018

Member

what happens with this fix if you have something like:

    "url" : {
        "parts" : {
        },
        "path" : "my_path"
    }

Does the path get parsed?

@javanna

javanna Sep 17, 2018

Member

what happens with this fix if you have something like:

    "url" : {
        "parts" : {
        },
        "path" : "my_path"
    }

Does the path get parsed?

This comment has been minimized.

@lipsill

lipsill Sep 17, 2018

Contributor

Hm... yes and no. path will be parsed, but it will be ignored: within the url only paths, parts and params are analyzed ;)

I can add this as a test case (that the parser does not throw an exception).

@lipsill

lipsill Sep 17, 2018

Contributor

Hm... yes and no. path will be parsed, but it will be ignored: within the url only paths, parts and params are analyzed ;)

I can add this as a test case (that the parser does not throw an exception).

This comment has been minimized.

@javanna

javanna Sep 17, 2018

Member

I am not sure that this behaviour is correct though, so I would fix that and add a test that makes sure the behaviour for this scenario is correct. Do you see what I mean?

@javanna

javanna Sep 17, 2018

Member

I am not sure that this behaviour is correct though, so I would fix that and add a test that makes sure the behaviour for this scenario is correct. Do you see what I mean?

This comment has been minimized.

@lipsill

lipsill Sep 18, 2018

Contributor

I am not sure that this behaviour is correct though

@javanna What you mean by this behaviour?
Currently the following api spec is parsed without a problem.

{
  "test": {
    "documentation": "whatever",
    "methods": [ "GET", "POST" ],
    "url": {
      "parts": {},
      "path": "/my_path"
    },
    "body": {
      "description" : "whatever",
      "required" : true
    }
  }
}

Or do you mean that the path needs to be validated as a string, although it will not be stored in the api spec.

TBH I find the usage of innerLevel (especially when combined with object parser) confusing and, as raised by the issue, unreliable and order dependent.

@lipsill

lipsill Sep 18, 2018

Contributor

I am not sure that this behaviour is correct though

@javanna What you mean by this behaviour?
Currently the following api spec is parsed without a problem.

{
  "test": {
    "documentation": "whatever",
    "methods": [ "GET", "POST" ],
    "url": {
      "parts": {},
      "path": "/my_path"
    },
    "body": {
      "description" : "whatever",
      "required" : true
    }
  }
}

Or do you mean that the path needs to be validated as a string, although it will not be stored in the api spec.

TBH I find the usage of innerLevel (especially when combined with object parser) confusing and, as raised by the issue, unreliable and order dependent.

This comment has been minimized.

@javanna

javanna Sep 19, 2018

Member

You are right, I forgot that path is ignored anyways, and I misread the parsing code thinking that the inner parsers would not consume the inner end object, and then we could end up exiting the loop before reading the whole container object. All good , I think your change is good, I will trigger tests.

@javanna

javanna Sep 19, 2018

Member

You are right, I forgot that path is ignored anyways, and I misread the parsing code thinking that the inner parsers would not consume the inner end object, and then we could end up exiting the loop before reading the whole container object. All good , I think your change is good, I will trigger tests.

}
}
@@ -91,6 +91,31 @@ public void testParseRestSpecCountApi() throws Exception {
assertThat(restApi.isBodyRequired(), equalTo(false));
}
public void testRequiredBodyWithoutUrlParts() throws Exception {
String spec = "{\n" +
" \"count\": {\n" +
" \"documentation\": \"whatever\",\n" +
" \"methods\": [ \"GET\", \"POST\" ],\n" +
" \"url\": {\n" +
" \"path\": \"/whatever\",\n" +
" \"paths\": [ \"/whatever\" ]\n" +
" },\n" +
" \"body\": {\n" +
" \"description\" : \"whatever\",\n" +
" \"required\" : true\n" +
" }\n" +
" }\n" +
"}";
parser = createParser(YamlXContent.yamlXContent, spec);
ClientYamlSuiteRestApi restApi = new ClientYamlSuiteRestApiParser().parse("count.json", parser);
assertThat(restApi, notNullValue());
assertThat(restApi.getPathParts().isEmpty(), equalTo(true));
assertThat(restApi.getParams().isEmpty(), equalTo(true));
assertThat(restApi.isBodyRequired(), equalTo(true));
}
private static final String REST_SPEC_COUNT_API = "{\n" +
" \"count\": {\n" +
" \"documentation\": \"http://www.elasticsearch.org/guide/en/elasticsearch/reference/current/search-count.html\",\n" +
ProTip! Use n and p to navigate between commits in a pull request.