Skip to content
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

Closed
hinok opened this issue Nov 7, 2017 · 62 comments

Comments

@hinok
Copy link

hinok commented Nov 7, 2017

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 the query 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 defined id, 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

Network error: Error writing result to store for query 

query PokemonQuery($id: ID) { 
  Pokemon(id: $id) { 
    name 
    url 
    createdAt 
  } 
}

Store error: the application attempted to write an object with no provided 
id but the store already contains an id of Pokemon:ciwnmyvxn94uo0161477dicbm 
for this object.

How to reproduce the issue:
https://codesandbox.io/s/mom8ozyy9

Check checkbox in provided demo.

Version

  • apollo-client@<1.6.0>
@jbaxleyiii
Copy link
Contributor

Hey @hinok 👋

This error only should occur when you have made a query with an id field for a portion, then made another that returns what would be the same object, but is missing the id field.

We are planning on making this just a warning (instead of throwing) but it does help a lot to keep the id field included in queries that share fields

@philcockfield
Copy link

Are you guys planning on converting this to a warning rather than error as @jbaxleyiii mentioned. We're getting this too - fine to bring back id's but a warning would be preferable.

@SanCoder-Q
Copy link

SanCoder-Q commented Dec 29, 2017

This id issue caused some of our production pages failure too. I expect Apollo could do more than throw an error and break the whole page. An explicit configuration that indicates using a field like id as the key to share the resources would be good.

@cheapsteak
Copy link
Member

cheapsteak commented May 3, 2018

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)

@johncade
Copy link

johncade commented May 4, 2018

👍 this is a blocking error for me. Please fix

@matrunchyk
Copy link

matrunchyk commented May 5, 2018

We can't use it in our projects with having this error.

@stefanoTron
Copy link

any update on this issue @jbaxleyiii ?

@pencilcheck
Copy link

It seems like missing __typename on any level along side id field will also trigger this error

@deividmarques
Copy link

I have the same problem here, no solution so far?

@divjakLab
Copy link

Have a same problem :) Would like to see some solution...

@razvangherghina
Copy link

+1

3 similar comments
@eugenegodun
Copy link

+1

@bjenkins24
Copy link

+1

@georgetroughton
Copy link

+1

@mike-marcacci
Copy link

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 apollo-codegen. The real issue here is that the scenarios that induce this error can be complex and missed by unit tests and quick human checks, but exist in real world usage.

@cmcaboy
Copy link

cmcaboy commented May 28, 2018

+1

2 similar comments
@ChinmoyDn
Copy link

+1

@rcreasi
Copy link

rcreasi commented May 31, 2018

+1

@billhanyu
Copy link

+1

1 similar comment
@ramusus
Copy link

ramusus commented Jun 12, 2018

+1

@wuzhuzhu
Copy link

wuzhuzhu commented Jun 13, 2018

+1 on updateManySomething

@ColorBuffer
Copy link

ColorBuffer commented Jul 3, 2018

You may forgot to select id in your query.

                post(id: $postID) {
                    id
                    comments(pagination: $pagination) {
                        edges {
                            node {
                                content
                            }
                            cursor
                        }
                        pageInfo {
                            endCursor
                        }
                    }
                }
            }```

@bennypowers
Copy link
Contributor

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 {accessToken} instead of {accessToken, id, ...rest}

@bennypowers
Copy link
Contributor

I've found a strange bug related to this issue.

I have a custom element <blink-feed> which subscribes to this query like this one when it connects to the DOM:

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 User type is queried both client-side and server-side.

With the query as shown above, the components render without error, however, the client-side User object is overwritten with its default values once the network query resolves.

If I omit the id from either the user or owner fields, the component errors on the first load, but then renders correctly on subsequent reloads (via apollo-cache-persist)

This same behaviour occurs if I alias the client-side user type, e.g., calling it Session instead of User

@wfsovereign
Copy link

Find this stack overflow can resolve my problem.

@emahuni
Copy link

emahuni commented Sep 3, 2019

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:
make sure to add id to all subselections of your queries
this makes sure the cache identifies queries and selections properly... notice this has nothing to do with following a certain pattern whether you put ids somewhere else or not, it's just that simple; put ids for sub-selections and to add to that especially for ones that recurse!

Aichnerc added a commit to pharmaziegasse/charm-webapp that referenced this issue Nov 6, 2019
The graphQL error has been fixed.
Ref: apollographql/apollo-client#2510
@kidroca
Copy link

kidroca commented Nov 7, 2019

I have a similar issue when an alias is used for the ID field:

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 MyProviderAccount query after GetService I would get the error:

Store error: the application attempted to write an object with no provided id but the store already contains an id of ProviderType:501 for this object

I wasn't expecting that when I use an alias for a field, I have to use the same alias everywhere so that cache would work


Edit: It's related to the id key not the alias. Using different aliases would still resolve data from cache if it's available:

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

@japrogramer
Copy link

@kidroca why not use a fragment with an alias .. that way now you only need to use the fragment everywhere.

@kidroca
Copy link

kidroca commented Nov 8, 2019

I am using a fragment where it makes sense. For example the MyProviderAccount -> account. But there are cases where I just want a few fields like in self -> I use the id to highlight the current user account in the providers list


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 id . It has a default logic to assume that if there is object.id then id is the ID

  1. The cache would map and normalize based on the received data
  2. It can't tell which key is the ID, just by the data itself (it doesn't know the schema - doesn't have to)
  3. It is smart enough to distinct whether and where you've used an alias though

For this reasons I've decided that you should either always alias all ids in a generic way (id: provider_id, id: service_id) or never alias ids, which seems the easier option

When the ID key is different across your Types (provider_id, service_id), it's best to provide a dataIdFromObject(object) function which can map the id => getIdFor(object.__typename) like the example provided in the docs. And you can still fallback to the default dataFromObject function in case an alias like id: provider_id was used

Also have in mind that if you're dealing with numeric ids - they probably aren't unique across types so the returned data for dataIdFromObject should be something like ${object.__typename}[${id}] (e.g. "Service[10]") and not just the id

@rmolinamir
Copy link

rmolinamir commented Nov 11, 2019

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 id fields to everything that was related to subscriptions.

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 id fields from the queries also works in the same Hacker News example, as @capaj says, the important thing is to match the query structures.

@Vadorequest
Copy link

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).
And it should be made obvious in the docs that all query must have an "id" field for Apollo to work properly.

Also, it's very hard to debug. Especially because the error message isn't always Store error: the application attempted to write an object with no provided id but the store already contains an id of but most of the time only Network error: Error writing result to store for query with the whole query behind... Quite complicated to understand what's the issue at hand with such cryptic error messages.

I eventually added id everywhere, in all fragments and queries to avoid this, and it works fine now.

@balazsorban44
Copy link

Totally agree with @Vadorequest! Came here to say something similar

@cecchi
Copy link

cecchi commented Apr 24, 2020

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.

Screen Shot 2020-04-24 at 8 26 10 AM

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)

@capaj
Copy link
Contributor

capaj commented Apr 24, 2020

@cecchi wow, this is amazing work! Someone should take that code and turn it into a code into a graphql-codegen plugin

@cecchi
Copy link

cecchi commented Apr 24, 2020

A plugin sounds like a good idea. We use graphql-codegen too, but we run them in series: validate followed by codegen. Since graphql-codegen uses @graphql-toolkit under the hood we're able to load the schema and documents once for both steps, using graphql-codegen's programmatic API.

If there's enough interest I can publish the validate fn as a standalone package.

@cheapsteak
Copy link
Member

Looks like this is addressed in Apollo Client 3, specifically this PR from Ben to remove that error
#5904

Here's the repro using "@apollo/client": "3.0.0-beta.44", no longer erroring
https://codesandbox.io/s/github/cheapsteak/test-ac-cache-store-bug/tree/master/?file=/src/index.js

@stephenh
Copy link

@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

@bennypowers
Copy link
Contributor

@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?

npm ERR! code E404
npm ERR! 404 Not Found - GET https://registry.npmjs.org/@homebound%2fgraphql-id-linter - Not found
npm ERR! 404
npm ERR! 404 '@homebound/graphql-id-linter@latest' is not in the npm registry.
npm ERR! 404 You should bug the author to publish it (or use the name yourself!)

@birhanuh
Copy link

@jbaxleyiii Thank you! You saved my day!

@stephenh
Copy link

@bennypowers shoot, I missed your comment, but I'd forgotten to make the `@homebound/graphql-id-linker~ npm module public; it should work now.

@jmtimko5
Copy link

jmtimko5 commented Dec 2, 2020

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?

@trackwell-mike
Copy link

Just confirming that we are using "@apollo/client": "^3.1.3" and are still seeing this "error" (logged as a WARN).

[Error: Error writing result to store for query: { ... } 
Store error: the application attempted to write an object with no provided id but the store already contains an id of User: ### for this object. The selectionSet that was trying to be written is: { ... }

@anarchist912
Copy link

Updated to @apollo/client v3.4.5, still had the issue. Would break my entire React component. Resolved it by making sure that nested data in the schema has the ID field (whatever the field is named) queried as well, e.g.

query {
  listOrder {
    _id
    customer {
       _id  <= YES, THIS IS NEEDED.
       name
    }
  }
}

@lsanwick
Copy link

I think the biggest that bothers me with this error is that it doesn't seem to fire the onError error link when it occurs, unlike traditional GraphQL errors.

@jamiemetca
Copy link

Find this stack overflow can resolve my problem.

Thanks @wfsovereign This solved my issue. Nested data also requires id.
From this;

  query CheckExists($input: Input) {
    checkExists(input: $input) {
      id
      name
      address {
        postcode
      }
      bank {
        accountNumber
      }
    }
  }

To this;

  query CheckExists($input: Input) {
    checkExists(input: $input) {
      id
      name
      address {
        id
        postcode
      }
      bank {
        id
        accountNumber
      }
    }
  }

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests