Fix the token expiration logic in SR Oauth#2177
Conversation
Naxin Fang (fangnx)
requested review from
a team and
Matthew Seal (MSeal)
as code owners
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the token expiration logic for Schema Registry OAuth authentication. The previous implementation incorrectly calculated when tokens should be refreshed, causing them to be refreshed much earlier than intended.
Changes:
- Fixed the token expiration calculation to properly compute the refresh buffer as
expires_in * (1 - token_expiry_threshold)instead ofexpires_in * token_expiry_threshold - Applied the fix consistently to both sync and async implementations
- Improved variable naming from
expiry_windowtorefresh_bufferfor better clarity
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/confluent_kafka/schema_registry/_sync/schema_registry_client.py | Fixed token expiration logic in the synchronous OAuth client to correctly calculate when tokens need refreshing |
| src/confluent_kafka/schema_registry/_async/schema_registry_client.py | Fixed token expiration logic in the asynchronous OAuth client to correctly calculate when tokens need refreshing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| refresh_buffer = self.token['expires_in'] * (1 - self.token_expiry_threshold) | ||
| return self.token['expires_at'] < time.time() + refresh_buffer |
There was a problem hiding this comment.
The existing test case test_expiry() in tests/schema_registry/_sync/test_bearer_field_provider.py (and the async equivalent) will fail with this fix because it uses inconsistent test data. The test sets expires_at = time.time() + 2 and expires_in = 1, which doesn't match real OAuth token behavior where expires_at should be approximately current_time + expires_in. The test needs to be updated to use consistent values, for example: {'expires_at': time.time() + 1, 'expires_in': 1} or adjust the sleep duration and assertions accordingly.
| refresh_buffer = self.token['expires_in'] * (1 - self.token_expiry_threshold) | ||
| return self.token['expires_at'] < time.time() + refresh_buffer |
There was a problem hiding this comment.
The existing test case test_expiry() in tests/schema_registry/_async/test_bearer_field_provider.py will fail with this fix because it uses inconsistent test data. The test sets expires_at = time.time() + 2 and expires_in = 1, which doesn't match real OAuth token behavior where expires_at should be approximately current_time + expires_in. The test needs to be updated to use consistent values, for example: {'expires_at': time.time() + 1, 'expires_in': 1} or adjust the sleep duration and assertions accordingly.
|
What
Checklist
References
JIRA:
Test & Review
Open questions / Follow-ups