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 property expiry in gspread client #1453

Merged
merged 1 commit into from
May 26, 2024

Conversation

lavigne958
Copy link
Collaborator

@lavigne958 lavigne958 commented Apr 16, 2024

In gspread client add a new property expiry which returns the expiry datetime object from the used credentials.

The expiry only exists for OAuth credentials otherwise it returns None.

@lavigne958 lavigne958 requested a review from alifeee April 16, 2024 22:24
@lavigne958 lavigne958 self-assigned this Apr 16, 2024
@alifeee
Copy link
Collaborator

alifeee commented Apr 16, 2024

Hi :)

This seems good. It seems like something difficult to test(?)

The issue #1169 asked for:

It would be interesting if there was a single exception that handled the case of expired authorization, to be used directly when you need to update the authorization.

I do not see that here. Is it a new GspreadException type that we could raise?

@lavigne958
Copy link
Collaborator Author

Hi :)

This seems good. It seems like something difficult to test(?)

yes because it needs the OAuth authentication method to be tested 😢

The issue #1169 asked for:

It would be interesting if there was a single exception that handled the case of expired authorization, to be used directly when you need to update the authorization.

I do not see that here. Is it a new GspreadException type that we could raise?

I chose not to raise an exception as the expiry can be refreshed on the first API call. and it's easier if the user checks the expiry and chooses to raise when it happens.

I'll try to catch the API error in case of expired token but it needs me to run a query, then wait for at least a week and try again. So far I did not managed to trigger some API error it just refreshes silently

@alifeee
Copy link
Collaborator

alifeee commented Apr 19, 2024

okay. It seems a simple enough change 👍.

Not sure if it fixes #1169 though as they asked for an Exception to be thrown(?)

@lavigne958
Copy link
Collaborator Author

okay. It seems a simple enough change 👍.

Not sure if it fixes #1169 though as they asked for an Exception to be thrown(?)

true, I won't close the user issue, I will let the user choose if this is enough and then close the issue it solves the issue.

@lavigne958
Copy link
Collaborator Author

@alifeee you're ok to merge this as is ? knowing that it's not solving #1169 . but so far it provides the info to the user.

@alifeee
Copy link
Collaborator

alifeee commented Apr 24, 2024

@alifeee you're ok to merge this as is ? knowing that it's not solving #1169 . but so far it provides the info to the user.

sure, I am ok. I still do not fully understand auth, so I would not know when this would be useful. But if it is good for you it is good for me. :)

@lavigne958
Copy link
Collaborator Author

sure, I am ok. I still do not fully understand auth, so I would not know when this would be useful. But if it is good for you it is good for me. :)

auth works using a class that encapsulate the HTTP requests. It does that by doing authentication calls before the actual call we want to do sometimes or adds some token to the HTTP call without us knowing about it.

Here the auth class can return the expiry of the given token, if it is expired then the user is expected to remove the file authorized_user.json in order to force a new login.

@alifeee
Copy link
Collaborator

alifeee commented Apr 30, 2024

ok, so the user would be expected to do something like...

if client.expiry > datetime.today():
  # throw error! cannot do things properly!
# no error, continue
worksheet.update_acell("A1", "hi")

If I have misunderstood, perhaps you could provide a pseudocode sample in which this attribute could be used helpfully? :)

@lavigne958
Copy link
Collaborator Author

ok, so the user would be expected to do something like...

if client.expiry > datetime.today():
  # throw error! cannot do things properly!
# no error, continue
worksheet.update_acell("A1", "hi")

If I have misunderstood, perhaps you could provide a pseudocode sample in which this attribute could be used helpfully? :)

yes that's how it could be used.
I don't feel confident enough in the value hold by the authentication library to trigger an exception, but I'm fine exposing this value for any user to access it.

@lavigne958
Copy link
Collaborator Author

To be honest I check the expiry value and it seems 1h in the past, I tried to print the value with UTC timezone and still 1h in the past, so that means the expiry date provided is wrong as it's already past so I am wondering on how is this value accurate.

I am now running a script that uses oauth, sends a request every hours I will check if the expiry value has changed.

@alifeee
Copy link
Collaborator

alifeee commented May 10, 2024

ok. seems a good change 👌 can merge

@lavigne958
Copy link
Collaborator Author

actually after my last test, the script has been running for over a week and never expired.
I believe we should close this PR. The expiry attribute is not reliable, it does not actually provide an expected expiry date.

In gspread client add a new property expiry which returns the expiry
datetime object from the used credentials.

The expiry only exists for OAuth credentials otherwise it returns None.

Signed-off-by: Alexandre Lavigne <[email protected]>
@lavigne958 lavigne958 force-pushed the feature/expose_credentials_expiry branch from 5c63b39 to c884ff6 Compare May 26, 2024 21:32
@lavigne958
Copy link
Collaborator Author

@alifeee I am ready to merge this PR, after checking the behavior, even if the credentials are expired, we can make requests and it still works anyway. and credentials get refreshed !

if anyone needs to know we can expose this property anyway even if it has a limited usage it does not break anything and provides information to the user.

Waitting for you review.

@lavigne958 lavigne958 merged commit 139215d into master May 26, 2024
10 checks passed
@lavigne958 lavigne958 deleted the feature/expose_credentials_expiry branch May 26, 2024 21:39
@alifeee alifeee mentioned this pull request Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants