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

Add "cookiejar" option to config. #180

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielatdattrixdotcom
Copy link
Contributor

A minor change to use the curl cookiejar and treat one file as a session. I needed this for a Tastypie API using SessionAuthentication. And a white space trimming courtesy of PyCharm.

…r file in CWD which it will remove when complete. Added condition for GET requests as well.
@svanoort-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@svanoort
Copy link
Owner

@danielatdattrixdotcom Thank you for your contribution - this is great. I have to ask, why did you close the PR? I was planning to test it out and merge once I get out of crunch mode at work and have a little time to review.

Cheers,
Sam

@svanoort-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@danielatdattrixdotcom
Copy link
Contributor Author

@svanoort I ran into an issue it created in not resetting the curl object to a base state so I closed it. The issue presents if you have a PUT then a POST. It needs code to reset the curl object to a ready state without wiping the cookiejar. I'll work out an alternate PR since this one is flawed.

@svanoort
Copy link
Owner

@danielatdattrixdotcom Probably because it's using CookieJar and CookieFile on the same file, which makes it r/w. One can unset the CookieJar opt to render it read-only, but then you lose the advantage of a persistent session after initial auth.

Considering this use case, would it not be better to create an Extractor that gets the cookies (there's already issue #144 to add extractors geared to cookie operations with the next release), and save cookies to a variable? The cookies are already available on the headers (which all extractors get to play with), there's just not a binding to handle them specially yet.

@danielatdattrixdotcom
Copy link
Contributor Author

@svanoort Just posted an updated commit with very minor adjustments. I'm writing tests for a project now, so if I encounter any issues I will adjust as-needed if I find I created a problem. I read through #144 , but since we've got curl and it already has mechanisms for managing cookies I did not see a need to reinvent it and add any more complex configuration setup for the simple use of cookies as a test session.

@danielatdattrixdotcom
Copy link
Contributor Author

@svanoort I ran into a segfault during testing and made one more adjustment.

@svanoort
Copy link
Owner

svanoort commented Apr 9, 2016

@danielatdattrixdotcom I do apologize for the delay (crunch mode at the day job) -- do you consider this to be in a state where I can hit it with the full integrated tests and merge if ready?

@danielatdattrixdotcom
Copy link
Contributor Author

@svanoort Yes, I think so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants