Bug #86287 for Data-FormValidator: Empty optional values that would normally be skipped are run through the validation routines if there are multiple values

Thu Jun 20 15:35:12 2013 wonko [...] cpan.org - Ticket created

Subject:

Empty optional values that would normally be skipped are run through the validation routines if there are multiple values

It's not uncommon to have multiple fields with the same name, but not require that they all (or any of them) be filled in. If this is the case D::FV will run the constraints on the missing (undef) values when they would normally be skipped. And since constraint routines can't return undef+but+true it will fail. For example, this (an optional field w 2 defined valid values) doesn't fail: perl -MData::FormValidator -le 'print Data::FormValidator->check({foo => [1,2]}, {optional => q(foo), constraint_methods => { foo => qr/^\d$/ }})->invalid;' Neither does this (an optional field w 2 undef values): perl -MData::FormValidator -le 'print Data::FormValidator->check({foo => [undef, undef]}, {optional => q(foo), constraint_methods => { foo => qr/^\d$/ }})->invalid;' But if you mix them, it fails on the undef (missing) value: perl -MData::FormValidator -le 'print Data::FormValidator->check({foo => [1,undef]}, {optional => q(foo), constraint_methods => { foo => qr/^\d$/ }})->invalid;' It should treat the 2nd value as just missing and not run it through the validation routine since the field itself is optional.

Thu Jun 20 15:45:17 2013 wonko [...] cpan.org - Correspondence added

This patch fixes this problem and doesn't cause any failures in the test suite.

Subject:

rt_86287.patch

--- lib/Data/FormValidator/Results.pm 2012-11-01 11:11:19.000000000 -0400 +++ /home/mpeters/development/arcos/lib/Data/FormValidator/Results.pm 2013-06-20 15:40:19.544633425 -0400 @@ -201,12 +201,10 @@ for my $field (keys %valid) { if (ref $valid{$field}) { if ( ref $valid{$field} eq 'ARRAY' ) { - for (my $i = 0; $i < scalar @{ $valid{$field} }; $i++) { - $valid{$field}->[$i] = undef unless (defined $valid{$field}->[$i] and length $valid{$field}->[$i] and $valid{$field}->[$i] !~ /^\x00$/); - } + # remove any undef/empty values + @{$valid{$field}} = grep { defined $_ && length $_ && $_ !~ /^\x00$/ } @{$valid{$field}}; # If all fields are empty, we delete it. - delete $valid{$field} unless grep { defined $_ } @{$valid{$field}}; - + delete $valid{$field} unless @{$valid{$field}}; } } else {

Thu Jun 20 16:06:45 2013 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #86287] Empty optional values that would normally be skipped are run through the validation routines if there are multiple values
Date:	Thu, 20 Jun 2013 16:06:28 -0400
To:	bug-Data-FormValidator [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

> But if you mix them, it fails on the undef (missing) value: > > perl -MData::FormValidator -le 'print Data::FormValidator->check({foo > => [1,undef]}, {optional => q(foo), constraint_methods => { foo => > qr/^\d$/ }})->invalid;' > > It should treat the 2nd value as just missing and not run it through > the validation routine since the field itself is optional.

Michael, Thanks for the feedback. I agree that we treat missing optional fields as just missing. But we also treat fields as atomic: The field itself is missing, invalid or valid. A field is /not/ treated as a collection of values, of which each may be missing, invalid, or valid. So, if a field is a "half valid", we have to decide: Is it valid or not. The safe answer is "not valid". You may be agreeing so far, but arguing that in this case a "half missing" field should be treated as "missing", instead of running it through validation and treating the field as "invalid". Is that right? I would see either outcome as about equally true-- the field is not really missing-- it has one value. It's also not really invalid either-- it has one valid value. Given the imperfect options, I would just assume leave the current behavior. I think what I would recommend is to use your own filter on that field which throws away undef values in a case like this. I don't think we can declare that the behavior is generally desirable, though. Consider a value like this: [1,undef,3] You can't throw away the middle value without collapsing the array, and it might be meaningful that the "3" is the third value, and not the second. Mark

Thu Jun 20 16:06:46 2013 The RT System itself - Status changed from 'new' to 'open'

Thu Jun 20 16:16:36 2013 wonko [...] cpan.org - Correspondence added

On Thu Jun 20 16:06:45 2013, mark@summersault.com wrote: Show quoted text

> You may be agreeing so far, but arguing that in this case a "half > missing" field should be treated as "missing", instead of running it > through validation and treating the field as "invalid". Is that right?

Kind of, except that this is an optional field so it's not missing either. It just doesn't get validated. If this was a required field then I agree the whole field should be marked as missing. Show quoted text

> I don't think we can declare that the behavior is generally desirable, > though. Consider a value like this: > > [1,undef,3] > > You can't throw away the middle value without collapsing the array, and > it might be meaningful that the "3" is the third value, and not the > second.

That makes sense. So would you accept a different patch that has the same results (skips validations for optional fields with missing values) that doesn't also remove those missing values from the data structure?

Thu Jun 20 16:53:55 2013 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #86287] Empty optional values that would normally be skipped are run through the validation routines if there are multiple values
Date:	Thu, 20 Jun 2013 16:53:38 -0400
To:	bug-Data-FormValidator [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

>> [1,undef,3] >> >> You can't throw away the middle value without collapsing the array, and >> it might be meaningful that the "3" is the third value, and not the >> second.

> > That makes sense. So would you accept a different patch that has the > same results (skips validations for optional fields with missing > values) that doesn't also remove those missing values from the data > structure?

While I agree the the current situation isn't perfect, there's still some risk with the proposed change. Normally DFV doesn't return "undef" as a valid value-- it returns it as "missing". With the proposed patch, you could get back a result like this, expecting that all the values had passed validation: [1,undef,3] You aren't supposed to need to recheck or re-validate after DFV gets done, but with the proposed patch you might need to re-check that you have a real value for each of 3 values returned, because now one value is undef-but-valid. I don't feel like there's a globally applicable intuitive solution that can be applied here. I think good choices include filter'ing out the undefs if that's appropriate for your project, or even considering a design where fields are named differently, and only single values are accepted. The regex options allow to easily work with similiarly named fields, marking them all required, optional in one go, or apply constraints. Something like CGI::Expand names it easy to unpack fields that are named like "field.0, "field.1", etc: https://metacpan.org/module/CGI::Expand For my own work, I plan to use FV_num_values(1) more (from DFV 4.80), ideally set it as a default, so I only get one value unless I explicitly expect more. Mark

Fri Jul 19 08:12:12 2013 MARKSTOS [...] cpan.org - Correspondence added

No further feedback. Closing. Re-open if there's more to discuss here.

Fri Jul 19 08:12:13 2013 MARKSTOS [...] cpan.org - Status changed from 'open' to 'rejected'

Bug #86287 for Data-FormValidator: Empty optional values that would normally be skipped are run through the validation routines if there are multiple values

Maintainer(s)' notes