Wishlist: Object construction should fail if Moose attributes not correct

[mjemmeson]

Hi,

It would be nice if instead of returning an object in the event of failure to parse the XML satisfactorily, Moose arguments like 'required' were honoured.

Looking at the code, using "required" doesn't seem to be an option at present - because of the use of 'default'. With a little playing it was possible to get a mismatched types error (e.g. "Attribute (method) does not pass the type constraint because: Validation failed for 'Str' with value undef") but nothing better.

Ideally, setting "required => 1" should cause construction to fail if the node isn't found. I was wondering if you had had any thoughts on this?

thanks,
Michael

edit - after looking further (to see if I could get it to die on invalid XML) it looks like the document parsing is lazy - is there a reason for that?

[robinsmidsrod]

@mjemmeson I'm sorry for the extremely long wait. The last question I can answer first. The reason why everything is lazy-evaluated is because it's just what I'm used to do, so that memory and CPU resources are not used for anything unless it is needed.

Back to the first question, what do you mean by "parsing the XML satisfactorily"? Does that just mean that the XML is well-formed, or that it also confirms to a schema or anything else? Right now the only test which is performed on the XML is that it is well-formed, and that is done by XML::LibXML when it constructs the DOM tree used for the xpath queries.

Can you explain what kind of behavior you'd like to be able to support with this required => 1 syntax? Some example XML data and example perl code for how you'd like to use XML::Rabbit would be awesome.

[mjemmeson]

Hi,
Thanks for the reply, and sorry my original post wasn't clear.

I think there are two issues here actually:

1. it is impossible to check if the XML is well formed without performing an attribute call, which feels messy, and means there is calls to code unrelated to what the user is trying to do.

    my $model = $class->new( xml => $broken_xml );
    unless ($model->foo) { # picking a random attribute, dies if XML not well formed
         die "Invalid XML"; # dies if <foo> node not set 
    }

Maybe a possible solution is to provide a method which calls the lazy document builder in XML::Rabbit::Role::Document

     my $model = $class->new( xml => $broken_xml );
     unless ($model->is_valid) {
          die "Invalid XML";
     }

Another would be to check at construction time, something like

    my $model = eval { $class->new( xml => $broken_xml, validate => 1 ) };
    die "Invalid XML: $@" if $@;

2. The "required" Moose attribute - I think I was hoping it would be possible to check for the presence of a node (even if empty), rather like "required" checks the attribute is specified in the constructor, since the XML is behaving like the constructor arguments here.

    package Foo;
    use 'XML::Rabbit::Root';
    has_xpath_value bar => '/bar', required => 1;
    
    ....
    
    $xml = q{<?xml version="1.0">
    <foo/>
    };
    ...
    my $obj = Foo->new( xml => $xml ); # dies because '/bar' is not present

I hope that is a bit clearer.

thanks again,
Michael

[robinsmidsrod]

@mjemmeson I've added a branch that add a method to root classes called validate_document_xml(). This should solve the first issue you've mentioned above. It also doesn't change any existing behavior, so existing use-cases should not be affected.

I'm wondering if it is better to return a boolean value (as I've implemented now) or the underlying XML::LibXML document ($self->_document) when this method succeeds. I'm wondering which API is the better one. Perl Best Practices (the book) advocates against using validate_XXX as method names, as it doesn't make its behavior obvious. I was thinking about calling it is_well_formed_xml, but then it shouldn't throw an exception, and then we'd need an additional attribute to store the exception message from the XML parser. I think it's better to throw an exception if it fails (and not just a boolean false value), as this gives us access to the actual XML parser error message. What do you think?

For your second issue I'm wondering how we could do this in the best way. The required parameter to the Moose has function indicates that this is a required constructor parameter, which makes no sense for a lazy Moose attribute. If this doesn't generate a Moose error it certainly should. So if we use the required parameter to any of our has_xpath_XXX sugar functions it must surely be removed before the parameters are passed to the Moose has attribute constructor function. I'm unsure if a required parameter should end up doing an exists or defined check against the XML document. Does XML::LibXML return different values if an element attribute exists but has no value and if the element attribute is not there at all? I would expect the first one to return an empty string and the other one an undef value. I'm not sure if I've explained my concern well here. One thing that is also important is that the core behavior and the sugar functions need to end up implementing the same behavior. That's why I think we should avoid using the required parameter. I'm thinking exists and/or defined might be better, but it doesn't roll off the tongue like required does. Maybe going for an inverse behavior by declaring optional => 1 for any attribute that is not required to be present. What do you think? Does this actually change any defined behavior? I think most of the documentation assumes that all XML elements and attributes are required, but I don't think it's actually explained anywhere.

[robinsmidsrod]

@mjemmeson One thing I forgot to mention in the previous comment is that the dump_document_xml, validate_document_xml and dump_xml methods are meta methods that has nothing to do with the class that is actually implemented. They are XML::Rabbit utility methods. I'm thinking that all of these methods should be callable on some sort of meta-class to avoid polluting the method namespace of the class you're implementing. Do you have any idea how to implement this and an API that would make sense? My Moose-fu is not the best when it comes to metaclass magic. I had a lot of help to implement what XML::Rabbit currently does in a moosified way. Pull requests are more than welcome!