-
Notifications
You must be signed in to change notification settings - Fork 325
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
Cookie extractor from Set-Cookie response. #184
base: master
Are you sure you want to change the base?
Cookie extractor from Set-Cookie response. #184
Conversation
Can one of the admins verify this patch? |
class CookieExtractor(AbstractExtractor): | ||
""" Extractor that pulls out a named cookie """ | ||
extractor_type = 'cookie' | ||
is_header_extractor = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! Glad you picked up and added this.
@danielatdattrixdotcom Thank you, this is a feature that I know is in fairly high demand. I've added a couple comments (small changes) before this is ready to be merged. It also needs some sort of small unit test in test_validators.py. This weekend I'm going to play with it a bit more and look more closely at the format of output (it might make sense to add a few options). You get kudos by the way for looking closely at how the other extractors are implemented and following that (and adding documentation as well)! |
This reverts commit 11b39f2. Why: breaks URL handling because it applies to the scheme (http:// etc)
… value from the list of extracted named cookies.
@svanoort Let me know on the args/options to extractors in the YAML/JSON files and your thoughts on that and I'll make the updates and tests. |
Oh no, I had write the same feature code, but @danielatdattrixdotcom 's documents is nice. |
@@ -1,5 +1,6 @@ | |||
import logging | |||
import json | |||
import Cookie |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, but we need to do the following for python 3 compatibility, since the library changed in Py3 😢 https://docs.python.org/2/library/cookie.html
if 'sys' is not in imports, add it:
PYTHON_MAJOR_VERSION = sys.version_info[0]
if PYTHON_MAJOR_VERSION > 2:
import http.cookies as Cookie
else:
import Cookie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even easier, just do this:
from .six.moves import http_cookies as Cookie
I'm testing this patch with the above and it works perfectly well on both Py2 and Py3:
$ python2 $(which pyresttest) http://localhost:8000 test/api.yml
Test Group Default SUCCEEDED: : 5/5 Tests Passed!
$ python3 $(which pyresttest) http://localhost:8000 test/api.yml
Test Group Default SUCCEEDED: : 5/5 Tests Passed!
@danielatdattrixdotcom Coming back to this now that the crunch period at work is settling down (spoiler: Jenkins, and specifically work on Pipeline Stage View plugin): I think it's not worth adding options to the extractor right now - I had a notion of eventually allowing use of regexes or other matchers in extraction, but it's not useful yet. We need support for templating with collections (or transformations on collections), and/or some embedded transformation language, if we're going to go down that route. It's planned in the future, but too far off to worry about now. Added one comment about a Python 3 compatibility challenge (just an import change). So, from my point of view, to get to merge, what we need are automated tests, covering a couple key edge cases: cookie doesn't exist, cookie exists with multiple values, normal cookie case. This means a unit test in test_validators.py, and a functional test in functionaltest.py -- the unit test just covers the extractor parsing and behavior (you can use some cookie info taken from a browser session). The functional test needs to work with real HTTP requests; I'd prefer to add cookie-setting to the test Django-tastypie app (probably easiest done with the sessions middleware option), but if that proves hard we can just invoke a public API or site, as long as the request is trivial. I've been using GitHub for this in the past. |
I needed this for working with an API that uses Django auth/sessions to work with CSRF cookie and X-CSRFToken header.