nestjs / graphql

GraphQL (TypeScript) module for Nest framework (node.js) 🍷
https://docs.nestjs.com/graphql/quick-start
MIT License
1.45k stars 392 forks source link

decorated resolvers with ResolveField are automatically added as fields to object types. #1514

Closed DonnyVerduijn closed 3 years ago

DonnyVerduijn commented 3 years ago

I'm submitting a...


[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

When using federated graphs, it seems to be impossible to avoid that stub resolvers that are decorated with the ResolveField decorator, create extra fields on the object type. For example, when a user has an accounts property with an array of accounts, every account has a field user that references back to the user in question.

Expected behavior

My preference would go out to hide fields from resolvers that are decorated with ResolveField by default, unless they are explicitly added to the object type.

@Directive(`@key(fields: "id")`)
@ObjectType()
export class Account {
  @Field(() => ID)
  id: string;

  @Field()
  name: string;

  @Field()
  balance: number;

  // this field is not part of the graph, because its missing Field() decorator
  // however, it's currently still available when resolving references 
  userId: string;

  // only this declaration should result in the user field added to accounts
  // it shouldn't rely on decorated resolvers.
  @Field(() => User)
  user: User; 
}

Alternatives would be to either create an additional Decorator for stub resolvers, allow the field to be hidden through its configuration options or through an additional decorator such as HideField that would apply the @internal directive in case of stub resolvers. However i think it's best to make this feature opt-in instead of opt-out.

Note that it seems that an @internal directive will be available in near feature to hide these fields from the supergraph. See: https://github.com/apollographql/federation/issues/371

Minimal reproduction of the problem with instructions

https://github.com/hardyscc/nestjs-cqrs-starter ./apps/service-account/src/account/resolver/account.resolver.ts

@ResolveField(() => User)
user(@Parent() account: Account) {
  return { __typename: 'User', id: account.userId };
}

This adds the user field to the accounts object type, but cannot be removed since it is required for federated graphs to resolve accounts in parallel with the user.

What is the motivation / use case for changing the behavior?

The reason why this is such a problem, is that it creates circular references in the graph which should be avoided by default. It also unexpectedly exposes fields where this might be unwanted.

Environment


Nest version: 7.6.15

For Tooling issues:
- Node version: 14
- Platform:  WSL Ubuntu 18
kamilmysliwiec commented 3 years ago

My preference would go out to hide fields from resolvers that are decorated with ResolveField by default, unless they are explicitly added to the object type.

There's no plan to change the default behavior.

Alternatives would be to either create an additional Decorator for stub resolvers, allow the field to be hidden through its configuration options or through an additional decorator such as HideField that would apply the @internal directive in case of stub resolvers. However i think it's best to make this feature opt-in instead of opt-out.

Adding a configuration property that would allow flagging a specific resolver as "internal" sounds interesting. Would you like to create a PR for this issue?

DonnyVerduijn commented 3 years ago

Hi Kamil, after giving it a second thought, i agree it would be unwise to change the default behavior. I guess it's best to wait for apollo its implementation of the @internal directive as stated in https://github.com/apollographql/federation/issues/371, before we look further into this?

kamilmysliwiec commented 3 years ago

Sounds good to me 👍 Let's wait then