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

API specs lost if predefined spec includes an endpoint at '/' #133

Open
amagid opened this issue Mar 29, 2023 · 1 comment
Open

API specs lost if predefined spec includes an endpoint at '/' #133

amagid opened this issue Mar 29, 2023 · 1 comment

Comments

@amagid
Copy link

amagid commented Mar 29, 2023

First of all, this module is awesome - thank you for maintaining it!

Overview of the Problem

I ran into an issue today on an application which I think has a fairly simple solution. I'm going to submit a PR for it and would appreciate someone looking through it to make sure there aren't any unintended side-effects.

Basically, if the existing oas document has an endpoint defined with the path '/', the generator fails to write an entry to the oas document for any API which:

  1. Has any path params or query params
  2. AND has a trailing slash in the request url path

Reproducing the Problem

Use the attached express server to generate docs.

minimal-reproduction.zip

This server has two endpoints: GET / and GET /some/other/endpoint/with/{pathParam}.

  1. Run the server with node index.js
  2. Call the endpoint http://localhost:3001/some/other/endpoint/with/5/ Note the trailing slash on the URL
  3. Check the generated oas docs - there shouldn't be any response information (or detailed param information) in the doc for this endpoint
  4. Call the endpoint again but remove the trailing slash (http://localhost:3001/some/other/endpoint/with/5)
  5. Check the oas docs again - there should now be detailed param and response information in the doc
  6. The server has a git repo bundled so you can reset the oas docs.

More Details

I tracked the issue down to getPathKey() in index.js, specifically where it generates the regex for checking the path keys. The regex is currently generated with this line:

`${pathKey.replace(/{([^/]+)}/g, '(?:([^\\\\/]+?))')}/?$`

In the case where the pathKey is '/', that will result in this pattern being generated:

`//?$`

This pattern will match any character string that ends with a forward slash. In addition, the path key is checked by getPathKey() only if the request url doesn't exactly match the path key in the swagger doc, which happens if it has path params or query params. So the end result here is that if a URL has path params or query params and also has a trailing slash, and if the swagger document includes a path key that is at the root of the api ('/'), then it will be merged into the swagger doc entry for the path '/' rather than the entry for the correct path.

This particular issue can be fixed very simply by adding a beginning check (^) to the generated pattern like so:

`^${pathKey.replace(/{([^/]+)}/g, '(?:([^\\\\/]+?))')}/?$`

That would then result in the following pattern being generated for the pathKey '/':

^//?$

Which will no longer incorrectly match other paths with trailing slashes.

Can anyone see any side-effects of this proposed change?

I'll submit a PR with tests soon.

@mpashkovskiy
Copy link
Owner

Thanks, Aaron for raising the issue. I see no potential problem with the described solution. The PR is very welcome!

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

No branches or pull requests

2 participants