search

YunoHost-Apps / wallabag2_ynh

Wallabag v2 package for YunoHost
https://www.wallabag.org/
GNU Affero General Public License v3.0
62 stars 14 forks source link

Upgrade old DB scheme (migration to 2.4.x for pre-2018 installs) #129

Closed lapineige closed 7 months ago

lapineige commented 2 years ago

Problem

Pre-2018 installation had a bad database scheme. This should fix it partly, and allow them to upgrade to wallabag 2.4.x. See : https://github.com/YunoHost-Apps/wallabag2_ynh/pull/125

Solution

PR Status

Automatic tests

Automatic tests can be triggered on https://ci-apps-dev.yunohost.org/ after creating the PR, by commenting "!testme", "!gogogadgetoci" or "By the power of systemd, I invoke The Great App CI to test this Pull Request!". (N.B. : for this to work you need to be a member of the Yunohost-Apps organization)

lapineige commented 2 years ago

Now here is the question : who will be able to test this ? :sweat_smile:

I kept my old installation backup to test this, but it will be a pain to test it…

Is there anyone here affected by this bug (see https://github.com/wallabag/wallabag/issues/5233 https://github.com/YunoHost-Apps/wallabag2_ynh/pull/101#issuecomment-824594592) who would be willing to test it ? To check if you are affected, you can either:

For normal upgrades, I will use the CI to check if it works well.

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:sunflower: Test Badge

lapineige commented 2 years ago

CI is failing with :

191504 WARNING ./upgrade: line 164: user_id: command not found 191520 WARNING ./upgrade: line 164: user: command not found 192061 WARNING ERROR 1091 (42000) at line 1: Can't DROP FOREIGN KEY FK_D247A21BA76ED395; check that it exists 192849 WARNING [Error] Upgrade failed. 379247 WARNING The app was restored to the way it was before the failed upgrade. 380258 ERROR Could not upgrade wallabag2: An error occurred inside the app upgrade script

So @nicofrand as a follow-up from your comment here https://github.com/YunoHost-Apps/wallabag2_ynh/pull/125#issuecomment-1046257013 : yes, we need to check if the key exist first, because it would fails if it does not exist.

I may use the IF EXISTS command that @Kdecherf provided here https://github.com/wallabag/wallabag/issues/5233#issuecomment-1041871930 but I don't know how to use it in Yunohost. Is that something to do in the SQL command ? Can someone help me with the synthax ? I want to run the whole block of command only if that foreign key exists (and if not, nothing is done).

nicofrand commented 2 years ago

Yes, using IF EXISTS should be enough.

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:carousel_horse: Test Badge

lapineige commented 2 years ago

It fails with :

WARNING ERROR 1064 (42000) at line 1: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'FK_D247A21BA76ED395' at line 1

I guess MariaDB version is too old, as @Kdecherf suggested.

So… what do we do ? :sweat_smile: ping @YunoHost-Apps/apps-group, do you have an idea how to execute an SQL command only if a particular ForeignKey exists in the database ? Or at least to handle (and ignore ?) the exception when it fails ? (Context: apparently we can't use the SQL syntax IF EXIST because MariaDB version is too old)

nicofrand commented 2 years ago

MariaDB version should be OK: mariadb Ver 15.1 Distrib 10.3.31-MariaDB, for debian-linux-gnu (x86_64) using readline 5.2

nicofrand commented 2 years ago

We need to select the right database though (see next code comment, but I did not test it).

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:rocket: Test Badge

lapineige commented 2 years ago

It fails :(

453047 INFO INFO - [##############++....] > Migrating old (pre-2018) database scheme... 453047 INFO DEBUG - ++ user_id 453048 INFO WARNING - ./upgrade: line 164: user_id: command not found 453049 INFO WARNING - ./upgrade: line 164: user: command not found 453050 INFO DEBUG - ++ user 453051 INFO DEBUG - ++ id 453052 INFO DEBUG - + ynh_exec_warn_less ynh_mysql_execute_as_root '--sql=START TRANSACTION; ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY IF EXISTS FK_D247A21BA76ED395; ALTER TABLE oauth2_access_tokens ADD CONSTRAINT FK_368A4209A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY IF EXISTS FK_D247A21B19EB6921; ALTER TABLE oauth2_access_tokens ADD CONSTRAINT FK_368A420919EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE oauth2_auth_codes DROP FOREIGN KEY IF EXISTS FK_A018A10DA76ED395; ALTER TABLE oauth2_auth_codes ADD CONSTRAINT FK_EE52E3FAA76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_clients DROP FOREIGN KEY IF EXISTS FK_F9D02AE6A76ED395; ALTER TABLE oauth2_clients ADD CONSTRAINT FK_635D765EA76ED395 FOREIGN KEY () REFERENCES (uid=0(root) gid=0(root) groups=0(root)); ALTER TABLE oauth2_refresh_tokens DROP FOREIGN KEY IF EXISTS FK_D394478CA76ED395; ALTER TABLE oauth2_refresh_tokens ADD CONSTRAINT FK_20C9FB24A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE config DROP FOREIGN KEY IF EXISTS FK_D48A2F7CA76ED395; ALTER TABLE config ADD CONSTRAINT FK_87E64C53A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE entry DROP FOREIGN KEY IF EXISTS FK_2B219D70A76ED395; ALTER TABLE entry ADD CONSTRAINT FK_F4D18282A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_auth_codes DROP FOREIGN KEY IF EXISTS FK_A018A10D19EB6921; ALTER TABLE oauth2_auth_codes ADD CONSTRAINT FK_EE52E3FA19EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE oauth2_refresh_tokens DROP FOREIGN KEY IF EXISTS FK_D394478C19EB6921; ALTER TABLE oauth2_refresh_tokens ADD CONSTRAINT FK_20C9FB2419EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE tagging_rule DROP FOREIGN KEY IF EXISTS FK_1AF95E7824DB0683; ALTER TABLE tagging_rule ADD CONSTRAINT FK_2D9B3C5424DB0683 FOREIGN KEY (config_id) REFERENCES config (id); COMMIT;' --database=wallabag2 453053 INFO DEBUG - + [[ 3 -eq 1 ]] 453053 INFO DEBUG - + ynh_mysql_execute_as_root '--sql=START TRANSACTION; ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY IF EXISTS FK_D247A21BA76ED395; ALTER TABLE oauth2_access_tokens ADD CONSTRAINT FK_368A4209A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_access_tokens DROP FOREIGN KEY IF EXISTS FK_D247A21B19EB6921; ALTER TABLE oauth2_access_tokens ADD CONSTRAINT FK_368A420919EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE oauth2_auth_codes DROP FOREIGN KEY IF EXISTS FK_A018A10DA76ED395; ALTER TABLE oauth2_auth_codes ADD CONSTRAINT FK_EE52E3FAA76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_clients DROP FOREIGN KEY IF EXISTS FK_F9D02AE6A76ED395; ALTER TABLE oauth2_clients ADD CONSTRAINT FK_635D765EA76ED395 FOREIGN KEY () REFERENCES (uid=0(root) gid=0(root) groups=0(root)); ALTER TABLE oauth2_refresh_tokens DROP FOREIGN KEY IF EXISTS FK_D394478CA76ED395; ALTER TABLE oauth2_refresh_tokens ADD CONSTRAINT FK_20C9FB24A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE config DROP FOREIGN KEY IF EXISTS FK_D48A2F7CA76ED395; ALTER TABLE config ADD CONSTRAINT FK_87E64C53A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE entry DROP FOREIGN KEY IF EXISTS FK_2B219D70A76ED395; ALTER TABLE entry ADD CONSTRAINT FK_F4D18282A76ED395 FOREIGN KEY (user_id) REFERENCES user (id); ALTER TABLE oauth2_auth_codes DROP FOREIGN KEY IF EXISTS FK_A018A10D19EB6921; ALTER TABLE oauth2_auth_codes ADD CONSTRAINT FK_EE52E3FA19EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE oauth2_refresh_tokens DROP FOREIGN KEY IF EXISTS FK_D394478C19EB6921; ALTER TABLE oauth2_refresh_tokens ADD CONSTRAINT FK_20C9FB2419EB6921 FOREIGN KEY (client_id) REFERENCES oauth2_clients (id); ALTER TABLE tagging_rule DROP FOREIGN KEY IF EXISTS FK_1AF95E7824DB0683; ALTER TABLE tagging_rule ADD CONSTRAINT FK_2D9B3C5424DB0683 FOREIGN KEY (config_id) REFERENCES config (id); COMMIT;' --database=wallabag2 453054 INFO DEBUG - + database=wallabag2 453055 INFO DEBUG - + '[' -n wallabag2 ']' 453056 INFO DEBUG - + database=--database=wallabag2 453056 INFO DEBUG - + mysql -B --database=wallabag2 453057 INFO DEBUG - ERROR 1005 (HY000) at line 1: Can't create table wallabag2.oauth2_access_tokens (errno: 121 "Duplicate key on write or update") 453058 INFO DEBUG - + ynh_exit_properly

I don't get it.

Can't create table wallabag2.oauth2_access_tokens (errno: 121 "Duplicate key on write or update")

Does that mean Walabag database is not properly setup ?

453048 INFO WARNING - ./upgrade: line 164: user_id: command not found 453049 INFO WARNING - ./upgrade: line 164: user: command not found

Why does it fails ?

nicofrand commented 2 years ago

First I think it would be cleaner to use a dedicated file for the SQL and use https://yunohost.org/fr/packaging_apps_helpers#ynh_mysql_execute_file_as_root. That might avoid bash issues within the SQL.

nicofrand commented 2 years ago

I think the parentheses are interpereted as a subshell: https://unix.stackexchange.com/questions/26063/how-are-parentheses-interpreted-at-the-command-line

nicofrand commented 2 years ago

See https://github.com/nicofrand/wallabag2_ynh/commit/b0caad910a9cce9b1d4dd146ababbc7eb4ad632d

lapineige commented 2 years ago

Ok, thank you. Can you do a Pull Request pointing to this branch ? Then I can merge it.

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

May the CI gods be with you! Test Badge

nicofrand commented 2 years ago

OK, so the foreign key drop now works properly but it seems creating the new one fails because they already exist, like in ALTER TABLE oauth2_access_tokens ADD CONSTRAINT FK_368A4209A76ED395 FOREIGN KEY (user_id) REFERENCES user (id);.

Unfortunately we can't use IF EXISTS here but I wonder if we need those ADD CONSTRAINT at all in our current context won't they be created through wallabag's migration tool?

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

May the CI gods be with you! Test Badge

lapineige commented 2 years ago

There is some weird issue during the installation phase of the upgrade part…

WARNING PHP Warning:  "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /var/www/wallabag2/vendor/doctrine/orm/lib/Doctrine/ORM/UnitOfWork.php on line 2665

I missed this one too :

Error: It looks like anybody can read/enter /var/www/wallabag2, which ain't super great from a security point of view ... Config files or other files may contain secrets or information that should in most case not be world-readable. You should remove all 'others' permissions with 'chmod o-rwx', and setup appropriate, exclusive permissions to the appropriate owner/group with chmod/chown.

And the SQL stuff fails:

66131 WARNING 21:29:02 ERROR     [console] Error thrown while running command "--no-interaction --env=prod doctrine:migrations:migrate". Message: "An exception occurred while executing 'ALTER TABLE `oauth2_clients` ADD CONSTRAINT FK_635D765EA76ED395 FOREIGN KEY (user_id) REFERENCES `user` (id)':
66133 WARNING SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2_clients` (errno: 121 "Duplicate key on write or update")" ["exception" => Doctrine\DBAL\Exception\DriverException { …},"command" => "--no-interaction --env=prod doctrine:migrations:migrate","message" => """  An exception occurred while executing 'ALTER TABLE `oauth2_clients` ADD CONSTRAINT FK_635D765EA76ED395 FOREIGN KEY (user_id) REFERENCES `user` (id)':\n  \n  SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2_clients` (errno: 121 "Duplicate key on write or update")  """]
66134 WARNING In AbstractMySQLDriver.php line 128:
66137 WARNING   An exception occurred while executing 'ALTER TABLE `oauth2_clients` ADD CON
66138 WARNING   STRAINT FK_635D765EA76ED395 FOREIGN KEY (user_id) REFERENCES `user` (id)':
66140 WARNING   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
66141 WARNING   _clients` (errno: 121 "Duplicate key on write or update")
66143 WARNING In Exception.php line 18:
66144 WARNING   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
66145 WARNING   _clients` (errno: 121 "Duplicate key on write or update")
66148 WARNING In PDOConnection.php line 141:
66150 WARNING   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
66151 WARNING   _clients` (errno: 121 "Duplicate key on write or update")

and:


125584 WARNING Here's an extract of the logs before the crash. It might help debugging the error:
125587 INFO DEBUG -
125587 INFO WARNING -   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
125588 INFO WARNING -   _clients` (errno: 121 "Duplicate key on write or update")
125588 INFO DEBUG -
125588 INFO DEBUG -
125589 INFO WARNING - In Exception.php line 18:
125589 INFO DEBUG -
125589 INFO WARNING -   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
125589 INFO WARNING -   _clients` (errno: 121 "Duplicate key on write or update")
125589 INFO DEBUG -
125589 INFO DEBUG -
125590 INFO WARNING - In PDOConnection.php line 141:
125590 INFO DEBUG -
125590 INFO WARNING -   SQLSTATE[HY000]: General error: 1005 Can't create table `wallabag2`.`oauth2
125590 INFO WARNING -   _clients` (errno: 121 "Duplicate key on write or update")
125590 INFO DEBUG -
125591 INFO DEBUG -
125591 INFO WARNING - doctrine:migrations:migrate [--write-sql [WRITE-SQL]] [--dry-run] [--query-time] [--allow-no-migration] [--configuration [CONFIGURATION]] [--db-configuration [DB-CONFIGURATION]] [--db DB] [--em EM] [--shard SHARD] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command> [<version>]
125591 INFO DEBUG -
125592 INFO DEBUG - + ynh_exit_properly
nicofrand commented 2 years ago

Most of those warnings are pre-existing and from wallabag itself.

Regarding the SQL, let's wallabag migration handle the key creation and remove all the lines after line 13. Can you do it and try please?

lapineige commented 2 years ago

!testme

yunohost-bot commented 2 years ago

:rocket: Test Badge

lapineige commented 2 years ago

I have to admit that I am lost about what that part of the SQL was supposed to do 😅

But I removed it, we will see if that works.

nicofrand commented 2 years ago

So, everything is OK for the CI, and we can now test it, right?

lapineige commented 2 years ago

Yes ! Thank you very much for your contribution, I am not sure I would have been able to do it.

nicofrand commented 2 years ago

Now that I take a new look at it I just made your remove the specific foreign keys that created the issue in https://github.com/wallabag/wallabag/issues/5233.

So it will fail again, I'll take more time to think about it.

lapineige commented 2 years ago

Well, I don't understand 😅, but if you find a solution I will let you handle it your way :)

pepeciseaux commented 2 years ago

Now here is the question : who will be able to test this ? sweat_smile

(…) To check if you are affected, you can either:

(…)

FYI I just tried to migrate my Wallabag app to testing branch, as explained in the readme. Command used: sudo yunohost app upgrade wallabag2 -u https://github.com/YunoHost-Apps/wallabag2_ynh/tree/testing

Migration failed, and it reverted to my previous standard version. Here are the results relating to the two points you mentionned:

My reverted standard version seems OK.

Can I do sth else to help?

lapineige commented 2 years ago

WARNING - In AbstractMySQLDriver.php line 128: DEBUG - WARNING - An exception occurred while executing 'ALTER TABLE entry ADD given_url LONGTEXT DEFAULT NUL WARNING - L, ADD hashed_given_url TINYTEXT DEFAULT NULL': DEBUG - WARNING - SQLSTATE[42000]: Syntax error or access violation: 1118 Row size too large. The maximum row WARNING - size for the used table type, not counting BLOBs, is 8126. This includes storage overhead, WARNING - check the manual. You have to change some columns to TEXT or BLOBs Migration 20190601125843 failed during Execution. Error An exception occurred while executing 'ALTER TABLE entry ADD given_url LONGTEXT DEFAULT NULL, ADD hashed_given_url TINYTEXT DEFAULT NULL':

That's a new one :thinking:

Can you try with this branch ? Use https://github.com/YunoHost-Apps/wallabag2_ynh/tree/2.4.3_OldDatabaseMigration instead of the testing branch URL.

kr-cr commented 2 years ago

It looks similar to what I had: https://github.com/YunoHost-Apps/wallabag2_ynh/pull/106#issuecomment-869158920

I tried with the branch you suggested and the upgrade went smoothly!

pepeciseaux commented 2 years ago

Can you try with this branch ? Use https://github.com/YunoHost-Apps/wallabag2_ynh/tree/2.4.3_OldDatabaseMigration instead of the testing branch URL.

Indeed, it seems that it worked for me too! 👍

Hope it will help to provide a "real" version, and thx for the work 🙏

(Sorry for the delay. I am not very comfortable with the idea of trying multiple non-stable branches on my machine. Anyway, I’ve done it and it gave us good news \o/)

lapineige commented 2 years ago

@nicofrand Coming back after a while, I'm a bit lost between this PR and yours https://github.com/YunoHost-Apps/wallabag2_ynh/pull/133

Can you remind me which one is supposed to do what ? :sweat_smile:

nicofrand commented 2 years ago

Mine can be used as a migration step (upgrade to my branch to fix the DB scheme then to the regular branch). We did not merge mine (neither this one) due to the CI failing.

Salamandar commented 7 months ago

@nicofrand @lapineige I rebased this branch onto packagingv2, and also added a if version < 2.4 on the code. Do you think the state is OK to merge now ?