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

Model saving after inserting creates a double instance #85

Open
JelleScheer opened this issue Nov 6, 2019 · 7 comments
Open

Model saving after inserting creates a double instance #85

JelleScheer opened this issue Nov 6, 2019 · 7 comments
Labels
enhancement New feature or request

Comments

@JelleScheer
Copy link

JelleScheer commented Nov 6, 2019

Hi!

In my use case I want to create an empty version of a model (let's call it Post) and after filling it I want to save it to the database and sync it with the empty version.

The example flow would be:

Post.insert({ data: { title: 'Basic title' } }).then((new_post) => {
     Post.api().post('', new_post.posts[0]);
  });

The insert function creates a new empty instance of the model, and if I look in the store it's visible as _no_key_1. After sending the empty instance along to the api().post() method it creates a new instance instead of replacing/syncing _no_key_1 with the response, so ultimately I have two instances in my store: _no_key_1 and an instance with the ID that was just stored in the database.

What can I do to sync the response with _no_key_1?

@kiaking
Copy link
Member

kiaking commented Nov 7, 2019

Ahh, this is a good one. Yeah, it would be great to have this feature. So I would like to start a discussion.

I think the ideal way to deal with this issue is to have some special option. But not sure how it should look like.

The hard part is how can we determine what record should be replaced. With your example, your record doesn't have a primary key set. That's why you're getting _no_key_X. If you set your id to be this.increment(), then it will become some integer.

Now, we need some way to let Vuex ORM Axios know what record to be deleted. In your case, you could do something like this to achieve what you want.

Post.insert({ data: { title: 'Basic title' } })
  .then((new_post) => {
    const p = new_post.posts[0]

    Post.api().post('', p, {
      delete: p.id
    });
  })
  .then((response) => {
    response.save();
  });

You need to call response.save because if you set delete option, the response will not be saved to the store. Maybe we could have something like replace option...? Which will delete the record, but still saves the response.

Post.insert({ data: { title: 'Basic title' } })
  .then((new_post) => {
    const p = new_post.posts[0]

    Post.api().post('', p, {
      replace: p.id // <- Replace!
    });
  });

Do you have any good idea on how the API should look like?

@kiaking kiaking added the enhancement New feature or request label Nov 7, 2019
@alexvdschans
Copy link

alexvdschans commented Nov 8, 2019

The way that we currently work around this issue is by extending the model itself with a custom store action and using the dataTransformer property as a callback to delete all entities with id == null. In our case this always deletes the unmatched record but this could obviously lead to unwanted deletes of other unpersisted models aswell:

Inside ./models/Board.js

static apiConfig = {
    actions: {
      store (board) {
        this.post('/api/v3/boards', board, {
          dataTransformer: (response) => {
              Board.delete((board) => {
                  return board.id == null
              })
              return response.data.data
          }
        }).catch(error => {
            board.errors = error.response.data
        });
      }
    }
  }

And inside the component:

 methods: {

            save: function() {

                Board.api().store(this.board);
                
            }

        },

An option would be to add a unique identifier to the dirty model and match it based on that, but it would require the backend to also return that identifier which is not ideally for all use-cases.

@JelleScheer
Copy link
Author

JelleScheer commented Nov 11, 2019

Instead of calling a replace function we could pass a parameter { sync: true } to Model.api.post() to sync the callback with the model that was saved. That way we would not have to delete the created model which would mean the frontend (in most cases) would not have to 'reset'.

To demonstrate with my use case:

Post.insert({ data: { title: 'Basic title' } }).then((new_post) => {
     Post.api().post('', new_post.posts[0], {sync: true});
});

This would sync new_post with the callback from the post method. This would require the axios plugin to generete a (u)uid under the hood for each model that was created/inserted so the sync method knows which model in the store to sync with. The sync method could just check that model to see which fields to sync.

@kiaking
Copy link
Member

kiaking commented Nov 15, 2019

Ah, interesting. Maybe it could work. All records created by Vuex ORM has $id attribute. It's always unique (it's the index key value of the record). So maybe we could use that. Well, still you need to pass data with that unique key, in your case at new_post.posts[0]. In this case, we're passing model instance so no problem.

But if you do things like Post.api().post('', { title: 'Example title' }, {sync: true});, There's no way to know what record to sync.

So I'm thinking we add some option here.

Post.api().post('...', { ... }, {
  sync: 1
})

When sync has value, Vuex ORM Axios will replace the record with the response.

Post.api().post('...', post, {
  sync: true
})

When sync is set to true, it will expect the data to be model, and replace the record by looking into models $id value.

What do you think?

@mean-cj
Copy link

mean-cj commented Nov 21, 2019

Hello @kiaking

I think "sync" option can be add
if enabled sync option , the first remove all data on current model
and remove all relations only hasOne , hasMany

example i want get all post with comment records from api
and sync data to Post Model

export default class Post extends Model {
  static fields() {
    return {
      id: this.attr(null),
      user: this.belongTo(User, 'user_id'),
      comments: this.hasMany(Comment,'coment_id')
    }
  }
}

Post.api().queryPost('...', post, {
  sync: true
})

Now i use , remove all comment and post before call api

export default class Post extends Model {
  static apiConfig = {
     actions: {
      queryPost() {
         this.model.deleteAll() // or Post.deleteAll() 
         Comment.deleteAll()
         return this.get('/posts')
      }
  }
}

Remove all data before insert is not perfect because vue render have blink
i think sync can remove only _id not found on the model or overide data is better than call deleteAll before insertOrUpdate

do you think?

i'm sorry for know little english.

@kiaking
Copy link
Member

kiaking commented Nov 27, 2019

@mean-cj Well if you want deleteAll records, then you could simply use create method, so maybe introducing API to specify Vuex ORM method would be enough.

Post.api().get('...', {
   persistBy: 'create'
})

This issue is originally about replacing single or few records with new one, so maybe we should have new issue for delete and insert all thing.

@parkan
Copy link

parkan commented Oct 12, 2020

chiming in with a +1 on this functionality, I think it's a quite common RESTy pattern to POST a model to get a server-assigned id

the workaround I am looking at now is having an extra model class TempPost extends Post which overrides the id field to be a UID type (not sure if there's a good way to override just one field?) and deleting the corresponding TempPost once the real Post is saved

however, this only really works for us because in our logic the model must be synced to proceed past a certain point, if you needed ambient sync that worked alongside other functionality it wouldn't work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants