Skip to content
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

Relation between types in resolvers - use an existing resolver to resolve a field #168

Closed
shlomokraus opened this issue Oct 14, 2018 · 14 comments
Labels
Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request

Comments

@shlomokraus
Copy link

What I am trying to achieve is similar to addRelation in graphql-compose (https://graphql-compose.github.io/docs/en/basics-relations.html).

Given those types who create a circular refrences:

@ObjectType()
export class Job implements IJob {
  @Field() id!: string;

  @Field() name!: string;

  @Field(type => [User])
  owners: IUser[];
}

  @ObjectType()
  export class User implements IUser {
    @Field() id!: string;

    @Field() name!: string;
  }

 @Resolver(User)
 export class UserResolver {
 // .....
 }

@Resolver(Job)
export class JobResolver {
 // .....
 }

So, given this query:

job(id: "abc") {
 owners {
    name
   }
}

Graphql will use UserResolver to resolve owners field. I know I can use FieldResolver, but then I will have to write another implementation for resolving User, while itis already existing in UserResolver. Also, I will not be able to make circular dependencies.

How should I approach it?

@MichalLytek
Copy link
Owner

but then I will have to write another implementation for resolving User, while itis already existing in UserResolver.

All common implementation should be moved to the service, there's no need for any weird pattern.

@Resolver(User)
class UserResolver {
  constructor(protected readonly userService: UserService) {}

  @Query(returns => [User])
  getUsers(@Args() args: GetUserArgs) {
    return this.userService.getUsers(args);
  }
}

@Resolver(Job)
class JobResolver {
  constructor(protected readonly userService: UserService) {}

  @FieldResolver(returns => [User])
  owners(@Args() args: GetUserArgs) {
    return this.userService.getUsers(args);
  }
}

@MichalLytek MichalLytek added the Question ❔ Not future request, proposal or bug issue label Oct 15, 2018
@shlomokraus
Copy link
Author

Wouldn't it be easier to use a decorator to mark that owners should use the "getUsers" resolver from UserResolver? This is still code duplications because if the service signature changes, or if it is more complicated and requires dependencies, they would need to be passed around.

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 15, 2018

Wouldn't it be easier

No, it's not intuitive to bind a query as a field resolver with graphql signatures and implementation.

This is still code duplications because if the service signature changes,

This service method should be strictly coupled with GetUserArgs args shape, like:

class CommonUserResolver {
  getUsersByArgs(args: GetUserArgs) {
    // ...
  }
}

You can then easily add more fields, remove field, change graphql type or whatever in GetUserArgs class, and there's nothing to change in getUsers or owners code, only in the UserService (CommonResolver).

or if it is more complicated and requires dependencies, they would need to be passed around.

You can just inject your dependencies to UserService, no need to pass it by args from different resolver.

I see you've encountered a huge barrier in thinking in dependency-injection, class-based way, rather than dynamic, JS spaghetti code 😃

BTW, my primary doubt about your problem is that it's invalid. Query getUsers should return a list of all users, although owners should return the list of users, but filtered by a relation with job. So it's not 1:1 identical resolver and can't be just copied. Right?

@shlomokraus
Copy link
Author

You can just inject your dependencies to UserService, no need to pass it by args from different resolver.

I was referring to a case where there might be multiple fields that require external multiple external services, so you need more than just one service. This will require each resolver to know the implementation details of the fields, instead of just knowing the query that abstracts them.

The pattern is not something I came up with, you can see that it is implemented in graphql-compose as it makes sense in a graph structure - define resolvers for basic types and link them together to create the graph. This is also similar to populate in mongo or join in sql.

The starting point of that thought is that User and Job are independent entities and shouldn't have any idea of how they are being resolved. I agree that it is not 1:1, that's why you need to define the link, so owners is linked to query getUsers(ids: string[]) where ids is the value of Job.owners.

This is something that can easily be described in a decorator, something like:

@Relation("getUsers")

and the value that is passed is the field value, or an updated custom projector.

@MichalLytek
Copy link
Owner

it makes sense in a graph structure - define resolvers for basic types and link them together to create the graph

I completely agree with that. But I don't agree with the proposed API and hiding complicated magic behind @Relation decorators that is stil not enough to express and resolve this relation.

So, instead of pointing one resolver to another to share the resolve code like in graphql-compose, it's better to extract the common logic to "resolvers" services that provide perfect reusability and clear separation. Also, the prepareArgs step can be done in method body of the field resolver. It's cleaner and more object-oriented way that TypeGraphQL tries to follow.

Let me show you an example with the post and author relation from graphql-compose example. Starting with creating proper object types for our models:

@ObjectType()
class Author {
  @Field(type => ID)
  id: string;

  @Field({ nullable: true })
  firstName?: string;

  @Field({ nullable: true })
  lastName?: string;

  @Field(type => [Post])
  posts: Post[];
}

@ObjectType()
class Post {
  @Field(type => ID)
  id: string;

  @Field()
  title: string;

  @Field()
  votes: number;

  @Field()
  authorId: number;
}

And creating proper args types:

@InputType()
class PostsFilterInput {
  @Field({ nullable: true })
  createdAtMin: Date;

  @Field(type => Int, { nullable: true })
  votesMin?: number;

  @Field(type => ID, { nullable: true })
  authorId?: string;

  @Field(type => ID, { nullable: true })
  reviewerId?: string;
}

@ArgsType()
class PostsArgs {
  @Field(type => Int, { nullable: true })
  limit = 10;

  @Field(type => Int, { nullable: true })
  skip?: number;

  @Field()
  filter: PostsFilterInput;
}

We can extract DB calls and other business logic to the "resolver" service:

@Service()
class PostService {
  getPosts({ filter, limit, skip = 0 }: PostsArgs): Promise<Post[]> {
    const criteria: any = {};
    if (filter) {
      if (filter.createdAtMin) {
        criteria.createdAt = { $gt: filter.createdAtMin };
      }
      if (filter.votesMin) {
        criteria.votes = { $gt: filter.votesMin };
      }
      if (filter.authorId) {
        criteria.authorId = filter.authorId;
      }
      if (filter.reviewerId) {
        criteria.reviewerId = { $has: filter.reviewerId };
      }
    }
    return DB.Posts.find(criteria)
      .limit(limit)
      .skip(skip);
  }
}

Then we can create our GraphQL resolvers. Both will have this service injected in constructor:
constructor(protected readonly postService: PostService) {}
and use the PostsArgs type. But posts field resolver of author will prepare the args before calling the service, based on it's context (root/parent value):
args.filter.authorId = author.id

So all in all, sharing resolvers is as simple as that:

@Resolver(of => Author)
class AuthorResolver implements ResolverInterface<Author> {
  constructor(protected readonly postService: PostService) {}

  @FieldResolver()
  posts(@Root() author: Author, @Args() args: PostsArgs) {
    // preparing your args based on context
    args.filter.authorId = author.id;
    // and calling the common resolver service
    return this.postService.getPosts(args);
  }
}

@Resolver()
class PostResolver {
  constructor(protected readonly postService: PostService) {}

  @Query(returns => [Post])
  getPosts(@Args() args: PostsArgs) {
	// root query - not preparing, just calling the service
    return this.postService.getPosts(args);
  }
}

So the resolvers are responsible for mapping inputs/args to correct values and common services allows to share implementation of common business logic. There's no need for special syntax sugar in cases like this one.

@shlomokraus Is this now a bit more clear? 😉

@MichalLytek MichalLytek added Community 👨‍👧 Something initiated by a community Discussion 💬 Brainstorm about the idea labels Oct 25, 2018
@shlomokraus
Copy link
Author

This is what we started doing, but it still raised some worries. We faced the same problem since we had to call this.postService.getPosts(args); from two different places, so again a resolver class of one type, is calling a service of a different type. Extracting the reference logic was also problematic as you've mentioned, so I took your idea, but with an extra step.

What we eventually did, and I'd like to hear your thoughts about it, is breaking the resolvers to Query and Field resolvers and then injecting query resolvers to different field resolvers. It is the same as the service example, only that it is more coupled.
The reason we broke the resolvers was to prevent circular dependencies since a field resolver only references another resolver, while a resolver never references a field resolver.

A modified example with a circular graph:

Types:

@ObjectType()
class Author {
  @Field(type => ID)
  id: string;

  @Field({ nullable: true })
  firstName?: string;

  @Field(type => [Post])
  posts: IPost[];
}

@ObjectType()
class Post {
  @Field(type => ID)
  id: string;

  @Field()
  title: string;

  @Field(type=>Author)
  author: IAuth;
}

This would break down to 4 resolvers:

2 Base resolvers:

@Resolver(of=>Author)
class AuthorQueryResolver {
   @Query(returns=>Author)
   async getAuthor(@Arg("id") id) { /* get author from db or micro service */ }

   @Query(returns=>Author)
   async getAuthorByPost(@Arg("postId") postId) { /* get author from db or micro service */ }
} 

@Resolver(of=>Post)
class PostQueryResolver {
   @Query(returns=>[Post])
   async getPostsByAuthor(@Arg("authorId") authorId) { /* get posts from db or micro service */ }
} 

2 Fields resolvers:

@Resolver(of=>Author)
class AuthorPostsFieldResolver {

   constructor(@inject(PostResolver) private readonly postResolver: PostResolver){}

   @FieldResolver(returns=>[Posts])
   async posts(@Root() author: IAuthor) {
      return this.postResolver.getPostsByAuthor(author.id);
   }
} 

@Resolver(of=>Author)
class PostAuthorFieldResolver {

   constructor(@inject(AuthorResolver) private readonly authorResolver: AuthorResolver){}

   @FieldResolver(returns=>Author)
   async author(@Root() post) { 
      return this.authorResolver.getAuthorByPost(post.id)
   }
} 

To take it another step further, if we already have the author id in the @Root argument, let's say if author field is typed s author: string | IAutho, we can eliminate getAuthorByPostId and have the field linked directly to getAuthor query. This is a cleaner solution, though might not be the best practice for everyone.

@shlomokraus
Copy link
Author

Btw, the reason I didn't extract the implementation to an external service, is that it kind of losses the idea of decorators. If all that the class is doing is being decorated and just passes the args to a service class, we can might as well just use the service class and attach it to the schema as object references ( { Query: { getAuthor: (...) => authorService.getAuthor(...) } }.
The nice things about decorators is that they provide this mapping on top of an existing service class.

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 27, 2018

Btw, the reason I didn't extract the implementation to an external service, is that it kind of losses the idea of decorators.

Decorators are only a syntactic sugar for providing metadata for classes, properties and params. They're not designed to mix your domain with infrastructure.

Treating resolver's classes as a domain services cause very tough coupling between the layers. That's why we don't execute raw SQL in resolvers but we use repository pattern or ORM (TypeORM) or ODM (mongoose) for talking with DB. Resolvers in TypeGraphQL and controllers in Nest/routing-controllers should be responsible only for the infrastructure layer, so they translate body/params/query data into arguments of a domain layer's services. And domain services shouldn't know/assume anything about the infrastructure (exposed via HTTP, websockets or GraphQL).

a resolver class of one type, is calling a service of a different type

I recently started to dislike this pattern too. So now I'm thinking about grouping resolvers in classes not by a base type but by the return type, used in services/repositories.

The API is of course a subject to change. Here's how it would look in case of our example:

@Resolver()
class PostResolver {
  constructor(protected readonly postService: PostService) {}

  @Query(returns => [Post])
  getPosts(@Args() args: PostsArgs) {
    return this.postService.getPosts(args);
  }

  // this one would use the method's name
  @FieldResolver(of => Author) 
  posts(@Root() author: Author, @Args() args: PostsArgs) {}
  // and this one would prevent method's name colission
  @FieldResolver(of => Author, author => author.posts) 
  getAuthorPosts(@Root() author: Author, @Args() args: PostsArgs) {
    // preparing your args based on context
    args.filter.authorId = author.id;
    return this.postService.getPosts(args);
  }
}

The ResolverInterface won't work in that case and it might be harder to find missing field resolvers for the object types but it's a good trade-off for solving the N services problem.

What do you think about that? Would this convince you to extract common logic into independent services? 😉

@shlomokraus
Copy link
Author

Decorators are only a syntactic sugar for providing metadata for classes, properties and params. They're not designed to mix your domain with infrastructure.

Agree. Maybe I didn't mention or emphasize the fact that all domain and database logic is any way extracted to external service\microservice. So the resolvers only parse arg, map and forward them to the service and parse results. The service, however, might be more general than the resolver.

PostResolver.getPosts() -> ContentService.getPostsByIds()

So if we extract that to yet another service it would look:

PostResolver.getPosts() -> PostResolverService.getPosts() -> ContentService.getPostsByIds

Decorators allow us to use separate logic like in the second example, but in a cleaner less redundant way as in the first one. What are your thoughts?

I recently started to dislike this pattern too. So now I'm thinking about grouping resolvers in classes not by a base type but by the return type, used in services/repositories.

I am not sure I follow, since the class is PostResolver, but it returns Author type. Is that on purpose?


From reading more, I think that what I am trying to actually achieve is graphql Connections. I saw your discussion on #142 and it is more relevant. The posts field can be defined as a connection to another resolver and that's' why I am having mental problems implementing the getPosts logic (even if this logic is to call the correct service) in the Author class itself.

My thoughts are that a resolver should only resolve fields that are part of its own type, and pass a connection when the field is a link to another type. Does that make sense?

@MichalLytek
Copy link
Owner

MichalLytek commented Oct 28, 2018

Decorators allow us to use separate logic like in the second example, but in a cleaner less redundant way as in the first one. What are your thoughts?

In your case it make sense to reuse the resolver as a service. But still it should be done explicitly by injecting it into constructor, not by a magic @Relation decorator or something.

I am not sure I follow, since the class is PostResolver, but it returns Author type. Is that on purpose?

In the showed proposal, the class is PostResolver and it returns Post type. Query -> getPosts, Author -> posts, Book -> posts, etc. - they all use the same service and can even call this.getPosts resolver.

that's' why I am having mental problems implementing the getPosts logic (even if this logic is to call the correct service) in the Author class itself

Me too, that's why I'm planing to introduce @FieldResolver(of => Author, author => author.posts) to be able to group all e.g. post resolving logic into one class. So then you could handle the connection logic (parsing args, making request, transforming response) in one method and then just call it in field resolvers of Author, Book or other types.

@Resolver()
class PostResolver {
  @Query(returns => [Post])
  getPosts(@Args() { filter, skip, limit }: PostsArgs) {
    const criteria: any = {};
    if (filter) {
      // filter to criteria transform from example
    }
    return DB.Posts.find(criteria)
      .limit(limit)
      .skip(skip);
  }

  @FieldResolver(of => Author, author => author.posts) 
  getAuthorPosts(@Root() author: Author, @Args() args: PostsArgs) {
    // preparing your args based on context
    args.filter.authorId = author.id;
    return this.getPosts(args);
  }
}

@shlomokraus
Copy link
Author

That's a good solution, but I feel (just a hunch) that it might not be intuitive and clear to all developers. I think the problem is that PostResolver defines the return of Author.posts, so when someone looks at Author he has no way of knowing how posts field is resolved unless he looks in PostResolver. Don't you think that it might be tricky when there are many resolvers?

@MichalLytek
Copy link
Owner

Yes, it might be a bit unintuitive at first glance but it's a fully opt-in feature, mostly targeted for advanced users who feel that they volatile the DRY principle.

If somebody don't feel comfortable with this kind of field resolvers grouping, he may use instead the common resolver service pattern (which I showed) or splitting into query and field resolvers and injecting them (like you've showed).

I don't wanna impose one and only solution to this problem. I want to let the users freedom of choice of the pattern that fit them best. My role is to make this possible by implementing e.g. this grouping feature and showing this patterns in "recipes" section in docs.

@XBeg9
Copy link

XBeg9 commented Nov 4, 2018

@19majkel94 where I can track progress of @FieldResolver(of => Author, author => author.posts) ? Thanks.

@MichalLytek
Copy link
Owner

@XBeg9 See #193 😉

@shlomokraus Closing this one as it will be solved by #193 🔒

@MichalLytek MichalLytek added Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request and removed Discussion 💬 Brainstorm about the idea Question ❔ Not future request, proposal or bug issue labels Nov 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community 👨‍👧 Something initiated by a community Duplicate 🔑 This issue or pull request already exists Enhancement 🆕 New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants