fix: migrate LB3 models mounted on LB4 app

[hacksparrow]

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 👈

[bajtos]

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.

[bajtos]

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?

[bajtos]

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)

[jannyHou]

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)

The patch has been reworked since my review.

[hacksparrow]

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]

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

[hacksparrow]

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

@raymondfeng @bajtos

[raymondfeng]

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

[jannyHou]

The changes LGTM 👏

[hacksparrow]

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

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.

[bajtos]

Almost there!

[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.

[hacksparrow]

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

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

[bajtos]

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]

@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

[bajtos]

👏

[bajtos]

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.

[nabdelgadir]

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

[emonddr]

Great stuff @hacksparrow :)