Bug 28479

Summary: TestBuilder.pm uses incorrect method for checking if objects to be created exists
Product: Koha Reporter: Joonas Kylmälä <joonas.kylmala>
Component: Test SuiteAssignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, kyle, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02,20.11.08
Bug Depends on:    
Bug Blocks: 22690    
Attachments: Bug 28479: Use primary keys to check object existence in TestBuilder
Bug 28479: Use primary keys to check object existence in TestBuilder
Bug 28479: Use primary keys to check object existence in TestBuilder

Description Joonas Kylmälä 2021-05-28 10:17:46 UTC
While working on bug 22690 it was discovered that TestBuilder.pm uses incorrect method to check if objects to be built already exists or not. The warning we got due to this was:

> 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

I'm working on a patch for this, I think it helps to explain more the problem – it was quite tough to debug but I think I got to the bottom of it finally.
Comment 1 Joonas Kylmälä 2021-05-28 10:48:39 UTC
Created attachment 121473 [details] [review]
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
Comment 2 David Nind 2021-05-28 23:34:55 UTC
Created attachment 121501 [details] [review]
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 <david@davidnind.com>
Comment 3 Nick Clemens 2021-06-11 15:47:22 UTC
Created attachment 121858 [details] [review]
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 <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Jonathan Druart 2021-06-21 10:06:26 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 5 Kyle M Hall 2021-07-02 13:04:43 UTC
Pushed to 21.05.x for 21.05.02
Comment 6 Fridolin Somers 2021-07-10 02:42:59 UTC
Pushed to 20.11.x for 20.11.08
Comment 7 Victor Grousset/tuxayo 2021-07-11 02:33:05 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.