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

match() throws an error if no matching validator is found. Fixes #64. #67

Merged
merged 2 commits into from
Feb 22, 2021

Conversation

aautio
Copy link
Contributor

@aautio aautio commented Aug 31, 2020

Hello 👋

Here is a breaking change to match(). I've implemented it as discussed in #64 and #63. Now match() will throw an error when no matching routes are found from OpenAPI schema. The error is an instance of ValidationError without the .data array.

To skip the error you can invoke match({ allowNoMatch: true }).

There was also a bug in the original tests for match. There was no matching path for /match/works-with-url-param in the OpenAPI schema. Thus match() did not invoke any validators and the test yielded a false negative. This was fixed by the first commit in this pull request.

Would this be ok to be published as 0.6.0? Feel free to edit anything as you wish. I'll be happy to help with this one further. Let me know if e.g. changes to ValidationError are not welcome.

Input was not validated when testing match & url-params.
The api spec was missing a proper path and thus match did not execute.
@coveralls
Copy link

coveralls commented Aug 31, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling dffa3ce on aautio:master into 71b4af0 on Hilzu:master.

@Hilzu
Copy link
Owner

Hilzu commented Sep 3, 2020

Hi Ari! Thanks for your PR.

To me this looks good. I wouldn't touch ValidationError for this feature. It's only meant for errors thrown when validating the incoming requests. You could replace it with just plain Error or create a new MatchError if you feel like it.

 * This is a breaking change.
 * To keep previous functionality call match({ allowNoMatch: true }).
@aautio
Copy link
Contributor Author

aautio commented Sep 3, 2020

Ok super 👌

I've switched the ValidationError to Error and updated docs & tests appropriately. Changes were pushed a couple of minutes ago.

@codeclown
Copy link
Contributor

@Hilzu Can we help in any way to get this merged? Checks have failed due to:

Bad response: 422 {"message":"service_job_id (723838053) must be unique for Travis Jobs not supplying a Coveralls Repo Token","error":true}

Either COVERALLS_REPO_TOKEN has become a requirement, or this is an issue in Travis. Some confusing discussion in lemurheavy/coveralls-public#1264.

@Hilzu
Copy link
Owner

Hilzu commented Feb 22, 2021

Sorry this got stuck due to me looking into the Travis issue. I'm merging this in and replacing Travis with GitHub actions which should work nicer for open-source projects.

@Hilzu Hilzu merged commit 2793c89 into Hilzu:master Feb 22, 2021
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.

4 participants