fix: multiple instances of the same repository class

[bajtos]

Fix DefaultCrudRepository to re-use legacy juggler Models across all instances of the same Repository class.

Before this change, each call of DefaultCrudRepository constructor was redefining the backing persisted model.

This commit adds a caching mechanism to DefaultCrudRepository: when setting up a backing model, we check datasource's modelBuilder registry to find if the backing model was not already created by an older instance of the repository.

See #1032 and #995.

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

[b-admike]

👏

[shimks]

Does deep assertion not work with shouldjs?

[raymondfeng]

We can use expect(model1).to.be.exactly(model2);

[bajtos]

We can. My experience is that equality assertion produces an error message that's rather unhelpful, because it it lists all model properties and properties of these properties. Because a model is attached to a datasource that has a reference to a model builder, eventually all models registered with the data source are printed in the diff. That's why I decided to use this form, because really all we want to see is whether the two models are the same V8 entity or not.

[virkt25]

cheap tends to have a negative connotation (at least in my head). Can we use lightweight or easy?

[bajtos]

How about something along the following lines?

A repository class is a lightweight object, its instances can be created with low runtime overhead, (etc.)

[raymondfeng]

There is a flaw in this PR:

1. A 3.x model classes can only be attached to one data source.
2. We introduce Repository so that it can represent modelClass + dataSource
3. Each instance of Repository should have its own 3.x model class
4. We should use bindModel instead of model.attachTo()

[bajtos]

A 3.x model classes can only be attached to one data source.
We introduce Repository so that it can represent modelClass + dataSource

Agreed.

Each instance of Repository should have its own 3.x model class

I disagree for two reasons:

• A datasource maintains a model builder with a registry of all models. When I attach the same LB4 model to the same LB3 datasource, and the repository creates a new PersistedModel each time, then each call of the repository constructor will override the modelBuilder entry.
• Creating a new Model class is expensive performance wise.

In my proposal, we will have a single LB3 PersistedModel for each pair of LB4 Model + LB3 dataSource. If you attach the same LB4 Model to multiple dataSources, then we will have multiple LB3 PersistedModels, one for each dataSource.

Is there any specific problem you are concerned about?

[b-admike]

Each instance of Repository should have its own 3.x model class

I would like to know why we can't re-use the model class if it's already defined as proposed in this PR?

[raymondfeng]

The 3.x model classes maintain two states:

1. The model definition
2. The attached data source

For example, a Customer model can only be attached to mongodb or mysql but we cannot have the same model definition to be used with mongodb or mysql unless we subclass it.

In LB4, we subclass Customer for each repository instance to delegate to juggler model methods without interfering with other instances. For example:

export class MyController {
  @repository(Customer, 'mongodb')
  private mongoRepo;

  @repository(Customer, 'mysql')
  private mysqlRepo;
}

@bajtos Please note in LB 3.x, all data source instances share the same modelBuilder if it's from loopback module. See https://github.com/strongloop/loopback/blob/master/lib/registry.js#L362.

If in LB4, we always create DataSource instances with its own modelBuilder, your PR should be fine.

[bajtos]

If in LB4, we always create DataSource instances with its own modelBuilder, your PR should be fine.

Yes, that's the case so far - in LB4, each DataSource has its own model builder.

[bajtos]

@raymondfeng As I commented above, each LB4 DataSource has its own model builder. Given that, are you ok with landing this pull request?

@virkt25 I removed the word "cheap" from the docs, per our discussion above. Does the patch LGTY now?

[bajtos]

The build failure on Node.js 10.x is unrelated to my changes. Looks like one of our dependencies and/or Node.js core is using experimental fs.promises module and this triggers a warning that's considered by our testing infrastructure as invalid usage of console logs in tests.

=== ATTENTION - INVALID USAGE OF CONSOLE LOGS DETECTED ===
(node:1868) ExperimentalWarning: The fs.promises API is experimental
    at writeOut (internal/process/warning.js:18:20)
    at output (internal/process/warning.js:69:3)
    at process.on (internal/process/warning.js:100:7)
    at process.emit (events.js:182:13)
   [...]

[bajtos]

How to troubleshoot the problem: run the tests with --trace-warning runtime flag.

$ node --trace-warnings ./packages/build/node_modules/.bin/_mocha 'packages/*/dist10/test/**/*.js'
(node:7885) ExperimentalWarning: The fs.promises API is experimental
    at Object.get [as promises] (fs.js:84:15)
    at Object.keys.forEach.key (/Users/bajtos/src/loopback/next/packages/testlab/node_modules/fs-extra/lib/fs/index.js:48:20)
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/Users/bajtos/src/loopback/next/packages/testlab/node_modules/fs-extra/lib/fs/index.js:47:17)
    at Module._compile (internal/modules/cjs/loader.js:678:30)
    [...]

The problem is caused by fs-extra - see jprichardson/node-fs-extra#577

[bajtos]

Rebased on top of the latest master, all CI checks are green again 💚

[raymondfeng]

@bajtos I have rebased the PR against the latest master.

[raymondfeng]

@bajtos I merged the PR for you.