RFC: Resolver for inclusion of related models #3387
Conversation
_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, |
There was a problem hiding this comment.
Is it possible to retrieve metadata describing the relation?
Do we want to introduce InclusionResolutionContext?
There was a problem hiding this comment.
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?
bajtos
force-pushed
the
spike/resolve-included-models
branch
from
3bd66a2 to
e1f6745
Compare
|
@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? |
examples/todo-list/src/repositories/todo-list-image.repository.ts
Outdated
Show resolved
Hide resolved
packages/repository/src/relations/belongs-to/belongs-to.helpers.ts
Outdated
Show resolved
Hide resolved
| ); | ||
|
|
||
| // add this line to register inclusion resolver | ||
| this.registerInclusion('products', this.products.inclusionResolver); |
There was a problem hiding this comment.
Does it make sense to have an option for createHasManyRepositoryFactoryFor to automate this.registerInclusion('products', this.products.inclusionResolver) by default?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
this.registerHasManyRelation(
Perhaps just this.hasMany to keep it similar to what we have in LB3.
// inside CategoryRepository constructor
this.hasMany('products', productRepositoryGetter);
bajtos
force-pushed
the
spike/resolve-included-models
branch
from
27be832 to
a3148a5
Compare
|
Rebased on top of the latest master 💪 |
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š <[email protected]>
Signed-off-by: Miroslav Bajtoš <[email protected]>
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:
I'll expand that sub-tasks to cover examples of queries requesting inclusion of related models. Is that what you are asking for? |
Yes, that's what I actually meant. Please disregard the mention of |
bajtos
added
Relations
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_.mddocument that provides a high-level overview of the proposal.Next, read through the code to see implementation details you are interested in: