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

Expose validator constraints to end-users using a new trait #238

Closed
wants to merge 8 commits into from

Conversation

kyrias
Copy link
Contributor

@kyrias kyrias commented Jan 17, 2023

This PR exposes all of the validator crate constraints defined for a specific type through a new trait called Constraints. The new trait only contains associated methods and so does not pose a backwards-compatibility issue.


We have tests that ensure that expected valid and invalid values will not be erroneously accepted/rejected. To do this we currently double-encode these constraints on both the actual types and then again in the test suite.

This is not scalable, and it's very hard to ensure that they're always kept in sync. So instead we want to be able to just ask the validator crate which constraints are defined for a specific type.

@Keats
Copy link
Owner

Keats commented Jan 17, 2023

Thanks for the PR.

I'm actually a bit torn on how to set up constraints. I don't want third party crates like indexmap as features of validator, it doesn't scale.
I'm thinking we should a single trait like serde Serialize/Deserialize that we implement for all of the std libs and that other crates can implement that combines HasLen/Contains so 3rd parties only have a single trait to implement.

As for the PR itself, I don't think there will be much changes (other than a rewrite of the derive crate) so I think I would leave it as it for now and revisit when that rewrites happen.

@kyrias
Copy link
Contributor Author

kyrias commented Jan 18, 2023

I'm actually a bit torn on how to set up constraints. I don't want third party crates like indexmap as features of validator, it doesn't scale.

I don't care about third-party crates, but not implementing the crate for the third-party crates that this crate already supports would be a breaking change, as it would break every crate currently using them.

I'm thinking we should a single trait like serde Serialize/Deserialize that we implement for all of the std libs and that other crates can implement that combines HasLen/Contains so 3rd parties only have a single trait to implement.

I think that would be a mistake as those are semantically disparate things.

Right now if you try to do #[validate(contains = "foo")] on e.g. a struct field of any regular struct type you'll get a compile-time error as it's not semantically meaningful to talk about whether or not any random struct contains a string. If all of these things were in a single trait you could at best only get a run-time error for this, at the worst a panic!, both of which I would argue would be an actively worse user experience.

@Keats
Copy link
Owner

Keats commented Jan 19, 2023

I don't care about third-party crates, but not implementing the crate for the third-party crates that this crate already supports would be a breaking change, as it would break every crate currently using them.

Yep that would be a breaking change.

e.g. a struct field of any regular struct type you'll get a compile-time error as it's not semantically meaningful to talk about whether or not any random struct contains a string

Right now there are some very ugly checks (https://github.com/Keats/validator/blob/master/validator_derive/src/asserts.rs#L8-L61 and the rest of the files) which fail entirely as soon as it's a unsupported crate or even just a type alias. I'm still not entirely clear on how that would work, maybe having the separate traits do make sense in the end but we need to remove those asserts hacks. That's part of the reason for the rewrite, and also to handle new attributes like sensitive (where the value is not added anywhere in the error) etc. I'm sure there are other easy things to improve as well, this crate started as a quick thing to learn proc macro 6 years ago after.

@kyrias
Copy link
Contributor Author

kyrias commented Feb 24, 2023

So I'm not really sure whether there's anything you want changed here currently?

Do you want me to file another PR to remove support for the third-party crates?

@Keats
Copy link
Owner

Keats commented Feb 24, 2023

No I think the first step is to rewrite the validation logic in the vein of #225 + a rewrite of the derive macro to support things like sensitive parameters and just be better designed as it was really just feature 2 added on top of other features without any cohesion.

@svenstaro
Copy link

I'd love this to get merged but I'm not exactly sure I understand what is blocking this right now. So if I read this correctly, it's blocked on #225 which is currently also not really getting worked on? Is there some clear path forward here and can it be assisted somehow?

pintariching and others added 8 commits April 14, 2023 12:50
* implemented validation trait for length

* converted identation to spaces

* changed the trait to not require HasLen

* added macro for generating impls

* implemented ValidateLength for some types

* using trait validation instead of the function

* added cfg for indexmap import

* changed trait to require length

* Revert "changed trait to require length"

This reverts commit a77bdc9.

* moved validation logic inside ValidateLength trait

* added trait validation for required

* added email trait validation

* fixed trait validation for email

* added range trait validation

* fixed range trait

* added url trait validation

---------

Co-authored-by: Tilen Pintarič <tilen.pintaric@aviko.si>
Keats#246)

* feat(range): add exclusive minimum and exclusive maximum for `range` validation

* test(range): add tests for `exc_min` and `exc_max` range validation

* docs: add docs for `exc_min` and `exc_max` for `range` validation

* chore: rename `exc_min`, `exc_max` to `exclusive_min`, `exclusive_max`

* chore(validation.rs): get rid of `collide` function
…kind

This makes it possible for code consuming the constraints to know
whether the type is used directly or if it is held in some kind of
collection.
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.

5 participants