Updated to the RC3 standard of JSON API

[eneuhauser]

Per JSON API RC3 release, the spec is close to being finalized. Now there is a line in the sand, it's easy to build against the RC3 standard until either another release candidate or 1.0 is ratified. The main feature that this pull request supports that #68 does not is the consistent linkage change, where a linked resource will have a linkage property that includes the type and id. The serializer is not backwards compatible and only works with the RC3 standard.

One concept of JSON API I have not been able to support is the self link. The current interface for the adapter does not allow accessing the owner record when doing a createRecord or updateRecord. I'm storing the self link within the resource's links, but there currently isn't a way to access it.

On the current project I'm working on, I have updated my server to be compliant with the RC3 standard and this adapter and serializer is working well.

[green-arrow]

The following could be because I'm not following the spec correctly, but humor me. In a case where there are links in objects in the included array, an error is thrown if the link is not a string, but an object. This means that I cannot, in an object in the included array, provide linkage to another object in the included array.

Example:

{
  "data": {
    "type": "myType",
    "id": "1",
    "name": "myType Name",
    "links": {
      "self": "/myTypes/1",
      "linkOne": {
        "self": "/myTypes/1/linkOne",
        "linkage": { "type": "subLink", "id": "1" }
      }
    }
  },
  "included": [{
    "type": "subLink",
    "id": "1",
    "name": "subLink Name",
    "links": {
      "self": "/myTypes/1/linkOne",
      "linkTwo": {
        "self": "/myTypes/1/linkOne/linkTwo",
        "linkage": { "type": "otherSubLink", "id": "1" } // <---- Link to another sideloaded record
      }
    }
  }, {
    "type": "otherSubLink",
    "id": "1",
    "name": "otherSubLink Name",
    "links": {
      "self": "/myTypes/1/linkOne/linkTwo"
    }
  }]
}

Thoughts?

[eneuhauser]

Hi @green-arrow,

I believe your interpretation is correct and I just committed a change that should fix this issue. I also included additional tests to check for this scenario. Thanks for the feedback.

[green-arrow]

@eneuhauser I just pulled down your branch with the recent update and it's working beautifully. Tests look good as well.

@kurko - is there anything in particular that is stopping this from being merged?

[kurko]

No. Will review this afternoon!
On Thu, Mar 19, 2015 at 8:52 AM Andrew Walton notifications@github.com
wrote:

@eneuhauser https://github.com/eneuhauser I just pulled down your
branch with the recent update and it's working beautifully. Tests look good
as well.

@kurko https://github.com/kurko - is there anything in particular that
is stopping this from being merged?

—
Reply to this email directly or view it on GitHub
#71 (comment).

[jonkoops]

I'm still missing the right Content-Type and Accept header for exchanging data. From the specification:

Any requests that contain content MUST include a Content-Type header whose value is application/vnd.api+json. This header value MUST also include media type extensions relevant to the request.

JSON API requests MUST include an Accept header specifying the JSON API media type. This header value MUST also include media type extensions relevant to the request. Servers MUST return a 406 Not Acceptable status code if this header is missing or specifies an unsupported media type.

I suggest overwriting the ajaxOptions method to set these headers in the AJAX request, like so:

ajaxOptions: function(url, type, options) {
  var hash = this._super(url, type, options);  

  hash.accepts = {
    json: 'application/vnd.api+json'
  };

  if (hash.data && type !== 'GET') {
    hash.contentType = 'application/vnd.api+json';
  }

  return hash;
}

Updated my comment to include the Accept header

[green-arrow]

Additional comment in regards to findBelongsTo:

findBelongsTo: function(store, record, url, relationship) {
  var related = record[relationship.key];
  // FIXME Without this, it was making unnecessary calls, but cannot create test to verify.
  if(related) { return; }
  return this.ajax(url, 'GET');
}

Due to this FIXME, belongsTo relationships don't get loaded if the key is found on the record.

Example model:

export default DS.Model.extend({
    vitals: DS.belongsTo('vitals', { async: true })
});

When trying to get the vitals property, the if(related) { return; } block is executed and a request is never fired.

[green-arrow]

Additional issue that I ran across: specifying an empty array for the root data attribute fails.

When normalizePayload is called, we recognize that we're dealing with an array and call extractArrayData:

extractArrayData: function(data, payload) {
  var type = data.length > 0 ? data[0].type : null, serializer = this;
  data.forEach(function(item) {
    if(item.links) {
      serializer.extractRelationships(item.links, item);
      //delete data.links;
    }
  });

  payload[type] = data;
}

If no data is present, we punt and say the type is null. This causes issues (particularly in the case where you are looking up a hasMany relationship). The serializer cannot resolve the type, and we get an error:

Error: Assertion Failed: The response from a findHasMany must be an Array, not undefined

[eneuhauser]

@jonkoops, I added 'application/vnd.api+json' in the list of accept headers and set the content type for non-GET requests. Both of these are overridable as properties in the adapter without having to override the whole ajaxOptions method.

@green-arrow, I've fixed the empty array issue. I also removed my override of findBelongsTo. I'm not seeing the extra requests I use to, so some other logic must have fixed that.

[jonkoops]

@eneuhauser Looks good to me 👍

[green-arrow]

@eneuhauser awesome.

Additional question. The spec says that you could include a query param ?include=relationship, making the server sideload the specified relationship. This means that the property on the model will most likely be marked as { async: true }. In this case, calling model.get('relationship') results in an API call being made. I would guess that if the record has already been side-loaded, we wouldn't want to kick off another API call.

Thoughts?

[eneuhauser]

@green-arrow, if you look at the compound-document-test, that's the use case it's validating. The test case ensures that regardless if the relationship is marked as async true or false, if a relationship is returned in the included section, it will not make another request.

This is the case when you do model.get('relationship;); however, if you do store.find('model'), it will always make a new call.

[green-arrow]

@eneuhauser gotcha. I will have to do some digging on my end then, because I am seeing more calls executed even though the belongsTo relationship has already been loaded in the included section.

[joshsmith]

@eneuhauser is this supposed to fire API calls when hasMany and belongsTo are marked as { async: true }?

[green-arrow]

@joshsmith @eneuhauser stated that API calls should not be fired in this case if they've already been loaded via the included section, but I am still seeing API calls fired for both relationship types.

[eneuhauser]

@joshsmith, the app will fire API calls when calling get on a hasMany and belongsTo that are marked as { async: true } when those resources have not already been pushed to the store. The resources can be pushed to the store by either being returned in an included block within the response or from a previous call.

@green-arrow, I'm not sure if you're still having your issue with having extra calls, but I had seen that once before, which is why I use to have the if(related) { return; } logic in the findBelongsTo. Unfortunately, I can't explain how that issue got cleared up for me and it doesn't exist in the integration tests. What's even odder is that var related = record[relationship.key]; should never return a value because record is a DS.Model and relationship.key shouldn't be a property.

[green-arrow]

@eneuhauser the following prevents duplicate requests:

app/adapters/application.js

findBelongsTo: function(store, snapshot, url, relationship) {
  if(snapshot.belongsTo(relationship.key)) { return; }

  return this._super(store, snapshot, url, relationship);
}

Note: This is using ember-data 1.0.0-beta.16 and taking advantage of the new snapshot API. By detecting if the belongsTo relationship exists on the snapshot, we can stop another call from being made.

It seems to me, though, that this behavior should be native to ember-data and not something that the JsonApiAdapter should have to do. If the record is already loaded, should ember-data simply not execute the findBelongsTo method on the adapter?

[green-arrow]

I think I've narrowed it down a bit more, but now I'm stuck. In ember-data, there is the following code:

BelongsToRelationship.prototype.getRecord = function() {
  //TODO(Igor) flushCanonical here once our syncing is not stupid
  if (this.isAsync) {
    var promise;
    if (this.link) {
      var self = this;
      promise = this.findLink().then(function() {
        return self.findRecord();
      });
    } else {
      promise = this.findRecord();
    }

    return PromiseObject.create({
      promise: promise,
      content: this.inverseRecord
    });
  } else {
    Ember.assert("You looked up the '" + this.key + "' relationship on a '" + this.record.constructor.typeKey + "' with id " + this.record.get('id') +  " but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async (`DS.belongsTo({ async: true })`)", this.inverseRecord === null || !this.inverseRecord.get('isEmpty'));
    return this.inverseRecord;
  }
};

The issue is that this.link is populated with a link to the record, so a new promise is being kicked off and a request made. If this.link was falsey, a new PromiseObject would be created with its content set to this.inverseRecord, whose value is the already loaded record.

In json-api-serializer, there is a method called extractRelationships. Near the bottom of the method, there is this if block:

if(id) {
  resource[link] = id;
}

If I change that block to the following, additional requests are not made:

if(id) {
  resource[link] = id;
  delete resource.links[link];
}

As far as I can tell, this doesn't seem to have any adverse side effects. I have not pulled down the source and run the tests though. Could someone try what I have above and see if all tests pass? I would do that myself but I have to hop off for a while to attention my kids 😄

[tobyzerner]

Polymorphic relationships are broken with this because the type keys of any linkage objects are disregarded/thrown away. I was able to fix by adding it back in to the getLinkageId and getLinkageIds functions at the very bottom of json-api-serializer.js:

function getLinkageId(linkage) {
  if(Ember.isEmpty(linkage)) { return null; }
  return (Ember.isArray(linkage)) ? getLinkageIds(linkage) : {type: Ember.String.singularize(linkage.type), id: linkage.id};
}
function getLinkageIds(linkage) {
  if(Ember.isEmpty(linkage)) { return null; }
  var ids = [], index, total;
  for(index=0, total=linkage.length; index<total; ++index) {
    ids.push({type: Ember.String.singularize(linkage[index].type), id: linkage[index].id});
  }
  return ids;
}

[green-arrow]

By adding the delete line above, the code that @tobscure provided, and the following to the adapter, I think we're pretty much covered:

findBelongsTo: function(store, snapshot, url, relationship) {
  if(snapshot.belongsTo(relationship.key)) { return; }

  return this._super(store, snapshot, url, relationship);
},
findHasMany: function(store, snapshot, url, relationship) {
  if(snapshot.hasMany(relationship.key).get('length')) { return []; }

  return this._super(store, snapshot, url, relationship);
}

[eneuhauser]

Unfortunately, none of these pass the tests.

@green-arrow, by definition of the model, snapshot.belongsTo(relationship.key) should always return true regardless if it's loaded. This is similar to what I was trying before. Essentially, you need to see if the relationship has already been loaded without calling get, since that results in an infinite loop. But as you stated, that really should be the job of ember-data, not the adapter. As for deleting the link, the link is used for other things, like updating the record.

@tobscure, type isn't lost because it is defined in the relationship.

If you could send me a sample of your response, I can see if I could duplicate the problem.

[green-arrow]

@eneuhauser that's not correct. snapshot.belongsTo(relationship.key) will return a value if one is already loaded. Otherwise, undefined is returned. At least that is what is stated in the ember-data docs:

http://emberjs.com/api/data/classes/DS.Snapshot.html#method_belongsTo

Calling belongsTo will return a new Snapshot as long as there's any data available, such as an ID. If there's no data available belongsTo will return undefined.

I was not aware that the link was used for other things. In that case, it seems that there is a bug in ember-data in the method I posted above. It seems like this.inverseRecord should be the first thing checked instead of this.link, and if it is defined, the promise immediately resolved and no AJAX call made. Does that seem right to you?

Maybe @mixonic or somebody else familiar with ember-data could chime in on that?

[tobyzerner]

@eneuhauser Given a polymorphic relationship setup like this:

App.Activity = DS.Model.extend({
  subject: DS.belongsTo('subject', {polymorphic: true})
});

App.Subject = DS.Model.extend();
App.Post = App.Subject.extend();
App.Discussion = App.Subject.extend();

And a JSON-API response like this:

{
  "data": [
    {
      "type": "activity",
      "id": "1",
      "links": {
        "subject": {
          "linkage": { "type": "post", "id": "1" }
        }
      }
    },
    {
      "type": "activity",
      "id": "2",
      "links": {
        "subject": {
          "linkage": { "type": "discussion", "id": "2" }
        }
      }
    }
  ],
  "included": [
    {
      "type": "post",
      "id": "1"
    },
    {
      "type": "discussion",
      "id": "1"
    }
  ]
}

Currently, this serializer is normalizing the payload to this:

{
  "subject": 1
}

Which causes Ember Data to freak out because it doesn't know which model type the relationship is to. Instead, my code normalizes it to:

{
  "subject": { "type": "post", "id": 1 }
}

Which fixes the problem. It was a quick fix; it shouldn't include the type if it's not a polymorphic relationship (and I'm guessing that's the cause of the failed tests).

[green-arrow]

The code I had for findHasMany won't work.

findHasMany: function(store, snapshot, url, relationship) {
  if(snapshot.hasMany(relationship.key).get('length')) { return []; }

  return this._super(store, snapshot, url, relationship);
}

The next method called is extractArray, which just returns Ember.A() if no payload is defined.

The root of this problem seems to be the prototypes for BelongsTo and HasMany executing an ajax call if this.link is defined. I have no idea how to resolve this problem.

[green-arrow]

Sorry for spamming this PR, but I did manage to find a super hacky workaround for findHasMany:

findHasMany: function(store, snapshot, url, relationship) {
  if(snapshot.hasMany(relationship.key).get('length')) { return new Ember.RSVP.Promise((resolve, reject) => reject()); }

  return this._super(store, snapshot, url, relationship);
}

In all, my application adapter looks like this, and I'm not seeing any extra requests being made for data loaded via the included section, and all my side-loaded data appears correctly.

import Ember from 'ember';
import JsonApiAdapter from 'ember-json-api/json-api-adapter';
import ENV from '../../config/environment';

export default JsonApiAdapter.extend({
  host: ENV.APP.adapter.host,
  namespace: ENV.APP.adapter.namespace,
  findBelongsTo: function(store, snapshot, url, relationship) {
    if(snapshot.belongsTo(relationship.key)) { return; }

    return this._super(store, snapshot, url, relationship);
  },
  findHasMany: function(store, snapshot, url, relationship) {
    if(snapshot.hasMany(relationship.key).get('length')) { return new Ember.RSVP.Promise((resolve, reject) => reject()); }

    return this._super(store, snapshot, url, relationship);
  }
});

[green-arrow]

@tobscure how are you making polymorphic relationships work with this serializer? It seems like polymorphic support was explicitly removed by @eneuhauser

/**
     * Use "links" key, remove support for polymorphic type
     */
    serializeBelongsTo: function(record, json, relationship) {
      var attr = relationship.key;
      var belongsTo = record.belongsTo(attr);
      var type = this.keyForRelationship(relationship.type.typeKey);
      var key = this.keyForRelationship(attr);

      if (isNone(belongsTo)) return;

      json.links = json.links || {};
      json.links[key] = belongsToLink(key, type, get(belongsTo, 'id'));
    }

To clarify, I have a polymorphic relationship defined, but ember-data is not pushing anything to the store. If I change the belongsTo relationship to point directly to the child object and not the parent, everything works fine.

[gentle-noah]

Thank you for all the work on this! This is exactly the solution I've been looking for, for the errors I've been running into 😄

[jonkoops]

@eneuhauser Seems like issue #3 is a blocking issue in specification compliance.

[andrewbranch]

@jonkoops I submitted a PR to @eneuhauser implementing the PUT/PATCH swap needed for specification compliance: eneuhauser#6

[kurko]

Ok, so unless someone comes shouting and saying this is not working, I'm merging this tonight and releasing a new version 😄

[fotinakis]

👍 👍 👍 Yay!

[jamonholmgren]

Awesome!

[eneuhauser]

Awesome, thanks! There are 3 issues reported in my repository that I haven't had a chance to look at yet. Additionally, I'm working on the findHasMany issue. I'm hoping to have time tomorrow to work on them. If you merge the current pull request, I can submit separate pull requests to solve those issues.

Edit: After looking at the issues, I don't think there is anything to fix. The findHasMany is the only open issue.

[jamonholmgren]

@eneuhauser Thanks for your hard work on this. And @kurko of course. As an open source project maintainer myself, I know these hours sometimes go unnoticed. Much appreciated.

[jonkoops]

I think we should get eneuhauser#8 fixed first, it's a minor issue but it could cause some confusion.

[eneuhauser]

I've made some comments in issue#8. Would love some feedback if I should merge my fix.

[joelcox]

Thanks for your great work on this @eneuhauser. We've been using your branch for some time now and I haven't run into any big issues so far.

[jonkoops]

@kurko In my opinion we should get this merged, @eneuhauser anything blocking in your opinion?

[eneuhauser]

@jonkoops and @kurko, I think RC3 is good to go. There will be more changes before 1.0 hits. Someone already opened Issue 10 about a change since RC3.

Since ember-json-api is one of the de facto implementations of JSON API, I think it would be a good idea to keep up with the release candidates. I can begin work on that issue, but until another release candidate hits or 1.0, I'm not going to merge it in.

[Boubalou]

@eneuhauser Based on the latest RC3 spec, there is an attributes field that hold all of the model fields, is there any plan on updating this PR to support the final revision RC3?

Edit:
Nevermind this question, it is already covered in issue 10.

Keep it up!

[green-arrow]

👍 on getting this merged

[jonkoops]

@kurko https://youtu.be/vCadcBR95oU

[kurko]

Folks, I'm merging this soon. I'm on vacation and without computer. Let me
see if I can do something via mobile this weekend 😄
On Tue, May 12, 2015 at 7:09 PM Jon Koops notifications@github.com wrote:

@kurko https://github.com/kurko https://youtu.be/vCadcBR95oU

—
Reply to this email directly or view it on GitHub
#71 (comment).

[kurko]

😄

Now, would someone take the torch for RC4? Seems pretty straightforward

[eneuhauser]

Thanks for merging! I have been heads down on a few non-Ember projects, but I am days away from a vacation. I should have some downtime to update to RC4. Someone has already submitted a pull request into my repo. I'll also look into upgrading Ember and Ember Data versions. According to the status page, JSON API 1.0 is slated for tomorrow. That will get some consistencies in implementations to get everyone on the same page.

[josepjaume]

For the record: The latest developments are in eneuhauser#14. I'm using it in my app and seems to work fine 👍