Bug 35473 - Core bookings and room reservations plugin tables clash
Summary: Core bookings and room reservations plugin tables clash
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
: 35697 (view as bug list)
Depends on: 29002
Blocks:
  Show dependency treegraph
 
Reported: 2023-12-04 09:37 UTC by Martin Renvoize
Modified: 2024-03-20 12:08 UTC (History)
8 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:
24.05.00,23.11.04


Attachments
Bug 35473: Handle room reservations plugin conflict (3.41 KB, patch)
2024-01-11 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35473: Handle room reservations plugin conflict (3.47 KB, patch)
2024-01-11 16:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35473: Also disable the plugin (1.07 KB, patch)
2024-01-15 15:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35473: Handle room reservations plugin conflict (3.47 KB, patch)
2024-02-05 09:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35473: Also disable the plugin (2.44 KB, patch)
2024-02-05 09:15 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35473: Handle room reservations plugin conflict (3.56 KB, patch)
2024-03-15 07:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35473: Also disable the plugin (2.53 KB, patch)
2024-03-15 07:27 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 Martin Renvoize 2023-12-04 09:37:16 UTC
Some early 23.11 adopters have spotted that the rooms reservations plugin already adds a 'bookings' table (without any form of table qualification which is modern best practice).

We'll want to try and identify when this is the case and migrate the table.  I'll contact the authors for the original plugin to discuss the next steps.. but there's lots of forks of this particular plugin so we'll want to somehow highlight issues to end users too.
Comment 1 Martin Renvoize 2024-01-11 10:11:53 UTC
The plugin has now been updated to prevent the conflict, however we still need to fix the database update
Comment 2 Martin Renvoize 2024-01-11 10:25:28 UTC
Created attachment 160827 [details] [review]
Bug 35473: Handle room reservations plugin conflict

We now look for the existance of the room reservations 'bookings' table
prior to attempting to install the core bookings table and migrate it in
the same way the upstream plugin does before proceeding with installing
the core bookings tables.
Comment 3 Martin Renvoize 2024-01-11 10:27:24 UTC
Kyle.. I think we'll need an additional check in the plugin side to look for whether the table has already been migrated prior to upgrade (i.e check that the bookings table has booking_id vs bookingid column.)

I'm also tempted to add a 'disable' call into here to disable the room reservations plugin if found to ensure people go and upgrade it prior trying to use it again.
Comment 4 Kyle M Hall 2024-01-11 16:41:13 UTC
Created attachment 160879 [details] [review]
Bug 35473: Handle room reservations plugin conflict

We now look for the existance of the room reservations 'bookings' table
prior to attempting to install the core bookings table and migrate it in
the same way the upstream plugin does before proceeding with installing
the core bookings tables.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2024-01-11 16:41:41 UTC
(In reply to Martin Renvoize from comment #3)
> Kyle.. I think we'll need an additional check in the plugin side to look for
> whether the table has already been migrated prior to upgrade (i.e check that
> the bookings table has booking_id vs bookingid column.)
> 
> I'm also tempted to add a 'disable' call into here to disable the room
> reservations plugin if found to ensure people go and upgrade it prior trying
> to use it again.

Sounds like a good plan to me!
Comment 6 Martin Renvoize 2024-01-15 15:34:08 UTC
Created attachment 161043 [details] [review]
Bug 35473: Also disable the plugin
Comment 7 Katrin Fischer 2024-01-22 10:03:11 UTC
*** Bug 35697 has been marked as a duplicate of this bug. ***
Comment 8 Marcel de Rooy 2024-01-26 07:30:02 UTC
 WARN   installer/data/mysql/db_revs/230600061.pl
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 11)
Comment 9 Martin Renvoize 2024-02-05 09:15:17 UTC
Created attachment 161742 [details] [review]
Bug 35473: Handle room reservations plugin conflict

We now look for the existance of the room reservations 'bookings' table
prior to attempting to install the core bookings table and migrate it in
the same way the upstream plugin does before proceeding with installing
the core bookings tables.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Martin Renvoize 2024-02-05 09:15:20 UTC
Created attachment 161743 [details] [review]
Bug 35473: Also disable the plugin
Comment 11 Martin Renvoize 2024-02-05 09:15:57 UTC
Tidied.. again.
Comment 12 Martin Renvoize 2024-02-15 10:47:46 UTC
I'm getting a bit tired of a tiny tidy requirement simply because master moved stopping a bug in it's tracks :(
Comment 13 Martin Renvoize 2024-02-15 10:48:18 UTC
This is a pretty darn critical bug for upgrades that can completely break Koha.. amazing it doesn't get any attention
Comment 14 Marcel de Rooy 2024-03-01 08:26:22 UTC
(In reply to Martin Renvoize from comment #13)
> This is a pretty darn critical bug for upgrades that can completely break
> Koha.. amazing it doesn't get any attention

Will have a look now. But you know about 34478 ;)
Comment 15 Marcel de Rooy 2024-03-01 08:41:19 UTC
This is an interesting patch. I tend to think that we should not touch the tables of a plugin. And just abort here? But that may be a bit too formal response?

I like to have bit more feedback from QA. Moving to ID and sending a mail.

Do we choose for Pragmatic here and just move the tables, or do we choose to just bail out and let the plugin stuff be handled outside Koha?

Note that the question also arises: What should we do for other plugins in the future that created tables in Koha and clash? I would normally not expect Koha to contain knowledge about plugin tables?
Comment 16 Julian Maurice 2024-03-01 09:02:52 UTC
(In reply to Marcel de Rooy from comment #15)
> Note that the question also arises: What should we do for other plugins in
> the future that created tables in Koha and clash? I would normally not
> expect Koha to contain knowledge about plugin tables?

I think we should try to avoid using table names that are already used by known plugins, but if that happens (like in this bug) I think it's the plugin responsibility to move its own data.
Now what should Koha do ? Stop the update process, and if the plugin is known add a message suggesting to update the plugin first ? Can the plugin update be done in the middle of the updatedb process ?
Comment 17 Jonathan Druart 2024-03-01 09:08:54 UTC
On bug 30650 (Curbside pickups) I took care of the plugin's data (the idea was to provide the same feature as the plugin).

The patch created the same tables as the plugin, then adjusted the structure to reach the new one.
Comment 18 Martin Renvoize 2024-03-01 11:59:01 UTC
Hmm, it does raise a fiar question.

In this case I worked with the plugin maintainers to get both sides of the equation done.. so, with this patch we don't just explode badly and you cna upgrade the plugin either before or after the upgrade and it works.

Withouth this patch, if you don't happen to know you need to upgrade the plugin first, you'll end up with a nasty silent failure during upgrade which, without some code that's very much aware of the plugin table layout isn't at all easy to spot.

I'm open to suggestions of course, but I couldn't see an alternative.
Comment 19 Tomás Cohen Arazi 2024-03-01 14:11:09 UTC
Plugins should always use:

    my $table_name = $self->get_qualified_table_name('name_you_want');
Comment 20 Martin Renvoize 2024-03-12 16:39:11 UTC
Do we have a proposed path forward here.. as it stands Koha just breaks silently at upgrade if room reservations was installed..
Comment 21 Kyle M Hall 2024-03-12 16:42:25 UTC
(In reply to Martin Renvoize from comment #20)
> Do we have a proposed path forward here.. as it stands Koha just breaks
> silently at upgrade if room reservations was installed..

IMO Koha continuing to function should trump all other things.
Comment 22 Katrin Fischer 2024-03-12 20:54:44 UTC
I think we have the best possible outcome here that will allow libraries to fix the issue without losing data.
Comment 23 Marcel de Rooy 2024-03-15 07:27:30 UTC
Created attachment 163159 [details] [review]
Bug 35473: Handle room reservations plugin conflict

We now look for the existance of the room reservations 'bookings' table
prior to attempting to install the core bookings table and migrate it in
the same way the upstream plugin does before proceeding with installing
the core bookings tables.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2024-03-15 07:27:32 UTC
Created attachment 163160 [details] [review]
Bug 35473: Also disable the plugin

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2024-03-15 07:28:32 UTC
(In reply to Katrin Fischer from comment #22)
> I think we have the best possible outcome here that will allow libraries to
> fix the issue without losing data.

No, I would not completely agree here. Certainly not the best possible outcome.
I tend to allow it but it creates precedence for mixing specific plugin details and our codebase. Which is actually not nice.
Theoretically, we should just abort the upgrade and leave plugin fixing to plugin support.
But feeling the pressure (..) we might make yet another exception here :)

Passing QA

QA team: Please dont make such db revs. And dont allow them in the future ;)
Comment 26 Marcel de Rooy 2024-03-15 07:29:35 UTC
(In reply to Kyle M Hall from comment #21)
> IMO Koha continuing to function should trump all other things.

The verb trump makes me think of someone :)
Comment 27 Katrin Fischer 2024-03-15 07:58:05 UTC
I believe the thinking was that if you are in the middle of an upgrade already, it's easier to make it continue and fix the plugin later from GUI. I can see that fixing "before" update could be another route. 
I DO hope that we won't run into more issues like this, but one can only hope.

Should we adjust our checks for existing tables somehow?
Comment 28 Katrin Fischer 2024-03-15 08:39:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 29 Fridolin Somers 2024-03-18 09:05:41 UTC
Pushed to 23.11.x for 23.11.04
Comment 30 Lucas Gass 2024-03-19 21:37:19 UTC
Not needed in 23.05.x, no backport.