Bug 18050

Summary: Missing constraint on aqbudgets.budget_period_id in aqbudgets
Product: Koha Reporter: Baptiste <baptiste.wojtkowski>
Component: DatabaseAssignee: Katrin Fischer <katrin.fischer>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: blocker    
Priority: P1 - high CC: agustinmoyano, aleisha, julian.maurice, lucas, martin.renvoize, mtj, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25078
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06, 19.11.12, 19.05.17
Bug Depends on:    
Bug Blocks: 5334, 8179, 26493, 15329    
Attachments: Bug 18050: Add FK constraint on aqbudgets.budget_period_id
Bug 18050: Add FK constraint on aqbudgets.budget_period_id
Bug 18050: [Do not push] Schema update
Bug 18050: Add relation alias to schema
Bug 18050: Add FK constraint on aqbudgets.budget_period_id
Bug 18050: [Do not push] Schema update
Bug 18050: Add relation alias to schema
Bug 18050: (follow-up) Force adding FK constraint
Bug 18050: (follow-up) Force adding FK constraint
Bug 18050: (QA follow-up) Adjust conditions and make use of message text
Bug 18050: Add FK constraint on aqbudgets.budget_period_id
Bug 18050: [Do not push] Schema update
Bug 18050: Add relation alias to schema
Bug 18050: (follow-up) Force adding FK constraint
Bug 18050: (QA follow-up) Adjust conditions and make use of message text
Bug 18050: move aqbudgetperiods table before aqbudgets, to fix sql error
follow-up pushed to master. Thanks !

Description Baptiste 2017-02-03 14:26:00 UTC
There is not foreign key constraint on the column budget_parent_id in the table aqbudget.
It is so possible to delete a budget without deleting the associated fund.
Comment 1 Katrin Fischer 2020-04-19 01:42:37 UTC
(In reply to Baptiste from comment #0)
> There is not foreign key constraint on the column budget_parent_id in the
> table aqbudget.
> It is so possible to delete a budget without deleting the associated fund.

2835 DROP TABLE IF EXISTS `aqbudgets`;
2836 CREATE TABLE `aqbudgets` ( -- information related to Funds
2837   `budget_id` int(11) NOT NULL auto_increment, -- primary key and unique number assigned to each fund by Koha
2838   `budget_parent_id` int(11) default NULL, -- if this fund is a child of another this will include the parent id (aqbudgets.budget_id)
...
2850   `budget_owner_id` int(11) default NULL, -- borrowernumber of the person who owns this fund (borrowers.borrowernumber)
2851   `budget_permission` int(1) default '0', -- level of permission for this fund (used only by the owner, only by the library, or anyone)
2852   PRIMARY KEY  (`budget_id`),
2853   KEY `budget_parent_id` (`budget_parent_id`),
2854   KEY `budget_code` (`budget_code`),
2855   KEY `budget_branchcode` (`budget_branchcode`),
2856   KEY `budget_period_id` (`budget_period_id`),
2857   KEY `budget_owner_id` (`budget_owner_id`)
2858 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;

budget_parent_id could have a constraint on aqbudgetperiods.budget_period_id

budget_owner_id should probably be an FK on borrowers.borrowernumber

Both should be still allowed to be empty (NULL), as they don't need to always be set.

Then we got:

budget_id = fund ID
budget_parent_id = used for child/sub funds to link them to the parent. 
I am not sure if you can have a FK constraint on another column in the same table?

I can add the changes to the kohastructure and via updatedatabase, but not sure what 'false' date we might expect to clean up here.
Comment 2 Katrin Fischer 2020-09-14 22:16:54 UTC
Created attachment 110076 [details] [review]
Bug 18050: Add FK constraint on aqbudgets.budget_period_id

This adds a FK constraint on aqbudgets.budget_period_id so that
a fund cannot be added with an invalid aqbudget.budget_period_id.

We should not have funds that belong to no budget. In case we have, the
update will be skipped and a note displayed.

Part1:
- Before applying the patch
- Make sure you have a budget with some funds linked to it
- You will have to change one of the funds to link to an invalid
  budget with SQL:
  UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id);
- Apply patch
- Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget.
- Repair your fund with SQL
  UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id);
  (... needs to be your existing budget_period_id)
