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

Change validation to consider the empty string a valid value #1482

Merged
merged 1 commit into from Mar 16, 2020

Conversation

@kraih
Copy link
Member

kraih commented Mar 16, 2020

This appears to be the default assumption of our users. The empty
string can now be validated with regexes and other checks.
Additionally non-browser users and form elements without any value
at all can now be considered during validation.

This appears to be the default assumption of our users. The empty
string can now be validated with regexes and other checks.
Additionally non-browser users and form elements without any value
at all can now be considered during validation.
@kraih

This comment has been minimized.

Copy link
Member Author

kraih commented Mar 16, 2020

Calling for a vote @mojolicious/core.

@kraih kraih added the vote label Mar 16, 2020
@kraih

This comment has been minimized.

Copy link
Member Author

kraih commented Mar 16, 2020

To be more precise, this is about the case where you have a query string like ?foo=&bar=baz. The question is if $validation->optional('foo') and $validation->required('foo') should accept the empty string for further validation.

@kalikiana

This comment has been minimized.

Copy link

kalikiana commented Mar 16, 2020

I actually ran into this in os-autoinst/openQA#2812 where we have an API which accepts an optional parameter that must be numeric or empty. If the empty string is given, the value is cleared. Other values can be updated without changing or clearing said value, however - so empty is not the same as undefined. I actually defined it as ->optional('size_limit_gb')->like(qr/^(|[0-9]+)\z/) thinking this would consider empty strings.

@marcusramberg

This comment has been minimized.

Copy link
Member

marcusramberg commented Mar 16, 2020

Can't make up my mind if this is better or not. Going to have to vote neutral.

Copy link
Member

jhthorsen left a comment

Looks sane to me 👍

@Grinnz
Grinnz approved these changes Mar 16, 2020
Copy link
Contributor

Grinnz left a comment

I am voting neutral. I think it would be good to provide more flexibility by doing this, but also I'm not sure about the implications for day-to-day use with forms as submitted by browsers.

@CandyAngel

This comment has been minimized.

Copy link
Member

CandyAngel commented Mar 16, 2020

tl;dr Voting yes

It seems to trip new users up quite a bit. Perhaps it is because it looks like you are asserting whether the topic has to be part of the input or can be omitted, but it is actually considering the value as well (which is really what the checks are for).

This change introduces a clearer separation between "is the topic in the input" and "does the value of the topic meet this criteria" (e.g. is not an empty string). It also makes using Mojolicious::Validator nicer to use outside of validating web forms (like validating CSV files from crazy systems that have unstable headers!) where an empty string in a required topic is valid.

Any code currently using optional to work around this will continue to work. Just trying to think if anyone could be relying on the not-empty-string required behaviour without any additional checks in a way that isn't already flawed.

My vote is yes because having optional and required being used for checking the presence of a topic and the checks being used to check the value is a simple and easy-to-follow concept.

@jberger

This comment has been minimized.

Copy link
Member

jberger commented Mar 16, 2020

I was going to vote neutral, but @CandyAngel 's argument persuaded me. Voting yes (👍 )

@kraih kraih merged commit 5254b37 into master Mar 16, 2020
4 checks passed
4 checks passed
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@daleif

This comment has been minimized.

Copy link

daleif commented Mar 17, 2020

Doesn't this get messy as an interface when you have an optional value that is allowed to be the empty string, but if it is not then extra requirement.

Seems messy if everything then has to go though regex

I agree on the old setup

optional('field') -> size(3,10)

was annoying because field was undef it it had no value, but the requirement worked if the field had a value.

Martchus added a commit to openqabot/openQA that referenced this pull request Mar 19, 2020
This explicitely allows optional parameters to be empty when saving
needles. The code handles the case when one or all of these parameters
are empty just fine. This change merely restores the old behavior
before Mojolicious change mojolicious/mojo#1482
was introduced.
Martchus added a commit to openqabot/openQA that referenced this pull request Mar 19, 2020
This explicitely allows optional parameters to be empty when saving
needles. The code handles the case when one or all of these parameters
are empty just fine. This change merely restores the old behavior
before Mojolicious change mojolicious/mojo#1482
was introduced.
@tim-2

This comment has been minimized.

Copy link
Contributor

tim-2 commented Mar 20, 2020

I agree with daleif. How do I handle the case that e.g. I expect an optional number. The browser always transmits an empty input field as empty string.

In this minimal example I always get has_error for an optional param. That doesn't seem right.

use Mojolicious::Lite;

get '/' => sub {
    my $c = shift;

    my $v = $c->validation;
    $v->optional('n')->num(0, 9);
    
    $c->stash(
        msg => $v->has_data ? $v->has_error ? 'has_error' : 'ok' : 'nodata',
    );
} => 'index';

app->start;

__DATA__

@@ index.html.ep
%= $msg
<form>
    %= number_field 'n'
    %= submit_button
</form>
@kraih

This comment has been minimized.

Copy link
Member Author

kraih commented Mar 20, 2020

@tim-2 That case could probably only be handled with another alternative to required and optional.

@daleif

This comment has been minimized.

Copy link

daleif commented Mar 20, 2020

Would it even be possible to do chaining in the case where the value can either be empty or need to match conditions?

I had to change mine into

$v -> optional('field');
$v -> size(3,50)  if $v -> param;

which works but is perhaps not elegant.

@kraih

This comment has been minimized.

Copy link
Member Author

kraih commented Mar 20, 2020

There should probably be something like $v->optional('foo')->not_empty->size(3, 50). The not_empty method would simply remove empty string values from the foo input values.

@tim-2

This comment has been minimized.

Copy link
Contributor

tim-2 commented Mar 20, 2020

Yes, this would be a nice solution.

@kraih

This comment has been minimized.

Copy link
Member Author

kraih commented Mar 20, 2020

We can actually resolve this with a new filter. f410199...c0347bd

@daleif

This comment has been minimized.

Copy link

daleif commented Mar 21, 2020

So just returning undef is enough to end the chain? (I'm still new at this so pardon if that is obvious)

(one question, not 100% related, are a check allowed to change the value passed on? I had a check that verifies a date (input via a string like 2020-03-21) and it seemed convenient to just replace the input with the generated DateTime object)

@CandyAngel

This comment has been minimized.

Copy link
Member

CandyAngel commented Mar 22, 2020

@dalief

Checks don't change the value, but filters can.

You can try to convert it to an object in a filter and then check it is of that object. I do this but with Time::Piece and strptime.

$v->required(create_on => 'trim', 'dateify')->date;

@daleif

This comment has been minimized.

Copy link

daleif commented Mar 23, 2020

@CandyAngel thanks, that might be a better solution

I guess the answer should be: checks should not change the value, but filters can.

it is not too hard changing the value in a check (as I just did ;-), but one probably should not.

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

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.