`host` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'target''s host name', `port` int(11) DEFAULT NULL COMMENT 'port number used to connect to target', `db` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'target''s database name', `syntax` varchar(80) COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'marc format provided by this target', `encoding` mediumtext COLLATE utf8mb4_unicode_ci DEFAULT NULL COMMENT 'characters encoding provided by this target', Seems to me that those fields should be NOT NULL.
Noting that my production table looks like: `host` varchar(255) COLLATE utf8mb4_unicode_ci NOT NULL, `port` int(11) DEFAULT NULL, `db` varchar(255) COLLATE utf8mb4_unicode_ci DEFAULT NULL, `syntax` varchar(80) COLLATE utf8mb4_unicode_ci NOT NULL, `encoding` mediumtext COLLATE utf8mb4_unicode_ci NOT NULL, Cannot find out why my host, syntax and encoding are NOT NULL. But it looks like that it is more correct. Did not find wrong dbrevs.
Linking it to the sync omnibus since it is at least very related in nature.
Here is the first kohastructure.sql from our git repo for z3950servers: DROP TABLE IF EXISTS `z3950servers`; CREATE TABLE `z3950servers` ( `host` varchar(255) default NULL, `port` int(11) default NULL, `db` varchar(255) default NULL, `userid` varchar(255) default NULL, `password` varchar(255) default NULL, `name` mediumtext, `id` int(11) NOT NULL auto_increment, `checked` smallint(6) default NULL, `rank` int(11) default NULL, `syntax` varchar(80) default NULL, `icon` text, `position` enum('primary','secondary','') NOT NULL default 'primary', `type` enum('zed','opensearch') NOT NULL default 'zed', `description` text NOT NULL, PRIMARY KEY (`id`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8; Encoding was added later: $DBversion = "3.00.00.049"; $dbh->do("ALTER TABLE `z3950servers` ADD `encoding` text default NULL AFTER type "); Conclusion: I must have changed these fields myself somehow. Although it seems the right thing, it is NOT a sync problem. Removing the omnibus reference.
Created attachment 139713 [details] [review] Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL This is not a sync issue between structure and db revs. It just seems a logical move. Note that the interface for long does not allow these fields to be empty. Test plan: Run dbrev. Add a new Z3950/SRU server. Verify if it works as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 139714 [details] [review] Bug 30571: (DBIx schema) Update for Z3950server.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 139715 [details] [review] Bug 30571: Z3950Servers.t - Create subtest for existing tests No real changes, just moving code and indentation. Ground work for following patch. Test plan: To let the test pass, disable strict_mode in koha-conf. Run perl t/db_dependent/Koha/Z3950Servers.t. Should pass now. However, in strict mode or with prove, it should fail. Run prove t/db_dependent/Koha/Z3950Servers.t. Failed? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 139716 [details] [review] Bug 30571: Z3950Servers.t - Add another subtest Subtest for testing nullability of host, syntax and encoding. Test plan: Run prove t/db_dependent/Koha/Z3950Servers.t. (Note: you need strict mode to pass this test.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #3) > Removing the omnibus reference. And moved to enhancement
Created attachment 140074 [details] [review] Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL This is not a sync issue between structure and db revs. It just seems a logical move. Note that the interface for long does not allow these fields to be empty. Test plan: Run dbrev. Add a new Z3950/SRU server. Verify if it works as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 140075 [details] [review] Bug 30571: (DBIx schema) Update for Z3950server.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 140076 [details] [review] Bug 30571: Z3950Servers.t - Create subtest for existing tests No real changes, just moving code and indentation. Ground work for following patch. Test plan: To let the test pass, disable strict_mode in koha-conf. Run perl t/db_dependent/Koha/Z3950Servers.t. Should pass now. However, in strict mode or with prove, it should fail. Run prove t/db_dependent/Koha/Z3950Servers.t. Failed? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 140077 [details] [review] Bug 30571: Z3950Servers.t - Add another subtest Subtest for testing nullability of host, syntax and encoding. Test plan: Run prove t/db_dependent/Koha/Z3950Servers.t. (Note: you need strict mode to pass this test.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Should we do any checks that there are not NULLs in the db before the update? The interface shouldn't create them, but it is possible
(In reply to Nick Clemens from comment #13) > Should we do any checks that there are not NULLs in the db before the > update? The interface shouldn't create them, but it is possible Thx Nick. Since it is highly unlikely to have such records, I decided to trust the functionality around db revs here. It will pause the upgrade to allow the admin to check what happened ;) Might add a bit of text in release notes for that.
Created attachment 141238 [details] [review] Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL This is not a sync issue between structure and db revs. It just seems a logical move. Note that the interface for long does not allow these fields to be empty. Test plan: Run dbrev. Add a new Z3950/SRU server. Verify if it works as expected. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 141240 [details] [review] Bug 30571: (DBIx schema) Update for Z3950server.pm No test plan. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 141242 [details] [review] Bug 30571: Z3950Servers.t - Create subtest for existing tests No real changes, just moving code and indentation. Ground work for following patch. Test plan: To let the test pass, disable strict_mode in koha-conf. Run perl t/db_dependent/Koha/Z3950Servers.t. Should pass now. However, in strict mode or with prove, it should fail. Run prove t/db_dependent/Koha/Z3950Servers.t. Failed? Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 141243 [details] [review] Bug 30571: Z3950Servers.t - Add another subtest Subtest for testing nullability of host, syntax and encoding. Test plan: Run prove t/db_dependent/Koha/Z3950Servers.t. (Note: you need strict mode to pass this test.) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 141244 [details] [review] Bug 30571: (QA follow-up) Update marc to MARC in docs ... don't say it, I know. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 22.11. Nice work everyone, thanks!
From IRC [13:21] <cait1> marcelr: oleonard's database had nulls, so the update failed Will still take a look how to improve the atomicupdate a bit.
Created attachment 141361 [details] [review] Bug 30571: (follow-up) Additional check at upgrade Bad data occurs. We are removing null hosts here and update empty syntax or encoding, informing the user. Test plan: Remove the NOT NULL condition if needed. Copy db_revs/220600056.pl to atomicupdate and test dbrev. You might see something like: Upgrade to 22.06.00.056 [11:48:22]: Bug 30571 - Table z3950servers: three cols NOT NULL Found bad data in table z3950servers: removed 1 records with host undefined Corrected empty syntax or encoding for the following hosts. Please check after upgrade. Updated hosts: lx2.loc.gov,lx2.loc.gov Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tomas: Check last patch only
(In reply to Marcel de Rooy from comment #22) > Created attachment 141361 [details] [review] [review] > Bug 30571: (follow-up) Additional check at upgrade > > Bad data occurs. We are removing null hosts here and update > empty syntax or encoding, informing the user. > > Test plan: > Remove the NOT NULL condition if needed. > Copy db_revs/220600056.pl to atomicupdate and test dbrev. > You might see something like: > Upgrade to 22.06.00.056 [11:48:22]: Bug 30571 - Table z3950servers: > three cols NOT NULL > Found bad data in table z3950servers: removed 1 records with host > undefined > Corrected empty syntax or encoding for the following hosts. Please > check after upgrade. > Updated hosts: lx2.loc.gov,lx2.loc.gov > > Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Forgot to add the second obvious step in testing: create some bad data..
Please rebase ASAP.
(In reply to Tomás Cohen Arazi from comment #25) > Please rebase ASAP. Only the last patch please.
Follow-up pushed to master, thanks Marcel!