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

Remove the body validation registry #446

Merged
merged 18 commits into from
Jul 12, 2019
Merged

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Jul 10, 2019

This PR removes the validation registry concept for the body parameters, since it was based on the assumption that a validator is associated with the serialised type — something we won't be doing anymore.

Deserialisation and validation will be two different things and so the validation is always possible, given a "schema" or something.

I've removed the code that was building up such infrastructure; by doing this I've discovered some mocking problems in certain parts of the code (basically missed cleanups) that were faking some test results. I've modified these as well to behave correctly.

I've added two harness tests to check that it's behaving correctly.

Closes #420

@XVincentX XVincentX marked this pull request as ready for review July 10, 2019 13:53
@@ -112,10 +112,10 @@ describe.each([['petstore.oas2.json'], ['petstore.oas3.json']])('server %s', fil

it('should not mock a verb that is not defined on a path', async () => {
const response = await server.fastify.inject({
method: 'POST',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case you're asking yourself: yes, this test has been wrong since forever. It's now fixed.

Array [
Object {
"code": "testName",
"message": "testMessage",
"code": "type",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was caused because the mocked functions apparently were not restored correctly, thous returning a not correct error

@@ -16,28 +16,6 @@
"default": "available"
},
"collectionFormat": "multi"
},
{
"in": "body",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By not mocking the validation process anymore everywhere there was a test checking a completely different thing now starting to fail because of the body validation (which is now happening for real) — therefore I removed the body parameter at all so that the request goes through correctly as it should.

@XVincentX XVincentX changed the title Remove the validation registry Remove the body validation registry Jul 10, 2019
@XVincentX XVincentX force-pushed the feat/validator-registry-off branch 7 times, most recently from 297a18e to 0853705 Compare July 10, 2019 22:27
@XVincentX XVincentX force-pushed the feat/validator-registry-off branch 4 times, most recently from 250ab6d to f4cd67e Compare July 11, 2019 07:41
Bumps [lodash.template](https://github.com/lodash/lodash) from 4.4.0 to 4.5.0. **This update includes security fixes.**
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.4.0...4.5.0)

Signed-off-by: dependabot-preview[bot] <[email protected]>
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.11 to 4.17.13. **This update includes security fixes.**
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.11...4.17.13)

Signed-off-by: dependabot-preview[bot] <[email protected]>
@XVincentX XVincentX force-pushed the feat/validator-registry-off branch from e0cb9bb to cdd7a15 Compare July 11, 2019 08:12
@XVincentX XVincentX force-pushed the feat/validator-registry-off branch from cdd7a15 to aadffed Compare July 11, 2019 08:21
@chris-miaskowski chris-miaskowski modified the milestones: 3.1.0, Quacker Sprint Jul 11, 2019
Copy link
Contributor

@chris-miaskowski chris-miaskowski left a comment

Choose a reason for hiding this comment

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

Hey @XVincentX looks great in general. Just a quick question, have you checked what happens if we send a text/plain in the body and try to validate that?

@XVincentX
Copy link
Contributor Author

have you checked what happens if we send a text/plain

Fastify currently is not configured to handle such content type, so you will receive a 415 and Prism won't be invoked at all.

@XVincentX XVincentX merged commit 3694664 into master Jul 12, 2019
@XVincentX XVincentX deleted the feat/validator-registry-off branch July 12, 2019 07:33
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.

Prism does not even attempt to validate a body if "consumes" not defined.
2 participants