updating to ED 1.13

[benkonrath]

Ember-data 1.13 was just released with some changes that affect EDA. You can read about the details here:
http://emberjs.com/blog/2015/06/18/ember-data-1-13-released.html

The first issue I found with when trying ED 1.13.4 with EDA 0.5.6 is that RESTAdapter.ajaxError() has been removed and EDA calls it through _super() (https://github.com/dustinfarris/ember-django-adapter/blob/v0.5.6/addon/adapters/drf.js#L76). While ED 1.13 does call ajaxError() on an adapter if it's present, this is a breaking change in ED 1.13 because RESTAdapter.ajaxError() is a public method that has been removed between ED 1.0.0-beta.19.2 and 1.13.0.

I want to decide on a plan for updating to ED 1.13. Should EDA 1.0 support the latest betas (e.g. 18, 19) and run with the deprecation warnings from ED? Or should we aim for EDA 1.0 to support the new conventions in ED 1.13 which means breaking compatibility with the 0.5.x? If we go this route, there isn't an issue with the breaking change in ED 1.13. On the other hand adding RESTAdapter.ajaxError() to EDA to maintain compatibility is easy enough.

My opinion is that EDA 1.0 should work with the new conventions in ED 1.13 but I wanted to see what you guys think before I start to work on this.

[dustinfarris]

Thanks for digging into this. ED 1.13 is a big release. I am against supporting ED betas. Let's fix up the breaking change with ajaxError and release 0.6.0.

[benkonrath]

Ok, we're on the same page. Is there a reason why you don't want to release EDA 1.0.0 that's compatible with ED 1.13? It's just a number to me but I'm just trying to understand your thoughts.

[dustinfarris]

Yeah, It's time for 1.0. Let's do that, and follow the pattern outlined in The Ember 2.x Project—when Ember/ED bump to 2.x, so do we, when they bump to 3.x, so do we, etc. We'll do it loosely, tracking only major version bumps to keep our support aligned, and keep minor/patch versions for our own improvements.

[e3b0c442]

I noticed that after attempting to update to ED 1.13, IDs for newly created objects (For some of my objects IDs are set client-side) were not being serialized/sent to the server. Is this related to the above noted change? If not I'll open a new issue for it.

Appreciate everybody's work on this, this adapter has saved me a ton of time.

[benkonrath]

@bierdybard Thanks for letting us know. It's definitely information that I'll need when I tackle the upgrade to ember-data 1.13.

[holandes22]

Hi guys, quick jump in here. I propose to have a Beta or RC release before final 1.0
I've seen this pattern with several addons and it is a good approach in my opinion, so people can try out the beta/rc and let us know of critical problems before closing the final release.

I would drop support for old ED beta versions and support 1.13 which is the first non-beta version, this way we are completely aligned with ember releases.

BTW, @benkonrath just wanted to say amazing work with supporting ED 1.13 and links PRs man!

[dustinfarris]

Beta sounds good. @benkonrath are you going to own the upgrade? I'd like to ship sometime next week if possible if we are going to allow time for a beta period. If you're busy @holandes22 or myself can take this one.

[holandes22]

I'd like to work on this if it is ok with you guys. Waiting for your ack to avoid duplicate work

[dustinfarris]

@holandes22 👍

[holandes22]

Just found out that ED 1.13 works with ember 1.12.1 and later, meaning we will have to do so as well, any issues with this? usually people upgrades to latest ember versions, but might leave behind someone.
In my opinion is OK, if you use ember-data you want to have the stable version, and if that requires a fairly new ember version the request is reasonable

[dustinfarris]

I have no problem dropping support for Ember <1.12.1. Ember has been very clear about how core libraries (Ember, ED, etc) are going to stay in sync version-wise going forward.

[benkonrath]

@holandes22 Great, thanks! I briefly investigated the update a couple of days ago. The first thing I found was that ajaxError has been replaced by handleResponse.

https://github.com/emberjs/data/blob/0d603e964d62b581acc8f1268d19f7e69bdf64fd/packages/ember-data/lib/adapters/rest-adapter.js#L716

Here's what I have so far. It's not complete but it might help you get started.

// addon/adapters/drf.js
  handleResponse: function(status, headers, payload) {
    if (this.isSuccess(status, headers, payload)) {
      return payload;
    } else if (this.isInvalid(status, headers, payload)) {
      return new DS.InvalidError(payload);
    }

    let errors = this.normalizeErrorResponse(status, headers, payload);

    if (status === 500) {
      return new DS.AdapterError(errors, 'Internal Server Error');
    } else {
      return new DS.AdapterError(errors);
    }
  },

  isInvalid: function(status) {
    return status === 400;
  },

Dropping support for older version of ember is ok by me too. We can always do a 0.5.7 release now so that people can use this version with Ember < 1.12.1. Thanks!

[holandes22]

Great, I'll start working on a PR

[holandes22]

So it turns out that to get ready for ember data 2.0, the task is much bigger. Several normalization methods have been renamed and they now basically need to conform with the JSON API spec, which will become the default adapter in 2.0

I will make a PR to use the new error hooks and format now (so we can support 1.13) and open a separate ticket to get EDA ready for ED 2.0 (according to http://emberjs.com/blog/2015/06/18/ember-data-1-13-released.html there will be ember-watson helpers to help with the transition)

[benkonrath]

Yeah, it's a big change. A quick fix would be to add the old RESTAdapter.ajaxError() to our adapter and call it instead of this._super() in our adapter (https://github.com/dustinfarris/ember-django-adapter/blob/v0.5.6/addon/adapters/drf.js#L76).

// addon/adapters/drf.js
  restAdapterAjaxError: function(jqXHR, responseText, errorThrown) {
    var isObject = jqXHR !== null && typeof jqXHR === 'object';

    if (isObject) {
      jqXHR.then = null;
      if (!jqXHR.errorThrown) {
        if (typeof errorThrown === 'string') {
          jqXHR.errorThrown = new DS.Error(errorThrown);
        } else {
          jqXHR.errorThrown = errorThrown;
        }
      }
    }
    return jqXHR;
  },

This should give us ED 1.13 compatibility and we can do a full restructure for our 2.0 release. What do you think?

[holandes22]

It's a nice approach, but I started already working on a PR to use the new handleResponse, I prefer working on that a little more and see how it goes. I should have something ready tomorrow for you guys to review.

If we get that working we will have 1.13 support but with deprecation warnings for all the other normalization methods, those we can fix in a separate ticket to support ED 2.0.0 (which should be coming out soon btw)

Anyway, let's see how it goes with my PR and decide

[benkonrath]

Sounds good. I think removing the depreciation warnings as soon as we can is probably a better approach anyway.

[holandes22]

Seeking an opinion here. Previously, the error response contained camelizedNames, whereas now, ember-data just adds the payload as is producing reponse errors with snake_case names (just as they come from the server).

Should we keep this behavior? @benkonrath any idea where this was/is handled either in EDA or ED?

I think ED just expects that the json payload contains attr names in camelCase, but can't find confirmation of that
Where in the pipeline the payload attrs are converted from snake_case to camelCase?, in particular where is done for response with errors?

[holandes22]

You can check WIP here https://github.com/holandes22/ember-django-adapter/tree/103-update-ember-data

[dustinfarris]

I don't believe error response data has ever been modified by ED or EDA. I have in the past relied on non_field_errors which were provided as is in response.errors, e.g.:

this.store.createRecord('user', data).save().then(function() {
  // Success ...
}, function(response) {
  // Error
  var nonFieldErrors = response.errors.non_field_errors;
  // ...
});

[benkonrath]

The field errors should definitely be camelCase but ED should do this (it used to do it anyway). For the non field errors, we should just decide what we want to do (keep as snake_case or convert to camelCase) and add an acceptance test so we know if it ever changes.

[dustinfarris]

It should be all one way or the other. And yes, preferably camel-cased.

[holandes22]

Yes agree. The error object contains now camelCase attributes. We modify this ourselves from the payload that gets to handleResponse, apparently if the response is successful, ED converts to camelCase up in the pipeline. I'll post a question at slack channel to see if this is correct behavior.

Another thing, I'm having one test failing in the command line (it passes in chrome), with this error:

TypeError: Requested property names of a value that is not an object. at http://localhost:7357/assets/vendor.js, line 76874
[object Object]

The test is

test('Server error', function(assert) {                                                                                                                                          
  assert.expect(4);                                                                                                                                                              

  return Ember.run(function() {                                                                                                                                                  

    return store.findRecord('post', 2).then({}, function(response) {                                                                                                             
      const error = response.errors[0];                                                                                                                                          

      assert.ok(response);                                                                                                                                                       
      assert.equal(error.status, 500);                                                                                                                                           
      assert.equal(error.details, '<h1>Server Error (500)</h1>');                                                                                                                
      assert.equal(response.message, 'Internal Server Error');                                                                                                                   
    });                                                                                                                                                                          
  });                                                                                                                                                                            
});

Not much idea what the problem is, did you encounter something similar at some point?

[benkonrath]

I see the problem in the command line too. The problem comes from the fact that the 500 method doesn't return json. I changed it to json and it worked, however, that's not a good test because a 500 in django doesn't return json. Perhaps this is a bug in ED 1.13? Not sure.

[holandes22]

@benkonrath ok, but weird is not failing on chrome.

I'm tempted to remove this test, my reasoning being that if you hit an API endpoint you are expecting json as content type and not html. What I usually do is add a global catcher so every exception that escapes the API gets converted to a proper json response. Is this a real case scenario?

[benkonrath]

@holandes22 The test was probably more relevant when the jqXHR object was being returned in the error case (as in ED beta.19). ED is now handling parse errors so removing the test should be ok.

https://github.com/emberjs/data/blob/v1.13.4/packages/ember-data/lib/adapters/rest-adapter.js#L958

It's still strange that it fails on the command line and not in browsers. Not sure what's going on there.

[benkonrath]

Forgot to point out that we currently have a similar try / catch block which is why the test is there.

https://github.com/dustinfarris/ember-django-adapter/blob/master/addon/adapters/drf.js#L81

[holandes22]

I see, thank you.

According to https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/model/errors/invalid.js#L44 there is where the error names should be converted to camelCase

[benkonrath]

That comment might be out of date because the example has a reference to ajaxError() which has been removed. extractErrors is still in the json serializer so it's probably still useful:

https://github.com/emberjs/data/blob/2385a7c2e64467a1823b91795a07bf68d3940b4c/packages/ember-data/lib/serializers/json-serializer.js#L1261

[holandes22]

yes, you are correct, found it after making the comment (missed that part in the docstring of handleResponse).

The problem is that for some reason the default implementation is not being able to map the properties in the payload to the DS.Model, so investigating that now

[holandes22]

If I format the error array as ED expects it as explained here http://emberjs.com/api/data/classes/DS.InvalidError.html
(which is similar to a jsonapi error obj but with a key 'details' instead of 'detail', not sure if this is an overlook in ED) I see that extractError maps it properly.

For example:

// response from server 400
{'field_name1': ['error 1', 'error 2'], 'field2': ['error 1']}

adapter converts to:

[
  {details: 'error 1', source: { pointer: 'data/attributes/field_name1' }},
  {details: 'error 2', source: { pointer: 'data/attributes/field_name1' }},
  {details: 'error 1', source: { pointer: 'data/attributes/field2' }}
]

This gets converted by default extractErrors properly (camelCased) to the hash:

{fieldName1: ['error 1', 'error 2'], fiedl2: ['error 1']}

But, I though that the InvalidError.errors property was replaced with this hash and then returned to the request, but apparently this is only set in the record, and the InvalidResponse has the unmodified payload in the errors attribute (the one that looks like a jsonapi error object).

So in the case of a 400, the record in error state will have an 'errors' property with an error array built upon the hash from extractErrors https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/store.js#L2188,
Which can be retrieved with:

post.get('errors.fieldName1') => [{attribute: 'fieldName1', message: 'error 1'}, {attribute: 'fieldName1', message: 'error 2'}]

This is somewhat counter-intuitive to me, instead of getting the validation issues from the error response, I need to fetch them from the record (and in a somewhat awkward manner).

I modified my branch to work this way, but does this sounds reasonable guys? I think I'm missing something here.

[dustinfarris]

I don't think you're missing anything. You have accomplished getting ED to set the errors on the model in the desired format. That is important for users and especially addons that expect errors to be formatted that way.

Meanwhile, DRF's response gets returned raw to the user in response.errors, which is great for our users who may not necessarily understand the internals, but want to inspect the DRF response for whatever reason.

[dustinfarris]

What happens with non_field_errors that don't have a field pointer?

[holandes22]

@dustinfarris good point on non_field_errors, I overlooked that test case. I think since it has no key, naturally won't appear in the record (as ED cannot map it). But since the InvalidError object contains the original payload, it will be present there (which is annoying forcing us to use 2 different objects to form the errors). I'll add a test case for it

BTW, ED 1.13.5 was released last night, I'll update the branch for that version two
They fixed the inconsistency I mentioned with the jsonapi error object (using "details" key instead of "detail")
emberjs/data#3497

[dustinfarris]

@holandes22 can you open a PR and add the "in progress" label.

[holandes22]

@dustinfarris sure, wanted to the test my branch a little more before submitting the PR, but I'm ready no, sending it in a few minutes