Conversation
|
Thanks for the contribution! Before we can merge this, we need @kwy404 to sign the Salesforce Inc. Contributor License Agreement. |
AmyScript
left a comment
There was a problem hiding this comment.
Nice catch, normalizing to UTC with .replace(tzinfo=timezone.utc) is exactly right. A couple of small things:
On the test (test_internals.py)
One thing to flag: this case expects the same value as the existing tz-aware one, so if CI runs in UTC it'll pass even without the fix. Could we force a non-UTC timezone so it fails on main? Something like:
@mock.patch.dict(os.environ, {"TZ": "America/New_York"})
def test_naive_datetime_is_treated_as_utc(self):
time.tzset() # POSIX only
try:
assert _timestamp_to_type(datetime(2023, 11, 28, 22, 9, 7), int) == 1701209347
finally:
del os.environ["TZ"]
time.tzset()On the string branch (internals.py)
Quick question: does the isinstance(ts, str) branch just below have the same issue? If it parses to a naive datetime and calls .timestamp(), it'd hit the same local-time bug. Might be worth normalizing it the same way for consistency.
Nit: could be nice to mention this in the changelog, since it slightly changes behavior for anyone who was relying on naive = local time.
|
Thanks for the review!
|
Summary
SQL-based installation stores save expiration and install times as naive UTC datetimes, but
_timestamp_to_typeconverted naive datetimes withdatetime.timestamp(), which assumes local time. On servers not running in UTC this shiftedbot_token_expires_atanduser_token_expires_atby the local offset, so token rotation could skip an already expired token. Naive datetimes are now treated as UTC, matching how naive ISO strings are already handled.Fixes #1731
Testing
Added a naive datetime case to
test_timestamp_to_typeintests/slack_sdk/oauth/installation_store/test_internals.py; it fails before the change on a machine outside UTC and passes after. The oauth test suites (sync and async),ruff format --check,ruff checkandscripts/run_mypy.shwere run locally.Category
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.