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

Connection based query splitting #4926

Closed
wants to merge 1 commit into from

Conversation

crpahl
Copy link

@crpahl crpahl commented Apr 22, 2024

Implementation for #4917.

Example inputs

Note, this will only split valid queries in the schema that you are calling split on. For now, I'll supply example queries that are being split from our own schema. The output below was captured using the debug keyword argument.

A simple query with no connections:

There are no connections in this query so nothing is split

Query

query MyQuery {
    account {
        name
        currentUser {
            fullName
        }
        maxUserCount
    }
}

OurSchema.split(query)

base: query MyQuery {  account {    name    currentUser {      fullName      __typename    }    maxUserCount    __typename  }}
nested:

A query with no nested connections

A slightly more complex example that shows how connection fields are split

query MyQuery {
    invoices {
        nodes {
            id
            invoiceNet
            invoiceNumber
        }
    }
    jobs {
        nodes {
            id
        }
    }
}

OurSchema.split(query)

[{:path=>".MyQuery",
  :paginated=>
   "query MyQuery($__appPlatformCursor: String!) {\n  invoices(after: $__appPlatformCursor, first: 50) {\n    nodes {\n      id\n      invoiceNet\n      invoiceNumber\n      __typename\n    }\n    __typename\n    __appPlatformPageInfo: pageInfo {\n      hasNextPage\n      endCursor\n    }\n  }\n}",
  :unrolled=>[]},
 {:path=>".MyQuery",
  :paginated=>
   "query MyQuery($__appPlatformCursor: String!) {\n  jobs(after: $__appPlatformCursor, first: 50) {\n    nodes {\n      id\n      __typename\n    }\n    __typename\n    __appPlatformPageInfo: pageInfo {\n      hasNextPage\n      endCursor\n    }\n  }\n}",
  :unrolled=>[]}]

A query with multiple nested connection

A query containing nested connections demonstrating how the system splits the nested connections into individual queries.

Query

query MyQuery {
    invoices {
        nodes {
            id
            invoiceNet
            invoiceNumber
            jobs {
                nodes {
                    jobNumber
                    jobStatus
                    id
                    visits {
                        nodes {
                            id
                            endAt
                            createdAt
                        }
                    }
                }
            }
            lineItems {
                edges {
                    node {
                        id
                        qty
                        name
                    }
                }
            }
        }
    }
    jobs {
        nodes {
            id
        }
    }
}

OurSchema.split(query)

[{:path=>".MyQuery",
  :paginated=>
   "query MyQuery($__appPlatformCursor: String!) {\n  invoices(after: $__appPlatformCursor, first: 50) {\n    nodes {\n      id\n      invoiceNet\n      invoiceNumber\n      __typename\n    }\n    __typename\n    __appPlatformPageInfo: pageInfo {\n      hasNextPage\n      endCursor\n    }\n  }\n}",
  :unrolled=>
   [{:rollup_field_name=>".MyQuery.invoices",
     :queries=>
      {:base=>"query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!) {\n  invoice(id: $__appPlatformUniqueIdForUnrolling) {\n    __typename\n  }\n}",
       :nested=>
        [{:path=>".UnrolledQuery.invoice",
          :paginated=>
           "query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!, $__appPlatformCursor: String!) {\n  invoice(id: $__appPlatformUniqueIdForUnrolling) {\n    jobs(after: $__appPlatformCursor, first: 50) {\n      nodes {\n        jobNumber\n        jobStatus\n        id\n        __typename\n      }\n      __typename\n      __appPlatformPageInfo: pageInfo {\n        hasNextPage\n        endCursor\n      }\n    }\n  }\n}",
          :unrolled=>
           [{:rollup_field_name=>".UnrolledQuery.invoice.jobs",
             :queries=>
              {:base=>"query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!) {\n  job(id: $__appPlatformUniqueIdForUnrolling) {\n    __typename\n  }\n}",
               :nested=>
                [{:path=>".UnrolledQuery.job",
                  :paginated=>
                   "query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!, $__appPlatformCursor: String!) {\n  job(id: $__appPlatformUniqueIdForUnrolling) {\n    visits(after: $__appPlatformCursor, first: 50) {\n      nodes {\n        id\n        endAt\n        createdAt\n        __typename\n      }\n      __typename\n      __appPlatformPageInfo: pageInfo {\n        hasNextPage\n        endCursor\n      }\n    }\n  }\n}",
                  :unrolled=>[]}]}}]}]}},
    {:rollup_field_name=>".MyQuery.invoices",
     :queries=>
      {:base=>"query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!) {\n  invoice(id: $__appPlatformUniqueIdForUnrolling) {\n    __typename\n  }\n}",
       :nested=>
        [{:path=>".UnrolledQuery.invoice",
          :paginated=>
           "query UnrolledQuery($__appPlatformUniqueIdForUnrolling: EncodedId!, $__appPlatformCursor: String!) {\n  invoice(id: $__appPlatformUniqueIdForUnrolling) {\n    lineItems(after: $__appPlatformCursor, first: 50) {\n      edges {\n        node {\n          id\n          qty\n          name\n          __typename\n        }\n        __typename\n      }\n      __typename\n      __appPlatformPageInfo: pageInfo {\n        hasNextPage\n        endCursor\n      }\n    }\n  }\n}",
          :unrolled=>[]}]}}]},
 {:path=>".MyQuery",
  :paginated=>
   "query MyQuery($__appPlatformCursor: String!) {\n  jobs(after: $__appPlatformCursor, first: 50) {\n    nodes {\n      id\n      __typename\n    }\n    __typename\n    __appPlatformPageInfo: pageInfo {\n      hasNextPage\n      endCursor\n    }\n  }\n}",
  :unrolled=>[]}]

