Only attach distinct fragments to document #906

jnwng · 2016-11-14T07:21:36Z

See this issue on react-apollo to see what lead to this coming to this particular repo, and #897 for information on the bug / temporary workarounds.

If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass
~~Update CHANGELOG.md with your change~~ (omitted because I'm not fully sure I should be doing this)
Add your name and email to the AUTHORS file (optional)
~~If this was a change that affects the external API, update the docs and post a link to the PR in the discussion~~ (omitted because it does not change an external API)

Attaching multiple references to the same fragment results in the creation of an invalid document - we want to make sure we’re only attaching distinct fragments to the document.

Steps to Reproduce

Attaching multiple references to the same fragment like:

const someFragment = createFragment(gql`
  fragment SomeFragment on SomeEdge {
    someField
  }
`);

const someOtherFragment = createFragment(gql`
  fragment SomeOtherFragment on SomeEdge {
    ...SomeFragment
  }
`, [someFragment, someFragment]);

results in a query document that looks like

query {
  fragment SomeOtherFragment on SomeEdge {
    ...SomeFragment
  }
  fragment SomeFragment on SomeEdge {
    someField
  }
  fragment SomeFragment on SomeEdge {
    someField
  }
}

(note that the fragment definition gets attached more than once). Per the GraphQL spec, having multiple fragments with the same name is invalid, and while we guard against registering multiple fragments with the same name here, we need to also make sure we're not attaching multiple instances of the same fragment in our document.

Do also note that the example above is boiled down to the exact steps that it would take to reproduce the issue, but in more realistic use cases this occurs when sibling fragments both include a dependency on another fragment (they wouldn't know that the other fragment had already included the required fragment in the document).

helfer

@jnwng This is a great PR, and I think we should merge it, but as far as I can tell it won't solve #897, because the issue there is that the query document already exists with all the fragment definitions, and then we're adding the same fragments again, which this PR won't prevent as far as I can tell.
Could you update it to fix #897 as well?

helfer · 2016-11-14T22:19:18Z

src/queries/getFromAST.ts

@@ -154,6 +155,6 @@ export function addFragmentsToDocument(queryDoc: Document,
  }
  checkDocument(queryDoc);
  return assign({}, queryDoc, {
-    definitions: queryDoc.definitions.concat(fragments),
+    definitions: queryDoc.definitions.concat(uniq(fragments)),


This won't be sufficient to avoid duplicates if one copy is already present in the definitions and another copy is added. An easy fix would be to apply uniq to the entire list of definitions, not just the fragments added.

good point, will update.

Attaching multiple references to the same fragment results in the creation of an invalid document - we want to make sure we’re only attaching _distinct_ fragments to the document.

Its possible that fragments have already been defined in the document itself - we want to check against those to make sure we’re only attaching fragments that either 1) don’t already exist in the document 2) are distinct in the fragments to be attached to the document

jnwng · 2016-11-15T01:23:26Z

what you said made sense - the fragment definition could exist in the document already. however, just using uniq won't work because the FragmentDefinition created by the document and the one being attached are potentially different.

basically, i'm filtered the fragments that we're attaching by their name so we don't duplicate the fragment names, however this may result in some unintended behavior (basically, if there's a name collision here, we probably need to throw a warning or something, because those fragments could have different bodies, and those fragments may get dropped silently). now that i've written that out, its also possible to that the fragments that are being attached are named the same but have different bodies, which will also fail the uniq check.

i've updated this so we can discuss, but i think i'm going to have to take another pass to check the fragment name and the SelectionSet. or is that doing too much?

stubailo · 2016-11-15T04:06:32Z

basically, if there's a name collision here, we probably need to throw a warning or something, because those fragments could have different bodies

We get a free pass here because createFragment already validates that it's impossible to declare the same fragment name twice with a different body, AFAIK. So checking the names should be enough.

stubailo · 2016-11-15T04:20:48Z

Oh, by the way - graphql-tag also enforces that documents with the same contents are === each other. So I think uniq should be sufficient?

Fix error handling for observer.next

jnwng · 2016-11-15T13:55:41Z

@stubailo for some reason the fragment definition coming from the query body isn't matching the one i'm attaching via createFragment, but that's the last piece here that will get this working

As mentioned in the feedback, we can actually just run `uniq` on the concat of the definitions and the fragments because the definitions that already exist in the document will always win over the attached fragments. However, slightly unexpectedly, FragmentDefinitions generated in the query document don’t `===` the same definition via `createFragment`, so we’ll use object equality to remove duplicate definitions.

jnwng · 2016-11-15T14:12:24Z

src/queries/getFromAST.ts

@@ -154,6 +156,6 @@ export function addFragmentsToDocument(queryDoc: Document,
  }
  checkDocument(queryDoc);
  return assign({}, queryDoc, {
-    definitions: queryDoc.definitions.concat(fragments),
+    definitions: uniqwith(queryDoc.definitions.concat(fragments), isequal),


@stubailo unfortunately, with the test i provide in this PR: console.log(queryDoc.definitions[1] === fragments[0]); // false, even though the fragment definition itself is identical. not exactly sure why that is given that you mentioned that graphql-tag should be returning the same definition, but in any case, using the object equality check here properly removes the duplicate definitions.

@jnwng Thanks! I'll take a look to find out what's going on so we can merge this today.

@jnwng the reason they're not === is that graphql-tag ensures that documents are ===, but not that individual definitions are ===. If you create the exact same document twice, the definitions will be ===, but that's more by accident than by design.

As far as I can tell we have two options:

use uniqwith with isequal as you did

use uniq and say that nobody should be adding fragments to a doc in two different ways

My preference is option 1. Option 2 would have better performance, but unless we put in checks that make sure people don't add fragments in ways that are not supported, it's going to be harder to debug when things go wrong.

@jnwng @stubailo thoughts?

Actually, there's a third option, which is to copy just the query or mutation definitions of any document passed to apollo-client, and pass all fragment definitions through createFragment before re-attaching them. Personally I don't like that option because I think it's overly complicated.

After talking to @stubailo we decided to go with option 2 for performance reasons, and because we're planning on doing a refactor that takes fragment handling completely out of apollo-client. I've made the test more comprehensive to make sure solution 2 actually works in the cases we expect to see.

Makes sense to me. Thanks for the explanation and context @helfer!

helfer · 2016-11-15T17:44:34Z

Also relevant: apollographql/graphql-tag#17

jnwng mentioned this pull request Nov 14, 2016

Sometimes the same fragment is added to a query twice #897

Closed

helfer reviewed Nov 14, 2016

View reviewed changes

jnwng added 3 commits November 14, 2016 17:18

Only attach distinct fragments to document

f21ca6d

Attaching multiple references to the same fragment results in the creation of an invalid document - we want to make sure we’re only attaching _distinct_ fragments to the document.

Adding name to AUTHORS

3e8825a

jnwng force-pushed the jw/uniq-fragments branch from e859801 to 0187354 Compare November 15, 2016 01:19

cannoneyed and others added 3 commits November 14, 2016 23:18

Fix error handling for reading from store

3065493

Merge branch 'master' into fix-error-handling

a8d213a

Merge pull request apollographql#910 from cannoneyed/fix-error-handling

2ae02e7

Fix error handling for observer.next

jnwng commented Nov 15, 2016

View reviewed changes

Merge branch 'master' into jw/uniq-fragments

28c64f5

helfer changed the base branch from master to unique-fragments November 15, 2016 16:14

helfer merged commit 7a09470 into apollographql:unique-fragments Nov 15, 2016

helfer mentioned this pull request Nov 15, 2016

Unique fragments #913

Merged

5 tasks

jnwng mentioned this pull request Dec 7, 2016

Fragment definitions within a graphql document ought to be unique. apollographql/graphql-tag#27

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only attach distinct fragments to document #906

Only attach distinct fragments to document #906

jnwng commented Nov 14, 2016

helfer left a comment

helfer Nov 14, 2016

jnwng Nov 15, 2016

jnwng commented Nov 15, 2016 •

edited

Loading

stubailo commented Nov 15, 2016

stubailo commented Nov 15, 2016

jnwng commented Nov 15, 2016

jnwng Nov 15, 2016

helfer Nov 15, 2016

helfer Nov 15, 2016

helfer Nov 15, 2016

helfer Nov 15, 2016

jnwng Nov 15, 2016

helfer commented Nov 15, 2016

Only attach distinct fragments to document #906

Only attach distinct fragments to document #906

Conversation

jnwng commented Nov 14, 2016

Steps to Reproduce

helfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnwng commented Nov 15, 2016 • edited Loading

stubailo commented Nov 15, 2016

stubailo commented Nov 15, 2016

jnwng commented Nov 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helfer commented Nov 15, 2016

jnwng commented Nov 15, 2016 •

edited

Loading