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

RFC: Resolver for inclusion of related models #3387

Closed
wants to merge 23 commits into from

Conversation

@bajtos
Copy link
Member

commented Jul 18, 2019

In this pull request, I am proposing a solution for resolving inclusion of related models and demonstrating the viability of the proposed solution.

See #2634 and #1352 for the previous discussions.

How to review this pull request:

Start by reading _SPIKE_.md document that provides a high-level overview of the proposal.

Next, read through the code to see implementation details you are interested in:

@bajtos bajtos requested review from raymondfeng and strongloop/loopback-next Jul 18, 2019

@bajtos bajtos self-assigned this Jul 18, 2019

@bajtos bajtos referenced this pull request Jul 18, 2019
2 of 2 tasks complete
_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Outdated
// inclusion requested by the user (e.g. scope constraints to apply)
inclusion: Inclusion,
// generic options object, e.g. carrying the Transaction object
options?: Options,

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 18, 2019

Member

Is it possible to retrieve metadata describing the relation?
Do we want to introduce InclusionResolutionContext?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 19, 2019

Author Member

Metadata describing the relation is always the same for every resolver invocation, therefore it's not passed in the arguments. Instead, the metadata is provided to the class constructor. Similarly how a getter of a repository for the target model is provided.

Do we want to introduce InclusionResolutionContext

I am not sure.

On the one hand, the current API where inclusion and options are provided via positional arguments, is simple and easy to understand. InclusionResolutionContext feels rather heavy-weight to me.

On the other hand, positional parameters are difficult to extend in the future. InclusionResolutionContext makes it easy to add additional context properties.

If we introduce InclusionResolutionContext, what properties would you like to see on that context object? Just inclusion and options?

_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved

@bajtos bajtos force-pushed the spike/resolve-included-models branch from 3bd66a2 to e1f6745 Jul 19, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

@raymondfeng Thank you for a great feedback! ❤️

I pushed few commits to improve my proposal, I think all comments should be either addressed in code or answered here on GitHub.

Do you have any more suggestions? Does the proposed design & implementation LGTY now?

);
// add this line to register inclusion resolver
this.registerInclusion('products', this.products.inclusionResolver);

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jul 19, 2019

Member

Does it make sense to have an option for createHasManyRepositoryFactoryFor to automate this.registerInclusion('products', this.products.inclusionResolver) by default?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 22, 2019

Author Member

I did consider that idea briefly.

I find it problematic for a method called create(...)FactoryFor to update register inclusion resolvers, it feels like a surprising side effect to me.

I consider createHasManyRepositoryFactoryFor and registerInclusion as lower-level building blocks that should stay decoupled, and can be used to build higher-level abstractions in the future.

For example, I can imagine creating a new method called e.g. registerHasManyRelation, that would call both createHasManyRepositoryFactoryFor and registerInclusion under the hood and initialize any repository properties (e.g. this.products) along the way.

// now
this.products = this.createHasManyRepositoryFactoryFor(
  'products',
  productRepositoryGetter,
);
this.registerInclusion('products', this.products.inclusionResolver);

// with the new API
this.registerHasManyRelation(
  'products', 
  productRepositoryGetter, 
  {allowInclusion: true} // optional
);

The non-trivial part is how to do this in a type-safe way - we need to let TypeScript understand that the string 'products' must match a property this.products with type HasManyRepositoryFactory.

Anyhow, I'd like to leave this out of this initial MVP scope.

I think it may be better to spend our time on #2483 (From relation definition to REST API with no repository/controller classes), that will allow developers to avoid manually written Repository classes altogether.

@raymondfeng If you like, I can create a follow-up task to investigate a more ergonomic API for registering relations inside Repository classes. WDYT?

This comment has been minimized.

Copy link
@bajtos

bajtos Jul 23, 2019

Author Member

this.registerHasManyRelation(

Perhaps just this.hasMany to keep it similar to what we have in LB3.

// inside CategoryRepository constructor
this.hasMany('products', productRepositoryGetter);
_SPIKE_.md Outdated Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
_SPIKE_.md Show resolved Hide resolved
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

Rebased on top of the latest master 💪

@bajtos bajtos referenced this pull request Jul 22, 2019
4 of 4 tasks complete

bajtos added some commits Jun 25, 2019

feat: introduce Repository API for handling inclusion of related models
Introduce two new protected methods in DefaultCrudRepository class:
- `normalizeFilter` converting LB4 Filter to legacy juggler filter
- `includeRelatedModels` to include related models to search results

Rework `find`, `findById` and `findOne` methods to leverage these new
helpers.

Simplify example-todo-list by leveraging `includeRelatedModels` too.

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
feat: generic HasMany inclusion resolver (in example only)
Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>
@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

@hacksparrow

LGTM. Next we need to update the repository package's README.md and show usage examples in the website docs.

Can you please clarify what do you mean by updating repository package's README? I mean what kind of content would you like to see there? AFAICT, the current README does not describe relations at all.

+1 to show usage examples in the docs. At the moment, my proposal has the following sub-task for each relation type:

Update documentation (e.g. Configuring a hasMany relation) to explain how to enable or disable inclusion.

I'll expand that sub-tasks to cover examples of queries requesting inclusion of related models. Is that what you are asking for?

@hacksparrow

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I'll expand that sub-tasks to cover examples of queries requesting inclusion of related models.

Yes, that's what I actually meant. Please disregard the mention of repository's README.md, I got too lost in the implementation details.

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I have created the following follow-up stories.

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