optional key in Struct but the value can't be undef
Closed this issue · 19 comments
i think im losing my mind here, how do i achieve the following with Data::Domain?
I want to allow { } but { name => undef } should fail.
so name can be omitted from the input HASH but it cant be undef.
tests dont seem to cover this case: t/data-domain.t
Code
use strict;
use warnings;
use feature qw/say/;
use Data::Dumper;
use Data::Domain qw/:all/;
my $domain = Struct(
-exclude => '*',
-fields => {
name => String(
-regex => qr/^[a-z0-9]{2,10}$/,
-optional => 1,
)
}
);
my @values = (
{},
{ name => undef },
);
for my $v (@values) {
my $err = $domain->inspect($v);
say '------------';
print 'input: ' . Dumper $v;
print 'success: ';
say $err ? 'NOTOK' : 'OK';
print 'details: ' . Dumper $err if $err;
}
output
------------
input: $VAR1 = {};
success: OK
------------
input: $VAR1 = {
'name' => undef
};
success: OK
Hi,
Thanks for your question, it's an interesting challenge.
The doc for "-optional" says "If true, the domain will accept undef, without generating an error message" ... so indeed, {name => undef} is accepted.
The current API for Struct() has no support for differentiating between a hash key containg undef or a hash key being absent. Usually this is not a problem because with %hash = (present => undef), expressions $hash{present} or $hash{absent} produce the same result. But your point is good : it would be nice to have an additional option for checking if a hash key exists or not. I'll try to to add this feature in the next version, but designing the proper API requires some thinking.
Meanwhile, what you can do is to add a condition requiring that all hash values must be defined :
my $domain = Struct(
-exclude => '*',
-fields => {
name => String(
-regex => qr/^[a-z0-9]{2,10}$/,
-optional => 1,
)
},
-values => List(-all => Whatever(-defined => 1)),
);
This should achieve the desired behaviour.
Best regards, Laurent
hi damil, thank you for the response.
yes your workaround seems to solve my issue. small remark ... the error message is quite cryptic with the workaround.
details: $VAR1 = {
'-values' => [
'Whatever: data defined/undefined',
undef
]
};
use strict;
use warnings;
use feature qw/say/;
use Data::Dumper;
use Data::Domain qw/:all/;
my $domain = Struct(
-exclude => '*',
-fields => {
first_name=> String(
-regex => qr/^[a-z0-9]{2,8}$/,
-optional => 1,
),
last_name => String(
-regex => qr/^[a-z0-9]{2,8}$/,
-optional => 1,
),
},
-values => List(-all => Whatever(-defined => 1)),
);
my @values = (
undef,
{},
{ first_name=> undef },
{ first_name=> 'a' },
{ first_name=> 'abc', last_name => undef },
{ first_name=> 'abc', last_name => 'def' },
{ first_name=> undef, last_name => 'def' },
);
for my $v (@values) {
my $err = $domain->inspect($v);
say '------------';
print 'input: ' . Dumper $v;
print 'success: ';
say $err ? 'NOTOK' : 'OK';
print 'details: ' . Dumper $err if $err;
}
Did you make a note of this issue somewhere else when you closed it? The workaround you described, while helping, does not really solve the issue at hand and the feedback that you get from the error messages are very unintuitive for a workflow where say.
{
first_name => 'required',
last_name => 'required'
}
is required for creating a resource and
{
first_name => 'optional_but_never_undef',
last_name => 'optional_but_never_undef'
}
for an update of an existing resource.
Well, I would like to add a kind of -exists condition, but so far I didn't find a syntax that would be pleasant and implementable.
The best candidate would be something like
Struct(-fields => {first_name => [-if_exists => String(...)],
last_name => ...})
but it is kind of awkward.
For the workaround, you could define your own message
my $domain = Struct(
-exclude => '*',
-fields => {
name => String(-regex => qr/^[a-z0-9]{2,10}$/, -optional => 1),
},
-values => List(-all => Defined(-messages => "can't be undef")),
);
but still this is not very satisfactory.
Another workaround would be to use a lazy domain
my $domain = Struct(
-exclude => '*',
-fields => {
name => sub {my $context = shift;
exists $context->{flat}{name} ? String( -regex => qr/^[a-z0-9]{2,10}$/) : Whatever},
},
);
can you reopen this issue? it doesnt seem to deserve a closed status yet.
ok reopened
additional info, I only want some of the Struct members to exist mandatorily. so passing -defined for -values doesnt seem to work.
I played around and added a -mandatory attribute to Data::Domain::Struct that takes a arrayref.
this doesnt break the existing API.
$ diff -u Domain.pm Domain.pm.orig
--- Domain.pm 2023-11-30 18:34:34.346956963 +0000
+++ Domain.pm.orig 2023-11-29 09:49:43.179438416 +0000
@@ -1156,12 +1156,11 @@
use warnings;
use Carp;
use Scalar::Does qw/does/;
-use List::MoreUtils qw/any/;
our @ISA = 'Data::Domain';
sub new {
my $class = shift;
- my @options = qw/-fields -exclude -keys -values -mandatory/;
+ my @options = qw/-fields -exclude -keys -values/;
my $self = Data::Domain::_parse_args(\@_, \@options, -fields => 'arrayref');
bless $self, $class;
@@ -1230,19 +1229,13 @@
path => []};
local $context->{flat} = {%{$context->{flat}}, %$data};
- my $mandatory = $self->{-mandatory} || [];
# check fields of the domain
foreach my $field (@{$self->{-fields_list}}) {
- if ( (any { $_ eq $field } $mandatory->@*) && !exists $data->{$field} ) {
- $msgs{$field} = 'mandatory.';
- }
- else {
- local $context->{path} = [@{$context->{path}}, $field];
- my $field_spec = $self->{-fields}{$field};
- my $subdomain = $self->_build_subdomain($field_spec, $context);
- my $msg = $subdomain->inspect($data->{$field}, $context);
- $msgs{$field} = $msg if $msg;
- }
+ local $context->{path} = [@{$context->{path}}, $field];
+ my $field_spec = $self->{-fields}{$field};
+ my $subdomain = $self->_build_subdomain($field_spec, $context);
+ my $msg = $subdomain->inspect($data->{$field}, $context);
+ $msgs{$field} = $msg if $msg;
}
Hi,
Thanks for the proposal.
Yes, the Struct domain should have one additional information to distinguish which fields are mandatory or not.
An additional arrayref could do the job, I was considering something like that. But a list of mandatory fields would break backward compatibility for clients that currently use Struct(field_name => SomeDomain(-optional => 1)). A list of non-mandatory fields would do, but it would be confusing with respect to the existing -optional flag.
So far the best variant I could imagine would be something like
Struct(
a => SomeDomain, # must exist, must be in SomeDomain
b => SomeDomain(-optional => 1), # can be absent or can be undef or must be in SomeDomain
c => [-if_exists => SomeDomain], # can be absent or must be in SomeDomain; cannot be undef
)
-optional only behaves differently if you actually add the -mandatory flag. without it everything works as usual. breaking backward compatibility would mean the same code breaks when Data::Domain is updated.
your -if_exists proposal seems to make it hard to reuse existing Data::Domain objects. with my -mandatory proposal you can use the same MyDomain() object and change it by passing MyDomain(-mandatory => qw [...]) you cant do that with if_exists, right?
especially for a use case where you want to use the same Data::Domain object for CREATE as well as UPDATE.
sub MyDomain {
Struct(-fields => {
first_name => String(-min_length => 3),
middle_name => String(-min_length => 3, -optional => 1),
last_name => String(-min_length => 3),
}
);
}
#use this for CREATE
MyDomain(-mandatory => [qw(first_name last_name)]);
#use this for UPDATE
MyDomain();
Sorry, I don't see the point. In the current version, fields are already mandatory by default. You have to explicitly say -optional for fields that are not mandatory. But this "optional" means that the field can either be absent or can be undef, which is what you want to avoid. So we need a syntax for specifying that we want an "exists" test.
But I like your idea of having different domains for CREATE and for UPDATE -- just need to find the proper way to accomplish that
Have a look at the last commits.
I added a -may_ignore option; it can take either an arrayref as you suggested, but also a regex or the '*' constant, like the -exclude option. There are some examples in the doc.
Waiting for your feedback before publishing to CPAN
ill get back to you tomorrow probably, thank you for your effort.
Struct(
-fields => { last_name => String() },
-may_ignore => '*',
);
the error message for
{ last_name => undef }
and
{ }
is the same.
'last_name' => 'String: undefined data'
I hope for a way to distinguish these two cases. the current error messages means to me that last_name was provided but undefined.
UPDATE: sorry, there is an error in this example, the same error message only occurs if you DONT set
-may_ignore
Something must be wrong; can you show me your code ?
Here is my test :
use strict;
use warnings;
use feature 'say';
use Data::Domain qw/:all/;
use Data::Dumper;
say "using Data::Domain version $Data::Domain::VERSION";
my $dom = Struct(-fields => { last_name => String() }, -may_ignore => '*');
my %tests = (normal => { last_name => 'Beethoven' },
undefined => { last_name => undef },
absent => {},
);
while (my ($name, $data) = each %tests) {
my $msg = $dom->inspect($data);
say "$name : ", $msg ? Dumper($msg) : 'OK';
}
yielding result
using Data::Domain version 1.13
normal : OK
absent : OK
undefined : $VAR1 = {
'last_name' => 'String: undefined data'
};
my bad, what I mean is that if you dont set -may_ignore Data::Domain cannot distinguish between undef and not-existing.
if you remove the -may_ignore setting from your example you get the following output
undefined : $VAR1 = {
'last_name' => 'String: undefined data'
};
absent : $VAR1 = {
'last_name' => 'String: undefined data'
};
same error message for clearly different input.
yes, sure, this is what you get with the current version, because the initial design didn't make any distinction between absent fields and fields containing undef. Maybe it was not the best choice, but I will not change this basic design, because that would be an abrupt change of behaviour, creating problems for other clients (including my own employer).
I hope you can attain your goals using the new attribute '-may_ignore'.
Otherwise your other option is to create your own subclass of Data::Domain where you implement the behaviour you need.
v1.13 is on CPAN