Easy-to-use database migrations at application level

[bajtos]

I find the current instructions for database migration as too involved for practical use, for example when running our example applications against a real database (SQL, MongoDB). I would like to propose a better user experience as a short-term solution to scratch this itch. Please see #487 for the discussion about a more comprehensive solution for long term.

In this pull request, I am introducing new APIs to simplify database migrations:

• A new interface MigrateableRepository describing repositores that know how to migrate their database schema.

• A new Application-level method app.migrateSchema() provided by RepositoryMixin and running schema migration provided by all registered repositories datasources under the hood.

Using these two new APIs, I also

• Implement MigrateableRepository in DefaultCrudRepository using autoupdate/automigrate APIs provided by legacy-juggler's DataSource class.

• Simplify the instructions shown in Database-migrations.db

• Add an example migrate.ts script to our Todo example to verify that the code snippet shown in the docs works as intended.

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• out of scope: Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

[bajtos]

I find the current instructions for database migration as too involved for practical use, for example when running our example applications against a real database (SQL, MongoDB). I would like to propose a better user experience as a short-term solution to scratch this itch. Please see #487 for the discussion about a more comprehensive solution for long term.

Right now, the pull request contains what I consider as a minimal viable implementation/improvement. Besides the changes proposed, I would like to discuss few more improvements we may or may not want to do, either as part of this pull request or in follow-ups:

• Add src/migrate.ts to all example repositories. I think it would be very useful to have automigration available in our TodoList example too. /cc @b-admike
• Introduce an npm/package.json script, e.g. npm run update-schema, as an alias for node dist/src/migrate.
• Modify the CLI template to include src/migrate.ts and npm run update-schema in all new projects.

@strongloop/loopback-next thoughts?

BTW, I have also considered an entirely different approach, where the migration is executed by a new lb4 command, thus keeping the implementation details of migrate.ts inside LB4 and outside of the scaffolded code. I see two problems with such approach:

1. When migrate.ts is a part of LB4 CLI, then application developers have no control about the process, they cannot add additional migration steps.
2. To migrate the database schema, users have to install LB4 CLI on their target machine. This may be a problem in many environments, plus it opens door for inconsistencies caused by different LB4 CLI versions installed on different machines.

[bajtos]

Hmm, I just realized that my proposed implementation is not flexible enough to support migration of related models. For example, when a Customer has many Order instances, the migration should create a foreign key constraint between Customer's id and Order's customerId properties/columns. This is difficult to achieve when migrating models one by one.

I hope that moving migrateSchema from Repository to DataSource should fix the problem, I'll give it a try later this week.

The high-level API at application (RepositoryMixin) level seems fine to me as-is, it can easily support both repository-level and datasource-level migrations, thus I think it won't be affected by the changes in the underlying implementation.

[bajtos]

@raymondfeng

I would prefer to have two methods and use updateSchema as default.

• migrateSchema(...)
• updateSchema(...)

Let's discuss.

I personally prefer a single method with an option to choose between an incremental update or destructive drop + create. I makes it easier to implement wrapper functions delegating the implementation to lower-level layers, e.g. a single app.migrateSchema calling a single repository.migrateSchema & forwarding the entire options argument.

When there are two methods, then we also need multiple wrapper functions (app.migrateSchema and app.updateSchema.

What are your arguments for having two methods?

I also find the names "update" and "migrate" not descriptive enough, the term "migrate" does not imply drop+create in my mind. AFAIK, the term "migration" is usually used for non-destructive upgrades. Few references:

• Ruby on Rails https://guides.rubyonrails.org/v3.2/migrations.html.
• Martin Fowler on Evolutionary Database Design: https://www.martinfowler.com/articles/evodb.html#AllDatabaseChangesAreMigrations
• .NET EntityFramework's Code First Migrations: https://docs.microsoft.com/en-us/aspnet/mvc/overview/getting-started/getting-started-with-ef-using-mvc/migrations-and-deployment-with-the-entity-framework-in-an-asp-net-mvc-application

It should be possible to filter what repos should be migrated/updated.

I don't understand what's the use case for such filter.

Here is the user story I am trying to address: As a developer wanting to run a LB4 app, I want an easy way how to update my database schema to match application's domain model. In this scenario, we always want to update all tables.

What scenario do you @raymondfeng have in mind for choosing a subset of tables only? I think this filter is easy to add later as an incremental improvement of my MVP solution, thus I am proposing to leave it out of this initial pull request.

[bajtos]

In d8c52a6, I reworked the underlying implementation to leverage DataSource-level autoupdate/automigrate API and thus allow connectors to set up foreign keys.

The pull request is ready for another round of review. @strongloop/loopback-maintainers PTAL.

I tested the migration with examples/todo-list and MySQL connector. The tables were created correctly, but unfortunately the foreign key constraint was not set up. I don't know whether this is a limitation of our MySQL connector, or whether it's a problem of the relation implementation in LB4. Either way, I consider this issue out of scope of this (initial) pull request.

[raymondfeng]

The tables were created correctly, but unfortunately the foreign key constraint was not set up. I don't know whether this is a limitation of our MySQL connector, or whether it's a problem of the relation implementation in LB4.

I think it's a limitation of LB3 automigrate/autoupdate.

[bajtos]

I think it's a limitation of LB3 automigrate/autoupdate.

Good to know, so I am not going to worry about this for now. I think the most important thing is that the current high-level design does not prevent the connector to setup referential integrity and possibly other constraints in the future.

[raymondfeng]

What scenario do you @raymondfeng have in mind for choosing a subset of tables only? I think this filter is easy to add later as an incremental improvement of my MVP solution, thus I am proposing to leave it out of this initial pull request.

Developers want to control what models that need to be migrated. For example, our UI allows people to select a list of models and click migrate.

In real world, some models are created newly while others are discovered from existing DB schema. We don't want run migrate for such models discovered.

We can simply add models to the options and pass it down to datasource.

[bajtos]

Developers want to control what models that need to be migrated. For example, our UI allows people to select a list of models and click migrate.

In real world, some models are created newly while others are discovered from existing DB schema. We don't want run migrate for such models discovered.

We can simply add models to the options and pass it down to datasource.

I feel it's a bit of a scope creep here, but I am ok to add models to the options in the TS/JS API as you suggested 👍

[bajtos]

@raymondfeng I added support for options.models (defaulting to migrate all models) and reworked a boolean option dropExistingSchema into existingSchema accepting a string enum.

LGTY now?

---

I would also like to hear opinions from @strongloop/loopback-maintainers on the following topic I mentioned earlier:

Right now, the pull request contains what I consider as a minimal viable implementation/improvement. Besides the changes proposed, I would like to discuss few more improvements we may or may not want to do, either as part of this pull request or in follow-ups:

• Add src/migrate.ts to all example repositories. I think it would be very useful to have automigration available in our TodoList example too. /cc @b-admike
• Introduce an npm/package.json script, e.g. npm run update-schema, as an alias for node dist/src/migrate.
• Modify the CLI template to include src/migrate.ts and npm run update-schema in all new projects.

[dhmlau]

IMHO, it is a good idea to have a more "automatic" way to create the database tables for users, especially for new users.

Another question or discussion point is that what we want for the default behaviour for someone creating a LB app for real use. Maybe it's just me, I might just want the migrate script to be run for the first time to create the db definitions or whenever that's needed rather than on every application start.

[b-admike]

LGTM overall, just have some (mostly) minor comments.

[b-admike]

nitpick: maybe add a comment for imports on top?