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

Bugfix: Don't mutate fragments that are attached to query documents #447

Merged
merged 1 commit into from
Jul 23, 2016

Conversation

nevir
Copy link
Contributor

@nevir nevir commented Jul 21, 2016

This fixes an issue that arises when you perform the same query multiple times, and that query has fragments.

Because addFragmentsToDocument appends fragments to queryDoc.definitions, each time the query is performed, the query document gains a new copy of each fragment. This is particularly egregious, since graphql-tag maintains an identity map for queries.

For example, the following currently results in a server error (duplicate fragment authorDetails) on the second query:

const fragments = createFragment(gql`
  fragment authorDetails on Author {
    author {
      firstName
      lastName
    }
}`);
const query = gql`{ author { ...authorDetails } }`;

client.query({query, fragments}); // Success
client.query({query, fragments}); // BOOM: Duplicate fragment authorDetails

nevir added a commit to convoyinc/apollo-client that referenced this pull request Jul 21, 2016
@@ -124,6 +125,7 @@ export function createFragmentMap(fragments: FragmentDefinition[]): FragmentMap
export function addFragmentsToDocument(queryDoc: Document,
fragments: FragmentDefinition[]): Document {
checkDocument(queryDoc);
queryDoc = cloneShallow(queryDoc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the cloneShallow works (as opposed to a deep clone) since we have OperationDefinitions as a top level attribute of the Document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah; definitions is the only value being mutated here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment about this? That we are basically using this to clone the definitions array?

Could we achieve this in a simpler way using:

queryDoc = {
  definitions: [...queryDoc.definitions],
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a few more properties on a Document than just the definitions - I'll go w/ assign (since we can't object spread in TypeScript)

@Poincare
Copy link
Contributor

Looks good to me other than the question I commented. Once this is rebased and @stubailo can review, we can probably merge.

@nevir
Copy link
Contributor Author

nevir commented Jul 22, 2016

rebased

@stubailo stubailo self-assigned this Jul 22, 2016
@stubailo
Copy link
Contributor

We either need to add a comment about the clone, or replace it with a more explicit clone of definitions specifically, then we can merge!

This fixes an issue that arises when you perform the same query multiple times, and that query has fragments.

Because `addFragmentsToDocument` _appends_ fragments to `queryDoc.definitions`, each time the query is performed, the query document gains a new copy of each fragment.  This is particularly egregious, since `graphql-tag` maintains an identity map for queries
@nevir
Copy link
Contributor Author

nevir commented Jul 23, 2016

updated to use assign instead of clone

@Poincare Poincare added the ready label Jul 23, 2016
@stubailo stubailo merged commit 7e65ae4 into apollographql:master Jul 23, 2016
@stubailo stubailo removed the ready label Jul 23, 2016
@stubailo
Copy link
Contributor

Thanks!

@nevir nevir deleted the immutable-docs branch July 23, 2016 09:07
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants