gshank / dbix-class-resultset-recursiveupdate

DBIx::Class::ResultSet::RecursiveUpdate
4 stars 9 forks source link

RecursiveUpdate needs to check unique_constraint_names, not just PKs #1

Open SineSwiper opened 12 years ago

SineSwiper commented 12 years ago

There often may be cases where a table is bound to an integer id PK, but the "real" key is the name, which is uniquely constrained. And in those cases, you might not know the integer PK. For example:

owner => { name => "Thing" },

Thing already exists, so trying to INSERT this would error. However, DBIC:RS:RU isn't going to find the row, despite given a proper unique key to find it. So, you end up with an uniqueness error because it tries to INSERT it.

The $object find should look for all keys, not just PKs. I have a partial solution here:

for my $constraint (
    sort { $b eq 'primary' ? 1 : $a eq 'primary' ? -1 : $a cmp $b }  # make primary first
$source->unique_constraint_names) {
    last if defined $object; 

    # the updates hashref might contain the constraint columns
    # but with an undefined value
    my @missing =
        grep { !defined $updates->{$_} && !exists $fixed_fields{$_} }
        $source->unique_constraint_columns($constraint);

    #warn "MISSING $constraint: " . join(', ', @missing) . "\n";
    unless ( @missing ) {
        warn 'finding by: ' . Dumper( $updates ); use Data::Dumper;
        $object = $self->find( $updates, { key => $constraint } );
    }
    last if defined $object; 

    # add the resolved columns to the updates hashref
    $updates = { %$updates, %$resolved };

    # the resolved hashref might contain the constraint columns
    # but with an undefined value
    @missing = grep { !defined $resolved->{$_} } @missing;

    #warn "MISSING2 $constraint: " . join( ', ', @missing ) . "\n";
    unless ( @missing ) {
        warn 'finding by +resolved: ' . Dumper( $updates ); use Data::Dumper;
        $object = $self->find( $updates, { key => $constraint } );
    }

}

This works, but it's flawed in regards to the more complicated test cases. I think this is because the other primary_column cases haven't been converted yet. I need help with these because I don't completely understand what they do.

On the other hand, I do have a test case written for my problem above (96multi_create.t):

diag '* second create_related with same arguments';
eval {
    my $artist = $schema->resultset('Artist')->first;

    my $cd_result = $schema->resultset('CD')->recursive_update(
        {

            artist => $artist->artistid,

            title  => 'TestOneCD2',
            year   => 2007,
            tracks => [

                {   pos   => 111,
                    title => 'TrackOne',
                },
                {   pos   => 112,
                    title => 'TrackTwo',
                }
            ],

            liner_notes => { notes => 'I can haz liner notes?' },
            genre => { name => 'TestGenre' }

        }
    );

    ok( $cd_result && ref $cd_result eq 'DBICTest::CD', "Got Good CD Class" );
    ok( $cd_result->title eq "TestOneCD2",             "Got Expected Title" );
    ok( $cd_result->notes eq 'I can haz liner notes?', 'Liner notes' );

    my $tracks = $cd_result->tracks;

    ok( $tracks->isa("DBIx::Class::ResultSet"),
        "Got Expected Tracks ResultSet"
    );

    foreach my $track ( $tracks->all ) {
        ok( $track && ref $track eq 'DBICTest::Track',
            'Got Expected Track Class' );
    }
};
diag $@ if $@;

diag '* third create_related with non-primary unique key';
eval {
    my $artist = $schema->resultset('Artist')->first;

    my $cd_result = $schema->resultset('CD')->recursive_update(
        {

            artist => $artist->artistid,

            title  => 'TestOneCD2',
            year   => 2007,
            tracks => [

                {   pos   => 111,
                    title => 'TrackOne',
                },
                {   pos   => 112,
                    title => 'TrackTwo',
                }
            ],

            liner_notes => { notes => 'I can haz liner notes?' },
            genre => { name => 'TestGenre' }

        }
    );

    ok( $cd_result && ref $cd_result eq 'DBICTest::CD', "Got Good CD Class" );
    ok( $cd_result->title eq "TestOneCD2",             "Got Expected Title" );
    ok( $cd_result->notes eq 'I can haz liner notes?', 'Liner notes' );
    ok( $cd_result->genre->name eq 'TestGenre', 'Got Expected Genre' );

    my $tracks = $cd_result->tracks;

    ok( $tracks->isa("DBIx::Class::ResultSet"),
        "Got Expected Tracks ResultSet"
    );

    foreach my $track ( $tracks->all ) {
        ok( $track && ref $track eq 'DBICTest::Track',
            'Got Expected Track Class' );
    }
};
diag $@ if $@;

(The second create_related block is the same as the original with the added genre. The third create_related is a new test.)

ribasushi commented 8 years ago

( have not looked into this in detail, I am simply doing a pass on all the tickets in the distribution, making sure I am not missing something )

The above likely can be addressed by repurposing this (internal but rather stable) piece of logic: https://metacpan.org/source/RIBASUSHI/DBIx-Class-0.082840/lib/DBIx/Class/ResultSet.pm#L863-899