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

Spike: Resolver for inclusion of related models #2634

Closed
7 tasks
bajtos opened this issue Mar 22, 2019 · 3 comments
Closed
7 tasks

Spike: Resolver for inclusion of related models #2634

bajtos opened this issue Mar 22, 2019 · 3 comments
Assignees
Labels
Relations Model relations (has many, etc.) spike

Comments

@bajtos
Copy link
Member

bajtos commented Mar 22, 2019

Continue the works started in #2124, leverage the building blocks implemented as an outcome of #2592 and create a detailed proposal for implementing resolver for navigational properties.

Implementation wise, I think we want to end up with the following user experience:

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);
  }
}

The question is how to implement _register**Inclusion APIs and process the registered inclusion handlers in find/findById implementations provided by DefaultCrudRepository. See #1352 (comment) for more details on that.

It is important to consider the scenario where we want to fetch multiple source models and include their related models in the result, e.g. TodoList.find(1, {include: [{relation: 'todos'}]}). Under the hood, we must avoid SELECT N+1 issue. Inclusion handler must be able to fetch related models for multiple source-model ids in one query (using inq operator under the hood). The design proposed in #1352 (comment) is already taking that into account, so does the existing implementation in juggler.

Acceptance criteria

  • A draft pull request demonstrating the proposed solution:
    • Use examples/todo-list to show the proposed user experience
    • Provide a PoC implementation in packages/repository
    • Include a markdown document in the monorepo root (/_SPIKE_.md) describing the proposal at a high level. Keeping the document in git (instead in GH comments) makes it easier to discuss individual parts of the proposal and keep the proposal versioned.
  • In Inclusion of related models [MVP] #1352 (comment), I am proposing to leverage the existing code in juggler, e.g. lib/include.js#L609-L634. If we decide to do that, then the spike should outline how we are going to make those bits available to LB4 repositories. Are we going to export it from juggler? Migrate the code over from juggler to LB4 repository module?
  • Pay a close attention to test coverage. How are we going to migrate the current test suite in juggler that's extensively covering different aspects of inclusion of related models?
  • The follow-up stories created as an outcome of this spike should include tasks to cover the following areas:
    • Update lb4 repository to generate calls of this._registerHasManyInclusion and friends.
    • Documentation updates
    • Announcement blog post
@bajtos bajtos added spike Relations Model relations (has many, etc.) 2019Q2 labels Mar 22, 2019
@bajtos bajtos changed the title Spike: resolver for inclusion of related models Spike: Resolver for inclusion of related models Mar 22, 2019
@bajtos bajtos added the p1 label Apr 9, 2019
@AnanthGopal
Copy link

Hi @bajtos

this._registerHasManyInclusion('todos', todoRepositoryGetter);

When I add this code I got the error "_registerHasManyInclusion" it's not defined. Please provide the full function for this _registerHasManyInclusion

@shadyanwar
Copy link

@AnanthGopal this is a spike. The feature is still being worked on and is not implemented or released yet.

@bajtos
Copy link
Member Author

bajtos commented Jul 26, 2019

MVP scope

Post-MVP

@bajtos bajtos closed this as completed Jul 26, 2019
@dhmlau dhmlau added this to the July 2019 milestone milestone Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Relations Model relations (has many, etc.) spike
Projects
None yet
Development

No branches or pull requests

5 participants