What broke from 2.0.24 to 2.3.0

Not much!

diff --git a/app/services/app_platform/bulk/visitors/add_pagination_to_query_visitor.rb b/app/services/app_platform/bulk/visitors/add_pagination_to_query_visitor.rb
index 0ef7e740641..91e459d4ec7 100644
--- a/app/services/app_platform/bulk/visitors/add_pagination_to_query_visitor.rb
+++ b/app/services/app_platform/bulk/visitors/add_pagination_to_query_visitor.rb
@@ -9,10 +9,8 @@ module AppPlatform
 
         def on_operation_definition(node, parent)
           modified_node = node.merge_variable(
-            {
-              name: "__appPlatformCursor",
-              type: GraphQL::Language::Nodes::TypeName.new(name: "String!"),
-            }
+            name: "__appPlatformCursor",
+            type: GraphQL::Language::Nodes::TypeName.new(name: "String!"),
           )
           super(modified_node, parent)
         end
@@ -22,16 +20,12 @@ module AppPlatform
             old_arguments = node.arguments
             new_arguments = [
               GraphQL::Language::Nodes::Argument.new(
-                {
-                  name: "after",
-                  value: GraphQL::Language::Nodes::VariableIdentifier.new(name: "__appPlatformCursor"),
-                }
+                name: "after",
+                value: GraphQL::Language::Nodes::VariableIdentifier.new(name: "__appPlatformCursor"),
               ),
               GraphQL::Language::Nodes::Argument.new(
-                {
-                  name: "first",
-                  value: 50,
-                }
+                name: "first",
+                value: 50,
               ),
             ]
 
@@ -39,7 +33,7 @@ module AppPlatform
             new_selections = [
               GraphQL::Language::Nodes::Field.new(
                 name: "pageInfo",
-                alias: "__appPlatformPageInfo",
+                field_alias: "__appPlatformPageInfo",
                 selections: [
                   GraphQL::Language::Nodes::Field.new(
                     name: "hasNextPage"
diff --git a/app/services/app_platform/bulk/visitors/add_typename_to_query_visitor.rb b/app/services/app_platform/bulk/visitors/add_typename_to_query_visitor.rb
index f33e337304d..a7409a149d3 100644
--- a/app/services/app_platform/bulk/visitors/add_typename_to_query_visitor.rb
+++ b/app/services/app_platform/bulk/visitors/add_typename_to_query_visitor.rb
@@ -12,9 +12,7 @@ module AppPlatform
             return super if has_typename
 
             modified_node = node.merge_selection(
-              {
-                name: "__typename",
-              }
+              name: "__typename"
             )
             return super(modified_node, parent)
           end

@crpahl crpahl marked this pull request as draft April 22, 2024 01:48
@crpahl
Copy link
Author

crpahl commented Apr 22, 2024

@rmosolgo I've created a draft PR for you to look at. This should let you know which parts of the GraphQL::Language API we're using. Let me know if you'd like any other information and what you'd like me to do with the PR from here. Thank you!

@rmosolgo
Copy link
Owner

Hey, thanks for sharing this. For posterity, the breaking changes came from here:

https://github.com/rmosolgo/graphql-ruby/pull/4718/files#diff-8efe1fce6b96346bafa1dd1b6c9c01a33b56b83225d483e4835cc6cf3975564cR387

https://github.com/rmosolgo/graphql-ruby/pull/4718/files#diff-8efe1fce6b96346bafa1dd1b6c9c01a33b56b83225d483e4835cc6cf3975564cR199-R200

It looks like I didn't have any tests for the merge_ methods yet, so I added some in 1c3af64. In the future, I'll take a little more care with AbstractNode#initialize, too! Sorry for the trouble with it.

(I might change that method in the future, but it sounds like Ruby might optimize my problem away: #4854).

I don't think GraphQL-Ruby needs to add anything particular here (aside from the new tests on merge_...), so I'll close this out. Thanks again for taking the time to report this issue!

@rmosolgo rmosolgo closed this Apr 29, 2024
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

Successfully merging this pull request may close these issues.

None yet

2 participants