- Run updatedatabase again - verify it runs successfully now.
- If you try to change the budget_period_id to a non-existing now with SQL, you will
  get a database error. The new FK doesn't permit it.
Part 2:
- Start fresh with the web installer, verify there are no errors on
  creating the database tables
Comment 3 Katrin Fischer 2020-09-14 23:04:28 UTC
Created attachment 110078 [details] [review]
Bug 18050: Add FK constraint on aqbudgets.budget_period_id

This adds a FK constraint on aqbudgets.budget_period_id so that
a fund cannot be added with an invalid aqbudget.budget_period_id.

We should not have funds that belong to no budget. In case we have, the
update will be skipped and a note displayed.

Part1:
- Before applying the patch
- Make sure you have a budget with some funds linked to it
- You will have to change one of the funds to link to an invalid
  budget with SQL:
  UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id);
- Apply patch
- Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget.
- Repair your fund with SQL
  UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id);
  (... needs to be your existing budget_period_id)
- Run updatedatabase again - verify it runs successfully now.
- If you try to change the budget_period_id to a non-existing now with SQL, you will
  get a database error. The new FK doesn't permit it.
Part 2:
- Start fresh with the web installer, verify there are no errors on
  creating the database tables
Comment 4 Katrin Fischer 2020-09-14 23:04:31 UTC
Created attachment 110079 [details] [review]
Bug 18050: [Do not push] Schema update
Comment 5 Katrin Fischer 2020-09-14 23:04:36 UTC
Created attachment 110080 [details] [review]
Bug 18050: Add relation alias to schema
Comment 6 Martin Renvoize 2020-09-16 11:14:05 UTC
Created attachment 110169 [details] [review]
Bug 18050: Add FK constraint on aqbudgets.budget_period_id

This adds a FK constraint on aqbudgets.budget_period_id so that
a fund cannot be added with an invalid aqbudget.budget_period_id.

We should not have funds that belong to no budget. In case we have, the
update will be skipped and a note displayed.

Part1:
- Before applying the patch
- Make sure you have a budget with some funds linked to it
- You will have to change one of the funds to link to an invalid
  budget with SQL:
  UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id);
- Apply patch
- Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget.
- Repair your fund with SQL
  UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id);
  (... needs to be your existing budget_period_id)
- Run updatedatabase again - verify it runs successfully now.
- If you try to change the budget_period_id to a non-existing now with SQL, you will
  get a database error. The new FK doesn't permit it.
Part 2:
- Start fresh with the web installer, verify there are no errors on
  creating the database tables

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2020-09-16 11:14:13 UTC
Created attachment 110170 [details] [review]
Bug 18050: [Do not push] Schema update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-09-16 11:14:22 UTC
Created attachment 110171 [details] [review]
Bug 18050: Add relation alias to schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-09-16 11:17:38 UTC
You've done a really nice comprehensive job here Katrin, thanks.

Signing off, it all looks good and works as expected from my perspective.

Minor point however.. this bug used to cover lots of other FK issues around this table too.. I agree with narrowing the scope, but it would be good to open bugs for the other parts, especially as you've taken the time to understand them a bit as shown by your first comment (not expecting you to take on adding them however.. just would be good to have them recorded somewhere before they get lost when this bug is pushed ;) )
Comment 10 Julian Maurice 2020-10-02 08:56:24 UTC
I think there is something wrong here: if database contains orphan budget, the updatedatabase will warn about it and go on.
One can see the warning and fix it, but there will be no easy way to re-run the update,
or one can miss the warning and continue using koha with a wrong schema.

That's not the first time I see this pattern, and I think there is a problem with that. What's the point of updatedatabase if it doesn't ensure that the schema is correct at the end ?
I think the updatedatabase should just die here, let the admin fix the problem and re-run the updatedatabase process.
Comment 11 Katrin Fischer 2020-10-02 09:28:00 UTC
(In reply to Julian Maurice from comment #10)
> I think there is something wrong here: if database contains orphan budget,
> the updatedatabase will warn about it and go on.
> One can see the warning and fix it, but there will be no easy way to re-run
> the update,
> or one can miss the warning and continue using koha with a wrong schema.
> 
> That's not the first time I see this pattern, and I think there is a problem
> with that. What's the point of updatedatabase if it doesn't ensure that the
> schema is correct at the end ?
> I think the updatedatabase should just die here, let the admin fix the
> problem and re-run the updatedatabase process.

We cannot do anything else in this case. When there are orphan funds (not budget here), then there might be orders linked to this fund - we cannot just delete them. We need to leave it up to the library to fix this. 

Our updatedatabase mechanism doesn't allow to stop and rerun (as far as I know?) and I think using the existing pattern with a warn is the best we can do here right now and it should not be put on this bug report to fix the underlying issue :)
Comment 12 Julian Maurice 2020-10-02 09:47:28 UTC
(In reply to Katrin Fischer from comment #11)
> (In reply to Julian Maurice from comment #10)
> > I think there is something wrong here: if database contains orphan budget,
> > the updatedatabase will warn about it and go on.
> > One can see the warning and fix it, but there will be no easy way to re-run
> > the update,
> > or one can miss the warning and continue using koha with a wrong schema.
> > 
> > That's not the first time I see this pattern, and I think there is a problem
> > with that. What's the point of updatedatabase if it doesn't ensure that the
> > schema is correct at the end ?
> > I think the updatedatabase should just die here, let the admin fix the
> > problem and re-run the updatedatabase process.
> 
> We cannot do anything else in this case. When there are orphan funds (not
> budget here), then there might be orders linked to this fund - we cannot
> just delete them. We need to leave it up to the library to fix this. 
> 
> Our updatedatabase mechanism doesn't allow to stop and rerun (as far as I
> know?) and I think using the existing pattern with a warn is the best we can
> do here right now and it should not be put on this bug report to fix the
> underlying issue :)

Can't we just die ?
In an atomic update file it won't stop the process because the file is eval'd, but once it is in updatedatabase.pl it can stop the process.
And if we die before changing the Version in systempreferences, next time updatedatabase is run it will restart from where it died.
Comment 13 Katrin Fischer 2020-10-04 18:32:57 UTC
I cannot attend the dev meeting this week, but I hope we can find a way to move forward with these kind of fixes.
Comment 14 Martin Renvoize 2020-10-07 14:45:14 UTC
As discussed in the dev meeting I think this bug should be unblocked whilst we work on the issues with updatedatabse discussed on bug 25078.

We should sort out all the existing warn cases that fall into this category at the same time on that bug and not hold up an important improvement for many.
Comment 15 Agustín Moyano 2020-10-08 22:50:46 UTC
Great job Katrin.. if I may add a comment, in the atomic update, if there are some budget periods that were not found, instead of not adding the FK constraint, I would:

1. Create a temporary table.. something like _bug_18050_aqbudgets where I would copy the offending rows

2. Update aqbudgets.budget_period_id to null, add the FK constraint

3. Emit the warning that some budget periods where not found, and a copy of those rows where preserved in _bug_18050_aqbudgets table

If it's ok to you, I can add a follow-up with those steps
Comment 16 Agustín Moyano 2020-10-09 18:19:11 UTC
Created attachment 111429 [details] [review]
Bug 18050: (follow-up) Force adding FK constraint

When there are inconsistent budget_period_id in aqbudgets this patch
creates the table _bug_18050_aqbudgets with the original data, then sets
the column to null and warns the user that there is inconsistent data.
Comment 17 Agustín Moyano 2020-10-09 18:21:02 UTC
Hi Katrin,

I added the follow-up just in case you'd like to check it out when you've got time. Please feel free to remove it if you feel it's not ok.

Thanks
Comment 18 Katrin Fischer 2020-10-09 19:14:29 UTC
(In reply to Agustín Moyano from comment #15)
> Great job Katrin.. if I may add a comment, in the atomic update, if there
> are some budget periods that were not found, instead of not adding the FK
> constraint, I would:
> 
> 1. Create a temporary table.. something like _bug_18050_aqbudgets where I
> would copy the offending rows
> 
> 2. Update aqbudgets.budget_period_id to null, add the FK constraint
> 
> 3. Emit the warning that some budget periods where not found, and a copy of
> those rows where preserved in _bug_18050_aqbudgets table
> 
> If it's ok to you, I can add a follow-up with those steps

Hi Augustin, 

it's not as simple as that I am afraid. If there are orphan aqbudgets, there could also be orders attached to them. And if there are orders, there could be linked items... the rabbit hole is deeper.

Maybe another better approach could be to go the other direction: We add a aqbudgetperiods entry - this would also allow the library to deal with things in the GUI.

I am not sure I am able to write that one, so if anyone beats me to it I'd be more than happy.
Comment 19 Katrin Fischer 2020-10-15 11:25:45 UTC
Augustin, I think I misunderstood initially what you are trying to do. I thought we were copying the data out and then removing it from the original table - but instead we set the id to NULL and just keep the former information.

I will give this a test run.
Comment 20 Katrin Fischer 2020-10-15 11:55:38 UTC
Created attachment 111712 [details] [review]
Bug 18050: (follow-up) Force adding FK constraint

When there are inconsistent budget_period_id in aqbudgets this patch
creates the table _bug_18050_aqbudgets with the original data, then sets
the column to null and warns the user that there is inconsistent data.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2020-10-15 11:55:44 UTC
Created attachment 111713 [details] [review]
Bug 18050: (QA follow-up) Adjust conditions and make use of message text

- When we run into invalid data, we use Augustin's suggested solution
  and copy the data into a separate table, setting the budget_period_id to
  NULL.
- We amend the output using the new $message.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Nick Clemens 2020-10-15 15:07:02 UTC
Created attachment 111751 [details] [review]
Bug 18050: Add FK constraint on aqbudgets.budget_period_id

This adds a FK constraint on aqbudgets.budget_period_id so that
a fund cannot be added with an invalid aqbudget.budget_period_id.

We should not have funds that belong to no budget. In case we have, the
update will be skipped and a note displayed.

Part1:
- Before applying the patch
- Make sure you have a budget with some funds linked to it
- You will have to change one of the funds to link to an invalid
  budget with SQL:
  UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id);
- Apply patch
- Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget.
- Repair your fund with SQL
  UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id);
  (... needs to be your existing budget_period_id)
- Run updatedatabase again - verify it runs successfully now.
- If you try to change the budget_period_id to a non-existing now with SQL, you will
  get a database error. The new FK doesn't permit it.
Part 2:
- Start fresh with the web installer, verify there are no errors on
  creating the database tables

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Nick Clemens 2020-10-15 15:07:06 UTC
Created attachment 111752 [details] [review]
Bug 18050: [Do not push] Schema update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Nick Clemens 2020-10-15 15:07:14 UTC
Created attachment 111753 [details] [review]
Bug 18050: Add relation alias to schema

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Nick Clemens 2020-10-15 15:07:17 UTC
Created attachment 111754 [details] [review]
Bug 18050: (follow-up) Force adding FK constraint

When there are inconsistent budget_period_id in aqbudgets this patch
creates the table _bug_18050_aqbudgets with the original data, then sets
the column to null and warns the user that there is inconsistent data.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Nick Clemens 2020-10-15 15:07:22 UTC
Created attachment 111755 [details] [review]
Bug 18050: (QA follow-up) Adjust conditions and make use of message text

- When we run into invalid data, we use Augustin's suggested solution
  and copy the data into a separate table, setting the budget_period_id to
  NULL.
- We amend the output using the new $message.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Jonathan Druart 2020-10-25 23:05:34 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 28 Mason James 2020-10-26 18:45:14 UTC
Created attachment 112529 [details] [review]
Bug 18050: move aqbudgetperiods table before aqbudgets, to fix sql error

00:02:16.310 koha_1       | DBD::mysql::st execute failed: Can't create table `koha_kohadev`.`aqbudgets` (errno: 150 "Foreign key constraint is incorrectly formed") at /usr/share/perl5/DBIx/RunSQL.pm line 278, <$args{...}> line 1.
00:02:16.310 koha_1       | Something went wrong loading file /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE TABLE `aqbudgets` ( -- information related to Funds
Comment 29 Tomás Cohen Arazi 2020-10-26 18:56:06 UTC
Created attachment 112530 [details] [review]
follow-up pushed to master. Thanks !
Comment 30 Katrin Fischer 2020-10-26 19:34:35 UTC
Sorry for missing this and thx for the fix!
Comment 31 Lucas Gass 2020-11-13 21:26:01 UTC
backported to 20.05.x for 20.05.06
Comment 32 Aleisha Amohia 2020-11-17 04:57:46 UTC
backported to 19.11.x for 19.11.12
Comment 33 Victor Grousset/tuxayo 2020-11-17 12:57:55 UTC
Backported to 19.05.x branch for 19.05.17