Cannot call `.save()` multiple times

[jacobq]

It seems that calling .save() on a record multiple times without waiting for the previous promise(s) to resolve leads to an error being thrown. AFAIK, there is nothing in the ember-data contract that prohibits this (though it seems rather pointless in this example).

Reproduction repo here: jacobq/ember-pouch-back-to-back-save

Perhaps this is another manifestation of whatever is causing #237

[jacobq]

Looking a little deeper into this, it seems to be because calling save results in a Pouch/Couch update regardless of whether or not data has actually changed. In other words, doing await record.save() 10 times will make 10 revisions even though the actual data hasn't changed. At least, that's what I'm surmising based on the error message I saw when running this on ember 2.x:

Error: Assertion Failed: 'item:A44E0283-8F82-3F6A-9234-BFF0BC19D788' was saved to the server, but the response returned the new id '223A8B1B-3581-4EB2-829B-D6610FE97C0F'. The store cannot assign a new id to a record that already has an id.
    at new EmberError (http://localhost:7357/assets/vendor.js:24386:25)
    at Object.assert (http://localhost:7357/assets/vendor.js:24629:15)
    at Class.updateId (http://localhost:7357/assets/vendor.js:101057:60)
    at Class.didSaveRecord (http://localhost:7357/assets/vendor.js:100998:12)
...

Without much knowledge of PouchDB/CouchDB, it seems to me that there is some kind of coupling between revisions or IDs in PouchDB/CouchDB and those in ember-data. If that is the case, isn't that bad? For one, it means that doing this will be problematic:

const createSpecialPersonRecord = () => store.createRecord('person', { id: 'special_id' });
let record = createSpecialPersonRecord();
await record.save();
await record.destroyRecord();
// ...
record = createSpecialPersonRecord(); // will throw error about ID already having been used
// ...

See also https://github.com/jacobq/ember-pouch-back-to-back-save-2x

[jacobq]

I started trying ember-pouch again this week and have been making some effort to learn more about how it & ember-data work.

One thing that stood out to me was that store.createRecord(...) does not call the Adapter's createRecord method (record.save() does this). Also, record.save() behaves differently depending on whether or not the store believes that the data exists / has been persisted to the back-end. e.g. from the guides (emphasis added):

Records in Ember Data are persisted on a per-instance basis. Call save() on any instance of DS.Model and it will make a network request.
Ember Data takes care of tracking the state of each record for you. This allows Ember Data to treat newly created records differently from existing records when saving.

This helped me understand why calling .save() again before the first has completed can result in duplicate entries or errors.

const rec = store.createRecord('post', { title: 'foo' });
// rec.isNew = true, rec.isSaving=false, rec.hasDirtyAttributes=true
rec.save(); // begins creating a new persistent entry in the DB
// rec.isNew = true, rec.isSaving=true, rec.hasDirtyAttributes=true
rec.save(); // looks like it should "update" the same record but since
            // it hasn't persisted yet and the current implementation of
            // `ember-pouch` doesn't check `rec.isSaving`, this actually
            // attempts to create another record in the DB.

After the second item is created in the DB, an error gets thrown by ember data because when the first rec.save() completed, the store updated its internal ID, so when the second transaction completes and tries to do the same, its ID is out-of-date.

-private.js:4458 Uncaught (in promise) Error: Cannot set the id 43580ECB-25E8-26F3-9CF2-7ACB02AD74A6 on the record item:66496535-8f93-42d8-98c3-92875ea66d13 as there is no such record in the cache.
    at InternalModelFactory.setRecordId (-private.js:4458)
    at Store.setRecordId (-private.js:14198)
    at RecordDataStoreWrapper.setRecordId (-private.js:8209)
    at RecordDataDefault.didCommit (-private.js:11634)
    at InternalModel.adapterDidCommit (-private.js:5961)
    at Store.didSaveRecord (-private.js:14140)
    at -private.js:15019
    at Backburner._run (backburner.js:1013)
    at Backburner._join (backburner.js:989)
    at Backburner.join (backburner.js:760)

[jacobq]

One way to work around this "back-to-back" problem is to modify our implementation of Adapter.createRecord so that it detects this case, e.g.:

createRecord: function(store, type, snapshot) {
  const record = snapshot.record;
  if (record._emberPouchSavePromise) {
    // `createRecord` has been called with this record before, 
    // so we don't want to actually create another entry in the DB this time
    return record._emberPouchSavePromise;
  }

  this._init(store, type);
  var data = this._recordToData(store, type, snapshot);
  let rel = this.get('db').rel;

  let id = data.id;
  if (!id) {
    id = data.id = rel.uuid();
  }
  this.createdRecords[id] = true;
  // Save a reference to this promise in case duplicate calls are made on this record
  record._emberPouchSavePromise = rel.save(this.getRecordTypeName(type), data).catch((e) => {
    delete this.createdRecords[id];
    throw e;
  });
  return record._emberPouchSavePromise;
},

[jlami]

I'm not sure if just dropping the second save would work. Because the record could have changed between the two calls, so we still should persist any changes.

I'm not really sure but I think this should be fixed in ember-data itself. That layer is responsible for all the state tracking and should know whether save is actually a correct action at that moment. The second call should then chain onto the first promise so it can see what the response was. And it should call adapter.updateRecord instead of createRecord if it succeeded, or start another createRecord if it failed. Fixing this in ember-pouch feels like the wrong place.

[jacobq]

...the record could have changed between the two calls, so we still should persist any changes.

I think you're right, but we need to make sure that they get persisted to the correct (same) record (rather than a copy).

I'm not really sure but I think this should be fixed in ember-data itself. That layer is responsible for all the state tracking and should know whether save is actually a correct action at that moment.

I was asking about this in the ember-data Discord channel and got this answer:
"basically, the adapter needs to decide whether to send a new request if you call save more than once and the previous requests haven't finished"

[jlami]

Wow, I'm not really sure how I feel about that. But if that is really their final answer we indeed have to do something ourselves.

[jacobq]

... [then] we indeed have to do something ourselves.

Yeah, I suspect that this is because there isn't a "one size fits all" solution that would make sense for every conceivable adapter (though I would think that this could be done by overriding a default instead...). Plus, applications can avoid this situation manually by checking flags on Model like isNew and isSaving before calling .save() again.

[runspired]

I'm not sure if just dropping the second save would work. Because the record could have changed between the two calls, so we still should persist any changes.

You could always choose to queue and chain saves, or discard the save if it contained the same data as an already pending save request.

That layer is responsible for all the state tracking

I don't recommend our state tracking, the proliferation of addons for additional state tracking makes it fairly clear that EmberData's is not good enough. It'll get better with time, we have some solid plans for it, but it's kinda far down the roadmap.

and should know whether save is actually a correct action at that moment.

This is unknowable, and as such will always be an app code decision. Only the developer knows their API contract and the adapter is where the developer aligns the API contract with the requests EmberData is issuing.

That said there are two potential solutions that allow you to continue issuing as many requests as you like.

1. Use lid in versions containing the IDENTIFIERS feature flag and have pouch make use of it for either the ID or to ensure that all saves save to a consistent ID.
2. Use adapter.generateIdForRecord to assign the ID on the client during creation.

[runspired]

P.S. once you have resolved this if you'd like to PR ember-pouch as an external-partner test to EmberData we're always happy to have more test coverage, especially around ensuring common patterns for extending the library continue to work as expected :)