Closed mrvinceit closed 4 months ago
Hmm, think to achieve this you would need to overwrite the resolver and pass it to your service.
If you where to add @ResolveField
you can also get the parent, maybe this is something you can work with?
Thanks for that suggestion, if I'm understanding you correctly then this @ResolveField
is the method decorator available in NestJS. The issue here is that in the case of relations, you start introducing N+1 select problems.
I think what would really be helpful, is if the Filterable
interface was able to be extended to include a property that represents the @Info
args or extracts the fields at this level, which may make sense as additional context for filtering.
As well, I was thinking about possibly hacking We have to be doing this somewhere right? For example with Sequelize or Typeorm, where essentially the QueryService is dynamically creating a query to execute on the DB? CustomAuthorizer
or HookArgs
to pass this information down to the QueryService.
Maybe as a first step, where in the NestJS Query pipeline, could a such a thing be implemented?
EDIT: Answered my own question here -- this looks like it could be partially implemented by adding parameter decorators to the ReadResolverBase
class findById
and query
methods
@TriPSs, what are you thoughts on enhancing the Filterable
object with a Selection<DTO>
property vs adding an additional parameter to the QueryService getById
, findById
findRelations
, query
, and queryRelations
methods?
Could you elaborate a bit more for me? Would also like to understand the will allow users to dynamically create the query
part.
Interesting, my first instinct would be that this could partly be achieved by overwritting the queryRelations
in a query service:
public async queryRelations<Relation>(RelationClass: Class<Relation>, relationName: string, entities: RelationEntity[], query: Query<Relation>): Promise<Map<RelationEntity, Relation[]>> {
}
As you there have the relation's class, name and the entities to fetch the relation for.
Will look into this a bit more.
Yes but you don't know which fields are requested for the relation so you would have to fetch the whole object graph, which might include other relations that were not needed by the original request.
Additionally, depending on the relation, you don't have access to query variables that might be in use to return the correct nested object, so you would have to depend on @ResolveField
which could be very inefficient for some queries (Spoiler Alert: some of these inefficient queries are mine :sweat_smile: ).
Ex:
query GetUserSocialDetails($id: String, $paging: CursorPaging, $time: Period) {
account(id: $id) {
_id
messages(paging: $paging) {
totalCount
cursor
summary(time: $time) {
total
}
edges {
node {
_id
friendLikes {
edges {
cursor
node {
_id
summary(time: $time) {
total
}
}
}
}
}
}
}
}
}
In this query, messages
is a one-to-many relation that has a summary
property that has a $time
variable and a nested friendLikes
one-to-many object that also has a variable-dependent summary
object.
Ideally, I would like to query for the messages
and related summary
objects, in one join
query, but I can't because I don't have the $time
variable that is needed to get the summary
details over the period specified by the $time
parameter. Nor can I, at run-time, optionally query for the friendLikes
since I don't know if it was requested.
Instead, the summary
object won't get resolved until later and would require a bunch of costly lookups for each message
object. The problem becomes N*M times worse when trying to resolve the nested friendLikes
and its summary
object.
@TriPSs any thoughts yet?
Not yet sorry. Still have the page saved to read into it 😅
@TriPSs, sure no problem. I did want to try and test this locally to get a start on this, but I'm having issues building locally and getting our external project to consume the library, we tried publishing the fork to NPM with no luck. Do any suggestions come to the top of your mind?
Usually what I do (prop not the best way) is compile the app and copy the dist src to the node_modules of my own app.
I was reading a bit on this and just thought that we already have something similar to this which we can use as starting point for this feature.
In aggregate-query-param.decorator.ts we already have a decorator to get certain fields from the query, we could make maybe a similar decorator that will return the whole query instead of only parts.
Is that something useful for you? Maybe as a starting point?
So I looked at it briefly and it does look promising, ultimately the param decorator would need to be passed through to the Queryservice as a standard parameter for the Read/ReadRelation resolvers.
I was also thinking of maybe adding something like enableLookAhead
to the @Relation
decorator, when enabled it will then join and select that relation if it's fetched inside the query.
Out of curiosity, how are you finding most consumers of the package are hydrating relations?
Not sure that I understand what you mean?
Yeah, I realized I probably should clarify 😄
What I was asking was really: Are you finding that many consumers of this package are using the TypeORM, Sequelize, Mongoose features? And if so, how are relations handled out-of-the-box? My thought was that relations were already joined to the parent object in at least some of these cases (TypeORM maybe Sequelize, too?)
If I understand correctly: Yea a relation is already joined if its used in a filter, if you are not filtering on it it will not be used inside the query; this is also one of the reasons why I added this so existing ones are reused.
@mrvinceit FYI: I'm checking out to create the decorator that will expose the complete query parsed with graphql-parse-resolve-info, example of the data:
{
name: 'subTasks',
alias: 'subTasks',
args: { paging: { first: 10 }, filter: {}, sorting: [] },
fieldsByTypeName: { SubTaskConnection: { pageInfo: [Object], edges: [Object] } },
fields: {
pageInfo: {
name: 'pageInfo',
alias: 'pageInfo',
args: {},
fieldsByTypeName: [Object]
},
edges: {
name: 'edges',
alias: 'edges',
args: {},
fieldsByTypeName: [Object]
}
}
}
Thinking of then also creating some small utils to make reading it a bit easier, like doesQueryLoadRelation
and getQueryRelationParam
etc. What do you think of this?
After some testing and checking I think it's better to map it ourself since graphql-parse-resolve-info
also add's all the SubTaskConnection
things inside the sub fields.
Example of what you than could get back from the decorator:
{
"completedTodoItems": {
"name": "completedTodoItems",
"args": {
"filter": {
"id": {
"in": [
1,
2,
3
]
}
}
},
"fieldsByTypeName": {
"pageInfo": {
"name": "pageInfo",
"args": {},
"fieldsByTypeName": {
"hasNextPage": {
"name": "hasNextPage",
"args": {},
"fieldsByTypeName": {}
},
"hasPreviousPage": {
"name": "hasPreviousPage",
"args": {},
"fieldsByTypeName": {}
},
"startCursor": {
"name": "startCursor",
"args": {},
"fieldsByTypeName": {}
},
"endCursor": {
"name": "endCursor",
"args": {},
"fieldsByTypeName": {}
}
}
},
"edges": {
"name": "edges",
"args": {},
"fieldsByTypeName": {
"node": {
"name": "node",
"args": {},
"fieldsByTypeName": {
"id": {
"name": "id",
"args": {},
"fieldsByTypeName": {}
},
"title": {
"name": "title",
"args": {},
"fieldsByTypeName": {}
},
"completed": {
"name": "completed",
"args": {},
"fieldsByTypeName": {}
},
"description": {
"name": "description",
"args": {},
"fieldsByTypeName": {}
},
"age": {
"name": "age",
"args": {},
"fieldsByTypeName": {}
},
"tags": {
"name": "tags",
"args": {
"filter": {
"name": {
"like": "test"
}
}
},
"fieldsByTypeName": {
"edges": {
"name": "edges",
"args": {},
"fieldsByTypeName": {
"node": {
"name": "node",
"args": {},
"fieldsByTypeName": {
"id": {
"name": "id",
"args": {},
"fieldsByTypeName": {}
},
"name": {
"name": "name",
"args": {},
"fieldsByTypeName": {}
}
}
},
"cursor": {
"name": "cursor",
"args": {},
"fieldsByTypeName": {}
}
}
}
}
}
}
},
"cursor": {
"name": "cursor",
"args": {},
"fieldsByTypeName": {}
}
}
},
"totalCount": {
"name": "totalCount",
"args": {},
"fieldsByTypeName": {}
}
}
}
}
I'm still thinking of maybe making fieldsByTypeName
an array instead of a object, @mrvinceit what you think?
I agree in general, and would take it one step further in that we should transform this into a strongly typed object, similar to how we represent the Filter
type, i.e. Filter<Task>
For example, assume subtasks
property is an array of tasks
, and tasks
have the following form:
@ObjectType()
@FilterableCursorConnection('subtasks', () => Task, {
connectionName: 'TaskConnection',
...
})
export class Task {
@Field(() => ID)
id: string;
@Field()
name: string;
@FilterableField({ withArgs: true }) // example prop to tell transformer that this may have args or '@FilterableFieldWithArgs`
complexProp: ComplexObject;
@Field(() => [Task], { nullable: 'itemsAndList' })
subtasks?: Task[];
The example output from graphql-parse-resolve-info
I think gives too many details, and requires that the consumer be intimately aware of how to parse the ResolveInfo
object which if that is the case we don't gain much more than just the raw ResolveInfo
.
Simple is better here IMO, being able to access the info object like so:
const info: SelectionInfo<Task> = {}; // Still think Selection might be appropriate name 😃
info.id // `id` is nullable along with most fields since they may not be included in the request -- SelectionInfo
// `aliases` prop is populated only when the prop is aliased
info.id?.aliases // 'aliases' can be defined as SelectionInfoAlias[] or Record<string, SelectionInfoAlias>; note: SelectionInfoAlias == Omit<SelectionInfo, 'alias'>
info.id?.aliases?.taskId
info.id?.aliases["taskId"]
info.complexProp // -- SelectionInfoWithArgs
info.complexProp?.args // given the "withArgs" prop, 'args' is always available and can be defined as Args[] or Record<string, Args>
info.complexProp?.args.filter // id: { in: [1, 2, 3] } or id: "Prop2" // basically whatever the arg object is, return it here
info.complexProp?.args["filter"]
// I think the Connection Types should be reduced to prop similar to the 'complexProp'
info.subtasks ==> SelectionInfoWithArgs<Task>
info.subtasks // TaskConnection Object -- assume only 'name' and 'complexProp' are selected fields under the 'nodes' in request
info.subtasks?.args.paging // { first: 2, after: "abc123" }
info.subtasks?.args.filter // id: { in: [1, 2, 3] }
// getting the node props -- usage of these props are just like the examples provided earlier
info.subtasks?.name
info.subtasks?.complexProp
// with aliases
info.subtasks?.aliases["subTaskWithAlias"].args.paging // { first: 10, after: "xyz789" }
info.subtasks?.aliases["subTaskWithAlias"].args.filter // id: { in: [10, 20, 30] }
A thing to point out, I don't think we need to handle the Relation
cases beyond the examples above since we are already doing so with xxxRelations
methods, we just need to be able to have the context details (i.e. aliases, args) available in those methods so that we can modify the requests as needed.
Handling the Relation
cases would include passing the context details (i.e. aliases, args) typed object to appropriate xxxRelation
methods. e.g.
SelectionInfo | SelectionInfoWithArgs
SelectionInfoWithArgs
Admittedly, this is focused on the simple(-ish) scenarios, I don't know how this may need to be accommodated if things like Aggregate relations are in use, but I feel that it would probably work similarly to the Connection objects
EDITED: Included the nested node props of Connection object and clarified Connection object type
Currently have two interfaces for two different decorators:
@GraphQLLookAheadRelations
- Returns SelectRelation<DTO>[]
interface SelectRelation<DTO> {
name: string
query: Query<DTO>
}
Array of relations to that have enableLookAhead
on and are being queried
@GraphQLResolveInfo
- Returns QueryResolveTree<DTO> | GraphQLResolveInfo
(GraphQLResolveInfo
is from graphql
package)
interface QueryResolveTree<DTO> {
name: string
alias: string
args?: Query<DTO>
fieldsByTypeName: {
[key in keyof DTO]: QueryResolveTree<
// If the key is a array get the type of the array
DTO[key] extends ArrayLike<any> ? DTO[key][number] : DTO[key]
>
}
}
It's not completely done yet but the look ahead part is working, pushed all changes here,
@TriPSs , that's awesome I'll definitely take look. In the meantime, I look forward to your thoughts on what I proposing and if you think it makes sense
@mrvinceit looks very interesting, maybe an idea to put those a bit on the places inside the PR?
Related to what you said:
withArgs: true
, if this is enabled then always have args? If so I think that will complicated the parsing a bit since now (at-least how I have it in the PR now) it does not require any metadata about the DTO (only relations).I love the selectRelations
bit, and to answer your question, yes withArgs: true
would always have args alternatively there is a decorator option to subclass the decorator and define @FilterableFieldWithArgs
if that's easier (hacky I know...)
I also think that focusing solely on relations still leaves out the plain objects that may have args defined, for example in this query here: https://github.com/TriPSs/nestjs-query/issues/55#issuecomment-1282258758
summary
may not be defined as a relation but it still requires args, so it is useful to pull this from the ResolveInfo details
Currently there is always a args
but if the query did not contain any it will be empty, or is the idea the make it possible to define custom args there?
Yes, there will always be an args
but empty if it doesn't contain anything, same goes with aliases
I added the latest example of what the @GraphQLResolveInfo
outputs in the PR
Maybe that output is a good start point and then we could write some utils around it to maybe fetch specific data out of it? What do you think?
Actually, I did make it a point to differentiate, properties with args vs those without, though, in reality, it is not mandatory that they be different types especially if the work to make them distinct is too much.
I added the latest example of what the
@GraphQLResolveInfo
outputs in the PRMaybe that output is a good start point and then we could write some utils around it to maybe fetch specific data out of it? What do you think?
Definitely, I'll move the rest of the comments to the PR
Something to add when we finalise this is that we can also add looking ahead for for example totalCount
of the offset paging, that we can skip the count query if the field is not fetched.
That's interesting, what would that design look like?
If I understand you correctly you would want to be able to "lookahead" when the totalCount
field is requested, so that a separate call to count
and countRelations
doesn't require an additional execute to populate the values?
EDIT: I suspect I might have what you're thinking mixed up 🤔 ... Code sample perhaps?
Currently the totalCount
is fetched with a separate query, If we know the field is not requested we can just skip that query :)
This could be a nice optimisation after we implemented this feature.
So totalCount
is always fetched? I'm assuming this only applies to the TypeORM queryservice?
As far as I know yes
Oh, that sucks. I think that in order to incorporate this field, we would need to not filter out ConnectionType objects, mainly thinking about the pageInfo
object here. I don't think that's too hard, we can add a $pageInfo
object prop to a ConnectionType.
If totalCount
is requested, that just calls the count
method(s) on the queryservice, yeah? And when using offset paging TypeORM is constructioning a LIMIT clause (e.g. in the case of SQL), so the count returned by the initial query call would not equal the number of records returned (unless the data set fits within the number of records for a given pagesize) .
If
totalCount
is requested, that just calls thecount
method(s) on the queryservice, yeah?
Correct, and these sometimes take longer then the original query somehow, so not doing them when they are not requested is also a nice performance boost.
Hiya!! @TriPSs, I guess I'm back! I'm looking over the latest commits and it doesn't appear to resolve this issue.
I know that there was the desire to allow for "look ahead" for things like relations and total counts for relations and paging, but I think that the implementation that is here is missing some the key points, is there still room to review and resolve?
@mrvinceit could you elaborate a bit again on what you want?
Sure, what is needed is the query service to pass an object that includes the passed args, aliases, and fields that comes from the (graphql) query.
In summary, I would propose the following changes:
relations
property to selections
with the details of passed arguments, aliases, and selected fields (the fields would also represent the relations)Selections<DTO>
property in a manner similar to Filter<DTO>
enableLookAhead
to include 'metadataOnly' option (this is to provide the option to fetch relation data or just provide the metadata of relations without eagerly fetching)Detailed discussion below:
Currently, what is seen here with Query
object https://github.com/TriPSs/nestjs-query/blob/907148a6ff6581c4d43f9e1b9bcaf1efde4e16e0/packages/core/src/interfaces/query.inteface.ts#L43 is a nested Query object which contains information for the relations (I'm assuming this based on discussions that this populated in conjunction with "enableLookAhead"). The relations
property is basically a named Query<T>
and does not contain these important details that are necessary to get a complete context to fully support "looking-ahead".
This is not just a one-off need, it is applicable for all the supported packages as each (Typeorm, Mongoose, Sequelize, Typegoose) supports field aliasing and argument (query parameters) passing in the creation of their underlying queries. Here's a common example of this for a standard SQL query:
SELECT id, colA, colA as AliasedColumn,
FROM tableH
WHERE id = $1 // id is 1257
which generates an example result
id | colA | AliasedColumn |
---|---|---|
1257 | sid-1257 | sid-1257 |
Additionally, the need for selected fields is necessary as a nested type in the gql query may not be defined as a relation but a complex object type. I have provided an example of this here https://github.com/TriPSs/nestjs-query/issues/55#issuecomment-1282258758, but I can provide further examples if that helps.
Might I suggest extending the enableLookAhead
prop to include the values: true | false | "metadataOnly"
?
This "metadataOnly" would support passing the relation info without the side effect of the implementing libraries actually querying for the data, I believe this to be more flexible, while not negating all the great work you put into the feature so far.
It would be great to have these rounded out, and I will be glad to provide help where I can.
I already did a small refactor to the decorator so I could properly use it for #151. We could pass that whole thing down now instead of only the relations, that way we can keep using the relations
for the lookahead and users who overwrite the query service can than also use the info
.
Would that solve your issue?
Yeah, I think that at a minimal level that would solve the issue.
I'm also curious about your thoughts regarding my changes, in particular the first point, since it seems that you suggesting keeping relations
and adding info
. Is there a scenario that you see where merging the two props would not be effective?
In summary, I would propose the following changes:
- revert the
relations
property toselections
with the details of passed arguments, aliases, and selected fields (the fields would also represent the relations)- provide typed
Selections<DTO>
property in a manner similar toFilter<DTO>
- extend
enableLookAhead
to include 'metadataOnly' option (this is to provide the option to fetch relation data or just provide the metadata of relations without eagerly fetching)
Personally I like to have a flat relations
who can be looked ahead as it makes that implementation a bit easier instead of reading it from selections which contains the whole query data.
With the implementation of #151 the relations data will be empty if no relations had the enableLookAhead
on.
Will check if I can implement this in #151 soon than we can always go from there.
Side note, I would suggest refactoring out enableLookAhead
and withDeleted
and making them a separate decorator options for TypeORM-related entities and/or the other implementations. Unlike the other query service and relation options which are implemented at the base nestjs-query-core level, these are very specific to TypeORM and think that is contributing to the differing applications of the selections
vs relations
Those can (eventually should) be implemented in the other packages, but since I only use TypeORM that is where my main focus is.
Personally I like to have a flat
relations
who can be looked ahead as it makes that implementation a bit easier instead of reading it from selections which contains the whole query data.With the implementation of #151 the relations data will be empty if no relations had the
enableLookAhead
on.Will check if I can implement this in #151 soon than we can always go from there.
If I'm understanding the implementation correctly of the relations
prop, it's only flat at the direct child prop level, as in relations of relations would not show up under the relations
prop correct?
And I understand your preference, though I think it is something that can be implemented with the TypeORM package from the jump, where you can keep the enableLookAhead
option and implement the benefits of relations
prop with the query build objects within the TypeORM implementation, using the passed in selectioninfo
prop to create the relations
object as is done currently.
Hope that last part made sense 😄
If I'm understanding the implementation correctly of the relations prop, it's only flat at the direct child prop level, as in relations of relations would not show up under the relations prop correct?
Enable look ahead now only goes one relation deep, but for relations that cannot be fetched with look ahead (one -> many) inside the relations resolver of the many relation it again gets the relations that it can look ahead. So currently flat is also working for the deeper relations.
And I understand your preference, though I think it is something that can be implemented with the TypeORM package from the jump, where you can keep the enableLookAhead option and implement the benefits of relations prop with the query build objects within the TypeORM implementation, using the passed in selectioninfo prop to create the relations object as is done currently.
Hope that last part made sense 😄
This does makes sense but eventually I would like to support this for the other packages to, having it already there makes it (I think) a bit easier for the other packages to also implement it.
Ok yeah, that's what I thought, and it completely makes sense.
With that being the case, I would think it'd be easier to put this work which is basically createLookAheadInfo<DTO>(relations, simplifiedInfo)
in the TypeORM builder, I'm thinking somewhere around here: https://github.com/TriPSs/nestjs-query/blob/e4713a99d943cef4e69d89b31e002d6bad7910b8/packages/query-typeorm/src/query/filter-query.builder.ts#L92
On the plus side, it would make the refactoring efforts down the road a bit easier as well.
I'm not 100% sure that is possible mainly because of this part: https://github.com/TriPSs/nestjs-query/blob/e4713a99d943cef4e69d89b31e002d6bad7910b8/packages/query-graphql/src/decorators/graphql-resolve-info.decorator.ts#L19-L22
We need the info about the relations which we have to look ahead.
I would like to propose that we finish #151 with the added changes that we are renaming info
to selections
and passing that also down to the query service so it could be used, than we can discuss on how to better improve this feature. What do you think of that?
My main reason for that being that #151 is also already open for to long and I want to prevent doing more in it which will then cause for it to be open longer.
Note: I do highly appreciate these discussions to make this package better 😄, thanks for your involvement ❤️
Is it possible to get the
parent
andinfo
resolver args from the CRUD resolver without too much hassle or code rewrite?The particular case here, we're hoping from a means of passing
parent
andinfo
resolver args in the CRUD DTOs, beause this will allow users to dynamically create the query