Bug 16170 - Pseudo foreign key in Items
Summary: Pseudo foreign key in Items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 5334
  Show dependency treegraph
 
Reported: 2016-03-30 11:37 UTC by Marcel de Rooy
Modified: 2016-12-05 21:25 UTC (History)
5 users (show)

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


Attachments
Bug 16170: Pseudo foreign key in Items (3.79 KB, patch)
2016-03-30 11:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16170 - Corrected to make work more smoothly (1000 bytes, patch)
2016-03-31 00:13 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16170: Pseudo foreign key in Items (3.80 KB, patch)
2016-03-31 00:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16170 - Corrected to make work more smoothly (1.03 KB, patch)
2016-03-31 00:15 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16170: Pseudo foreign key in Items (3.85 KB, patch)
2016-03-31 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16170 - Corrected to make work more smoothly (853 bytes, patch)
2016-03-31 06:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16170: [QA Follow-up] Adjust Items.t (2.04 KB, patch)
2016-04-01 07:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16170: [QA Follow-up] Fix embeded typos (1.73 KB, patch)
2016-04-01 07:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16170: Pseudo foreign key in Items (3.86 KB, patch)
2016-05-02 12:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16170 - Corrected to make work more smoothly (863 bytes, patch)
2016-05-02 12:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16170: [QA Follow-up] Adjust Items.t (2.05 KB, patch)
2016-05-02 12:52 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 16170: [QA Follow-up] Fix embeded typos (1.74 KB, patch)
2016-05-02 12:52 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2016-03-30 11:37:37 UTC
While many of us would like to get rid of biblioitems, the current scheme includes a biblioitemnumber and a biblionumber in Items. (Which is not so great..)
But also note that biblionumber is NOT defined as a foreign key in Items, although a belongs_to relation has been added to the DBIx scheme! This inconsistency should be resolved.
The "remove biblioitem table" operation is a large one, but in the meantime we better make biblionumber a regular FK not a pseudo one.
Comment 1 Marcel de Rooy 2016-03-30 11:54:44 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2016-03-31 00:13:47 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2016-03-31 00:15:21 UTC Comment hidden (obsolete)
Comment 4 Mark Tompsett 2016-03-31 00:15:57 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2016-03-31 06:37:15 UTC
Thanks, Marc.
I have renamed the file from 1xxxx_dbrev.perl to 16170_dbrev.sql (!) and did not test again :)
Comment 6 Marcel de Rooy 2016-03-31 06:44:37 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2016-03-31 06:44:42 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-03-31 12:54:39 UTC
t/db_dependent/Items.t ................................. 1/9 DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_ut`.`items`, CONSTRAINT `items_ibfk_4` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "INSERT INTO `items` ( `biblioitemnumber`, `itype`) VALUES ( ?, ? )" with ParamValues: 0='88', 1='ITEM_LEVEL'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
Comment 9 Marcel de Rooy 2016-03-31 13:59:04 UTC
(In reply to Jonathan Druart from comment #8)
> t/db_dependent/Items.t ................................. 1/9 DBD::mysql::st
> execute failed: Cannot add or update a child row: a foreign key constraint
> fails (`koha_ut`.`items`, CONSTRAINT `items_ibfk_4` FOREIGN KEY
> (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON
> UPDATE CASCADE) [for Statement "INSERT INTO `items` ( `biblioitemnumber`,
> `itype`) VALUES ( ?, ? )" with ParamValues: 0='88', 1='ITEM_LEVEL'] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.

Good catch, but what now?
This is a good example of our bad database design.
DBIx tries to insert records in items based on biblioitemnumber but ignores the biblionumber one level up.
We can adjust this statement of course, but it is a workaround..
Comment 10 Marcel de Rooy 2016-04-01 07:52:25 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2016-04-01 07:52:30 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2016-04-01 07:54:53 UTC
Remember to adjust DBIx (for testing you could just insert is_foreign_key => 1 for biblionumber in Items.pm

If you would run t/db_dependent/Barcodes_ValueBuilder.t, TestBuilder needs to see that biblionumber is a foreign key.
Comment 13 Kyle M Hall 2016-04-28 10:55:56 UTC
Looks like numerous unit tests will now fail with the new key constraint such as t/db_dependent/Barcodes.t and t/db_dependent/Accounts.t.

I did not wait for the full run of unit tests to complete.
Comment 14 Marcel de Rooy 2016-04-28 12:31:41 UTC
(In reply to Kyle M Hall from comment #13)
> Looks like numerous unit tests will now fail with the new key constraint
> such as t/db_dependent/Barcodes.t and t/db_dependent/Accounts.t.
> 
> I did not wait for the full run of unit tests to complete.

Did you run the dbix schema adjust script?
Comment 15 Marcel de Rooy 2016-04-29 05:50:29 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to Kyle M Hall from comment #13)
> > Looks like numerous unit tests will now fail with the new key constraint
> > such as t/db_dependent/Barcodes.t and t/db_dependent/Accounts.t.
> > 
> > I did not wait for the full run of unit tests to complete.
> 
> Did you run the dbix schema adjust script?

Kyle: You most probably did not. If I do, these test pass with me. Note that Holds.t does not pass (in current master). And a fix is in PQA for ILSDI_Services.t.
See comment 12, giving you an q-a-d alternative too for running update_dbix_class_files.pl (for testing only).
Comment 16 Kyle M Hall 2016-05-02 12:39:32 UTC
> Kyle: You most probably did not. If I do, these test pass with me. Note that
> Holds.t does not pass (in current master). And a fix is in PQA for
> ILSDI_Services.t.
> See comment 12, giving you an q-a-d alternative too for running
> update_dbix_class_files.pl (for testing only).

Indeed, that was my issue!
Comment 17 Kyle M Hall 2016-05-02 12:52:19 UTC
Created attachment 51096 [details] [review]
Bug 16170: Pseudo foreign key in Items

While many of us would like to get rid of biblioitems one day, the current
scheme includes a biblioitemnumber and a biblionumber in Items.
(Which is not so great..)

But also note that biblionumber is NOT defined as a foreign key in Items,
although a belongs_to relation has been added to the DBIx scheme!
This inconsistency should be resolved. The "remove biblioitem table"
operation is a large one, but in the meantime we better make biblionumber
a regular FK not a 'pseudo' one.

Note: If in an (very) exceptional case biblionumbers are found in items,
that do not exist in biblio, this patch prints a warning at upgrade
time and does not add the constraint.

@RM: Please update the DBIx scheme accordingly.

Test plan:
[1] Run the upgrade. Check if the FK constraint has been added.
[2] Remove the FK constraint. Change the biblionumber of one item to an
    unexisting record. Run the upgrade again. Notice the warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested both cases: constraint added as well as warning printed.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2016-05-02 12:52:29 UTC
Created attachment 51097 [details] [review]
Bug 16170 - Corrected to make work more smoothly

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2016-05-02 12:52:31 UTC
Created attachment 51098 [details] [review]
Bug 16170: [QA Follow-up] Adjust Items.t

Items.t inserts an item via biblioitem not correctly linked to biblio.
The new foreign key constraint does not allow that.
Actually, we should be choosing to either remove biblioitems from Koha or
remove biblionumber from items.

Note: This seems to be the only case where an item is added this way.

Test plan:
Run Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2016-05-02 12:52:34 UTC
Created attachment 51099 [details] [review]
Bug 16170: [QA Follow-up] Fix embeded typos

Just noticed this typo three times here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2016-05-03 15:05:29 UTC
Pushed to master for Koha 16.04!
Comment 22 Julian Maurice 2016-05-16 07:58:06 UTC
Patches pushed to 3.22.x, will be in 3.22.7