Relay: Fragment issue when committing Mutation

[danielgriese]

Hi there,

I have just implemented a new feature in our product using Relay and it worked like a charm so far. Thank you for your awesome work and for open sourcing Relay!

Unfortunately I have encountered a tricky issue, that I currently do not know how to circumvent and if it is a bug in relay or if my schema is just programmed in a stupid way. Both options are valid at this point in time... :D

Situation

I have page showing a "Claim" that refers to a "Medium". This claim has an applied "Policy" that can be changed by a mutation. All these types implement a common Resource interface.
The shortened Schema looks like this.

interface Resource {
  id: ID!
  name: String
}

type Claim implements Node, Resource {
  id: ID!
  name: String
  medium: Medium
  policy: Policy
}

type Medium implements Node, Resource {
  id: ID!
  name: String
  remoteUrl: String
}

type Policy implements Node, Resource {
  id: ID!
  name: String
}

The mutation is described as this:

type Mutation {
  updateClaimPolicy(input: UpdatePolicyInput!): UpdatePolicyPayload
}

input UpdatePolicyInput {
  id: ID!
  policy: String!
  clientMutationId: String!
}

type UpdatePolicyPayload {
  claim: Claim
  clientMutationId: String!
}

The client side mutation code:

export default class UpdateClaimPolicyMutation extends Relay.Mutation {

  // we depend on the claim id, so make sure we get it
  static fragments = {
    claim: () => Relay.QL`
      fragment on Claim {
        id
      }
    `,
  };

  getMutation() {
    return Relay.QL`mutation {updateClaimPolicy}`;
  }

  getVariables() {
    return { id: this.props.claim.id, policy: this.props.policy._id };
  }

  /**
  * All that could change...
  * For now this is only the claims policy and the updater
  */
  getFatQuery() {
    return Relay.QL`
      fragment on UpdatePolicyPayload {
        claim {
          policy
          updatedOn
          updatedBy
        }
      }
    `;
  }

  // tell relay how to react to the payload...
  // in this case, update the claim
  getConfigs() {
    return [{
      type: 'FIELDS_CHANGE',
      fieldIDs: {
        claim: this.props.claim.id,
      },
    }];
  }

}

The displaying container has the following (shortened) fragment:

claim: () => Relay.QL`
      fragment on Claim {
        _id
        medium {
          name
        }
        policy {
          name
        }
    }
    `,

So far everything is fine. I can navigate to the page and update the policy by the given mutation.

Problem

I have another page showing a list of occurred activities in the system.
An activity has a generic target that can be any type implementing the Resource interface.

type Activity implements Node {
  id: ID!
  time: DateTime
  user: User
  action: String
  target: Resource
}

As I need to render the target differently depending on the type, I need to query for different data for different objects.

const mediumFragment = Relay.QL`
  fragment on Medium {
    name
    remoteUrl
  }
`;

const claimFragment = Relay.QL`
  fragment on Claim {
    medium {
      ${mediumFragment}
    }
  }
`;

export default Relay.createContainer(ActivityLine, {
  fragments: {
    activity: () => Relay.QL`
      fragment on Activity {
        id
        time
        user {
          _id
            fullName
        }
        action
        target {
          id
          __typename
          name
          ${mediumFragment}
          ${claimFragment}
        }
      }
    `,
  },
});

I need both fragments, as the target can either be a Medium directly or a Claim whos medium information I want to get as well.

This still works fine!
But when I show an activity with a claim (and expanded Medium) and then navigate to the claim edit-page (which still works fine btw) and execute the change policy mutation, I get the following error:

Fragment \"F3\" cannot be spread here as objects of type \"Claim\" can never be of type \"Medium\".

In the network tab, I see the request to the server asking for a Fragment on Medium, even though the medium is not in the mutation or its fat query.

"mutation UpdateClaimPolicyMutation($input_0:UpdatePolicyInput!){updateClaimPolicy(input:$input_0){clientMutationId,...F5}} fragment F0 on User{_id,fullName,id} fragment F1 on Claim{id} fragment F2 on Claim{policy{_id,name,id},id} fragment F3 on Medium{id} fragment F4 on Node{id} fragment F5 on UpdatePolicyPayload{claim{policy{_id,id},updatedBy{id,...F0},updatedOn,id,...F1,...F2,...F3,...F4,policy{_id,id},updatedBy{id,...F0},updatedOn,id}}"

Random Guessing

I guess the issue is that Relay is internally still tracking the combined queries from activities and the claim edit page and is somehow mixing up the requested fragments.
Of course a claim can never be a Medium! But I never asked to do that. I asked to get a Medium-Fragment on a Resource earlier, which might be a Claim. But not both at the same time of course.

So this seems kind of strange to me. Why would it try to apply the Medium Fragment on the Claim.

• It only happens when I load the activities first (my guess is tracking of the nodes/queries).
• And only when I make a mutation.

Querying the data works without any issue and making the mutation without accessing the activities first also works smoothly.

Do you have any possibilities to replicate this issue or have any comments how this can be avoided?
I wouldn't have a problem to simply "kill" tracked queries from my Activity-List page when I navigate away from it. I do not need any caching or anything else here. But I guess this is not the root-cause in this issue.

Thank you very much in advance for your support and kind regards,
Daniel

[NevilleS]

This reminds me of #750. It likely shares the same root cause, probably triggered by the fact that the node that is being updated by the mutation, in your case claim, has been used in previous queries as a more generic type (in your case as the target in your ActivityLine container)...

So this seems kind of strange to me. Why would it try to apply the Medium Fragment on the Claim.

My hunch is that the Claim in this case corresponds to the target in your previous query, which was a more generic interface. When that node was queried in the past, the tracked query included both the Medium and Claim fragments (rightfully so). Tracked queries are stored by ID, so it wouldn't be surprising to see that this gets muddled up. In other words, it's unlikely that the mutation query intersection currently considers the _type_ of the field in the mutation payload as a way to narrow the intersection to prune fragments. That might be the fix, although it's a pretty complex thing to do!

One thing you could do to workaround this issue in your code is to probably just loosen the type of the node in your mutation payload. Right now it's a Claim, but if you made it a Target, knowing full well it will _always_ be a Claim, then you probably would be safe. And yes, I'm aware of how gross that sounds :smile:

[NevilleS]

OK, we ran into this issue in our codebase now too, so I was able to do a bit more digging.

So the immediate issue here (invalid mutation queries) appears to be caused by how intersectRelayQuery simply includes all tracked fields for a node without paying any attention to the _type_ of the node in the mutation payload. That means if you had a query like:

node(id: "123") {
   ... on User { email }
   ... on Blog { title }
}

And a mutation like this:

class UpdateBlogMutation extends Relay.Mutation {
  getFatQuery() {
    return Relay.QL`
      fragment on UpdateBlogMutationPayload {
        blog
      }
    `
  }

  getConfigs(): Relay.MutationConfig[] {
    return [{
      type: "FIELDS_CHANGE",
      fieldIDs: {
        blog: this.props.blogID  // "123"
      }
    }]
  }

  getMutation() { return Relay.QL`mutation { update_blog }` }
  getVariables() { return this.props.blog }
}

...intersectRelayQuery will take the tracked fields for the node with id "123", including the two fragments (on User and Blog), and then try to expand them, something like this:

mutation update_blog(input: {...}) {
  blog {
     ... on User { email }
     ... on Blog { title }
  }
}

And that's how you get bad queries!

[NevilleS]

Amusingly, I found a rather obnoxious workaround: just wrap the problematic container fragments in a generic inline fragment so that the tracked query is valid :laughing:

i.e. change this:

node(id: "123") {
   ... on User { email }
   ... on Blog { title }
}

...to this:

node(id: "123") {
   ... on Node {
     ... on User { email }
     ... on Blog { title }
  }
}

Since intersectRelayQuery basically just copies over the tracked fields, this becomes a mutation query like this:

mutation update_blog(input: {...}) {
  blog {
     ... on Node {
       ... on User { email }
       ... on Blog { title }
    }
  }
}

Since the invalid fragment expansion (User within Blog) has been indirected via a generic Node fragment first, it works!

[josephsavona]

...intersectRelayQuery will take the tracked fields for the node with id "123", including the two fragments (on User and Blog), and then try to expand them

@NevilleS Thanks for debugging this - it's super helpful to have a repro. Since we know the type of each field/fragment, intersectRelayQuery should be able to filter out tracked fragments of a mismatched type. In your example, given a concrete parent field blog of type Blog, intersection can skip concrete fragments (!fragment.isAbstract()) of any other type. If either the parent or child is abstract it _should_ be safe to keep (intersect) the child. Any interest in submitting another great PR? ;-)

[NevilleS]

Yup, I'd be happy to write a fix :+1:. You're saying intersectRelayQuery would be the place for this?

OK, so you're saying the Relay AST gives us a way to know definitely that a given fragment is abstract (e.g. Node) vs. concrete (e.g. Blog). That would be a relatively coarse filter though, because it's still entirely possible for the payload to be an abstract type, but not be compatible with some of the tracked fragments on the node itself...

For example, let's say Blog is, itself, abstract and has a few implementations: PhotoBlog, TextBlog, VideoBlog, etc. So our initial query might actually look like this:

node(id: "123") {
   ... on User { email }
   ... on Blog {
    title
    ... on PhotoBlog { photo }
    ... on TextBlog { text }
    ... on VideoBlog { video }
  }
}

With that in mind, the mutation payload type might be abstract, but we still know that we can't expand the User query in it:

mutation update_blog(input: {...}) {
  blog {
     ... on User { email }
     ... on Blog {
      title
      ... on PhotoBlog { photo }
      ... on TextBlog { text }
      ... on VideoBlog { video }
    }
  }
}

What we'd really need to do would be determine whether the given fragment type is a valid option for the payload type. Is there an existing way to do that using the Relay AST?

[NevilleS]

In other words, I think we basically need to do exactly what GraphQL does, which is compare if the types overlap: see https://github.com/graphql/graphql-js/blob/master/src/utilities/typeComparators.js#L105 for an example of this.

The babel relay plugin does validation like this during transformation, but I get the feeling that a lot of that type information is discarded, or at least not immediately accessible from the RelayQuery...

[NevilleS]

One approach would be to modify the babel relay plugin to dump the "possible types" (available via GraphQL) into the metadata that actually gets stored by Relay, which would give us the information needed to perform the type intersection...

[josephsavona]

The list of possible types for a field/fragment could be quite large, so we'd prefer to avoid any solution that would require having that list on the client (e.g. as part of query metadata). Let me look into this a bit more and report back here.

[NevilleS]

Yeah, agreed... There are much more efficient ways to store the type
information required that don't involve decorating every fragment with
their possible types. Some kind of lookup...

On Tue, Feb 9, 2016, 9:13 PM Joseph Savona [email protected] wrote:

The list of possible types for a field/fragment could be quite large, so
we'd prefer to avoid any solution that would require having that list on
the client (e.g. as part of query metadata). Let me look into this a bit
more and report back here.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/relay/issues/782#issuecomment-182169457.

[josephsavona]

Unfortunately, lookups still require the type information to be on the client :-/

[josephsavona]

A generalized version of the problem is as follows. Data is loaded via some fragment:

fragment on Task {
  attachment { # type: Attachment implements Document
    viewerHasSeen # some random thing to mutate
    ... on Photo { ... }
    ... on Video { ... }
  }
}

Note that a given attachment record could also be loaded in other places in the app, perhaps in a list of documents:

fragment on User {
  documents { # type: [Document]
    ... on Attachment { ... }
    ... on Other { ... } # where an `Attachment` cannot be an `Other`
  }
}

This means that it's possible to have a tracked fragment on Other for some attachment, such that it could generate the following mutation query, where a FIELDS_CHANGE config sets the id for field attachment to be some Attachment record:

mutation MarkAttachmentAsSeen(attachmentID: $id) {
  attachment {
    viewerHasSeen # valid, is a field on Attachment
    ... on Attachment # valid
    ... on Other # invalid composition
  }
}

Relay cannot skip tracking the ... on Other fragment since it _is_ a valid fragment that should be refetched in some cases, depending on the mutation. Relay also doesn't have enough information to know which tracked fragments can or can't be included in the mutation query (without having access to the full type hierarchy, which is impractical to load in the client).

This suggests a two-part solution:

1. Mutation fields must have generic types, e.g. the attachment field on the above mutation would have to be typed as Node, rather than Attachment. Relay could warn if it finds a mutation field with a specific type, although this requires both the fat query and config, and therefore may not be feasible to test during the build stage.
2. Relay should construct queries by wrapping any tracked fields (as opposed to fragments) within a fragment with the record's type. In this case, it would generate a mutation query such as:

mutation MarkAttachmentAsSeen(attachmentID: $id) {
  attachment {
    ... on Attachment { # generated by Relay
      hasViewerSeen
    }
  }
}

