Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Add capability to register database migration from the application/module layer - Closes #3197 #3701

Merged
merged 23 commits into from
May 28, 2019

Conversation

lsilvs
Copy link
Contributor

@lsilvs lsilvs commented May 22, 2019

What was the problem?

All migrations were being handled by Chain Module and there was no way to run application/modules specific migrations.

How did I fix it?

Migrations logic was moved to Controller and now it has its own database table called internal_migrations to isolate it from app/modules migrations and allow updating database/code together.

registerMigrations was introduced in Application class in order to pass all registered migrations during Controller loading.

Chain-specific migrations was moved to ChainModule class.
Network-specific migrations was moved to NetworkModule class.
Migration-specific migrations was moved to Framework.

Because 20160723182900_create_schema.sql file had migrations belonging to migrations, chain and network, it was split into 3 files and another migration (20190521000002_migrate_migrations_table.sql) was created to take care of the split.

See bellow current list of migrations and which module it belongs to:

       id       |                                name                          module
----------------+----------------------------------------------------+--------------------------
 20160723182900 | create_schema                                      | [framework|chain|network]
 20160723182901 | create_views                                       | [chain]
 20160724114255 | create_memory_tables                               | [chain]
 20160724132825 | upcase_memory_table_addresses                      | [chain]
 20160725173858 | alter_mem_accounts_columns                         | [chain]
 20160908120022 | add_virgin_column_to_mem_accounts                  | [chain]
 20160908215531 | protect_mem_accounts_columns                       | [chain]
 20161007153817 | create_memory_table_indexes                        | [chain]
 20161016133824 | add_broadhash_column_to_peers                      | [network]
 20170113181857 | add_constraints_to_peers                           | [network]
 20170124071600 | recreate_trs_list_view                             | [chain]
 20170319001337 | create_indexes                                     | [chain]
 20170321001337 | create_rounds_fees_table                           | [chain]
 20170328001337 | recreate_trs_list_view                             | [chain]
 20170403001337 | calculate_blocks_rewards                           | [chain]
 20170408001337 | create_index                                       | [chain]
 20170422001337 | recreate_calculate_blocks_rewards                  | [chain]
 20170428001337 | recreate_trs_lisk_view                             | [chain]
 20170614155841 | unique_delegates_constraint                        | [chain]
 20170921105500 | recreate_revert_mem_account_trigger                | [chain]
 20171207000001 | remove_peers_dapp_table                            | [network]
 20171207000006 | create_transfer_trs_table                          | [chain]
 20171207000007 | recreate_full_block_list_view                      | [chain]
 20171227155620 | rename_port_to_ws_port                             | [network]
 20180205000000 | underscore_patch                                   | [framework]
 20180205000001 | drop_rewards_related_functions                     | [chain]
 20180205000002 | add_height_column_to_peers                         | [network]
 20180205000003 | create_rounds_rewards_table                        | [chain]
 20180205000004 | apply_round_exception_mainnet                      | [chain]
 20180214164336 | change_case_for_blocks_columns_in_mem_accounts     | [chain]
 20180227120000 | enforce_uppercase_trs_recipienId                   | [chain]
 20180327170000 | support_long_peer_version_numbers                  | [network]
 20180419001337 | alter_mem_blockid_column                           | [chain]
 20180423001337 | remove_virgin_column                               | [chain]
 20180503114500 | add_row_id_to_trs_list_view                        | [chain]
 20180814001337 | add_indexes_for_trs_table                          | [chain]
 20180901001337 | recreate_full_blocks_list_view                     | [chain]
 20180903001337 | rename_rate_to_rank_in_delegates                   | [chain]
 20181023001337 | change_round_type_in_mem_rounds                    | [chain]
 20181106000001 | remove_duplicate_..._constraint_in_dapps           | [chain]
 20181106000002 | remove_duplicate_..._constraint_in_votes           | [chain]
 20181106000003 | remove_duplicate_..._constraint_in_transfer        | [chain]
 20181106000004 | remove_duplicate_..._constraint_in_intransfer      | [chain]
 20181106000005 | remove_duplicate_..._constraint_in_multisignatures | [chain]
 20181106000006 | change_wsport_type_in_peers                        | [network]
 20190103000001 | drop_peers_clock                                   | [network]
 20190104000001 | add_recipient_public_key_to_full_block_list        | [chain]
 20190111111557 | add_protocolVersion_column_to_peers                | [network]
 20190204170819 | migrate_trs_dependant_tables_to_trs_trable         | [chain]
 20190313102300 | drop_blocks_list_and_trs_list_views                | [chain]
 20190319111600 | remove_unconfirmed_state                           | [chain]
 20190319111700 | remove_unconfirmed_state_from_mem_accounts_trigger | [chain]
 20190410112400 | add_asset_field_mem_accounts                       | [chain]

How to test it?

dropdb lisk_dev
createdb lisk_dev
node framework/test/test_app/index.js -n devnet

Review checklist

framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/index.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/modules/network/migrations/index.js Outdated Show resolved Hide resolved
@lsilvs lsilvs requested a review from shuse2 May 22, 2019 13:35
Copy link
Contributor

@pablitovicente pablitovicente left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good; I would create following issues:

  • Add logs showing the migration name and namespace
  • Make migrations version dependent so we can plan for the case we want to revert a complete set of migrations (maybe this calls for a more in depth talk about migrations strategy)

Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please adjust all SQL file structure based on the feedback.

framework/src/controller/application.js Outdated Show resolved Hide resolved
framework/src/controller/application.js Outdated Show resolved Hide resolved
framework/src/controller/application.js Outdated Show resolved Hide resolved
framework/src/controller/application.js Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
framework/src/controller/migrations/migration.js Outdated Show resolved Hide resolved
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the migration entity have following interface methods:

  1. hasInternalMigrationsTable
  2. hasMigrationsTable
  3. getLastInternalMigrationId
  4. readPendingInternalMigrations
  5. readPending
  6. applyPendingInternalMigration
  7. applyPendingMigration
  8. applyInternal
  9. applyList

Which clearly shows a lot of duplicate stuff happening in there. First question What is internal migration? It just the normal migration in the framework scope.

So instead of creating a new concept and a lot of duplicate code, implement the migration process with the following conditions:

  1. Choose a namespace for framework migrations e.g. lisk-framework
  2. Register framework migrations in application constructor.
  3. While migrating, implement the logic that migrations from the framework should be executed first. That's what you are currently trying to do.

The other solution would be (which I prefer):

  1. Don't treat that framework migrations (which are just schema definition of migrations table) as migration itself.
  2. Write idempotent SQL for what you currently have as framework migration
  3. Execute that SQL every time before running the actual migrations

Look at any other famous and established frameworks in this domain e.g. ActiveRecord, Eloquent, Doctrine you will notice none treat creating migrations table as a migration itself.

And one more thing, the migration you created for framework, containing:

UPDATE "migrations" SET namespace = 'network' WHERE id IN ('20161016133824', '20170113181857', '20171207000001', '20171227155620', '20180205000002', '20180327170000', '20181106000006', '20190103000001', '20190111111557');
UPDATE "migrations" SET namespace = 'chain' WHERE namespace = 'undefined';

These should be actually be splitted to their own specific modules. And how you do is to create a migration with id (timestamp) which executed first. Then rest of the migration from that module will be identified properly.

@lsilvs
Copy link
Contributor Author

lsilvs commented May 24, 2019

@nazarhussain I only agree with your last point (separate the 20190521000002_migrate_migrations_table.sql file into its own module migration files).
Everything else I will leave as it is. Feel free to propose your implementation in another PR.

@lsilvs
Copy link
Contributor Author

lsilvs commented May 24, 2019

Actually 20190521000002_migrate_migrations_table.sql is already in the correct place as it updates the migrations table and modules migrations shouldn't touch migrations table imo.

@lsilvs
Copy link
Contributor Author

lsilvs commented May 27, 2019

@nazarhussain I refactored the code to get rid of the table which kept the framework migrations. Let me know what do you reckon.

@lsilvs lsilvs requested a review from nazarhussain May 27, 2019 15:34
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lsilvs All seems good, just two small things to see as we discussed.

  1. Don't set 'undefined' as a value rather set the constraint after setting namspaces
  2. Rename the file 'migration.js' to 'migration_entity.js'

@lsilvs lsilvs force-pushed the 3197-implement-register-migration branch from b9ababa to f9cd530 Compare May 28, 2019 12:42
@lsilvs lsilvs requested a review from nazarhussain May 28, 2019 13:11
@shuse2 shuse2 merged commit 0bfc639 into development May 28, 2019
@shuse2 shuse2 deleted the 3197-implement-register-migration branch May 28, 2019 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add capability to register database migration from the application/module layer
4 participants