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

Hint that a field of an union or interface type will resolve only to types specified in the selection set #951

Open
n1ru4l opened this issue May 27, 2022 · 6 comments

Comments

@n1ru4l
Copy link

n1ru4l commented May 27, 2022

Note: this is a very early thought-dump

Given the following schema and operation:

interface Searchable {
  id: ID!
}

type User implements Searchable {
  id: ID!
  name: String!
}

type Post implements Searchable {
  id: ID!
}

type Comment implements Searchable {
  id: ID!
}

type Query {
  search: [Searchable!]!
}
query Search($needle: String!) {
  search(needle: $needle) {
    ... on User {
      id
      name
    }
  }
}

The generated TypeScript type must be:

type SearchQuery = {
  search: { id?: never; name?: never } | { id: string; name: string };
};

The Query.search resolver implementation could process the selection set and only resolve to types that are actually requested via a fragment/inline-fragment.

import { GraphQLObjectType, GraphQLNonNull, GraphQLList } from "graphql";
import { parseResolveInfo } from "graphql-parse-resolve-info";
import { GraphQLSearchable } from "./searchable";

const items = [
  { type: "User", id: "User:1", name: "Laurin" },
  { type: "Post", id: "Post:1", title: "My first post." },
  { type: "Comment", id: "Comment:1", content: "Hi Laurin" }
];

const GraphQLQueryType = new GraphQLObjectType({
  name: "Query",
  fields: () => ({
    search: {
      type: new GraphQLNonNull(
        new GraphQLList(new GraphQLNonNull(GraphQLSearchableType))
      ),
      resolve(_source, args, _context, info) {
        const requestedTypeNames = Object.keys(
          parseResolveInfo(info)?.fieldsByTypeName ?? {}
        );

        return items.filter((item) => requestedTypeNames.includes(item.type));
      }
    }
  })
});

Full runnable example over here: https://codesandbox.io/s/graphql-yoga-interface-field-only-resolve-selected-types-bzw7b6


It could be helpful if there was a way of hinting via the schema (and introspection) that a specific field of an interface or union type will only resolve to those types that are explicitly selected via fragment/inline-fragments on the selection set within the executed document. E.g. via a directive.

type Query {
  search: [Searchable!]! @onlyResolveToSelectedEntities
}

For the following operation:

query Search($needle: String!) {
  search(needle: $needle) {
    ... on User {
      id
      name
    }
  }
}

instead, we can now generate

type SearchQuery = {
  search: { id: string; name: string };
};

For the following operation:

query Search($needle: String!) {
  search(needle: $needle) {
    ... on User {
      id
      name
    }
    ... on Comment {
      id
      content
    }
  }
}

instead, we can now generate

type SearchQuery = {
  search:
    | { id: string; name: string; content?: never }
    | { id: string; name?: never; content: string };
};

In addition, the GraphQL execution algorithm could take that instruction into account and raise an exception in case the resolved type would not match the selected types within the selection set.

So far I am unsure what should happen if you select an interface type on such a field. I guess tools should interpret this as "this can now resolve to all entities that implement the interface", which would be future-proof in case a new interface member was added.

query Search($needle: String!) {
  search(needle: $needle) {
    ... on Searchable {
      id
    }
  }
}
type SearchQuery = {
  search: { id: string };
};

This approach makes sense for union types, where there are currently no shared fields.

It could furthermore help people build more resilient and future-proof GraphQL schemas.


Related issues:

@n1ru4l n1ru4l changed the title Hint that a field of an union or interface type will resolve only to types requested in a selection set Hint that a field of an union or interface type will resolve only to types specified in the selection set May 27, 2022
@benjie
Copy link
Member

benjie commented May 27, 2022

@n1ru4l If I understand you correctly, you're saying it would be nice to have an opt-in way of having GraphQL limit you to only returning types from an interface-returning field that are compatible with what the user has requested; e.g. if they request ... on User {...} but don't request anything on Post or Comment and also don't request any of the shared fields, then the result should only contain User objects and should not contain the "empty objects" in the response that you'd receive for unqueried types.

My first comment would be, in these days of the widespread use of __typename on every selection set, this case is going to come up very rarely even for unions since __typename being present would require that all types be available. With interfaces it should be even more rare because querying the shared fields on an interface is kind of the point of interfaces.

My second would be that this would require some kind of "look ahead" where GraphQL determines which concrete types are queried on the selection set, and then feeds this information into the resolver so that the resolver knows only to return these types. GraphQL doesn't currently have anything like this in the spec (as far as I can recall), so that would be quite a divergence from the status quo, I think.

I'd also argue that GraphQL resolves things layer by layer, so what is returned from a resolver should never be dependent on its selection set. For example if I issue your Search query above, and then count the results, it would be extremely unexpected if the number of results were to change simply because I added the __typename or id field to the selection set at the interface level. Arguably that's what you're opting into via the directive, but it still feels... wrong?

I think the best approach to this would be instead to make the requirement a field argument: search(needle: $needle, only: [User, Comment]), but I understand that this could not (generally) be automatically picked up by codegen.

An interesting topic, for sure 🤔

@n1ru4l
Copy link
Author

n1ru4l commented May 27, 2022

@benjie I guess __typename would require special handling - which is something I also would like to avoid and don't like. Personally, I am also not a huge fan of __typename and would prefer people using globally unique IDs instead. Apollo Client's popularity kind of led towards automatically adding __typename to anything 😅

Regarding the lookaheads: You mean in terms of the specification? To me this kind of fits into Value Completion phase - or do you mean access to the selection set context in that phase?

I'd also argue that GraphQL resolves things layer by layer, so what is returned from a resolver should never be dependent on its selection set.

People are already doing this with solutions like the following: https://engineering.zalando.com/posts/2021/03/optimize-graphql-server-with-lookaheads.html We have seen a lot of our clients do similar stuff.

For union fields, it is also currently the only safe way to introduce new types to a union type if you do not "own" all the operations executed against your schema and can instruct your API consumers to consume GraphQL in a future-proof way on the client.

It could also help in generating better types for the Query.node field popular in Relay: dotansimha/graphql-code-generator#7782 (comment)

I think the best approach to this would be instead to make the requirement a field argument: search(needle: $needle, only: [User, Comment]), but I understand that this could not (generally) be automatically picked up by codegen.

This is also the pattern that the GitHub API follows for their Query.search field. Maybe an alternative solution could be to make exceptions for sub-specifications.
In the best scenario, I would prefer to have such behavior defined within the GraphQL specification - but I can see that this might not happen as it is too specific and would dictate how people build and design their schemas.

@benjie
Copy link
Member

benjie commented May 27, 2022

Regarding the lookaheads: You mean in terms of the specification?

Yes, in terms of the spec.

To me this kind of fits into Value Completion phase

It would certainly go somewhere within ExecuteField() or a descendent of that. From what you wrote before I thought you were saying that the resolver would need to only return acceptable types, and thus it should be part of ResolveFieldValue(). Currently the resolver is only officially passed the objectValue and argumentValues; I believe you're proposing passing more information into it explicitly in the spec. (In GraphQL.js we have GraphQLResolveInfo which is more information, but this stuff is not officially specified.) I agree that the resolver should be aware of this because it should "do less work" - no point doing more work and then throwing it away - but what I don't agree with is how the resolver receives this information. Currently I'm in favour of an argument.

For union fields, it is also currently the only safe way to introduce new types to a union type if you do not "own" all the operations executed against your schema and can instruct your API consumers to consume GraphQL in a future-proof way on the client.

This is a really good argument. We say that you should always expect a new type to be added to a union, and you should handle that case (i.e. having a default block in a switch statement), but nonetheless giving clients a way to opt out of this (similar to client controlled nullability) does seem desirable. @mjmahone had a really good argument a few WGs back about the complexities that Relay faces when handling unexpected types in nested interfaces (or something like that, I don't remember the specifics) - perhaps he has some feedback here.

In the best scenario, I would prefer to have such behavior defined within the GraphQL specification

I agree with this desire. It might be good to lay out explicitly the problems we're solving (like the one above regarding supporting adding types to unions in a non-breaking way) and then lay out what the possible solutions to it could be, and the tradeoffs of each one. For example, the argument approach: could that be supported by codegen if we were to add a special directive (or similar) to that argument or union telling the type system what it means?

@n1ru4l
Copy link
Author

n1ru4l commented May 27, 2022

Problem: Future Proof Union Types

Handling fields of a union type in a future-proof way is hard to handle on the client-side. Depending on who is consuming the schema, e.g. a different team or external public users, there is a high probability that adding a new type to the union will eventually end up breaking clients.

There has been an initiative for allowing to constraint union members to an interface. This is usually great when union members contain similar types, but does not address the issue when the union types are different from each other and have no shared interface or properties at all.

type UserNotFoundError {
  message: String!
}

type User {
  id: ID!
}

union UserByIdResult = User | UserNotFoundError

type Query {
  userById(id: ID): UserByIdResult!
}

A common pattern for avoiding unexpected breakage of clients after adding a new type to a union is to use the operation document being executed within the field resolver implementation in order to restrict the type (entity) being resolved to only those specifically selected through the fragments and inline-fragments that are within the selection set of that field.

It is currently impossible to hint to clients that a union field has implemented such behavior, which makes it hard for tools like GraphQL Code Generator to take this into account and generate more client-friendly typings.

Problem: Relay Query.node(id:) field

Within the Relay GraphQL Server Specification all global unique types (entity) implement the Node interface as following:

interface Node {
  id: ID!
}

type Faction implements Node {
  id: ID!
  name: String
  ships: ShipConnection
}

type Ship implements Node {
  id: ID!
  name: String
}

The specification, furthermore, introduces a Query.node field that can be used for fetching a unique type (entity) via it's ID.

type Query {
  node(id: ID!): Node
}

Example Query Operation for fetching a Ship:

query Ship($id: ID!) {
  ship: node(id: $id) {
    ... on Ship {
      id
      name
    }
  }
}

When executing such an operation we want the Query.node field to always resolve a Ship type. However, if we are accidentally passing the id of a Faction instead, the server could instead resolve to a Faction giving us the following execution result:

{ "data": { "ship": {} } }

This is not helpful at all to the client as the client actually wants a Ship.

It might be more straightforward if the Query.node resolver implementation takes into account the operation document being executed within the field resolver implementation in order to restrict the type (entity) being resolved to only those specifically selected through the fragments and inline-fragments that are within the selection set of that field.

It is currently impossible to hint to clients that a union field has implemented such behavior, which makes it hard for tools like GraphQL Code Generator to take this into account and generate more client-friendly typings (see dotansimha/graphql-code-generator#7782 (comment)).

Problem: Search fields

TODO: full breakdown of this situation

Possible Solution: Introduction of a Schema Directive

The introduction of a new directive that can be applied on fields that have a union or interface type, could expose the necessary meta information for tooling to generate more client-friendly typings.

Example: Union

Schema

type Cat {
  id: ID!
  name: String!
  isChonker: Boolean!
}

type Dog {
  id: ID!
  name: String!
  isWoofer: Boolean!
  isBarker: boolean!
}

type HousePet = Cat | Dog

type Query {
  pet(id: ID!): HousePet @resolvesOnlySelectedMembers
}

Operation

query Doggo($id: ID){
  pet(id: $id) {
    ... on Dog {
      id
      name
      isWoofer
    }
  }
}

TypeScript Output without @resolvesOnlySelectedMembers on Query.pet

type DoggoQueryResult = {
  pet: 
    null 
    | { id?: never; name?: never; isWoofer?: never;}
    | { id: string; name: string; isWoofer: boolean }
}

TypeScript Output with @resolvesOnlySelectedMembers on Query.pet

type DoggoQueryResult = {
  pet: 
    null 
    | { id: string; name: string; isWoofer: boolean }
}

Furthermore, in addition to exposing useful metadata, the execution algorithm could use the same metadata in order to enforce a correctly implemented behavior within the Query.pet resolver implementation by asserting the resolved type and the listed/referenced types within the selection set for that field. In case of a mismatch, an error could be raised.

@benjie
Copy link
Member

benjie commented May 30, 2022

This is a great summary of the problem, great work in writing it up.

However, as currently stated, I think your proposed solution breaks normalized caching (the same field with the same arguments on the same type may return different values in different queries because the value is influenced by the selection set - so future queries may remove/add things to the store in an unexpected/unpleasant way). This could be addressed by having normalised caching factor in this behavior, but that would be quite a significant divergence from the status quo.

More importantly, however, I think that it either breaks field merging, or it does unexpected things with fragments.

Take for example the following query:

query Q {
  currentUser {
    ...FragA
    ...FragB
  }
}

fragment FragA on User {
  pet { ... on Dog { name wagsTail } }
}
fragment fragB on User {
  pet { ... on Cat { name numberOfLives } }
}

Imaging pet is a Cat; then we'd expect FragA to resolve to {pet: null} and FragB to resolve to {pet: { ... }}; but merging of fields says that they must both be an object or both not an object.

We effectively merge first and have this resulting query:

query Q {
  currentUser {
    pet {
      ... on Dog { name wagsTail }
      ... on Cat { name numberOfLives }
    }
  }
}

But now, the client code that depends on FragA will get unexpected behavior because they'll have a cat object that's not the Dog (the only thing that they queried and expected) - they'd expect null if it were not a Dog. Ideally fragments should be isolated - though they may gain additional fields, the fields that they declare should be unaffected by other sibling fragments.

This is why I recommended solving the problem via a field argument ─ it solves both the problems I laid out here. However, it does not solve your code generation issue without some further thought. Perhaps going with something like I suggested before:

{ search(needle: $needle, only: [User, Comment]) { ... } }

and applying a special directive to the only argument in the GraphQL schema (rather than to the field itself) you could get the behaviour you desire without the complexities I lay out in this post?

@vbrzezina
Copy link

Hello, I just ran into this myself and wonder what's the status of this problematic? What can we do to make some progress in the matter? Can I participate anyhow? I would really welcome this functionality.. Thanks 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants