Querystring option #176

Merged
merged 8 commits into from Feb 21, 2012

Projects

None yet

2 participants

@csainty
Contributor
csainty commented Feb 18, 2012

#172

Took a stab at adding support for passing in a querystring option parameter.
First contribution to the project and I am still pretty new to Node, so happy to talk over any problems in code.

It got a bit interesting with oAuth, the oAuth code supports pulling its parameters from the body or the querystring, with the querystring taking preference with a basic truthy test. This looked wrong to me, but the change I have made would cause a change in functionality in certain circumstances.

Also included is a js based test runner if you want to point the npm test script at it. Running windows here, so needed something platform agnostic.

csainty added some commits Feb 18, 2012
@csainty csainty Added support for a "query" option value that is a hash of querystrin…
…g values that is merged (taking precedence over) with the querystring passed in the uri string.
2f34fd1
@csainty csainty Added a js based test runner so I can run tests on windows. a32d9e7
@csainty csainty Tidied up an issue where ?> was being appended to URLs.
Also fixed what appears to be a bug in oAuth where the querystring will trump the request body. If this is deliberate then perhaps check for entries in the querystring rather than just it's existence since it is always going to exist now.
e0b6ce0
@csainty csainty Refactored to match the composable style d47150d
@mikeal
Member
mikeal commented Feb 19, 2012

the query option should be named qs to match the composable API.

also, it's unsafe to set properties on self.uri because the underlying url parsing library in core is subject to change. it would be better to simply re-compose the url string with the new querystring and reset self.uri to a new url.parse(newurl).

oAuth is dark voodoo that nobody really understands. it's unclear to me how signing should work if both the querystring and form body are present (which is a totally legal HTTP request). i would just leave oAuth alone and once someone is trying to use both querystring arguments and form bodies we can see how different servers react and tweak as necessary.

@csainty csainty Renamed query to qs. It was actually my first choice, but there appea…
…red to be conflicts with the qs = require('querystring'). These are no longer present though and must have been unrelated.
0f24051
@csainty
Contributor
csainty commented Feb 19, 2012

I'll need to have a think about the self.uri side of things. I want to support appending qs {} values to an existing querystring that was part of the uri.
I'll take a look later tonight after I have a think on it. Spotted a bug as well that I will fix for the next update.

csainty added some commits Feb 19, 2012
@csainty csainty Changed test structure to no longer require a server, modeled on the …
…oauth tests. This also lets me revert some of the changes I had to make to the test server and proxy tests
becedaa
@csainty csainty Modified how the qs function works, it now no longer tweaks the exist…
…ing request uri, instead it recreates a new one. This allows me to revert all the other changes I had to make previously and gives a nice clean commit that is self contained.
9b2bbf0
@csainty
Contributor
csainty commented Feb 19, 2012

Ok I went away and thought about it a bit. The change is now self contained and much cleaner while still passing all tests.
The only piece I am unhappy with is where it formats then parses the uri object. It feels like too much work for simply merging querystrings.
At this stage I do not see a good work around other than removing the dependency on self.uri as an actual uri object and instead making it our own structure that can be tweaked without the double processing.

@mikeal
Member
mikeal commented Feb 20, 2012

the patch looks great now but i'm wondering what the run.js is?

we have a shell script that execs all the tests and can be executed using npm test.

@csainty
Contributor
csainty commented Feb 20, 2012

The current script is a bash script. I'm on windows here and it doesn't run. I can drop it from the commit if you like, but thought it might be of interest.

@mikeal
Member
mikeal commented Feb 20, 2012

good point.

i'll take it but please remove the bash script, change package.json to use this, and change it to running a static list of files because people tend to keep tests that aren't passing in that directory without checking them in.

@csainty csainty Switched npm test from the bash script to a node script so that it is…
… cross-platform.

Also now based off a static list of files so failing tests can be kept in the tests folder while being worked on.
c94b200
@csainty
Contributor
csainty commented Feb 20, 2012

Done.

Keep in mind this commit is now behind master a little and there are extra tests that will need to be added to static list in run.js.

Rebase etiquette is something I still am a little unsure of, I read conflicting things about whether I should rebase, merge or leave it to the project owner a this point.

@mikeal
Member
mikeal commented Feb 21, 2012

if it merged cleanly i won't rebase, i'll just merge it now and then add the other tests in myself.

@mikeal mikeal merged commit 10a5e23 into request:master Feb 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment