From: Mike Wisener Date: Fri, 24 Jan 2014 03:09:17 +0000 (+0000) Subject: allow creating multiple related columns when updating or creating new records X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Controller-DBIC-API.git;a=commitdiff_plain;h=88b67dcdf3e0e7eec62209595a8a90f142da308d allow creating multiple related columns when updating or creating new records fixes RT#65168 --- diff --git a/Changes b/Changes index 3840398..454929e 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,8 @@ Revision history for Catalyst-Controller-DBIC-API: {{ $dist->version }} {{ $NEXT }} + - Allow creating multiple related objects when updating or creating new + records (RT#65168) 2.005001 2014-01-13 21:43:59+01:00 Europe/Vienna - Fixed test failures with JSON 2.90 (RT#90188, thanks Samuel Kaufman!) diff --git a/dist.ini b/dist.ini index ba1412d..fb33f8f 100644 --- a/dist.ini +++ b/dist.ini @@ -6,6 +6,7 @@ author = Alexander Hartmaier author = Florian Ragwitz author = Oleg Kostyuk author = Samuel Kaufman +author = Mike Wisener license = Perl_5 copyright_holder = Luke Saunders, Nicholas Perez, Alexander Hartmaier, et al. diff --git a/lib/Catalyst/Controller/DBIC/API.pm b/lib/Catalyst/Controller/DBIC/API.pm index 7cfb6e6..a60ef81 100644 --- a/lib/Catalyst/Controller/DBIC/API.pm +++ b/lib/Catalyst/Controller/DBIC/API.pm @@ -686,22 +686,28 @@ sub validate_object { if ( ref $allowed_fields ) { my $related_source = $object->result_source->related_source($key); - my $related_params = $params->{$key}; + + # Could be an arrayref of hashrefs, or just a hashref. + my $related_rows = ref($params->{$key}) eq "ARRAY" ? $params->{$key} : [ $params->{$key} ]; my %allowed_related_map = map { $_ => 1 } @$allowed_fields; my $allowed_related_cols = ( $allowed_related_map{'*'} ) ? [ $related_source->columns ] : $allowed_fields; - foreach my $related_col ( @{$allowed_related_cols} ) { - if (defined( - my $related_col_value = - $related_params->{$related_col} - ) - ) - { - $values{$key}{$related_col} = $related_col_value; + foreach my $related_row ( @$related_rows ) { + my %valid_row; + foreach my $related_col ( @{$allowed_related_cols} ) { + if (defined( + my $related_col_value = + $related_row->{$related_col} + ) + ) + { + $valid_row{$related_col} = $related_col_value; + } } + push(@{$values{$key}}, \%valid_row) if (keys %valid_row); } } else { @@ -843,34 +849,43 @@ with the specified parameters. =cut sub update_object_relation { - my ( $self, $c, $object, $related_params, $relation ) = @_; - my $row = $object->find_related( $relation, {}, {} ); + my ( $self, $c, $object, $related_records, $relation ) = @_; + my $row_count = scalar(@$related_records); - if ($row) { - foreach my $key ( keys %$related_params ) { - my $value = $related_params->{$key}; - if ( ref($value) && !( reftype($value) eq reftype(JSON::true) ) ) - { - $self->update_object_relation( $c, $row, - delete $related_params->{$key}, $key ); - } + # validate_object should always wrap single related records in [ ] + while (my $related_params = pop @$related_records) { - # accessor = colname - elsif ( $row->can($key) ) { - $row->$key($value); - } + # if we only have one row, don't need to worry about introspecting + my $row = $row_count > 1 + ? $self->find_related_row( $object, $relation, $related_params ) + : $object->find_related($relation, {}, {} ); - # accessor != colname - else { - my $accessor = - $row->result_source->column_info($key)->{accessor}; - $row->$accessor($value); + if ($row) { + foreach my $key ( keys %$related_params ) { + my $value = $related_params->{$key}; + if ( ref($value) && !( reftype($value) eq reftype(JSON::true) ) ) + { + $self->update_object_relation( $c, $row, + delete $related_params->{$key}, $key ); + } + + # accessor = colname + elsif ( $row->can($key) ) { + $row->$key($value); + } + + # accessor != colname + else { + my $accessor = + $row->result_source->column_info($key)->{accessor}; + $row->$accessor($value); + } } + $row->update(); + } + else { + $object->create_related( $relation, $related_params ); } - $row->update(); - } - else { - $object->create_related( $relation, $related_params ); } } @@ -907,10 +922,45 @@ sub insert_object_from_params { $object->insert; while ( my ( $k, $v ) = each %rels ) { - $object->create_related( $k, $v ); + foreach my $row (@$v) { + $object->create_related( $k, $row ); + } } } +=method_protected find_related_row + +Attempts to find the related row by introspecting the result source and determining +if we have enough data to properly update. + +=cut + +sub find_related_row { + my ($self, $object, $relation, $related_params) = @_; + + # make a shallow copy, grep + hash slicing and autovivication creates undef + # values in the hash we operate on. + my $search_params = { %$related_params }; + + my @pri = $object->result_source->related_source($relation)->primary_columns; + my @have = grep { defined($_) } @{$search_params}{ @pri }; + if (@have && @pri == @have) { + return $object->find_related($relation, @have, { key => 'primary' }) + } + + # if we were not passed a pri key, see if we meet any unique_constaints + my %constraints = $object->result_source->related_source($relation)->unique_constraints; + while ( my ($constraint_name,$constraints) = each %constraints ) { + my @needed = @$constraints; + my @have = grep { defined($_) } @{$search_params}{ @needed }; + return $object->find_related($relation, @have, { key => $constraint_name }) + if (@have && @have == @needed); + } + + # didn't find anything + return undef; +} + =method_protected delete_objects Iterates through each object calling L. diff --git a/t/lib/RestTest/Controller/API/REST/Artist.pm b/t/lib/RestTest/Controller/API/REST/Artist.pm index d2ceada..ea5a4bc 100644 --- a/t/lib/RestTest/Controller/API/REST/Artist.pm +++ b/t/lib/RestTest/Controller/API/REST/Artist.pm @@ -7,8 +7,8 @@ __PACKAGE__->config ( action => { setup => { PathPart => 'artist', Chained => '/api/rest/rest_base' } }, class => 'RestTestDB::Artist', create_requires => ['name'], - create_allows => ['name'], - update_allows => ['name'], + create_allows => ['name', { cds => ['*'] }], + update_allows => ['name', { cds => ['*'] }], prefetch_allows => [[qw/ cds /],{ 'cds' => 'tracks'}], ); diff --git a/t/lib/RestTest/Controller/API/RPC/Artist.pm b/t/lib/RestTest/Controller/API/RPC/Artist.pm index 2d0367d..e9a23cd 100644 --- a/t/lib/RestTest/Controller/API/RPC/Artist.pm +++ b/t/lib/RestTest/Controller/API/RPC/Artist.pm @@ -8,9 +8,9 @@ __PACKAGE__->config ( action => { setup => { PathPart => 'artist', Chained => '/api/rpc/rpc_base' } }, class => 'RestTestDB::Artist', create_requires => ['name'], - create_allows => ['name'], - update_allows => ['name'], + create_allows => ['name', { cds => ['*'] }], + update_allows => ['name', { cds => ['*'] }], prefetch_allows => [[qw/ cds /],{ 'cds' => 'tracks'}], - ); + ); 1; diff --git a/t/rest/create.t b/t/rest/create.t index ba850c1..17b3d34 100644 --- a/t/rest/create.t +++ b/t/rest/create.t @@ -105,4 +105,57 @@ my $producer_create_url = "$base/api/rest/producer"; 'json for bulk create returned' ); } +# test create of single related row +{ + my $test_data = $json->encode( + { name => 'Futuristic Artist', cds => { 'title' => 'snarky cd name', year => '3030' } } + ); + my $req = PUT($artist_create_url); + $req->content_type('text/x-json'); + $req->content_length( + do { use bytes; length($test_data) } + ); + $req->content($test_data); + $mech->request($req); + cmp_ok( $mech->status, '==', 200, 'request with single related row okay' ); + my $count = $schema->resultset('Artist') + ->search({ name => 'Futuristic Artist' }) + ->count; + ok( $count, 'record with related object created' ); + $count = $schema->resultset('Artist') + ->search_related('cds', { title => 'snarky cd name' }) + ->count; + ok( $count, "record's related object created" ); +} + +# test create of multiple related rows +{ + my $test_data = $json->encode( + { name => 'Futuristic Artist 2', + cds => [ + { 'title' => 'snarky cd name 2', year => '3030' }, + { 'title' => 'snarky cd name 3', year => '3030' }, + ] + } + ); + + my $req = PUT($artist_create_url); + $req->content_type('text/x-json'); + $req->content_length( + do { use bytes; length($test_data) } + ); + $req->content($test_data); + $mech->request($req); + cmp_ok( $mech->status, '==', 200, 'request with multiple related rows okay' ); + my $count = $schema->resultset('Artist') + ->search({ name => 'Futuristic Artist 2' }) + ->count; + ok( $count, 'record with related object created' ); + $count = $schema->resultset('Artist') + ->search_related('cds', { title => ['snarky cd name 2','snarky cd name 3'] }) + ->count; + ok( $count == 2, "record's related objects created" ) or explain diag $count; + +} + done_testing(); diff --git a/t/rest/update.t b/t/rest/update.t index 7cd6ce5..cfcd2db 100644 --- a/t/rest/update.t +++ b/t/rest/update.t @@ -25,6 +25,9 @@ my $track_url = "$base/api/rest/track/"; my $track_update_url = $track_url . $track->id; my $tracks_update_url = $track_url; +my $artist_url = "$base/api/rest/artist/"; +my $artist_update_url = $artist_url . "1"; + # test invalid track id caught { diag 'DBIx::Class warns about a non-numeric id which is ok because we test for that too'; @@ -101,6 +104,79 @@ my $tracks_update_url = $track_url; is( $track->cd->year, 2009, 'related row updated' ); } +{ + my $test_data = $json->encode( + { name => 'Caterwauler B. McCrae', + cds => [ + { + cdid => 1, + title => 'All the bees are gone', + year => 3030, + }, + { + cdid => 2, + title => 'All the crops are gone', + year => 3031 + } + ] + } + ); + + my $req = POST( $artist_update_url, Content => $test_data ); + $req->content_type('text/x-json'); + $mech->request($req); + + cmp_ok( $mech->status, '==', 200, 'Multi-row update returned 200 OK' ); + + my $artist = $schema->resultset('Artist')->search({ artistid => 1 }); + ok ($artist->next->name eq "Caterwauler B. McCrae", "mutliple related row parent record update"); + + # make sure the old cds don't exist, it's possible we just inserted the new rows instead of updating them + my $old_cds = $artist->search_related('cds', { title => ['Spoonful of bees', 'Forkful of bees'] } )->count; + ok ($old_cds == 0, 'update performed update and not create on related rows'); + + my @cds = $artist->search_related('cds', { year => ['3030', '3031'] }, { order_by => 'year' })->all; + ok (@cds == 2, 'update modified proper number of related rows'); + ok ($cds[0]->title eq 'All the bees are gone', 'update modified column on related row'); + ok ($cds[1]->title eq 'All the crops are gone', 'update modified column on second related row'); +} + +# update related rows using only unique_constraint on CD vs. primary key +# update the year on constraint match +{ + my $test_data = $json->encode( + { name => 'Caterwauler McCrae', + cds => [ + { + artist => 1, + title => 'All the bees are gone', + year => 3032, + }, + { + artist => 1, + title => 'All the crops are gone', + year => 3032 + } + ] + } + ); + + my $req = POST( $artist_update_url, Content => $test_data ); + $req->content_type('text/x-json'); + $mech->request($req); + + cmp_ok( $mech->status, '==', 200, 'Multi-row unique constraint update returned 200 OK' ); + + my $artist = $schema->resultset('Artist')->search({ artistid => 1 }); + ok ($artist->next->name eq "Caterwauler McCrae", "multi-row unique constraint related row parent record updated"); + + my $old_cds = $artist->search_related('cds', { year => ['3030', '3031'] }, { order_by => 'year' })->count; + ok ( $old_cds == 0, 'multi-row update with unique constraint updated year' ); + + my $cds = $artist->search_related('cds', { 'year' => 3032 } )->count; + ok ( $cds == 2, 'multi-row update with unique constraint okay' ); +} + # bulk_update existing objects { diff --git a/t/rpc/create.t b/t/rpc/create.t index f2757a3..a12e766 100644 --- a/t/rpc/create.t +++ b/t/rpc/create.t @@ -78,6 +78,59 @@ my $producer_create_url = "$base/api/rpc/producer/create"; ); } +# test create of single related row +{ + my $test_data = $json->encode( + { name => 'Futuristic Artist', cds => { 'title' => 'snarky cd name', year => '3030' } } + ); + my $req = PUT($artist_create_url); + $req->content_type('text/x-json'); + $req->content_length( + do { use bytes; length($test_data) } + ); + $req->content($test_data); + $mech->request($req); + cmp_ok( $mech->status, '==', 200, 'request with single related row okay' ); + my $count = $schema->resultset('Artist') + ->search({ name => 'Futuristic Artist' }) + ->count; + ok( $count, 'record with related object created' ); + $count = $schema->resultset('Artist') + ->search_related('cds', { title => 'snarky cd name' }) + ->count; + ok( $count, "record's related object created" ); +} + +# test create of multiple related rows +{ + my $test_data = $json->encode( + { name => 'Futuristic Artist 2', + cds => [ + { 'title' => 'snarky cd name 2', year => '3030' }, + { 'title' => 'snarky cd name 3', year => '3030' }, + ] + } + ); + + my $req = PUT($artist_create_url); + $req->content_type('text/x-json'); + $req->content_length( + do { use bytes; length($test_data) } + ); + $req->content($test_data); + $mech->request($req); + cmp_ok( $mech->status, '==', 200, 'request with multiple related rows okay' ); + my $count = $schema->resultset('Artist') + ->search({ name => 'Futuristic Artist 2' }) + ->count; + ok( $count, 'record with related object created' ); + $count = $schema->resultset('Artist') + ->search_related('cds', { title => ['snarky cd name 2','snarky cd name 3'] }) + ->count; + ok( $count == 2, "record's related objects created" ) or explain diag $count; + +} + # test stash config handling { my $req = POST( @@ -125,4 +178,6 @@ my $producer_create_url = "$base/api/rpc/producer/create"; cmp_ok( $mech->status, '==', 400, 'invalid param value produces error' ); } +# test creating record with multiple related-rows + done_testing(); diff --git a/t/rpc/update.t b/t/rpc/update.t index c0fcc77..a877759 100644 --- a/t/rpc/update.t +++ b/t/rpc/update.t @@ -26,6 +26,8 @@ my $any_track_update_url = "$base/api/rpc/any/track/id/" . $track->id . "/update"; my $tracks_update_url = "$base/api/rpc/track/update"; +my $artist_update_url = "$base/api/rpc/artist/id/1/update"; + # test invalid track id caught { diag 'DBIx::Class warns about a non-numeric id which is ok because we test for that too'; @@ -124,6 +126,79 @@ my $tracks_update_url = "$base/api/rpc/track/update"; } { + my $test_data = $json->encode( + { name => 'Caterwauler B. McCrae', + cds => [ + { + cdid => 1, + title => 'All the bees are gone', + year => 3030, + }, + { + cdid => 2, + title => 'All the crops are gone', + year => 3031 + } + ] + } + ); + + my $req = POST( $artist_update_url, Content => $test_data ); + $req->content_type('text/x-json'); + $mech->request($req); + + cmp_ok( $mech->status, '==', 200, 'Multi-row update returned 200 OK' ); + + my $artist = $schema->resultset('Artist')->search({ artistid => 1 }); + ok ($artist->next->name eq "Caterwauler B. McCrae", "mutliple related row parent record update"); + + # make sure the old cds don't exist, it's possible we just inserted the new rows instead of updating them + my $old_cds = $artist->search_related('cds', { title => ['Spoonful of bees', 'Forkful of bees'] } )->count; + ok ($old_cds == 0, 'update performed update and not create on related rows'); + + my @cds = $artist->search_related('cds', { year => ['3030', '3031'] }, { order_by => 'year' })->all; + ok (@cds == 2, 'update modified proper number of related rows'); + ok ($cds[0]->title eq 'All the bees are gone', 'update modified column on related row'); + ok ($cds[1]->title eq 'All the crops are gone', 'update modified column on second related row'); +} + +# update related rows using only unique_constraint on CD vs. primary key +# update the year on constraint match +{ + my $test_data = $json->encode( + { name => 'Caterwauler McCrae', + cds => [ + { + artist => 1, + title => 'All the bees are gone', + year => 3032, + }, + { + artist => 1, + title => 'All the crops are gone', + year => 3032 + } + ] + } + ); + + my $req = POST( $artist_update_url, Content => $test_data ); + $req->content_type('text/x-json'); + $mech->request($req); + + cmp_ok( $mech->status, '==', 200, 'Multi-row unique constraint update returned 200 OK' ); + + my $artist = $schema->resultset('Artist')->search({ artistid => 1 }); + ok ($artist->next->name eq "Caterwauler McCrae", "multi-row unique constraint related row parent record updated"); + + my $old_cds = $artist->search_related('cds', { year => ['3030', '3031'] }, { order_by => 'year' })->count; + ok ( $old_cds == 0, 'multi-row update with unique constraint updated year' ); + + my $cds = $artist->search_related('cds', { 'year' => 3032 } )->count; + ok ( $cds == 2, 'multi-row update with unique constraint okay' ); +} + +{ my $req = POST( $any_track_update_url, { title => 'baa' } ); $mech->request( $req, $content_type ); cmp_ok( $mech->status, '==', 200, 'Stash update okay' ); @@ -141,6 +216,7 @@ my $tracks_update_url = "$base/api/rpc/track/update"; is( $track->get_column('position'), '14', 'Position changed' ); } + # bulk_update existing objects {