-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Store error: the application attempted to write an object with no provided id but the store already contains an id of... #2510
Comments
Hey @hinok 👋 This error only should occur when you have made a query with an We are planning on making this just a warning (instead of throwing) but it does help a lot to keep the |
Are you guys planning on converting this to a |
This |
Should this issue be re-opened, and only closed after it's made to be just a warning? Currently, if you make a change to one query by adding an ID field, you could be breaking other parts of your app that you haven't touched at all (because they use queries without ids and you used a query with an id) |
👍 this is a blocking error for me. Please fix |
We can't use it in our projects with having this error. |
any update on this issue @jbaxleyiii ? |
It seems like missing __typename on any level along side id field will also trigger this error |
I have the same problem here, no solution so far? |
Have a same problem :) Would like to see some solution... |
+1 |
3 similar comments
+1 |
+1 |
+1 |
For what it’s worth, I don’t mind the current behavior, as long as it can be caught compile-time using a tool like |
+1 |
2 similar comments
+1 |
+1 |
+1 |
1 similar comment
+1 |
+1 on updateManySomething |
You may forgot to select
|
PSA: This can also happen if you try to write an object without an ID, as happened to me when I created an unauthenticated user by mutating with |
I've found a strange bug related to this issue. I have a custom element query BlinkFeed (...) {
orientation @client
# user :: User
user @client { id locale avatar }
blinks( ... ) @connection(key: "feed") {
pageInfo { hasNextPage hasPreviousPage }
edges {
cursor
node {
id
...
# owner :: User
owner {
id
avatar
nickname
iFollow
}
}
}
}
} Note that the With the query as shown above, the components render without error, however, the client-side If I omit the id from either the This same behaviour occurs if I alias the client-side user type, e.g., calling it |
Find this stack overflow can resolve my problem. |
The Stackoverflow answer pointed to in this issue is actually the sensible one and actually solves this issue, which is not a bug by the way. I think this needs to be documented somewhere. In a nutshell: |
The graphQL error has been fixed. Ref: apollographql/apollo-client#2510
I have a similar issue when an alias is used for the Example:query GetService ($id: ID!) {
service(id: $id) {
image
# list of account
providers {
id: provider_id
firstName: first_name
lastName: last_name
}
}
self: account {
# if I don't use an alias here it worked even when MyProviderAccount didn't include the `provider_id`
id: provider_id
}
}
query MyProviderAccount {
account {
provider_id # using the same alias here would fix the problem
first_name
last_name
}
} When I try to run the
Edit: It's related to the query QueryA {
account {
provider_id
firstName: first_name
}
}
query QueryB {
account {
provider_id
name: first_name
}
} Running the above queries in sequence - first A then B (or reverse) would not make a 2nd request, but resolve the 2nd query from cache |
@kidroca why not use a fragment with an alias .. that way now you only need to use the fragment everywhere. |
I am using a fragment where it makes sense. For example the I've read more about cache and looks like this is the correct behavior, because the cache doesn't exactly know which key is the
For this reasons I've decided that you should either always alias all ids in a generic way ( When the ID key is different across your Types (provider_id, service_id), it's best to provide a Also have in mind that if you're dealing with numeric ids - they probably aren't unique across types so the returned data for |
I wanted to chime in and report that this issue happens in the official example of the Hacker News clone: https://www.howtographql.com/basics/0-introduction/ The solution was to indeed modify the queries and add It does make sense that it works as intended because it's essentially replacing data in the cache by their identifier keys, but the error message is lacking IMO. Edit: Removing the |
I ran into this today, I added a new subquery somewhere and it broke my whole app because it's a Next.js app which uses both SSR and CSR and depending on the cached data it would randomly crash. For instance, refreshing a page with cmd+shift+r to erase cache would work fine (SSR) but then when navigating to another page (CSR) it would randomly crash THE WHOLE THING, depending on which page is loaded. This definitely SHOULD NOT throw an exception, but rather a warning (as proposed in 2017). Also, it's very hard to debug. Especially because the error message isn't always I eventually added |
Totally agree with @Vadorequest! Came here to say something similar |
This eventually became enough of a pain in the neck that we wrote a validation rule/linter to ensure the "id" field was present in every possible GraphQL selection set. This rule is intended to be used with the GraphQL validation APIs. Our implementation is wrapped up in some other tooling but these are the relevant bits: function IdOnObjectSelectionSetRule(context: ValidationContext): ASTVisitor {
/**
* Given a set of SelectionNodes from a SelectionSet, ensure "id" is included.
*/
function selectionSetIncludesId(selectionNodes: readonly SelectionNode[]) {
return selectionNodes.some(selectionNode => {
switch (selectionNode.kind) {
case 'Field':
return selectionNode.name.value === 'id'
// Fragments have their own selection sets. If they do not include an "id",
// we will catch it when we visit those selection sets.
case 'FragmentSpread':
case 'InlineFragment':
return true
}
})
}
/**
* Walk the AST and inspect every SelectionSetNode. Every SelectionSet must include an "id"
* field, if one exists. It can include it directly, or as part of a fragment.
*
* See: https://graphql.org/graphql-js/language/#visit
*/
return {
SelectionSet(node: SelectionSetNode) {
let type = context.getType()
// Unwrap NonNull types
if (isNonNullType(type)) {
type = type.ofType
}
// Ensure this is an object type (meaning it has fields)
if (!isObjectType(type)) {
return undefined
}
const possibleFieldsIncludesId = 'id' in type.getFields()
if (possibleFieldsIncludesId && !selectionSetIncludesId(node.selections)) {
context.reportError(
new GraphQLError(
`Missing \"id\" field in \"${type.name}\"`,
node,
undefined,
undefined,
// N.B. In our implementation we pass in the source location as well for more
// useful errors.
location ? [location] : undefined
)
)
}
return undefined
}
}
} Usage is something like this: import { loadSchema, loadDocuments } from '@graphql-toolkit/core'
function validate(schemaAst: GraphQLSchema, sources: Source[]) {
let errors: GraphQLError[] = []
sources.forEach(source => {
if (source.document) {
errors = errors.concat(
// If you want pretty errors, also make "source.location" available to the IdOnObjectSelectionSetRule
validateGraphQL(schemaAst, source.document, [IdOnObjectSelectionSetRule])
)
}
})
return errors
}
// Load the schema and documents
const [schemaAst, documents] = await Promise.all([
loadSchema(/*... your schema ...*/, {
loaders: [new UrlLoader()]
}),
loadDocuments(/*... your documents ...*/, {
loaders: [new GraphQLFileLoader()],
skipGraphQLImport: true
})
])
const errors = validate(schemaAst, documents) |
@cecchi wow, this is amazing work! Someone should take that code and turn it into a code into a graphql-codegen plugin |
A plugin sounds like a good idea. We use If there's enough interest I can publish the validate fn as a standalone package. |
Looks like this is addressed in Apollo Client 3, specifically this PR from Ben to remove that error Here's the repro using |
@cecchi thanks for that visitor/linter! I dropped it into a graphql-code-generator plugin: https://github.com/homebound-team/graphql-id-linter cc @capaj |
Looks like this wasn't published yet?
|
@jbaxleyiii Thank you! You saved my day! |
@bennypowers shoot, I missed your comment, but I'd forgotten to make the `@homebound/graphql-id-linker~ npm module public; it should work now. |
This feels like a very important gotcha in using Apollo GQL that should be documented explicitly and obviously, and be added to standard Apollo linters. Can we add this to the docs? |
Just confirming that we are using
|
Updated to query {
listOrder {
_id
customer {
_id <= YES, THIS IS NEEDED.
name
}
}
} |
I think the biggest that bothers me with this error is that it doesn't seem to fire the |
Thanks @wfsovereign This solved my issue. Nested data also requires id.
To this;
|
Hey,
Today in one of my applications I found this issue. I'm not sure if this is a bug or desired behaviour because I couldn't find any information about it in the documentation.
The fix is quite easy, just add
id
to thequery
without id.So the question is: should every query has defined
id
for a resource?It seems to work without
id
but when there will be the next query with definedid
, this error appears.I'm asking because I have a few other cases where external API doesn't have
id
so this problem can appear again in the future.Intended outcome:
Error shouldn't appear and application should render data.
Actual outcome:
Error appears
How to reproduce the issue:
https://codesandbox.io/s/mom8ozyy9
Check
checkbox
in provided demo.Version
The text was updated successfully, but these errors were encountered: