[BUGFIX beta] Overhaul queryRecord by pangratz · Pull Request #4300 · warp-drive-data/warp-drive

pangratz · 2016-04-05T22:07:16Z

Currently the use case for store.queryRecord is not communicated very
well and there has been some confusion on what the expected server
response should look like (array, or an object).

There are several issues with the current code base:

in general, if the serializer returns an array for the primary data
returned by normalizeQueryRecordResponse, then store.queryRecord
resolves with an array
if rest serializer is used and an array is returned by the adapter,
then the first entry of the array is used as primary data and
store.queryRecord resolves with the first record
if json-api serializer is used and the primary data is an array, then
store.queryRecord resolves with an array of the primary records
the API documentation for queryRecord is similar to query and
doesn't indicate when this method should be used in contrast to
store.query and getting the first record of the array
an assertion for the payload returned by the adapter for queryRecord
not being an empty object has been added in 2.4, which is a regression
to the behavior of 2.3

This commit addresses the above issues and makes the following changes:

add assertion that store.queryRecord never resolves with an array
add deprecation warning for the rest-serializers' queryRecord, if an
array is returned instead of a single record
removes the assertion that the returned payload from the adapter for
queryRecord is not an empty object
add assertion within json-api serializer that the primary data of the
normalized response is not an array

I am not so sure about the deprecation for the rest-serializer, since it looks like currently also findRecord works when an array is returned (see this test). But since json-api-adapter doesn't allow data: [...] responses for queryRecord, this seems like an inconsistency which is a little surprising IMHO ¯\_(ツ)_/¯ .

This addresses issues raised in #3977, #4227 and #4255.

sly7-7 · 2016-04-06T06:54:40Z

addon/-private/system/store.js

+    The request is made through the adapters' `queryRecord`:
+
+    ```javascript
+    // app/adapters/application.js


maybe pointing to app/adapters/user.js would be better here ?

👍 You're absolutely right!

pangratz · 2016-04-06T07:50:35Z

Thanks for the feedback @sly7-7!

sly7-7 · 2016-04-06T08:48:42Z

I think this is a good answer to related issues, disambiguate, and making the things consistent, I'm all for it

pangratz · 2016-04-14T06:45:07Z

This has been discussed in the team meeting and overall the taken approach looks good.

I will take a look at #4310 to see if it can be addressed with this PR as well.

bmac · 2016-04-15T00:07:17Z

addon/serializers/json-api.js

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


What are your thoughts on doing this in normalizeQueryRecordResponse?

pangratz · 2016-04-19T12:15:43Z

I've updated this PR to address the following issues:

put assertion for queryRecord being an array in the json-api serializer from _normalizeResponse to normalizeQueryRecordResponse, which is a better place as pointed out by @bmac 👍
remove assertion for payload not being empty in the _queryRecord method of finder.js. This assertion (introduced in Store._find asserts adapterPayload not empty #3916) caused a regression which has been reported in Can't return null to queryRecord anymore #4310. Removing the assertion allows to return an empty object again, when the rest adapter is used. Tests for store.queryRecord using the rest and json-api adapter have been added, so the expected behavior for those methods are tested and future regressions are caught.
updated the documentation for store.queryRecord to point out the behavior: the returned promise resolves with either the found record or null when the adapter returned nothing for the primary data

taras · 2016-04-25T18:25:52Z

Just to clarify the clarification, does this PR ensure that queryRecord always returns a single object and not an array?

pangratz · 2016-04-25T18:58:03Z

Just to clarify the clarification, does this PR ensure that queryRecord always returns a single object and not an array?

@taras yes, store.queryRecord will always resolve with a single record. Is that what you meant?

taras · 2016-04-25T19:01:32Z

Cool, thank you :)

bmac · 2016-05-01T17:32:33Z

addon/serializers/json-api.js

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


Instead of wrapping this in an if block should we replace false with !Array.isArray(normalized.data) so it gets stripped correctly is a prod build?

Absolutely! Don't know why I chose the if here 😕

Currently the use case for `store.queryRecord` is not communicated very well and there has been some confusion on what the expected server response should look like (array, or an object). There are several issues with the current code base: - in general, if the serializer returns an array for the primary data returned by `normalizeQueryRecordResponse`, then `store.queryRecord` resolves with an array - if rest serializer is used and an array is returned by the adapter, then the first entry of the array is used as primary data and `store.queryRecord` resolves with the first record - if json-api serializer is used and the primary data is an array, then `store.queryRecord` resolves with an array of the primary records - the API documentation for `queryRecord` is similar to `query` and doesn't indicate when this method should be used in contrast to `store.query` and getting the first record of the array - an assertion for the payload returned by the adapter for `queryRecord` not being an empty object has been added in 2.4, which is a regression to the behavior of 2.3 ----------------------------------------------------------------------- This commit addresses the above issues and makes the following changes: - add assertion that `store.queryRecord` never resolves with an array - add deprecation warning for the rest-serializers' `queryRecord`, if an array is returned instead of a single record - removes the assertion that the returned payload from the adapter for `queryRecord` is not an empty object - add assertion within json-api serializer that the primary data of the normalized response is not an array

bmac · 2016-05-03T02:09:22Z

Thanks @pangratz

pangratz added the team-review label Apr 5, 2016

sly7-7 reviewed Apr 6, 2016
View reviewed changes

pangratz removed the team-review label Apr 14, 2016

pangratz mentioned this pull request Apr 14, 2016

Can't return null to queryRecord anymore #4310

Closed

bmac reviewed Apr 15, 2016
View reviewed changes

bmac self-assigned this Apr 28, 2016

bmac reviewed May 1, 2016
View reviewed changes

bmac merged commit 581781c into warp-drive-data:master May 3, 2016

pangratz deleted the overhaul-queryRecord branch May 3, 2016 08:15

This was referenced May 4, 2016

queryRecord doesn't seem to return a fullly hydrated model #4227

Closed

queryRecord returns array, expect single record #4255

Closed

This was referenced May 16, 2016

queryRecord throws when the response is empty #3890

Closed

[JSON API] Can't use queryRecord for single resource #3596

Closed

MarkMT mentioned this pull request Oct 7, 2016

Update description of queryRecord() emberjs/guides#1689

Merged

dweremeichik mentioned this pull request Oct 18, 2016

Fixes queryRecord issue #148 pouchdb-community/ember-pouch#152

Merged

MarkMT mentioned this pull request Nov 19, 2016

"Finding Records" inconsistent emberjs/guides#1717

Open

Conversation

pangratz commented Apr 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sly7-7 Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

pangratz Apr 6, 2016

Choose a reason for hiding this comment

Uh oh!

pangratz commented Apr 6, 2016

Uh oh!

sly7-7 commented Apr 6, 2016

Uh oh!

pangratz commented Apr 14, 2016

Uh oh!

bmac Apr 15, 2016

Choose a reason for hiding this comment

Uh oh!

pangratz commented Apr 19, 2016

Uh oh!

taras commented Apr 25, 2016

Uh oh!

pangratz commented Apr 25, 2016

Uh oh!

taras commented Apr 25, 2016

Uh oh!

bmac May 1, 2016

Choose a reason for hiding this comment

Uh oh!

pangratz May 2, 2016

Choose a reason for hiding this comment

Uh oh!

bmac commented May 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pangratz commented Apr 5, 2016 •

edited

Loading