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: correctly derive a valid JSON Schema from an OAS parameter #338

Merged
merged 29 commits into from
Jun 11, 2019

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Jun 4, 2019

This PR updates Graphite in Prism to use the latest version which includes the JSON Schema normalisation features.

  1. Update packages (Typescript, Types, Graphite)
  2. Cleaned up the examples from optional properties. In the new graphite a lot of fields are not mandatory and this requires an additional round of checks in Prism. An example is here
  3. The new graphite pointed out some inconsistencies in the previous handmade validator — which are now fixed since it has been offloaded to Ajv
  4. Added a specific test to make sure the validator is not case sensitive (headers) — comically speaking, this worm hole started from this specific issue

P.S: There are some other weird things happening in the deserialisation that I want to lookup and fix, but not here, not now.

Closes #324

@XVincentX XVincentX marked this pull request as ready for review June 10, 2019 07:47
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, great job so far! Left you a bunch of smaller comments.
My last point is that I couldn't find a test case for the originally reported issue (the header param).
Maybe we could take the exact spec provided in the original issue report and use it to feed the test?

@XVincentX
Copy link
Contributor Author

@philsturgeon @chris-miaskowski

Please have another look.

@XVincentX
Copy link
Contributor Author

My last point is that I couldn't find a test case for the originally reported issue (the header param).
Maybe we could take the exact spec provided in the original issue report and use it to feed the test?

Can you elaborate more? What is the test that's missing? The original issue was on the camel case of the headers, which now have a test.

philsturgeon
philsturgeon previously approved these changes Jun 10, 2019
@philsturgeon
Copy link
Contributor

@XVincentX what I meant about the tests, is that we should probably add some cases to the functional tests for changes as we go. packages/http/src/mocker/tests/functional.spec.ts didnt seem to get any love?

@XVincentX
Copy link
Contributor Author

@philsturgeon That test file is testing the mocker, which is after the validation step. I understand what you mean though, I'll find the relevant file and add the stuff you asked for.

@XVincentX XVincentX merged commit fa52a38 into master Jun 11, 2019
@XVincentX XVincentX deleted the feat/schema branch June 11, 2019 09:11
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.

Error 422, Missing Authorization header param
3 participants