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

Easy-to-use database migrations at application level #2059

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@bajtos
Member

bajtos commented Nov 20, 2018

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 bajtos self-assigned this Nov 20, 2018

@bajtos bajtos requested review from raymondfeng, nabdelgadir and strongloop/loopback-maintainers Nov 20, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 20, 2018

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 bajtos force-pushed the feat/update-schema branch from 65efb0d to 223f5bd Nov 20, 2018

@bajtos bajtos changed the title from feat(repository): migrateSchema APIs to Easy-to-use database migrations at repository & application level Nov 20, 2018

@bajtos bajtos force-pushed the feat/update-schema branch from b3f3789 to 61045e7 Nov 20, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 20, 2018

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 bajtos changed the title from Easy-to-use database migrations at repository & application level to [WIP] Easy-to-use database migrations at application level Nov 20, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 22, 2018

@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:

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 bajtos force-pushed the feat/update-schema branch from 61045e7 to d8c52a6 Nov 22, 2018

@bajtos bajtos changed the title from [WIP] Easy-to-use database migrations at application level to Easy-to-use database migrations at application level Nov 22, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 22, 2018

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.

@bajtos bajtos force-pushed the feat/update-schema branch 2 times, most recently from d12b495 to f015845 Nov 22, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 26, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 26, 2018

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

This comment has been minimized.

Member

raymondfeng commented Nov 26, 2018

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

This comment has been minimized.

Member

bajtos commented Nov 26, 2018

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 bajtos force-pushed the feat/update-schema branch from f015845 to 929d80e Nov 27, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 27, 2018

@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

This comment has been minimized.

Contributor

dhmlau commented Nov 27, 2018

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.

Show resolved Hide resolved docs/site/Database-migrations.md Outdated
{% include code-caption.html content="src/application.ts" %}
```ts
export class TodoListApplication extends BootMixin(

This comment has been minimized.

@b-admike

b-admike Nov 29, 2018

Member

nitpick: maybe add a comment for imports on top?

Show resolved Hide resolved examples/todo/src/migrate.ts Outdated
Show resolved Hide resolved packages/repository/src/mixins/repository.mixin.ts
feat(repository): migrateSchema APIs
Introduce a new Application-level method `app.migrateSchema()` provided
by `RepositoryMixin`, this method executes schema migration as
implemented by datasources registered with the application.

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.

@bajtos bajtos force-pushed the feat/update-schema branch from 929d80e to fcebea9 Nov 29, 2018

@bajtos bajtos merged commit ad0229b into master Nov 29, 2018

5 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.225%
Details
security/snyk - package.json (dhmlau) No manifest changes detected

@bajtos bajtos deleted the feat/update-schema branch Nov 29, 2018

@bajtos bajtos referenced this pull request Nov 29, 2018

Merged

feat: scaffold DB migration script for new app projects #2094

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment