apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.39k stars 2.66k forks source link

Best practice when a mutation deletes an object #899

Closed yurigenin closed 6 years ago

yurigenin commented 8 years ago

What is the best practice for deleting items from the apollo store when a mutation deletes them on the server? I could not find anything related to this in the docs. Do I just have to return ids of deleted items as a result of mutation and then use reducer/updatequeris to manually delete items from the store? Or there is some way to let apollo client handle it automatically? Any help is appreciated.

richard-uk1 commented 8 years ago

Related StackOverflow question: http://stackoverflow.com/questions/40484900/how-do-i-handle-deletes-in-react-apollo/40505977

helfer commented 8 years ago

@yurigenin Right now updateQueries or result reducers are the recommended way to remove items from the store. We're open to re-implementing better ways of doing this, so if you have an idea, please share it here!

yurigenin commented 8 years ago

@helfer @stubailo How do I actually purge the objects from the store? If I use reducers/updateQueries it only removes objects from the lists that contain them. But objects themselves stay in the store and become pretty much orphaned.

richard-uk1 commented 8 years ago

@yurigenin I think at the moment, you'd need to remove them manually, but I may just have missed that functionality when reading through the code

helfer commented 8 years ago

@yurigenin right now there's no cache expiration. Deleting objects manually can be a bit dangerous, because you need to make sure that no other query is using them.

A few people have already asked for this, so here's a discussion around cache expiration: https://github.com/apollostack/apollo-client/issues/825

jfrolich commented 7 years ago

It might help to have an example of the updateQueries best practice. Anyone here able to share it, it would be great to have in the documentation, because it is part of any CRUD app :). How about the option of creating a function that removes an object based on the data-id? In this way I can just call that function after delete to clear it from the local cache, it might even be called automatically if the mutation returns null or so?

maslianok commented 7 years ago

Yeah, also faced up with this and have no idea how to correctly implement this behavior

jfrolich commented 7 years ago

I think the easiest and most flexible way is to use the reducer function. At least I got it working quite easily using that. Still maybe a bit too much boilerplate for such a common action, but at least it solves it for me.

TSMMark commented 7 years ago

Can someone provide example code of how to use reducer function to remove store data?

HriBB commented 7 years ago

Already posted, but check this link http://stackoverflow.com/questions/40484900/how-do-i-handle-deletes-in-react-apollo?answertab=active#tab-top

I also use react-addons-update https://facebook.github.io/react/docs/update.html

npm install --save react-addons-update

Using the following query

const GET_BRANDS = gql`
  query brands {
    brand_categories {
      id title
      brands {
        id title description active pos
      }
    }
  }`

This is my updateQueries function

import update from 'react-addons-update'

export const DELETE_BRAND = gql`
  mutation deleteBrand($id: Int!) {
    deleteBrand(id: $id)
  }`

graphql(DELETE_BRAND, {
  props: ({ ownProps, mutate }) => ({
    deleteBrand: (id) => mutate({
      variables: { id },
      optimisticResponse: {
        deleteBrand: id,
      },
      updateQueries: {
        brands: (prev, { mutationResult }) => {
          // will find category and brand index from the previous list
          let categoryIndex, brandIndex
          prev.brand_categories.forEach((category, cindex) => {
            const bindex = category.brands.findIndex(b => b.id === id)
            if (bindex > -1) {
              categoryIndex = cindex
              brandIndex = bindex
            }
          })
          // then remove brand from the next list
          return update(prev, {
            brand_categories: {
              [categoryIndex]: {
                brands: { $splice: [[brandIndex, 1]] }
              }
            }
          })
        },
      },
    }),
  }),
}),

Side question: is there an easier way to find indexes?

yurigenin commented 7 years ago

The problem is that this will not delete the brand itself from the store... It only updates the list.

jfrolich commented 7 years ago

Here my version as a reducer. If you use proper ids, you get updates for free, but you need to handle the case of addition and removal from lists.

const withContentItem = graphql(gql`
[YOUR QUERY] 
`, {
  options: props => ({
    variables: { ... },
    reducer: (state, action) => {
      // insert
      if (
        action.type === 'APOLLO_MUTATION_RESULT' /* is it a mutation? */
        && action.operationName === 'mutateElement' /* mutate element? */
        && action.result.data.mutateElement.contentItemId === state.contentItem.id /* operate on right content id? */
        && !state.contentItem.elements.find((el) => el.id === action.result.data.mutateElement.id) /* not already in list */
      ) {
        return update(state, {
          contentItem: {
            elements: {
              $push: [action.result.data.mutateElement] /* add to end of list */
            }
          }
        })
      }
      // delete
      if (
        action.type === 'APOLLO_MUTATION_RESULT'
        && action.operationName === 'deleteElement'
        && action.result.data.deleteElement.contentItemId === state.contentItem.id /* operate on right content id? */
      ) {
        return update(state, {
          contentItem: {
            elements: {
              $splice: [[state.contentItem.elements.findIndex(el => el.id === action.result.data.deleteElement.id), 1]]
            }
          }
        })
      }
      return state
    }
  }),
})
nosovsh commented 7 years ago

We are missing one big use case when frontend can not determine results of what queries and how were changed after deletion (same applies for creating new entities). Let's say I queried users(age=29, limit=20, offset=10, sort='name'). Then I sent mutation to delete some user. And now I can not use reducer or updateQueries because I don't know HOW should I change results of users query. Only server knows how to filter users based on provided variables. The only thing I can do is to invalidate somehow cache for all users queries. But as far as I know there is no way to do it in Apollo. Am I right?

jlovison commented 7 years ago

@nosovsh You can invalidate the entire cache, for everything, on a delete. That's what I ended up going with as a solution, though in my application deletes are a rarity anyways (i.e. perhaps once a week for a user at most).

Yes, it would be really nice if there was a way to invalidate a single item using the object ID the store is using to keep track of it, across all possible queries that would be including it. I tried to suss out exactly how the application is keying the cache for objects, and why all the solutions seem to couple it with the query (it must not be simple key/value, or these suggested workarounds would be total overkill), but after about an hour of digging into the code, I eventually just decided to go with the nuke option.

A clear explanation of the store and cache invalidation strategies would be quite welcome - as someone that isn't particularly familiar with Relay, I feel like most of the examples/discussion/documentation on the Apollo internal cache assumes a strong familiarity with Relay's store, and how it's plugged into Apollo.

For example, it wasn't clear at all to me if I zap a cached item from one query, if it zaps it from other queries that would return it, or if on deletion I need to essentially loop over every possible query that would return it and check if it's in the cache, and if so, zap it for each query. And if zapping it once zaps it everywhere, I'm again confused why I can't just zap it directly using the objectID generated to track it.

nosovsh commented 7 years ago

@jamiter problem with resetStore() is that after you call it all active queries will be immediately refetched. And because Apollo uses recycling of queries a lot of them could be active even if components that were using them already gone. see #462 That can cause enormous network load so I can not use it.

jamiter commented 7 years ago

@nosovsh, I think you mentioned me by accident.

nosovsh commented 7 years ago

oops, I meant @jlovison

renato commented 7 years ago

(Please tell me if my question would be better asked in SO instead.)

I was using updateQueries to delete from the store but it only works for the current parameters in a query and I'm deleting data for other cached parameters (ie. I filter by month and I'd like to delete data from multiple months). Anyone knows the recommended way to do this (without refetching)?

Plus, I thought migrating to the new update API could give me more flexibility on this matter, but I couldn't find any doc about deleting from the store when using update. What is the recommended way to do this? Does it support the case I described above?

Thanks!

helfer commented 7 years ago

@renato with the update function you should be able to write null to the store. This won't actually remove any data, but it will break the links, so to the client it will look like the data isn't there any more and it will refetch.

What's the reason you want to delete from the cache and how important is that feature to you?

renato commented 7 years ago

@helfer I'm sorry, I'm probably missing something.

I don't really need to "delete from the cache" but this is the way that worked so far for me when deleting something from the view.

I've a list of items in my React view injected by Apollo. When I execute a mutation that deletes one item how do I remove that specific item from my view? So far I've been deleting it from the cache using the updateQueries method.

However, the last query I'm working on has a month parameter and I can navigate through months so they become cached by Apollo. I've a mutation that deletes some items that affects multiple months, not only the currently active parameter. The updateQueries method only affects the active parameter so when I navigate to other months after this mutation, the old cache (before the delete operation) is loaded.

If you don't delete from the cache in these situations, what is the solution? Just invalidate the cache or refetch?

dcworldwide commented 7 years ago

What's the reason you want to delete from the cache and how important is that feature to you?

To synchronise my view model (mobx) with my model store (apollo)

chris-guidry commented 7 years ago

Based on the blog post Apollo Client’s new imperative store API, here's how I setup delete mutations:

this.props.FooDelete({
  variables: {
    input: {
      id: this.props.id
    }
  },
  update: (store, { data: { deleteFoo } }) => { 
    const data = store.readQuery({query: fooQuery() }); // the GraphQL query is returned by the function fooQuery
    const index = data.allFoos.edges.findIndex(edge => edge.node.id === this.props.id);
    if (index > -1) {
      data.allFoos.edges.splice(index, 1);
    }
    store.writeQuery({ query: fooQuery(), data });
  }
})
.then( res => {
  console.log('Foo Deleted.');
});
derailed commented 7 years ago

I am confused. Most of the sample code I see on this thread, seems to deal with running a delete mutation from a single query. What if I actually have multiple queries referencing that object? I thought the whole point of Apollo tracking refs ie Type-X was to handle such a use case.

For example say user deletes User-1. User-1 is referenced by multiple queries say queryA and queryB. Based on what I see in this thread, it seems it would be the responsibility of the component update to correctly update the store for each of the referencing queries??

I hope I a missing the point here as this would be a maintenance nightmare! So getting back to the original question: What is the best way to handle this use case ie delete the reference and have apollo remove the reference from ALL queries tracking that instance?

chris-guidry commented 7 years ago

@derailed a few thoughts:

  1. The store is only going to save the user as one item in the cache so even if multiple queries are returning the user, it should only need to be removed once. If you haven't already, try out the Chrome Apollo extension, which will allow you to see how the store is updated post mutation.
  2. If there are items in the store related to the user such as BlogUser, ToDoUser, etc., those would most likely need to be explicitly removed from the store within the "update" section of the mutation.
  3. Generally, mutations are only going to impact a small number of tables, in which case the mutation update of the store makes sense. However, if a particular mutation is going to have wide ranging affects, you could consider resetting the entire store with client.resetStore(). It's documented as part of the login/logout functionality, but it can be used elsewhere.
derailed commented 7 years ago

Hi Chris,

Thank you for the reply! I am at loss here. I have a bunch of queries falling in this use case ie referencing a user object. I am indeed using the extension and it seems to report the correct behavior per the implementation ie the user object is deleted from the query but still shows on the left handside under the store tab aka `Apollo Client State, which leaves me to believe that user is still in the store. So as you can see my update cb currently deletes the user from the current query and not the actual store. Not sure how do actually to this??

As you can see here 3 other queries references that user and need to be updated. I haven't yet found a way to deal with that elegantly besides having to refetch to make sure that user is now gone in related queries. That said my update scenarios work ie on an update mutation the user changes are correctly propagated which leads me to believe the store is actually wired correctly?

My expectations here is during a delete mutation all the queries having the referenced user object should be "automatically" deleted without having to manually update all related queries as the reference is no longer active. Per your #2 point it does not seems to be handled by Apollo that way. Hence to currently get the right effect and short of either reseting or refetching one is left with having to know all the potential queries that might be affected by that deletion. Does this sound right, or am I missing it?

Thank you for you clarifications!!

Here is what I have in my code when a user is deleted:

this.apollo.mutate({ mutation: DeleteUser, variables: deleteInputs(this.user.id), // BOZO!! Lame! refetchQueries: [{ query: Facilities }, { query: Accounts }, { query: OrphanUsers }], optimisticResponse: optimisticDelete(id), update: (store: any, data: any) => { const cache = store.readQuery({ query: PvdmUsers }); store.writeQuery({ query: PvdmUsers, data: { pvdmUsers: _.filter(cache.pvdmUsers, (u: UserFragment): boolean => { return u.id != id }) } }); } }) .toPromise() .then(() => this.back()) .catch(error => this.error = error.message);

On Thu, Jun 15, 2017 at 7:54 AM, Chris Guidry notifications@github.com wrote:

@derailed https://github.com/derailed a few thoughts:

  1. The store is only going to save the user as one item in the cache so even if multiple queries are returning the user, it should only need to be removed once. If you haven't already, try out the Chrome Apollo extension, which will allow you to see how the store is updated post mutation.
  2. If there are items in the store related to the user such as BlogUser, ToDoUser, etc., those would most likely need to be explicitly removed from the store within the "update" section of the mutation.
  3. Generally, mutations are only going to impact a small number of tables, in which case the mutation update of the store makes sense. However, if a particular mutation is going to have wide ranging affects, you could consider resetting the entire store with client.resetStore() http://dev.apollodata.com/react/auth.html#login-logout. It's documented as part of the login/logout functionality, but it can be used elsewhere.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apollographql/apollo-client/issues/899#issuecomment-308735861, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAP3CStYjjN1sbUviGxJ_NQDJP8xbv-ks5sETeZgaJpZM4KwUgp .

chris-guidry commented 7 years ago

@derailed - in the Apollo Chrome Extension, under the Store tab, the User is not removed after a delete mutation? If you post the code for the delete mutation, then I can give feedback on it.

derailed commented 7 years ago

@chris-guidry - Tx Chris!

I believe, the user is correctly removed from the query but not from the store. I think I am not understanding out to actually remove my user from the store. This code works for the users query. However I have 2 other query referencing that user and the given user still show in those.

So I think my confusion here is I expect the delete to be propagated to the other queries since it's a delete mutation, but don't think that's the way things actually work. But perhaps I am totally missing the point here??

Here is the code

   this.apollo.mutate({
      mutation: DeleteUser,
      variables: deleteInputs(this.user.id),
      // BOZO!! Lame!
      refetchQueries: [{ query: Facilities }, { query: Accounts }, { query: OrphanUsers }],
      optimisticResponse: optimisticDelete(id),
      update: (store: any, data: any) => {
        const cache = store.readQuery({ query: PvdmUsers });
        store.writeQuery({
          query: PvdmUsers,
          data: {
            pvdmUsers: _.filter(cache.pvdmUsers, (u: UserFragment): boolean => { return u.id != id })
          }
        });
      }
    })
      .toPromise()
      .then(() => this.back())
      .catch(error => this.error = error.message);
MichaelDeBoey commented 7 years ago

@derailed You're right that the User is still in the store, but isn't given back when asking for the PvdmUsers query. Currently there's no way of really deleting an object from the store I think (hence why this thread is still alive).

derailed commented 7 years ago

@MichaelDeBoey @chris-guidry - Tx Michael! I think you are right.

One thing just dawned on me. I think a delete mutation does not have any special meaning in Apollo, it's just a mutation. Thus it would be the resp of the dev on the update to make sure the deletion is correctly propagated across all queries referencing it. I think this is why I have been struggling with this concept as I thought given the special handling of refs that though regular mutation works nicely, in a delete situation special care must be taken to ensure the store integrity. I hope I am wrong about this, but if not this is going to be very painful...

MichaelDeBoey commented 7 years ago

@derailed No you're right. With a normal mutation (create or update), you get the created/updated Object back as a response, so Apollo knows how to handle it (in most cases). When deleting an object, Apollo doesn't know what to do (yet), so we have to specify it ourself in the update method we provide. If Apollo would automatically delete the object from the store, that could always break several things.

For instance when you have queries ListAllUsers and ListAdminUsers, where User:1 is in both lists and Apollo then deleted User:1 from the store, then de references would be deleted in both queries. But what if we have another query ListBlogPosts, where you have the following:

query {
  ListBlogPosts () {
    id
    title
    user {
      id
      name
    }
  }
}

Apollo can't know what to do (unless we tell it). Does it also delete all the Blogposts?, does it only delete the reference to the deleted user? That's why we have to specify it ourself in the update method.

I know it can be cumbersome if you have a lot of references to the deleted User, but that's (currently) the only way to handle it I think.

Maybe @helfer and/or @stubailo can correct me if I'm wrong?

derailed commented 7 years ago

@MichaelDeBoey Thank you Michael for the explanation! I was hoping there was a better solution here as I am going to need to a) figure out which queries are active during the delete. b) for each active query traverse to see if that user is actually referenced c) delete the ref d) update the store. Lots of manual operation here especially in lite of new features creeping in and introduction new refs to users, that will need to update that delete operation.

Yikes!

MichaelDeBoey commented 7 years ago

@derailed I don't think you have to see if the query is active, you can just do the readQuery I think (I'm not sure tho 😕). The only pain will be that you need to read every query with every possible variables I think, that's why the updateQueries comes in handy, 'cause you can name your queries and just put in all the names there and they will all be re-run with the right variables they were already executed with. Or maybe you can check just check the ROOT_QUERY for all executed queries?

@helfer or @stubailo should confirm that last one, 'cause I'm not sure if that's possible or not

helfer commented 7 years ago

Hi @derailed! @MichaelDeBoey is right, there's currently no easy way of deleting things from the store, but we're thinking of adding that soon. Right now you'll have to delete the reference from all queries that use it. A simpler way is to refetch all the queries that could reference that object, which you could even do as part of the mutation, if you wanted to (in that case it would be called a "fat query").

If we added a special store.deleteObject functionality or something like that, it could work by automatically removing all references or simply marking the object as deleted and then not including it in lists etc. any more. The question is what should happen to queries that now have incomplete data. Most likely the best we could do is return null in that case.

lucfranken commented 7 years ago

This really seems to go to the same point I made an issue for here: https://github.com/apollographql/apollo-client/issues/1697

This seems to be the clue from @MichaelDeBoey

The only pain will be that you need to read every query with every possible variables I think, that's why the updateQueries comes in handy, 'cause you can name your queries and just put in all the names there and they will all be re-run with the right variables they were already executed with.

Is there no way Apollo can "understand" the relation between resources?

derailed commented 7 years ago

@helfer - Thanks for the update Jonas! I think that call will be helpful. Also a call to see who has contention on a given object would be nice to have. Think the store might be headed toward a client side db??

TSMMark commented 7 years ago

A db in redux? Is that a thing?

helfer commented 7 years ago

We're headed in a somewhat different direction that's very close to the imperative store API we added before 1.0. The store will be pluggable, and have functions like readQuery,writeQuery,watchQuery,readFragment`, etc. on it.

acomito commented 7 years ago

is there a TLDR for a delete mutation?

TSMMark commented 7 years ago

@acomito If I'm not mistaken, TLDR: you can either:

  1. manually update every single query that depends on the deleted record(s) using updateQueries.
  2. manually refetch every single query that depends on the deleted record(s) using refetchQueries.
  3. manually query every record that will change as part of the mutation aka "fat query".
acomito commented 7 years ago

It would be amazing if you could just take the response from the mutation (in your .then()) and do

ApolloStore.remove({ _id: 'fdsakfjaslkfjsalfjsaf'});

rather than

graphql(DELETE_BRAND, {
  props: ({ ownProps, mutate }) => ({
    deleteBrand: (id) => mutate({
      variables: { id },
      optimisticResponse: {
        deleteBrand: id,
      },
      updateQueries: {
        brands: (prev, { mutationResult }) => {
          // will find category and brand index from the previous list
          let categoryIndex, brandIndex
          prev.brand_categories.forEach((category, cindex) => {
            const bindex = category.brands.findIndex(b => b.id === id)
            if (bindex > -1) {
              categoryIndex = cindex
              brandIndex = bindex
            }
          })
          // then remove brand from the next list
          return update(prev, {
            brand_categories: {
              [categoryIndex]: {
                brands: { $splice: [[brandIndex, 1]] }
              }
            }
          })
        },
      },
    }),
  }),
}),

I've read 5 different things on doing a simple delete from the store, but am still getting errors tyring to use update/updateQueries:

    this.props.deleteBuilding({ 
      variables,
      update: (store, { data }) => { 
        const storeData = store.readQuery({query: GET_BUILDINGS });
        console.log(storeData);
      }
    })
    .then(res => {
      return browserHistory.push('/buildings')
    })
    .catch(e => console.log(e))

resulting in

Error: Can't find field buildings({}) on object (ROOT_QUERY) {
  "user": {
    "type": "id",
    "id": "User:ohiLFDnPcH9fHXYGS",
    "generated": false
  },
  "buildings({\"params\":{\"searchText\":null}})": [
    {
      "type": "id",
      "id": "Building:38DDv5sf7RjdJBYwf",
      "generated": false
    },
TSMMark commented 7 years ago

@acomito Following your example, I believe you would need a typename in there as well:

ApolloStore.remove({ __typename: 'Brand', id: 'fdsakfjaslkfjsalfjsaf' });
acomito commented 7 years ago

refetchQueries does not seem to work for a delete. In my .then() I redirect to a table of buildings, and the deleted building is still there.

    this.props.deleteBuilding({ variables, refetchQueries: [{ query: GET_BUILDINGS }] })
    .then(res => {
      this.setState({false: true});
      return browserHistory.push('/admin/buildings')
    })
    .catch(e => console.log(e))
acomito commented 7 years ago

Okay, so I am able to delete a building from my store, but now I'm getting some errors about typenames or something:


    this.props.deleteBuilding({ 
      variables,
      //refetchQueries: [{ query: GET_BUILDINGS }],
      updateQueries: {
          getBuildings: (prev, { mutationResult }) => {
            return update(prev, {
                buildings: {
                  $unshift: [mutationResult.data],
                },
              });
          },
        },
    })
    .then(res => {
      this.setState({false: true});
      return browserHistory.push('/admin/buildings')
    })
    .catch(e => console.log(e))
You're using fragments in your queries, but either don't have the addTypename:
  true option set in Apollo Client, or you are trying to write a fragment to the store without the __typename.
   Please turn on the addTypename option and include __typename when writing fragments so that Apollo Client
   can accurately match fragments.

WARNING: heuristic fragment matching going on!

stale[bot] commented 7 years ago

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

stale[bot] commented 7 years ago

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

stale[bot] commented 7 years ago

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

MichaelDeBoey commented 7 years ago

I don't think this one should be closed yet 🙂

Geczy commented 7 years ago

I ended up doing something like this

updateQueries: {
  userQuery: (prev, { mutationResult }) => {
    const newReservation = mutationResult.data.deleteReservation
    return {
      user: {
        ...prev.user,
        reservations: [
          ...prev.user.reservations.filter(
            r => r.id !== newReservation.id
          ),
        ],
      },
    }
  },
},
n1ru4l commented 7 years ago

I thought about the ability to use directives in order to mark mutations. E.g. @delete. then the mutation would return either null or the Id of the object that should get deleted. In case of a mutation that deletes multiple items the mutation could return a list of IDs. We could then either implement this in Apollo or as a plugin, I did not check out Apollo 2 yet so I can not tell where it would fit in best.

Any suggestions or opinions on this approach? 😊

jbaxleyiii commented 7 years ago

I think this is a few things.

  1. Improving the docs to outline how to do this
  2. Implementing some level of cache invalidation mechanism to support the core client to intelligently update

Anyone want to take as stab at number one? I'd be happy to help!

We are actively working on number 2!

Keeping this issue open until the docs are at least improved here.

@peggyrayzis I think this should be an entire section on the new docs