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

Allow optional use of req.url #857

Merged
merged 3 commits into from
Nov 12, 2023

Conversation

nikkegg
Copy link
Contributor

@nikkegg nikkegg commented Aug 14, 2023

Solves #843 and #113

@cdimascio I could not find where to update the docs wiki, but am happy to do so if you point me in the right direction.

@cdimascio
Copy link
Owner

Thanks for the change. I will review in next few days. Good question on the wiki....

Since migrating the doc to wiki, I do not currently have a good mechanism for updates.

Open to suggestions. I found this on SO https://stackoverflow.com/questions/10642928/how-can-i-make-a-pull-request-for-a-wiki-page-on-github

Perhaps post the doc content in the comments here and I will manually integrate into the wiki upon merge

@sennyeya
Copy link

@nikkegg Thanks for picking this up! Excited to use this!

@@ -49,6 +49,7 @@ export class OpenApiValidator {
if (options.$refParser == null) options.$refParser = { mode: 'bundle' };
if (options.validateFormats == null) options.validateFormats = true;
if (options.formats == null) options.formats = {};
if (options.useRequestUrl == null) options.useRequestUrl = false;
Copy link
Owner

@cdimascio cdimascio Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you help me understand why this is an option controlled by the client? what is the downside using this behavior i.e. req.path and req.url for all requests? for instance, can we make both paths work withour requiring the client to configure the behavior

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req.path and req.url are localized by express to be router specific, so if we nest router A in router B req.path/req.url will only have router A data (/test), no router B data (/api/). #211 introduced the current default behavior as the desired use case was that users were validating against a single spec that included router B data. I don't think there's an easy way without naive searching of the spec to handle both.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdimascio what @sennyeya said - using req.path or req.url is scoped to the router using it and is unaware of the url defined by the parent router. The flip side of this is that you can't use separate schemas in the parent router and child router, because originalUrl is always set to the one defined by the parent router i.e /api. But using req.url in the child router allows us to decouple child and parent routers and schemas. Also req.url is intended to be used like so by the express framework I believe.
Screenshot 2023-08-26 at 14 38 31

@nikkegg
Copy link
Contributor Author

nikkegg commented Oct 31, 2023

@cdimascio just a little bump on this 🙏🏻

@cdimascio cdimascio merged commit f732379 into cdimascio:master Nov 12, 2023
3 checks passed
@cdimascio
Copy link
Owner

@nikkegg the change is in v5.1.0, can you provide a brief doc write up for its usage. i will include it in the doc

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

Successfully merging this pull request may close these issues.

3 participants