feat(go-sdk): Automatically refresh expired authentication tokens #1608
Conversation
The Looker Go SDK was not automatically refreshing expired authentication tokens, causing authentication to fail after one hour. This change modifies the AuthSession to proactively manage the token's lifecycle. An IsActive method was added to check the token's validity, and a Login method was added to refresh the token when needed. The Do method was updated to call Login before making any API requests, ensuring the token is always active.
Summary of ChangesHello @drstrangelooker, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue in the Looker Go SDK where authentication tokens were not automatically refreshed, leading to authentication failures after their one-hour expiry. The changes introduce a robust token management system within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces automatic token refresh for the Go SDK by adding IsActive and Login methods to AuthSession and calling Login before each API request. This is a good improvement to handle expired tokens proactively. My main feedback is regarding thread safety. The new stateful nature of AuthSession introduces a data race when used concurrently. I've provided a suggestion to add a mutex and update the new methods to be thread-safe. This is important for the reliability of the SDK in concurrent applications.
| func (s *AuthSession) IsActive() bool { | ||
| if s.token == nil { | ||
| return false | ||
| } | ||
| leeway := time.Second * 10 | ||
| return s.token.Expiry.After(time.Now().Add(leeway)) | ||
| } | ||
| | ||
| func (s *AuthSession) Login() (*oauth2.Token, error) { | ||
| if s.IsActive() { | ||
| return s.token, nil | ||
| } | ||
| | ||
| token, err := s.source.Token() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| s.token = token | ||
| return token, nil | ||
| } |
There was a problem hiding this comment.
The AuthSession is likely to be used concurrently, but the new methods IsActive and Login introduce data races when accessing the s.token field. IsActive reads s.token while Login writes to it, without any synchronization.
To make AuthSession thread-safe, you should add a sync.RWMutex to the struct:
import "sync" type AuthSession struct { // ... other fields token *oauth2.Token source oauth2.TokenSource mu sync.RWMutex }Then, you can update IsActive and Login to use this mutex. The implementation below uses a read-lock for checking the token and a write-lock for refreshing it, which is efficient. It also uses the double-checked locking pattern to prevent unnecessary locking when the token is valid.
I've also replaced the magic number 10 * time.Second with a constant for better readability.
const tokenLeeway = 10 * time.Second func (s *AuthSession) IsActive() bool { s.mu.RLock() defer s.mu.RUnlock() if s.token == nil { return false } return s.token.Expiry.After(time.Now().Add(tokenLeeway)) } func (s *AuthSession) Login() (*oauth2.Token, error) { // First, check with a read lock to avoid contention if the token is valid. s.mu.RLock() if s.token != nil && s.token.Expiry.After(time.Now().Add(tokenLeeway)) { token := s.token s.mu.RUnlock() return token, nil } s.mu.RUnlock() // If the token is invalid or nil, acquire a write lock to refresh it. s.mu.Lock() defer s.mu.Unlock() // Re-check after obtaining the write lock, in case another goroutine refreshed it. if s.token != nil && s.token.Expiry.After(time.Now().Add(tokenLeeway)) { return s.token, nil } token, err := s.source.Token() if err != nil { return nil, err } s.token = token return token, nil }
Go Tests 6 files 6 suites 2m 14s ⏱️ Results for commit 218e428. ♻️ This comment has been updated with latest results. |
The Looker Go SDK was not automatically refreshing expired authentication tokens, causing authentication to fail after one hour. This change modifies the AuthSession to proactively manage the token's lifecycle. An IsActive method was added to check the token's validity, and a Login method was added to refresh the token when needed. The Do method was updated to call Login before making any API requests, ensuring the token is always active.
Fixes #1606 🦕