Sometimes the same fragment is added to a query twice

[richburdon]

I have a parent container List that renders a list of child container Items. I also have pagination working (i.e., a fetchMore property that implements an updateQuery function).

However, if I change the value of one of the query variables (e.g., a "text" filter) then paging causes an error (the Item fragment is included twice as below).

From my experience with Relay, the parent query variables should be used to parameterize the fragment, but I don't know if that's relevant here, or possible (e.g., ${Item.getFragment('item', { filter: variables.filter })})

ApolloError: "GraphQL error: There can only be one fragment named "ItemFragment".

query GetItems($text: String, $offset: Int, $count: Int) {
  items(text: $text, offset: $offset, count: $count) {
    id
    ...ItemFragment
    __typename
  }
}

fragment ItemFragment on Item {
  id
  title
  __typename
}

fragment ItemFragment on Item { // Only happens when $text is changed.
  id
  title
  __typename
}

Unrelated, but one disadvantage of the convention to put fragments as static members of the component is you can't reference these from static propTypes {} member functions.

[stubailo]

Unrelated, but one disadvantage of the convention to put fragments as static members of the component is you can't reference these from static propTypes {} member functions.

Could you file this on react-apollo please?

The above looks like a bug, where we are accidentally appending the fragment twice.

Can you reproduce some of your code here? Where you are running the query and setting the variables, etc?

[richburdon]

Thanks @stubailo (other issue here apollographql/react-apollo#323):

Child Item:

export const ItemFragments = {
  item: new Fragment(gql`
    fragment ItemFragment on Item {
      id
      title
    }
  `)
};

export class Item extends React.Component {
  static propTypes = {
    item: ItemFragments.item.propType
  };
  render() {
    let { item } = this.props;
    return (
      <div>{ this.props.title }</div>
    );
  }
}

Parent List:

const GetItemsQuery = gql`
  query GetItems($text: String, $offset: Int, $count: Int) { 
    items(text: $text, offset: $offset, count: $count) {
      id
      ...ItemFragment
    }
  }
`;

  graphql(GetItemsQuery, {
    options: ({ text }) => {
      return {
        fragments: ItemFragments.item.fragments(),
        variables: {
          text: text,
          offset: 0,
          count: 10
        }
      };
    },

    props({ ownProps: { text }, data: { loading, items, fetchMore } }) {
      return {
        loading,
        text,
        items,
        fetchMoreItems() {
          return fetchMore({
            variables: {
              text: text,
              offset: items.length
            },
            updateQuery: (previousResult, { fetchMoreResult }) => {
              return _.assign({}, previousResult, {
                items: [...previousResult.items, ...fetchMoreResult.data.items]
              });
            }
          });
        }
      }
    }
  }),

NOTE: I was expecting to be able to parameterize "...ItemFragment" so that optional parts of the the parent's query can be passed down; for example something like:

  ...ItemFragment({ text })

[richburdon]

Temporary workaround:

networkInterface.use([{
  applyMiddleware({ request }, next) {
    let definitions = {};
    request.query.definitions = _.filter(request.query.definitions, (definition) => {
      let name = definition.name.value;
      if (definitions[name]) {
        return false;
      } else {
        definitions[name] = true;
        return true;
      }
    });

    next();
  }
}]);

[stubailo]

Great workaround.

If you could submit a failing test that would be super helpful as well: https://github.com/apollostack/apollo-client/blob/dc8cc0063d93a8c15a90771bc88183bc058b5e1b/test/fetchMore.ts

[richburdon]

OK. I'll try to wrap my head around that and have a stab at it (might be closer to Monday though).

Thanks for the comments.

[stubailo]

Yeah, we won't be able to get to a fix before Monday either. Thanks for the really detailed report!

[yurigenin]

I had the same issue. The workaround for me was to explicitly specify the same query and fragments in the call to fetchMore.

The problem is here:

ObservableQuery.prototype.fetchMore = function (fetchMoreOptions) {
        var _this = this;
        return Promise.resolve()
            .then(function () {
            var qid = _this.queryManager.generateQueryId();
            var combinedOptions = null;
            if (fetchMoreOptions.query) {
                combinedOptions = fetchMoreOptions;
            }
            else {
                var variables = assign({}, _this.variables, fetchMoreOptions.variables);
                combinedOptions = assign({}, _this.options, fetchMoreOptions, {
                    variables: variables,
                });
            }

            var fullQuery = getFromAST_1.addFragmentsToDocument(combinedOptions.query, combinedOptions.fragments);
            combinedOptions = assign({}, combinedOptions, {
                query: fullQuery,
                forceFetch: true,
            });
             ........

In the else statement _this.options already contains a full query that includes fragments. So when we call addFragmentsToDocument we add the same fragments again resulting in duplicate fragments which causes validation errors.

Initially, the fragments were already added to the query in the ApolloClient.watchQuery.

When a variable for a query changes the GraphQL.subscribeToQuery is called but since we already have a subscription we do not create a new query but instead call ObservableQuery.setOptionson the existing one that does not reset the options.

[stubailo]

Looks like the fix would be in here: https://github.com/apollostack/apollo-client/blob/372bca9881b374caf56d0cbd0795a88dcc13ec0f/src/queries/getFromAST.ts#L150

[helfer]

@stubailo are you suggesting that we "fix" this by checking whether a fragment with the same name already exists on the query? The other fix would be not to attach it twice in the first place. I'm leaning towards a simple fix for now, because we'll soon want to refactor how we transform queries anyway.

[jnwng]

@helfer see #906 for a (fairly) simple fix for this particular issue (which is not attaching it twice in the first place)

warnings about fragment name collisions are already being surfaced in createFragment

[helfer]

Solved in #913