Require id field for all GraphQL entities in all queries and mutations
The problem
Imagine this scenario:
- Feature A is implemented on a page, and uses a GraphQL query that fetches an entity, but does not request its
id
. - Later, feature B on the same page adds/modifies an unrelated query so that it fetches the same entity, but does request its
id
. - Feature A's query now fails, with something like:
Store error: the application attempted to write an object with no provided id but the store already contains an id of SomeEntity:gid://gitlab/SomeEntity/123 for this object.
This scenario is exactly what lead to #326071 (closed). In that case, feature A is the security reports MR widget, where this is the query that failed, and feature B is the general MR widget, with this change that caused it, and finally the one-line fix.
What's concerning is that this is so easy to miss, because these two queries are completely unrelated. It's easy to imagine this sort of thing happening again: a query is written in a page that does not fetch the id
of an entity, then later, new unrelated query fetches the same entity but with the id
.
How can we prevent this in future?
Straw man proposal 1
There may be other ways to prevent this from happening in future, but for the sake of proposing something that can be discussed, I suggest we lint/enforce that all GraphQL entity queries include the id
of the entity, to prevent failed Apollo cache writes.
Cons
- This may not be be safe, since apparently some entities can replace existing ones, re-using the same
id
(e.g., Designs: see https://gitlab.slack.com/archives/C0GQHHPGW/p1616757169165200?thread_ts=1616755540.159300&cid=C0GQHHPGW), as described by @ntepluhina:for Designs we needed to ‘construct’ the unique identifier with the link to the file because you can upload the image with the same name, it will replace an old one and will have the same ID
- It seems redundant/verbose to always specify a field that isn't explicitly used.