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.

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

Matching paths with parameters does not recognize due to failing to split on extension . #261

Open
makdad opened this issue Apr 26, 2020 · 5 comments

Comments

@makdad
Copy link
Contributor

makdad commented Apr 26, 2020

Foremost, thanks for the great Gem.

I am applying this Gem retroactively to a project to add an OAS3 specification to its API. The API in question explicitly requires the response format as part of the URL, instead of a MIME type. I believe Committee cannot recognize this at the moment, thus I'm leaving an issue here. I'm happy to help fix, but given that is my first issue, I wanted to reach out first.

Here's the issue: for my project, if I want to update an object called a Fribble, with the ID parameter 123, I would call:

PUT /v1/fribble/123.json

... to invoke the API. In my OAS3 document, it looks like this:

paths:
  /v1/fribble/{id}.json:
    put:
      # and so on

Expected behavior: path matches, and my request/response is validated by Committee.
Actual behavior: path does not match, and nothing is validated.

Upon investigation, I noticed that OpenAPIParser::PathItemFinder splits path names on the / character when trying to infer path parameters in matching_paths_with_params.
Doing so leaves my actual parameter -- 123 -- mixed in with .json, which does not match the parameter type of integer (at least, I am assuming this is why it does not match), and thus, I assume that the request remains unmatched.

I locally can "fix" the problem by changing the split('/') method to instead split on forward slashes or periods: split(/[\/\.]/), however I feel that that may have adverse effects as well if paths had periods in them (which is totally valid).

I could attempt to fix this by only splitting on the . when it is at the end of the request path (e.g. there is no trailing / afterward).

If I am merely "holding it wrong", I sincerely apologize for raising the issue; I do understand that the API spec I am trying to model is quite odd in its interpretation, but that is precisely why I want to use this tool to improve it :)

Thanks in advance for your time.

@ota42y
Copy link
Member

ota42y commented May 9, 2020

Thanks for the bug report, I didn't know the problem

Probably it doesn't matter how we divide the path.
We check the requested path and the definition path in this method
https://github.com/ota42y/openapi_parser/blob/fa9be23269ad18b7721e923a1cf7506ed39daf0e/lib/openapi_parser/path_item_finder.rb#L72-L71

This method split these paths by / and check each item is same or path_template? method return true.
I think when we call this method with /v1/fribble/{id}.json and /v1/fribble/123.json,
the method return nil because path_template? method with {id}.json return false.

path_template? method check the parameter matched to ^\[.+\]$ so does not match if there are parameters in the middle.

And we doesn't supported even if multiple parameters are put in one hierarchy.
(/v1/{value1}-abc-{value2}/users).
So we can't solve these problem to change split rule.

Perhaps, I think we can fix this problem by these changes.

  • change path_template?(schema_path) to parse_path_parameter(schema_path, request_path)
    • this method return nil or Hash
    • when returning nil, schema_path dosen't include path_parameter on doesn't match request_path.
    • when returning Hash, schema_path include path_parameter and match request_path.
    • the Hash include parameter name and value.
    • e.g. parse_path_parameter("{id}-{value2}.json", "123-abc.json") return {"id" => "123", "value2" => "abc"}
    • if you have better method name, please use it :)
  • use parse_path_parameter result in extract_params
  • fix validation (if we needed)

Thanks your good issue :)

@makdad
Copy link
Contributor Author

makdad commented Jul 20, 2020

@ota42y I've started a pull request on my local copy of the repo to address this feature in openapi_parser. Still having a little testing trouble, but I'd like to ask for your review in the original implementation, as it is my first time contributing to this repository, and I want to make sure I am following guidelines, style, etc.

https://github.com/makdad/openapi_parser/pull/1/files

@makdad
Copy link
Contributor Author

makdad commented Aug 27, 2020

This issue can be closed by updating openapi_parser Gem to 0.12.0 (we have implemented this feature on that Gem).

@makdad
Copy link
Contributor Author

makdad commented Aug 27, 2020

@ota42y I can easily make a PR to committee to bump the version of openapi_parser, but it looks like you just did the 4.2.0 version release for committee. Should I make a new PR that bumps the version to 4.2.1 or even 4.3.0, or is committee set up for some common release cycle?

In my case, I would very much like to make use of the new changes I implemented in openapi_parser 0.12.0 as part of my company's committee implementation, so I admit that I might be in more of a hurry to release a new version :)

I am not sure of the normal release process for this Gem...

@ota42y
Copy link
Member

ota42y commented Aug 27, 2020

You can use the latest openapi_parser version without updating committee.

committee use openapi_parser >= 0.11.1 but independent of the release cycle.
openapi_parser dose not break backward compatibility now, and we do not update the minimum dependent version possible.

I released for different reasons, so we can use committee == 4.1.0 with openapi_parser == 0.12.1 and committee == 4.2.0 with openapi_parser == 0.11.1.

If you want to use newest openapi_parser, please use bundle update openapi_parser (We fix warning so please use 0.12.1)
And I recommend update committee to 4.2.0 because include effective fix but it not required to use openapi_parser == 0.12.1.

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

No branches or pull requests

2 participants