Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX beta] Overhaul queryRecord #4300

Merged
merged 1 commit into from
May 3, 2016
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
92 changes: 84 additions & 8 deletions addon/-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -1043,20 +1043,96 @@ Store = Service.extend({
},

/**
This method delegates a query to the adapter. This is the one place where
adapter-level semantics are exposed to the application.
This method makes a request for one record, where the `id` is not known
beforehand (if the `id` is known, use `findRecord` instead).

Exposing queries this way seems preferable to creating an abstract query
language for all server-side queries, and then require all adapters to
implement them.
This method can be used when it is certain that the server will return a
single object for the primary data.

This method returns a promise, which is resolved with a `RecordObject`
once the server returns.
Let's assume our API provides an endpoint for the currently logged in user
via:

```
// GET /api/current_user
{
user: {
id: 1234,
username: 'admin'
}
}
```

Since the specific `id` of the `user` is not known beforehand, we can use
`queryRecord` to get the user:

```javascript
store.queryRecord('user', {}).then(function(user) {
let username = user.get('username');
console.log(`Currently logged in as ${username}`);
});
```

The request is made through the adapters' `queryRecord`:

```javascript
// app/adapters/user.js
import Adapter from "ember-data/adapter";

export default Adapter.extend({
queryRecord(modelName, query) {
return Ember.$.getJSON("/api/current_user");
}
});
```

Note: the primary use case for `store.queryRecord` is when a single record
is queried and the `id` is not kown beforehand. In all other cases
`store.query` and using the first item of the array is likely the preferred
way:

```
// GET /users?username=unique
{
data: [{
id: 1234,
type: 'user',
attributes: {
username: "unique"
}
}]
}
```

```javascript
store.query('user', { username: 'unique' }).then(function(users) {
return users.get('firstObject');
}).then(function(user) {
let id = user.get('id');
});
```

This method returns a promise, which resolves with the found record.

If the adapter returns no data for the primary data of the payload, then
`queryRecord` resolves with `null`:

```
// GET /users?username=unique
{
data: null
}
```

```javascript
store.queryRecord('user', { username: 'unique' }).then(function(user) {
console.log(user); // null
});
```

@method queryRecord
@param {String} modelName
@param {any} query an opaque query to be used by the adapter
@return {Promise} promise
@return {Promise} promise which resolves with the found record or `null`
*/
queryRecord(modelName, query) {
assert("You need to pass a model name to the store's queryRecord method", isPresent(modelName));
Expand Down
6 changes: 5 additions & 1 deletion addon/-private/system/store/finders.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,14 @@ export function _queryRecord(adapter, store, typeClass, query) {
promise = _guard(promise, _bind(_objectIsAlive, store));

return promise.then(function(adapterPayload) {
assert("You made a `queryRecord` request for a " + typeClass.modelName + ", but the adapter's response did not have any data", payloadIsNotBlank(adapterPayload));
var record;
store._adapterRun(function() {
var payload = normalizeResponseHelper(serializer, store, typeClass, adapterPayload, null, 'queryRecord');

assert("Expected the primary data returned by the serializer for a `queryRecord` response to be a single object or null but instead it was an array.", !Array.isArray(payload.data), {
id: 'ds.store.queryRecord-array-response'
});

Copy link
Contributor

Choose a reason for hiding this comment

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

At first sight it seemed that was not necessary here, but I guess it handles the case of custom adapters/serializers, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep 👍

//TODO Optimize
record = store.push(payload);
});
Expand Down
10 changes: 10 additions & 0 deletions addon/serializers/json-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,16 @@ const JSONAPISerializer = JSONSerializer.extend({
return normalizedPayload;
},

normalizeQueryRecordResponse() {
let normalized = this._super(...arguments);

assert('Expected the primary data returned by the serializer for a `queryRecord` response to be a single object but instead it was an array.', !Array.isArray(normalized.data), {
id: 'ds.serializer.json-api.queryRecord-array-response'
});

return normalized;
},

/**
@method extractAttributes
@param {DS.Model} modelClass
Expand Down
10 changes: 10 additions & 0 deletions addon/serializers/rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,16 @@ var RESTSerializer = JSONSerializer.extend({
continue;
}

runInDebug(function() {
let isQueryRecordAnArray = requestType === 'queryRecord' && isPrimary && Array.isArray(value);
let message = "The adapter returned an array for the primary data of a `queryRecord` response. This is deprecated as `queryRecord` should return a single record.";

deprecate(message, !isQueryRecordAnArray, {
id: 'ds.serializer.rest.queryRecord-array-response',
until: '3.0'
});
});

/*
Support primary data as an object instead of an array.

Expand Down
10 changes: 0 additions & 10 deletions tests/integration/adapter/find-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,16 +141,6 @@ testInDebug('When a single record is requested, and the payload is blank', (asse
}, /You made a `findRecord` request for a person with id the-id, but the adapter's response did not have any data/);
});

testInDebug('When a single record is queried for, and the payload is blank', (assert) => {
env.registry.register('adapter:person', DS.Adapter.extend({
queryRecord: () => Ember.RSVP.resolve({})
}));

assert.expectAssertion(() => {
run(() => store.queryRecord('person', { name: 'the-name' }));
}, /You made a `queryRecord` request for a person, but the adapter's response did not have any data/);
});

testInDebug('When multiple records are requested, and the payload is blank', (assert) => {
env.registry.register('adapter:person', DS.Adapter.extend({
coalesceFindRequests: true,
Expand Down
50 changes: 50 additions & 0 deletions tests/integration/adapter/json-api-adapter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import setupStore from 'dummy/tests/helpers/store';
import Ember from 'ember';

import {module, test} from 'qunit';
import testInDebug from 'dummy/tests/helpers/test-in-debug';

import DS from 'ember-data';
import isEnabled from 'ember-data/-private/features';
Expand Down Expand Up @@ -234,6 +235,55 @@ test('find many records', function(assert) {
});
});

test('queryRecord - primary data being a single record', function(assert) {
ajaxResponse([{
data: {
type: 'posts',
id: '1',
attributes: {
title: 'Ember.js rocks'
}
}
}]);

run(function() {
store.queryRecord('post', {}).then(function(post) {
assert.equal(passedUrl[0], '/posts');

assert.equal(post.get('title'), 'Ember.js rocks');
});
});
});

test('queryRecord - primary data being null', function(assert) {
ajaxResponse([{
data: null
}]);

run(function() {
store.queryRecord('post', {}).then(function(post) {
assert.equal(passedUrl[0], '/posts');

assert.strictEqual(post, null);
});
});
});

testInDebug('queryRecord - primary data being an array throws an assertion', function(assert) {
ajaxResponse([{
data: [{
type: 'posts',
id: '1'
}]
}]);

assert.expectAssertion(function() {
run(function() {
store.queryRecord('post', {});
});
}, "Expected the primary data returned by the serializer for a `queryRecord` response to be a single object but instead it was an array.");
});

test('find a single record with belongsTo link as object { related }', function(assert) {
assert.expect(7);

Expand Down
52 changes: 51 additions & 1 deletion tests/integration/adapter/rest-adapter-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1299,7 +1299,25 @@ test("query - data is normalized through custom serializers", function(assert) {
}));
});

test("queryRecord - returns a single record in an object", function(assert) {
test("queryRecord - empty response", function(assert) {
ajaxResponse({});

store.queryRecord('post', { slug: 'ember-js-rocks' }).then(assert.wait(function(post) {
assert.strictEqual(post, null);
}));
});

test("queryRecord - primary data being null", function(assert) {
ajaxResponse({
post: null
});

store.queryRecord('post', { slug: 'ember-js-rocks' }).then(assert.wait(function(post) {
assert.strictEqual(post, null);
}));
});

test("queryRecord - primary data being a single object", function(assert) {
ajaxResponse({
post: {
id: '1',
Expand Down Expand Up @@ -1338,6 +1356,38 @@ test("queryRecord - returning an array picks the first one but saves all records
}));
});

testInDebug("queryRecord - returning an array is deprecated", function(assert) {
let done = assert.async();

ajaxResponse({
post: [{ id: 1, name: "Rails is omakase" }, { id: 2, name: "Ember is js" }]
});

assert.expectDeprecation('The adapter returned an array for the primary data of a `queryRecord` response. This is deprecated as `queryRecord` should return a single record.');

run(function() {
store.queryRecord('post', { slug: 'rails-is-omakaze' }).then(function() {
done();
});
});
});

testInDebug("queryRecord - returning an single object doesn't throw a deprecation", function(assert) {
let done = assert.async();

ajaxResponse({
post: { id: 1, name: "Rails is omakase" }
});

assert.expectNoDeprecation();

run(function() {
store.queryRecord('post', { slug: 'rails-is-omakaze' }).then(function() {
done();
});
});
});

test("queryRecord - data is normalized through custom serializers", function(assert) {
env.registry.register('serializer:post', DS.RESTSerializer.extend({
primaryKey: '_ID_',
Expand Down
26 changes: 26 additions & 0 deletions tests/integration/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -590,3 +590,29 @@ testInDebug('store#findRecord that returns an array should assert', assert => {
});
}, /expected the primary data returned from a `findRecord` response to be an object but instead it found an array/);
});

module("integration/store - queryRecord", {
beforeEach() {
initializeStore(DS.Adapter.extend());
}
});

testInDebug('store#queryRecord should assert when normalized payload of adapter has an array an data', function(assert) {
env.adapter.queryRecord = function() {
return {
cars: [{ id: 1 }]
};
};

env.serializer.normalizeQueryRecordResponse = function() {
return {
data: [{ id: 1, type: 'car' }]
};
};

assert.expectAssertion(function() {
run(function() {
store.queryRecord('car', {});
});
}, /Expected the primary data returned by the serializer for a `queryRecord` response to be a single object or null but instead it was an array./);
});