Keats / validator

Simple validation for Rust structs
MIT License
1.97k stars 141 forks source link

Feature: Add `exclusive_min` and `exclusive_max` to `range` validation #246

Closed ivan-gj closed 1 year ago

ivan-gj commented 1 year ago

Description

This pull request adds the ability to introduce exclusive_min and exclusive_max parameters (temporarily named exc_min and exc_max) to the range validation. With this new feature, users can specify whether the range should exclude the minimum and/or maximum values.

For example, if a user wants to validate that a height field is within the range of 0.0 (exclusive) to 100.0 (inclusive), they can now use the following code:

#[validate(range(exclusive_min = 0.0, max = 100.0))]
height: f32

This situation is mostly relevant to non-integers, but I added the feature to support all the currently supported types because... who am I to stop people from using this feature with integers? Sometimes, it even makes sense (your limit is 299 but it is cleaner to say exclusive_max = 300).

It also forbids from inserting both inclusive and exclusive "minimums" or "maximums", so this is not possible:

#[validate(range(exclusive_min = 0.0, min = 10.0))]  // error
height: f32

Motivation

Currently, the range validation in this crate only allows users to specify an inclusive range. However, there are cases where users may want to exclude one or both of the endpoints of the range.

I come from a Python background, and pydantic is an well known validation library that has this feature, which was probably the main motivation for me to do this, as I have had to use this kind of exclusive limitation in some particular cases.

Another reason why I decided to do this was to make it possible for developers of schemars to automate the JSON Schema generation of these kind of exclusive limits.

Changes

Aside from the feature itself, tests and documentation have also been updated to reflect the new arguments and their limitations.

If there is any other thing that should be added in this Pull Request, please point it out, and I will try and do it.

Keats commented 1 year ago

I can see the point of half open/closed interval but I've never seen excluding the first value of the range. When is that useful?

ivan-gj commented 1 year ago

Do you mean having something along the lines of (-180.0, 180.0]? Because that is definitely a thing (and I have actually used it).

Keats commented 1 year ago

How do you represent it with the Rust range syntax? I was thinking of just using that for the next version to represent the range (eg instead of min=1, max=3 have 1..3)

ivan-gj commented 1 year ago

~~I think it is not semantically the same "range". The 1..10 range is a simple way of setting up an iterable of 1, 2, 3, 4, 5, 6, 7, 8, 9.~~

The range as it is understood in this repo (and in the JSON schema) is simply a validation of either <, >, >= or <=; or a sensible combination. Although slightly related for the specific case of integers... it is a completely different thing for real numbers.

Okay, I just checked, and it definitely seems like Rust's Range is quite more powerful than I expected.

Let me think about it... although I have a feeling that we should keep the min/max approach, at least as an alternative option.

ivan-gj commented 1 year ago

Hi! I have thought about it and, I strongly believe that we should have an exclusive minimum. It is supported in other important validation libraries, such as pydantic, and expected in validation schemas such as the JSON schema, and it would be a bummer to be forced to write custom validations for such a simple x > exc_min validation.

I do believe, though, that using Rust's Range variations could be a very nice feature, and a nice addition to the range validation. But, as I said, and addition, more than a substitution.

The reasoning behind this is that, well, things become clunky once you want to represent things such as:

#[validate(range(max = 123.0))]

because you'd have to do something like:

#[validate(range(-f64::INFINITY..=123.0))]

And that is definitely longer, and clearly not that humanly readable. I'd say clearly a downgrade.

Doing a Range with an exclusive minimum is even uglier.


So, what I would do, is to keep the min, max, exc_min, and exc_max, and open a new PR adding support for X..Y and X..=Y cases as a nice-to-have (I'd give it a try).

What do you think?

Keats commented 1 year ago

I would rather use the API from https://marshmallow.readthedocs.io/en/stable/marshmallow.validate.html#marshmallow.validate.Range where you have min_inclusive/max_inclusive as bool to keep min/max. If we add those though, the range syntax does not make too much sense since it might be confusing. You could do 1..=3 and max_inclusive=false

ivan-gj commented 1 year ago

I am not that big of a fan... I feel that it possibly exists because of the positional arguments min and max, that allow short declarations such as Range(12, 20); which is something impossible currently in validator... and it creates problems, such as that Range(12) technically means that the minimum value is 12, but it is not clear at all. Not a fan.

So, if applying the marshmallow system, the ugliest case would look like this:

#[validate(range(min = -180.0, max = 180.0, exclusive_min = true, exclusive_max = true)]

And with the proposed system in this PR, it would look like this:

#[validate(range(exc_min = -180.0, exc_max = 180.0))]

And honestly, I'd rather go with the later, but now that I'm thinking, this following one looks the best now to me:

#[validate(range(exclusive_min = -180.0, exclusive_max = 180.0))]

It feels a lot clearer than the marshmallow version; while it becomes more explicit in the "exclusive" tag.

ivan-gj commented 1 year ago

For what it's worth, I asked ChatGPT and it agrees that the 3rd option is better :sweat_smile:

Keats commented 1 year ago

They are booleans in https://datatracker.ietf.org/doc/html/draft-wright-json-schema-validation-00#section-5.3 as well so if we want to eventually generate openapi/jsonschema files from it it might be better to match it imo

Nevermind they are numbers in openapi 3.1

ivan-gj commented 1 year ago

Yep, I was basing myself mostly on the latest JSON Schema validation specification: https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-validation-01#name-exclusivemaximum

Keats commented 1 year ago

Can you add the full name (exclusive) and do it against https://github.com/Keats/validator/pull/249 ?

ivan-gj commented 1 year ago

There it goes! :smiley:

ivan-gj commented 1 year ago

Okay, I'll remove it this weekend.

Keats commented 1 year ago

Thanks!

ivan-gj commented 1 year ago

You are welcome! It was a blast. Thank you for merging. Looking forward to the next release!