Bug 35610 - Missing FK on old_reserves.branchcode
Summary: Missing FK on old_reserves.branchcode
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords: release-notes-needed
Depends on:
Blocks: 5334 35353
  Show dependency treegraph
 
Reported: 2023-12-20 09:27 UTC by Jonathan Druart
Modified: 2024-04-05 08:46 UTC (History)
6 users (show)

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


Attachments
Bug 35610: Add FK on old_reserves.branchcode (2.29 KB, patch)
2023-12-20 09:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35610: Add FK on old_reserves.branchcode (2.59 KB, patch)
2023-12-20 09:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35610: Add FK on old_reserves.branchcode (2.64 KB, patch)
2024-01-26 14:28 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35610: Set branchcode to NULL before adding the FK (1.17 KB, patch)
2024-03-25 15:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 35610: Add FK on old_reserves.branchcode (2.70 KB, patch)
2024-03-27 01:36 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35610: Set branchcode to NULL before adding the FK (1.23 KB, patch)
2024-03-27 01:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 35610: (QA follow-up): tidy up code (1.61 KB, patch)
2024-03-27 01:37 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-12-20 09:27:53 UTC
Why don't we have a foreign key on this column?
Comment 1 Katrin Fischer 2023-12-20 09:34:07 UTC
I believe we don't have strict FK on the statistic tables like statistics and action_logs and as old_reserves is also used mainly for statistics, it makes kind of sense.

