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.

Sign up for GitHub

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

Jump to bottom

fix: multiple instances of the same repository class #1302

Merged
merged 1 commit into from
May 14, 2018
Merged

fix: multiple instances of the same repository class #1302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion docs/site/Repositories.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ summary:
---

A Repository is a type of _Service_ that represents a collection of data within
a DataSource.
a DataSource. A repository class is a lightweight object, its instances can be
created with low runtime overhead. Typically a new repository instance is
created for each incoming request.

## Example Application

Expand Down
16 changes: 14 additions & 2 deletions packages/repository/src/repositories/legacy-juggler-bridge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
NamedParameters,
PositionalParameters,
} from '../common-types';
import {Entity} from '../model';
import {Entity, ModelDefinition} from '../model';
import {Filter, Where} from '../query';
import {EntityCrudRepository} from './repository';

Expand Down Expand Up @@ -88,7 +88,19 @@ export class DefaultCrudRepository<T extends Entity, ID>
`Entity ${entityClass.name} must have at least one id/pk property.`,
);

// Create an internal legacy Model attached to the datasource
this.setupPersistedModel(definition);
}

// Create an internal legacy Model attached to the datasource
private setupPersistedModel(definition: ModelDefinition) {
const dataSource = this.dataSource;

const model = dataSource.getModel(definition.name);
if (model) {
// The backing persisted model has been already defined.
this.modelClass = model as typeof juggler.PersistedModel;
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@b-admike b-admike May 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// We need to convert property definitions from PropertyDefinition
// to plain data object because of a juggler limitation
Expand Down
7 changes: 7 additions & 0 deletions packages/repository/test/unit/repositories/legacy-juggler-bridge.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,13 @@ describe('DefaultCrudRepository', () => {
await model.deleteAll();
});

it('shares the backing PersistedModel across repo instances', () => {
const model1 = new DefaultCrudRepository(Note, ds).modelClass;
const model2 = new DefaultCrudRepository(Note, ds).modelClass;

expect(model1 === model2).to.be.true();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does deep assertion not work with shouldjs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

it('implements Repository.create()', async () => {
const repo = new DefaultCrudRepository(Note, ds);
const note = await repo.create({title: 't3', content: 'c3'});
Expand Down