graphql-ppx icon indicating copy to clipboard operation
graphql-ppx copied to clipboard

Repeated fragment in auto-generated query.

Open Zimmi48 opened this issue 2 years ago • 11 comments

I've written a GraphQL request which uses several times the same fragment through different fragments (see the code below and https://github.com/coq/bot/blob/bc65314888df8e8c6c8bad55624f9492a36a1e78/bot-components/GitHub_GraphQL.ml#L30-L47 for the full context).

  fragment Column on ProjectColumn {
    id @ppxCustom(module: "ID")
    databaseId
  }

  fragment Project on Project {
    columns(first:50) {
      nodes { ... Column }
    }
  }

  fragment ProjectCards on ProjectCardConnection {
    nodes {
      id @ppxCustom(module: "ID")
      column { ... Column }
      project { ... Project }
    }
  }

The auto-generated query contains two copies of the Column fragment, this makes the GraphQL query ill-formed.

This is all the more annoying that, in this case, GitHub's GraphQL API doesn't answer with a clear error message but with:

{
  "data": null
}

(separately reported to GitHub)

Zimmi48 avatar Sep 13 '21 12:09 Zimmi48

Ah interesting! How would that work in JS?

jfrolich avatar Sep 13 '21 12:09 jfrolich

I'm an OCaml dev, so I'm not sure I can answer that. But what I did to debug the problem (because I didn't understand why GitHub was answering with an empty data field) was to print the request it was sending, then I confirmed with curl that it was not working:

curl https://api.github.com/graphql -X POST -H "Authorization: bearer $GH_TOKEN" -d '{"query":"query GetMilestoneMergedPullRequests($owner: String!, $repo: String!, $number: Int!)  {\nrepository(owner: $owner, name: $repo)  {\nmilestone(number: $number)  {\npullRequests(first: 100, states: [MERGED])  {\nnodes  {\n...PullRequestWithMilestoneAndCards   \n}\n\n}\n\n}\n\n}\n\n}\nfragment PullRequestWithMilestoneAndCards on PullRequest   {\nid  \ndatabaseId  \nnumber  \nmilestone  {\n...Milestone   \n}\n\nprojectCards(first: 20)  {\n...ProjectCards   \n}\n\n}\nfragment Milestone on Milestone   {\nid  \nnumber  \ntitle  \ndescription  \n}\nfragment ProjectCards on ProjectCardConnection   {\nnodes  {\nid  \ncolumn  {\n...Column   \n}\n\nproject  {\n...Project   \n}\n\n}\n\n}\nfragment Column on ProjectColumn   {\nid  \ndatabaseId  \n}\nfragment Project on Project   {\ncolumns(first: 50)  {\nnodes  {\n...Column   \n}\n\n}\n\n}\nfragment Column on ProjectColumn   {\nid  \ndatabaseId  \n}\n","variables":{"owner":"coq","repo":"coq","number":37}}'

Then I tested the auto-generated query in GitHub's GraphQL explorer:

query GetMilestoneMergedPullRequests($owner: String!, $repo: String!, $number: Int!)  {
  repository(owner: $owner, name: $repo)  {
    milestone(number: $number)  {
      pullRequests(first: 100, states: [MERGED])  {
        nodes  {
          ...PullRequestWithMilestoneAndCards   
        }
        
      }
      
    }
    
  }
  
}
fragment PullRequestWithMilestoneAndCards on PullRequest   {
  id  
  databaseId  
  number  
  milestone  {
    ...Milestone   
  }
  
  projectCards(first: 20)  {
    ...ProjectCards   
  }

}
fragment Milestone on Milestone   {
  id  
  number  
  title  
  description  
}
fragment ProjectCards on ProjectCardConnection   {
  nodes  {
    id  
    column  {
      ...Column   
    }
    
    project  {
      ...Project   
    }
  
  }

}
fragment Column on ProjectColumn   {
  id  
  databaseId  
}

fragment Project on Project   {
  columns(first: 50)  {
    nodes  {
      ...Column   
    }
  
  }

}
fragment Column on ProjectColumn   {
  id  
  databaseId  
}

and it highlighted the first Column fragment with the message There can be only one fragment named "Column".

Zimmi48 avatar Sep 13 '21 13:09 Zimmi48

Another simpler (and complete) example where it triggered this issue:

  fragment Milestone on Milestone {
    id
  }

  fragment PullRequest on PullRequest {
    id
    milestone { ... Milestone }
  }

  query issueMilestone($owner: String!, $repo: String!, $number: Int!) {
    repository(owner:$owner, name:$repo) {
      issue(number:$number) {
        id
        milestone { ... Milestone }
        timelineItems(itemTypes:[CLOSED_EVENT],last:1) {
          nodes {
            ... on ClosedEvent {
              closer {
                ... on PullRequest { ... PullRequest }
                ... on Commit {
                  associatedPullRequests(first: 2) {
                    nodes { ... PullRequest }
                  }
                }
              }
            }
          }
        }
      }
    }
  }

Zimmi48 avatar Sep 13 '21 13:09 Zimmi48

I think some production users haven't hit this issue because they use the template tag option (default for rescript-apollo-client). What client are you using? I think this should be fixed regardless btw.

jfrolich avatar Sep 13 '21 14:09 jfrolich

I am calling graphql-ppx directly (I am not aware of any available client for native OCaml).

Zimmi48 avatar Sep 13 '21 15:09 Zimmi48

This issue didn't happen with lower versions? (because handling of this shouldn't have changed too much I think)

jfrolich avatar Sep 13 '21 15:09 jfrolich

Before upgrading to 1.2.0, I didn't use fragments as much because the object encoding didn't make them as essential. So I don't think it's a regression but rather that I had never had an occasion of encountering this issue before.

Zimmi48 avatar Sep 13 '21 15:09 Zimmi48

Interesting. I think JS GraphQL clients might do some deduplication. Need to look into this a little deeper.

jfrolich avatar Sep 14 '21 06:09 jfrolich

Hey @jfrolich, do you think this issue is reasonably easy to fix? I tried to look at the code to see if I could help, but I don't understand the code enough to manage to do that.

Zimmi48 avatar Nov 04 '21 15:11 Zimmi48

BTW, you were right that (at least some) JS GraphQL clients do some deduplication: https://github.com/apollographql/graphql-tag/issues/27

Zimmi48 avatar Nov 05 '21 09:11 Zimmi48

I encountered the same issue while trying to implement a data-layer using GraphQL PPX. It generated a duplicate fragment, which yields in server error.

vknez avatar Jun 24 '22 17:06 vknez