Description
Marcel de Rooy
2016-03-30 11:37:37 UTC
Created attachment 49681 [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. Created attachment 49706 [details] [review] Bug 16170 - Corrected to make work more smoothly Created attachment 49707 [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> Tested with my patch too. Created attachment 49708 [details] [review] Bug 16170 - Corrected to make work more smoothly Signed-off-by: Mark Tompsett <mtompset@hotmail.com> NOTE: Should get a sign off by Marcel or someone else before switching to SIGNED OFF. Thanks, Marc. I have renamed the file from 1xxxx_dbrev.perl to 16170_dbrev.sql (!) and did not test again :) Created attachment 49723 [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> Created attachment 49724 [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> 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. (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.. Created attachment 49771 [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> Created attachment 49772 [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> 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. 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. (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? (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). > 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!
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> 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> 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> 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> Pushed to master for Koha 16.04! Patches pushed to 3.22.x, will be in 3.22.7 |