Bug 28479 - TestBuilder.pm uses incorrect method for checking if objects to be created exists
Summary: TestBuilder.pm uses incorrect method for checking if objects to be created ex...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Joonas Kylmälä
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22690
  Show dependency treegraph
 
Reported: 2021-05-28 10:17 UTC by Joonas Kylmälä
Modified: 2021-07-11 02:33 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.02,20.11.08


Attachments
Bug 28479: Use primary keys to check object existence in TestBuilder (3.43 KB, patch)
2021-05-28 10:48 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 28479: Use primary keys to check object existence in TestBuilder (3.47 KB, patch)
2021-05-28 23:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 28479: Use primary keys to check object existence in TestBuilder (3.53 KB, patch)
2021-06-11 15:47 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.