Bug 30490 - Adjust foreign key for parent item type
Summary: Adjust foreign key for parent item type
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 30486
  Show dependency treegraph
 
Reported: 2022-04-07 14:32 UTC by Marcel de Rooy
Modified: 2023-06-08 22:28 UTC (History)
0 users

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1 (2.48 KB, patch)
2022-08-23 13:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1 (2.50 KB, patch)
2022-09-02 07:13 UTC, Nick Clemens
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-07 14:32:54 UTC
This is a sync issue between schema and upgrades.

MASTER structure:   CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`) ON DELETE CASCADE ON UPDATE CASCADE

$DBversion = '20.06.00.022'; ALTER TABLE itemtypes ADD CONSTRAINT itemtypes_ibfk_1 FOREIGN KEY (parent_type) REFERENCES itemtypes (itemtype)

NOTE: When you do not add CASCADE, you fallback to RESTRICT !

Main question is now: Which is right? Should the delete be cascading or not? I would opt for RESTRICT here. A pragmatical choice too; deleting item types will normally not be done so much :)
Comment 1 Katrin Fischer 2022-06-25 15:33:18 UTC
My feeling would be that if you delete a parent itemtype, it should set the field to NULL in the 'child itemtypes'. Does that make sense?

IIUC quietly deleting the 'children' seems a dangerous move, although it should hopefully not be possible while items exist for them.
Comment 2 Marcel de Rooy 2022-08-23 12:07:25 UTC
(In reply to Katrin Fischer from comment #1)
> My feeling would be that if you delete a parent itemtype, it should set the
> field to NULL in the 'child itemtypes'. Does that make sense?

Yes. That is the same as RESTRICT.

> IIUC quietly deleting the 'children' seems a dangerous move, although it
> should hopefully not be possible while items exist for them.

Looking closer, the items table has no FK on itype. So you could delete itemtypes but the items will not be deleted.
But note that the interface will not allow you to delete the itemtype.
But you could in a script or via sql etc, ending up with items that have not existing itemtype.
But that is outside the narrower scope of this report. And probably already reported somewhere else..
Comment 3 Marcel de Rooy 2022-08-23 13:14:03 UTC
(In reply to Marcel de Rooy from comment #2)
> (In reply to Katrin Fischer from comment #1)
> > My feeling would be that if you delete a parent itemtype, it should set the
> > field to NULL in the 'child itemtypes'. Does that make sense?
> 
> Yes. That is the same as RESTRICT.

Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot delete the parent type if there still is a child itemtype.
Comment 4 Marcel de Rooy 2022-08-23 13:25:33 UTC
Created attachment 139678 [details] [review]
Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1

Change from CASCADE to restrict.
In harmony with dbrev 20.06.00.022.

Test plan:
Run the dbrev.
Bonus:
update itemtypes set parent_type='VM' where itemtype='CF';
delete from itemtypes where itemtype='VM';
=> ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`koha_myclone`.`itemtypes`, CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`))

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2022-08-23 13:25:57 UTC
Self signoff
Comment 6 Marcel de Rooy 2022-08-23 13:37:34 UTC
RM: Dont forget DBIx update please, not included here
Comment 7 Katrin Fischer 2022-08-23 13:59:17 UTC
(In reply to Marcel de Rooy from comment #3)
> (In reply to Marcel de Rooy from comment #2)
> > (In reply to Katrin Fischer from comment #1)
> > > My feeling would be that if you delete a parent itemtype, it should set the
> > > field to NULL in the 'child itemtypes'. Does that make sense?
> > 
> > Yes. That is the same as RESTRICT.
> 
> Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot
> delete the parent type if there still is a child itemtype.

Hm, I not sure about this one.

You might just add a parent itemtype for testing, the only purpose right now is the max checkouts, later decide to remove it... It's not a required/mandatory field, so I'd day NULL would be ok, maybe with a confirmation warning in the GUI.

This way you need to remove it in all itemtypes first, more clicks, but I can live it, just think the other is nicer :)
Comment 8 Marcel de Rooy 2022-08-23 14:02:04 UTC
(In reply to Katrin Fischer from comment #7)
> (In reply to Marcel de Rooy from comment #3)
> > (In reply to Marcel de Rooy from comment #2)
> > > (In reply to Katrin Fischer from comment #1)
> > > > My feeling would be that if you delete a parent itemtype, it should set the
> > > > field to NULL in the 'child itemtypes'. Does that make sense?
> > > 
> > > Yes. That is the same as RESTRICT.
> > 
> > Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot
> > delete the parent type if there still is a child itemtype.
> 
> Hm, I not sure about this one.
> 
> You might just add a parent itemtype for testing, the only purpose right now
> is the max checkouts, later decide to remove it... It's not a
> required/mandatory field, so I'd day NULL would be ok, maybe with a
> confirmation warning in the GUI.
> 
> This way you need to remove it in all itemtypes first, more clicks, but I
> can live it, just think the other is nicer :)

This would be an enhancment actually.
Now I restore the sync gap between dbrev and structure.
You are invited to open a new report? :)
Comment 9 Nick Clemens 2022-09-02 07:13:12 UTC
Created attachment 140072 [details] [review]
Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1

Change from CASCADE to restrict.
In harmony with dbrev 20.06.00.022.

Test plan:
Run the dbrev.
Bonus:
update itemtypes set parent_type='VM' where itemtype='CF';
delete from itemtypes where itemtype='VM';
=> ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`koha_myclone`.`itemtypes`, CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`))

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Tomás Cohen Arazi 2022-09-22 12:43:09 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!