Bug 30571 - Table z3950servers: Make host, syntax and encoding NOT NULL
Summary: Table z3950servers: Make host, syntax and encoding NOT NULL
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-20 13:34 UTC by Marcel de Rooy
Modified: 2023-12-28 20:42 UTC (History)
3 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:
22.11.00


Attachments
Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL (4.12 KB, patch)
2022-08-24 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30571: (DBIx schema) Update for Z3950server.pm (2.47 KB, patch)
2022-08-24 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Create subtest for existing tests (3.59 KB, patch)
2022-08-24 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Add another subtest (2.95 KB, patch)
2022-08-24 14:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL (4.14 KB, patch)
2022-09-02 07:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30571: (DBIx schema) Update for Z3950server.pm (2.49 KB, patch)
2022-09-02 07:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Create subtest for existing tests (3.61 KB, patch)
2022-09-02 07:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Add another subtest (2.97 KB, patch)
2022-09-02 07:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL (4.19 KB, patch)
2022-10-03 10:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30571: (DBIx schema) Update for Z3950server.pm (2.55 KB, patch)
2022-10-03 10:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Create subtest for existing tests (3.66 KB, patch)
2022-10-03 10:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30571: Z3950Servers.t - Add another subtest (3.03 KB, patch)
2022-10-03 10:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30571: (QA follow-up) Update marc to MARC in docs (2.45 KB, patch)
2022-10-03 10:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30571: (follow-up) Additional check at upgrade (2.48 KB, patch)
2022-10-05 11:54 UTC, Marcel de Rooy
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 2022-04-20 13:34:47 UTC
`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.
Comment 1 Marcel de Rooy 2022-04-20 13:37:35 UTC
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.
Comment 2 Marcel de Rooy 2022-04-20 13:39:08 UTC
Linking it to the sync omnibus since it is at least very related in nature.
Comment 3 Marcel de Rooy 2022-08-24 12:04:25 UTC
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.
Comment 4 Marcel de Rooy 2022-08-24 14:24:06 UTC
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>
Comment 5 Marcel de Rooy 2022-08-24 14:24:09 UTC
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>
Comment 6 Marcel de Rooy 2022-08-24 14:24:13 UTC
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>
Comment 7 Marcel de Rooy 2022-08-24 14:24:17 UTC
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>
Comment 8 Marcel de Rooy 2022-08-24 14:36:41 UTC
(In reply to Marcel de Rooy from comment #3)
> Removing the omnibus reference.

And moved to enhancement
Comment 9 Nick Clemens 2022-09-02 07:28:14 UTC
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>
Comment 10 Nick Clemens 2022-09-02 07:28:19 UTC
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>
Comment 11 Nick Clemens 2022-09-02 07:28:23 UTC
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>
Comment 12 Nick Clemens 2022-09-02 07:28:27 UTC
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>
Comment 13 Nick Clemens 2022-09-02 07:29:20 UTC
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
Comment 14 Marcel de Rooy 2022-09-02 07:51:44 UTC
(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.
Comment 15 Katrin Fischer 2022-10-03 10:36:17 UTC
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>
Comment 16 Katrin Fischer 2022-10-03 10:36:22 UTC
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>
Comment 17 Katrin Fischer 2022-10-03 10:36:27 UTC
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>
Comment 18 Katrin Fischer 2022-10-03 10:36:32 UTC
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>
Comment 19 Katrin Fischer 2022-10-03 10:36:36 UTC
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>
Comment 20 Tomás Cohen Arazi 2022-10-03 11:32:40 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 21 Marcel de Rooy 2022-10-05 11:25:23 UTC
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.
Comment 22 Marcel de Rooy 2022-10-05 11:54:18 UTC
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>
Comment 23 Marcel de Rooy 2022-10-05 11:55:17 UTC
Tomas: Check last patch only
Comment 24 Marcel de Rooy 2022-10-05 11:58:31 UTC
(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..
Comment 25 Tomás Cohen Arazi 2022-10-11 12:58:29 UTC
Please rebase ASAP.
Comment 26 Marcel de Rooy 2022-10-11 13:57:58 UTC
(In reply to Tomás Cohen Arazi from comment #25)
> Please rebase ASAP.

Only the last patch please.
Comment 27 Tomás Cohen Arazi 2022-10-12 12:06:45 UTC
Follow-up pushed to master, thanks Marcel!