Bug 10517 - koha-restore fails to create mysqluser@mysql_hostname so zebra update fails
Summary: koha-restore fails to create mysqluser@mysql_hostname so zebra update fails
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Rudolf Byker
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-30 01:30 UTC by John.Seymour
Modified: 2022-07-05 09:29 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.06


Attachments
patch koha-restore (2.35 KB, application/x-shellscript)
2013-06-30 01:30 UTC, John.Seymour
Details
Use "DROP USER IF EXISTS" and remove " || true" (770 bytes, patch)
2019-05-10 09:10 UTC, Rudolf Byker
Details | Diff | Splinter Review
Use `DROP USER IF EXISTS` and remove ` || true`. (2.40 KB, patch)
2020-01-08 09:30 UTC, Rudolf Byker
Details | Diff | Splinter Review
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. (2.41 KB, patch)
2021-11-19 13:42 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. (2.47 KB, patch)
2022-04-25 11:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`. (2.57 KB, patch)
2022-04-29 09:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 10517: (QA follow-up) No need to remove second true (1.18 KB, patch)
2022-04-29 09:20 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 John.Seymour 2013-06-30 01:30:33 UTC
Created attachment 19284 [details]
patch koha-restore

kohe-restore fails to create mysqluser@mysql_hostname so zebra update fails. It also attempts to drop the mysqluser which does not exist causing an error. Please find attached an updated version which corrects these problems.
Comment 1 Katrin Fischer 2013-06-30 16:09:17 UTC
Hi Robin, could you maybe take a look at this?
Comment 2 Robin Sheat 2013-07-01 13:03:01 UTC
I don't think removing the instruction to remove the user if it exists is a good idea, as it might be there. And it it is there, then we want to remove it. As it stands, the error coming from that failing should be ignored and the script should proceed. If that's not happening, that's a bug in and of itself (though looking at it, it should be OK.)

As for adding the permission for @$mysql_hostname, I wonder if that should be fixed to 'localhost' like it is in koha-create. This is to account for connecting via either a UNIX socket or a loopback TCP connection within a machine. For situations where the mysql server is on a remote host, we're probably reaching the point of complexity where koha-restore isn't suitable in it's current form.

So anyway, I think it should probably be modified such that mysql_hostname is always 'localhost', and that'll solve the zebra mysql issue.

Any thoughts on this, John?
Comment 3 John.Seymour 2013-07-04 03:41:53 UTC
I think that as the koha-remove script drops the user and if you are restoring to a new install the user should not exist, the existence of an existing user  is an error that should be flagged.

As for the  @$mysql_hostname I originaly had is as a static 'localhost' but as the hostname is available for the koha-conf.xml file I thought the use of this value would future proof the script. Feel free to change it back to a static if you think this is more appropriate.
Comment 4 Mason James 2016-05-18 02:26:13 UTC
any chance a new patch could be added in a diff/patch format?

 https://en.wikipedia.org/wiki/Diff_utility
Comment 5 Rudolf Byker 2019-05-10 08:55:14 UTC
We need to decide whether clobbering (overwriting) an instance is acceptable, and stick with that. If it is, then no error should be shown, and the user and database should be removed and re-created (or just updated). If it isn't, then the script should check everything at the very beginning, and stop before making ANY changes.

Another option is to give koha-restore an "--overwrite" flag to choose between these behaviours. Even then, we need to decide on a default.

I'm keen to improve/rewrite the script, but will leave the decision on default behaviour to the Koha experts.
Comment 6 Rudolf Byker 2019-05-10 09:10:26 UTC
Created attachment 89529 [details] [review]
Use "DROP USER IF EXISTS" and remove " || true"

Anyway, if removing "ERROR 1396 (HY000) at line 1: Operation DROP USER failed for 'koha_rsc'@'%'" is all you're after, this patch fixes it. "DROP USER IF EXISTS" has been available since MySQL 5.7, which is old by now.
Comment 7 Katrin Fischer 2020-01-05 10:17:27 UTC
Formatting this as a patch might be a good 'rescue' task :)
Comment 8 Rudolf Byker 2020-01-07 08:04:23 UTC
Hi Katrin, I don't understand your last message. To whom was it directed?
Comment 9 Katrin Fischer 2020-01-07 08:28:06 UTC
The attached file is not a properly formatted patch file and can't be processed with our tools - I set the "Academy" keyword the same time as the comment. It's a keyword we use for easy bugs and tasks that are good for people starting out developing on Koha.
Comment 10 Rudolf Byker 2020-01-08 08:33:44 UTC
There is more than one attachment. The one from 2013 is not a patch. My one from 2019-05-10 is a patch created with "diff". Which one are you referring to?
Comment 11 Katrin Fischer 2020-01-08 08:55:09 UTC
HI Rudolf, yours actually :) - diff doesn't create a patch file that can be used with our tools. Have a look at git format --patch and https://wiki.koha-community.org/wiki/Commit_messages. If you compare to patch files from other bugs you will see yours is missing the header line with authorship etc.
Also missing bug reference (subject line), description, test plan etc. So this needs a little more work. Please let us know if we can help - I was not sure if you were still around and just trying to encourage someone to maybe pick this up to finish. Usually in January we have a group of students tackle things, but it won't happen this year :(
Comment 12 Katrin Fischer 2020-01-08 08:55:57 UTC
... oh and status should be set to "Needs Sign-off" once the patch is fixed up and Assignee should be you, if you intent continuing work on it.
Comment 13 Katrin Fischer 2020-01-08 09:01:08 UTC
> used with our tools. Have a look at git format --patch and

git format-patch
Comment 14 Rudolf Byker 2020-01-08 09:06:26 UTC
OK, let me be your student then, and try the things you suggested. (BTW, I also noticed that my patch was reversed. :facepalm: )
Comment 15 Rudolf Byker 2020-01-08 09:30:42 UTC
Created attachment 96975 [details] [review]
Use `DROP USER IF EXISTS` and remove ` || true`.

All right, here is my attempt at a proper patch!
Comment 16 Martin Renvoize 2020-01-14 14:40:45 UTC
Sorry, I meant to get to this earlier.

I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are maintaining support for Jessie up to the end of it's LTS date (2020-06-30). I intend to drop support for that Debian version at the release of 20.05 (at which time I believe we should also drop support for it from 19.11  at 19.11.06 and 19.05 at 19.05.12).

However, even once we have dropped such support, unless I am mistaken, stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead.  As such, we'll need a clear deprecation of mysql 5.5 support outlined and publicised before we can make this change.

I am, however, pretty keen to take such an approach and drop 5.5 support with this release and so this patch is 'on hold' for now.
Comment 17 Victor Grousset/tuxayo 2020-05-05 15:14:26 UTC
For some reason comment 15 doesn't have the test plan. But it turns out the patch has a test plan. Here it is, to remove confusion and help discussing it:

Subject: [PATCH] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`.

When restoring a Koha instance, and the database user does not exist, an error is shown,
although it is prevented from stopping the script by ` || true`. This patch removes the
` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one
would expect, without confusing error messages, whether the user existed or not.

To test:
1) Assume your Koha instance is called `asdf`.
1) Export the Koha instance using `koha-dump`.
2) Remove the instance completely using `koha-remove`.
3) Import it again using `koha-restore`.
4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing.
5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
6) Remove the instance again using `koha-remove`.
7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance.
8) Import your koha instance again using `koha-restore`.
9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one.
10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
Comment 18 Victor Grousset/tuxayo 2020-05-05 15:19:59 UTC
> 2) Remove the instance completely using `koha-remove`.

I'm having an issue here. Maybe it's my dev environment.

root@4dfd0fcc251b:kohadevbox$ koha-remove kohadev
Removing Koha instance kohadev
[ ok ] Stopping Plack daemon for kohadev:.
userdel: user kohadev-koha is currently used by process 1005
/usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. Exiting.




Using koha-disable doesn't help. There are still starman and apache processes running that are from the unix user of the instance.

Could there be anything more to do before removing the instance?
Comment 19 Victor Grousset/tuxayo 2020-05-05 15:21:46 UTC
(In reply to Martin Renvoize from comment #16)
> I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are
> maintaining support for Jessie up to the end of it's LTS date (2020-06-30).
> I intend to drop support for that Debian version at the release of 20.05 (at
> which time I believe we should also drop support for it from 19.11  at
> 19.11.06 and 19.05 at 19.05.12).
> 
> However, even once we have dropped such support, unless I am mistaken,
> stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead. 
> As such, we'll need a clear deprecation of mysql 5.5 support outlined and
> publicised before we can make this change.
> 
> I am, however, pretty keen to take such an approach and drop 5.5 support
> with this release and so this patch is 'on hold' for now.


Is there an existing place like in the wiki that contains these info? Nothing more than you said, the current situation and future plans.
Comment 20 David Cook 2020-05-06 04:07:06 UTC
(In reply to Martin Renvoize from comment #16)
> 
> I am, however, pretty keen to take such an approach and drop 5.5 support
> with this release and so this patch is 'on hold' for now.

For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we already have mixed support for 5.5.

I ended up removing "IF EXISTS" and updating koha-remove to use --force so that it could survive fatal errors.
Comment 21 Rudolf Byker 2020-05-06 08:26:37 UTC
(In reply to Victor Grousset/tuxayo from comment #18)
> > 2) Remove the instance completely using `koha-remove`.
> 
> I'm having an issue here. Maybe it's my dev environment.
> 
> root@4dfd0fcc251b:kohadevbox$ koha-remove kohadev
> Removing Koha instance kohadev
> [ ok ] Stopping Plack daemon for kohadev:.
> userdel: user kohadev-koha is currently used by process 1005
> /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8.
> Exiting.
> 
> 
> 
> 
> Using koha-disable doesn't help. There are still starman and apache
> processes running that are from the unix user of the instance.
> 
> Could there be anything more to do before removing the instance?

Probably your dev environment. I've never had this issue, and I'm using my own patch. Did you check which process is still using the user using `ps -u kohadev-koha`?

By the way: Is the test plan supposed to be in the commit message, in a comment, or both?
Comment 22 Rudolf Byker 2020-05-06 08:34:22 UTC
(In reply to David Cook from comment #20)
> (In reply to Martin Renvoize from comment #16)
> > 
> > I am, however, pretty keen to take such an approach and drop 5.5 support
> > with this release and so this patch is 'on hold' for now.
> 
> For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we
> already have mixed support for 5.5.
> 
> I ended up removing "IF EXISTS" and updating koha-remove to use --force so
> that it could survive fatal errors.

Are you talking about the `--force` option on the MySQL command line utility? That smells like technical debt. Using it is the same or worse as using `|| true`, since you will hide lots of potential errors. We only need to handle one very specific error (which is not even an error, if you consider Idempotency to be a good programming pattern for such scripts – which it is). Using a catch-all to hide all errors is almost always a Bad Idea (TM).
Comment 23 Katrin Fischer 2020-05-06 09:20:34 UTC
(In reply to Rudolf Byker from comment #21)
> By the way: Is the test plan supposed to be in the commit message, in a
> comment, or both?

It shoudl be in the commit message - in bugzilla it will then appear automatically in a comment, if you use git bz. See here:
https://wiki.koha-community.org/wiki/Commit_messages
Comment 24 David Cook 2020-05-07 00:35:46 UTC
(In reply to Rudolf Byker from comment #22)
> (In reply to David Cook from comment #20)
> > (In reply to Martin Renvoize from comment #16)
> > > 
> > > I am, however, pretty keen to take such an approach and drop 5.5 support
> > > with this release and so this patch is 'on hold' for now.
> > 
> > For what it's worth, I think koha-remove uses "DROP USER IF EXISTS", so we
> > already have mixed support for 5.5.
> > 
> > I ended up removing "IF EXISTS" and updating koha-remove to use --force so
> > that it could survive fatal errors.
> 
> Are you talking about the `--force` option on the MySQL command line
> utility? That smells like technical debt. Using it is the same or worse as
> using `|| true`, since you will hide lots of potential errors. We only need
> to handle one very specific error (which is not even an error, if you
> consider Idempotency to be a good programming pattern for such scripts –
> which it is). Using a catch-all to hide all errors is almost always a Bad
> Idea (TM).

Yes, the --force option for the MySQL cli utility.

I am only suggesting it as a temporary measure. Versions of Debian we support are using MySQL server 5.5, but some Koha Debian utilities (the ones that are aiming at being idempotent) fail on those Debian versions. Either we remove the idempotency targeting newer versions of MySQL server, or we do a measure like --force (which will still warn so it doesn't hide the errors it just doesn't die on them). We really need to do one or the other. Or drop support for older versions of Debian I suppose.
Comment 25 Rudolf Byker 2020-05-07 06:59:21 UTC
OK, if you really need to support older versions, let's do it cleanly. Here is a way to check if the user exists:

EXISTS=$(mysql -uroot -p -s -e "select count(*) from mysql.user where user = 'asdf'" | tail -n 1)
if [[ $EXISTS -gt 0 ]]
then
  echo "Now delete the user."
fi
Comment 26 Victor Grousset/tuxayo 2020-05-07 21:54:31 UTC
(In reply to Katrin Fischer from comment #23)
> in bugzilla it will then appear
> automatically in a comment, if you use git bz.

@Rudolf that's the cause I think. Your commit seems fine but if you don't use git-bz[1] you have to manually copy the commit message in the comment created when uploading an attachment.

[1] https://wiki.koha-community.org/wiki/Git_bz_configuration
Comment 27 Victor Grousset/tuxayo 2020-05-07 22:01:48 UTC
(In reply to Rudolf Byker from comment #21)
> Probably your dev environment. I've never had this issue, and I'm using my
> own patch. Did you check which process is still using the user using `ps -u
> kohadev-koha`?

starman and apache
Comment 28 David Cook 2020-05-07 23:56:11 UTC
(In reply to Rudolf Byker from comment #25)
> OK, if you really need to support older versions, let's do it cleanly. Here
> is a way to check if the user exists:
> 
> EXISTS=$(mysql -uroot -p -s -e "select count(*) from mysql.user where user =
> 'asdf'" | tail -n 1)
> if [[ $EXISTS -gt 0 ]]
> then
>   echo "Now delete the user."
> fi

Sounds like a plan to me, although I think it would be better to create a new function in koha-functions.sh, since we need to drop the user in koha-restore as well as koha-remove. I think we batch too many SQL statements together for these CLI utilities. 

But, I'm not planning on doing that work anytime soon. So don't feel obligated to listen to that.

Ultimately what I was trying to say before was that koha-remove already uses the DROP USER IF EXISTS syntax, so it seems fair to use it in koha-restore too, even if it causes problems for people using older DB versions, since that problem already exists in the master codebase.
Comment 29 David Cook 2020-05-08 00:00:17 UTC
(In reply to Rudolf Byker from comment #25)
> OK, if you really need to support older versions, let's do it cleanly. 

By the way, Martin Renvoize is the release manager and Katrin Fischer is the QA manager, so ultimately they're the ones who decide what gets pushed. I'm just a fellow contributor. Don't feel like you need to listen to me. They're the ones to listen to.

You may also find contributing easier if you're using git-bz (https://wiki.koha-community.org/wiki/Git_bz_configuration) or even the community's Docker testing images which comes bundled with git-bz (https://gitlab.com/koha-community/koha-testing-docker).

I notice you submitted a patch last year as well. Good to see you back here!
Comment 30 Victor Grousset/tuxayo 2020-05-08 14:28:36 UTC
> You may also find contributing easier if you're using git-bz

+1

> Docker testing images which comes bundled with git-bz

+1


@Rudolf come to IRC if you any question setting those up.
Comment 31 Victor Grousset/tuxayo 2020-05-08 17:59:27 UTC
(In reply to Rudolf Byker from comment #21)
> Probably your dev environment. I've never had this issue, and I'm using my
> own patch.

Can you confirm that one should be able to do the following
- spin a fresh dev env (who provides the command koha-remove) on master, without any patch
- koha-remove MY_INSTANCE
- and not have the following error:

userdel: user kohadev-koha is currently used by process 864
/usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. Exiting.


Here are the running process before koha-remove
https://copycat.drycat.fr/?c9032e785372c590#4Kc3ir7uygVpWD67jz2huvguZbgWKy5QkYTmhYVPYu1z

after

https://copycat.drycat.fr/?e42e97345c1dc5c4#tgEsEof33SSxREncJRmtq6XV93QnpcBtmhynUkcYrBL
Comment 32 Victor Grousset/tuxayo 2020-05-14 21:32:09 UTC
== 1 ==
I might had issues related to bug 25482 because my dev env was Debian 10 (koha-testing-docker ku-md10)
Which could explain cause koha-plack to not work which would cause issue to stop the instance.

Actually only Apache processes are still there when doing koha-remove.
With koha-disable, no errors and starman and Apache processes are still there.

== 2 ==
So I tried with Debian 9 + MySQL 5.5 (koha-testing-docker ku)
And now I get this issue.

> root@b92acf73a243:kohadevbox$ koha-remove kohadev
> Removing Koha instance kohadev
> ERROR 1064 (42000) at line 1: You have an error in your SQL syntax;
> check the manual that corresponds to your MySQL server version for
> the right syntax to use near 'IF EXISTS `koha_kohadev`@`%`' at line 1

== 3 ==
So I tried with Debian 9 + MariaDB (from it's repos) (koha-testing-docker ku-md9)
Same error as with Debian 10. Processes still here.

> userdel: user kohadev-koha is currently used by process 839
> /usr/sbin/deluser: `/usr/sbin/userdel kohadev-koha' returned error code 8. > Exiting.
Comment 33 Victor Grousset/tuxayo 2020-05-14 21:43:57 UTC
(In reply to Martin Renvoize from comment #16)
> I'm afraid we cannot yet use the 'IF EXISTS' syntax here as we are
> maintaining support for Jessie up to the end of it's LTS date (2020-06-30).
> I intend to drop support for that Debian version at the release of 20.05 (at
> which time I believe we should also drop support for it from 19.11  at
> 19.11.06 and 19.05 at 19.05.12).
> 
> However, even once we have dropped such support, unless I am mistaken,
> stretch is still stuck at MySQL 5.5 (though it recommends MariaDB instead. 
> As such, we'll need a clear deprecation of mysql 5.5 support outlined and
> publicised before we can make this change.

My second test of the above comment seems to show that MySQL 5.5 is already not supported in our case.
I'm just spinning up a Koha with MySQL 5.5 and run koha-remove then it fails on a syntax error mentioning IF EXISTS.

So, should we still avoid IF EXISTS?(I don't have the big picture)
In this case it doesn't look like it will break anything new for anyone. "Just" breaking a second thing when using koha-remove.
Comment 34 Martin Renvoize 2021-11-11 15:29:22 UTC
Right.. time has moved on... I'm thinking we can accept this patch now as-is?

Promoting to the QA queue
Comment 35 Joonas Kylmälä 2021-11-13 13:58:11 UTC
(In reply to Rudolf Byker from comment #15)
> Created attachment 96975 [details] [review] [review]
> Use `DROP USER IF EXISTS` and remove ` || true`.
> 
> All right, here is my attempt at a proper patch!

The author name is missing, it just has your username I guess: "dolf".
Comment 36 Rudolf Byker 2021-11-15 12:55:34 UTC
(In reply to Joonas Kylmälä from comment #35)
> (In reply to Rudolf Byker from comment #15)
> > Created attachment 96975 [details] [review] [review] [review]
> > Use `DROP USER IF EXISTS` and remove ` || true`.
> > 
> > All right, here is my attempt at a proper patch!
> 
> The author name is missing, it just has your username I guess: "dolf".

I don't understand what you mean here.
Comment 37 Katrin Fischer 2021-11-18 13:37:01 UTC
Hi,

it's about the patch file, see:

From 06fd450e1f9749e8fa1d6455e32f597ac0ef85b4 Mon Sep 17 00:00:00 2001
From: dolf <rudolfbyker@gmail.com>
Date: Wed, 8 Jan 2020 11:20:02 +0200
Subject: [PATCH] Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`.

This means that you will show up as "dolf" in some places. We do have mappings to make things show nicer/full names, but always easier to make sure it has the information you want to display.

I think you might want to run:
git config --global user.name "firstname lastname"

I believe if you don't want to resubmit, we could also fix this as part of QA, but you should tell us what it should read like. Rudolf Byker?
Comment 38 Rudolf Byker 2021-11-18 14:19:01 UTC
> git config --global user.name "firstname lastname"

I'll keep this in mind for the future.

> we could also fix this as part of QA

That would be great, thanks.

> but you should tell us what it should read like. Rudolf Byker?

Exactly :)
Comment 39 Katrin Fischer 2021-11-19 13:42:46 UTC
Created attachment 127865 [details] [review]
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`.

When restoring a Koha instance, and the database user does not exist, an error is shown,
although it is prevented from stopping the script by ` || true`. This patch removes the
` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one
would expect, without confusing error messages, whether the user existed or not.

To test:
1) Assume your Koha instance is called `asdf`.
1) Export the Koha instance using `koha-dump`.
2) Remove the instance completely using `koha-remove`.
3) Import it again using `koha-restore`.
4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing.
5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
6) Remove the instance again using `koha-remove`.
7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance.
8) Import your koha instance again using `koha-restore`.
9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one.
10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name.

Sponsored-by: Reformational Study Centre
Comment 40 Katrin Fischer 2021-11-19 13:43:43 UTC
Martin, are we missing your sign-off line here? I think you were the one to switch to signed-off.

Fixed the author line.
Comment 41 Katrin Fischer 2022-03-14 20:44:24 UTC
(In reply to Katrin Fischer from comment #40)
> Martin, are we missing your sign-off line here? I think you were the one to
> switch to signed-off.
> 
> Fixed the author line.

Ping?
Comment 42 Martin Renvoize 2022-04-11 11:45:16 UTC
Yes, it should have my SO line.. oops
Comment 43 Martin Renvoize 2022-04-25 11:53:24 UTC
Created attachment 133758 [details] [review]
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`.

When restoring a Koha instance, and the database user does not exist, an error is shown,
although it is prevented from stopping the script by ` || true`. This patch removes the
` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one
would expect, without confusing error messages, whether the user existed or not.

To test:
1) Assume your Koha instance is called `asdf`.
1) Export the Koha instance using `koha-dump`.
2) Remove the instance completely using `koha-remove`.
3) Import it again using `koha-restore`.
4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing.
5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
6) Remove the instance again using `koha-remove`.
7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance.
8) Import your koha instance again using `koha-restore`.
9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one.
10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name.

Sponsored-by: Reformational Study Centre
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Marcel de Rooy 2022-04-29 09:20:53 UTC
Created attachment 134353 [details] [review]
Bug 10517: Use `DROP USER IF EXISTS` and remove ` || true`.

When restoring a Koha instance, and the database user does not exist, an error is shown,
although it is prevented from stopping the script by ` || true`. This patch removes the
` || true` guard, and rather uses `DROP USER IF EXISTS` which does exactly what one
would expect, without confusing error messages, whether the user existed or not.

To test:
1) Assume your Koha instance is called `asdf`.
1) Export the Koha instance using `koha-dump`.
2) Remove the instance completely using `koha-remove`.
3) Import it again using `koha-restore`.
4) No error should be shown, and the import should work. There was NO user to drop, therefore `DROP USER IF EXISTS` did nothing.
5) A database user called `koha_asdf` should exist, and have all privileges on the database of the same name.
6) Remove the instance again using `koha-remove`.
7) Now create a database user in MySQL called `koha_instance` where instance is the name of your dumped koha instance.
8) Import your koha instance again using `koha-restore`.
9) Again, no error should be shown, and the import should work. There WAS a user to drop, therefore `DROP USER IF EXISTS` dropped the user before creating a new one.
10) Again, a database user called `koha_asdf` should exist, and have all privileges on the database of the same name.

Sponsored-by: Reformational Study Centre
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2022-04-29 09:20:58 UTC
Created attachment 134354 [details] [review]
Bug 10517: (QA follow-up) No need to remove second true

This is just theoretical, but by removing the second true you
could end the script on another error.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Fridolin Somers 2022-05-03 21:27:08 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 47 Kyle M Hall 2022-05-13 16:11:39 UTC
Pushed to 21.11.x for 21.11.06
Comment 48 Victor Grousset/tuxayo 2022-06-23 02:07:16 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.