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

Predefined query URLs difficult in RESTAdapter #2834

Closed
jamesarosen opened this issue Mar 6, 2015 · 14 comments
Closed

Predefined query URLs difficult in RESTAdapter #2834

jamesarosen opened this issue Mar 6, 2015 · 14 comments

Comments

@jamesarosen
Copy link

I have a model, User. In addition to supporting things like GET /users and GET /users?page=1, my server has some predefined optimized filters like /users/active, /users/inactive, and /users/recently-created.

I'm trying to write a UserAdapter that supports this, but I'm having trouble.

Attempt 1

My first instinct was to define buildURL, but that function doesn't get the query. RESTAdapter.prototype.findQuery looks like this:

findQuery: function(store, type, query) {
  if (this.sortQueryParams) {
    query = this.sortQueryParams(query);
  }
  return this.ajax(this.buildURL(type.typeKey), 'GET', { data: query });
}

That is, it just tacks the query on to the all URL as query parameters.

Attempt 2

My second idea was to override findQuery itself. Its surface area is fairly small, so that would be pretty easy:

findQuery: function(store, type, query) {
  if (this.sortQueryParams) {
    query = this.sortQueryParams(query);
  }
  var url = this.buildURL(type.typeKey);
  if (query.subset) {
    url += '/' + subset;
    delete query.subset;
  }
  return this.ajax(url, 'GET', { data: query });
}

This works -- in theory. The problem is that findQuery is private, which means I'm not allowed to override it in my adapter.

Recommendation

Either

  1. change the API of buildURL to buildURL(type, id, record, query)
  2. make findQuery protected or public
@HamptonMakes
Copy link

Agreed that this would be awesome. I tend to deal with these not as cleanly... by using pushPayload and an Ajax request that I manage. ;P

@jamesarosen
Copy link
Author

@hcatlin thanks for the suggestion of a workaround! I'll use that for now :)

@HamptonMakes
Copy link

Actually, I have discovered a much, much easier way to do this. In Ember Data, the relationship between the query and the response is pretty loose. That is, the RESTSerializer will deal with a lot of different types of data coming back... plurals, singulars... (almost) whatever!

I just realized today, that if I define /projects/current on my API, then issue this.store.find('project', 'current');, the store will happily hit the path above, and then deal with a non-matching id coming back. I'm using it for singulars, but after looking at how RESTSerializer works, it seems that it should be just fine with a list of projects coming back.

I don't think you have to do any magic overriding to get this working

Let me know if that works!

@HamptonMakes
Copy link

And, shit, I think I was wrong. sigh.

@hjdivad
Copy link
Member

hjdivad commented Mar 19, 2015

FWIW I'm also overriding findQuery. It's an obvious place to support these kinds of queries; it makes sense to me for this (or perhaps something similar) to be public.

@wecc
Copy link
Contributor

wecc commented Jun 2, 2015

@jamesarosen @hcatlin @hjdivad there's been work done in the BuildURLMixin that I suspect would make this a lot easier. Would any of you be able to confirm if the query sent to buildURL() or alternatively override urlForFindQuery() solves this?

@hjdivad
Copy link
Member

hjdivad commented Jun 2, 2015

@wecc what's the reason for making this a mixin rather than having it in the base adapter?

@wecc
Copy link
Contributor

wecc commented Jun 2, 2015

@hjdivad an adapter doesn't necessarily have to talk to it's storage using AJAX

@hjdivad
Copy link
Member

hjdivad commented Jun 3, 2015

@wecc makes sense; still, if the app adapter is already extending DS.ActiveModelAdapter the mixin is an implementation detail app authors don't need to know about right?

@wecc
Copy link
Contributor

wecc commented Jun 3, 2015

@hjdivad confirm

@wecc
Copy link
Contributor

wecc commented Sep 16, 2015

@jamesarosen @hcatlin @hjdivad did overriding urlForQueryRecord work? can this issue be closed?

@hjdivad
Copy link
Member

hjdivad commented Sep 16, 2015

@wecc can't say, still on an older version of ember-data, I'm afraid.

@vanor89
Copy link

vanor89 commented Dec 2, 2015

Overwriting the urlForFindRecord worked for me ie:

return `${namespace}/collection/${id}?${includeItemParam}&${includeLastModifiedUserParam}`; 

@fivetanley
Copy link
Member

Seems to be satisfied by the new buildURL hooks @wecc linked to above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants