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

query keys in query string #19

Closed
Dhaulagiri opened this issue Apr 7, 2016 · 11 comments
Closed

query keys in query string #19

Dhaulagiri opened this issue Apr 7, 2016 · 11 comments
Labels

Comments

@Dhaulagiri
Copy link

The docs for query show how to include just certain keys in a request, but in my use I am still seeing the query keys being included in the query string. For example:

// example request and template
this.store.query('app', { 'appName': 'foo' })
queryUrlTemplate: '{+host}/apps/{appName}/app'

Yields this url:

https://foo.com/apps/foo/app?appName=foo

When I really want just this:

https://foo.com/apps/foo/app

Are the docs incorrect or is there something I'm doing incorrectly here?

@amiel
Copy link
Owner

amiel commented Apr 7, 2016

@Dhaulagiri Yes, this is an issue. Unfortunately, ember-data appends the query string after the url is built with buildURL.

I kind of think this is a bug in ember-data but would like to check with the core team and find out if this is intentional.

There is a workaround. The idea is to make sure the query object ends up being empty. You can do this by deleting appName from the query object in a urlSegment. For example:

// adapter
export default ApplicationAdapter.extend(UrlTemplates, {
  queryUrlTemplate: '{+host}/apps/{appName}/app'

  urlSegments: {
    appName(type, id, snapshot, query) {
      var appName = query.appName;
      delete query.appName;
      return appName;
    },
  },
});

@amiel
Copy link
Owner

amiel commented Apr 7, 2016

@igorT do you have any thoughts on if it makes sense that the default adapter is appending the query string to the result from buildURL or if that should only happen in buildURL?

@cfator
Copy link

cfator commented May 3, 2016

I agree Ember.Data lets you construct the URL but not the query params which seems really strange.

I ended up using RESTAdapter.sortQueryParams to achieve this.

@amiel
Copy link
Owner

amiel commented May 4, 2016

@cfator oh, would you be willing to share your solution?

@kehphin
Copy link

kehphin commented May 23, 2016

I think this will work:

export default ApplicationAdapter.extend(UrlTemplates, {
  queryUrlTemplate: '{+host}/apps/{appName}/app',

  urlSegments: {
    appName(type, id, snapshot, query) {
      return query.appName;
    }
  },

  sortQueryParams: function(params) {
    return {};
  }
});

Keep in mind that you might be doing other developers a disservice when they try to add a query param to this code and are left pondering why it doesn't work... heh

@amiel
Copy link
Owner

amiel commented May 23, 2016

@kehphin Nice! I wonder if it would be worth including sortQueryParams in the UrlTemplate mixin.

Theoretically, it shouldn't be a problem. If one wants to include query params with url templates, they should define them as part of the template with queryUrlTemplate: '{+host}/apps/{appName}/app?{?query*}'.

However, I agree that this could be confusing and some people might even be depending on the existing functionality. My other concern here is that might only be compatible with a few versions of ember-data.

I'd love some input here.

@OandrewO
Copy link

@amiel : Did you find out if this was an intended behavior on the Ember Data teams' part? I was able to construct the correct endpoint by following the workarounds in this thread, but they seem very hacky.

@amiel
Copy link
Owner

amiel commented Sep 22, 2016

@OandrewO I have not heard from the Ember Data team on this, and I agree, the workarounds are hacky. I'll try pinging them again on slack.

@amiel
Copy link
Owner

amiel commented Sep 27, 2016

@OandrewO / others: It looks like emberjs/data#3099 will provide the hook we need to customize the query params (dataForRequest).

Once that gets un-feature flagged and I personally have an app that is caught up to that version, I will be happy to work on having ember-data-url-templates use that hook to prevent this issue.

If anyone else has an app up to canary and wants to work on it, please do :)

For now, looking through the source, using sortQueryParams seems like the best short-term solution.

@amiel amiel closed this as completed in e7a080b Sep 27, 2016
@amiel
Copy link
Owner

amiel commented Sep 27, 2016

Ok, I just released 0.2.0 with this hack.

With this change, you should be able to remove either hack (sortQueryParams, or the delete hack).

Note that if you were using sortQueryParams for another reason, or depend on the fact that the query params were already appended, this could cause unintended changes.

@misteroh
Copy link

awesome! thanks!!

@amiel amiel removed the help wanted label Sep 27, 2016
amiel added a commit that referenced this issue Oct 26, 2018
This way, we have a test that verifies that #19 continues to be resolved even after future changes.
amiel added a commit that referenced this issue Oct 26, 2018
This way, we have a test that verifies that #19 continues to be resolved even after future changes.
amiel added a commit that referenced this issue Oct 26, 2018
Add a test to verify the query params hack for #19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants