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

Added Session object for cookie storage and defaults #328

Merged
merged 7 commits into from
Apr 15, 2015

Conversation

tvoinarovskyi
Copy link
Member

Simple snippet of code:

>>> import asyncio
>>> from aiohttp import client
>>> loop = asyncio.get_event_loop()
>>> s = client.Session(loop=loop)
>>> r = loop.run_until_complete(s.get("http://google.com"))
>>> s.cookies == r.cookies
True

yield from asyncio.async(resp.release(), loop=loop)
continue
@asyncio.coroutine
def options(self, url, allow_redirects=True, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

Why allow_redirects is positional argument here when data for post/put isn't?

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

Question: if I'm using Cookie based auth and my cookie eventually expired, how could I renew it? Same issue with JWT tokens and other things with TTL.

The solution I see is to provide a callback to session and let it call it in case of first 401 error to reauth the session.

@asvetlov
Copy link
Member

I think session expiration is too complex in general for implementing it into client itself.
You may wrap your calls into try/except block for renewing session by default.
Or derive from Session and override .request() method.

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

@asvetlov if it was so easy: I can't wrap every call with try/except/renew since I actually don't know when and where session will get expired. That's why passing some callback coroutine to trigger on auth error could be helpful.

Yes, overriding .request() is what I'm doing now (: But thought that's quite common problem to have some base solution on the ground.

break
@asyncio.coroutine
def head(self, url, allow_redirects=False, **kwargs):
resp = yield from self.request('HEAD', url,
Copy link
Member

Choose a reason for hiding this comment

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

use hdrs.METH_XXXX instead of strings

@asvetlov
Copy link
Member

Actually you have a need for two callbacks:

  • one for detection for session expiring. In general techically it may be not only 401 response code but everything including some specific body for 200 OK. Site builders sometimes don't follow any standards.
  • another one for session renewal.

I think it's overengeneering. At least we can add those callbacks later.

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

Eh, forgot about bad sites. Ok, may be I just want too much now (: Anyway, Session is a good step forward.

@fafhrd91
Copy link
Member

i'd like to use something like base_url

session = Session('http://python.com/')
session.get('/docs/index.html')
session.post('/login.html')

@kxepal
Copy link
Member

kxepal commented Apr 14, 2015

@fafhrd91 I'll a bit extend your proposal: how about something like this? Two points:

  1. Construction urls with __call__ using unescaped path segments
  2. Extraction credentials from URL into Authorization header

However, all these features in additional to http-method-like class methods are a functionality of HTTP resources, not a session which should only keep and track a state, made requests apply it onto outgoing requests and update from incoming responses.

@asvetlov
Copy link
Member

I'm strongly against base_url in Session class, it should be added in descendant.
Session is for processing cookies/keepalives only.

Adding base url is a can of worms: you have to process:

  • absolute urls in .get() etc
  • schema changing (from http to https for example)
  • another question is: should base_url be changed on redirect to outside site?

I feel adding base_url looks very appealing but think that should be implemented in another class. Processing different schema (http, https, //) and working with absolute and relative urls is not trivial.

@fafhrd91
Copy link
Member

how often you change schema? maybe if you building proxy. if you build client for existing rest-style service it is much easier to use base_url. my feeling people use it as client for existing service more often that anything else.

@asvetlov
Copy link
Member

My guess is to add base_url in new class derived from Session

On Tue, Apr 14, 2015 at 5:47 PM, Nikolay Kim notifications@github.com
wrote:

how often you change schema? maybe if you building proxy. if you build
client for existing rest-style service it is much easier to use base_url.
my feeling people use it as client for existing service more often that
anything else.


Reply to this email directly or view it on GitHub
#328 (comment).

Thanks,
Andrew Svetlov

@fafhrd91
Copy link
Member

ok. let just rename to ClientSession

@asvetlov
Copy link
Member

ok with ClientSession name.

@fafhrd91
Copy link
Member

i'm planing to release next 0.15 release during this week, let's include session as well.

 - Used hdrs.METH_ objects for session calls
 - allow_redirects now id keyword-only for get head options in session
@tvoinarovskyi
Copy link
Member Author

Fixed minor issues, pointed out before.
Added docs on ClientSessions.

asvetlov added a commit that referenced this pull request Apr 15, 2015
Added Session object for cookie storage and defaults
@asvetlov asvetlov merged commit 7e66b2a into aio-libs:master Apr 15, 2015
@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants