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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inclusion of related models #1352

Open
dhmlau opened this issue May 24, 2018 · 17 comments

Comments

Projects
None yet
@dhmlau
Copy link
Contributor

commented May 24, 2018

Description / Steps to reproduce / Feature proposal

Follow up task for PR #1342

Inclusion of related models (same as LB3)
For example, Customer model has {include: ['order']}. When query on Customer, the result should includes the Order array.

Duplicates:

Follow-up stories:

  • Add support for "include" and "fields" to findById (REST API) #1721

Acceptance Criteria

  • Modify the implementation of the following DefaultCrudRepository methods to correctly support inclusion of related models as configured via filter.include property.
    • find
    • findOne
    • findById
  • Include integration-level tests using in-memory database to verify the implementation.
  • Add new acceptance-level tests for "find" method to verify how to send a REST request to find models including related models. Because the REST API is generated by our CLI, I think these tests should be implemented in the todo-list example repository.
  • Documentation
  • Blog post!
@bajtos

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

I added the following acceptance criteria:

  • Modify the implementation of the following DefaultCrudRepository methods to correctly support inclusion of related models as configured via filter.include property.
    • find
    • findOne
    • findById
  • Include integration-level tests using in-memory database to verify the implementation.
  • Add new acceptance-level tests for "find" method to verify how to send a REST request to find models including related models. Because the REST API is generated by our CLI, I think these tests should be implemented in the todo-list example repository.

I think we need a spike to determine how to actually implement relation traversal, because right now, a repository for source model does not necessarily know how to obtain an instance of a repository for accessing the target model.

/cc @dhmlau

@dhmlau dhmlau referenced this issue Oct 31, 2018

Closed

[Spike] How to do relation traversal #1952

0 of 3 tasks complete
@dhmlau

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

@bajtos , I've created the spike #1952. I've copied and pasted part of the acceptance criteria over there. Could you please review? Thanks.

@dhmlau dhmlau added Relations and removed needs grooming labels Oct 31, 2018

@raymondfeng

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Cross-posting from #1939.

I realized this PR is just a starting point to fully support include. There are a few issues:

@mrbatista

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

@dhmlau The inclusion of related models should work with belongsTo relation like LB3.

@bajtos

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

I was thinking about this problem and perhaps we can implement this at LB4 Repository level?

Let's say "Customer" has many "Order" instances, the relation is called "orders" and we want to fetch a customer with their orders.

The way how inclusion works in juggler and LB 3.x, such request creates multiple queries:

  1. First, Customer instances are fetched the usual way.
  2. Secondly, we process "filter.include" entries and load related models - see lib/include.js. The relation name (e.g. "orders") is provided by the caller in filter.include and juggler uses the relation name to look up the relation definition, the target model, etc.

For LoopBack 4, I am thinking about making the relation lookup more explicit and letting the Repository to specify a lookup table for inclusion. Similarly to how we explicitly build HasManyRepositoryFactory and BelongsToAccessor now.

A mock-up usage to illustrate what I mean:

export class TodoListRepository extends DefaultCrudRepository<
  TodoList,
  typeof TodoList.prototype.id
> {
  public readonly todos: HasManyRepositoryFactory<
    Todo,
    typeof TodoList.prototype.id
  >;

  constructor(
    @inject('datasources.db') dataSource: juggler.DataSource,
    @repository.getter(TodoRepository)
    protected todoRepositoryGetter: Getter<TodoRepository>,
  ) {
    super(TodoList, dataSource);
    this.todos = this._createHasManyRepositoryFactoryFor(
      'todos',
      todoRepositoryGetter,
    );
   ////// THE FOLLOWING LINE IS NEWLY ADDED
   this._registerHasManyInclusion('todos', todoRepositoryGetter);
  }
}

export class TodoRepository extends DefaultCrudRepository<
  Todo,
  typeof Todo.prototype.id
> {
  public readonly todoList: BelongsToAccessor<
    TodoList,
    typeof Todo.prototype.id
  >;

  constructor(
    @inject('datasources.db') dataSource: juggler.DataSource,
    @repository.getter('TodoListRepository')
    protected todoListRepositoryGetter: Getter<TodoListRepository>,
  ) {
    super(Todo, dataSource);

    this.todoList = this._createBelongsToAccessorFor(
      'todoList',
      todoListRepositoryGetter,
    );
   ////// THE FOLLOWING LINE IS NEWLY ADDED
    this._registerBelongsToInclusion('todoList', todoListRepositoryGetter);
  }
}

Implementation-wise, the base Repository implementation should process include inside the find method. A mock-up:

class DefaultCrudRepository {
  // ...
  _inclusions: {[key: string]: InclusionHandler};

  async find(filter?: Filter<T>, options?: Options): Promise<T[]> {
    const include = filter.include;
    filter = Object.assign({}, filter, {include: undefined});

    const models = await ensurePromise(
      this.modelClass.find(filter as legacy.Filter, options),
    );
    const entities = this.toEntities(models);

    for (relationName of include) {
      const handler = this._inclusions[relationName];
      if (!(handler)) throw new HttpErrors.BadRequest('Invalid inclusion.');
      await handler.fetchIncludedModels(entities, key);
    }
  }

  _registerHasManyInclusion(relationName, targetRepoGetter) {
    const meta = this.entityClass.definition.relations[relationName];
    this._inclusions[relationName] = new HasManyInclusionHandler(meta, targetRepoGetter);
  }
}

class HasManyInclusionHandler {
  constructor(public relation, public repositoryGetter) {}

  fetchIncludedModels(entities, relationName) {
    // see https://github.com/strongloop/loopback-datasource-juggler/blob/f0a6bd146b7ef2f987fd974ffdb5906cf6a584db/lib/include.js#L609-L634
   const objIdMap2 = includeUtils.buildOneToOneIdentityMapWithOrigKeys(entities, this.relation.keyFrom);

    const filter = {};
    filter.where[relation.keyTo] = {
      inq: uniq(objIdMap2.getKeys()),
    };
  
    const targets = findWithForeignKeysByPage(
      await this.repositoryGetter(), filter, this.relation.keyTo, 0);
   const targetsIdMap = includeUtils.buildOneToManyIdentityMapWithOrigKeys(
      targets, this.relation.keyTo);
    // store targets to entities, e.g. 
    // set entities[0].orders to the target with "customerId=entities[0].id"
  }
@samarpanB

This comment has been minimized.

Copy link
Contributor

commented Jan 16, 2019

Hello @bajtos , what's the update on this ? When can we expect this feature in LB4. We are using LB4 since its GA, but this missing feature is making us to not to move to production. Majorly due to performance issues. In order to fetch related models, we have to query other repositories manually to fetch data which in case of find method is too many DB hits.
We really need this.

@cbullokles

This comment has been minimized.

Copy link

commented Jan 22, 2019

Is this issue still there? I'm trying to execute a code similar to the todo-list tutorial and it is not working... using MySQL...

@sbacem

This comment has been minimized.

Copy link

commented Feb 6, 2019

Also i need this feature,
But how i can use QueryBuilder Juggler and get left join table,
I would create new function in Repository and use it in controller,
This solution it is recommended ?
Any exemple ?

@shadyanwar

This comment has been minimized.

Copy link

commented Feb 6, 2019

Explore the way to resolve inclusion property is listed in February 2019 milestones. So, hopefully we will have something by the beginning of next month.

@bajtos

This comment has been minimized.

Copy link
Member

commented Mar 22, 2019

Cross-posting the list of stories created in [Spike] Explore the way to resolve inclusion property #2152:

  • Fix repository-json-schema to handle circular references #2628
  • Allow controllers to provide definitions of models referenced in operation spec #2629
  • Enhance getJsonSchema to describe navigational properties #2630
  • Implement getJsonSchemaRef and getModelSchemaRef helpers #2631
  • Modify Repository find* methods to include navigational properties #2632
  • Add navigational properties to examples/todo-list #2633
  • Spike: Resolver for inclusion of related models #2634

@dhmlau dhmlau referenced this issue Apr 1, 2019

Closed

Monthly Milestone - April 2019 🌿🐰☔️ #2669

20 of 46 tasks complete

@emonddr emonddr modified the milestone: April 2019 milestone Apr 1, 2019

@dhmlau dhmlau referenced this issue Apr 5, 2019

Open

[Roadmap] 🚀2Q2019 🚀 #2692

15 of 22 tasks complete

@dhmlau dhmlau referenced this issue Apr 30, 2019

Closed

Monthly Milestone - May 2019 🌷🐝 🍄 #2818

27 of 32 tasks complete
@samarpanB

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

Hello @bajtos , When can we expect this feature to land ? Its a must-have for a production app. If you need help to make this feature to complete sooner, I can gladly contribute with your guidance. Please let me know.

@dhmlau

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@samarpanB, thanks for your offer! We also wish to get it completed soon.
Currently there are 2 tasks that are part of this epic. We are planning them for June milestone, but if you could contribute, that would be perfect.

  • Implement getJsonSchemaRef and getModelSchemaRef helpers #2631
  • Enhance getJsonSchema to describe navigational properties #2630
@samarpanB

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

@dhmlau Let me try the second one (#2630 ) first.

@dhmlau

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

Thanks @samarpanB. I just assign you that issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.