Skip Menu |

This queue is for tickets about the DBIx-Class-Fixtures CPAN distribution.

Report information
The Basics
Id: 37510
Status: open
Priority: 0/
Queue: DBIx-Class-Fixtures

People
Owner: Nobody in particular
Requestors: JJSCHUTZ [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: 1.001000
Fixed in: (no value)



Subject: Fixtures fails to dump related class if class already seen through prior relationship
Suppose A belongs to B, B might have C, C has many D. I want to specify a particular A and naturally expect to have all its related B, C, D dumped as well. So json config would be: has_many: { fetch: '1', quantity: 'all' }, might_have: { fetch: '1' }, belongs_to: { fetch: '1' }, sets: [{ class: 'A', ids: [ '1' ] }] A, B, C get dumped; A is in 'sets' so gets dumped first, then B gets processed as a relationship of A, then C as a rel of B (since recursive dump options (_inherited_attributes) include might_have) but not C's relationships, since 'has_many' is not an inherited attribute. That D is not automatically dumped is an annoyance, but not the main issue in this bug report. (Sidenote: Adding has_many and belongs_to to _inherited_attributes causes deep recursion - perhaps there is a better way to detect which objects have already been processed, such as a hash of class name and primary key, so recursion stops immediately on an already processed object? Then all types of relationships could be followed, maybe?) To work around this behaviour, I added 'C' to the sets: sets: [{ class: 'A', ids: [ '1' ] }, { class: 'C', ids: [ '7' ] }] But still C's relationship D does not get dumped, because C has already been dumped (the 'exists' file test in dump_object causes the relationship recursion to be bypassed). That's the bug. If I just specify C: sets: [{ class: 'C', ids: [ '7' ] }] then D is dumped as expected. If C was processed first, the result would be different, but as classes are processed in alphabetical order, D is never dumped regardless of the ordering in 'sets'. Asymmetrical behaviour! The workaround appears to be that a rule must exist for the problem class: rules: { C: { fetch: [{ rel: 'D', quantity: 'all' }] } } but that is not obvious (took me several hours to work out) and not desirable - it would be nice if it just "worked".
Please find suggested patch attached. This implements breadth-first recursion instead of depth-first, so all the top level items listed in 'sets' get processed first, then related items. This also solved deep recursion problems that I was seeing when setting rules to follow has_many type relationships.
--- DBIx-Class-Fixtures-1.001000/lib/DBIx/Class/Fixtures.pm 2008-07-09 14:25:50.000000000 +0930 +++ DBIx-Class-Fixtures-1.001000.orig/lib/DBIx/Class/Fixtures.pm 2008-04-26 02:02:53.000000000 +0930 @@ -494,7 +494,6 @@ $config->{rules} ||= {}; my @sources = sort { $a->{class} cmp $b->{class} } @{delete $config->{sets}}; my %options = ( is_root => 1 ); - $self->{queue} = []; foreach my $source (@sources) { # apply rule to set if specified my $rule = $config->{rules}->{$source->{class}}; @@ -540,10 +539,6 @@ } } - while (my $entry = shift @{$self->{queue}}) { - $self->dump_object(@$entry); - } - foreach my $dir ($output_dir->children) { next if ($dir eq $tmp_output_dir); $dir->remove || $dir->rmtree; @@ -581,8 +576,6 @@ # write file my $exists = (-e $file->stringify) ? 1 : 0; - return if $exists; - unless ($exists) { $self->msg('-- dumping ' . $file->stringify, 2); my %ds = $object->get_columns; @@ -647,7 +640,7 @@ # use Data::Dumper; print ' -- ' . Dumper($c_params{set}, $rule->{fetch}) if ($rule && $rule->{fetch}); $c_params{set} = merge( $c_params{set}, $rule) if ($rule && $rule->{fetch}); # use Data::Dumper; print ' -- ' . Dumper(\%c_params) if ($rule && $rule->{fetch}); - $self->_queue_object($_, \%c_params) foreach $related_rs->all; + $self->dump_object($_, \%c_params) foreach $related_rs->all; } } } @@ -675,16 +668,10 @@ $related_rs = $related_rs->search($fetch->{cond}, { join => $fetch->{join} }) if ($fetch->{cond}); $related_rs = $related_rs->search({}, { rows => $fetch->{quantity} }) if ($fetch->{quantity} && $fetch->{quantity} ne 'all'); $related_rs = $related_rs->search({}, { order_by => $fetch->{order_by} }) if ($fetch->{order_by}); - $self->_queue_object($_, { %{$params}, set => $fetch }) foreach $related_rs->all; + $self->dump_object($_, { %{$params}, set => $fetch }) foreach $related_rs->all; } } -sub _queue_object { - my ($self, $object, @params) = @_; - - push(@{$self->{queue}}, [ $object, @params ]); -} - sub _generate_schema { my $self = shift; my $params = shift || {};
On Wed Jul 09 01:16:09 2008, JJSCHUTZ wrote: Show quoted text
> Please find suggested patch attached. > > This implements breadth-first recursion instead of depth-first, so all > the top level items listed in 'sets' get processed first, then related > items. > > This also solved deep recursion problems that I was seeing when setting > rules to follow has_many type relationships.
Thanks for the patch. However I don't think it is acceptable to simply return if the object already exists as the relationship settings may be different on subsequent passes. So we still need to look at its relations each time it's attempted to be dumped. For example: A has_many B B has_many C B has_many D sets: [{ class: 'A', ids: [ '1' ], fetch: [{ rel: 'b', quantity: 'all', fetch: [{ rel: 'c', quantity: 'all' }] }] }, { class: 'B', quantity: 'all', fetch: [{ rel: 'd', quantity: 'all' }] }] The first set here dumps some 'B' objects and all of their 'C' relations. The second set dumps all 'B' objects and all of their 'D' relations. If we skip processing of an object that has already been dumped then the 'B' objects from the first set wouldn't have both their 'c' and 'd' relations dumped as is expected. I think the problem you illustrate is that the general has_many, belongs_to and might_have relations are ignored if an object has already been dumped whereas the 'fetch' relations are processed anyway. However allowing these rels to be processed repeatedly is going to cause even more recursion problems I suspect. I agree entirely that it would be nice if the has_many setting were inherited, but we need to be smart about it. I think the solution is that instead of just returning if the object exists, some extra check should be made to determine if there's anything new in the set config and if so just process the new rels, otherwise return. I hope that makes sense. And I'm sorry that you had to go digging around to such a degree.