Deep nested objects fix (RT Bug #81236)

[dim0xff]

Wrong packing/unpacking for nested objects

my $obj = StorableClass->new(
    h => {
         a => [ StorableClass->new(s => 'value'),
         ...
    ],
    ...
} );
my $hashref = $obj->pack;

Packs into:

{
    __CLASS__ => 'StorableClass',
    h => {
        a => [ bless({s => 'value'}, 'StorableClass') ]    # Blessed! XXX
    }
}

[karenetheridge]

On Fri, May 30, 2014 at 03:07:21AM -0700, Dmitry Latin wrote:

Wrong packing/unpacking for nested objects

Awesome, thanks! I hope to circle back to doing some maintenance on MooseX::Storage in the next week or so, and will include your patch.

[karenetheridge]

Thanks! I've merged this with some very small amendments for style to minimize the diffs, and squashed the test commits together with their corresponding .pm fixes.

[karenetheridge]

I'm terribly sorry, but I've reverted these changes in release 0.50. While mostly only new features are added, some existing behaviour has changed (see https://rt.cpan.org/Ticket/Display.html?id=104106), and until we can at least successfully pack/unpack as many objects as we could before, this feature can't be released as stable.

I've added a TODO test (t/080) which demonstrates a usecase that doesn't work that ought to, and created a new branch (https://github.com/moose/MooseX-Storage/tree/topic/deep-pack-unpack) which re-adds these changes, along with one fix that I was able to identify.

The unpacking case for ArrayRef[JSON::PP::Boolean] is tricky -- it's going to require passing around the type constraint into the expand subs (probably as one of the @args, after the option hash) so we can recurse down into it. I played around with it a little bit while I was hoping to still fix this, but it was getting ugly, so I decided that reverting now and then taking more time to consider a good solution would be better.

Sorry again. This is a desirable feature, but it needs more iterations and testing first (and a -TRIAL release - that was an error on my part sending it out as stable right away).

[dim0xff]

Probably JSON::PP::Boolean type handler should look like here:

MooseX::Storage::Engine->add_custom_type_handler(
    'JSON::PP::Boolean' => (
        expand   => sub { $_[0]{__value} ? JSON::PP::true : JSON::PP::false },
        collapse => sub { { __CLASS__ => 'JSON::PP::Boolean', __value => "$_[0]" } },
    )
);

Because 1 and [0, 1] from

{
    __CLASS__ => 'Foo',
    one_bool => 1,
    many_bools => [ 0, 1 ],
}

couldn't be converted into objects.

PS: actually it could, but ArrayRef[JSON::PP::Boolean] should be a Moosified type with coercion, I think.

[karenetheridge]

collapse => sub { { __CLASS__ => 'JSON::PP::Boolean', __value => "$_[0]" } },

This doesn't work -- anything used in __CLASS__ must be a Moose class with a meta method.

It should be possiblefor the type handlers to handle any type -- the trick is that the ArrayRef and HashRef expand subs need to recurse into the type constraint to find the root type (and if we're unpacking something like HashRef[ArrayRef[JSON::PP::Boolean]] this needs to keep recursing until the bottom).

I had an experimental code diff that looked like this:

      # These are the trickier ones, (see notes)
      'ArrayRef' => {
          expand => sub {
--            my ( $array, @args ) = @_;
++            my ( $array, $options, $type_constraint ) = @_;
++# XXX we need to know what the attribute is that we are expanding!!!
++# we could get this passed along in @args!!!
++print "### in ArrayRef expansion for array ", Dumper($array);
              foreach my $i (0 .. $#{$array}) {
++                my $element_type_constraint = $type_constraint && $type_constraint->can('type_parameter') ? $type_constraint->type_parameter : undef;
                  if (ref($array->[$i]) eq 'HASH') {
                      $array->[$i] = exists($array->[$i]{$CLASS_MARKER})
--                        ? $OBJECT_HANDLERS{expand}->($array->[$i], @args)
--                        : $TYPES{HashRef}{expand}->($array->[$i], @args);
++                        ? $OBJECT_HANDLERS{expand}->($array->[$i], $options)
++                        : $TYPES{HashRef}{expand}->($array->[$i], $options);
                  }
                  elsif (ref($array->[$i]) eq 'ARRAY') {
--                    $array->[$i] = $TYPES{ArrayRef}{expand}->($array->[$i], @args);
++# XXX this case might degrade into the third case...
++                    $array->[$i] = $TYPES{ArrayRef}{expand}->($array->[$i], $options);
++                }
++                elsif ($element_type_constraint
++                       and my $handler = eval { __PACKAGE__->find_type_handler($element_type_constraint, $array->[$i]) }) {
++print "### here is the OTHER case for expanding an arrayref element -- need moar code here\n";
++# XXX look at the element type constraint parameter type...
++# if we have an entry for it, then recurse!
++# maybe this needs to be the first check we do, not the last?
++                    $array->[$i] = $handler->{expand}->($array->[$i], $options, $element_type_constraint);
                  }
              }
              $array;

[dim0xff]

Take a look to last commit 7e73099 I suggest the idea to prefer using $TYPES expand/collapse when exists, otherwise fallback to default $OBJECT_HANDLERS expand/collapse (via ->unpack method)