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

Duplicate Parallel calls to federated services with Federation 2 #1316

Closed
AneethAnand opened this issue Dec 16, 2021 · 8 comments · Fixed by #1319
Closed

Duplicate Parallel calls to federated services with Federation 2 #1316

AneethAnand opened this issue Dec 16, 2021 · 8 comments · Fixed by #1319
Assignees

Comments

@AneethAnand
Copy link
Contributor

AneethAnand commented Dec 16, 2021

Query planner generates - Duplicate parallel calls to the federated services
Repository to reproduce the error (https://github.com/AneethAnand/federation2)

Graphql Query

query test {
      viewer {
        user {
          user1
          user2
          id
          user3
          network {
                  id
              network1
          }
          user4
          user5
          user6
          user7
          user8
        }
        viewer1
        viewer2
        viewer3
    }
}

Actual QueryPlan

QueryPlan {
  Parallel {
    Fetch(service: "A1") {
      {
        viewer {
          user {
            user1
            user2
            id
            user3
            network {
              id
              network1
            }
            user4
            user5
            user6
            user7
            user8
          }
          viewer1
          viewer2
          viewer3
        }
      }
    },
    Fetch(service: "V1") {
      {
        viewer {
          user {
            user2
            id
            user3
            network {
              id
              network1
            }
            user4
            user5
            user6
            user7
            user8
          }
          viewer1
          viewer2
          viewer3
        }
      }
    },
  },
}

Expectation

    Fetch(service: "A1") {
      {
        viewer {
          user {
            user1
            user2
            id
            user3
            network {
              id
              network1
            }
            user4
            user5
            user6
            user7
            user8
          }
          viewer1
          viewer2
          viewer3
        }
      }
    }

Remove one of the field say "viewer3" in the query, the gateway works as expected.

QueryPlan {
  Fetch(service: "A1") {
    {
      viewer {
        user {
          user1
          user2
          id
          user3
          network {
            id
            network1
          }
          user4
          user5
          user6
          user7
          user8
        }
        viewer1
        viewer2
      }
    }
  },
}
pcmanus pushed a commit to pcmanus/federation that referenced this issue Dec 16, 2021
When the number of possible plans for a query is too high to generate
all plans, we use a simple strategy to discard some options, but that
strategy had a trivial bug (a simple loop that was reordering an element
in an array was mishandling the case where the element had to move to
the end) leading to invalid plans.

Fixes apollographql#1316
@pcmanus pcmanus self-assigned this Dec 16, 2021
@pcmanus
Copy link
Contributor

pcmanus commented Dec 16, 2021

Thanks for the report and sorry for the inconvenience. Created #1319 with a fix.

pcmanus pushed a commit to pcmanus/federation that referenced this issue Dec 17, 2021
When the number of possible plans for a query is too high to generate
all plans, we use a simple strategy to discard some options, but that
strategy had a trivial bug (a simple loop that was reordering an element
in an array was mishandling the case where the element had to move to
the end) leading to invalid plans.

Fixes apollographql#1316
@AneethAnand
Copy link
Contributor Author

AneethAnand commented Dec 29, 2021

@pcmanus Can you publish the changes for the fix? I dont see the change reflected in "2.0.0-alpha.2"

@AneethAnand
Copy link
Contributor Author

@abernix / @pcmanus Can you tell me how can I point query-planner dependency to your private federation mono-repo? I tried using query-planner-js's package.json but it is dependent on internal-js and query-graph. Not sure if the federation repo has a package.json that points to all those 3 folders.

@abernix
Copy link
Member

abernix commented Jan 4, 2022

@AneethAnand Those are all referenced in this repository. This code doesn't live in any private federation monorepo. What you might be looking for is a release to be cut that includes this fix, which is something that hasn't yet been done but I understand should be coming in the next couple days. (Is that right, @pcmanus?)

federation/package.json

Lines 34 to 44 in 950eb93

"dependencies": {
"@apollo/composition": "file:composition-js",
"@apollo/federation-internals": "file:internals-js",
"@apollo/gateway": "file:gateway-js",
"@apollo/harmonizer": "file:harmonizer",
"@apollo/query-graphs": "file:query-graphs-js",
"@apollo/query-planner": "file:query-planner-js",
"@apollo/router-bridge": "file:router-bridge",
"@apollo/subgraph": "file:subgraph-js",
"apollo-federation-integration-testsuite": "file:federation-integration-testsuite-js"
},

@cpeacock
Copy link
Contributor

cpeacock commented Jan 4, 2022

@AneethAnand We should be cutting that new release sometime this week.

@AneethAnand
Copy link
Contributor Author

Thanks for the update @abernix and @cpeacock !

@pcmanus
Copy link
Contributor

pcmanus commented Jan 10, 2022

@AneethAnand Sorry for the delay, but 2.0.0-alpha.3 was published on Friday and contains that fix if you'd like to give it a shot. Thanks again for the report!

@AneethAnand
Copy link
Contributor Author

@pcmanus No problem I saw the newer version published on Friday and we have already started using it. Thanks again!

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 a pull request may close this issue.

4 participants