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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: migrate LB3 models mounted on LB4 app #3779

Merged
merged 1 commit into from Sep 27, 2019

Conversation

@hacksparrow
Copy link
Member

hacksparrow commented Sep 20, 2019

Migrates LB4 models mounted on LB4 an app.

Fixes #2825.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • 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
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch from 759ad10 to 24e80d2 Sep 20, 2019
Copy link
Member

bajtos left a comment

I find the proposed approach problematic because it introduces unwanted coupling between repository and booter-lb3app. LB3 applications are not a business of @loopback/repository.
The code iterating over datasources in the LB3 app belongs to packages/booter-lb3app.

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch 3 times, most recently from 5997fee to 6d8bb99 Sep 24, 2019
@hacksparrow hacksparrow changed the title [WIP] fix: migrate LB3 models mounted on LB4 app fix: migrate LB3 models mounted on LB4 app Sep 24, 2019
@hacksparrow hacksparrow marked this pull request as ready for review Sep 24, 2019
@hacksparrow hacksparrow requested a review from raymondfeng as a code owner Sep 24, 2019
@hacksparrow hacksparrow requested a review from bajtos Sep 24, 2019
Copy link
Member

bajtos left a comment

The patch looks reasonable now 馃憤

I am little bit concerned that we are testing this feature in isolation, there is no test to verify that npm run migrate is actually going to pick up datasources contributed by the LB3 app. Did you verify this scenario manually? Is there any reasonable way how to automate such test?

packages/booter-lb3app/src/lb3app.booter.ts Outdated Show resolved Hide resolved
const ds = dataSources[key];
processedDs.push(ds.name);
this.app
.bind(`datasources.lb3-${ds.name}`)

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 24, 2019

Member

I would like to discuss the binding naming convention with @raymondfeng and possibly with the wider community @strongloop/loopback-next .

Aspects to consider:

  • There should be very low probability of a naming conflict between a key generated for a LB3 datasource and a key created by LB4 API app.dataSource.
  • The key should make it obvious what's the datasource name and that this datasource is coming from a LB3 app.

I think the current proposal datasources.lb3-${ds.name} is reasonable 馃憤

Few other options to consider:

  • lb3-datasources.${ds.name} (e.g. lb3-datasources.db)
  • datasources.\$lb3\$${ds.name} (e.g. datasources.$lb3$db)

This comment has been minimized.

Copy link
@jannyHou

jannyHou Sep 25, 2019

Contributor

I am good with either
datasources.lb3-${ds.name} (pro: the prefix is consistent with other datasources)
or
lb3-datasources.${ds.name} (pro: we can apply the same naming convention for other lb3 artifacts like models)

@bajtos bajtos dismissed their stale review Sep 24, 2019

The patch has been reworked since my review.

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch from 6d8bb99 to f0e9f46 Sep 24, 2019
@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Sep 24, 2019

Did you verify this scenario manually?

Yes, in a standalone app.

Is there any reasonable way how to automate such test?

Will work on this next and propose something.

@dhmlau

This comment has been minimized.

Copy link
Contributor

dhmlau commented Sep 24, 2019

@hacksparrow, there's a decrease in the test coverage. Could you please look into it? Thanks.

@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Sep 25, 2019

@dhmlau the decrease is not in the related package (booter-lb3app). What do we do?

@raymondfeng @bajtos

@raymondfeng

This comment has been minimized.

Copy link
Member

raymondfeng commented Sep 25, 2019

@hacksparrow I don't see obvious cause of coverage to worry about in this case.

Copy link
Contributor

jannyHou left a comment

The changes LGTM 馃憦

@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Sep 26, 2019

@bajtos I have been considering some ways for verifying datasources contributed by LB3 are picked up, but they all involve app.findByTag('datasource'), which is already covered by the unit test in the PR.

An approach which is more appropriate to the reported issue (Database migrations for mounted LoopBack 3 Models) would be an integration test case, which verifies the state of the models in the database with no knowledge of the datasources used. But this will require a more elaborate setup (will need an SQL database) and dependencies. Is it worth it?

In my opinion, the unit test should suffice because the implementation of migration is done by enumerating the datasources bound to the app. If we can verify that a datasource is bound to the app, we can be sure all models attached to it will be migrated if the corresponding connector supports migration.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 26, 2019

An approach which is more appropriate to the reported issue (Database migrations for mounted LoopBack 3 Models) would be an integration test case, which verifies the state of the models in the database with no knowledge of the datasources used. But this will require a more elaborate setup (will need an SQL database) and dependencies. Is it worth it?

In my opinion, the unit test should suffice because the implementation of migration is done by enumerating the datasources bound to the app. If we can verify that a datasource is bound to the app, we can be sure all models attached to it will be migrated if the corresponding connector supports migration.

Fair enough. We can introduce an integration or end-to-end test later in the future, if needed. 馃憤

For posterity, I think it should be possible to write a test that does not require the real database, the test needs to replace the real automigrate or autoupdate method on a LB3 datasource with a sinon stub, and then verify the stub was called from app.migrateSchema.

Copy link
Member

bajtos left a comment

Almost there!

packages/booter-lb3app/src/lb3app.booter.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/lb3app.booter.ts Outdated Show resolved Hide resolved
packages/booter-lb3app/src/lb3app.booter.ts Outdated Show resolved Hide resolved
@derdeka

This comment has been minimized.

Copy link

derdeka commented Sep 26, 2019

Is it possible to disable this feature by options somehow (migration of lb3 models)? What happens if lb3 models and lb4 models have the same names or use the same database tables? Not all users might want to mix up lb3 and lb4 applications in that way.

@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Sep 26, 2019

@derdeka mounting LB3 app on a LB4 requires multiple steps to be followed. If one does not want their LB3 models to be mounted, those models should be removed from the LB3 app while the LB3 app being set up to be mounted in the LB4 app.

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch from f0e9f46 to a3caed9 Sep 26, 2019
@hacksparrow

This comment has been minimized.

Copy link
Member Author

hacksparrow commented Sep 26, 2019

@bajtos thanks for the nice suggestions, I have made the changes accordingly.

@@ -5,6 +5,7 @@

import {BootBindings, Booter} from '@loopback/boot';
import {CoreBindings, inject} from '@loopback/core';
import {DataSource} from '@loopback/repository';

This comment has been minimized.

Copy link
@bajtos

bajtos Sep 27, 2019

Member

As I commented before, @loopback/repository is not in regular/peer dependencies of booter-lb3app, therefore it may not be available at runtime, when this booter is installed in a LB project.

Also the DataSource type is not the right one to use, it described LB4 objects but your code is using LB3 instances! The correct type is juggler.DataSource.

Please remove this import and find a different solution how to describe the shape (interface) of LB3 datasource objects.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 27, 2019

@derdeka

Is it possible to disable this feature by options somehow (migration of lb3 models)? What happens if lb3 models and lb4 models have the same names or use the same database tables? Not all users might want to mix up lb3 and lb4 applications in that way.

Thank you for raising this concern!

I see only one scenario where import of LB3 & LB4 models can create conflict, and that's when there is a model with the same name attached to a datasource configured to use the same database, and thus share the same table. Additionally, the LB3 and LB4 models must defined differently.

It makes me wonder, what is the intent of application developer in such case. If the model definition is different between LB3 and LB4 applications, isn't this going to create inconsistencies in data? E.g. LB3 endpoints writing table rows in different format than expected by the LB4 model.

Setting that aside, I think it makes sense to give developers more control over which models are migrated. I don't think the decision should be based on LB3 or LB4, I'd prefer a more generic solution allowing developers to specify which datasources to migrate or even which models to migrate.

It turns out this is already supported via SchemaMigrationOptions and its models field. My recommendation is to overwrite migrateSchema in your application class and supply the list of models to migrate.

class MyApplication extends BootMixin(
  RepositoryMixin(RestApplication)
) {
  // ...

  migrateSchema(options: SchemaMigrationOptions = {}): Promise<void> {
    if (!options.models) {
      options = {...options, models: [/* put your model names here */]};
    }
    return super.migrateSchema(options);
  }
}

I think this is not ideal, let's discuss how to improve it here: #3819

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch from a3caed9 to 72f44f6 Sep 27, 2019
@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch 2 times, most recently from f812a00 to e811b0e Sep 27, 2019
@bajtos
bajtos approved these changes Sep 27, 2019
Copy link
Member

bajtos left a comment

馃憦

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Sep 27, 2019

Please consider adding a short paragraph to Mounting the LoopBack 3 Application in a LoopBack 4 Project and mention that npm run migrate will migrate all models from the LB3 application too.

Copy link
Contributor

nabdelgadir left a comment

Can you also update the lb3 application example and remove these lines?

@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch 2 times, most recently from 110fae0 to 485d739 Sep 27, 2019
Migrates LB4 models mounted on LB4 an app.
@hacksparrow hacksparrow force-pushed the fix/lb3-migration branch from 485d739 to 88f8bfd Sep 27, 2019
@hacksparrow hacksparrow merged commit 7d36f6d into master Sep 27, 2019
3 checks passed
3 checks passed
Travis CI - Pull Request Build Passed
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hacksparrow hacksparrow deleted the fix/lb3-migration branch Sep 27, 2019
@hacksparrow hacksparrow referenced this pull request Sep 27, 2019
2 of 2 tasks complete
@emonddr

This comment has been minimized.

Copy link
Contributor

emonddr commented Sep 27, 2019

Great stuff @hacksparrow :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can鈥檛 perform that action at this time.