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

Remove refresh auth #119

Merged
merged 7 commits into from Sep 27, 2016

Conversation

Projects
None yet
3 participants
@georgeyk
Copy link
Contributor

commented Aug 19, 2016

Hi guys,

This PR is just a suggestion, close it if it's out of the project goals.
But basically, I felt annoyed to pass refresh_auth if my client needs to update the token from time to time.

So I changed the TapiocaClientExecutor and TapiocaAdapter to basically call refresh_authentication method whenever it is necessary (and without break existing adapters).

Unfortunately it breaks the interface for clients that already use refresh_auth flag.

I've removed some dead-code and pep8 fixes.
I would happilly update this code after your thoughts.

georgeyk added some commits Aug 19, 2016

TapiocaAdapter: add default implementation of is_authentication_expir…
…ed method

After we remove refresh_auth parameter from TapiocaClientExecutor,
is_authentication_expired should return False by default.
So, for tokens that never expire or adapters that won't use tokens won't need
to implement is_authentication_expired method.
@coveralls

This comment has been minimized.

Copy link

commented Aug 19, 2016

Coverage Status

Coverage remained the same at 91.195% when pulling 67ad837 on georgeyk:remove-refresh-auth into 6e9b117 on vintasoftware:master.

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2016

:o this error seems to come from requests lib... (only in py32 - weirdo)

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Aug 23, 2016

Hey @georgeyk, thanks for the PR, will take a look this week and get back to you. Don't worry about the py32 error, I dropped the support for it (just like the whole python community hehe).

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

Hey, just keeping the PR alive, tell me if I can help =)

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

Hey @georgeyk, I've been thinking about this. Let me expose my points here to you, maybe you can help me:

  • I'm not 100% ok with your approach because it hides from the client user about whether of not the token refresh is happening. In fact the user does not even know whether the wrapper supports or not token refreshing.
  • Requiring a explicitly passed refresh_token=True parameter and raising a NotImplementedError is a good thing because it tells the client user that token refreshing is not currently supported and he might what to work on it and PR back to the repository.
  • I understand that in most cases token refreshing is desirable to be an automatic operation. This is good because this way more inexperienced developers do not have to understand the mechanics and deal with it.
  • At the same time, there might be a case where the client user does not want to automatically refresh the token and since Tapioca is a very generic solution, it must support this case as well.
  • So far one solution that looks viable to me is: we introduce a new parameter that can be passed on the initialization of the wrapper, maybe call it refresh_token_by_default, that is False by default. But when the user sets it to True, it will try to refresh token by default, unless refresh_token=False is passed in the request parameter.
  • Since we are talking about this, there are other issues I'd like to tackle. For example, I was talking with @cassiobotaro this week, and we need a way to allow the user to retrieve the "updated token" so it can be saved for future use (otherwise the expired token will keep on).

This is what I have so far, what do you think? Any other ideas?

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2016

@filipeximenes the follow-up:

Thinking about the client user, would he mind to know if his credentials needs to be "updated" ? Personally, I just want to access a resource and I want to get this (auth-thing) out of my way.

The worst scenario would be the "token refresh" optionally implemented (and used), right ?
I mean the resource supports more than one way to perform authentication.
In my opinion, they are two different clients sharing the same resources, but let's assume just one client.
I agree we need some way to configure the client once and then all the subsequent calls will use this parameter to automatically detects whether "token refresh" or not.
I'm not really familiar with tapioca's code, but I'll take a look how to update this PR.

Your last point is tricky. Because refresh_authentication does not have access to the caller's references (that holds the token used in request via api_params).
My current workaround on this is to keep the token outside the adapter and the client, but accessible to the adapter class. Then in get_request_kwargs I read the current token and in refresh_authentication I update it.

Maybe, another class in the chain to deal with authencation/ token retrieval might work. I'll see if I can do a PoC.

georgeyk added some commits Aug 29, 2016

Implement token refresh behavior
Added refresh_token_by_default parameter in TapiocaClient.
Added refresh_token parameter to request methods (TapiocaClientExecutor)
The refresh_token_by_default controls if the client should try to
automatically refresh token (default is False).

The refresh_token overrides refresh_token_by_default for a single
request call.
@coveralls

This comment has been minimized.

Copy link

commented Aug 29, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.966% when pulling 5089c7e on georgeyk:remove-refresh-auth into 6e9b117 on vintasoftware:master.

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2016

Hi,

So, I've updated the PR to match the last comments. Tell me if this is heading in the right direction.

Except for the last part that we are talking about access the updated token, I think it should be subject of another PR.

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Sep 1, 2016

Thanks @georgeyk. I'm a bit busy this week will take a look on the next.

@@ -444,35 +434,14 @@ def test_simple_pages_max_item_zero_iterator(self):

self.assertEqual(iterations_count, 0)

@responses.activate
def test_simple_pages_max_item_zero_iterator(self):

This comment has been minimized.

Copy link
@filipeximenes

filipeximenes Sep 20, 2016

Member

Why was this removed?

This comment has been minimized.

Copy link
@georgeyk

georgeyk Sep 20, 2016

Author Contributor

@filipeximenes this test is duplicated (expand until line 425)

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

An idea to solve retrieving the new token would be to make the method is_authentication_expired update the token in the params AND return the new token so we can save it. What do you think? Do you see any possible problems?
(we can leave this to a separate PR)

I made a small comment on the PR the rest looks good. Can you add information about this new behavior to the docs before we merge?

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Sep 20, 2016

Btw, very sorry for the delay, things are pretty busy around here

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2016

@filipeximenes thx for the review, I'll try to update the docs in the next days (also in a hush here).

About the token retrieval issue, returning token data in is_authentication_expired won't fix it (imho). The caller still won't have access to the method (unless associated like a regular "resource" - afaik).

I was thinking about replace this machinery to a callback-style to be defined upon client initialization (like MyTapiocaClient(..., auth_callback=callable_here or None)) but expose token retrieval through client interface also work.
My use case benefits from callback because I can easily attach another routine to save the refreshed token externally (using redis or similar) and update client parameters.
I think we should continue this ideas in another PR ou issue, so can track all of them and vote for cons/pros. =)

@coveralls

This comment has been minimized.

Copy link

commented Sep 20, 2016

Coverage Status

Coverage decreased (-0.2%) to 90.966% when pulling 88928dd on georgeyk:remove-refresh-auth into 6e9b117 on vintasoftware:master.

@filipeximenes filipeximenes merged commit 9612391 into vintasoftware:master Sep 27, 2016

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 90.966%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@filipeximenes

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

@georgeyk Looking awesome, thanks!

@filipeximenes

This comment has been minimized.

Copy link
Member

commented Sep 27, 2016

On PyPI as version 1.2.3

@georgeyk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.