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

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lipsill
Contributor

lipsill commented Sep 14, 2018

Previously ClientYamlSuiteRestApiParser threw an exception when an api
spec contained neither path parts nor url parameter sections.

Fixes #31649

Test: ClientYamlSuiteRestApiParser parses specs without path parts
Previously ClientYamlSuiteRestApiParser threw an exception when an api
spec contained neither path parts nor url parameter sections.
}
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.

@javanna javanna added the :Core/Build label Sep 17, 2018

@elasticmachine

This comment has been minimized.

Show comment
Hide comment
@elasticmachine

elasticmachine commented Sep 17, 2018

@javanna

This comment has been minimized.

Show comment
Hide comment
@javanna

javanna Sep 19, 2018

Member

test this please

Member

javanna commented Sep 19, 2018

test this please

@javanna javanna self-assigned this Sep 19, 2018

@javanna

This comment has been minimized.

Show comment
Hide comment
@javanna

javanna Sep 20, 2018

Member

hi @lipsill the test failures are unrelated to your changes, would you be able to merge master in, so I can run tests again?

Member

javanna commented Sep 20, 2018

hi @lipsill the test failures are unrelated to your changes, would you be able to merge master in, so I can run tests again?

@lipsill

This comment has been minimized.

Show comment
Hide comment
@lipsill

lipsill Sep 20, 2018

Contributor

@javanna thanks for the ping. I had a look yesterday but did not see a commit addressing the bwc version issue I saw in the CI ...
I just merged. Can you give this one a try again;)

Contributor

lipsill commented Sep 20, 2018

@javanna thanks for the ping. I had a look yesterday but did not see a commit addressing the bwc version issue I saw in the CI ...
I just merged. Can you give this one a try again;)

@javanna

This comment has been minimized.

Show comment
Hide comment
@javanna

javanna Sep 20, 2018

Member

retest this please

Member

javanna commented Sep 20, 2018

retest this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment