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

Safer argument parsing (attempt 2) #221

Merged
merged 1 commit into from Mar 25, 2015
Merged

Conversation

bobjflong
Copy link
Contributor

I couldn't stomach the merge conflicts in #220 (I was working with an old fork).

An issue I've seen crop up is that if a global key is set (which most of your docs use), and then client API keys are used (eg. if using OAuth), then unexpected results can happen. This includes loading customer data from the parent global key.

> require 'stripe'

> Stripe.api_key = "sk_test_foo"
=> "sk_test_foo"

> Stripe::Customer.all
# => loads customers from the global key

> Stripe::Customer.all({}, { api_key: nil })
# => ALSO loads customers from the global key!!

With my changes:

> Stripe::Customer.all({}, { api_key: nil })
TypeError: api_key must be a String

I've also added normalize_opts usage to a bunch of places. I'm sure not all of these are necessary. I've added tests for the static self.foo ones. It is a slight flaw I think that the current "normalization" of opts is spread across lots of resources, when it should be enforced centrally. I have some ideas about how to do that. But I think with a look over, this could be a good step forward and prevent some nasty errors.

@bobjflong bobjflong force-pushed the BL/nilkey2 branch 3 times, most recently from 454aab6 to 5af03b2 Compare March 7, 2015 14:14
@russelldavis
Copy link
Contributor

I couldn't stomach the merge conflicts in #220 (I was working with an old fork).

Ugh, that sucks. Github should really get with the times and show the diff as what the final merge will look like, a la Bitbucket

@russelldavis
Copy link
Contributor

I've also added normalize_opts usage to a bunch of places.

The request method already calls normalize_opts on what's passed in, so I think some of those additions are unnecessary. Not sure off hand about the other ones, but if you think they are, can you move them to a separate PR? (I like to keep each PR as focused as possible.)

@russelldavis
Copy link
Contributor

(Also, note that the Travis tests are currently failing.)

@bobjflong bobjflong force-pushed the BL/nilkey2 branch 2 times, most recently from 4da0903 to 257347d Compare March 7, 2015 18:15
@bobjflong
Copy link
Contributor Author

@russelldavis I backed out most of those normalize_opts.

I actually think that spec failing is flakiness in the spec itself. Ruby 1.8 doesn't preserve hash ordering so the resulting query string can be different. I've patched the spec here.

@@ -22,7 +23,7 @@ def self.retrieve(id=nil, opts={})
opts = id
id = nil
end
super(id, opts)
super(id, Util.normalize_opts(opts))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like another unnecessary call to normalize_opts, since super already calls it.

(After this, I think the PR will be ready to merge -- sorry for the delay.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, it's gone

russelldavis added a commit that referenced this pull request Mar 25, 2015
@russelldavis russelldavis merged commit bc3f514 into stripe:master Mar 25, 2015
@russelldavis
Copy link
Contributor

@bobjflong thanks for pushing this through!

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

Successfully merging this pull request may close these issues.

None yet

3 participants