Overlapping fragment sub-queries are not merged

[kav]

Describe the bug
I've got a structure like

 fragment A on Users {
  userId
  role
  organization {
    name
  }
}

query HomeScreenQuery($userId: UUID) {
  usersCollection(filter: { userId: { eq: $userId } }, first: 1) {
    edges {
      node {
        ...A
        userId
        role
        organization {
          preferProjects
        }
      }
    }
  }
}

And seeing error

Payload did not contain a value for field preferProjectName: preferProjectName. Check that you are parsing with the same query that was used to fetch the payload

Expected behavior
I'd expect the payload to contain the merged result from the fragement and the query. Seems like maybe that level of merging is not yet implemented?

Versions:

• PostgreSQL: 14.1
• pg_graphql commit ref: '0.4.0'

Additional context
A simpler example of overlapping fragments and discussion on expected handling can be seen here:
graphql/graphql-spec#399 (comment)

[olirice]

thanks for opening & the reference to the spec

Seems like maybe that level of merging is not yet implemented?

correct, the current behavior is never to merge. The backend does resolve both, but one will overwrite the other in the result payload. I'll dig through the edge cases, check where that isn't compliant, and add some notes here

--

In the short term, providing an alias to one or both of the instances of organization will cause both selection sets to return (not merged) and is backwards compatible as we update the behavior to be spec compliant

[kav]

Thanks for the quick reply and great workaround @olirice. You rock!

[fnimick]

Any word on this? This breaks automatic pagination with some clients that insert duplicate edges fields to retrieve cursor information for pagination.

[olirice]

@fnimick could you please send more info about which clients have that behavior? That may increase the priority of this issue if its a common one

[fnimick]

In particular, Houdini (the most popular / fully-featured graphql client for Sveltekit) has an automatic pagination feature that inserts a duplicate edges subquery to select the cursor for pagination. It's easy to work around by adding an alias to the edges that you are selecting, though.

[kav]

It's still a bit of a pain for me as well FWIW. One thing that might be worth discussing is the spec behavior seems to be to merge the fragments before resolving and which filter, etc keys to use to disambiguate that merge. It sounds, and I haven't reviewed the code as yet so I'm out on a limb here, like you're resolving each separately and then merging.

[fnimick]

@kav "The backend does resolve both, but one will overwrite the other in the result payload." - you are correct, this behavior is not spec compliant, and apparently it will be updated to be spec compliant in a future release.

[bryanmylee]

Another point mentioned that might increase priority on this is that overlapping fragments are often located independently of each other.

In a larger application, it's often difficult to make sure every fragment has a unique alias to avoid the overlaps from overwriting each other.

Furthermore, the aliases make it almost impossible to do optimistic local cache mutations with clients like Apollo, since we have to update an increasing number of fields for every fragment, made even more difficult if each fragment depends on different fields.

// adding a new child to a parent node
function addChildOptimistically() {
  parent.childrenCollection.edges.push({ // used by XFragment
    node: { ... }
  });
  parent.y_childrenCollection.edges.push({ // used by YFragment
    node: { ... }
  });
  parent.z_childrenCollection.edges.push({ // used by ZFragment
    node: { ... }
  });
  // ...
}

If overlapping fragments worked as intended, our team would just prohibit aliases (unless the field has ordering / filter properties), and we can clean up our optimistic update code to:

function addChildOptimistically() {
  parent.childrenCollection.edges.push({
    node: { ... }
  });
}

[kav]

Will second Bryan to say the bugs this causes are an absolute pain in the rear to fix. A lot of action at a distance. Fragments basically end up cache poisoning each other.

[bryanmylee]

@olirice I'm working on a fix for this at the moment which involves a custom deep-merge function that properly combines the fragment results instead of overriding the values stored at the field key in the result map.

However, I need pointers on how fragments should be merged across collections.

If I have two fragments:

query {
  ...title_query
  ...content_query
}

fragment title_query on Query {
  blogPostCollection {
    edges {
      node {
        title
      }
    }
  }
}

fragment content_query on Query {
  blogPostCollection {
    edges {
      node {
        content
      }
    }
  }
}

The result should be (based on experiments with Hasura):

{
  "blogPostCollection": {
    "edges": [
      {
        "node": {
          "title": "Blog post 1",
          "content": "Lorem ipsum"
        }
      },
      {
        "node": {
          "title": "Blog post 2",
          "content": "dolor sit"
        }
      }
    ]
  }
}

Initially, I thought this meant that every object needed to be uniquely identifiable, even when id is not queried for, to allow different fragment results to be merged properly.

However, it seems like Hasura just merges the objects by index and returns an error if two collection fields are requested with different arguments.

{
  "errors": [
    {
      "message": "inconsistent arguments between multiple selections of field 'teamConnection'",
      "extensions": {
        "path": "$.selectionSet",
        "code": "validation-failed"
      }
    }
  ]
}

Should we take that same approach?

[imor]

That is a valid approach to follow as per the spec. See Field Selection Merging and Field Collection for details.

[kav]

@bryanmylee if you take a look at the graphql spec conversation I linked up top it has a solid explanation of how you might allow connections with different parameters in the same payload using unique keys and does a pretty good job walking through the cases.

One big call out and you may be doing the right thing but phrasing it differently than I expect: the merge should happen before resolution. You aren't merging results you should be merging before querying the db. That will have a number of benefits in that a lot of questions about what can be safely merged and what can't will be extremely clear on the query side.

I can add a bit more detail if any of this is unclear or seems nonsensical. Traveling at the moment so a bit of mobile stream of consciousness.

[bryanmylee]

I'm getting the hang of the library. A few questions though: how should we merge the spans and positions of two selection sets? If two fragments for example have the spans 20:28--30:9 and 8:28--16:9, can we just assume that the merged field will have a selection set with minimal span that encapsulates both merged sets e.g. 8:28--30:9?

This isn't really covered by the GraphQL spec and seems to be more of a implementation detail for debugging, error reporting, and tracing.

[olirice]

If two fragments for example have the spans 20:28--30:9 and 8:28--16:9, can we just assume that the merged field will have a selection set with minimal span that encapsulates both merged sets e.g. 8:28--30:9?

that sounds most reasonable to me, I'm not sure what else could be done that would still count as correct

[bryanmylee]

If two fragments for example have the spans 20:28--30:9 and 8:28--16:9, can we just assume that the merged field will have a selection set with minimal span that encapsulates both merged sets e.g. 8:28--30:9?

that sounds most reasonable to me, I'm not sure what else could be done that would still count as correct

I've updated the PR to include that behaviour, it's ready for your review!