There is no good way to do findQuery with data binding to fetch just one record #551

Closed
darthdeus opened this Issue Dec 25, 2012 · 26 comments

Comments

Projects
None yet
10 participants
@darthdeus
Member

darthdeus commented Dec 25, 2012

If I have a view that looks like this

App.UserLinkView = Ember.View.extend({

  user: function() {
    return App.User.findByUsername(this.get("username"));
  }.property(),

  template: Ember.Handlebars.compile('<a {{action showUser view.user href=true}}>@{{view.username}}</a>')
});

and use it like this

{{view App.UserLinkView username="wycats"}}

there is no good way of implementing the findByUsername method. For example if I try to do

App.User.reopenClass({
  findByUsername: function(username) {
    return App.store.findQuery(App.User, { username: username });
  }
});

it won't work, becuase findQuery expects to receive multiple records, not just one. Which means the server implementation should be something like this

def index
  respond_with User.find_all_by_username(params[:username])
end

This won't work though, because the view is expecting just a single instance of a App.User model.

A hackish solution to this would be to do App.store.find(App.User, username) which works, but it seems that Ember Data will store the ID you use to search by, so if you later use the user in a route, it would look like /users/wycats instead of /users/1

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Dec 26, 2012

Member

I have something along these lines in my app:

App.UserController = Ember.ObjectController.extend({
  _content: null,

  content: function(key, value) {
    var content = this.get('_content');
    if (arguments.length === 2) {
      content = Ember.makeArray(value);
      this.set('_content', content);
    }
    return Ember.get(content, 'firstObject');
  }.property('_content.firstObject')
});

// both will work
App.UserController.set('content', App.User.find(1));
App.UserController.set('content', App.User.find({username: 'toto'}));

Handeling it on the controller level is probably the only way, as we could not return a model object without id... We would have no way to re-associate it later. Imagine you do :

var user1 = App.store.findQuery(App.User, { username: 'toto' });
var user2 = App.store.findQuery(App.User, { username: 'toto' });

How could you ensure user1 === user2 ?

Member

tchak commented Dec 26, 2012

I have something along these lines in my app:

App.UserController = Ember.ObjectController.extend({
  _content: null,

  content: function(key, value) {
    var content = this.get('_content');
    if (arguments.length === 2) {
      content = Ember.makeArray(value);
      this.set('_content', content);
    }
    return Ember.get(content, 'firstObject');
  }.property('_content.firstObject')
});

// both will work
App.UserController.set('content', App.User.find(1));
App.UserController.set('content', App.User.find({username: 'toto'}));

Handeling it on the controller level is probably the only way, as we could not return a model object without id... We would have no way to re-associate it later. Imagine you do :

var user1 = App.store.findQuery(App.User, { username: 'toto' });
var user2 = App.store.findQuery(App.User, { username: 'toto' });

How could you ensure user1 === user2 ?

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Dec 28, 2012

Member

@tchak The issue is that even if I return the ID, it will use the queried username as the ID, not the returned parameter with key "id".

Member

darthdeus commented Dec 28, 2012

@tchak The issue is that even if I return the ID, it will use the queried username as the ID, not the returned parameter with key "id".

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Dec 29, 2012

Member

I'm not sure I understand the implementation though. Why are you expecting key, value in the content. Shouldn't it be just value?

In the way I need to use this, I can't really rely on a controller, as I need to bind it in the view (the example in the issue). I'm not really sure how to do that.

Member

darthdeus commented Dec 29, 2012

I'm not sure I understand the implementation though. Why are you expecting key, value in the content. Shouldn't it be just value?

In the way I need to use this, I can't really rely on a controller, as I need to bind it in the view (the example in the issue). I'm not really sure how to do that.

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Dec 29, 2012

Member

Another issue that comes to mind is that if I fetch the user like you showed, it won't get cached. Which means another time when I need to find a user by the same username, it will do the same request again, because the identity map only works with IDs, right?

It would probably require a separate method like findOne which would take into consideration that the parameters which I'm using to search can be used to identify the record, so I if request a record with the same parameters again, it will just give me the locally stored one.

I'm not sure if that's possible to do performance wise though, but it might be possible easy do with one parameter.

Member

darthdeus commented Dec 29, 2012

Another issue that comes to mind is that if I fetch the user like you showed, it won't get cached. Which means another time when I need to find a user by the same username, it will do the same request again, because the identity map only works with IDs, right?

It would probably require a separate method like findOne which would take into consideration that the parameters which I'm using to search can be used to identify the record, so I if request a record with the same parameters again, it will just give me the locally stored one.

I'm not sure if that's possible to do performance wise though, but it might be possible easy do with one parameter.

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Dec 29, 2012

Member

@tchak Also I've been trying to reproduce the example you gave me, but really without any luck

Scvrush.UserLinkView = Ember.View.extend({
  tagName: "span",

  users: function() {
    return Scvrush.store.find(Scvrush.User, { username: this.get("username") });
  }.property("username"),

  user: function() {
    return this.get("users.firstObject");
  }.property("users.@each"),

  template: Ember.Handlebars.compile('<a {{action showUser view.user href=true}}>@{{view.username}}</a>')
});

and invoking the view as

{{view Scvrush.UserLinkView username="someone"}}

and I'm just getting undefined every time, both in users and user properties, even though the request is made and correct data returned.

Member

darthdeus commented Dec 29, 2012

@tchak Also I've been trying to reproduce the example you gave me, but really without any luck

Scvrush.UserLinkView = Ember.View.extend({
  tagName: "span",

  users: function() {
    return Scvrush.store.find(Scvrush.User, { username: this.get("username") });
  }.property("username"),

  user: function() {
    return this.get("users.firstObject");
  }.property("users.@each"),

  template: Ember.Handlebars.compile('<a {{action showUser view.user href=true}}>@{{view.username}}</a>')
});

and invoking the view as

{{view Scvrush.UserLinkView username="someone"}}

and I'm just getting undefined every time, both in users and user properties, even though the request is made and correct data returned.

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Dec 30, 2012

Member

Looks like my example was right all along, I just needed to re-render the view on the right time.

Scvrush.UserLinkView = Ember.View.extend({
  tagName: "span",

  users: function() {
    return Scvrush.store.find(Scvrush.User, { username: this.get("username") });
  }.property("username"),

  user: function() {
    return this.get("users.firstObject");
  }.property("users.@each"),

  usersChanged: function() {
    this.rerender();
  }.observes("users.@each"),

  template: Ember.Handlebars.compile('<a {{action showUser view.user href=true}}>@{{view.username}}</a>')
});

I'm not sure why that is required though, feels like a bug to me.

Member

darthdeus commented Dec 30, 2012

Looks like my example was right all along, I just needed to re-render the view on the right time.

Scvrush.UserLinkView = Ember.View.extend({
  tagName: "span",

  users: function() {
    return Scvrush.store.find(Scvrush.User, { username: this.get("username") });
  }.property("username"),

  user: function() {
    return this.get("users.firstObject");
  }.property("users.@each"),

  usersChanged: function() {
    this.rerender();
  }.observes("users.@each"),

  template: Ember.Handlebars.compile('<a {{action showUser view.user href=true}}>@{{view.username}}</a>')
});

I'm not sure why that is required though, feels like a bug to me.

@ambivalentno

This comment has been minimized.

Show comment
Hide comment
@ambivalentno

ambivalentno Jan 31, 2013

It would be really convenient for me to have something like App.Object.getOne function to use in routes (setting model for slug-based urls)

It would be really convenient for me to have something like App.Object.getOne function to use in routes (setting model for slug-based urls)

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Jan 31, 2013

Member

When #571 is fixed, we can easily do that by using this as the implementation

var users = App.User.findQuery({ username: username });

users.one("didLoad", function() {
  users.resolve(users.get("firstObject"));
});

return users;
Member

darthdeus commented Jan 31, 2013

When #571 is fixed, we can easily do that by using this as the implementation

var users = App.User.findQuery({ username: username });

users.one("didLoad", function() {
  users.resolve(users.get("firstObject"));
});

return users;
@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Mar 27, 2013

Member

This would be a great feature. The main issue is that when we're returning a single object, we need to update its contents, but we can't replace the whole thing. However, it's entirely possible that your findOne will return an object that's already in the store. In that case you'd end up with two records that have the same id but are not identical JS objects.

Member

wagenet commented Mar 27, 2013

This would be a great feature. The main issue is that when we're returning a single object, we need to update its contents, but we can't replace the whole thing. However, it's entirely possible that your findOne will return an object that's already in the store. In that case you'd end up with two records that have the same id but are not identical JS objects.

@darthdeus

This comment has been minimized.

Show comment
Hide comment
@darthdeus

darthdeus Mar 27, 2013

Member

What about returning a wrapper, something like AdapterPopulatedRecordArray, which would be representing possibly just one record. It would have loading, found and not_found states. If it finds a record that has the same id as one in identity map, it would load it from the identity map, if not, it would represent the new record. It should also be a deferrable which would make using it quite simple

Member

darthdeus commented Mar 27, 2013

What about returning a wrapper, something like AdapterPopulatedRecordArray, which would be representing possibly just one record. It would have loading, found and not_found states. If it finds a record that has the same id as one in identity map, it would load it from the identity map, if not, it would represent the new record. It should also be a deferrable which would make using it quite simple

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Mar 27, 2013

Member

@wagenet I really believe it should be handled on the controller level, like it was the case in SC. In SC you could assign an enumerable to the content of ObjectController and it would resolve to the firstObject as soon as it was available. I can work on a PR if the idea is accepted.

@darthdeus you reinventing ObjectController :)

Member

tchak commented Mar 27, 2013

@wagenet I really believe it should be handled on the controller level, like it was the case in SC. In SC you could assign an enumerable to the content of ObjectController and it would resolve to the firstObject as soon as it was available. I can work on a PR if the idea is accepted.

@darthdeus you reinventing ObjectController :)

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Sep 5, 2013

Member

This is easily accomplished with 1.0:

model: function(params) {
  return this.store.find('post', { id: params.post_id }).then(function(array) {
    return array[0];
  });
}
Member

wycats commented Sep 5, 2013

This is easily accomplished with 1.0:

model: function(params) {
  return this.store.find('post', { id: params.post_id }).then(function(array) {
    return array[0];
  });
}
@fusion2004

This comment has been minimized.

Show comment
Hide comment
@fusion2004

fusion2004 Jan 2, 2014

@wycats At least in 1.0.0-beta.4, it did not work properly by returning the 0 indexed element of the array variable.

This however, did work for me:

model: function(params) {
  return this.store.find('post', { id: params.post_id }).then(function(array) {
    return array.get('firstObject');
  });
}

@wycats At least in 1.0.0-beta.4, it did not work properly by returning the 0 indexed element of the array variable.

This however, did work for me:

model: function(params) {
  return this.store.find('post', { id: params.post_id }).then(function(array) {
    return array.get('firstObject');
  });
}
@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Feb 24, 2014

Contributor

Would be nice to have a wrapper for this, like findSingle, which takes a query object and expects the JSON in the singular form, e.g:

// this.store.findSingle('user', { username: 'j123' }) 
{
  "user": {
    "id": 1,
    "name": "John",
    "username": "j123"
  }
}

and would return a singular record.

Contributor

knownasilya commented Feb 24, 2014

Would be nice to have a wrapper for this, like findSingle, which takes a query object and expects the JSON in the singular form, e.g:

// this.store.findSingle('user', { username: 'j123' }) 
{
  "user": {
    "id": 1,
    "name": "John",
    "username": "j123"
  }
}

and would return a singular record.

@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Feb 25, 2014

Contributor

Maybe it could be findByQuery?

Contributor

knownasilya commented Feb 25, 2014

Maybe it could be findByQuery?

@ryanjm

This comment has been minimized.

Show comment
Hide comment
@ryanjm

ryanjm Jun 2, 2014

@knownasilya I agree that a findByQuery would be helpful. I'm needing to find a single query but pass additional parameters with it. Have you figured out a good solution for this yet?

find with a query option is expecting an array and my server is passing a single object (I'm using wycats solution here) so I'm not sure how to work around it right now.

ryanjm commented Jun 2, 2014

@knownasilya I agree that a findByQuery would be helpful. I'm needing to find a single query but pass additional parameters with it. Have you figured out a good solution for this yet?

find with a query option is expecting an array and my server is passing a single object (I'm using wycats solution here) so I'm not sure how to work around it right now.

@MartinElvar

This comment has been minimized.

Show comment
Hide comment
@MartinElvar

MartinElvar Jun 13, 2014

The problem with..

return this.store.find('post', { slug: params.slug }).then(function(array) {
  return array.get('firstObject');
});

Say i have visited the posts pages, which contains all the post, and i click on a post. Now the post has already been loaded on posts page, but it will make another request to the server, as a id was not provided to the find method.

this.store.find('post', 1)

Would search through the cache, before querying the server.

The problem with..

return this.store.find('post', { slug: params.slug }).then(function(array) {
  return array.get('firstObject');
});

Say i have visited the posts pages, which contains all the post, and i click on a post. Now the post has already been loaded on posts page, but it will make another request to the server, as a id was not provided to the find method.

this.store.find('post', 1)

Would search through the cache, before querying the server.

@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Jun 13, 2014

Contributor

@MartinElvar that's a very good point. So having a findByQuery or something similar could be optimized for the store.

Contributor

knownasilya commented Jun 13, 2014

@MartinElvar that's a very good point. So having a findByQuery or something similar could be optimized for the store.

@recipher

This comment has been minimized.

Show comment
Hide comment
@recipher

recipher Jun 13, 2014

+1

On 13 June 2014 15:09, Ilya Radchenko notifications@github.com wrote:

@MartinElvar https://github.com/MartinElvar that's a very good point.
So having a findByQuery or something similar could be optimized for the
store.


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

+1

On 13 June 2014 15:09, Ilya Radchenko notifications@github.com wrote:

@MartinElvar https://github.com/MartinElvar that's a very good point.
So having a findByQuery or something similar could be optimized for the
store.


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

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jun 25, 2014

Member

@knownasilya @MartinElvar How do you expect the store to know about the query? The whole point of findQuery is to always hit the server...

As for findByQuery returning single record, we could do it now that we have PromiseProxy. But it would be basically @wycats implementation wrapped in a PromisProxy. I am against a new method on the adapter for singular query, seems redundant, and if it just an api formatting problem, seems easy to handle with a custom adapter or serializer.

Member

tchak commented Jun 25, 2014

@knownasilya @MartinElvar How do you expect the store to know about the query? The whole point of findQuery is to always hit the server...

As for findByQuery returning single record, we could do it now that we have PromiseProxy. But it would be basically @wycats implementation wrapped in a PromisProxy. I am against a new method on the adapter for singular query, seems redundant, and if it just an api formatting problem, seems easy to handle with a custom adapter or serializer.

@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Jun 25, 2014

Contributor

@tchak I see what you mean. Only if the query was a composite key, then you could do something of the sort, otherwise the store can't possibly know that there aren't other records on the server that it doesn't posses.

So maybe it's a lack of composite key functionality in findById..

Contributor

knownasilya commented Jun 25, 2014

@tchak I see what you mean. Only if the query was a composite key, then you could do something of the sort, otherwise the store can't possibly know that there aren't other records on the server that it doesn't posses.

So maybe it's a lack of composite key functionality in findById..

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jul 3, 2014

Member

What you mean by composite key is something like "#{title}-#{date}" but where the uniciy is guaranteed only for full key ? If yes I would suggest implementing it on the adapter/serializer level by setting the id attr with composed key value.

Member

tchak commented Jul 3, 2014

What you mean by composite key is something like "#{title}-#{date}" but where the uniciy is guaranteed only for full key ? If yes I would suggest implementing it on the adapter/serializer level by setting the id attr with composed key value.

@knownasilya

This comment has been minimized.

Show comment
Hide comment
@knownasilya

knownasilya Jul 3, 2014

Contributor

That's a very good suggestion @tchak .

Contributor

knownasilya commented Jul 3, 2014

That's a very good suggestion @tchak .

@tchak

This comment has been minimized.

Show comment
Hide comment
@tchak

tchak Jul 3, 2014

Member

@knownasilya I am not sure how common this kind of pattern is. If it is common enough maybe we could provide a config option on the serializer. But in all cases this is the place where it belongs.

Member

tchak commented Jul 3, 2014

@knownasilya I am not sure how common this kind of pattern is. If it is common enough maybe we could provide a config option on the serializer. But in all cases this is the place where it belongs.

@MartinElvar

This comment has been minimized.

Show comment
Hide comment
@MartinElvar

MartinElvar Jul 14, 2014

@tchak I would say it's a very common use case, at least if you want to seo optimise your webpage, and "show" pages are accessed via slugs like /posts/ember-js-is-rocking-my-world; i can imagine many would like that.

@tchak I would say it's a very common use case, at least if you want to seo optimise your webpage, and "show" pages are accessed via slugs like /posts/ember-js-is-rocking-my-world; i can imagine many would like that.

@fusion2004

This comment has been minimized.

Show comment
Hide comment
@fusion2004

fusion2004 Jul 14, 2014

@MartinElvar @tchak That is definitely my use case for wanting this!

@MartinElvar @tchak That is definitely my use case for wanting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment