[bug] updating primary key inserts new record

[SzNagyMisu]

trafficstars

PR #285 does not work because of line https://github.com/vuex-orm/vuex-orm/blob/150a01a0dc37d29b2f1e30cabe68a12f543a01ba/src/query/Query.ts#L932 There the variable instances is already filtered, and does not contain the original keys, but - if I am not mistaken - this.state.data does, so in the end they remain in the vuex store. There is a test case provided but that does not check if the original key exists or not.

Probably related to issue #398

[kiaking]

@SzNagyMisu Thank you so much for the report! We must fix this.

[SzNagyMisu]

Any ideas, concerning how it should be fixed? I have given some time and have one or two possible solutions. Also I would be happy to provide a pull request if you are interested.

[kiaking]

@SzNagyMisu That would be awesome! I would love to see your fix. Could you give it a try?

[SzNagyMisu]

@kiaking Happily :smiley:

The simplest of solutions I can come up with is to use the updateIndexes() method on the whole state.data object. So instead of this inside the commitUpdate() method:

instances = this.updateIndexes(instances)
this.commit('update', instances, () => {
  this.state.data = { ...this.state.data, ...instances }
})

we could write this:

this.commit('update', instances, () => {
  this.state.data = this.updateIndexes({ ...this.state.data, ...instances })
})

It may have performance issues, though, iterating through the complate state.data every time a single record is updated.

So perhaps we could simply make the updateIndexes() method deal with the state.data instead of the instances and call it after the merge:

commitUpdate (instances: Data.Instances): Data.Collection {
  this.commit('update', instances, () => {
    this.state.data = { ...this.state.data, ...instances }
    this.updateIndexes()
  })
  // return ...
}

private updateIndexes (instances: Data.Instances): void {
  Object.keys(instances).forEach(key => {
    const instance = instances[key]
    const id = String(this.model.getIndexIdFromRecord(instance))

    if (key !== id) {
      instance.$id = id
      this.state.data[id] = instance
      delete this.state.data[key]
    }
  })
}

but I'm not sure the updateIndexes() has the right to do this...

What do you think? Do you have any other ideas?

[kiaking]

@SzNagyMisu Thanks for the idea! Yeah, that might work. I need to dive into the code once more to get my head spinning. An update is pretty complex work right lol

I'll find a time to look into this one.

[SzNagyMisu]

@kiaking Any progress on this one?

[kiaking]

No, not yet sorry. It's on my list but haven't had time to do it...

[moravecjakub]

@kiaking Hello, I have a question about this issue. Currently I'm solving some issue with entity id handling in orm. Because when I create some temporary entity at client-side, then synchronize them to server side, I must insert new one and delete temporary one, which causes flashing while vue components rendering. There are two ways how to solve it - transactions or allow update id. I know that update id is not easy task, but I've found https://github.com/vuex-orm/vuex-orm/pull/285 which seems to be on a great way, how about that? Or is there some way how to implement something like transaction - dispatch more actions without re-rendering components? Another options is to separate client-side entity ids and server-side ids, but it looks like highway to hell :(

[moravecjakub]

Hi, I've made new pull request, which fixes duplicating of record. What is your opinion? https://github.com/vuex-orm/vuex-orm/pull/664

[drewbaker]

I just ran into this issue today when trying to create an optimistic UI pattern.

I create a temp record client side, then persist it to the server and update it after the server with a new ID, and I end up with a duplicate.

        // Create a temp Folder instantly in Vuex
        let optimsticFolder = await dispatch("insert", {
            data: {
                parent_id: parentId,
                name: name,
                project_id: projectId
            }
        })
        optimisticFolder = _get(optimsticFolder, "folders[0]", {})

        // Shape GQL vars for 8base
        let variables = {
            name: name,
            project: { connect: { id: projectId } },
            parent: { connect: { id: parentId } }
        }

        // Now submit to server, update Vuex with result or remove on error
        return await client
            .mutate({
                mutation: FOLDER_CREATE,
                variables
            })
            .then(({ data }) => {
                let folder = _get(data, "directoryCreate", {})
                dispatch("update", { where: optimisticFolder.id, data: folder })
                return folder
            })
            .catch(e => {
                dispatch("delete", optimisticFolder.id)
                throw new Error("GraphQL error trying to create a folder")
            })

Would love to get this working somehow.

[drewbaker]

It would be great to have a rollbackDelete method for this sort of thing too. Being able to optimistically delete, then rollback deletion on error server side would be great.

Currently I do this manually like this:

        // Get document so we can roll back on errors
        const oldDocument = Document.find(id)

        // Delete from Vuex immediately
        dispatch("delete", id)

        // Send to server here....

        // On error, add back old document
        dispatch("insertOrUpdate", {
                where: id,
                data: oldDocument
        })

[NicolasReibnitz]

Hi, I've made new pull request, which fixes duplicating of record. What is your opinion? #664

The fix by @moravecjakub seems to be working like a charm! I took the liberty to fork the latest version of Vuex ORM and apply his work to use it cleanly in my project. But if anyone wants to join the fun, please don't hesitate:

https://github.com/DasLaboratory/vuex-orm

Cheers and thanks, @moravecjakub, for your excellent work. This problem was "bugging" (🙄) me for a while!