Bug 30571

Summary: Table z3950servers: Make host, syntax and encoding NOT NULL
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: DatabaseAssignee: Marcel de Rooy <m.de.rooy>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: nick
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL
Bug 30571: (DBIx schema) Update for Z3950server.pm
Bug 30571: Z3950Servers.t - Create subtest for existing tests
Bug 30571: Z3950Servers.t - Add another subtest
Bug 30571: Table z3950servers: Make host, syntax and encoding NOT NULL
Bug 30571: (DBIx schema) Update for Z3950server.pm
Bug 30571: Z3950Servers.t - Create subtest for existing tests
Bug 30571: Z3950Servers.t - Add another subtest

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.