MichalLytek / type-graphql

Create GraphQL schema and resolvers with TypeScript, using classes and decorators!
https://typegraphql.com
MIT License
7.98k stars 674 forks source link

A `FieldResolver` with `args` is executed later #1711

Closed ZvikaZ closed 2 weeks ago

ZvikaZ commented 2 weeks ago

Describe the Bug I have a Resolver with 3 FieldResolvers. In the query I call the Resolver twice. If all the FieldResolvers are without args, I see the 3 FieldResolvers called one after other, and then again - which makes perfectly sense to me - let's finish with one query, with all its fields, and then the next query. However, if one of the FieldResolver has args, I see the other two FieldResolvers called, twice, and only then the last FieldResolver will be called at the end.

To Reproduce

import {Resolver, Query, FieldResolver, Root, Args, ArgsType, Field} from "type-graphql";
import {Parent} from "../entities/Parent";

@ArgsType()
class MyArgs {
  @Field(() => Number)
  num?: number;
}

@Resolver(of => Parent)
export class ParentResolver {
  @Query(() => Parent)
  parent(
    @Args(() => MyArgs) args: MyArgs,
  ): Parent {
    console.log("parent");
    return {}
  }

  @FieldResolver(() => String)
  f1(
    @Root() parent: Parent,
    @Args(() => MyArgs) args: MyArgs,
  ): String {
    console.log("f1");
    return "first"
  }

  @FieldResolver(() => String)
  f2(@Root() parent: Parent): String {
    console.log("f2");
    return "second"
  }

  @FieldResolver(() => String)
  f3(@Root() parent: Parent): String {
    console.log("f3");
    return "third"
  }

}

With the query:

query {
  p1: parent(num:1) {
    f1(num: 3)
    f2
    f3
  }
  p2: parent(num:2) {
    f1(num: 4)
    f2
    f3
  }
}

Expected Behavior I expected to see in the logs execution of f1, f2 and f3 (maybe in some other order), and then again the three of them.

Instead, we see f2 and f3, and then again f2 and f3, and only then f1 (twice)

Logs Problem:

parent
parent
f2
f3
f2
f3
f1
f1

If I remove the args, I get the expected result:

parent
parent
f1
f2
f3
f1
f2
f3

Environment (please complete the following information):

Additional Context I have checked it with pure GraphQL, without type-graphql, and I don't see this problem. It's interesting (even thought I'm not sure relevant), that it's a third behavior (f1 is together with f2 and f3, but second parent is only after all first query has finished).

That's the pure GraphQL log:

parent { num: 1 }
f1 { num: 3 }
f2
f3
parent { num: 2 }
f1 { num: 4 }
f2
f3

Thanks.

MichalLytek commented 2 weeks ago

When you use arg, it may trigger the async pipeline, hence your class method is called asyncronusly, after the sync one and the next tick of the event loop.

ZvikaZ commented 2 weeks ago

The code I wrote is a toy example. In my real world code, there are 50,000 instances of parent. I see in the logs 50K calls to f2 and f3, which span over 3 seconds, and only then f1 starts. However, f1 calculation takes 5 seconds, and is the bottleneck of the query, causing it to take ~8 seconds.

If f1 started together with f2 and f3, I could have saved those 3 seconds, and the query would have taken only ~5 seconds.

Is there something that could be done to improve this situation?

Is it normal that it takes 3 seconds just to span all 50*2K FieldResolvers? Or maybe it indicates that something is wrong with my code?

MichalLytek commented 2 weeks ago

Yes, it is normal that processing large amount of data takes some time. If they are synchronous operations, you would have no benefit or processing the f1 before or at the same time.

ZvikaZ commented 2 weeks ago

Well, I've mentioned f1 calculation, but it wasn't accurate term - it should have been f1 evaluation. Most of these operations are accessing DBs.

Anyway, I've bypassed this by removing the args from f1 (and hardcoding the desired values to f1), and indeed, it helped. It's a workaround for now, but not very scalable...

It'd be great if you consider fixing this. (or at least, letting know if a PR is welcomed, if someone will want to address this).

MichalLytek commented 2 weeks ago

I'm afraid that's how node.js works. If you have sync operations pending, async promise callback is not executed until the job is finished. TypeGraphQL have middlewares, auth guards, args validations, etc. so that's why it needs the async pipeline for field resolvers with args. Do you use args validation?

Maybe you can try to speed up scheduling db calls by wrapping the sync field resolvers with setImmediate so it goes to the event loop queue and gives a chance to execute the f1 field resolver.

ZvikaZ commented 2 weeks ago

I'm not using args validation. I will try using setImmediate, thanks for the advice!

Regarding the time that it takes to spawn all the fieldresolvers, I've mentioned earlier that it takes 3 seconds. Meanwhile, I've tried with other query, that added more fields that are resolved by the regular query, before calling the fieldresolvers. The strange thing is that now spawning the same 3 fieldresolves for the same 50K instances takes about 6 seconds, twice than before.

Any suggestions how can I improve this?