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

Fix multipart encoding of nested parameters #550

Merged
merged 1 commit into from
Apr 8, 2019

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Apr 5, 2019

r? @brandur-stripe @remi-stripe
cc @stripe/api-libraries

This one was a bit tricky. stripe-python rolls its own multipart encoder, which only supported two types of values: file-like (i.e. an object that has a read attribute) or strings. All other types caused an exception to be raised.

So there were actually two different issues to fix:

  1. the parameters need to be flattened (which solves the problem for compound types -- lists and dicts are gone after this step)
  2. handle non-string scalar types (booleans, integers, etc.)

The first issue was easily solved by calling api_requestor._api_encode() in MultipartDataGenerator to flatten the parameters.

To solve the second issue, I changed util.utf_8() to stringify all non-file-like values. This could potentially have side effects for regular form-encoding, but I'm fairly sure that's not the case: form-encoding works by passing the dictionary of flattened params to urlencode which would stringify non-string values anyway.

The second issue is solved by stringifying non-file-like values before adding them to the multipart buffer.

@remi-stripe
Copy link
Contributor

To solve the second issue, I changed util.utf_8() to stringify all non-file-like values. This could potentially have side effects for regular form-encoding, but I'm fairly sure that's not the case: form-encoding works by passing the dictionary of flattened params to urlencode which would stringify non-string values anyway.

Could we special-case this just for file upload? My inner paranoia is tingling right now with that sentence :p

@ob-stripe
Copy link
Contributor Author

ob-stripe commented Apr 5, 2019

Not sure why the PR is failing on Travis for some Python versions -- I suspect a circular import issue, but I can't reproduce locally. I'll try shuffling some imports around anyway.

EDIT: Fixed.

@ob-stripe ob-stripe force-pushed the ob-fix-multipart-nested-params branch 2 times, most recently from eabbcd9 to 9ffd8f5 Compare April 5, 2019 21:44
@ob-stripe ob-stripe force-pushed the ob-fix-multipart-nested-params branch from 9ffd8f5 to 0f7d84f Compare April 5, 2019 21:51
@ob-stripe
Copy link
Contributor Author

@remi-stripe I've updated the implementation to only stringify values in MultipartDataGenerator.

@remi-stripe
Copy link
Contributor

Thanks for making the change. This looks great to me and I like that you explicitly tested a nested dict!

@remi-stripe remi-stripe removed their assignment Apr 5, 2019
Copy link
Contributor

@brandur-stripe brandur-stripe left a comment

Choose a reason for hiding this comment

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

Thanks for looking at this, and awesome tests! LGTM.

@ob-stripe
Copy link
Contributor Author

Thanks both!

@ob-stripe ob-stripe merged commit fc8d4e5 into master Apr 8, 2019
@ob-stripe ob-stripe deleted the ob-fix-multipart-nested-params branch April 8, 2019 18:43
@ob-stripe
Copy link
Contributor Author

Released as 2.24.1.

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