Release v4.3.1: Replace naive datetime.now() with timezone-aware UTC timestamps#82
Release v4.3.1: Replace naive datetime.now() with timezone-aware UTC timestamps#82
Conversation
…ware timestamps Co-authored-by: obervinov <49606362+obervinov@users.noreply.github.com>
|
@copilot You also need to increase the version of the module and update the changelog. |
Co-authored-by: obervinov <49606362+obervinov@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR prepares release v4.3.1 by standardizing timestamp creation on UTC timezone-aware datetimes to reduce inconsistencies when comparing application timestamps with PostgreSQL-stored timestamps.
Changes:
- Replaced
datetime.now()withdatetime.now(timezone.utc)in core rate limit logic and token issuance. - Updated test fixtures and rate limit tests to use UTC-aware “now”.
- Bumped package version to 4.3.1 and added v4.3.1 release notes to the changelog.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| users/users.py | Makes issued token expiry timestamps UTC-aware. |
| users/storage.py | Updates token storage docstring example to use UTC-aware timestamps. |
| users/ratelimiter.py | Switches rate-limit time comparisons and calculations to UTC-aware “now”. |
| tests/test_rate_limits.py | Updates test reference “now” timestamps to UTC-aware. |
| tests/conftest.py | Updates fixture timestamps to UTC-aware. |
| pyproject.toml | Bumps version from 4.3.0 to 4.3.1. |
| CHANGELOG.md | Adds v4.3.1 release entry. |
Comments suppressed due to low confidence (1)
tests/test_rate_limits.py:20
- These tests still assert
re.match(timestamp_pattern, str(user['rate_limits']))wheretimestamp_patternonly matches naive timestamps (no+00:00suffix). Ifrate_limitsis now timezone-aware (as produced bydatetime.now(timezone.utc) + ...),str()will include an offset and the regex/assertions will fail. Update the pattern/assertion to accept timezone offsets or format the datetime explicitly.
now = datetime.datetime.now(datetime.timezone.utc)
user = users_instance.user_access_check(user_id='testUser5', role_id='admin_role')
assert user['rate_limits'] is not None
assert re.match(timestamp_pattern, str(user['rate_limits'])) is not None
assert isinstance(user['rate_limits'], datetime.datetime)
assert user['rate_limits'] >= now + datetime.timedelta(minutes=58)
assert user['rate_limits'] < now + datetime.timedelta(hours=24)
| # Calculate expiration | ||
| expires_at = datetime.now() + timedelta(minutes=ttl_minutes) | ||
| expires_at = datetime.now(timezone.utc) + timedelta(minutes=ttl_minutes) | ||
|
|
There was a problem hiding this comment.
expires_at is now timezone-aware UTC, but users_tokens.token_expires_at is defined as TIMESTAMP (no timezone) in the test schema and is compared in SQL against NOW() (timestamptz). Inserting aware datetimes into a TIMESTAMP column can undergo implicit timezone conversion depending on the DB/session timezone, causing expiry checks to be incorrect in non-UTC environments. Either store naive UTC timestamps consistently or migrate the column (and queries) to TIMESTAMPTZ.
| @@ -278,19 +278,19 @@ def fixture_users(vault_instance, postgres_instance): | |||
| 'requests_history': [ | |||
| ( | |||
| 'testUser6', 'testMessage1', 'testChat6', 'allowed', | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now() - timedelta(hours=3) | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now(timezone.utc) - timedelta(hours=3) | |||
| ), | |||
| ( | |||
| 'testUser6', 'testMessage2', 'testChat6', 'allowed', | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now() - timedelta(hours=4) | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now(timezone.utc) - timedelta(hours=4) | |||
| ), | |||
| ( | |||
| 'testUser6', 'testMessage3', 'testChat6', 'allowed', | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now() - timedelta(hours=5) | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now(timezone.utc) - timedelta(hours=5) | |||
| ), | |||
| ( | |||
| 'testUser6', 'testMessage4', 'testChat6', 'allowed', | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now() - timedelta(hours=6), datetime.now() + timedelta(hours=24) | |||
| '{"role_id": "admin_role", "status": "allowed"}', datetime.now(timezone.utc) - timedelta(hours=6), datetime.now(timezone.utc) + timedelta(hours=24) | |||
| ), | |||
| ] | |||
There was a problem hiding this comment.
These fixtures now generate timezone-aware datetimes, but the test DB schema defines users_requests.timestamp/rate_limits as TIMESTAMP (no timezone). Inserting aware datetimes into TIMESTAMP can be implicitly converted based on the server/session timezone, making tests (and potentially behavior) environment-dependent. Prefer inserting naive UTC datetimes (e.g., .replace(tzinfo=None)) or update the schema to TIMESTAMPTZ and keep everything timezone-aware end-to-end.
| Example: | ||
| >>> storage = Storage(database_connection) | ||
| >>> storage.store_token("user1", "hash123", "salt456", datetime.now() + timedelta(minutes=10)) | ||
| >>> storage.store_token("user1", "hash123", "salt456", datetime.now(timezone.utc) + timedelta(minutes=10)) |
There was a problem hiding this comment.
The docstring example now uses timezone.utc (and timedelta) but doesn’t show the required imports. Since this is presented as a runnable example, include the necessary from datetime import timedelta, timezone (or adjust the example to avoid undeclared names).
| # If rate limits is active (compared the last request with the current time) | ||
| exist_rate_limit = user_requests[0][2] | ||
| if exist_rate_limit and datetime.strptime(exist_rate_limit, '%Y-%m-%d %H:%M:%S.%f') >= datetime.now(): | ||
| if exist_rate_limit and datetime.strptime(exist_rate_limit, '%Y-%m-%d %H:%M:%S.%f') >= datetime.now(timezone.utc): |
There was a problem hiding this comment.
datetime.strptime(..., '%Y-%m-%d %H:%M:%S.%f') returns a naive datetime, but it’s being compared to datetime.now(timezone.utc) (timezone-aware). In Python this raises TypeError: can't compare offset-naive and offset-aware datetimes at runtime. Normalize both sides (e.g., make parsed timestamps UTC-aware, or keep everything naive UTC consistently) before comparing.
| if exist_rate_limit and datetime.strptime(exist_rate_limit, '%Y-%m-%d %H:%M:%S.%f') >= datetime.now(timezone.utc): | |
| if exist_rate_limit and datetime.strptime( | |
| exist_rate_limit, '%Y-%m-%d %H:%M:%S.%f' | |
| ).replace(tzinfo=timezone.utc) >= datetime.now(timezone.utc): |
| # If the rate limit has already expired - reset the rate limit | ||
| if datetime.now() >= datetime.strptime(latest_rate_limit_timestamp, '%Y-%m-%d %H:%M:%S.%f'): | ||
| if datetime.now(timezone.utc) >= datetime.strptime(latest_rate_limit_timestamp, '%Y-%m-%d %H:%M:%S.%f'): | ||
| log.info('[Users.RateLimiter]: The rate limit %s for user ID %s has expired and will be reset', latest_rate_limit_timestamp, self.user_id) |
There was a problem hiding this comment.
latest_rate_limit_timestamp is parsed into a naive datetime via strptime, but then compared against datetime.now(timezone.utc) (aware). This will raise a naive/aware comparison TypeError. Consider attaching timezone.utc to parsed values (if the DB stores UTC) or switching to a consistent naive-UTC strategy throughout the rate limiter.
| request_timestamp = request[1] | ||
| if request_timestamp >= datetime.now() - timedelta(hours=1): | ||
| if request_timestamp >= datetime.now(timezone.utc) - timedelta(hours=1): | ||
| requests_per_hour = requests_per_hour + 1 | ||
| if request_timestamp >= datetime.now() - timedelta(days=1): | ||
| if request_timestamp >= datetime.now(timezone.utc) - timedelta(days=1): | ||
| requests_per_day = requests_per_day + 1 |
There was a problem hiding this comment.
request_timestamp values coming from PostgreSQL TIMESTAMP columns are typically naive datetimes. Comparing them to datetime.now(timezone.utc) - ... will raise TypeError (naive vs aware) and break counter calculations. Convert DB timestamps to UTC-aware (or make now naive) before these comparisons.
v4.3.1 - 2026-02-11
What's Changed
Full Changelog: v4.3.0...v4.3.1
🐛 Bug Fixes
datetime.nowcalls withdatetime.nowtimezone.utcto ensure consistency with PostgreSQL's UTC-basedNOWfunction and prevent timestamp comparison issues across systemsusers/ratelimiter.py: 8 instances in rate limit calculations and timestamp comparisonsusers/users.py: Token expiration calculationusers/storage.py: Docstring exampletests/conftest.py: 20 test fixture timestampstests/test_rate_limits.py: 3 test assertion timestamps