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

feat: optionally allow validatorOptions of class-validator #233

Merged

Conversation

rinkstiekema
Copy link
Contributor

The validate function from class-validator has the optional function validatorOptions. An example usage that makes sense for FireORM would be forbidding non-whitelisted values, such that document will not be polluted accidentally. Find validatorOptions documentation here

I have added an optional property validatorOptions to MetadataStorageConfig, hope the code and tests are up to standards!

@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #233 (96175fa) into master (b66b24e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
+ Coverage   94.85%   94.88%   +0.02%     
==========================================
  Files          24       24              
  Lines         583      586       +3     
  Branches       95       95              
==========================================
+ Hits          553      556       +3     
  Misses         27       27              
  Partials        3        3              
Impacted Files Coverage Δ
src/MetadataStorage.ts 100.00% <ø> (ø)
src/AbstractFirestoreRepository.ts 90.72% <100.00%> (+0.09%) ⬆️
src/BaseFirestoreRepository.ts 100.00% <100.00%> (ø)
src/Batch/BaseFirestoreBatchRepository.ts 95.83% <100.00%> (ø)
src/Batch/FirestoreBatchUnit.ts 90.00% <100.00%> (ø)
src/MetadataUtils.ts 100.00% <100.00%> (ø)
.../Transaction/BaseFirestoreTransactionRepository.ts 86.95% <100.00%> (+0.28%) ⬆️

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 b66b24e...96175fa. Read the comment docs.

Copy link
Owner

@wovalle wovalle left a comment

Choose a reason for hiding this comment

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

This looks good!

I don't love the idea to test the internals of ClassValidator inside BaseFirestoreRepository (like testing if it whitelists or not) but I guess we have no other place to do it for now (until #198 is done).

I see that there are lint issues in the test, you can use prettier --write . to fix them 😄

@rinkstiekema
Copy link
Contributor Author

Yes I think you are right. I wrote those tests like this to check whether the validatorOptions were actually applied to all database operations, not so much to test the internals of ClassValidator. Very similar to the must not validate if the validateModels: false test.

I might take some time to take up #198 sometime soon, as I think integrated validation is a great feature of FireORM. Might aswell show some appreciation for a great project!

@wovalle wovalle changed the base branch from master to release-v0-20 February 23, 2021 23:02
@wovalle wovalle merged commit 23cc4f2 into wovalle:release-v0-20 Feb 23, 2021
@wovalle
Copy link
Owner

wovalle commented Feb 23, 2021

@all-contributors add @rinkstiekema for code

@allcontributors
Copy link
Contributor

@wovalle

I've put up a pull request to add @rinkstiekema! 🎉

@whateverbot
Copy link
Collaborator

🎉 This PR is included in version 0.20.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.

3 participants