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 access token introspection to avoid new access token fetch if feasible #50

Merged
merged 8 commits into from
Sep 23, 2022

Conversation

dsprayberry
Copy link
Contributor

@dsprayberry dsprayberry commented Sep 13, 2022

Description of change

LinkedIn seemingly does not appreciate when we fetch new access tokens while a valid one already exists. This has been observed to exhibit itself in the form of 429 errors suggesting that a user has reached a rate limit while trying to use a refresh token to fetch a new access token in discovery.

This change checks the expiration of an existing access_token (if one is present in the config) and proceeds with that token if so.

Manual QA steps

  • Tested locally with various access tokens.
  • Added several unittests.

Risks

  • Access Tokens could expire between checking the validity and issuing a request, and this does not provide handling for that case.

Rollback steps

  • revert this branch

tap_linkedin_ads/client.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zachharris1 zachharris1 left a comment

Choose a reason for hiding this comment

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

LGTM

@dmosorast dmosorast merged commit f2b0c67 into master Sep 23, 2022
@dmosorast dmosorast deleted the fix/check_access_token_validity branch September 23, 2022 18:18
@dmosorast
Copy link
Contributor

Merged in lieu of tests passing because tests passed on local, and automation is in a state right now that's not able to be quickly fixed.

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.

3 participants