litl / rauth

A Python library for OAuth 1.0/a, 2.0, and Ofly.
http://readthedocs.org/docs/rauth/en/latest/
MIT License
1.6k stars 174 forks source link

Update Requests to v1.0.0 #56

Closed maxcountryman closed 11 years ago

naktinis commented 11 years ago

I would like to see this soon as well (for now I'm patching this myself). For example, Requests 1.0 does not support calling requests.session with hooks as a parameter.

tgrosinger commented 11 years ago

I can confirm this as well. Line 486 of service.py should not pass any parameters to requests.session.

maxcountryman commented 11 years ago

It's on my TODO list. Currently trying to fight off the flu tho so don't expect it this week.

maxcountryman commented 11 years ago

What I'm planning to do here is completely remove the hook-based architecture from rauth. The new requests' API allows for directly manipulation over the bytes sent via a Request object. So the general outline of the new rauth implementation would utilize this Request object, probably by subclassing it and providing the necessary OAuth logic as a set of methods. Hopefully an initial version of this will be available soon. To begin with I may push this to a branch and then as I solidify the API merge it back onto master in tandem with a release.

maxcountryman commented 11 years ago

Unfortunately this has been delayed by an apparent design bug exposed while working on a prototype implementation, you can follow the bug here: https://github.com/kennethreitz/requests/issues/1133

The last word seems to be that there is a bug in the design of the API and that until it's resolved it's probably best to hold off on writing code that would make use of custom Request objects and Session adaptors.

Hopefully this will be resolved soon and we can continue to work on the next iteration of rauth.

tgrosinger commented 11 years ago

Is there any workaround you would recommend that I can use in the mean time?

maxcountryman commented 11 years ago

@tgrosinger use requests < 1.0.0.

tgrosinger commented 11 years ago

I embarrassingly realized this shortly after posting that. Thank you.

maxcountryman commented 11 years ago

Good news:

I think @sigmavirus24 has proposed a solution that will hopefully be pulled into Requests shortly. Once that change is made we can go forward with our plans for rauth.

sigmavirus24 commented 11 years ago

@maxcountryman I'll talk to Kenneth about this later tonight on IRC. If you want to be around for it, you can hop into the IRC channel on Freenode.

maxcountryman commented 11 years ago

@sigmavirus24 cool thanks, I'll try to hop on. freenode/#python-requests is it?

sigmavirus24 commented 11 years ago

@sigmavirus24 cool thanks, I'll try to hop on. freenode/#python-requests is it?

Yep. Sorry that I forgot to mention the channel name. :)

maxcountryman commented 11 years ago

This looks like it shouldn't be too far off. Here's a bit of our conversation on IRC from last night:

https://botbot.me/freenode/python-requests/1800374/

maxcountryman commented 11 years ago

We're waiting on the result of https://github.com/kennethreitz/requests/pull/1151. Once that's in I'll begin working on a new version of Rauth.

maxcountryman commented 11 years ago

I've got a first pass going over here: https://github.com/maxcountryman/rauth/tree/requests-v1

Feel free to take a look, comment, and so forth. There's some other design considerations I'm working through so this may change going forward. But for now you can use OAuth 1.0/a with Requests v1.x using that branch. (Note that I haven't implemented OAuth 2.0 or Ofly but those should be fairly trivial and I'll add them shortly.)

mahmoudhossam commented 11 years ago

Just wanted you to know that pip installs requests 1.1.x

maxcountryman commented 11 years ago

@mahmoudhossam if you install using the setup.py on master it will use the correct version.

mahmoudhossam commented 11 years ago

Can you please update the PyPI package?

maxcountryman commented 11 years ago

@mahmoudhossam we won't be doing a release until the next version of rauth is done. I would recommend instead installing from the repo directly, which pip will allow you to do.

maxcountryman commented 11 years ago

I'm actually working on the next release now, but I can't give a definitive timeline. We have a number of goals we're trying to address during this redesign and working out exactly how it will work is taking the most time. I expect implementation should be fairly quick.

mahmoudhossam commented 11 years ago

I installed requests==0.14 manually as a workaround.

Thanks for a really great library.

maxcountryman commented 11 years ago

@mahmoudhossam okay sorry about the trouble. I'm hoping to have this fixed on PyPI by next week. Happy you're finding rauth useful! :cake:

maxcountryman commented 11 years ago

I just pushed some changes to a branch: https://github.com/litl/rauth/compare/requests-v1

I think this general approach, where we wrap requests.Session and expose a custom Session object per service-type is probably how things will look going forward. Feel free to take a look. If you pull it locally you should be able to use the latest version of Requests without issue. If you do run into any problems, please report them.

mahmoudhossam commented 11 years ago

In the requests-v1 branch, setup.py still requires requests<1.0.0. I think this should be updated.

mahmoudhossam commented 11 years ago

I also found a regression, doing the following:

request_token, request_token_secret = \
    twitter.get_request_token(method='GET', oauth_callback=callback_url)

Returns the following error:

Traceback (most recent call last):
  File "tw.py", line 15, in <module>
    twitter.get_request_token(method='GET', oauth_callback=callback_url)
  File "/home/mahmoud/Projects/twitter_unfollower/venv/lib/python2.7/site-packages/rauth/service.py", line 453, in get_request_token
    data = self.get_raw_request_token(method=method, **kwargs)
  File "/home/mahmoud/Projects/twitter_unfollower/venv/lib/python2.7/site-packages/rauth/service.py", line 442, in get_raw_request_token
    r = self.request(method, self.request_token_url, **kwargs)
  File "/home/mahmoud/Projects/twitter_unfollower/venv/lib/python2.7/site-packages/rauth/service.py", line 535, in request
    return session.request(method, url, header_auth=header_auth, **kwargs)
  File "/home/mahmoud/Projects/twitter_unfollower/venv/lib/python2.7/site-packages/rauth/session.py", line 85, in request
    return super(OAuth1Session, self).request(method, url, **req_kwargs)
TypeError: request() got an unexpected keyword argument 'oauth_callback'
maxcountryman commented 11 years ago

@mahmoudhossam thank you for your comments. They're very helpful and most appreciated! You're absolutely right that setup.py needs to be updated. As for the second point I'm hoping to write up a change log but now almost all params will be passed in via the standard requests params or data dicts.

maxcountryman commented 11 years ago

@mahmoudhossam I've updated the setup.py. Would you mind trying this for me as well?

twitter.get_request_token(params={'oauth_callback': callback_url})
mahmoudhossam commented 11 years ago

I have a better idea, why don't we include the oauth_callback parameter to the service instead?

I think it'll be a lot cleaner than having keyword arguments here and there.

maxcountryman commented 11 years ago

The reason to pass it via params or data is because it remains consistent with the Request API for which rauth is just a loose wrapper. We don't want to start special-casing things like this or we'll end up with a confusing API.

mahmoudhossam commented 11 years ago

As a user, it sure is easier for me to put it in the service, it's also easier to remember.

The service will be the place where all the urls and the configuration live, I don't see how that's confusing.

Except you're talking about the implementation, I don't really know much about how rauth or requests are implemented.

Plus, the callback url is service specific, so in the context of OAuth, it makes sense to declare it there.

Maybe it makes sense for requests to do it this way, but for rauth, we should do the thing that makes more sense in the context of OAuth.

maxcountryman commented 11 years ago

We already were doing it the way you suggest and it was confusing because some OAuth params were presented this way and others had to be passed to Requests via params or data (which btw is where they all end up anyway).

My preference now is to move away from this model: it's fragile, it's confusing, and most importantly it obscures the actual codepath.

I can appreciate that it seems like it would be easier to remember, but unfortunately it makes the service wrappers as a set harder to understand because their APIs start to diverge as more and more special cases are introduced. Instead this is one consistent way of doing things that is actually pretty straightforward and easy to wrap your head around.

For those reasons the upcoming release will remove as many of these special cases as possible (including this one). This will mean if you know and understand the Requests' API you won't need to learn much more to use rauth.

joeshaw commented 11 years ago

Throwing in my 2¢ here... if an option is going to be the same for all users of a given service wrapper and makes sense to be passed down through to a session, then setting it on the Service object is the most convenient API.

The base URL and consumer token/secret (oauth1) or client id/secret (oauth2) elements are good examples of this.

I'm not sure if oauth_callback fits in well here, because it's conceivable that you might use different values depending on how your app is set up -- some OAuth providers are not strict about what the callback URL has to be.

In cases where it's not a good fit, I agree with Max that consistency with the Requests API is useful. rauth is built on top of Requests, after all, and sits as a light layer on top of its API. It's not a complete abstraction layer. rauth would be doing a disservice to expose an API that didn't fit in well with Requests' own.

sigmavirus24 commented 11 years ago

You could always use params.setdefault('oauth_callback', self.oauth_callback) (I'm guessing at the structure, since I haven't looked at rauth's source) so that if there isn't any passed via the params, it's tacked on from the service by you guys.

maxcountryman commented 11 years ago

@sigmavirus24 that might be okay. My concern is confusing things that are actual Requests' params with attributes of the service wrapper. I'm not sure that's the best way to do things.

maxcountryman commented 11 years ago

@mahmoudhossam here's my current thinking: I'd like to remove the special cases for the upcoming release to kind of clear the air. This is because of the problems we were having with there being an exceptional amount of special casing throughout the wrappers. Then after the dust settles, I think we should revisit this. There probably are reasonable opportunities to introduce things like an oauth_callback attribute. How does that sound?

mahmoudhossam commented 11 years ago

@maxcountryman That sounds reasonable.

Have you tried the code you posted earlier? Do I still need to test it?

maxcountryman commented 11 years ago

@mahmoudhossam I've been testing it and trying it with the examples as I work on it, yes. You don't have to try it but it might be helpful. I'm hoping to have a first final draft of the changes by the end of today. Right now I'm working on a new test suite. Once that's done things should be stabilized. So it might be helpful to have people go over the changes after that point. We'll hopefully do a code review before things are merged into master and an actual release happens.

mahmoudhossam commented 11 years ago

I just tried 0.5.0, it works for me too.

Good work :+1:

I still think the way the callback and verifier are passed isn't optimal, but we'll get to that later.

maxcountryman commented 11 years ago

Hi again everyone,

Good news: the code is more or less written. I'm sure there are bugs just waiting to be found. I haven't adapted this yet to test with all the major providers rauth works with presently so I'm sure we'll run into a few problems there. However the major work of porting the codebase to Requests v1.x is done. I'd appreciate if it anyone who is willing could pull the requests-v1 branch and give it a shot. I'm particularly interested in feedback as related to the changes from rauth 0.4.x.

There's a number of other things I could use extra :eyes: on as well: the README, documentation, and examples should be gone over with an eye for typos and other things. Also I'm probably missing things in the CHANGELOG so if anyone spots major changes I've not made a note of there already, please let me know.

Finally I'm not sure when this will be released, but soon. I want to make sure it goes through a proper code review before merging it into master. I also need to make sure that it works with a specific set of providers as well.

maxcountryman commented 11 years ago

Good news: the new version of rauth is now vetted against the required set of providers I need it to minimally work with. This doesn't mean there aren't bugs or edge cases I've overlooked, but as a first final draft it's definitely a positive sign. Porting from 0.4.x was fairly straightforward. I'll try to prepare a small guide that should highlight most of the differences and things to look out for. Currently I've pushed some bugfixes to the requests-v1 branch. Later I'll rebase that branch against master and push it to 0.5.0. If you're doing testing please use requests-v1 for now. 0.5.0 is behind.

maxcountryman commented 11 years ago

I've updated the 0.5.0 branch. That is now up-to-date with the requests-v1 branch and is rebased against master so that some necessary inclusions in the pull are ignored, i.e. deleting _docs/_build/.

maxcountryman commented 11 years ago

0.5.0 has been released to PyPI.