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

Feature schema #5

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Feature schema #5

wants to merge 5 commits into from

Conversation

lloria91
Copy link
Collaborator

adding custom schema validation feature

schema should be compiled Joi schema object.

@lloria91 lloria91 requested a review from fkanout May 11, 2020 08:06
@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #5 into master will decrease coverage by 9.52%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master       #5      +/-   ##
===========================================
- Coverage   100.00%   90.47%   -9.53%     
===========================================
  Files            1        1              
  Lines           68       42      -26     
===========================================
- Hits            68       38      -30     
- Misses           0        4       +4     
Impacted Files Coverage Δ
src/index.js 90.47% <100.00%> (-9.53%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 84d9a62...882077c. Read the comment docs.

@fkanout
Copy link
Owner

fkanout commented May 11, 2020

@lloria91 I'm was expecting to see a test that covers the use case that we started this implementation in the first place 😄

@lloria91
Copy link
Collaborator Author

@lloria91 I'm was expecting to see a test that covers the use case that we started this implementation in the first place 😄

Test about Complex case is the test no? Schema is almost same as in worker.

body: // Joi schema definition
params: // Joi schema definition
headers: // Joi schema definition
schema: // Compiled Joi schema object
Copy link
Owner

Choose a reason for hiding this comment

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

what is the difference between Compiled Joi schema object and Joi schema?

Copy link
Owner

Choose a reason for hiding this comment

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

if it's the same we should keep the terms

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we have simple JS that is transformed to joi.object. Compiled is already Joi object and don't need to call Joi.object function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they call it compilation of JS object to Joi object on their site

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So what I want to say there is that if you validate query, params and others separately with old version you need to pass JS object. But for schema need to pass Joi object

src/index.js Outdated
@@ -94,6 +102,18 @@ export default (inputs) => {
}

try {
if (schema) {
Copy link
Owner

Choose a reason for hiding this comment

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

If the schema is true, are we still checking the others (headers, query ...etc)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

check workers APS-38 MR please. schema contains Joi object. In general its already prepared Joi object and in validator no need to call Joi.object function

README.md Outdated
- Input validation (`query`, `params`, `body`, `headers`).
- Input validation
- `query`, `params`, `body`, `headers` - each input part is validated separately.
- `schema` - validates all input together, should be used with more complex schemas as [Joi.when](https://hapi.dev/module/joi/api/?v=17.1.1#anywhencondition-options) or [Joi.alternatives](https://hapi.dev/module/joi/api/?v=17.1.1#alternatives).
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
- `schema` - validates all input together, should be used with more complex schemas as [Joi.when](https://hapi.dev/module/joi/api/?v=17.1.1#anywhencondition-options) or [Joi.alternatives](https://hapi.dev/module/joi/api/?v=17.1.1#alternatives).
- `schema` - validates all input together, should be used with more complex schemas like [Joi.when](https://hapi.dev/module/joi/api/?v=17.1.1#anywhencondition-options) and [Joi.alternatives](https://hapi.dev/module/joi/api/?v=17.1.1#alternatives).

if (schema) {
await handleErrorWithSource(
"schema",
schema.validateAsync({
Copy link
Owner

Choose a reason for hiding this comment

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

I don't really understand how the schema is working, roles. e.g.
Add an example with an explanation in the README.

@fkanout
Copy link
Owner

fkanout commented May 11, 2020

@lloria91 I'm was expecting to see a test that covers the use case that we started this implementation in the first place 😄

Test about Complex case is the test no? Schema is almost same as in worker.

The worker case is a when not alternative case 🤔

@lloria91
Copy link
Collaborator Author

@lloria91 I'm was expecting to see a test that covers the use case that we started this implementation in the first place 😄

Test about Complex case is the test no? Schema is almost same as in worker.

The worker case is a when not alternative case 🤔

FInally after testing when and alternatives, it is alternative ))

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.

2 participants