This would impose an extra restriction on GraphQL schema, so I'm still curious if there is an alternative.

cc @wincent @yuzhi

[NevilleS]

Well, one thing to remember is that it's perfectly valid (by GraphQL standards) to add that "indirection" layer that I mentioned in an earlier comment. So you could, technically, just always wrap the overall mutation query in a ... on Node fragment and it would probably be valid GraphQL...

For example, this fails:

mutation MarkAttachmentAsSeen(attachmentID: $id) {
  attachment {
    viewerHasSeen # valid, is a field on Attachment
    ... on Attachment # valid
    ... on Other # invalid composition
  }
}

...but if we did this, it'd be valid:

mutation MarkAttachmentAsSeen(attachmentID: $id) {
  attachment {
    ... on Node { # added by the mutation query building
      viewerHasSeen # valid, is a field on Attachment
      ... on Attachment # valid
      ... on Other # valid composition
    }
  }
}

This "solution" seems very suboptimal, since it's easy to infer that the ... on Node is unnecessary and that the ... on Other is impossible to ever expand, but it'd work.

[josephsavona]

Yeah the indirection via the anonymous Node fragment also works and is probably the least intrusive solution. If we also filter non-matching concrete types during tracked query intersection, this should eliminate most of the non-matching fragments in practice and those that aren't will be ignored.

[danielgriese]

Wow, thank you for investigating this so quickly!

I can confirm that wapping the query in the abstract type, even though it seems unnecessary, does solve the issue.

I did not change the mutation, I instead changed the Query when showing the activity list, as this one contained the different fragments.

export default Relay.createContainer(ActivityLine, {
  fragments: {
    activity: () => Relay.QL`
      fragment on Activity {
        [...]
        target { # this is always of type Resource, but still we need to wrap it for now...
          ... on Resource {
             id
             __typename
             name
             ${mediumFragment}
             ${claimFragment}
          }
        }
      }
    `,
  },
});

I had to use target { ... on Resource } instead of Node, as "name" wouldn't be known.
But it's good to know that the generic Interface Type also works.

Still strange though as my schema already defines "target" to be of type Resource. Looks like unnecessary double-casting. But hey, if it does the job. Why not?! It's ok for me as a work around right now. :D

Thanks and kind regards,
Daniel

[josephsavona]

Still strange though as my schema already defines "target" to be of type Resource. Looks like unnecessary double-casting

@danielgriese Thinking purely from the perspective of the mutation query, it is unnecessary casting. The difficulty is that the Claim was also fetched in other contexts (as a Resource), so there are valid fragments on the claim (... on Medium) that are not valid Claims. We can change Relay to automatically output the extra fragment casts to avoid the need for it in components, but this is a good workaround for now.

[josephsavona]

It looks like the fix here is to wrap all mutation fragments in ... on Node { ... on FieldType { ${originalFragment} } } - @NevilleS still interested in that PR? :-)

[NevilleS]

Yeah, I can do it... it's so gross though! LOL

[NevilleS]

Are we sure the ... on Node is always valid? Intrinsically I believe that to be true, since fragment tracking only happens on nodes, but...

[josephsavona]

... on Node may not always be valid; we may have to add a boolean metadata flag indicating whether a field's type implements Node. I'd have to double-check writeRelayUpdatePayload to find any edge-cases that support mutation fields without ids.

[NevilleS]

Hey @josephsavona, I've been out for a week or so on "IRL" stuff, but I hope to pull a PR together this week. Are we in agreement that adding a defensive wrapper like ... on Node { ... on FieldType { ${originalFragment} } } during the mutation query building step is the best solution today? That'd be pretty easy, but would of course make _all_ mutation queries that much uglier :laughing:

[josephsavona]

@NevilleS Yes, the general shape of the solution - wrapping tracked fields in conditioning fragments - is correct. It isn't exactly clear what the most efficient way to determine the fragment type to wrap the field in (perhaps we have to store the parent field type of a fragment when we track it?).

cc @wincent who is focusing on simplifying mutations - he might have more ideas here.

[NevilleS]

Actually, I was thinking I'd be able to just use the type of the node in
the mutation payload, unrelated to the tracked fragments. This means the
query tracking is unaffected and only the mutation query building logic
needs to be aware of this.

I haven't tried anything yet though - is it possible to get the type
information of the mutation payload at runtime? Is that stored at all?

On Tue, Feb 23, 2016, 1:35 AM Joseph Savona [email protected]
wrote:

@NevilleS https://github.com/NevilleS Yes, the general shape of the
solution - wrapping tracked fields in conditioning fragments - is correct.
It isn't exactly clear what the most efficient way to determine the
fragment type to wrap the field in (perhaps we have to store the parent
field type of a fragment when we track it?).

cc @wincent https://github.com/wincent who is focusing on simplifying
mutations - he might have more ideas here.

—
Reply to this email directly or view it on GitHub
https://github.com/facebook/relay/issues/782#issuecomment-187568438.

[josephsavona]

Do you mean the type of the field in the fat query? You can get the type of a field with RelayQueryNode#getType, but that wouldn't help since:

fragment on Foo {
  bar { ... }
}

Is conceptually identical to :

fragment on Foo {
  bar {
    ... on BarType { ... {
  }
}

[NevilleS]

Alright, I started poking around in the source a bit today to figure out how I might approach this.

I think we agree that it's generally true that an "indirection layer" is needed in the mutation query, so that we don't include invalid tracked queries. After some thought, I don't think we need to do any type inspection at all (of parents, etc.); just inserting a single ... on Node fragment _should_ be sufficient. I could be forgetting a corner case, though.

In other words, intersected mutation fragments typically look something like this:

fragment F0 on SomeMutationPayload {
  some_node_field { // this *must* implement Node, by definition
    ...F0  // tracked query
    ...F1  // tracked query
    ...F2  // tracked query
  }
}

The issue is that one of F0, F1, F2 might be an invalid type since some_node_field will probably be a concrete type, not just Node... and we want to "fix" this by inserting an indirection layer like this:

fragment F0 on SomeMutationPayload {
  some_node_field {
    ... on Node {
      ...F0  // tracked query
      ...F1  // tracked query
      ...F2  // tracked query
    }
  }
}

Alright, so the fix is simplified down to "insert another fragment as a child of the intersected query", I think. When I started poking around I was hoping to do this in RelayMutationQuery, but then I realized that intersectRelayQuery is the one that creates the entire some_node_field { ... } fragment, so I started poking around in that traversal. After a bit in there I changed my mind and I think I'll add code to RelayMutationQuery to "transform" the intersected query, since it really doesn't seem like something that intersectRelayQuery should "know" about.

I think I really just need to play with the Relay AST a bit more and figure out how to transform the intersected query with this indirection fragment.

Note that this will break a _lot_ of unit tests :smile:

[NevilleS]

Yeah, that doesn't work... Tracked queries aren't always fragments like ...F0, it seems. When I experiment sometimes the children of the intersected query are fields like id, someFieldName, etc. That means when you wrap all those tracked children in a Node fragment, those fields become invalid :cry:

I guess that means I've caught up to @josephsavona's train of thought - the trouble is figuring out a valid fragment type to wrap each tracked field in...

[josephsavona]

the trouble is figuring out a valid fragment type to wrap each tracked field in...

Yeah, it isn't immediately clear what the best solution is. A good start would probably be to have a failing unit test that's well documented so we have a concrete case to discuss.

[NevilleS]

You can see https://github.com/NevilleS/relay/blob/add-indirection-to-mutation-queries/src/mutation/__tests__/RelayMutationQuery-test.js#L214 as a start. This covers the case where the tracked fields are fragments, so the single wrapper around all of them works...

[steveluscher]

This thread is tl;dr, but in case it's related: #922.

Regardless, if you want to future-proof your code, change your fat query to:

fragment on UpdatePolicyPayload @relay(pattern: true) {
  claim {
    policy
    updatedOn
    updatedBy
  }
}

[NevilleS]

Heh, yeah, it's gotten pretty long.

As I understand it, you're saying that all fat queries should use the pattern directive since they are (technically) invalid GraphQL queries if they use unterminated fields like the claim { policy }, right?

For this particular issue, the problem is that there may be tracked queries on the claim node that are invalid when expanded within the claim { ... } fragment, but your point about future proofing is still valid :smile:

[josephsavona]

Yeah, this is a separate issue from the pattern thing.

[theorygeek]

Just curious if there's a solution already in progress for this?

It sounds like the difficult part of the fix is determining the correct type to use on the fragment. What would be the consequences of adding the __typename field to queries for types that implement Node, similar to how Relay adds the id field now? Relay could then use that information when it's doing the tracked query intersection. It seems like wrapping field access in a fragment against the concrete type is guaranteed to always be valid.

[NevilleS]

Simply wrapping each intersected fragment in one for it's own type isn't quite enough, since that fragment expansion would still be invalid in many cases (namely, the ones identified here). What would work is a "double wrap" of the fragment type, then a generic fragment on Node wrapper; I think that works in all cases, but there could be corner cases as well where this results in an invalid query (which would break existing applications).

Generally speaking __typename is already added to most queries, I think. It doesn't necessarily help since queries are tracked regardless if a response is received; for example, you could do a fragment on SomeSpecificType { __typename, fieldA } which wouldn't return anything; this query still needs to be tracked though, so that future refetches send this query in case a matching node occurs later.

I believe what needs to happen is some way to carry the "type" information through from the babelRelayPlugin so that it is available in the Relay AST when constructing the mutation query, but I definitely wanted to defer to the Relay team's judgment on that...

[josephsavona]

I believe what needs to happen is some way to carry the "type" information through from the babelRelayPlugin so that it is available in the Relay AST when constructing the mutation query,

@NevilleS babel-relay-plugin records the type information for every node, along with whether the type is abstract or concrete - these can be accessed with getType(): string and isAbstract(): boolean on fields/fragments/queries. Can you put up a PR for your commit that you referenced above? That way we can import it easily and experiment.

cc @wincent this was the issue i mentioned the other day

[NevilleS]

If a PR helps, then a PR you shall have...

[josephsavona]

@NevilleS sweet, thanks!

[NevilleS]

Ran into this again in our codebase, this time on a RANGE_ADD constructing the mutation query for an edge... hmm.

[zuker]

@NevilleS I'm facing the same issue with RANGE_ADD right now

[NevilleS]

I take it you're using GraphQL interfaces too? It seems likely that people will run into this.

[zuker]

Yep, graphql-relay-js. Should I get rid of it?

[NevilleS]

?

No, I mean you probably have interfaces in your GraphQL schema, like type Banana implements Fruit, etc. Then you have things like FruitConnection, which you try to add a Banana too, and then something explodes like Fragment "F3" cannot be spread here as objects of type "Banana" can never be of type "Apple".

Right?

[zuker]

Yes, it explodes with error exactly like this. I've tried some workarounds from this thread (not sure if I've got them right though :) but no luck.

[NevilleS]

The workaround is to manually add the indirection layers to your existing fragment definitions. So, following my fruit analogy, go find the places that look something like this:

fragment on Banana {
  isPeeled
}

...and add the indirection layers:

fragment on Banana {
  ...on Node {
    ...on Banana {
      isPeeled
    }
  }
}

[zuker]

@NevilleS Thank you for your clarification! Mutation succeeds now.

[zuker]

Hi again!
I have new issue with fragments intersection. Hope someone will help me to resolve it.

Input

Schema: https://gist.github.com/zuker/cb5a7385d4c8c51fdabb3b841a14ffcf#file-schema-graphqls
Component: https://gist.github.com/zuker/cb5a7385d4c8c51fdabb3b841a14ffcf#file-accountpassworditemlist-jsx (all comoponents futher are just React stateless components)
Mutation: https://gist.github.com/zuker/cb5a7385d4c8c51fdabb3b841a14ffcf#file-introducepassworditemmutation-js
Routes: https://gist.github.com/zuker/cb5a7385d4c8c51fdabb3b841a14ffcf#file-routes-js
Queries: https://gist.github.com/zuker/cb5a7385d4c8c51fdabb3b841a14ffcf#file-queries-js

Cases

1 When component's fragment has:

password_items(first: 20) {
    edges {
        node {
            id
            title
            pass
        }
    }
}

this.props.account.password_items.edges[].node has all props populated. But mutation fails with

[ [Error: Cannot query field "title" on type "Account".],
  [Error: Cannot query field "pass" on type "Account".] ]

2 When component's fragment has:

... on Node {
    ... on Account {
        password_items(first: 20) {
            edges {
                node {
                    id
                    pass
                    title
                }
            }
        }
    }
}

The same as above (PasswordItem and Account are merged)

3 When component's fragment has:

password_items(first: 20) {
    edges {
        node {
            ... on Node {
                ... on PasswordItem {
                    id
                }
            }
            ... on Node {
                ... on PasswordItem {
                    title
                }    
            }
            ... on Node {
                ... on PasswordItem {
                    pass
                }    
            }
        }
    }
}

this.props.account.password_items.edges[].node has only {__dataID__: "1"} and mutation runs successfully.

4 When component's fragment has:

... on Node {
    ... on Account {
        password_items(first: 20) {
            ....
        }
    }
}

this.props.account has only {__dataID__: "1"} and mutation run fails with
invariant.js:38 Uncaught Invariant Violation: writeRelayUpdatePayload(): Cannot insert edge without a configuredparentIDor anewPasswordItemEdge.source.idfield.
also following warning is displayed:
warning.js:44 Warning: RelayMutation: Expected propaccountsupplied toIntroducePasswordItemMutationto be data fetched by Relay. This is likely an error unless you are purposely passing in mock data that conforms to the shape of this mutation's fragment.

Thanks in advance!

[NevilleS]

Hi @zuker. This looks unrelated to this issue so it'd be better to ask this on StackOverflow or a separate issue. I think you'll find several examples of the Expected prop ... to be data fetched by Relay elsewhere. I'd prefer to keep this issue discussion related to the problem of invalid fragment expansions in mutation queries, thanks 👍

[zuker]

@NevilleS I think my problem is related to invalid fragment expansions since in case 1 and 2 Account and PasswordItem got probably merged (fields tilte and pass are PasswordItem's fields) and all entire issue is a result of workaround you provided (anyway thanks again for it :).
UPD: basically if you could advice me how to wrap the password_items connection properly with ... on Node ... on Type it would be great. Thanks!

[zuker]

It seems I've figured why I'm getting

[ [Error: Cannot query field "title" on type "Account".],
  [Error: Cannot query field "pass" on type "Account".] ]

Fields of Account should be explicitly stated getFatQuery(), not just IntroducePasswordItemPayload { account } but IntroducePasswordItemPayload { account { id } }

Thanks!

[zuker]

Faced with Fragment "F..." cannot be spread here as objects of type "xxx" can never be of type "yyy" after some refactoring. Probably I've missed something while refactoring, but issue reproduce scenario got strange now and I think it is worth describing here.
The "spread" error happens only on first mutation request after full app restart (frontend and backend). After first failed mutation request (with the "spread" error) all other requests are successful until server restart AND page refresh. Mutation continues to run without an error after server restart or page refresh, but when page refresh and page reload happens afterwards it fails for the first time and then everything is ok. There are no any other mutations in my app now and before the mutation only one or two queries happen, also server rendering is used so maybe it also has its effect.
Thanks!

[theorygeek]

Just curious if there's been any progress on this issue? It's been a while since there was any activity on it.

[josephsavona]

@theorygeek We haven't been able to make progress on a fix for this yet. For now, the workaround is documented here: https://github.com/facebook/relay/issues/782#issuecomment-213110469.

[mattecapu]

I'm working with union types and the "generic indirection workaround" doesn't work for them.
What will work is instead to wrap the tracked fragments in a fragment of the union type.

Example:
This is my query which returns the union type Foo which is the union of Bar and Quux

foo(arg: $arg) {
  ... on Bar { bar }
  ... on Quux { quux }
}

Now if I mutate something of type Bar, the Quux fragment gets added as well

fragment on MutationPayload {
  my_bar {
    ... on Bar { bar }
    ... on Quux { quux }        # error
  }
}

But if everything is wrapped in the union type of the query from which the fragments are taken, it should work fine

fragment on MutationPayload {
  my_bar {
    ... on Foo {
      ... on Bar { bar }
      ... on Quux { quux }      # useless, but valid
    }
  }
}

Anyway, at this point, it comes natural to me to ask why Quux fragment is tracked for Bar in the first place.

[mattecapu]

I made a PR that (maybe) fixes the behaviour I addressed in the last comment

[NevilleS]

Just to be clear, the Quux fragment is tracked because you defined that
fragment for that node - Relay doesn't attempt to "prune" fragments at all
based on the network response.

On Sun, Aug 7, 2016, 7:24 AM Matteo Capucci [email protected]
wrote:

I made a PR that (maybe) fixes the behaviour I addressed in the last
comment

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/relay/issues/782#issuecomment-238077063, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABv9N9b6E7YLmcQE9jFEc03shlirXv38ks5qdcCBgaJpZM4HQjrP
.

[mattecapu]

Oh yeah, now I get it. It could be a subtype or something like that, right?
Anyway the PR doesn't break this. It just makes sure fragments are placed in the right context.

[tomconroy]

I ran into this issue but the workaround above did not help https://github.com/facebook/relay/issues/782#issuecomment-213110469

I have a generic component with a fragment like this (Authorable and Displayable are interfaces):

fragment on Node {
  __typename
  id
  ...on Displayable { displayName }
  ...on Authorable { authors }
}

This component is used by many types, including Author which is not Authorable. When mutating an Author node which is being tracked for this fragment, I get the server error GQL Error: Fragment "F2" cannot be spread here as objects of type "Author" can never be of type "Authorable".

The fix mentioned in https://github.com/facebook/relay/issues/782#issuecomment-182179360 did work for my case.

  type UpdateAuthorPayload {
-   author: Author
+   author: Node
    clientMutationId: ID
  }

[NevilleS]

Hmm, that's an interesting workaround @tomconroy... it requires schema changes of course, but the alternative (manually wrapping all your fragments in that "indirection sandwich" I proposed) is a pretty hideous solution.

We're trying to decide on our preferred solution for this issue and I see four options:
1) Add the "indirection sandwich" to the fragments that might lead to invalid expansion
2) Modify your schema to only return Nodes in mutations to avoid the invalid expansion
3) Use Relay.GraphQLMutation to manually specify the mutation query so it's guaranteed to be valid
4) Change schema to not use interfaces so that invalid expansions are (probably?) not possible

Right now options 1 and 4 are kinda off the table for me - the former is too clumsy and the latter way too extreme.

Option 3 is looking attractive... I haven't used Relay.GraphQLMutation much, but I think it might be the right way forward given that Relay 2 is going to ditch a lot of the declarative API. It looks like the Relay team did build out the basic features I'd want in this low-level API: optimistic updates, support for RANGE_ADD configs, etc.

Option 2 (from @tomconroy) requires schema changes and sacrifices type safety on the queries, which I don't like very much... but it could potentially work.

@josephsavona, since we've both had some time to subconsciously think about this one, what do you think are the pro/cons of these options for a Relay app today? I'm leaning towards rewriting most of my problematic mutations using the low-level API, but don't want to overlook things.

[josephsavona]

The best workaround to this category of mutation issues is to use RelayGraphQLMutation. This is a lower-level API than RelayMutation, and the mutation query is static (you have to specify all the fields to retch explicitly). Because the mutation queries are static and not generated, it avoids the issues here where the tracked query & fat query intersection can sometimes produce an invalid query.

To access this module you'll have to explicitly require it via the path, as it isn't available on the public Relay instance.

[NevilleS]

Yeah, I tend to agree. In the source comments it mentions not supporting optimistic updates or rollbacks, but it _does_ look like those features are supported...? With optimistic updates I think the low-level API is definitely a good option.

[josephsavona]

The comments may be out of date, so definitely go by the API ;-)

[NevilleS]

Thought so. Thanks for chiming in! 👍

[sibelius]

@josephsavona do u have an example of usage of RelayGraphQLMutation?

[josephsavona]

@sibelius Not yet, the best bet is to look at the unit tests.

[NevilleS]

We've been using the low-level API a bit today, it's working pretty well! Thanks for implementing RANGE_ADD in there @josephsavona, it's definitely needed when mutating connections...

[mrogelja]

Hey ! If there any news on this issue? Do you think that a fix is possible on Relay 1 before the release of Relay 2 ?

I'm just wondering how to implement the @tomconroy workaround with graphql-relay-js. I mean, is it possible to set a mutation output field type as generic "Node" ?

[mattecapu]

I made a PR (#1322) which patches this but it's been sitting here for three months now...

[josephsavona]

If you're running into this issue we recommend checking out the static mutation API (RelayGraphQLMutation, has to be required by path). As I commented above, the unit tests give an overview of the API.

[gre]

It's hard for me to describe the queries, but I'm experimenting a similar bug. What I can see is a specific query Q generated by Relay is making one of my mutation M to break.
both Q and M works taken separately, but as soon as I trigger Q before M, then M will break (Relay generate a wrong fragment). So it's a bit unpredictable.

[josephsavona]

Thanks for filing this issue and your patience as we address it. The recommended workaround to this issue is to use Relay.GraphQLMutation for mutations, which gives you complete control over the mutation query sent to the server.