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

Fixing arrays in get params #49

Merged
merged 3 commits into from
Jul 23, 2015
Merged

Fixing arrays in get params #49

merged 3 commits into from
Jul 23, 2015

Conversation

maxhoffmann
Copy link

I had an issue sending arrays via GET params to a Rails server. In the current implementation the key in the params does not contain square brackets so that the server doesn’t know that it should handle the incoming params as an array. It only takes the last one.

There already is a test that checks for the implementation without square brackets. Is this defined as a standard somewhere? I researched on some sites and apparently PHP and Ruby’s Rack only support the square bracket notation. As transformRequest does only work for data and also not for GET requests I can’t override this functionality via the axios API. It seems that this has to be handled internally.

I’m proposing to change the current implementation so that square brackets are used for array params in GET requests. What do you think?

@maxhoffmann
Copy link
Author

Any opinion on this one?

@mzabriskie
Copy link
Member

I'm doing some research to determine if there is a standard.

@maxhoffmann
Copy link
Author

Cool, thanks. 👍

@maxhoffmann
Copy link
Author

Any update on this? I’m currently using my fixed version directly from github. Would be nice to merge this in. :)

@maxhoffmann
Copy link
Author

According to my research Rails and PHP only support the bracket notation.

@mzabriskie
Copy link
Member

I think that bracket notation is the correct implementation. It looks like rails, express, php, and java support it. My only hangup is if there should be an option for using one vs the other. This change won't be backward compatible for anyone depending on ?foo=bar&foo=baz.

@maxhoffmann
Copy link
Author

I could add an option so that people can enable it if the change breaks something. Although I’d say increasing the major version and adding a line to changelog should be sufficient.

In case you’d add an option: how would you name and add the config parameter?

axios({
  disableGetBracketNotation: true
});

I think bracket notation should be the default, so the option should fall back to the old behavior.

@mzabriskie
Copy link
Member

I've been thinking and it may be better instead of just using a boolean flag, allow specifying a function for how to handle params. This would accommodate any variety of patterns: foo[]=bar&foo[]=baz, foo=bar&foo=baz, foo=bar,baz, etc.

There are a couple options for doing this.

  1. Allow developer to provide a buildUrl method in the config. This would replace the internal buildUrl method if supplied. This could be done per request, or set as the default axios.defaults.buildUrl = function (url, params) { ... };
  2. Allow providing a method that just builds out the params, which would be used by buildUrl. Something like axios.defaults.processParams = function (params) { ... }; This would allow re-using more of the core logic, but isn't as flexible.

@maxhoffmann
Copy link
Author

Yep, passing a function sounds better. So would you move buildUrl to defaults.js and make it a config option like transformRequest?

@mzabriskie
Copy link
Member

Yeah, that's basically what I'm thinking. The thing I'm debating is that there's a bit of other logic in buildUrl that it may be nice to preserve. Specifically the encode function keeps the params looking nice.

Maybe buildUrl could take encode as an argument. Then implementing your own buildUrl could still leverage this functionality. Let me play around with this a bit, and see what makes sense.

@webbushka
Copy link

Any decisions made on this?

@renfredxh
Copy link

I'm using a Python backend and ran into this issue. An extensible buildUrl function would be nice, but I think the bracket notation is such a wide use case that it would be useful to have a boolean flag option for it. jQuery's $.get enables bracket encoding by default and many people are used to having it available.

@mzabriskie
Copy link
Member

I have a branch that supports providing your own buildUrl, but I don't love it. Another alternative would be to support params as a string, then you could use qs, or whatever you want, to format params how your backend expects. See #72

@AndrewRayCode
Copy link

Also hoping for a fix. An ajax library that incorrectly serializes an array is not good!

FYI express's req.query also expects the key[]=val notation

@AndrewRayCode
Copy link

For anyone hitting this, I made a wrapper around axios that is 100% untested and definitely only suits the bare minimum of my needs, but it addresses this issue.

It assumes a function where you pass in params (query string params) as an object, and the method, and the url

//  see https://github.com/mzabriskie/axios/pull/49
if( method.toLowerCase() === 'get' ) {
    for( let key in params ) {
        if( _.isArray( params[ key ] ) ) {
            url += '?' + key + '[]=' + _.map( params[ key ], encodeURIComponent ).join( '&' + key + '[]=' );
            delete params[ key ];
        }
    }
}

@mzabriskie
Copy link
Member

Sorry for the delay on this. A little background on the current implementation. The original version of axios was meant to provide an API directly compatible with Angular's $http service. The way that axios is sending Array params is how Angular is doing it.

In the time since, axios has diverged in several ways. I was initially pretty set on not breaking backwards compatibility, but after stewing on it, and seeing that most (all?) popular backends support [] for array params, I think it's best to just make a breaking change, and bump the version.

@mzabriskie
Copy link
Member

@maxhoffmann I would like to merge this. Can you make a couple quick changes?

lib/helpers/buildUrl.js:12
Add the following lines at the end of the encode function:

  replace(/%5B/gi, '[').
  replace(/%5D/gi, ']');

test/specs/helpers/buildUrl.spec.js:33
Change the expect to the following:

  toEqual('/foo?foo[]=bar&foo[]=baz');

This will make the URLs look a bit nicer. Thanks!

@maxhoffmann
Copy link
Author

Sure, changes are done. Looking forward to remove my fork from our package.json. 👍

mzabriskie added a commit that referenced this pull request Jul 23, 2015
Fixing arrays in get params
@mzabriskie mzabriskie merged commit 3b10b6a into axios:master Jul 23, 2015
@maxhoffmann
Copy link
Author

Thanks for merging! When is the next release?

@iam4x
Copy link

iam4x commented Jul 28, 2015

bump, waiting on this too 👍

@liverbool
Copy link

👍

@maxhoffmann
Copy link
Author

any plans for bumping the version number?

@mzabriskie
Copy link
Member

@maxhoffmann sorry for the long delay on this. I just released 0.6.0 which includes your PR.

@axios axios locked and limited conversation to collaborators May 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants