-
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] Explore the way to resolve inclusion property #2152
Comments
As I understand LB4 design, the Model class is describing shape of the data only, it does not deal with obtaining that data (fetching it from a database or resolving a model relation). It's the Repository that deals with resolving model relations and fetching related data. We are following this design in HasMany/BelongsTo relation too. You can think of I feel that your proposal proposal to move resolvers to model classes is violating this design constraint and we should investigate different approaches. Re-posting the code sample from the problematic proposal: import { TodoList } from './TodoList';
class Todo extends Entity {
@inclusion()
TodoList: ()=>TodoList
} |
@jannyHou , could you please add the acceptance criteria? Thanks! |
Relevant discussions: #2442 #2256 Reposting #2442 (comment)
Here is one reason why we should not mix the property and the relation decorator:
For long term, I would like to propose following API at conceptual level (implementation details will be most likely different): // Relations are defined at model-level
@belongsTo(() => AddressBook)
@model()
class Address extends Entity {
// the foreign key is defined explicitly
@foreignKey(() => AddressBook, 'id')
@property({
length: 36,
postgresql: {
dataType: 'uuid',
},
})
addressBookId: string;
}
// A custom class or interface describes format
// of data returned by a query
@model()
class AddressWithRelations extends Address {
@property()
addressBook?: AddressBook;
}
// Repository API
class DefaultCrudRepository {
find(filter: Filter<Address>): Promise<AddressWithRelations>;
create(data: Address): Promise<Address>;
// ...
} |
The part @model()
class Product {
@property()
name: string;
}
@model()
class Category {
@property()
name: string;
}
// tslint:disable-next-line:no-any
type Include<Target, As extends keyof any> = {
readonly [P in As]?: Target
};
type CategoryQuery = Category & Include<ProductQuery[], 'products'>;
type ProductQuery = Product & Include<CategoryQuery, 'category'>; TypeScript is returning the following errors:
The following definition works well:
Personally, I like this approach better because it's easier to understand. Regarding Please note that we need multiple types to describe mutations.
Now that we know how to deal with types, we need to figure out how to generate OpenAPI schema for these different models. That's out of scope of this spike though, see #1722 and related issues. |
In 312be08 (see https://github.com/strongloop/loopback-next/tree/spike/relation-inclusion-properties-as-model), I looked into approach based on defining a new model class for model with relation links. @model()
export class TodoListImage extends Entity {
// ...
}
@model()
export class TodoListImageWithRelations extends TodoListImage {
@property(() => TodoList)
todoList?: Value<TodoList>;
} While this approach makes it easy to generate OpenAPI spec (once we fix our generator to correctly handle circular references), it's not great in many other aspects.
Next, I'd like to investigate approach where the relation links are described in an interface and JSON schema is built from relation metadata. |
I am quite pleased with the results, see Now I need to write some dev documentation & describe the proposed changes, to make the PoC code ready for review. |
Opened a pull request showing a PoC and creating space for further discussion: #2592 |
I am closing the spike as done. Cross-posting #2592 (comment) I have converted Inclusion of related models #1352 into an Epic and added the following new stories:
|
Hi @bajtos , is released relation link feature? We are waiting to use this feature. |
@AnanthGopal not yet. We need to implement the issues listed in my previous comment, plus any additional items discovered by #2634. Would you like to help us and contribute some of the work needed to make this feature happen? We are happy to help you along the way. See https://loopback.io/doc/en/contrib/index.html and https://loopback.io/doc/en/lb4/submitting_a_pr.html to get started. |
Description / Steps to reproduce / Feature proposal
A follow-up story of the relation traverse spike #1952
The previous spike story turns out that we could improve our design of describing&defining model properties.
Copied the discussion and proposal from it:
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:
And a overall design question to consider (from @bajtos ) :
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 @Property metadata. If we choose a different approach for describing model+relations in TypeScript, then we also need a new way how to obtain JSON Schema for such new types.
Here is a proposal of the next steps:
The text was updated successfully, but these errors were encountered: