From 855d70b3e92baea86821e60ae220f10d589ec2bb Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 11 Dec 2019 08:29:27 +0100 Subject: [PATCH] Bug 21901: Add FK on subscription and serial tables In order to improve performance in the serial modules and add DB constraints, this patch is going to add missing foreign key on the following columns: * serial.biblionumber * serial.subscription * subscriptionhistory.biblionumber * subscriptionhistory.subscriptionid * subscription.biblionumber Once done, some code can be removed from the Del* subroutines, as the ON CASCASE clause will make the RDBMS handles the deletions. Test plan: 0/ It would be useful to test the update DB entry on a big and old production DB, to make sure the constraints will be added correctly. We could remove the entries before creating the constraints, but it can be unecessary 1/ Make sure you can recreate a fresh install with the kohastructure.sql from this patch 2/ Make sure you can upgrade from a master install 3/ Create a subscription, serial, etc. and delete the biblio => The subscription/serials should have been deleted from the DB 4/ Create a subscription, serial, etc. and delete the subscription => The serials should have been deleted from the DB Signed-off-by: Martin Renvoize --- C4/Biblio.pm | 7 --- C4/Serials.pm | 2 - .../data/mysql/atomicupdate/bug_21901.perl | 62 ++++++++++++++++++ installer/data/mysql/kohastructure.sql | 63 ++++++++++--------- t/db_dependent/Biblio.t | 34 +++++++++- 5 files changed, 128 insertions(+), 40 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug_21901.perl diff --git a/C4/Biblio.pm b/C4/Biblio.pm index f745b93d32..c64d5a98c0 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -386,13 +386,6 @@ sub DelBiblio { return $error if $error; - # We delete attached subscriptions - require C4::Serials; - my $subscriptions = C4::Serials::GetFullSubscriptionsFromBiblionumber($biblionumber); - foreach my $subscription (@$subscriptions) { - C4::Serials::DelSubscription( $subscription->{subscriptionid} ); - } - # We delete any existing holds my $holds = $biblio->holds; while ( my $hold = $holds->next ) { diff --git a/C4/Serials.pm b/C4/Serials.pm index f56309e98c..66e6839328 100644 --- a/C4/Serials.pm +++ b/C4/Serials.pm @@ -1731,8 +1731,6 @@ sub DelSubscription { my ($subscriptionid) = @_; my $dbh = C4::Context->dbh; $dbh->do("DELETE FROM subscription WHERE subscriptionid=?", undef, $subscriptionid); - $dbh->do("DELETE FROM subscriptionhistory WHERE subscriptionid=?", undef, $subscriptionid); - $dbh->do("DELETE FROM serial WHERE subscriptionid=?", undef, $subscriptionid); Koha::AdditionalFieldValues->search({ 'field.tablename' => 'subscription', diff --git a/installer/data/mysql/atomicupdate/bug_21901.perl b/installer/data/mysql/atomicupdate/bug_21901.perl new file mode 100644 index 0000000000..58657f91b7 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_21901.perl @@ -0,0 +1,62 @@ +$DBversion = 'XXX'; # will be replaced by the RM +if( CheckVersion( $DBversion ) ) { + + $dbh->do(q| + ALTER TABLE serial + MODIFY COLUMN biblionumber INT(11) NOT NULL + |); + + unless ( foreign_key_exists( 'serial', 'serial_ibfk_1' ) ) { + $dbh->do(q| + ALTER TABLE serial + ADD CONSTRAINT serial_ibfk_1 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE + |); + } + + $dbh->do(q| + ALTER TABLE serial + MODIFY COLUMN subscriptionid INT(11) NOT NULL + |); + + unless ( foreign_key_exists( 'serial', 'serial_ibfk_2' ) ) { + $dbh->do(q| + ALTER TABLE serial + ADD CONSTRAINT serial_ibfk_2 FOREIGN KEY (subscriptionid) REFERENCES subscription (subscriptionid) ON DELETE CASCADE ON UPDATE CASCADE + |); + } + + $dbh->do(q| + ALTER TABLE subscriptionhistory + MODIFY COLUMN biblionumber int(11), + MODIFY COLUMN subscriptionid int(11) + |); + + unless ( foreign_key_exists( 'subscriptionhistory', 'subscription_history_ibfk_1' ) ) { + $dbh->do(q| + ALTER TABLE subscriptionhistory + ADD CONSTRAINT subscription_history_ibfk_1 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE + |); + } + + unless ( foreign_key_exists( 'subscriptionhistory', 'subscription_history_ibfk_2' ) ) { + $dbh->do(q| + ALTER TABLE subscriptionhistory + ADD CONSTRAINT subscription_history_ibfk_2 FOREIGN KEY (subscriptionid) REFERENCES subscription (subscriptionid) ON DELETE CASCADE ON UPDATE CASCADE + |); + } + + $dbh->do(q| + ALTER TABLE subscription + MODIFY COLUMN biblionumber int(11) + |); + + unless ( foreign_key_exists( 'subscription', 'subscription_ibfk_3' ) ) { + $dbh->do(q| + ALTER TABLE subscription + ADD CONSTRAINT subscription_ibfk_3 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE + |); + } + + SetVersion( $DBversion ); + print "Upgrade to $DBversion done (Bug 21901 - Add foreign key constraints on serial)\n"; +} diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index 0f092725b5..0617637dec 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1401,30 +1401,6 @@ CREATE TABLE `search_marc_to_field` ( FOREIGN KEY(search_field_id) REFERENCES search_field(id) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; --- --- Table structure for table `serial` --- - -DROP TABLE IF EXISTS `serial`; -CREATE TABLE `serial` ( -- issues related to subscriptions - `serialid` int(11) NOT NULL auto_increment, -- unique key for the issue - `biblionumber` varchar(100) NOT NULL default '', -- foreign key for the biblio.biblionumber that this issue is attached to - `subscriptionid` varchar(100) NOT NULL default '', -- foreign key to the subscription.subscriptionid that this issue is part of - `serialseq` varchar(100) NOT NULL default '', -- issue information (volume, number, etc) - `serialseq_x` varchar( 100 ) NULL DEFAULT NULL, -- first part of issue information - `serialseq_y` varchar( 100 ) NULL DEFAULT NULL, -- second part of issue information - `serialseq_z` varchar( 100 ) NULL DEFAULT NULL, -- third part of issue information - `status` tinyint(4) NOT NULL default 0, -- status code for this issue (see manual for full descriptions) - `planneddate` date default NULL, -- date expected - `notes` MEDIUMTEXT, -- notes - `publisheddate` date default NULL, -- date published - publisheddatetext varchar(100) default NULL, -- date published (descriptive) - `claimdate` date default NULL, -- date claimed - claims_count int(11) default 0, -- number of claims made related to this issue - `routingnotes` MEDIUMTEXT, -- notes from the routing list - PRIMARY KEY (`serialid`) -) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; - -- -- Table structure for table `sessions` -- @@ -1937,7 +1913,7 @@ CREATE TABLE subscription_numberpatterns ( DROP TABLE IF EXISTS `subscription`; CREATE TABLE `subscription` ( -- information related to the subscription - `biblionumber` int(11) NOT NULL default 0, -- foreign key for biblio.biblionumber that this subscription is attached to + `biblionumber` int(11) NOT NULL, -- foreign key for biblio.biblionumber that this subscription is attached to `subscriptionid` int(11) NOT NULL auto_increment, -- unique key for this subscription `librarian` varchar(100) default '', -- the librarian's username from borrowers.userid `startdate` date default NULL, -- start date for this subscription @@ -1981,9 +1957,35 @@ CREATE TABLE `subscription` ( -- information related to the subscription `previousitemtype` VARCHAR( 10 ) NULL, `mana_id` int(11) NULL DEFAULT NULL, PRIMARY KEY (`subscriptionid`), - KEY `by_biblionumber` (`biblionumber`), CONSTRAINT subscription_ibfk_1 FOREIGN KEY (periodicity) REFERENCES subscription_frequencies (id) ON DELETE SET NULL ON UPDATE CASCADE, - CONSTRAINT subscription_ibfk_2 FOREIGN KEY (numberpattern) REFERENCES subscription_numberpatterns (id) ON DELETE SET NULL ON UPDATE CASCADE + CONSTRAINT subscription_ibfk_2 FOREIGN KEY (numberpattern) REFERENCES subscription_numberpatterns (id) ON DELETE SET NULL ON UPDATE CASCADE, + CONSTRAINT subscription_ibfk_3 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; + +-- +-- Table structure for table `serial` +-- + +DROP TABLE IF EXISTS `serial`; +CREATE TABLE `serial` ( -- issues related to subscriptions + `serialid` int(11) NOT NULL auto_increment, -- unique key for the issue + `biblionumber` int(11) NOT NULL, -- foreign key for the biblio.biblionumber that this issue is attached to + `subscriptionid` int(11) NOT NULL, -- foreign key to the subscription.subscriptionid that this issue is part of + `serialseq` varchar(100) NOT NULL default '', -- issue information (volume, number, etc) + `serialseq_x` varchar( 100 ) NULL DEFAULT NULL, -- first part of issue information + `serialseq_y` varchar( 100 ) NULL DEFAULT NULL, -- second part of issue information + `serialseq_z` varchar( 100 ) NULL DEFAULT NULL, -- third part of issue information + `status` tinyint(4) NOT NULL default 0, -- status code for this issue (see manual for full descriptions) + `planneddate` date default NULL, -- date expected + `notes` MEDIUMTEXT, -- notes + `publisheddate` date default NULL, -- date published + publisheddatetext varchar(100) default NULL, -- date published (descriptive) + `claimdate` date default NULL, -- date claimed + claims_count int(11) default 0, -- number of claims made related to this issue + `routingnotes` MEDIUMTEXT, -- notes from the routing list + PRIMARY KEY (`serialid`), + CONSTRAINT serial_ibfk_1 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT serial_ibfk_2 FOREIGN KEY (subscriptionid) REFERENCES subscription (subscriptionid) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; -- @@ -1992,8 +1994,8 @@ CREATE TABLE `subscription` ( -- information related to the subscription DROP TABLE IF EXISTS `subscriptionhistory`; CREATE TABLE `subscriptionhistory` ( - `biblionumber` int(11) NOT NULL default 0, - `subscriptionid` int(11) NOT NULL default 0, + `biblionumber` int(11) NOT NULL, + `subscriptionid` int(11) NOT NULL, `histstartdate` date default NULL, `histenddate` date default NULL, `missinglist` LONGTEXT NOT NULL, @@ -2001,7 +2003,8 @@ CREATE TABLE `subscriptionhistory` ( `opacnote` LONGTEXT NULL, `librariannote` LONGTEXT NULL, PRIMARY KEY (`subscriptionid`), - KEY `biblionumber` (`biblionumber`) + CONSTRAINT subscription_history_ibfk_1 FOREIGN KEY (biblionumber) REFERENCES biblio (biblionumber) ON DELETE CASCADE ON UPDATE CASCADE, + CONSTRAINT subscription_history_ibfk_2 FOREIGN KEY (subscriptionid) REFERENCES subscription (subscriptionid) ON DELETE CASCADE ON UPDATE CASCADE ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; -- diff --git a/t/db_dependent/Biblio.t b/t/db_dependent/Biblio.t index 4b8f29723b..1d12d29b37 100755 --- a/t/db_dependent/Biblio.t +++ b/t/db_dependent/Biblio.t @@ -38,6 +38,8 @@ $schema->storage->txn_begin; my $dbh = C4::Context->dbh; Koha::Caches->get_instance->clear_from_cache( "MarcSubfieldStructure-" ); +my $builder = t::lib::TestBuilder->new; + subtest 'GetMarcSubfieldStructureFromKohaField' => sub { plan tests => 25; @@ -588,7 +590,7 @@ subtest 'deletedbiblio_metadata' => sub { }; subtest 'DelBiblio' => sub { - plan tests => 2; + plan tests => 5; my ($biblionumber, $biblioitemnumber) = C4::Biblio::AddBiblio(MARC::Record->new, ''); my $deleted = C4::Biblio::DelBiblio( $biblionumber ); @@ -596,6 +598,36 @@ subtest 'DelBiblio' => sub { $deleted = C4::Biblio::DelBiblio( $biblionumber ); is( $deleted, undef, 'DelBiblo should return undef is the record did not exist'); + + my $biblio = $builder->build_sample_biblio; + my $subscription = $builder->build_object( + { + class => 'Koha::Subscriptions', + value => { biblionumber => $biblio->biblionumber } + } + ); + my $serial = $builder->build_object( + { + class => 'Koha::Serials', + value => { + biblionumber => $biblio->biblionumber, + subscriptionid => $subscription->subscriptionid + } + } + ); + my $subscription_history = $builder->build_object( + { + class => 'Koha::Subscription::Histories', + value => { + biblionumber => $biblio->biblionumber, + subscriptionid => $subscription->subscriptionid + } + } + ); + C4::Biblio::DelBiblio($biblio->biblionumber); # Or $biblio->delete + is( $subscription->get_from_storage, undef, 'subscription should be deleted on biblio deletion' ); + is( $serial->get_from_storage, undef, 'serial should be deleted on biblio deletion' ); + is( $subscription_history->get_from_storage, undef, 'subscription history should be deleted on biblio deletion' ); }; subtest 'MarcFieldForCreatorAndModifier' => sub { -- 2.20.1