If we decide to merge reserve and old_reserves into a holds table, that will probably have to change.
Comment 2 Jonathan Druart 2023-12-20 09:35:30 UTC
Created attachment 160097 [details] [review]
Bug 35610: Add FK on old_reserves.branchcode
Comment 3 Jonathan Druart 2023-12-20 09:36:57 UTC
Created attachment 160098 [details] [review]
Bug 35610: Add FK on old_reserves.branchcode
Comment 4 Jonathan Druart 2023-12-20 10:33:35 UTC
(In reply to Katrin Fischer from comment #1)
> I believe we don't have strict FK on the statistic tables like statistics
> and action_logs and as old_reserves is also used mainly for statistics, it
> makes kind of sense.

FK can be NULL.
Comment 5 Jonathan Druart 2023-12-20 10:34:26 UTC
Do you mean we want to keep the branchcode values for deleted libraries?
Comment 6 Katrin Fischer 2023-12-20 13:18:26 UTC
(In reply to Jonathan Druart from comment #5)
> Do you mean we want to keep the branchcode values for deleted libraries?

It might be something people query, but I am not sure how often branches are closed etc. I was thinking maybe a better moment to fix might be when merging, because we could advertise that change a bit more. But if there is a need now, maybe it's ok.
Comment 7 Teertha Chatterjee 2023-12-25 08:08:08 UTC
For academic libraries in India we have seen several instances of library branch re-organisation due to accreditation compliance purpose. In such cases, we are often faced with a the need for a mechanism to track such changes.
Comment 8 Kyle M Hall 2024-01-26 14:24:36 UTC
(In reply to Teertha Chatterjee from comment #7)
> For academic libraries in India we have seen several instances of library
> branch re-organisation due to accreditation compliance purpose. In such
> cases, we are often faced with a the need for a mechanism to track such
> changes.

Action logs and statistics exist for that very reason. All other tables need to have proper FK constraints.
Comment 9 Kyle M Hall 2024-01-26 14:28:07 UTC
Created attachment 161540 [details] [review]
Bug 35610: Add FK on old_reserves.branchcode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Victor Grousset/tuxayo 2024-02-16 03:08:06 UTC
As expected, doing an upgrade when there is
1. a hold on a library with no data linked (for easy deletion) in sample data: Liberty
2. cancel the hold
3. delete the library
4. apply the patch
5. run updatedatabase

fails with:

DEV atomic update /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35610.pl  [01:44:28]: Bug 35610 - Add FK on old_reserves.branchcode
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`#sql-alter-1-2f`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/C4/Installer.pm line 741

That's a rough to have such an upgrade requiring manual DB fixes. And it's not a weird workflow that can be considered messing with the DB.

There doesn't seem to be a mention about how to handle that in bug 25260.

How about before adding the FK having an UPDATE that NULLs the old_reserves.branchcode that don't match an existing branch?
Or another approach?

---

(In reply to Katrin Fischer from comment #6)
> I was thinking maybe a better moment to fix might be when
> merging, because we could advertise that change a bit more.

What would be needed to advertise in advance? Like if they are using old_reserves for stats they should stop, use action logs/statistics and backup old_reserve before the future upgrade for past data?
Comment 11 Jonathan Druart 2024-03-25 15:51:21 UTC
Created attachment 163827 [details] [review]
Bug 35610: Set branchcode to NULL before adding the FK
Comment 12 Jonathan Druart 2024-03-25 15:51:53 UTC
(In reply to Victor Grousset/tuxayo from comment #10)
> As expected, doing an upgrade when there is
> 1. a hold on a library with no data linked (for easy deletion) in sample
> data: Liberty
> 2. cancel the hold
> 3. delete the library
> 4. apply the patch
> 5. run updatedatabase
> 
> fails with:
> 
> DEV atomic update
> /kohadevbox/koha/installer/data/mysql/atomicupdate/bug_35610.pl  [01:44:28]:
> Bug 35610 - Add FK on old_reserves.branchcode
> ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot add or
> update a child row: a foreign key constraint fails
> (`koha_kohadev`.`#sql-alter-1-2f`, CONSTRAINT `old_reserves_ibfk_branchcode`
> FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE
> SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/C4/Installer.pm line 741

Fixed.
Comment 13 Victor Grousset/tuxayo 2024-03-27 01:36:58 UTC
Created attachment 163966 [details] [review]
Bug 35610: Add FK on old_reserves.branchcode

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Victor Grousset/tuxayo 2024-03-27 01:37:01 UTC
Created attachment 163967 [details] [review]
Bug 35610: Set branchcode to NULL before adding the FK

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2024-03-27 01:37:03 UTC
Created attachment 163968 [details] [review]
Bug 35610: (QA follow-up): tidy up code
Comment 16 Victor Grousset/tuxayo 2024-03-27 01:39:44 UTC
Works, makes sense, QA script happy, code looks good, passing QA :)

Test with comment 10 and checked that before the patch, orphan branchcodes where left and that after the patch, existing ones where handled and it was not possible to create new ones.
Comment 17 Katrin Fischer 2024-04-04 12:52:54 UTC
I agree with this change, but please add a note/warning in the release notes so people are aware on update that this might "lose" them data by setting the deleted branchcodes to NULL.
Comment 18 Katrin Fischer 2024-04-04 13:25:20 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 19 Katrin Fischer 2024-04-04 20:19:36 UTC
Sadly, this broke several tests...

prove t/db_dependent/Koha/Old/Hold.t 

prove t/db_dependent/Koha/Old/Hold.t 
t/db_dependent/Koha/Old/Hold.t ..     # Looks like you planned 8 tests but ran 1.
t/db_dependent/Koha/Old/Hold.t .. 1/2 
#   Failed test 'anonymize() tests'
#   at t/db_dependent/Koha/Old/Hold.t line 82.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 255 just after 1.
t/db_dependent/Koha/Old/Hold.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests
Comment 20 Katrin Fischer 2024-04-04 20:21:08 UTC
prove t/db_dependent/Koha/Patrons.t 
t/db_dependent/Koha/Patrons.t .. 30/43     # Looks like you planned 113 tests but ran 69.
t/db_dependent/Koha/Patrons.t .. 31/43 
#   Failed test 'Test Koha::Patrons::merge'
#   at t/db_dependent/Koha/Patrons.t line 1844.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 11 just after 31.
t/db_dependent/Koha/Patrons.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 13/43 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Patrons.t (Wstat: 2816 Tests: 31 Failed: 1)
  Failed test:  31
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 43 tests but ran 31.
Files=1, Tests=31, 12 wallclock secs ( 0.04 usr  0.01 sys +  8.47 cusr  1.94 csys = 10.46 CPU)
Result: FAIL
Comment 21 Katrin Fischer 2024-04-04 20:21:47 UTC
prove t/db_dependent/UsageStats.t 
t/db_dependent/UsageStats.t .. 1/4     # No tests run!

#   Failed test 'No tests run for subtest "BuildReport() tests"'
#   at t/db_dependent/UsageStats.t line 138.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 255 just after 4.
t/db_dependent/UsageStats.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/4 subtests 

Test Summary Report
-------------------
t/db_dependent/UsageStats.t (Wstat: 65280 Tests: 4 Failed: 1)
  Failed test:  4
  Non-zero exit status: 255
Files=1, Tests=4,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.87 cusr  0.11 csys =  1.00 CPU)
Result: FAIL
Comment 22 Katrin Fischer 2024-04-04 20:22:16 UTC
prove t/db_dependent/Koha/Old/Holds.t 
t/db_dependent/Koha/Old/Holds.t ..     # Looks like you planned 10 tests but ran 4.
t/db_dependent/Koha/Old/Holds.t .. 1/2 
#   Failed test 'anonymize() tests'
#   at t/db_dependent/Koha/Old/Holds.t line 114.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 255 just after 1.
t/db_dependent/Koha/Old/Holds.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/2 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Old/Holds.t (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 2 tests but ran 1.
Files=1, Tests=1,  1 wallclock secs ( 0.01 usr  0.00 sys +  0.83 cusr  0.08 csys =  0.92 CPU)
Result: FAIL
Comment 23 Katrin Fischer 2024-04-04 20:24:33 UTC
HELP...

prove t/db_dependent/TestBuilder.t 
t/db_dependent/TestBuilder.t .. 1/16 
    #   Failed test 'TestBuilder should be able to create an object for every source'
    #   at t/db_dependent/TestBuilder.t line 85.
    #          got: '1'
    #     expected: '0'
    # The following sources have not been generated correctly: OldReserve
    # Looks like you failed 1 test of 1.
t/db_dependent/TestBuilder.t .. 3/16 
#   Failed test 'Build all sources'
#   at t/db_dependent/TestBuilder.t line 93.
t/db_dependent/TestBuilder.t .. 7/16     # Looks like you planned 5 tests but ran 4.
t/db_dependent/TestBuilder.t .. 12/16 
#   Failed test 'build_object() tests'
#   at t/db_dependent/TestBuilder.t line 473.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`old_reserves`, CONSTRAINT `old_reserves_ibfk_branchcode` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/t/lib/TestBuilder.pm line 384
# Looks like your test exited with 11 just after 12.
t/db_dependent/TestBuilder.t .. Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 6/16 subtests 

Test Summary Report
-------------------
t/db_dependent/TestBuilder.t (Wstat: 2816 Tests: 12 Failed: 2)
  Failed tests:  3, 12
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 16 tests but ran 12.
Files=1, Tests=12, 16 wallclock secs ( 0.01 usr  0.01 sys + 11.46 cusr  3.43 csys = 14.91 CPU)
Result: FAIL
Comment 24 Jonathan Druart 2024-04-05 06:47:31 UTC
You forget the DBIC changes. It fixed all the failures. I pushed it to master.

"Bug 35610: DBIC schema changes"
Comment 25 Katrin Fischer 2024-04-05 08:46:57 UTC
(In reply to Jonathan Druart from comment #24)
> You forget the DBIC changes. It fixed all the failures. I pushed it to
> master.
> 
> "Bug 35610: DBIC schema changes"

I like easy fixes, thanks for the help :)