Bug 37419 - Deleting the record source deletes the associated biblio_metadata rows
Summary: Deleting the record source deletes the associated biblio_metadata rows
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 35919
Blocks: 36372 37513 37510
  Show dependency treegraph
 
Reported: 2024-07-22 08:57 UTC by Janusz Kaczmarek
Modified: 2024-11-25 11:38 UTC (History)
8 users (show)

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


Attachments
Bug 37419: Update FK constraint on record sources to avoid data loss (3.25 KB, patch)
2024-07-29 11:24 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 37419: DBIC schema (1.13 KB, patch)
2024-07-29 11:26 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 37419: Update FK constraint on record sources to avoid data loss (3.30 KB, patch)
2024-07-29 12:17 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 37419: DBIC schema (1.17 KB, patch)
2024-07-29 12:18 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 37419: Update FK constraint on record sources to avoid data loss (3.37 KB, patch)
2024-08-01 10:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37419: DBIC schema (1.23 KB, patch)
2024-08-01 10:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2024-07-22 08:57:40 UTC
After applying the Bug 36372, it is possible to manually set record source for a given bibliographic record.  At the same time, from /cgi-bin/koha/admin/record_sources it is possible to delete a record source currently being used, without any control.  This in turn causes the deletion of all the rows from the biblio_metadata table linked with the record source being deleted, which is fatal.  BTW the biblio and biblioitems rows remain in the database.

This effect is caused by "CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON DELETE CASCADE" in biblio_metadata setup.

Of course the same effect occurs when deleting the record_sources row by hand, directly in the database.

