Skip to content

Conversation

erezrokah
Copy link
Collaborator

Fixes #27

  • I consolidated all the validations into one schema (allows reuse of validation logic)
  • I had to override some of the Joi errors messages since they were not very useful (see the helper customErrorBuilder function)
  • Added missing validations for request (mapping template) and partitionKey
  • Removed some more unnecessary async/await usage
  • Add addCors utility method to avoid code duplication (the one at the s3 package had a better implementation that didn't override existing response parameters)
  • Added missing tests to get to 100% coverage - I'm not saying we should remain at 100% all the time, it just makes it easier to find out if you're adding code that is not covered

@horike37
Copy link
Collaborator

@erezrokah
Thank you for implementing it 👍

@theburningmonk
Can you review this PR? Since I have not understand how to use Joi validation yet.

@theburningmonk
Copy link
Collaborator

@horike37 sorry, will take a look today

Copy link
Collaborator

@horike37 horike37 left a comment

Choose a reason for hiding this comment

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

@erezrokah
Looks good to me from my perspective 👍
I'm working on DynamDB proxy feature, so I need to reflect this validation logic to my working branch for consistency.

@erezrokah
Copy link
Collaborator Author

@erezrokah
Looks good to me from my perspective
I'm working on DynamDB proxy feature, so I need to reflect this validation logic to my working branch for consistency.

Let me know if you have any questions on migrating to Joi

@theburningmonk
Copy link
Collaborator

@erezrokah Impressive work! It looks good for me too, my one feedback is that it'll be nice to split the validate.test.js into separate files - e.g. validate.common.test.js, validate.s3.test.js, etc. so that it's easier to read and extend in the future, even if it means some code duplication.

@horike37 horike37 merged commit 2bc837f into serverless-operations:master Aug 27, 2019
@erezrokah
Copy link
Collaborator Author

@erezrokah Impressive work! It looks good for me too, my one feedback is that it'll be nice to split the validate.test.js into separate files - e.g. validate.common.test.js, validate.s3.test.js, etc. so that it's easier to read and extend in the future, even if it means some code duplication.

Thanks for the feedback.
Let me have another pass on the test file to improve it. I'll do it in another pr

@erezrokah erezrokah deleted the refactor/use_hapi_joi_for_validation branch August 28, 2019 05:05
@theburningmonk
Copy link
Collaborator

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chore: validate the configuration in a consistent way
3 participants