Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle data migration for the tables renamed after v2.2.0 #340

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

floss4good
Copy link

This migration will copy data from ocsms_conversation_read_states and ocsms_sendmessage_queue legacy tables to ocsms_conv_r_states and ocsms_sendmess_queue, respectively, new tables after which will perform the following DB schema changes:

  • Drop ocsms_conversation_read_states legacy table (if data successfully copied to new table);
  • Drop ocsms_sendmessage_queue legacy table (if found);
  • Rename (DROP + ADD) some indexes from ocsms_smsdatas table:
    • smsdata_user_mailboxsmsdata_user_mbox
    • smsdata_user_mailbox_datesmsdata_user_mbox_date
    • smsdata_user_mailbox_addresssmsdata_user_mbox_addr
    • smsdata_user_mailbox_address_datesmsdata_user_mbox_addr_date

With this it will be possible to update the DB for already installed versions.

I only tested it with Nextcloud 20.0.12 by manually running the occ migrations:migrate ocsms command and also with a fresh install of the Phone Sync application. Not tested with Nextcloud 21 or newer since the app is currently configured to work between versions 18 and 20.

Since this is my first contribution to the Nextcloud ecosystem – therefore also my first for this project – please find below some remarks:

  1. I named my migration class/file Version020300Date20210926000100 assuming that the next version to be released will be 2.3 however, it is not clear to me what version should be used (the one from which you are migrating or the one to which you are migrating). If someone can clarify this aspect, I can just rename the class/file, if needed.

  2. I noticed that migration Version020109Date20201216203338 was altered although it was already included in version 2.2.0 (see commit 224382c) in order to rename the tables and indexes – effective in case of new installations – that are also subject of current migration. From my understanding this is a bad practice and these changes should have been done in a new migration. Check the second Note from documentation:

    Since Nextcloud stores, which migrations have been executed already you must not “update” migrations. The recommendation is to keep them untouched as long as possible. You should only adjust it to make sure it still executes, but additional changes to the database should be done in a new migration.

    If you want I can revert the changes and adjust my pull request accordingly.

  3. One of the renamed tables – ocsms_sendmessage_queue – seems to not be used at all. From my point of view, if someone (@nerzhul, most probably) can confirm this, it would be a good moment to drop this table and remove the useless data migration part from current migration file. However, I will open a distinct issue for this.

…s and for renaming some indexes.

Refs: 224382c, 94a7175
Signed-off-by: Bogdan Popescu <floss4good@disroot.org>
@floss4good floss4good mentioned this pull request Sep 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the branch nextcloud:master October 11, 2021 09:57
@nerzhul
Copy link
Collaborator

nerzhul commented Jan 10, 2022

hello @floss4good thanks for your contribution. Yeah it was for future, but android permissions and google play store removed us then i stopped the project after battling for months with google play bots to try to re-enable the app...
App has nothing to send messages. I should rewrite app at a point because i migrated part of the code from java to golang (which works) but make it harder to maintain, while it's more performant

@nerzhul
Copy link
Collaborator

nerzhul commented Jan 10, 2022

it may also be harder to me to test it as now i'm using signal mainly on my phone and not the original SMS backed backend

@erdoukki
Copy link

Confirm working #329 (comment)
On FP4 with Android 11 with APK from FDROID (disabling INCOMPATIBILITY warning in advanced prefs)
On NextCloud 23.0.3 (on a docker)

THANKS !

@mnalis
Copy link

mnalis commented Mar 21, 2023

As I've had previous data I didn't want to lose, and NextCloud 25.0.4 does not seem to have occ migrations:migrate command anymore; I've done the migrations manually on my MySQL (MariaDB):

rename table oc_ocsms_conversation_read_states to oc_ocsms_conv_r_states;
alter table oc_ocsms_conv_r_states modify `int_date` bigint(32) NOT NULL;

rename table oc_ocsms_sendmessage_queue to oc_ocsms_sendmess_queue;
alter table oc_ocsms_sendmess_queue modify `id` bigint(10) NOT NULL AUTO_INCREMENT;

alter table oc_ocsms_smsdatas modify `id` bigint(10) NOT NULL AUTO_INCREMENT,
  modify `sms_id` bigint(5) NOT NULL, modify `sms_date` bigint(10) NOT NULL,
  modify `sms_mailbox` smallint(1) NOT NULL, modify `sms_type` smallint(1) NOT NULL;
alter table oc_ocsms_smsdatas drop index smsdata_user_mailbox, 
  drop index smsdata_user_mailbox_date, drop index smsdata_user_mailbox_address, 
  drop index smsdata_user_mailbox_address_date, drop index smsdata_user_smsid;
alter table oc_ocsms_smsdatas add index  `smsdata_user_mbox` (`user_id`,`sms_mailbox`),
  add index `smsdata_user_smsid` (`user_id`, `sms_id`),
  add index `smsdata_user_mbox_date` (`user_id`, `sms_mailbox`, `sms_date`),
  add index `smsdata_user_mbox_addr` (`user_id`, `sms_mailbox`, `sms_address`),
  add index `smsdata_user_mbox_addr_date` (`user_id`, `sms_mailbox`, `sms_address`, `sms_date`);

Doing that and fixing CSS as indicated in #347 (comment) made ocsms work for me again (until next time...)

@floss4good
Copy link
Author

@mnalis, since Nextcloud version 22 the migration:* commands are only available when debug is enabled.
#See Critical changes for developers & admins for Nextcloud 22 #26407

Anyway, I should probably make some time and rework this PR and hopefully someone could review it.

@floss4good floss4good marked this pull request as draft February 11, 2024 14:34
@floss4good
Copy link
Author

Hello @nerzhul,

I would like to update/rework this PR but this time I also want to include the cleanup mentioned in #341 (because keeping old or unused tables will just make the code harder to maintain).
Are you OK with this approach or you prefer a separate PR for #341 cleanup?

@mnalis
Copy link

mnalis commented Jul 27, 2024

@floss4good do you perhaps have it working in NextCloud 29 (#356)?
Could you help fixing it ?

@floss4good
Copy link
Author

@mnalis I'm still on an older Nextcloud version (mainly because I still use ocsms) and I've been postponing the work on this for ~2 months already because of other priorities; so I can't promise anything right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants