-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Spike] feat: inclusion #2124
Conversation
From @bajtos
Good catch, can you work with @raymondfeng to improve the type definition please? We should eventually take a look at the JSON Schema/OpenAPI parameter schema emitted for 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. |
@@ -44,6 +44,10 @@ export class TodoListRepository extends DefaultCrudRepository< | |||
'image', | |||
todoListImageRepositoryGetter, | |||
); | |||
this._inclusionHandler.registerHandler<Todo, typeof Todo.prototype.id>( |
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.
this._registerInclusion('todos', todoRepositoryGetter);
Under the hood, the helper should look up the definition of todos
relation to learn about the target model, keyTo
, and any other metadata needed. See how _createHasManyRepositoryFactoryFor
is 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 belongsTo
relation 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.
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 Todo
model only has property todoListId
as the foreign key, but not todoList
as its parent property. Therefore given the object
const todo = {
id: 1,
title: 'a todo item',
todoListId: 1,
todoList: {
id: 1,
color: red
}
}
method toEntity(todo)
won't convert property todoList
and removes it in the constructed todo entity.
@bajtos Thank you for the review!
Good stuff, I didn't realize the SELECT N+1 issue before. I changed the handler function to take in an array of fks. The PoC PR implements the inclusion handler in Inclusion FilterThe Filter interface is not designed to describe related entities, see its definition(and Inclusion's definition). I was thinking of modify the
But after I found 👇 Relation propertyLet's continue the above topic. Currently adding a hasMany relation is not essentially equivalent to adding a belongsTo relation in terms of reshaping a model:
Which means a Todo model cannot describe an instance that includes its parent like {
id: 1,
color: red,
todoListId: 1,
todoList: {
id: 1,
desc: 'a todo list'
}
} Is it restricted by the circular model reference issue? If that's the case, I assume simply add a relation property Think it from a more general level, probably we could isolate the relation traverse from entity inclusion:
Repository level relationSeems we preserved the relation definition in model definition for the purpose of supporting the LB3 inclusion. But if we decide to abandon the juggler inclusion system, as I explained above, we don't necessary to keep model level relation decorators anymore. |
this._handlers[relationName] = fetchIncludedItems; | ||
const self = this; | ||
|
||
async function fetchIncludedItems( |
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.
It can be simplified as:
this._handlers[relationName] = async (
fks: SID[],
filter?: Filter<TE>,
): Promise<TE[]> => {...}
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.
Maybe we should define a type such as:
export type IncludedItemsFetcher<SID, TE> = (
fks: SID[],
filter?: Filter<TE>,
) => Promise<TE[]>
// SID: the ID of source entity | ||
// TID: the ID of target entity | ||
|
||
export class InclusionHandler<SE extends Entity, SID> { |
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.
The name is a bit confusing as InclusionHandler
contains a list of _handlers
.
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.
yeah it should be plural
`The inclusion handler for relation ${relationName} is not found!` + | ||
`Make sure you defined ${relationName} properly.`; | ||
|
||
return this._handlers[relationName] || new Error(errMsg); |
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.
It's suspicious to return an Error
but later on it checks:
const handler = this._inclusionHandler.findHandler(relation);
if (!handler) {
throw new Error('Fetch included items is not supported');
}
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.
yeah the code here should definitely be refined ^
`The inclusion handler for relation ${relationName} is not found!` + | ||
`Make sure you defined ${relationName} properly.`; | ||
|
||
return this._handlers[relationName] || new Error(errMsg); |
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.
It's suspicious to return an Error
but later on it checks:
const handler = this._inclusionHandler.findHandler(relation);
if (!handler) {
throw new Error('Fetch included items is not supported');
}
_handlers: {[relation: string]: Function} = {}; | ||
constructor(public sourceRepository: DefaultCrudRepository<SE, SID>) {} | ||
|
||
registerHandler<TE extends Entity, TID>( |
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.
I like the idea to delegate resolution of inclusion
to a list of functions.
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.
I think we can borrow some ideas from https://graphql.org/learn/execution/. It will allow us even define custom relations.
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.
Good catch! When building belongsTo relation, I had a gut feeling that it's not a good idea to decorate the foreign key (e.g. I think the current design is a workaround for TypeScript limitation explained in microsoft/TypeScript#27519 A possible solution is to define a type wrapper that will prevent the circular reference problem at the cost of losing design: type metadata (the metadata will say "Object" instead of the actual type). export type Related<T> = T | undefined;
class Todo extends Entity {
// ...
todoList: Related<TodoList>;
}
class TodoList extends Entity {
// ...
todos: Related<Todo>;
} Note: AFAICT, it's important to use a named type. Replacing For longer term, I'd like to change the way how relations are defined: relation decorators provide relation metadata only, foreign keys are defined explicitly. For example: class Todo extends Entity {
// ...
@foreignKey(() => TodoList, 'id') // target model, target property
@property({type: 'number', required: true})
todoListId: number;
@belongsTo(()=> TodoList)
todo: Related<Todo>;
} |
One more idea to consider - perhaps we can define two model classes, the first describing only the properties actually stored in the database, the second including relations? class Customer extends Entity {
// ...
}
class CustomerWithRelations extends Customer {
orders?: Order[]
} Create/update methods will greatly benefit from this distinction:
|
Thanks for all the feedback! I understand that the best outcome of a spike is a concrete solution, but I am afraid the inclusion story is more complicated than we expect, and it would be better to create separate stories to explorer different potential approaches. Like what @raymondfeng points out, GraphQL's query system has two features we could borrow:
Here is a proposal of the next steps:
|
import { TodoList } from './TodoList';
class Todo extends Entity {
@inclusion()
TodoList: ()=>TodoList
} How do you envision serializing such structure to JSON, for transmission over HTTP?
I like that approach! Although I am not sure how will Also I think you need to use type TodoQuery = TodoMutation & Inclusion<Todo> |
Good catch!
That's something I am trying out on local but haven't got it work :(
Hmm could you elaborate more about it? Do you mean infer the JSON schema of a model? |
IIUC your proposal, when we create an instance of When creating the HTTP response body, we want JSON data of the related TodoList instance, e.g. {
id: '1',
desc: 'todo description',
TodoList: {
id: '1',
title: 'list name',
}
} My question is how do you envision converting the value from a resolver function into a JSON object? Maybe I misunderstood your proposal? |
@bajtos good question. The resolver function should call But in terms of how we build the type to describe {
id: '1',
desc: 'todo description',
TodoList: {
id: '1',
title: 'list name',
}
} That's also my concern 😞 and I am not even sure is it possible to build such a type at run time. |
I really like the last two proposals from #2124 (comment) (so if we create the follow up stories and had to prioritize them, I'd like to see those two higher in the list) and the idea of moving definition of relations to repository level. |
Another aspect to consider: how are we going to generate JSON Schema and OpenAPI schema for payloads containing both model properties (Customer's id, name, email) and data of related model (Customer's orders, addresses, etc.). At the moment, we are leveraging |
Hi Guys, Did you find any solution for the related model issue? |
Related to #1952
Copy the discussion here:
This PR is a PoC of the inclusion handler proposed by @batjos in comment.
Takes
findById
as an example method, after adding the code that fetches the included items,TodoList.findById(1, {include: [{relation: 'todos'}]})
returnsThe PoC contains:
A
InclusionHandlerFactory
is created to return a handler function that fetches the included items.When a relation is created, a corresponding handler will be registered in the source repository.
TBDDONE: retrieve the relation metadata inInclusionHandlerFactory
function:TBD(improve)DONE: Turn the factory to a class, each repository has aInclusionHandler
class instance as property:this._inclusionHandler.register<TargetEntity>(targetRepositoryGetter)
to register the relation handler.this._inclusionHandler.searchHandler(relationName)
for each entry in the filter.include.I also discovered a few problems worth discussion:
The
Filter
interface is not designed to describe related entities, see its definition(andInclusion
's definition).I feel the inclusion handler's implementation would have overlapping code with relation repository's find method, I will investigate how to leverage those relation repositories when register the handler.