Bug 31133 - TestBuilder fragile on virtual fks
Summary: TestBuilder fragile on virtual fks
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 24857 30275
Blocks:
  Show dependency treegraph
 
Reported: 2022-07-11 23:29 UTC by Tomás Cohen Arazi
Modified: 2023-06-08 22:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 31133: Regression tests (1) (1.92 KB, patch)
2022-07-12 03:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 31133: Add a way to handle multiple relationships on same column (5.29 KB, patch)
2022-07-12 03:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK (1.53 KB, patch)
2022-07-12 07:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK (1.50 KB, patch)
2022-07-12 07:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 31133: Don't generate values for more than one FK (1.55 KB, patch)
2022-07-12 14:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2022-07-11 23:29:33 UTC
As exposed with the introduction of Koha::Checkouts::Renewal(s), which has two relationships (checkout and old_checkout) defined at the DBIC level (and not at DB level) that point the same column name, to the same fk name, TestBuilder fails in some situations.

Looking at the code I found there's some code for detecting duplicate fk introduced by bug 16155, but it doesn't seem to be effective: it fails randomly depending on the key that is picked.

I will submit my attempt to make things work, but more eyes are needed.
Comment 1 Tomás Cohen Arazi 2022-07-12 03:05:54 UTC
Created attachment 137599 [details] [review]
Bug 31133: Regression tests (1)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2022-07-12 03:06:01 UTC
Created attachment 137600 [details] [review]
Bug 31133: Add a way to handle multiple relationships on same column

This patch introduces a syntax for TestBuilder so we can specify which
is the preferred relationship in situations like
Koha::Checkouts::Renewals that have more than one virtual FK [1] for the
same column. In this case, there are the following relationships:

* checkouts
* old_checkouts

both of which link the *checkout_id* column to the *issue_id* column of
the **issues** and **old_issues** tables respectively.

It does so by defining a mapping between the (concatenated, maybe
multiple) related source column and foreign counterpart. The mapping
structure is explained in the POD.

To test:
1. Apply the regression tests
2. Run the tests a couple times:
   $ kshell
  k$ prove t/db_dependent/TestBuilder_multi_vfk.t
=> FAIL: Tests fail often, randomly!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! No random failures!
5. Bonus, run:
   $ kshell
  k$ prove t/db_dependent/TestBuilder.t
=> FAIL: It fails consistently about Koha::Biblio::ItemGroup

[1] i.e. defined at DBIC level but not present on the DB structure.

DISCLAIMER: This is not a complete solution as it still requires a way
to override the hardcoded default. This would require a special syntax
for build() and build_object() which I prefer to delay until we really
need it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2022-07-12 03:07:13 UTC
Ok, this should cover one part of the problem. There's still Koha::Biblio::ItemGroup which is failing, ran out of fuel today/yesterday, late already :-D
Comment 4 Jonathan Druart 2022-07-12 07:00:33 UTC
Did you notice that "Bug 30275: (QA follow-up) Trivial fix to schema file" fixes this problem?
Comment 5 Jonathan Druart 2022-07-12 07:03:37 UTC
(In reply to Jonathan Druart from comment #4)
> Did you notice that "Bug 30275: (QA follow-up) Trivial fix to schema file"
> fixes this problem?

Hum no, it did not!
Comment 6 Jonathan Druart 2022-07-12 07:11:53 UTC
Created attachment 137603 [details] [review]
[ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK
Comment 7 Jonathan Druart 2022-07-12 07:13:07 UTC
What about this alternative, Tomas? At least we won't have more code to add for other specific cases.
Comment 8 Martin Renvoize 2022-07-12 07:15:35 UTC
That's pretty elegant and simple.. I like
Comment 9 Jonathan Druart 2022-07-12 07:26:22 UTC
Created attachment 137604 [details] [review]
[ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK
Comment 10 Jonathan Druart 2022-07-12 07:27:06 UTC
It was not working, randomly failing!
The second patch should be correct.
Comment 11 Jonathan Druart 2022-07-12 07:28:05 UTC
Note that I don't get any other failures, t/db_dependent/TestBuilder.t is passing.
Comment 12 Jonathan Druart 2022-07-12 07:30:43 UTC
Note that I don't get any other failures, t/db_dependent/TestBuilder.t is passing.(In reply to Jonathan Druart from comment #11)
> Note that I don't get any other failures, t/db_dependent/TestBuilder.t is
> passing.

Wrong! Hard morning...

Can't use string ("Koha::Biblio::ItemGroup") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 830.
Comment 13 Jonathan Druart 2022-07-12 07:40:07 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> Ok, this should cover one part of the problem. There's still
> Koha::Biblio::ItemGroup which is failing, ran out of fuel today/yesterday,
> late already :-D

I've attached a patch on bug 24857 for that: "Bug 24857: Remove object_class from singular classes".
Comment 14 Tomás Cohen Arazi 2022-07-12 10:14:32 UTC
My first implementation was similar to Jonathan's. The 'problem' was that it randomly generated old_issues and it felt like a very rare use case. It felt like it was better to just have a default resolution path. I'm fine with either.
Comment 15 Tomás Cohen Arazi 2022-07-12 14:08:13 UTC
Created attachment 137634 [details] [review]
Bug 31133: Don't generate values for more than one FK

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2022-07-12 14:09:34 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!