Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions addon/adapters/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ export default Ember.Object.extend(FetchMixin, Evented, {
findRelated(resource, url) {
let type = resource;
if (typeof type === 'object') {
resource = resource.resource;
type = resource.type;
}
// use resource's service if in container, otherwise use this service to fetch
Expand All @@ -142,18 +141,25 @@ export default Ember.Object.extend(FetchMixin, Evented, {
@return {Promise}
*/
createResource(resource) {
let url = this.get('url');
const json = this.serializer.serialize(resource);
return this.fetch(url, {
return this.fetch(this.get('url'), {
method: 'POST',
body: JSON.stringify(json)
body: JSON.stringify(this.serializer.serialize(resource))
}).then(function(resp) {
if (resource.toString().match('JSONAPIResource') === null) {
return resp;
Copy link
Owner

@pixelhandler pixelhandler Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aars I'm curious if we need to set id in here as well. I forget if the id is already written during deserialization. Can merge w/o any change here but would be worth looking into just to confirm.

Copy link
Collaborator Author

@aars aars Sep 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're referring to here. I'm not clear on what is happening after then() here. What is the purpose of if (resource.toString().match('JSONAPIResource') === null) { ?

I think I interpreted that as checking if the resource was new or somehow was expected to be the same as the response, but I don't see how I got that from this line.

I'm suddenly quite confused about this method.

  • createResource should only be called on new resources, but there is nothing preventing this. Should we? (I don't think so, since we don't really enforce "newness")
  • When successful, we should update the resource with properties gotten from response (as happens from line #151). Always. No exception.
  • Return original resource.
  • That's it. But that leaves some questions:
    • What is return resp doing? I think the tests show that resp is already a Resource instance, so fetch might already update the resource? Is it?!
      • This probably has to do with fetchSuccessHandler returning either a resource instance or simply json-data. I need to figure out where the raw json-data is used.
    • And of course, what is that if (resource.toString().match()) line doing?
    • Who handles/catches the error? The user I guess (should be ok)
    • If fetch is successful (user won't catch that error) are there situation we should handle/check?
      • I can imagine: createResource called with resource that already has an id, but server ignores the id on POST (since that's not supposed to be used to update a resource), creates a new resource with the given properties, and returns them all with a different id. If we then simply update the id on the given resource we dun goofed.
      • I imagine we handle this like so:
        • isNew? Error about attempting to create an existing resource. No POST for you.
        • isNew? We POST since you asked nicely, but we error on receiving a mismatching id.

I'll dive into what fetch is doing exactly and where resources are instantiated exactly.

Also, I think I dun goofed myself by doing this.get('url') here, that should of course be this.fetchUrl(). Though that's what it did originally. But that's wrong, right? (needs fetchUrl)
Ah no. I see now that fetch calls fetchUrl, nice.

edit: Running all tests we never enter the if (resource.toString().match('JSONAPIResource') === null) { situation. I'm now even more unclear on it's usage ;)

} else {
resource.set('id', resp.get('id') );
let json = resp.getProperties('attributes', 'relationships', 'links', 'meta', 'type', 'isNew', 'id');
resource.didUpdateResource(json);
resource.set('id', resp.get('id'));
resource.didUpdateResource(
resp.getProperties(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems cleaner and easier to read. Sometimes I like to be conservative on extra lines, but it readability is more important.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good! :D

I have two colorcolumns in vim, one at 80 chars, one at 100 chars, making me very twitchy about lines surpassing those.

'attributes',
'relationships',
'links',
'meta',
'type',
'isNew',
'id'
)
);
this.cacheUpdate({ data: resource });
return resource;
}
Expand Down Expand Up @@ -263,9 +269,7 @@ export default Ember.Object.extend(FetchMixin, Evented, {
patchRelationship(resource, relationship) {
return this.fetch(this._urlForRelationship(resource, relationship), {
method: 'PATCH',
body: JSON.stringify({
data: resource.get(['relationships', relationship, 'data'].join('.'))
})
body: JSON.stringify(this._payloadForRelationship(resource, relationship))
});
},

Expand Down Expand Up @@ -307,7 +311,8 @@ export default Ember.Object.extend(FetchMixin, Evented, {
@return {String} url
*/
_urlForRelationship(resource, relationship) {
let url = resource.get(['relationships', relationship, 'links', 'self'].join('.'));
let meta = resource.constructor.metaForProperty(relationship);
let url = resource.get(['relationships', meta.relation, 'links', 'self'].join('.'));
Copy link
Owner

@pixelhandler pixelhandler Sep 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using the meta 👍 from the property. It's something I really wanted to do just have not made the time.

return url || [this.get('url'), resource.get('id'), 'relationships', relationship].join('/');
},

Expand All @@ -316,12 +321,15 @@ export default Ember.Object.extend(FetchMixin, Evented, {
@private
@param {Resource} resource instance, has URLs via it's relationships property
@param {String} relationship name (plural) to find the url from the resource instance
@param {String} id the id for the related resource
@param {String} id the id for the related resource or undefined current relationship data
@return {Object} payload
*/
_payloadForRelationship(resource, relationship, id) {
let data = resource.get(['relationships', relationship, 'data'].join('.'));
let resourceObject = { type: pluralize(relationship), id: id.toString() };
// actual resource type of this relationship is found in related-proxy's meta.
let meta = resource.constructor.metaForProperty(relationship);
let data = resource.get(['relationships', meta.relation, 'data'].join('.'));
if (id === undefined) { return {data: data}; }
let resourceObject = { type: pluralize(meta.type), id: id.toString() };
return { data: (Array.isArray(data)) ? [resourceObject] : resourceObject };
},

Expand Down
13 changes: 8 additions & 5 deletions addon/models/resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,12 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, {
*/
addRelationship(related, id) {
if (id !== undefined) { id = id.toString(); } // ensure String id.
let key = ['relationships', related, 'data'].join('.');

// actual resource type of this relationship is found in related-proxy's meta.
let meta = this.constructor.metaForProperty(related);
let key = ['relationships', meta.relation, 'data'].join('.');
let data = this.get(key);
let type = pluralize(related);
let type = pluralize(meta.type);
let identifier = { type: type, id: id };
let owner = (typeof getOwner === 'function') ? getOwner(this) : this.container;
let resource = owner.lookup(`service:${type}`).cacheLookup(id);
Expand All @@ -207,7 +210,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, {
} else {
data = identifier;
if (resource) {
this.set(`${related}.content`, resource);
this.set(`${meta.relation}.content`, resource);
}
}
return this.set(key, data);
Expand Down Expand Up @@ -463,9 +466,9 @@ function useComputedPropsMetaToSetupRelationships(owner, factory, instance) {
let meta = factory.metaForProperty(prop);
if (meta && meta.kind) {
if (meta.kind === 'hasOne') {
setupRelationship.call(instance, prop);
setupRelationship.call(instance, meta.relation);
} else if (meta.kind === 'hasMany') {
setupRelationship.call(instance, prop, Ember.A([]));
setupRelationship.call(instance, meta.relation, Ember.A([]));
}
}
} catch (e) {
Expand Down
48 changes: 48 additions & 0 deletions fixtures/api/employees.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
{
"data": [
{
"type": "employees",
"id": "1",
"links": {
"self": "http://api.pixelhandler.com/api/v1/employees/1"
},
"attributes": {
"name": "The Special"
},
"relationships": {
"supervisor": {
"links": {
"self": "http://api.pixelhandler.com/api/v1/employees/1/relationships/supervisor",
"related": "http://api.pixelhandler.com/api/v1/employees/1/supervisor"
}
}
}
},
{
"type": "supervisors",
"id": "2",
"links": {
"self": "http://api.pixelhandler.com/api/v1/supervisors/2"
},
"attributes": {
"name": "The Boss"
},
"relationships": {
"direct-reports": {
"links": {
"self": "http://api.pixelhandler.com/api/v1/supervisors/2/relationships/direct-reports",
"related": "http://api.pixelhandler.com/api/v1/supervisors/2/direct-reports"
}
}
}
}
],
"meta": {
"page": {
"sort": "-date",
"total": 2,
"limit": "5",
"offset": "0"
}
}
}
19 changes: 19 additions & 0 deletions fixtures/api/employees/1.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"data": {
"type": "employees",
"id": "1",
"links": {
"self": "http://api.pixelhandler.com/api/v1/employees/1"
},
"attributes": {
"name": "The Special"
},
"relationships": {
"supervisor": {
"links": {
"related": "http://api.pixelhandler.com/api/v1/employees/1/supervisor"
}
}
}
}
}
20 changes: 20 additions & 0 deletions fixtures/api/supervisors/2.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
{
"data": {
"type": "supervisors",
"id": "2",
"links": {
"self": "http://api.pixelhandler.com/api/v1/supervisors/2"
},
"attributes": {
"name": "The Boss"
},
"relationships": {
"direct-reports": {
"links": {
"self": "http://api.pixelhandler.com/api/v1/supervisors/2/relationships/direct-reports",
"related": "http://api.pixelhandler.com/api/v1/supervisors/2/direct-reports"
}
}
}
}
}
2 changes: 1 addition & 1 deletion tests/dummy/app/models/employee.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ import { hasOne, hasMany } from 'ember-jsonapi-resources/models/resource';
export default PersonResource.extend({
type: 'employees',
pictures: hasMany('pictures'),
supervisor: hasOne({resource: 'supervisor', type: 'employees'})
supervisor: hasOne('supervisor')
});
2 changes: 1 addition & 1 deletion tests/dummy/app/models/supervisor.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,5 @@ import { hasMany } from 'ember-jsonapi-resources/models/resource';

export default EmployeeResource.extend({
type: 'supervisors',
directReports: hasMany('employees')
directReports: hasMany({resource: 'direct-reports', type: 'employees'})
});
134 changes: 93 additions & 41 deletions tests/unit/adapters/application-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { setup, teardown, mockServices } from 'dummy/tests/helpers/resources';
import postMock from 'fixtures/api/posts/1';
import postsMock from 'fixtures/api/posts';
import authorMock from 'fixtures/api/authors/1';
import employeeMock from 'fixtures/api/employees/1';
import supervisorMock from 'fixtures/api/supervisors/2';

let sandbox;

Expand Down Expand Up @@ -224,51 +226,27 @@ test('#findRelated', function(assert) {
test('#findRelated is called with optional type for the resource', function (assert) {
assert.expect(4);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving these to fixtures is a good idea, thanks.

const done = assert.async();
let supervisor = this.container.lookup('model:supervisor').create({
type: 'supervisors',
id: '1000000',
attributes: {
name: 'The Boss',
},
relationships: {
employees: {
links: {
related: 'http://api.pixelhandler.com/api/v1/supervisors/1000000/employees'
}
}
}
});

let EmployeeAdapter = Adapter.extend({type: 'employees', url: '/employees'});
this.registry.register('service:supervisors', EmployeeAdapter.extend({
cacheLookup: function() {
return supervisor;
}
let supervisor = this.container.lookup('model:supervisor').create(supervisorMock.data);
let employee = this.container.lookup('model:employee').create(employeeMock.data);

let SupervisorAdapter = Adapter.extend({type: 'supervisors', url: '/supervisors'});
SupervisorAdapter.reopenClass({isServiceFactory: true});
let EmployeeAdapter = Adapter.extend({type: 'employees', url: '/employees'});
EmployeeAdapter.reopenClass({isServiceFactory: true});

this.registry.register('service:employees', EmployeeAdapter.extend({
cacheLookup: function () { return employee; }
}));
EmployeeAdapter.reopenClass({ isServiceFactory: true });
this.registry.register('service:employees', EmployeeAdapter.extend());
let service = this.container.lookup('service:employees');
let stub = sandbox.stub(service, 'findRelated', function () {
return RSVP.Promise.resolve(supervisor);
});
let employee = this.container.lookup('model:employee').create({
type: 'employees',
id: '1000001',
attributes: {
name: 'The Special',
},
relationships: {
supervisor: {
links: {
related: 'http://api.pixelhandler.com/api/v1/employees/1000001/supervisor'
}
}
}
let employeeService = this.container.lookup('service:employees');
let stub = sandbox.stub(employeeService, 'findRelated', function () {
return RSVP.Promise.resolve(null);
});
let url = employee.get('relationships.supervisor.links.related');
employee.get('supervisor').then(() => {

let url = supervisor.get('relationships.direct-reports.links.related');
supervisor.get('directReports').then(() => {
assert.ok(stub.calledOnce, 'employees service findRelated method called once');
assert.equal(stub.firstCall.args[0].resource, 'supervisor', 'findRelated called with supervisor resource');
assert.equal(stub.firstCall.args[0].resource, 'direct-reports', 'findRelated called with "direct-reports" resource');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aars I like using 'direct-reports' better than using 'supervisor' this improves the test.

assert.equal(stub.firstCall.args[0].type, 'employees', 'findRelated called with employees type');
assert.equal(stub.firstCall.args[1], url, 'findRelated called with url, ' + url);
done();
Expand Down Expand Up @@ -404,6 +382,30 @@ test('#createRelationship (to-one)', function(assert) {
});
});

test('#createRelationship uses optional resource type', function (assert) {
assert.expect(2);
const done = assert.async();

mockServices.call(this);
let adapter = this.subject({type: 'supervisors', url: '/supervisors'});
sandbox.stub(adapter, 'fetch', function () { return RSVP.Promise.resolve(null); });
let resource = this.container.lookup('model:supervisor').create(supervisorMock.data);
let promise = adapter.createRelationship(resource, 'directReports', '1');

assert.ok(typeof promise.then === 'function', 'returns a thenable');
promise.then(() => {
let jsonBody = JSON.stringify({data: [{type: 'employees', id: '1'}]});
assert.ok(
adapter.fetch.calledWith(
supervisorMock.data.relationships['direct-reports'].links.self,
{method: 'POST', body: jsonBody}
),
'#fetch called with url and options with data'
);
done();
});
});

test('#deleteRelationship (to-many)', function(assert) {
assert.expect(2);
const done = assert.async();
Expand Down Expand Up @@ -452,6 +454,30 @@ test('#deleteRelationship (to-one)', function(assert) {
});
});

test('#deleteRelationship uses optional resource type', function (assert) {
assert.expect(2);
const done = assert.async();

mockServices.call(this);
let adapter = this.subject({type: 'supervisors', url: '/supervisors'});
sandbox.stub(adapter, 'fetch', function () { return RSVP.Promise.resolve(null); });
let resource = this.container.lookup('model:supervisor').create(supervisorMock.data);
let promise = adapter.deleteRelationship(resource, 'directReports', '1');

assert.ok(typeof promise.then === 'function', 'returns a thenable');
promise.then(() => {
let jsonBody = JSON.stringify({data: [{type: 'employees', id: '1'}]});
assert.ok(
adapter.fetch.calledWith(
supervisorMock.data.relationships['direct-reports'].links.self,
{method: 'DELETE', body: jsonBody}
),
'#fetch called with url and options with data'
);
done();
});
});

test('#patchRelationship (to-many)', function(assert) {
assert.expect(2);
const done = assert.async();
Expand Down Expand Up @@ -502,6 +528,32 @@ test('#patchRelationship (to-one)', function(assert) {
});
});

test('#patchRelationship uses optional resource type', function (assert) {
assert.expect(2);
const done = assert.async();

mockServices.call(this);
let adapter = this.subject({type: 'supervisors', url: '/supervisors'});
sandbox.stub(adapter, 'fetch', function () { return RSVP.Promise.resolve(null); });
let resource = this.container.lookup('model:supervisor').create(supervisorMock.data);
resource.addRelationship('directReports', '1');
let promise = adapter.patchRelationship(resource, 'directReports');

assert.ok(typeof promise.then === 'function', 'returns a thenable');
promise.then(() => {
let jsonBody = JSON.stringify({data: [{type: 'employees', id: '1'}]});
assert.ok(
adapter.fetch.calledWith(
supervisorMock.data.relationships['direct-reports'].links.self,
{method: 'PATCH', body: jsonBody}
),
'#fetch called with url and options with data'
);
done();
});
});


// Even though create- and deleteRelationship both use _payloadForRelationship,
// which does the casting of id to String, we test them seperately to ensure this
// is always tested, even when internals change.
Expand Down
Loading