Closed danielgriese closed 7 years ago
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:
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!
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!
...
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? ;-)
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?
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
...
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...
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.
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 notifications@github.com 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.
Unfortunately, lookups still require the type information to be on the client :-/
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:
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.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
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.
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.
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
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 Claim
s. 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.
It looks like the fix here is to wrap all mutation fragments in ... on Node { ... on FieldType { ${originalFragment} } }
- @NevilleS still interested in that PR? :-)
Yeah, I can do it... it's so gross though! LOL
Are we sure the ... on Node
is always valid? Intrinsically I believe that to be true, since fragment tracking only happens on nodes, but...
... 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 id
s.
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:
@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.
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 notifications@github.com 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.
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 { ... {
}
}
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:
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...
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.
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...
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
}
}
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:
Yeah, this is a separate issue from the pattern
thing.
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.
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...
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
If a PR helps, then a PR you shall have...
@NevilleS sweet, thanks!
Ran into this again in our codebase, this time on a RANGE_ADD constructing the mutation query for an edge... hmm.
@NevilleS I'm facing the same issue with RANGE_ADD right now
I take it you're using GraphQL interfaces too? It seems likely that people will run into this.
Yep, graphql-relay-js
. Should I get rid of it?
?
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?
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.
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
}
}
}
@NevilleS Thank you for your clarification! Mutation succeeds now.
Hi again! I have new issue with fragments intersection. Hope someone will help me to resolve it.
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
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 configured
parentIDor a
newPasswordItemEdge.source.idfield.
also following warning is displayed:
warning.js:44 Warning: RelayMutation: Expected prop
accountsupplied to
IntroducePasswordItemMutationto 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!
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 👍
@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!
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!
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!
Just curious if there's been any progress on this issue? It's been a while since there was any activity on it.
@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.
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.
The mutation is described as this:
The client side mutation code:
The displaying container has the following (shortened) fragment:
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.
As I need to render the target differently depending on the type, I need to query for different data for different objects.
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:
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.
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.
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