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.

Sign up for GitHub

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 #4497] query/queryRecord/filter now support adapter options #4856

Merged
merged 3 commits into from
Mar 29, 2018
Merged

[BUGFIX #4497] query/queryRecord/filter now support adapter options #4856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0152014
[BUGFIX #4497] query/queryRecord/filter now support adapter options
stefanpenner Mar 13, 2017
9ff2c86
fix tests
runspired Mar 29, 2018
7eaf18c
make options the last arg
runspired Mar 29, 2018
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
33 changes: 23 additions & 10 deletions addon/-private/system/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1261,18 +1261,25 @@ Store = Service.extend({
@method query
@param {String} modelName
@param {any} query an opaque query to be used by the adapter
@param {Object} options optional, may include `adapterOptions` hash which will be passed to adapter.query
@return {Promise} promise
*/
query(modelName, query) {
query(modelName, query, options) {
assert(`You need to pass a model name to the store's query method`, isPresent(modelName));
assert(`You need to pass a query hash to the store's query method`, query);
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string');

let adapterOptionsWrapper = {};

if (options && options.adapterOptions) {
adapterOptionsWrapper.adapterOptions = options.adapterOptions
}

let normalizedModelName = normalizeModelName(modelName);
return this._query(normalizedModelName, query);
return this._query(normalizedModelName, query, null, adapterOptionsWrapper);
},

_query(modelName, query, array) {
_query(modelName, query, array, options) {
let token = heimdall.start('store._query');
assert(`You need to pass a model name to the store's query method`, isPresent(modelName));
assert(`You need to pass a query hash to the store's query method`, query);
Expand All @@ -1288,7 +1295,7 @@ Store = Service.extend({
assert(`You tried to load a query but you have no adapter (for ${modelName})`, adapter);
assert(`You tried to load a query but your adapter does not implement 'query'`, typeof adapter.query === 'function');

let pA = promiseArray(_query(adapter, this, modelName, query, array));
let pA = promiseArray(_query(adapter, this, modelName, query, array, options));
instrument(() => {
pA.finally(() => { heimdall.stop(token); });
});
Expand Down Expand Up @@ -1389,21 +1396,26 @@ Store = Service.extend({
@method queryRecord
@param {String} modelName
@param {any} query an opaque query to be used by the adapter
@param {Object} options optional, may include `adapterOptions` hash which will be passed to adapter.queryRecord
@return {Promise} promise which resolves with the found record or `null`
*/
queryRecord(modelName, query) {
queryRecord(modelName, query, options) {
assert(`You need to pass a model name to the store's queryRecord method`, isPresent(modelName));
assert(`You need to pass a query hash to the store's queryRecord method`, query);
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string');

let normalizedModelName = normalizeModelName(modelName);

let adapter = this.adapterFor(normalizedModelName);
let adapterOptionsWrapper = {};

if (options && options.adapterOptions) {
adapterOptionsWrapper.adapterOptions = options.adapterOptions
}

assert(`You tried to make a query but you have no adapter (for ${normalizedModelName})`, adapter);
assert(`You tried to make a query but your adapter does not implement 'queryRecord'`, typeof adapter.queryRecord === 'function');

return promiseObject(_queryRecord(adapter, this, modelName, query).then(internalModel => {
return promiseObject(_queryRecord(adapter, this, modelName, query, adapterOptionsWrapper).then(internalModel => {
// the promise returned by store.queryRecord is expected to resolve with
// an instance of DS.Model
if (internalModel) {
Expand Down Expand Up @@ -1778,10 +1790,11 @@ Store = Service.extend({
@param {String} modelName
@param {Object} query optional query
@param {Function} filter
@param {Object} options optional, options to be passed to store.query
@return {DS.PromiseArray}
@deprecated
*/
filter(modelName, query, filter) {
filter(modelName, query, filter, options) {
assert(`You need to pass a model name to the store's filter method`, isPresent(modelName));
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string');

Expand All @@ -1792,13 +1805,13 @@ Store = Service.extend({
let promise;
let length = arguments.length;
let array;
let hasQuery = length === 3;
let hasQuery = length >= 3;

let normalizedModelName = normalizeModelName(modelName);

// allow an optional server query
if (hasQuery) {
promise = this.query(normalizedModelName, query);
promise = this.query(normalizedModelName, query, options);
} else if (arguments.length === 2) {
filter = query;
}
Expand Down
14 changes: 7 additions & 7 deletions addon/-private/system/store/finders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ export function _findAll(adapter, store, modelName, sinceToken, options) {
}, null, 'DS: Extract payload of findAll ${modelName}');
}

export function _query(adapter, store, modelName, query, recordArray) {
export function _query(adapter, store, modelName, query, recordArray, options) {
let modelClass = store.modelFor(modelName); // adapter.query needs the class

let promise;
Expand All @@ -143,14 +143,14 @@ export function _query(adapter, store, modelName, query, recordArray) {

if (createRecordArray) {
recordArray = recordArray || store.recordArrayManager.createAdapterPopulatedRecordArray(modelName, query);
promise = Promise.resolve().then(() => adapter.query(store, modelClass, query, recordArray));
promise = Promise.resolve().then(() => adapter.query(store, modelClass, query, recordArray, options));
} else {
promise = Promise.resolve().then(() => adapter.query(store, modelClass, query));
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it intentional that options isn't passed along if createRecordArray is false? There isn't such a distinction in the documentation about when an included adapterOptions hash will be passed to adapter.query.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was a mistake actually.

@runspired / @hjdivad / @igorT can you sanity check that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Speaking with @runspired he reminded me what this was intentionally, as without a record array, we cannot pass options as a fourth argument without breaking user-land arity checks. Also, creating the record array eagerly and providing it here, had some performance implications.

The plan at the time, was to eventually change this API to use an object, instead of positional arguments, to avoid this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additional note: if the arity check says that adapter.query uses less than 4 arguments (recordArray is 4th, options is 5th), we don't pass either along.

I'm not sure we were as worried about breaking user-land arity checks as we were about the performance costs of materializing the snapshotRecordArray when we didn't need to.

If your query implementation has an arity of 4/5 and is still not receiving options, then let's dive into that; however, simply adding it into the function signature in prep for usage should be enough to receive any options that were passed in.

Copy link
Contributor

Choose a reason for hiding this comment

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

@runspired and I dug into this more, and there's an issue with the arity check where it doesn't properly detect the argument length for adapter.query. This is captured in #6232.

}

let label = `DS: Handle Adapter#query of ${modelClass}`;

let label = `DS: Handle Adapter#query of ${modelName}`;
promise = Promise.resolve(promise, label);

promise = _guard(promise, _bind(_objectIsAlive, store));

return promise.then(adapterPayload => {
Expand All @@ -173,11 +173,11 @@ export function _query(adapter, store, modelName, query, recordArray) {
}, null, `DS: Extract payload of query ${modelName}`);
}

export function _queryRecord(adapter, store, modelName, query) {
export function _queryRecord(adapter, store, modelName, query, options) {
let modelClass = store.modelFor(modelName); // adapter.queryRecord needs the class
let promise = Promise.resolve().then(() => adapter.queryRecord(store, modelClass, query));
let label = `DS: Handle Adapter#queryRecord of ${modelName}`;
let promise = Promise.resolve().then(() => adapter.queryRecord(store, modelClass, query, options));

let label = `DS: Handle Adapter#queryRecord of ${modelName}`;
promise = Promise.resolve(promise, label);
promise = _guard(promise, _bind(_objectIsAlive, store));

Expand Down
49 changes: 48 additions & 1 deletion tests/integration/adapter/store-adapter-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,12 @@ module("integration/adapter/store-adapter - DS.Store and DS.Adapter integration
name: DS.attr('string')
});

env = setupStore({ person: Person, dog: Dog });
env = setupStore({
serializer: DS.JSONAPISerializer,
adapter: DS.JSONAPIAdapter,
person: Person,
dog: Dog
});
store = env.store;
adapter = env.adapter;
},
Expand Down Expand Up @@ -1363,6 +1368,48 @@ test("store.findRecord should pass adapterOptions to adapter.findRecord", functi
});
});

test("store.query should pass adapterOptions to adapter.query ", function(assert) {
assert.expect(2);

env.adapter.query = function(store, type, query, array, options) {
assert.ok(!('adapterOptions' in query));
assert.deepEqual(options.adapterOptions, { query: { embed: true } });
return { data: [] };
};

return run(() => {
return store.query('person', {}, { adapterOptions: { query: { embed: true } } });
});
});

test("store.filter should pass adapterOptions to adapter.query", function(assert) {
assert.expect(2);

env.adapter.query = function(store, type, query, array, options) {
assert.ok(!('adapterOptions' in query));
assert.deepEqual(options.adapterOptions, { query: { embed: true } });
return { data: [] };
};

return run(() => {
return store.filter('person', {}, () => {}, { adapterOptions: { query: { embed: true } } });
});
});

test("store.queryRecord should pass adapterOptions to adapter.queryRecord", function(assert) {
assert.expect(2);

env.adapter.queryRecord = function(store, type, query, snapshot) {
assert.ok(!('adapterOptions' in query));
assert.deepEqual(snapshot.adapterOptions, { query: { embed: true } });
return { data: { type: 'person', id: 1, attributes: {} } };
};

return run(() => {
return store.queryRecord('person', {}, { adapterOptions: { query: { embed: true } } });
});
});

test("store.findRecord should pass 'include' to adapter.findRecord", function(assert) {
assert.expect(1);

Expand Down
4 changes: 2 additions & 2 deletions tests/integration/record-arrays/adapter-populated-record-array-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ test('pass record array to adapter.query based on arity', function(assert) {

return store.query('person', { }).then(recordArray => {
env.adapter.query = function(store, type, query, _recordArray) {
assert.equal(arguments.length, 4);
assert.equal(arguments.length, 5);
return payload;
};
return store.query('person', { });
Expand Down Expand Up @@ -218,7 +218,7 @@ test('pass record array to adapter.query based on arity', function(assert) {

return store.query('person', actualQuery).then(recordArray => {
env.adapter.query = function(store, type, query, _recordArray) {
assert.equal(arguments.length, 4);
assert.equal(arguments.length, 5);
return payload;
};

Expand Down