Passing Graphql query resolver args as part of call to QueryService

[tripss]

Passing Graphql query resolver args as part of call to QueryService

[TriPSs]

Those can (eventually should) be implemented in the other packages, but since I only use TypeORM that is where my main focus is.

[TriPSs]

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?

[mrvinceit]

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 CustomAuthorizer or HookArgs to pass this information down to the QueryService. 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?

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

[mrvinceit]

@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?

[TriPSs]

Could you elaborate a bit more for me? Would also like to understand the will allow users to dynamically create the query part.

[mrvinceit]

Sure, this would provide users with the ability to "look-ahead", so they can use the GQL ResolveInfo object to design better queries.

This is something largely available with Graphile and the look-ahead functionality is discussed here

Snippet:

[TriPSs]

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.

[mrvinceit]

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 😅 ).

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.

[mrvinceit]

@TriPSs any thoughts yet?

[TriPSs]

Not yet sorry. Still have the page saved to read into it 😅

[mrvinceit]

@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?

[TriPSs]

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.

[TriPSs]

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?

[mrvinceit]

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.

[TriPSs]

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.

[mrvinceit]

Out of curiosity, how are you finding most consumers of the package are hydrating relations?

[TriPSs]

Not sure that I understand what you mean?

[mrvinceit]

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?)

[TriPSs]

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.

[TriPSs]

@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?

[TriPSs]

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?

[mrvinceit]

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.

• findRelation ==> would possibly have SelectionInfo | SelectionInfoWithArgs
• queryRelations ==> would always have 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

[TriPSs]

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,

[mrvinceit]

@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

[TriPSs]

@mrvinceit looks very interesting, maybe an idea to put those a bit on the places inside the PR?

Related to what you said:

• I'm not quite sure I understand 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).

[mrvinceit]

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: #55 (comment)

summary may not be defined as a relation but it still requires args, so it is useful to pull this from the ResolveInfo details

[TriPSs]

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?

[mrvinceit]

Yes, there will always be an args but empty if it doesn't contain anything, same goes with aliases

[TriPSs]

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?

[mrvinceit]

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.

[mrvinceit]

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?

Definitely, I'll move the rest of the comments to the PR

[TriPSs]

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.

[mrvinceit]

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?

[TriPSs]

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.

[mrvinceit]

So totalCount is always fetched? I'm assuming this only applies to the TypeORM queryservice?

[TriPSs]

As far as I know yes

[mrvinceit]

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.

[mrvinceit]

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) .

[TriPSs]

If totalCount is requested, that just calls the count 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.

[mrvinceit]

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?

[TriPSs]

@mrvinceit could you elaborate a bit again on what you want?

[mrvinceit]

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:

• revert the relations property to selections 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 to Filter<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)

Detailed discussion below:

Currently, what is seen here with Query object

nestjs-query/packages/core/src/interfaces/query.inteface.ts

Line 43 in 907148a

relations?: SelectRelation<DTO>[]

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 #55 (comment), 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.

[TriPSs]

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?

[mrvinceit]

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 to selections 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 to Filter<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)

[TriPSs]

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.

[mrvinceit]

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

[mrvinceit]

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 😄

[TriPSs]

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.

[mrvinceit]

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:

nestjs-query/packages/query-typeorm/src/query/filter-query.builder.ts

Line 92 in e4713a9

public select(query: Query<Entity>): SelectQueryBuilder<Entity> {

On the plus side, it would make the refactoring efforts down the road a bit easier as well.

[TriPSs]

I'm not 100% sure that is possible mainly because of this part:

nestjs-query/packages/query-graphql/src/decorators/graphql-resolve-info.decorator.ts

Lines 19 to 22 in e4713a9

	// Get all relations that have look ahead enabled
	const relations = getRelationsDescriptors(DTOClass).filter(
	(relation) => relation.relationOpts.enableLookAhead && !relation.isMany
	)

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 ❤️

[mrvinceit]

I can get on board with that, I think that with #151 bringing in the selection object will definitely do this some justice.

It would be really great to address this at the same time as well:

• provide typed Selections<DTO> property in a manner similar to Filter<DTO>

I left some notes (#55 (comment)) on this previously, and it looks like I still have the draft of this implementation #71 😅

And thank you, I appreciate these discussions as well, this has been really a great package, and definitely many props to you for bringing it so far.