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

Integer-index encode all arrays #674

Merged
merged 1 commit into from
Aug 15, 2018
Merged

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Aug 14, 2018

Changes all arrays from classic Rack encoding:

arr[]=...&arr[]=...&arr[]=...

To integer-indexed encoding:

arr[0]=...&arr[1]=...&arr[2]=...

We think that this should be tractable now that we've fully converted
all endpoints over to the new AbstractAPIMethod infrastructure on the
backend (although we should do a little more testing to make sure that
all endpoints still work).

As part of the conversion, we also remove any places that we were "spot
encoding" to get required integer-indexed syntax. This should now all be
built in.

cc @stevene-stripe @zanker-stripe

Changes all arrays from classic Rack encoding:

``` sh
arr[]=...&arr[]=...&arr[]=...
```

To integer-indexed encoding:

``` sh
arr[0]=...&arr[1]=...&arr[2]=...
```

We think that this should be tractable now that we've fully converted
all endpoints over to the new AbstractAPIMethod infrastructure on the
backend (although we should do a little more testing to make sure that
all endpoints still work).

As part of the conversion, we also remove any places that we were "spot
encoding" to get required integer-indexed syntax. This should now all be
built in.
# This method is invoked for any arrays being encoded and checks that if
# the array contains all non-empty maps, that each of those maps must start
# with the same key so that their boundaries can be properly encoded.
def self.check_array_of_maps_start_keys!(arr)
Copy link
Contributor

Choose a reason for hiding this comment

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

This method was designed to check that arrays being encoded were compatible with Rack syntax for arrays of maps — now that we're using integer-indexed arrays everywhere, we don't need to check this anymore.

Copy link

@zanker-stripe zanker-stripe left a comment

Choose a reason for hiding this comment

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

Seems sane to me, beyond 👍ing that we should re-verify the changes are working fine against the server.

@@ -33,39 +33,11 @@ class UtilTest < Test::Unit::TestCase
g: [],
}
assert_equal(
"a=3&b=%2Bfoo%3F&c=bar%26baz&d[a]=a&d[b]=b&e[]=0&e[]=1&f=",
"a=3&b=%2Bfoo%3F&c=bar%26baz&d[a]=a&d[b]=b&e[0]=0&e[1]=1&f=",

Choose a reason for hiding this comment

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

Do we have a test that verifies an array of hashes too? {foo: [{bar: :a}, {bar: :b}]} -> foo[0][bar]=a&foo[1][bar]=b?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah! Check just a bit lower in "#url_encode should prepare strings for HTTP", and it seems to do a pretty good job.

@zanker-stripe
Copy link

LGTM

@brandur-stripe
Copy link
Contributor

Okay, I tested a few places where we've used non-integer-index arrays:

  1. The expand parameter.
  2. attributes when creating a new product.
  3. types when listing events.

Numbers 1 and 2 worked fine, but number 3 didn't. Script:

require "stripe"
Stripe.api_key = "sk_test_BQokikJOvBiI2HlWgH4olfQ2"

events = Stripe::Event.list(
  limit: 10,
  type: ["invoice.created", "invoice.updated"],
)

events.each do |event|
  p event.type
end

And result:

$ STRIPE_LOG=debug bundle exec ruby event_list.rb
INFO Request to Stripe API method=get num_retries=0 path=/v1/events
DEBU Request details query_params="limit=10&type[0]=invoice.created&type[1]=invoice.updated"
INFO Response from Stripe API api_version=2018-07-27 elapsed=0.222724 method=get path=/v1/events request_id=req_syHFJh71e34U0y status=400
DEBU Response details body="{  \"error\": {    \"message\": \"Invalid string: [\\"invoice.created\\", \\"invoice.updated\\"]\",    \"param\": \"type\",    \"type\": \"invalid_request_error\"  }}" request_id=req_syHFJh71e34U0y
DEBU Dashboard link for request request_id=req_syHFJh71e34U0y url="https://dashboard.stripe.com/test/logs/req_syHFJh71e34U0y"
ERRO Stripe API error status=400 error_message="Invalid string: [\"invoice.created\", \"invoice.updated\"]" error_param=type error_type=invalid_request_error request_id=req_syHFJh71e34U0y
/Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:296:in `handle_error_response': Invalid string: ["invoice.created", "invoice.updated"] (Stripe::InvalidRequestError)
        from /Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:247:in `rescue in execute_request_with_rescues'
        from /Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:215:in `execute_request_with_rescues'
        from /Users/brandur/stripe/stripe-ruby/lib/stripe/stripe_client.rb:171:in `execute_request'
        from /Users/brandur/stripe/stripe-ruby/lib/stripe/api_operations/request.rb:19:in `request'
        from /Users/brandur/stripe/stripe-ruby/lib/stripe/api_operations/list.rb:9:in `list'
        from event_list.rb:4:in `<main>'

It looks like we encoded the parameter as expected, but the server balked at it — so we may still have a little more backend work to do before we can fully enable this.

@brandur-stripe
Copy link
Contributor

And nevermind — I used type instead of types (apparently they're both supported, but only types will take an array).

types works fine.

Okay, I'm pretty comfortable with this change (and therefore the one in Java as well) then.

@remi-stripe Anything else that you can think of?

brandur-stripe pushed a commit to stripe/stripe-go that referenced this pull request Aug 14, 2018
Changes all arrays from classic Rack encoding:

``` sh
arr[]=...&arr[]=...&arr[]=...
```

To integer-indexed encoding:

``` sh
arr[0]=...&arr[1]=...&arr[2]=...
```

See some additional background in: stripe/stripe-ruby#674
brandur-stripe pushed a commit to stripe/stripe-go that referenced this pull request Aug 14, 2018
Changes all arrays from classic Rack encoding:

``` sh
arr[]=...&arr[]=...&arr[]=...
```

To integer-indexed encoding:

``` sh
arr[0]=...&arr[1]=...&arr[2]=...
```

See some additional background in: stripe/stripe-ruby#674
@remi-stripe
Copy link
Contributor

This looks good to me, especially as you explicitly tested multiple edge-cases!

@brandur-stripe
Copy link
Contributor

Thanks Remi! Let's go for it then.

@brandur-stripe brandur-stripe merged commit 59ef4c2 into master Aug 15, 2018
@brandur-stripe brandur-stripe deleted the brandur-integer-indexes branch August 15, 2018 17:14
@brandur-stripe
Copy link
Contributor

Released as 3.22.0.

brandur-stripe pushed a commit to stripe/stripe-go that referenced this pull request Aug 15, 2018
Changes all arrays from classic Rack encoding:

``` sh
arr[]=...&arr[]=...&arr[]=...
```

To integer-indexed encoding:

``` sh
arr[0]=...&arr[1]=...&arr[2]=...
```

See some additional background in: stripe/stripe-ruby#674
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants