fix(libsql): standardize timestamp storage to RFC 3339 with UTC offset#705
fix(libsql): standardize timestamp storage to RFC 3339 with UTC offset#705
Conversation
#663) Replace all `datetime('now')` defaults in libsql_migrations.rs with `strftime('%Y-%m-%dT%H:%M:%fZ', 'now')` so new rows get proper RFC 3339 timestamps (e.g. `2024-01-15T10:30:00.123Z`) instead of naive datetimes (e.g. `2024-01-15 10:30:00`). Add tracing::warn! to parse_timestamp() naive fallback paths so legacy timestamps are still accepted but produce a visible deprecation signal. Backward compatible: no data migration needed; existing naive timestamps continue to parse correctly via the multi-format fallback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
scope: db/libsql
Summary of ChangesHello, 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 significantly enhances the handling of timestamps within the Highlights
Changelog
Activity
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 standardizes timestamp storage to RFC 3339 format in UTC for the libsql backend. This is achieved by replacing datetime('now') with strftime in all database migrations. To handle legacy data, warnings are now logged when parsing naive timestamps. The changes are supported by a comprehensive set of new tests. My review includes a couple of suggestions to improve code maintainability by reducing duplication and to enhance the new tests for better coverage.
| if let Ok(ndt) = NaiveDateTime::parse_from_str(s, "%Y-%m-%d %H:%M:%S%.f") { | ||
| tracing::warn!( | ||
| timestamp = s, | ||
| "parsing naive timestamp without timezone; assuming UTC — consider re-running migrations" | ||
| ); | ||
| return Ok(ndt.and_utc()); | ||
| } | ||
| // Naive without fractional seconds (legacy format) | ||
| if let Ok(ndt) = NaiveDateTime::parse_from_str(s, "%Y-%m-%d %H:%M:%S") { | ||
| tracing::warn!( | ||
| timestamp = s, | ||
| "parsing naive timestamp without timezone; assuming UTC — consider re-running migrations" | ||
| ); | ||
| return Ok(ndt.and_utc()); |
There was a problem hiding this comment.
These two blocks for parsing naive datetimes are very similar and contain duplicated code, including the warning message. You can refactor this into a loop over an array of possible formats to improve maintainability and reduce duplication.
// Naive fallback for legacy formats
let naive_formats = ["%Y-%m-%d %H:%M:%S%.f", "%Y-%m-%d %H:%M:%S"];
for fmt in naive_formats {
if let Ok(ndt) = NaiveDateTime::parse_from_str(s, fmt) {
tracing::warn!(
timestamp = s,
"parsing naive timestamp without timezone; assuming UTC — consider re-running migrations"
);
return Ok(ndt.and_utc());
}
}| assert!( | ||
| DateTime::parse_from_rfc3339(&started_at).is_ok(), | ||
| "started_at not valid RFC 3339: {started_at}" | ||
| ); |
There was a problem hiding this comment.
For completeness and to make the test more robust, you should also verify that last_activity is a valid RFC 3339 timestamp using DateTime::parse_from_rfc3339, just as you do for started_at.
assert!(
DateTime::parse_from_rfc3339(&started_at).is_ok(),
"started_at not valid RFC 3339: {started_at}"
);
assert!(
DateTime::parse_from_rfc3339(&last_activity).is_ok(),
"last_activity not valid RFC 3339: {last_activity}"
);
ilblackdragon
left a comment
ilblackdragon
left a comment
There was a problem hiding this comment.
Code Review
Clean, well-scoped PR. All 55 datetime('now') occurrences are replaced, backward compat is preserved via the naive fallback, and test coverage is solid. A few suggestions:
1. Warning noise on legacy data (medium)
The tracing::warn! in parse_timestamp() will fire on every row read from an existing database with naive timestamps. On a DB with thousands of rows this will flood logs. Consider:
- Downgrading to
tracing::debug!ortracing::trace! - Or using
std::sync::Once/AtomicBoolto warn once per process
2. Update src/db/CLAUDE.md (low)
Line 84 still documents DEFAULT (datetime('now')) as the libSQL convention. Per project rules ("Update both sides"), this should be updated to DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) to keep spec and code in sync.
3. Overall
Core change is correct and complete — no missed occurrences in migrations or runtime SQL. Tests are well-structured (tempfile, tokio::test, covers RFC 3339 + naive + invalid + DB-level DEFAULT). Low risk.
2 participants
Summary
Closes #663.
datetime('now')defaults inlibsql_migrations.rswithstrftime('%Y-%m-%dT%H:%M:%fZ', 'now')so new rows get proper RFC 3339 timestamps with UTC offsettracing::warn!toparse_timestamp()naive fallback paths so legacy timestamps produce a deprecation signalTest plan
test_parse_timestamp_rfc3339-- verifies RFC 3339 with Z and +00:00 offsetstest_parse_timestamp_naive_fallback-- verifies backward compat with legacy naive formattest_parse_timestamp_invalid-- verifies error on bad inputtest_default_timestamps_are_rfc3339-- inserts a row with DEFAULT values and asserts RFC 3339 formatcargo clippy --all --all-features-- zero warningscargo test --lib-- 2539 passed (libsql), 2548 passed (postgres)Generated with Claude Code