From 188479881fdafb6ef77d9e7278738dba372f15c2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Joonas=20Kylm=C3=A4l=C3=A4?= Date: Fri, 28 May 2021 13:33:24 +0300 Subject: [PATCH] Bug 28479: Use primary keys to check object existence in TestBuilder The TestBuilder::build_object function used any foreign keys to check whether an object already exists or not. This brought incorrectly results of unrelated objects because using any other keys other than primary keys don't guarantee our results to point to one single object. For example, as is put here in the unit test, if you created two items with the same biblionumber and then tried to create a hold using build_object() we were using the biblionumber to check whether an item was linked to the hold already. Thus, we were checking whether a random item was already linked to the hold instead of the one we wanted either by passing it explicitly to build_object() or the one build_object() created implicitly. This also resulted in following warnings when there were more than one match: DBIx::Class::Storage::DBI::select_single(): Query returned more than one row. SQL that returns multiple rows is DEPRECATED for ->find and ->single at /kohadevbox/koha/t/lib/TestBuilder.pm line 235 To test: $ prove t/db_dependent Signed-off-by: David Nind Signed-off-by: Nick Clemens Signed-off-by: Jonathan Druart Signed-off-by: Kyle M Hall --- t/db_dependent/TestBuilder.t | 23 ++++++++++++++++++++++- t/lib/TestBuilder.pm | 9 ++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/t/db_dependent/TestBuilder.t b/t/db_dependent/TestBuilder.t index 7d07469196..f97626c06b 100755 --- a/t/db_dependent/TestBuilder.t +++ b/t/db_dependent/TestBuilder.t @@ -21,7 +21,7 @@ use Modern::Perl; use utf8; -use Test::More tests => 14; +use Test::More tests => 15; use Test::Warn; use File::Basename qw(dirname); @@ -477,3 +477,24 @@ subtest 'build_sample_biblio() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'Existence of object is only checked using primary keys' => sub { + + plan tests => 1; + + $schema->storage->txn_begin; + + my $biblio = $builder->build_sample_biblio(); + my $item1 = $builder->build_sample_item({ biblionumber => $biblio->biblionumber }); + my $item2 = $builder->build_sample_item({ biblionumber => $biblio->biblionumber }); + warnings_are { + $builder->build_object({ + class => 'Koha::Holds', + value => { + biblionumber => $biblio->biblionumber + } + }); + } [], "No warning about query returning more than one row"; + + $schema->storage->txn_rollback; +}; diff --git a/t/lib/TestBuilder.pm b/t/lib/TestBuilder.pm index a39be70866..d8897b8145 100644 --- a/t/lib/TestBuilder.pm +++ b/t/lib/TestBuilder.pm @@ -231,8 +231,15 @@ sub _create_links { if( $cnt_scalar == @$keys ) { # if one or more fk cols are null, the FK constraint will not be forced return {} if $cnt_null > 0; + # does the record exist already? - return {} if $self->schema->resultset($linked_tbl)->find( $fk_value ); + my @pks = $self->schema->source( $linked_tbl )->primary_columns; + my %fk_pk_value; + for (@pks) { + $fk_pk_value{$_} = $fk_value->{$_} if defined $fk_value->{$_}; + } + return {} if !(keys %fk_pk_value); + return {} if $self->schema->resultset($linked_tbl)->find( \%fk_pk_value ); } # create record with a recursive build call my $row = $self->build({ source => $linked_tbl, value => $fk_value }); -- 2.39.5