toranb / ember-data-django-rest-adapter

An ember-data adapter for django web applications powered by the django-rest-framework
MIT License
154 stars 27 forks source link

Support Error Handling #64

Closed g-cassie closed 10 years ago

g-cassie commented 10 years ago

I was doing some searching this morning for a way to handle ajax errors globally and came across the ajaxError method on DS.RESTAdapter.

It looks like ActiveModelAdapter implements this method to provide some basic error handling when requests return validation errors:

  ajaxError: function(jqXHR) {
    var error = this._super(jqXHR);

    if (jqXHR && jqXHR.status === 422) {
      var jsonErrors = Ember.$.parseJSON(jqXHR.responseText)["errors"],
          errors = {};

      forEach(Ember.keys(jsonErrors), function(key) {
        errors[Ember.String.camelize(key)] = jsonErrors[key];
      });

      return new DS.InvalidError(errors);
    } else {
      return error;
    }
  }

I think we could adapt this default behaviour for DjangoRestFramework. As I understand it, DRF will return a 400 status code when there are validation errors, and the error messages are contained in the rootkey, rather than being nested under an errors object. Other than that, it seems like an easy add.

Let me know your thoughts.

craigteegarden commented 10 years ago

From the original issue (https://github.com/emberjs/data/issues/1278) it looks like overriding the default ajaxError was added to put the record into the InvalidError state instead of the default error state when 422 status codes were returned.

In their situation, it seems like 422 is used exclusively for validation errors, does DRF use the 400 status code in the same way as the 422 code?

g-cassie commented 10 years ago

I see what you are saying, and I think you may be right that the 400 status code is not exclusively used for validation errors.

Looks like there was some discussion about using 422 for validation errors a few months ago here: https://github.com/tomchristie/django-rest-framework/pull/906. They didn't seem to reach any conclusion but judging by the last post it seems 422 responses have not been ruled out for django-rest-framework.

Maybe this is another non-starter from me!

craigteegarden commented 10 years ago

Thanks for bringing this up for discussion! Let's keep an eye on it.

Symmetric commented 10 years ago

Would it be possible to add a few lines of recommended best-practices for error handling to the README until this issue is fixed?

This is pretty unintuitive behaviour (though completely understandable given DRF's current design), and I'd imagine that everybody using this project seriously will have to roll their own error handling code, so some pointers would be very helpful.

It certainly took me a lot of pain just to find this issue, let alone figure out a workaround.

toranb commented 10 years ago

I'm certainly open to a pull request if you have a good example. I didn't find this was much of an issue today w/ the latest build (and a beta build of ember-data).

I usually do simple error handling in the form itself these days (not the model like I did back in 0.13)

here is a super simple example

var first = appointment.save();
var last = customer.save();
Ember.RSVP.all([first, last]).then(function(persisted) {
  //both save methods are legit
}, function(response) {
  var error = response.responseText;
});