-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[Spike] feat: inclusion #2124
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] feat: inclusion #2124
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import {Entity} from '../model'; | ||
| import {Getter} from '@loopback/core'; | ||
| import { | ||
| Filter, | ||
| Where, | ||
| resolveHasManyMetadata, | ||
| resolveBelongsToMetadata, | ||
| RelationMetadata, | ||
| RelationType, | ||
| constrainWhere, | ||
| } from '../'; | ||
| import {AnyObject} from '..'; | ||
| import {DefaultCrudRepository} from './legacy-juggler-bridge'; | ||
| import {inspect} from 'util'; | ||
| import { | ||
| HasManyDefinition, | ||
| BelongsToDefinition, | ||
| HasManyResolvedDefinition, | ||
| BelongsToResolvedDefinition, | ||
| } from '../relations'; | ||
|
|
||
| type ResolvedRelationMetadata = | ||
| | HasManyResolvedDefinition | ||
| | BelongsToResolvedDefinition; | ||
|
|
||
| // SE: the source entity | ||
| // TE: the target entity | ||
| // SID: the ID of source entity | ||
| // TID: the ID of target entity | ||
|
|
||
| export class InclusionHandler<SE extends Entity, SID> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name is a bit confusing as
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah it should be plural |
||
| _handlers: {[relation: string]: Function} = {}; | ||
| constructor(public sourceRepository: DefaultCrudRepository<SE, SID>) {} | ||
|
|
||
| registerHandler<TE extends Entity, TID>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea to delegate resolution of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can borrow some ideas from https://graphql.org/learn/execution/. It will allow us even define custom relations.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| relationName: string, | ||
| targetRepoGetter: Getter<DefaultCrudRepository<TE, TID>>, | ||
| ) { | ||
| this._handlers[relationName] = fetchIncludedItems; | ||
| const self = this; | ||
|
|
||
| async function fetchIncludedItems( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It can be simplified as: this._handlers[relationName] = async (
fks: SID[],
filter?: Filter<TE>,
): Promise<TE[]> => {...}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should define a type such as: export type IncludedItemsFetcher<SID, TE> = (
fks: SID[],
filter?: Filter<TE>,
) => Promise<TE[]> |
||
| fks: SID[], | ||
| filter?: Filter<TE>, | ||
| ): Promise<TE[]> { | ||
| const targetRepo = await targetRepoGetter(); | ||
| const relationDef: ResolvedRelationMetadata = self.getResolvedRelationDefinition( | ||
| relationName, | ||
| ); | ||
| filter = filter || {}; | ||
| filter.where = self.buildConstrainedWhere<TE>( | ||
| fks, | ||
| filter.where || {}, | ||
| relationDef, | ||
| ); | ||
| console.log(`inclusion filter: ${inspect(filter)}`); | ||
|
|
||
| return await targetRepo.find(filter); | ||
| } | ||
| } | ||
|
|
||
| findHandler(relationName: string) { | ||
| const errMsg = | ||
| `The inclusion handler for relation ${relationName} is not found!` + | ||
| `Make sure you defined ${relationName} properly.`; | ||
|
|
||
| return this._handlers[relationName] || new Error(errMsg); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's suspicious to return an const handler = this._inclusionHandler.findHandler(relation);
if (!handler) {
throw new Error('Fetch included items is not supported');
}
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah the code here should definitely be refined ^
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's suspicious to return an const handler = this._inclusionHandler.findHandler(relation);
if (!handler) {
throw new Error('Fetch included items is not supported');
} |
||
| } | ||
|
|
||
| buildConstrainedWhere<TE extends Entity>( | ||
| ids: SID[], | ||
| whereFilter: Where<TE>, | ||
| relationDef: ResolvedRelationMetadata, | ||
| ): Where<TE> { | ||
| const keyPropName: string = relationDef.keyTo; | ||
| const where: AnyObject = {}; | ||
| where[keyPropName] = {inq: ids}; | ||
| return constrainWhere(whereFilter, where as Where<TE>); | ||
| } | ||
|
|
||
| getResolvedRelationDefinition(name: string): ResolvedRelationMetadata { | ||
| const relationMetadata: RelationMetadata = this.sourceRepository.entityClass | ||
| .definition.relations[name]; | ||
|
|
||
| switch (relationMetadata.type) { | ||
| case RelationType.hasMany: | ||
| return resolveHasManyMetadata(relationMetadata as HasManyDefinition); | ||
| case RelationType.belongsTo: | ||
| return resolveBelongsToMetadata( | ||
| relationMetadata as BelongsToDefinition, | ||
| ); | ||
| default: | ||
| throw new Error(`Unsupported relation type ${relationMetadata.type}`); | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From @bajtos
This block of code will be repeated a lot, I'd like to see a simpler solution, e.g. a sugar API building on top of
InclusionHandlerFactory.Ideally, the entire block should collapse to a single statement, e.g.
Under the hood, the helper should look up the definition of
todosrelation to learn about the target model,keyTo, and any other metadata needed. See how_createHasManyRepositoryFactoryForis implemented.Also based on the code in juggler, every relation needs a slightly different implementation of the inclusion handler. Please include an example/acceptance test showing how to include models via
belongsTorelation to ensure requirements of other relation types were considered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bajtos
I added the belongsTo handler and it does fetch the parent instance for the child. BUT there is a problem when construct the returned data:
The
Todomodel only has propertytodoListIdas the foreign key, but nottodoListas its parent property. Therefore given the objectmethod
toEntity(todo)won't convert propertytodoListand removes it in the constructed todo entity.