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

Support defining fragments for non-root field types, avoids explicit transform (standalone delegateToSchema) #876

Closed
Druotic opened this issue Jul 9, 2018 · 3 comments
Labels
bug docs Focuses on documentation changes feature New addition or enhancement to existing solutions has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository

Comments

@Druotic
Copy link
Contributor

Druotic commented Jul 9, 2018

/label bug
/label has-reproduction
/label schema-stitching

UPDATE: this description is still useful for describing the scenario, but the root issue is the behavior differences between the standalone delegateToSchema and info.mergeInfo.delegateToSchema. See comments below for futher info.

Currently (I think), the only way to add fragments for any field deeper than a root level type's field is using transforms. Here is an example to demonstrate. In this example, there is a proxy that is stitching together multiple remote schemas.

Proxy layer:

// proxy layer resolvers
exports.resolvers = function (bookAugmentServiceSchema, pageServiceSchema) {
  return {
    Book: {
      pagesConnection: {
        fragment: '... on Book { id }',
        async resolve (book, args, context, info) {
          return delegateToSchema({
            schema: bookAugmentServiceSchema,
            operation: 'query',
            fieldName: 'pagesForBook',
            args: { bookId: book.id }
            context,
            info,
            transforms: [
              new ReplaceFieldWithFragments(schema, [
                {
                  field: 'node',
                  fragment: '... on PagesForBookEdge { pageId }'
                }
              ])
            ]
          });
        }
      }
    },
    PagesForBookEdge: {
      node: {
        // Ideally we'd be able to specify the fragment here instead
        // fragment: '... on PagesForBookEdge { pageId }',
        async resolve (pageEdge, args, context, info) {
          const pageId = pageEdge.pageId;
          return delegateToSchema({
            schema: pageServiceSchema,
            operation: 'query',
            fieldName: 'page',
            args: { id: pageId },
            context,
            info
          });
        }
      }
    }
  };
};

// proxy layer typedefs
exports.typeDefs = `
  extend type PagesForBookEdge {
    node: Page!
  }

  extend type Book {
    pagesConnection: PagesForBookConnection!
  }
`;

Other remote schemas for completeness:

// book service #1 typedefs (primary owner of Book type)
exports.typeDefs = `
  type Book {
    id: ID!
    title: String!
  }
`;

// book service #2 typedefs (exposes queries for augmenting the Book type at the proxy)
exports.typeDefs = `
  # note: normally there would be PageInfo (next token, etc) here to support
  # paging list results... ignoring all that for simplicitly.
  type PagesForBookConnection {
    edges: [PagesForBookEdge!]!
  }

  type PagesForBookEdge {
    # this pageId is only used for stitching purposes, it will be the same as node.id at the proxy
    pageId: ID!
  }

  type Query {
    pagesForBook (bookId: ID!): PagesForBookConnection!
  }
`;

// page service typedefs
exports.typeDefs = `
  type Page {
    id: ID!
    text: String!
  }
`;

And finally, sample query to trigger behavior:

query BookPages($bookId: ID!) {
  book(id: $bookId) {
    pagesConnection {
      edges {
        node {
          id
          text
        }
      }
    }
  }
}

When pagesConnection is referenced, ... on Book { id } is inserted into the downstream book query as intended. However, for the node reference, the ... on PagesForBookEdge { pageId } fragment does not get added unless the ReplaceFieldWithFragments transform is explicitly used. So, what'll happen (without the explicit transform) is a query like the following will get generated (because book service 2 only knows about/expects a pageId query field on the PagesForBookEdge type):

query BookPages($bookId: ID!) {
  book(id: $bookId) {
    pagesConnection {
      edges
    }
  }
}

...and it'll complain with an error like Field "edges" of type "[PagesForBookEdge!]!" must have a selection of subfields. Did you mean "edges { ... }"?

I'm proposing that fragment resolution/appending be smarter. Instead of simply looking at the root query type and looking for RootType:field fragment definitions, I propose the following:

  1. Maintain a type mapping of field:Type, or lookup as needed - there should be enough info to calculate the type for any given field (change needed)
  2. When a node is visited, look up the parent node's type. Today, this only happens if the parent is a root type. If the parent is a field, the type is undefined in replaceFieldsWithFragments and the function exits early without adding any fragments (change needed)
  3. Use the existing resolver map lookup to lookup ParentType:field resolvers and add the fragments like normal (no change needed).

I've started working on an implementation of this, but I wanted to document the issue I'm trying to address for when I open that PR. Also, I want to be sure this is something everyone is on-board with while I'm working on the change.

@ghost ghost added docs Focuses on documentation changes feature New addition or enhancement to existing solutions labels Jul 9, 2018
@Druotic
Copy link
Contributor Author

Druotic commented Jul 11, 2018

Oops! Turns out this was an issue because I was using the standalone delegateToSchema instead of the wrapper exposed via info.mergeInfo.delegateToSchema. This took wayyy too long to figure out :'( ... and the fix is much simpler than my proposition above.

Given that we're passing info to delegateToSchema anyway, it seems like we should have all the info we need to properly delegate in the standalone version as well. I'm going to take a stab at decoupling things a little so that the standalone behaves the same as info.mergeInfo.delegateToSchema to avoid future confusion by others.

Druotic pushed a commit to Druotic/graphql-tools that referenced this issue Jul 12, 2018
Druotic pushed a commit to Druotic/graphql-tools that referenced this issue Jul 12, 2018
@ghost ghost added bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository schema-stitching labels Jul 12, 2018
@Druotic
Copy link
Contributor Author

Druotic commented Jul 12, 2018

Ugh, can't remove the old labels (docs/feature) after adding more appropriate labels. I labeled this as a bug since delegateToSchema is now a publicly exposed standalone function and should have similar feature parity to the mergeInfo version.

@Druotic Druotic changed the title Support defining fragments for non-root field types, avoids explicit transform Support defining fragments for non-root field types, avoids explicit transform (for standalone delegateToSchema) Jul 12, 2018
@ghost ghost added bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository schema-stitching labels Jul 12, 2018
@Druotic Druotic changed the title Support defining fragments for non-root field types, avoids explicit transform (for standalone delegateToSchema) Support defining fragments for non-root field types, avoids explicit transform (standalone delegateToSchema) Jul 12, 2018
@ghost ghost added bug has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository schema-stitching labels Jul 12, 2018
Druotic pushed a commit to Druotic/graphql-tools that referenced this issue Jul 12, 2018
stubailo pushed a commit that referenced this issue Jul 31, 2018
* Fixes issue #876, mirror delegateToSchema behavior for standalone

* Update CHANGELOG

* Whoops failed at merge conflict
@hwillson
Copy link
Contributor

hwillson commented Sep 7, 2018

Fixed in #885. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug docs Focuses on documentation changes feature New addition or enhancement to existing solutions has-reproduction ❤ Has a reproduction in a codesandbox or single minimal repository
Projects
None yet
Development

No branches or pull requests

2 participants