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
bajtos opened this issue Mar 22, 2019 · 3 comments

Comments

@bajtos
Copy link
Member

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 #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 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

This comment has been minimized.

Copy link

commented Apr 17, 2019

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

This comment has been minimized.

Copy link

commented Apr 23, 2019

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

@dhmlau dhmlau added this to the June 2019 milestone milestone May 31, 2019

@dhmlau dhmlau referenced this issue Jun 3, 2019
24 of 37 tasks complete

@bajtos bajtos self-assigned this Jun 4, 2019

@nabdelgadir nabdelgadir removed this from the June 2019 milestone milestone Jun 4, 2019

@dhmlau dhmlau added 2019Q3 and removed 2019Q2 labels Jul 8, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

MVP scope

  • Run repository tests for PostgreSQL #3436
  • Run repository tests for Cloudant #3437
  • Reject create/update requests when data contains navigational properties #3439
  • Verify relation type in resolve{Relation}Metadata #3440
  • Add keyFrom to resolved relation metadata #3441
  • Test relations against databases #3442
  • Add findByForeignKeys helper (initial version) #3443
  • Support inq splitting in findByForeignKeys #3444
  • Introduce InclusionResolver concept #3445
  • Include related models in DefaultCrudRepository #3446
  • Implement InclusionResolver for hasMany relation #3447
  • Implement InclusionResolver for belongsTo relation #3448
  • Implement InclusionResolver for hasOne relation #3449
  • Update todo-list example to use inclusion resolver #3450
  • Add inclusion resolvers to lb4 relation CLI #3451
  • Blog post: announce Inclusion of related models #3452

Post-MVP

  • Include related models with a custom scope #3453
  • Recursive inclusion of related models #3454
  • Reject queries with incompatible "filter.fields" and "filter.include" #3455
  • Spike: robust handling of ObjectID type for MongoDB #3456

@bajtos bajtos closed this 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
Projects
None yet
5 participants
You can’t perform that action at this time.