fix: standardize libSQL timestamps as RFC 3339 UTC#683
fix: standardize libSQL timestamps as RFC 3339 UTC#683tsubasakong wants to merge 3 commits intonearai:mainfrom
Conversation
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 enhances the robustness and consistency of timestamp handling within the libSQL database layer. It transitions the default timestamp generation in all database migrations to the explicit RFC 3339 UTC format, which eliminates ambiguity and improves data integrity. To ensure a smooth transition, the system now logs warnings when encountering older, naive timestamp formats, while still correctly parsing them for backward compatibility. These changes are supported by new, focused unit tests that validate the correct parsing and formatting of both new and legacy timestamp types. Highlights
Changelog
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 libSQL timestamps to RFC 3339 UTC format, which is a great improvement for consistency. The changes involve updating all datetime('now') calls in migrations to use strftime for the new format. I appreciate that you've maintained backward compatibility for parsing legacy naive timestamps and added warnings to guide users towards the new format. The new tests provide good coverage for these changes.
I have a couple of suggestions to improve the maintainability and robustness of the code. One is to refactor a small piece of duplicated code in the timestamp parsing logic for conciseness. The others are to use a more explicit and readable format for generating timestamps in SQL, both in the migrations and in the related test query, to avoid relying on potentially fragile SQLite behavior and make the intent clearer to future developers.
| if let Ok(ndt) = NaiveDateTime::parse_from_str(s, "%Y-%m-%d %H:%M:%S%.f") { | ||
| tracing::warn!( | ||
| timestamp = %s, | ||
| "parsed naive timestamp without timezone; assuming UTC for backward compatibility" | ||
| ); | ||
| 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, | ||
| "parsed naive timestamp without timezone; assuming UTC for backward compatibility" | ||
| ); | ||
| return Ok(ndt.and_utc()); | ||
| } |
There was a problem hiding this comment.
There's some code duplication in the handling of legacy naive timestamp formats. You can combine these two if let blocks into one by chaining the parsing attempts with or_else. This will make the code more concise and easier to maintain.
if let Ok(ndt) = NaiveDateTime::parse_from_str(s, "%Y-%m-%d %H:%M:%S%.f")
.or_else(|_| NaiveDateTime::parse_from_str(s, "%Y-%m-%d %H:%M:%S"))
{
tracing::warn!(
timestamp = %s,
"parsed naive timestamp without timezone; assuming UTC for backward compatibility"
);
return Ok(ndt.and_utc());
}| let conn = backend.connect().await.unwrap(); | ||
| let mut rows = conn | ||
| .query( | ||
| "SELECT strftime('%Y-%m-%dT%H:%M:%fZ', 'now')", |
There was a problem hiding this comment.
To align with the suggestion for more explicit timestamp formatting in the migrations file, this test query should also be updated. Using a more verbose but clearer construction for generating the RFC 3339 timestamp will improve maintainability.
"SELECT strftime('%Y-%m-%dT%H:%M:%S', 'now') || '.' || substr(strftime('%f', 'now'), 4) || 'Z'",| version INTEGER PRIMARY KEY, | ||
| name TEXT NOT NULL, | ||
| applied_at TEXT NOT NULL DEFAULT (datetime('now')) | ||
| applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) |
There was a problem hiding this comment.
The format string '%Y-%m-%dT%H:%M:%fZ' is a bit confusing as it relies on a non-standard and sparsely documented quirk of SQLite's strftime where %f seems to imply seconds plus fractional seconds. For better readability and to avoid relying on this potentially fragile behavior, consider using a more explicit construction. This would make the intent clearer to future maintainers. This comment applies to all similar strftime calls in this file.
| applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')) | |
| applied_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%S', 'now') || '.' || substr(strftime('%f', 'now'), 4) || 'Z') |
1 participant
Summary
datetime(now)defaults/usages in migrations with RFC 3339 UTCstrftime(...)values\n- keep legacy naive timestamp parsing for backward compatibility, but warn when that fallback is used\n- add focused timestamp parsing/format coverage for the canonical libSQL timestamp format\n\nFixes Standardize libSQL timestamp storage to RFC 3339 with UTC offset #663\n\n## Validation\n-git diff --check\n- verifiedsrc/db/libsql_migrations.rsno longer containsdatetime(now)\n- added unit coverage for RFC 3339 + legacy naive timestamp parsing paths\n\n## Notes\n- supersedes fix: standardize libSQL timestamps as RFC 3339 UTC #680, which appears to have been closed with a reason intended for a different PR\n- Rust toolchain is not installed on this host, so I could not runcargo testlocally; CI will perform the full compile/test pass.