I am not sure if the decision to link biblio_metadata with record_sources with ON DELETE CASCADE is just, even if we controlled the use of the specific record source before deletion (as we do with itemtypes for instance).
Comment 1 Arthur Suzuki 2024-07-25 07:41:45 UTC
I was able to reproduce the issue
Comment 2 David Cook 2024-07-29 01:05:47 UTC
Sounds like this should be "ON DELETE RESTRICT"
Comment 3 Katrin Fischer 2024-07-29 07:33:37 UTC
(In reply to David Cook from comment #2)
> Sounds like this should be "ON DELETE RESTRICT"

Makes sense. Adding RM_priority as this could lead to severe data loss, especially since rows are deleted directly instead of being moved...
Comment 4 Tomás Cohen Arazi (tcohen) 2024-07-29 11:24:10 UTC
Created attachment 169823 [details] [review]
Bug 37419: Update FK constraint on record sources to avoid data loss

Without this patch, deleting a record source will delete the associated
biblio_metadata rows, which is a severe data loss.

This patch makes the constraint restrict this action.

To test:
1. Add a record source
2. Set the record source to some records
   $ koha-mysql kohadev
   > UPDATE biblio_metadata SET record_source_id='your source id' WHERE
biblionumber=1;
3. Delete the record source
=> FAIL: Record metadata deleted
4. Apply this patch
5, Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: DB update goes well
6. Repeat 1~3 with another record
=> SUCCESS: Source cannot be deleted if there are linked records

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi (tcohen) 2024-07-29 11:26:05 UTC
Created attachment 169824 [details] [review]
[DO NOT PUSH] Bug 37419: DBIC schema

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Janusz Kaczmarek 2024-07-29 12:17:59 UTC
Created attachment 169827 [details] [review]
Bug 37419: Update FK constraint on record sources to avoid data loss

Without this patch, deleting a record source will delete the associated
biblio_metadata rows, which is a severe data loss.

This patch makes the constraint restrict this action.

To test:
1. Add a record source
2. Set the record source to some records
   $ koha-mysql kohadev
   > UPDATE biblio_metadata SET record_source_id='your source id' WHERE
biblionumber=1;
3. Delete the record source
=> FAIL: Record metadata deleted
4. Apply this patch
5, Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: DB update goes well
6. Repeat 1~3 with another record
=> SUCCESS: Source cannot be deleted if there are linked records

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Comment 7 Janusz Kaczmarek 2024-07-29 12:18:02 UTC
Created attachment 169828 [details] [review]
Bug 37419: DBIC schema

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Comment 8 Janusz Kaczmarek 2024-07-29 12:20:56 UTC
Just one remark: the message generated after an unsuccessful deletion sounds: "Something went wrong: Error: Something went wrong, check Koha logs for details."  I wonder if it shouldn't/couldn't be more detailed...
Comment 9 Tomás Cohen Arazi (tcohen) 2024-07-29 12:33:57 UTC
(In reply to Janusz Kaczmarek from comment #8)
> Just one remark: the message generated after an unsuccessful deletion
> sounds: "Something went wrong: Error: Something went wrong, check Koha logs
> for details."  I wonder if it shouldn't/couldn't be more detailed...

Indeed. I'm working on bug 37510 to properly handle the DBIC exception from ->delete() in the codebase, and then will handle it on the Vue side. Thanks for the feedback!
Comment 10 Martin Renvoize (ashimema) 2024-08-01 10:56:05 UTC
Created attachment 169932 [details] [review]
Bug 37419: Update FK constraint on record sources to avoid data loss

Without this patch, deleting a record source will delete the associated
biblio_metadata rows, which is a severe data loss.

This patch makes the constraint restrict this action.

To test:
1. Add a record source
2. Set the record source to some records
   $ koha-mysql kohadev
   > UPDATE biblio_metadata SET record_source_id='your source id' WHERE
biblionumber=1;
3. Delete the record source
=> FAIL: Record metadata deleted
4. Apply this patch
5, Run:
   $ ktd --shell
  k$ updatedatabase
=> SUCCESS: DB update goes well
6. Repeat 1~3 with another record
=> SUCCESS: Source cannot be deleted if there are linked records

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2024-08-01 10:56:08 UTC
Created attachment 169933 [details] [review]
Bug 37419: DBIC schema

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Janusz Kaczmarek <januszop@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Katrin Fischer 2024-08-01 16:27:30 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 13 Lucas Gass (lukeg) 2024-08-29 17:27:05 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 14 Fridolin Somers 2024-09-16 14:09:32 UTC
Depends on Bug 35919 not in 23.11.x
Comment 15 Katrin Fischer 2024-11-22 17:34:07 UTC
I am seeing something strange here when running the database scripts for comparing new/udpate before release:

In the generated kohastructure.sql:

@@ -1151,7 +1150,7 @@ CREATE TABLE `biblio_metadata` (
   KEY `timestamp` (`timestamp`),
   KEY `record_metadata_fk_2` (`record_source_id`),
   CONSTRAINT `record_metadata_fk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE,
-  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON DELETE RESTRICT ON UPDATE CASCADE
+  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON UPDATE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
 /*!40101 SET character_set_client = @saved_cs_client */;


Where has the "ON DELETE RESTRICT" gone?
Comment 16 Janusz Kaczmarek 2024-11-22 22:31:15 UTC
How is that possible?  In master we see at line 1147 (from this bug):

-  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON DELETE CASCADE ON UPDATE CASCADE
+  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON DELETE RESTRICT ON UPDATE CASCADE

and then noting in this region. Strange...
Comment 17 Katrin Fischer 2024-11-23 08:25:32 UTC
(In reply to Janusz Kaczmarek from comment #16)
> How is that possible?  In master we see at line 1147 (from this bug):
> 
> -  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`)
> REFERENCES `record_sources` (`record_source_id`) ON DELETE CASCADE ON UPDATE
> CASCADE
> +  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`)
> REFERENCES `record_sources` (`record_source_id`) ON DELETE RESTRICT ON
> UPDATE CASCADE
> 
> and then noting in this region. Strange...

I have no explanation either. I'll probably repeat the steps on Monday just to make sure, but we need to resolve.
Comment 18 Katrin Fischer 2024-11-25 10:04:26 UTC
@@ -1151,7 +1150,7 @@ CREATE TABLE `biblio_metadata` (
   KEY `timestamp` (`timestamp`),
   KEY `record_metadata_fk_2` (`record_source_id`),
   CONSTRAINT `record_metadata_fk_1` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE,
-  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON DELETE RESTRICT ON UPDATE CASCADE
+  CONSTRAINT `record_metadata_fk_2` FOREIGN KEY (`record_source_id`) REFERENCES `record_sources` (`record_source_id`) ON UPDATE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;
 /*!40101 SET character_set_client = @saved_cs_client */;

We have come to the conclusion that ON UPDATE RESTRICT is missing form the dump, as this is the default behavior. So this is a non-issue actually and things are in order.
Comment 19 Tomás Cohen Arazi (tcohen) 2024-11-25 11:27:25 UTC
(In reply to Katrin Fischer from comment #18)
>
> We have come to the conclusion that ON UPDATE RESTRICT is missing form the
> dump, as this is the default behavior. So this is a non-issue actually and
> things are in order.

Small behavior differences in MariaDB would cause this. It really depends on which MariaDB version you're running this at.

When you regenerated for 24.05, this is what we used:

```
-- Server version	11.3.2-MariaDB-1:11.3.2+maria~ubu2204
```

for 23.11 we used this:

```
-- Server version      11.2.2-MariaDB-1:11.2.2+maria~ubu2204
```
Comment 20 Katrin Fischer 2024-11-25 11:28:45 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #19)
> (In reply to Katrin Fischer from comment #18)
> >
> > We have come to the conclusion that ON UPDATE RESTRICT is missing form the
> > dump, as this is the default behavior. So this is a non-issue actually and
> > things are in order.
> 
> Small behavior differences in MariaDB would cause this. It really depends on
> which MariaDB version you're running this at.
> 
> When you regenerated for 24.05, this is what we used:
> 
> ```
> -- Server version	11.3.2-MariaDB-1:11.3.2+maria~ubu2204
> ```
> 
> for 23.11 we used this:
> 
> ```
> -- Server version      11.2.2-MariaDB-1:11.2.2+maria~ubu2204
> ```

The instructions say to use this:
DB_IMAGE=mariadb:latest ktd up -d

So it makes sense that a newer version was used this time.
Comment 21 Tomás Cohen Arazi (tcohen) 2024-11-25 11:38:48 UTC
(In reply to Katrin Fischer from comment #20)
> The instructions say to use this:
> DB_IMAGE=mariadb:latest ktd up -d
> 
> So it makes sense that a newer version was used this time.

Absolutely. Just noted that minor changes are common because of the version change.