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

Update QP tests to use JSON serializer #4297

Merged
merged 15 commits into from
Jun 30, 2020
Merged

Update QP tests to use JSON serializer #4297

merged 15 commits into from
Jun 30, 2020

Conversation

JakeDawkins
Copy link
Contributor

@JakeDawkins JakeDawkins commented Jun 22, 2020

This PR aims to do a couple things:

  • Add a test format (cucumber) that can be used across multiple languages & implementations of a query planner to test for compliance of a generated query plan
  • Use the new JSON standard query plan described in Change query planner AST nodes into well-defined structures. #4265 to run those tests rather than the pretty-printed graphql-esque query plan to make parsing/serializing in other languages more reasonable.

Todo:

  • serialize the existing query plan to the future JSON structure for tests
  • write readme for language implementers

@JakeDawkins
Copy link
Contributor Author

JakeDawkins commented Jun 24, 2020

Reviewer Notes

Reviewing this PR should mostly be checking the query plan snapshots in build-query-plan.feature against the new types defined here (also collapsed below) and against the old query plan, to make sure no essential information from the plan was dropped on accident.

Make sure to give the readme a once-over as well to make sure it all makes sense

Types
export interface QueryPlan {
  kind: 'QueryPlan';
  node?: PlanNode;
}

export type PlanNode = SequenceNode | ParallelNode | FetchNode | FlattenNode;

export interface SequenceNode {
  kind: 'Sequence';
  nodes: PlanNode[];
}

export interface ParallelNode {
  kind: 'Parallel';
  nodes: PlanNode[];
}

export interface FetchNode {
  kind: 'Fetch';
  serviceName: string;
  variableUsages?: string[];
  requires?: SelectionNode[];
  source: string;
}

export interface FlattenNode {
  kind: 'Flatten';
  path: ResponsePath;
  node: PlanNode;
}

export type SelectionNode = FieldNode | InlineFragmentNode;

export interface FieldNode {
  readonly kind: 'Field';
  readonly alias?: string;
  readonly name: string;
  readonly selections?: SelectionNode[];
}

export interface InlineFragmentNode {
  readonly kind: 'InlineFragment';
  readonly typeCondition?: string;
  readonly selections: SelectionNode[];
}

A couple good tests to look at to get a picture of what changed are the follows. I've added collapsed sections containing their original query plan for easier reviewing. Search the .feature file for these cases and compare with the original query plans below. I'd suggest doing a side-by-side window to make it a little easier :)

should use two independent fetches when requesting root fields from two services,

Query Plan
QueryPlan {
  Parallel {
    Fetch(service: "accounts") {
      {
        me {
          name
        }
      }
    },
    Sequence {
      Fetch(service: "product") {
        {
          topProducts {
            __typename
            ... on Book {
              __typename
              isbn
            }
            ... on Furniture {
              name
            }
          }
        }
      },
      Flatten(path: "topProducts.@") {
        Fetch(service: "books") {
          {
            ... on Book {
              __typename
              isbn
            }
          } =>
          {
            ... on Book {
              __typename
              isbn
              title
              year
            }
          }
        },
      },
      Flatten(path: "topProducts.@") {
        Fetch(service: "product") {
          {
            ... on Book {
              __typename
              isbn
              title
              year
            }
          } =>
          {
            ... on Book {
              name
            }
          }
        },
      },
    },
  },
}

when requesting an extension field from another service, it should add the field's representation requirements to the parent selection set and use a dependent fetch

Query Plan
QueryPlan {
  Sequence {
    Fetch(service: "accounts") {
      {
        me {
          name
          __typename
          id
        }
      }
    },
    Flatten(path: "me") {
      Fetch(service: "reviews") {
        {
          ... on User {
            __typename
            id
          }
        } =>
        {
          ... on User {
            reviews {
              body
            }
          }
        }
      },
    },
  },
}

should properly expand nested unions with inline fragments

Query Plan
QueryPlan {
  Fetch(service: "documents") {
    {
      body {
        __typename
        ... on Image {
          attributes {
            url
          }
        }
        ... on Text {
          attributes {
            bold
          }
        }
      }
    }
  },
}

@abernix abernix requested review from abernix and trevor-scheer June 24, 2020 18:07
Base automatically changed from master to main June 24, 2020 18:18
Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly I'm pretty sad to lose the currently very readable query plan format. Eye-parsing the results here against the existing tests / snapshots was disappointing. I don't think we need to block this work based on this concern, but it would be a huge win to land on a simple serialization format that we feel comfortable imposing on other consumers of these tests.

i.e. something less graphql-y where indentation === nesting

QueryPlan
  Parallel
    Fetch(service: "accounts")
      {me{name}}
    Sequence
      Fetch(service: "product")
        {topProducts{__typename ... on Book{__typename isbn}... on Furniture{name}}}
      Flatten(path: "topProducts.@") {
        Fetch(service: "books", requires: {... on Book{__typename isbn}})
          {... on Book{__typename isbn title year}}
      Flatten(path: "topProducts.@")
        Fetch(service: "product", requires: {... on Book{ __typename isbn title year})
          {... on Book{name}}

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exciting! Thanks for putting this together and polishing it up. I've left a few thoughts / requests / suggestions throughout. Let me know what you think, and I assuming the feedback around isn't too vicious, I think we can get this merged quite soon.

packages/apollo-gateway/src/__tests__/CucumberREADME.md Outdated Show resolved Hide resolved
packages/apollo-gateway/src/__tests__/CucumberREADME.md Outdated Show resolved Hide resolved
packages/apollo-gateway/src/__tests__/CucumberREADME.md Outdated Show resolved Hide resolved
packages/apollo-gateway/src/__tests__/CucumberREADME.md Outdated Show resolved Hide resolved
@JakeDawkins
Copy link
Contributor Author

JakeDawkins commented Jun 29, 2020

Admittedly I'm pretty sad to lose the currently very readable query plan format. Eye-parsing the results here against the existing tests / snapshots was disappointing. I don't think we need to block this work based on this concern, but it would be a huge win to land on a simple serialization format that we feel comfortable imposing on other consumers of these tests.

i.e. something less graphql-y where indentation === nesting

QueryPlan
  Parallel
    Fetch(service: "accounts")
      {me{name}}
    Sequence
      Fetch(service: "product")
        {topProducts{__typename ... on Book{__typename isbn}... on Furniture{name}}}
      Flatten(path: "topProducts.@") {
        Fetch(service: "books", requires: {... on Book{__typename isbn}})
          {... on Book{__typename isbn title year}}
      Flatten(path: "topProducts.@")
        Fetch(service: "product", requires: {... on Book{ __typename isbn title year})
          {... on Book{name}}

@trevor-scheer I agree that losing the prettier printed version of the tests is a loss. My concern with reformatting (even in the future) is difficulty in parsing the pretty printed tests. I imagine we'd end up expecting people to build a parser in each language, which seems like too much of an ask.

- used fixure names from __fixures__/schema/index
- added types for clarity in some spots
- build the service list in 1-step for schema composition
- swap to matching test step titles exactly
- use graphql-js `Kind` instead of strings to match in the serializer
@Enrico2
Copy link
Contributor

Enrico2 commented Jun 30, 2020

Hey, I have a local branch that is able to parse (and serialize to) the same JSON as in the feature file. IIRC, we'd like to end up using the same cucumber files to test the implementation of query planning on the Rust side. For when that's the case, did we give any thought as to where the shared feature file would live?

@JakeDawkins
Copy link
Contributor Author

@Enrico2 Maybe we could discuss that in a few minutes, but we don't have anything concrete yet. For now, It's just copy/pasted from here probably. But I know that's not a great option for keeping everything in sync

@abernix
Copy link
Member

abernix commented Aug 7, 2020

@JakeDawkins Does this close #4265?

abernix pushed a commit to apollographql/federation that referenced this pull request Sep 4, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants