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

Add queryOne method #2584

Merged
merged 1 commit into from
Jun 14, 2015
Merged

Add queryOne method #2584

merged 1 commit into from
Jun 14, 2015

Conversation

thaume
Copy link

@thaume thaume commented Dec 12, 2014

The findQueryOne method let developers query their server-side API for a single record and also pass a query parameter to this call.

For instance, you could do the following:

this.store.findQueryOne('post', 1, { include: 'comments' });

@thaume thaume changed the title Add query one method Add queryOne method Dec 12, 2014
@thaume
Copy link
Author

thaume commented Dec 12, 2014

I still need to write some tests, but there are quite a few to write so I just want to be sure this is the way we want to implement findQueryOne. I was wondering if we should always fetch fresh data on a findQueryOne or should we first search in the store for the record's id?

@igorT
Copy link
Member

igorT commented Dec 12, 2014

We should always fetch fresh data, same as findQuery does.

@bmac
Copy link
Member

bmac commented Dec 12, 2014

I think it would be useful for this could be made to work without requiring an id to solve issues like #2558.

@thaume
Copy link
Author

thaume commented Dec 13, 2014

@igorT OK cool that's what I thought

@bmac you mean a findQuery that would resolve to a promiseObject instead of promiseArray?

@thaume
Copy link
Author

thaume commented Dec 18, 2014

@igorT just pushed some tests, let me know what you think

@igorT
Copy link
Member

igorT commented Jan 6, 2015

@thaume sorry for not getting back to you sooner. Can you please rebase? I think this looks great for now. I hope we can figure out a way in the future to also allow easy passing of a custom URL and not just options.

@thaume thaume force-pushed the addQueryOneMethod branch 2 times, most recently from 93d4787 to f8e6dbf Compare January 6, 2015 07:33
@thaume
Copy link
Author

thaume commented Jan 6, 2015

@igorT no problem man, the rebase is done! Let me know if you want me to open another PR to keep working on it or if it can wait!

@thaume thaume force-pushed the addQueryOneMethod branch 2 times, most recently from 21be80a to 5758e7a Compare January 16, 2015 10:56
@wecc
Copy link
Contributor

wecc commented Jan 22, 2015

This should solve #1576

@abobwhite
Copy link

#1326, which is closed, will also be solved by this pull request.

@bakura10
Copy link
Contributor

+1 on this feature. This is useful for cases like getting something by a slug instead of id (the slug is considered unique in some cases, so it's like getting by id, but needs to use a query params). Having said that, it's not really hard to workaround this.

However, it seems that in the extractQueryOne, you assume that there is only one resource, however I think it should call extractArray instead of extractSingle. The reason is that while you EXPECT only one record from your server, you are still hitting a "collection URI", not a single item URI.

@bmac bmac added this to the 1.0.0-beta.15 milestone Feb 10, 2015
@thaume
Copy link
Author

thaume commented Feb 10, 2015

@bakura10 I don't think you'd use this method to get records through slugs, since you need to use an id to fetch.
What do you mean by "hitting a "collection URI", not a single item URI"? I think that fetchQueryOne is actually supposed to get a PromiseObject instead of a PromiseArray. Since you have to use an id, it will always return a single record.
The use case is more to add queryParams to load relationships server-side than to filter a record that you are fetching with an id (so it is unique).
What do you think?

@bakura10
Copy link
Contributor

What I mean by collection URI is "/users", while "item URI" is "/users/5". "/users" is expected to return a collection, so "/users?first_name=foo" is expected to return a collection too, as well as "/users?slug=bar", but in my case, as slug are unique, I can guarantee myself that I will return at most 1 element, but as this is still a collection URI, I return it as a collection (so wrapped with a "users" in plural). It would feel very strange for me to have a collection URI return things that is not a collection.

@thaume
Copy link
Author

thaume commented Feb 10, 2015

Yes totally agree with you, but fetchQueryOne will issue a request of the
form "users/21?rel=posts" where you'll get the user with id=21 and you also
will attach all its posts.
I think you might want to try the store.query('users', { slug: bar }) to
issue "users?slug=bar" and get a collection of records.

Le mar. 10 févr. 2015 22:53, Michaël Gallego notifications@github.com a
écrit :

What I mean by collection URI is "/users", while "item URI" is "/users/5".
"/users" is expected to return a collection, so "/users?first_name=foo" is
expected to return a collection too, as well as "/users?slug=bar", but in
my case, as slug are unique, I can guarantee myself that I will return at
most 1 element, but as this is still a collection URI, I return it as a
collection (so wrapped with a "users" in plural). It would feel very
strange for me to have a collection URI return things that is not a
collection.


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

@bakura10
Copy link
Contributor

Haaa... Ok, I misunderstood the issue then. I thought it was actually made for use cases like a query is expected to return only one result. Sorry sorry, definitely a good idea then, I see the use case now! :)

@param {any} query an opaque query to be used by the adapter
@return {Promise} promise
*/
findQueryOne: function(typeName, id, query) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favor of renaming the store api to fetchQueryOne since it looks like this will always fetch a new record. Keeping the adapter API as findQueryOne seems fine and consistent with the other adapter methods.

Copy link
Author

Choose a reason for hiding this comment

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

Then you would also rename findQuery -> fetchQuery ?

@wecc
Copy link
Contributor

wecc commented Feb 11, 2015

Relevant: emberjs/rfcs#27

@bmac
Copy link
Member

bmac commented Feb 12, 2015

Marked this for discussion at tomorrow's Ember Data meeting.

@@ -745,7 +745,7 @@ export default Ember.Object.extend({
call is made to `DS.Store#findQuery`. By default this method is an
alias for [extractArray](#method_extractArray).

@method extractFindQuery
@method extractFindQueryOne
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should stay as extractFindQuery

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! I Guess I mixed names while changing from findQuery to findQueryOne. Thanks :)

@thaume
Copy link
Author

thaume commented Jun 11, 2015

@igorT juste need to squash and we're good to go

@thaume
Copy link
Author

thaume commented Jun 11, 2015

@igorT commits squashed ! Thanks for the review !

@bmac bmac mentioned this pull request Jun 13, 2015
13 tasks
}));
});

ok(!store.hasRecordForId('person', 1), "The record has been unloaded");
Copy link
Member

Choose a reason for hiding this comment

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

Why is this id 1?

Copy link
Author

Choose a reason for hiding this comment

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

Yep that was an old test from the "fetchQueryOne" method which worked with an id, removing the test

@thaume
Copy link
Author

thaume commented Jun 13, 2015

@bmac @igorT changes have been made

igorT added a commit that referenced this pull request Jun 14, 2015
@igorT igorT merged commit c29b8fd into emberjs:master Jun 14, 2015
@igorT
Copy link
Member

igorT commented Jun 14, 2015

Thanks!

@bmac bmac mentioned this pull request Jun 16, 2015
@allthesignals
Copy link
Contributor

I'm confused. I thought queryRecord mentioned here enables something like this.store.findQueryOne('post', 1, { include: 'comments' });. Am I misunderstanding? This function builds to /posts?1.

(For context, my question stems from #3454).

Thanks!

@DevBlaaast
Copy link

That was the old implementation. Now you cannot pass an id to queryRecord,
only a query object : queryRecord('post', { query: { slug: 'my-post' } });
On Sep 3, 2015 23:51, "Matt Gardner" notifications@github.com wrote:

I'm confused. I thought queryRecord mentioned here
http://emberjs.com/blog/2015/06/18/ember-data-1-13-released.html#toc_query-and-queryrecord
enables something like this.store.findQueryOne('post', 1, { include:
'comments' });. Am I misunderstanding? This function builds to /posts?1.

(For context, my question stems from #3454
#3454).

Thanks!


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

@Fryie
Copy link

Fryie commented Sep 28, 2015

this must be the most confusing PR i have ever seen! Can somebody clarify how to make what @allthesignals is asking for is supposed to work?

@wecc
Copy link
Contributor

wecc commented Sep 28, 2015

@Fryie feel free to ping me in the Ember Community Slack, happy to help

@hunterboerner
Copy link

@wecc what's the solution you came up with?

@igorT
Copy link
Member

igorT commented Dec 21, 2015

The conversation which helped @hunterboerner for the record

wecc [3:03 AM] 
@hunterboerner: `queryOne` became `queryRecord`, kind of. The idea is that you shouldn't need an ID to fetch a single record, like if you only have a slug: `store.queryRecord('post', { slug: 'my-post' })`(edited)

​[3:03] 
by default this would give you `/posts?slug=my-post` iirc

​[3:04] 
`store.findRecord('post', 123)` on the other hand accepts an ID and would give you the URL `/posts/123` by default

​[3:05] 
so... what it kind of boils down to is "How do I pass query params to `findRecord`" and/or "How do I pass the ID to `queryRecord`"(edited)

​[3:07] 
> How do I pass query params to `findRecord`?
You can use the `adapterOptions` to pass additional information to your adapter, and pass a query hash in there. Here's a very simple implementation of this: https://github.com/emberjs/data/issues/3596#issuecomment-126604014(edited)

wecc [3:12 AM] 
> How do I pass the ID to `queryRecord`?
Since `queryRecord` only accepts `modelName` and `query` you would have to add `id` to the `query` hash and then customize the `urlForQueryRecord` hook to add the ID to the URL (and possibly remove the ID from the query in the `queryRecord` hook)

@terion-name
Copy link

@igorT why is all this so complicated?
It is a such common use case, it is a part of json-api spec, so many requests for this and still no straight way to add query parameters to 'findRecord'. Why?

@jeffjarchow
Copy link

I agree. This is a very simple use case. There is no way I want to send a request like
/posts?id=1&include=replies when I should be able to send /posts/1?include=replies. I believe this is the problem that is trying to be solved here with still no solution. It is necessary to be able to send the id AND query params in a single request.

When you are dealing with a database that has hundreds of millions of records, your 10 ms query could turn into a few seconds. From an API side, most programmers don't look to see if the id is one of the many possible query params. That means if your default query limit for a GET list is 20 records, the DB Engine will search for a 'like' match to the id query param until it a) finds its 20 matches or b) reaches the end of the table. Where as a query looking for a single record sets limit to 1 and returns an exact match as soon as it finds the indexed record. Big performance difference on the db.

Luckily for me I have control over the API that I am connecting to with Ember.

@terion-name
Copy link

@jeffjarchow I now use a solution provied by @igorT:

export default DS.JSONAPIAdapter.extend({
  urlForFindRecord(id, modelName, snapshot) {
    let url = this._super(...arguments);
    let query = Ember.get(snapshot, 'adapterOptions.query');
    if (query) {
      url += '?' + Ember.$.param(query); // assumes no query params are present already
    }
    return url;
  }
});

this.store.findRecord('project', params.project_id, {adapterOptions: {query: {include: 'user,developer', limit: 1}}});

This works, but so hacky :(

@jeffjarchow
Copy link

Thank you @terion-name for the snippet. I'll have to play with that to see if I can get that working. I would just be afraid of it not being compatible as things change. Currently I am on a major upgrade from 1.10 to 1.13 and all the changes that go along with that, like eliminating views.

@EarvinKayonga
Copy link

@terion-name, U rock.

@Subtletree
Copy link

For anyone else finding this, since Ember 2.4 it's built in: ds-finder-include

var article = this.store.findRecord('article', 1, { include: 'comments' });

@runspired runspired added 🏷️ feat This PR introduces a new feature and removed Feature labels Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ feat This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet