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

fix #380: allow any string to be servers variables #381

Merged
merged 1 commit into from Oct 2, 2020

Conversation

xg1990
Copy link
Contributor

@xg1990 xg1990 commented Sep 30, 2020

According to the OpenAPI 3.0 specification, we should be able to use any variables in servers. More comments can be found in this issue: #380

This change is based on the latest documentation of path-to-regexp that we can specify the pattern of each path parameters

@xg1990
Copy link
Contributor Author

xg1990 commented Sep 30, 2020

This might also fix #113

@cdimascio
Copy link
Owner

cdimascio commented Oct 1, 2020

thanks for the issue and the PR @xg1990. much appreciated.

i see. so looks like the current regex is not allowing you to have /s in the variable name. am i understanding this correctly?

with your new change, we'll end up with simple paths looking similar to the following

// given the path 'http://example.com/test/{me}/hello/{there}'

> urlPath.replace(/{(\w+)}/g, (substring, p1) => `:${p1}(.*)`)

// will now produce
'http://example.com/test/:me(.*)/hello/:there(.*)'

// rather than
'http://example.com/test/:me/hello/:there'

This seems reasonable. The new path matcher will have the same behavior as the previous logic, however we can now match the /s

Assuming, I'm understanding the issue correctly, your change looks reasonable to me

@xg1990
Copy link
Contributor Author

xg1990 commented Oct 1, 2020

Hi @cdimascio ,

Thanks for this response.

i see. so looks like the current regex is not allowing you to have /s in the variable name. am i understanding this correctly?

Yes, to be more precise, it's / in variable value

And your understanding is right, according to path-to-regexp 's documentation, if we have /:there(.*) then it will allow anything in the path

@cdimascio cdimascio merged commit b0c2a0c into cdimascio:master Oct 2, 2020
@cdimascio
Copy link
Owner

@all-contributors add @xg1990 for code

@allcontributors
Copy link
Contributor

@cdimascio

I've put up a pull request to add @xg1990! 🎉

@cdimascio
Copy link
Owner

@xg1990 your fix is in v4.0.6

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

Successfully merging this pull request may close these issues.

None yet

2 participants