clean up address verify property and some miscellaneous request logic #127
Conversation
| value = [value] | ||
| verify_params[key] = value | ||
| if verify_params: | ||
| url += '?' + _urlencode_list(verify_params) |
There was a problem hiding this comment.
It's not clear to me why this list encoding needs its own special function, as opposed to passing the list as a parameter to requestor.request. It looks like _urlencode_list behaves slightly differently than the default list encoder in that it doesn't include an index inside the [], and our API docs do not indicate that an index should be included in the param name. Perhaps the other list encoder is incorrect?
There was a problem hiding this comment.
frankly I think we should just switch to encoding the bodies to JSON and get rid of all of this (absolutely awful) urlencoding crap
There was a problem hiding this comment.
I agree that we should eventually move away from the nasty URL encoding for URL params. I looked into it some more, and part of the challenge is that our underlying HTTP library has to support both Python Requests (the good one), and also Google App Engine's urlfetcher (which is somewhat non-standard, and probably enforced for performance/security reasons on their cloud compute environment).
The way the client library is structured now has it so that URL parameters are encoded earlier on, before we get to the part where the HTTP library is even engaged.
Python Requests has a clean interface to encode URL params that also takes care of the URL encoding issues, but we aren't taking advantage of this.
In a future iteration of this library, I'd like to see the URL encoding logic pushed as far down as possible, and maintain a Python dictionary for the URL parameters and payload throughout most of the code, and do the necessary conversions only where necessary.
I would like to see the "multiple-path handling" logic for accommodating the two different HTTP libraries eventually be implemented as two different adapters or strategies hidden behind a wrapper for cleaner abstraction.
- For the second part, regarding lists of params, I had a hard time finding an official spec, but this SO details some of the issues.
To pass in a list of params, the two common practices are to do one of:
a) base/url?fav_foods=pizza&fav_foods=ice_cream&fav_foods=sushi OR
b) base/url?fav_foods[]=pizza&fav_foods[]=ice_cream&fav_foods[]=sushi
It's very wrong to have a list index inside the param. The former works, but various HTTP frameworks aren't implemented correctly to handle it, and the latter is also not idea, being a pattern that is used by some popular HTTP frameworks like Django and others, but again, not ubiquitous.
frankly I think we should just switch to encoding the bodies to JSON and get rid of all of this
James's suggestion to do away with URL parameters and just encode the whole thing and pass as a JSON body is the most correct/ideal.
In Conclusion
Given all of the options (and the difficulty in implementing them right now), and that this change is correct, I am comfortable accepting and merging this code as is.
There was a problem hiding this comment.
I strongly agree with moving away from URL-encoding request parameters.
@jontsai putting an adapter in front of the HTTP request logic is a good idea, whether or not we stick with URL encoding parameters. In addition to that, I would love to drop support for urlfetch, as it's not in the GAE Python 3 runtime. Sadly, however, Google has stated that they will continue to support the 2.7 runtime indefinitely, so it's not clear when we'll be able to do that.
| value = [value] | ||
| verify_params[key] = value | ||
| if verify_params: | ||
| url += '?' + _urlencode_list(verify_params) |
There was a problem hiding this comment.
I agree that we should eventually move away from the nasty URL encoding for URL params. I looked into it some more, and part of the challenge is that our underlying HTTP library has to support both Python Requests (the good one), and also Google App Engine's urlfetcher (which is somewhat non-standard, and probably enforced for performance/security reasons on their cloud compute environment).
The way the client library is structured now has it so that URL parameters are encoded earlier on, before we get to the part where the HTTP library is even engaged.
Python Requests has a clean interface to encode URL params that also takes care of the URL encoding issues, but we aren't taking advantage of this.
In a future iteration of this library, I'd like to see the URL encoding logic pushed as far down as possible, and maintain a Python dictionary for the URL parameters and payload throughout most of the code, and do the necessary conversions only where necessary.
I would like to see the "multiple-path handling" logic for accommodating the two different HTTP libraries eventually be implemented as two different adapters or strategies hidden behind a wrapper for cleaner abstraction.
- For the second part, regarding lists of params, I had a hard time finding an official spec, but this SO details some of the issues.
To pass in a list of params, the two common practices are to do one of:
a) base/url?fav_foods=pizza&fav_foods=ice_cream&fav_foods=sushi OR
b) base/url?fav_foods[]=pizza&fav_foods[]=ice_cream&fav_foods[]=sushi
It's very wrong to have a list index inside the param. The former works, but various HTTP frameworks aren't implemented correctly to handle it, and the latter is also not idea, being a pattern that is used by some popular HTTP frameworks like Django and others, but again, not ubiquitous.
frankly I think we should just switch to encoding the bodies to JSON and get rid of all of this
James's suggestion to do away with URL parameters and just encode the whole thing and pass as a JSON body is the most correct/ideal.
In Conclusion
Given all of the options (and the difficulty in implementing them right now), and that this change is correct, I am comfortable accepting and merging this code as is.
Justintime50
left a comment
Justintime50
left a comment
There was a problem hiding this comment.
lftm based on our convos reviewing. Looks like this will need to be rebased against master?
allows users to just pass `True` for verify and verify_strict instead of having to pass `[True]`. Also properly quotes in case a user accidentally passes something icky. as always, I am a firm believer that functions should be functions, not classmethods
Roguelazer
force-pushed
the
true_verifications
branch
from
26c5466 to
e1f0b7e
Compare
|
rebased |
allows users to just pass
Truefor verify and verify_strict instead of having to pass[True]. Also properly quotes in case a user accidentally passes something icky.as always, I am a firm believer that functions should be functions, not classmethods