rollbackAttributes() not working after more than one 422 error

[akappen]

I have an API server that performs server side validation of records and responds with the expected 422 status and a body of validation errors. The model correctly parses the validation errors in the body and makes them available on the errors object for display in the template. At this point if the user cancels the edit operation, I rollback the attribute changes to revert the model to the last saved version. This all works as expected.

If the model is saved two or more times with validation errors, each request responds with the same 422 status and validation errors body. If I try to rollback the attribute values at this point the original attributes are not restored!

I attempted to capture this in a test, but I couldn't figure out how to test this 422 error case. If anyone can point me to tests in ember data that exercise the 422 response condition I might be able to use those as a guide.

Instead of having a nice reproducible test, I can walk through the problem in the browser's javascript console. The first example shows proper rollback after a single 422 failure. The second shows a broken rollback after two 422 failures. This behavior happens on all of my models, not just the task model used in the example.

DEBUG: -------------------------------
DEBUG: Ember                    : 1.13.7
DEBUG: Ember Data               : 1.13.8
DEBUG: jQuery                   : 1.11.3
DEBUG: Ember Simple Auth        : 0.8.0-beta.3
DEBUG: Ember Simple Auth Devise : 0.8.0-beta.3
DEBUG: -------------------------------

With one 422 response the rollback works:

> var task
undefined
> $E.store.findRecord('task', 1).then(function(t) {task = t;})
Promise {_id: 181, _label: undefined, _state: undefined, _result: undefined, _subscribers: Array[0]}
> task
Class {id: "1", store: Class, container: Container, _internalModel: InternalModel, currentState: Object…}
> task.get("name")
"Make a card!"
> task.set("name", null)
Class {id: "1", store: Class, container: Container, _internalModel: InternalModel, currentState: Object…}
> task.get("name")
null
> task.save()
Class {__ember1440016252384: null, __nextSuper: undefined, __ember_meta__: Object}
jquery.js:9664 PUT http://localhost:4200/api/v1/tasks/1 422 (Unprocessable Entity)
. . . cut stack trace . . .
Error: The adapter rejected the commit because it was invalid
. . . cut stack trace . . .
> task.get("name")
null
> task.get("errors.name")
[Object, Object]
> task.get("errors.name")[0].message
"Name can't be blank"
> task.get("errors.name")[1].message
"Name is too short (minimum is 10 characters)"
> task.rollbackAttributes()
undefined
> task.get("name")
"Make a card!"

With more than one 422 response rollback fails to restore the attributes (note at the end that the model's 'data' object still has the original values even though rollback didn't restore them on the model itself):

> var task
undefined
> $E.store.findRecord('task', 1).then(function(t) {task = t;})
Promise {_id: 267, _label: undefined, _state: undefined, _result: undefined, _subscribers: Array[0]}
> task.get("name")
"Make a card!"
> task.set("name", null)
Class {id: "1", store: Class, container: Container, _internalModel: InternalModel, currentState: Object…}
> task.get("name")
null
> task.save()
Class {__ember1440017317668: null, __nextSuper: undefined, __ember_meta__: Object}
jquery.js:9664 PUT http://localhost:4200/api/v1/tasks/1 422 (Unprocessable Entity)
. . . cut stack trace . . .
Error: The adapter rejected the commit because it was invalid
. . . cut stack trace . . .

> task.get("name")
null
> task.get("errors.name")
[Object, Object]
> task.get("errors.name")[0].message
"Name can't be blank"
> task.get("errors.name")[1].message
"Name is too short (minimum is 10 characters)"
> task.save()
Class {__ember1440017317668: null, __nextSuper: undefined, __ember_meta__: Object}
jquery.js:9664 PUT http://localhost:4200/api/v1/tasks/1 422 (Unprocessable Entity)
. . . cut stack trace . . .
Error: The adapter rejected the commit because it was invalid
. . . cut stack trace . . .

> task.get("name")
null
> task.get("errors.name")[0].message
"Name can't be blank"
> task.get("errors.name")[1].message
"Name is too short (minimum is 10 characters)"
> task.rollbackAttributes()
undefined
> task.get("name")
null
> task.get("errors.name")
null
> task.get("data").name
"Make a card!"

The 422 response body has this format:

{"errors":
  {
    "name":["Name can't be blank","Name is too short (minimum is 10 characters)"],
    "description":["Description can't be blank"]
  }
}

[RobIsHere]

I encountered this problem, too. Some debugging reveals that this line is causing the bug:
https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/model/states.js#L357

It happens this way:
_attributes is copied to _inFlightAttributes before saving
_attributes is set to an empty Object
(only second+ time when already in invalid state: invalid.exit is called and clears _inFlightAttributes )
save fails
_inFlightAttributes is copied back to _attributes, to recover the model like before saving
_inFlightAttributes is set to an empty Object

So invalid.exit should not clear _inFlightAttributes in this case.

Sorry, i don't know enough about ember data and the side effects of a change like this. So i cannot make a fix and PR myself.

[icecoldmax]

I'm encountering exactly this problem too. My app uses a modal to edit a model. Click submit and it saves or encounters errors, click cancel and it closes the modal and does this.get("model").rollbackAttributes();.

Like @akappen, cancelling works fine after 1 validation error, but after the second one it seems to rollback to the state it was after the first validation error, rather than reloading completely.

I don't know about the internals either but I seem to have gotten around it by making a function to do it manually:

rollbackRecordAttrs() {
  this.get("model").eachAttribute((attr) => {
    let savedAttr = this.get(`model._internalModel._data.${attr}`);
    this.get("model").set(attr, savedAttr);
  });
},

actions: {
  save() {
    this.get("model").save().then(() => {
      this.closeModal();
    }, () => {
       // Empty error function
    });
  },

  cancel() {
    this._super();
    this.rollbackRecordAttrs();
  }
}

Another issue is that relationships don't yet rollback either. I did read that that's in the works though. I have found you can get the ids of belongsTo relationships by looking at:

let keyName = "nameOfBelongsToRelationship"; // whatever it may be
this.get(`model._internalModel._relationships.initializedRelationships.${keyName}.canonicalMembers.list.firstObject.id`);

[tchak]

will investigate this weekend

[RobIsHere]

Any progress on this Bug? For now I fixed it with an initializer changing the RootState like this:

import DS from 'ember-data';

export function initialize(/* container, application */) {

// Fix bug with rollback after 2 or more invalid saves
delete DS.RootState.loaded.created.invalid.exit;
delete DS.RootState.loaded.updated.invalid.exit;

}

export default {
name: 'model-fix',
initialize: initialize
};

[pangratz]

Ok, so I looked at it and I think the change suggested by @RobIsHere is the way to go: remove the exit handler on the invalid state. This has been introduced in #1755, where another bug has been fixed by this (rollback after setting an attribute didn't work). Removing the exit doesn't re-introduce the bug, so this seems to be fixed since then elsewhere in the code base.

@RobIsHere do you want to submit a PR with the above mentioned change? I can help with adding a test to catch a regression ...

[icecoldmax]

I'm having some success with the fix by @RobIsHere, however I encountered something else I think might be worth mentioning, rare though it may be.

As I mentioned above my app uses a modal to edit a model. Well we tried to be clever and put 3 buttons on the modal: save, cancel and delete, the idea being you can just do it all from one modal. However, the DB puts a foreign key constraint on deleting the record that still has associated data, and due to business requirements we just need to inform the user with an error that deletion is prohibited while that data exists (so they have to purposefully go and delete the associated data first). We do that with a backend validation in rails and then expect this.get('model.errors') to be populated. THIS ALL WORKS.

The problem seems to be in the state of the model. The code:

  actions: {
    save() {
      this.get('model').save().then(() => {
        this.closeModal();
      }, () => {});
    },

    cancel() {
      this._super();
      this.get('model').rollbackAttributes();
    },

    deleteResource() {
      this.get('model').destroyRecord().then(() => {
        this.closeModal();
      }, () => {
        // no rollbackAttributes() here as that clears the validation error message
      });
    }
  }

A problem arises when you press delete, it fails validations, and then you hit save. Since the save button is calling the save() action, it tries to send another DELETE action, instead of the regular PUT, and I can't rollbackAttributes() because that'll clear the error message leaving the user with no information.

Another problem occurs when I fail a Save twice, then hit Delete (which fails), then cancel. The next time I open the model, the errors are still there. If I hit delete in that state, I get:

Uncaught Error: Attempted to handle event `deleteRecord` on <integrated@model:contractor::ember1186:0dcf92aa-442f-42f3-a234-1fa4d874dd11> while in state root.deleted.inFlight.

I realise it's mostly our fault, caused by a cumbersome UI, and we actually successfully worked around this by separating the delete and edit modals, but I feel like it is related to the state of the model and so warranted inclusion in this issue.

[alvincrespo]

👍 Just came across this issue as well.

[icecoldmax]

Thanks @tchak and everyone else.

Will this be backported to 1.13?

[bmac]

I have cherry-picked this fix to the 1.13 branch. It will be in the next release, likely sometime early next week. Edit: This fix has been released in Ember Data 1.13.14.

[alvincrespo]

Thanks everyone! This is very awesome. 👍