From 1b98db47a942c239d803c635b868b1ce2d1d457f Mon Sep 17 00:00:00 2001 From: boxbeam Date: Wed, 24 Apr 2024 17:04:31 -0400 Subject: [PATCH] fix(db): fix bugs based on sqlite date format difference from chrono (#1951) * fix(db): fix repository refresh job * Provide fix for sqlite date format * Update implementation for repository provider * Fix formatting * Add compiletime test case for operation types * Fix tests * Make end bound exclusive * Fix formatting --- ee/tabby-db/src/github_repository_provider.rs | 11 ++- ee/tabby-db/src/lib.rs | 83 ++++++++++++++++++- ee/tabby-db/src/user_completions.rs | 12 +-- ee/tabby-db/src/user_events.rs | 6 +- ee/tabby-webserver/src/integrations/github.rs | 2 +- ee/tabby-webserver/src/service/analytic.rs | 8 +- 6 files changed, 103 insertions(+), 19 deletions(-) diff --git a/ee/tabby-db/src/github_repository_provider.rs b/ee/tabby-db/src/github_repository_provider.rs index 8d551df62adb..c14a1b1145e6 100644 --- a/ee/tabby-db/src/github_repository_provider.rs +++ b/ee/tabby-db/src/github_repository_provider.rs @@ -3,7 +3,7 @@ use chrono::{DateTime, Utc}; use sqlx::{prelude::FromRow, query, query_as}; use tabby_db_macros::query_paged_as; -use crate::{DbConn, SQLXResultExt}; +use crate::{AsSQLiteDateTime, DbConn, SQLXResultExt}; #[derive(FromRow)] pub struct GithubRepositoryProviderDAO { @@ -156,8 +156,12 @@ impl DbConn { ) -> Result { let res = query!( "INSERT INTO github_provided_repositories (github_repository_provider_id, vendor_id, name, git_url) VALUES ($1, $2, $3, $4) - ON CONFLICT(github_repository_provider_id, vendor_id) DO UPDATE SET github_repository_provider_id = $1, name = $2, git_url = $3", - github_provider_id, vendor_id, name, git_url).execute(&self.pool).await?; + ON CONFLICT(github_repository_provider_id, vendor_id) DO UPDATE SET name = $3, git_url = $4, updated_at = DATETIME('now')", + github_provider_id, + vendor_id, + name, + git_url + ).execute(&self.pool).await?; Ok(res.last_insert_rowid()) } @@ -177,6 +181,7 @@ impl DbConn { github_provider_id: i64, cutoff_timestamp: DateTime, ) -> Result<()> { + let cutoff_timestamp = cutoff_timestamp.as_sqlite_datetime(); query!( "DELETE FROM github_provided_repositories WHERE github_repository_provider_id = ? AND updated_at < ?;", github_provider_id, diff --git a/ee/tabby-db/src/lib.rs b/ee/tabby-db/src/lib.rs index 01afaf5f77b0..a713786c488c 100644 --- a/ee/tabby-db/src/lib.rs +++ b/ee/tabby-db/src/lib.rs @@ -1,9 +1,14 @@ -use std::{ops::Deref, path::Path, sync::Arc}; +use std::{ + fmt::Display, + ops::{Add, Deref, Sub}, + path::Path, + sync::Arc, +}; use anyhow::anyhow; use cache::Cache; use cached::TimedSizedCache; -use chrono::{DateTime, NaiveDateTime, Utc}; +use chrono::{DateTime, Duration, NaiveDateTime, Utc}; pub use email_setting::EmailSettingDAO; pub use github_repository_provider::{GithubProvidedRepositoryDAO, GithubRepositoryProviderDAO}; pub use invitations::InvitationDAO; @@ -256,9 +261,53 @@ where } } +pub trait AsSQLiteDateTime { + fn as_sqlite_datetime(&self) -> String; +} + +impl AsSQLiteDateTime for DateTime { + fn as_sqlite_datetime(&self) -> String { + self.format("%F %X").to_string() + } +} + #[derive(Default, Clone)] pub struct DateTimeUtc(DateTime); +impl Display for DateTimeUtc { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.as_sqlite_datetime()) + } +} + +impl std::fmt::Debug for DateTimeUtc { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.as_sqlite_datetime()) + } +} + +impl DateTimeUtc { + pub fn now() -> Self { + Utc::now().into() + } +} + +impl Add for DateTimeUtc { + type Output = Self; + + fn add(self, rhs: Duration) -> Self::Output { + ((self.0) + rhs).into() + } +} + +impl Sub for DateTimeUtc { + type Output = Self; + + fn sub(self, rhs: Duration) -> Self::Output { + ((self.0) - rhs).into() + } +} + impl From> for DateTimeUtc { fn from(value: DateTime) -> Self { Self(value) @@ -291,7 +340,7 @@ impl<'a> Encode<'a, Sqlite> for DateTimeUtc { &self, buf: &mut >::ArgumentBuffer, ) -> sqlx::encode::IsNull { - self.0.encode_by_ref(buf) + >::encode(self.0.as_sqlite_datetime(), buf) } } @@ -352,6 +401,34 @@ mod tests { assert_eq!(new_token.len(), 36); assert_ne!(old_token, new_token); } + + #[tokio::test] + async fn test_timestamp_format() { + let db = DbConn::new_in_memory().await.unwrap(); + + let time = DateTimeUtc::now(); + + let time_str = time.as_sqlite_datetime(); + let sql_time: String = sqlx::query_scalar::<_, String>("SELECT ?;") + .bind(time) + .fetch_one(&db.pool) + .await + .unwrap(); + + assert_eq!(time_str, sql_time); + + let sql_time: String = sqlx::query_scalar::<_, String>("SELECT DATETIME('now');") + .fetch_one(&db.pool) + .await + .unwrap(); + assert_eq!(sql_time, DateTimeUtc::now().as_sqlite_datetime()); + + // No assertions, these will fail at compiletime if adding/subtracting from these types + // yields DateTime, which could be dangerous + let time = DateTimeUtc::now(); + let _added_time: DateTimeUtc = time.clone() + Duration::milliseconds(1); + let _subbed_time: DateTimeUtc = time - Duration::milliseconds(1); + } } #[cfg(any(test, feature = "testutils"))] diff --git a/ee/tabby-db/src/user_completions.rs b/ee/tabby-db/src/user_completions.rs index cf1d9459c20a..70e989162b9d 100644 --- a/ee/tabby-db/src/user_completions.rs +++ b/ee/tabby-db/src/user_completions.rs @@ -5,7 +5,7 @@ use cached::CachedAsync; use chrono::{DateTime, Utc}; use sqlx::{prelude::FromRow, query}; -use crate::{DateTimeUtc, DbConn}; +use crate::{AsSQLiteDateTime, DateTimeUtc, DbConn}; #[derive(FromRow)] pub struct UserCompletionDAO { @@ -41,7 +41,9 @@ impl DbConn { let duration = Duration::from_millis(ts as u64); let created_at = DateTime::from_timestamp(duration.as_secs() as i64, duration.subsec_nanos()) - .context("Invalid created_at timestamp")?; + .context("Invalid created_at timestamp")? + .as_sqlite_datetime(); + let res = query!( "INSERT INTO user_completions (user_id, completion_id, language, created_at) VALUES (?, ?, ?, ?);", user_id, @@ -109,7 +111,7 @@ impl DbConn { SUM(selects) as selects, SUM(views) as views FROM user_completions JOIN users ON user_id = users.id AND users.active - WHERE user_completions.created_at >= DATE('now', '-1 year') + WHERE user_completions.created_at >= DATETIME('now', '-1 year') AND ({users_empty} OR user_id IN ({users})) GROUP BY 1, 2 ORDER BY 1, 2 ASC @@ -171,8 +173,8 @@ impl DbConn { no_selected_users = users.is_empty(), no_selected_languages = languages.is_empty(), )) - .bind(start) - .bind(end) + .bind(start.as_sqlite_datetime()) + .bind(end.as_sqlite_datetime()) .fetch_all(&self.pool) .await?; Ok(res) diff --git a/ee/tabby-db/src/user_events.rs b/ee/tabby-db/src/user_events.rs index 6960d484ce7d..d98774eef6e1 100644 --- a/ee/tabby-db/src/user_events.rs +++ b/ee/tabby-db/src/user_events.rs @@ -5,7 +5,7 @@ use chrono::{DateTime, Utc}; use sqlx::{prelude::FromRow, query}; use tabby_db_macros::query_paged_as; -use crate::DbConn; +use crate::{AsSQLiteDateTime, DbConn}; #[derive(FromRow)] pub struct UserEventDAO { @@ -50,8 +50,8 @@ impl DbConn { ) -> Result> { let condition = Some(format!( "created_at >= '{}' AND created_at < '{}'", - start.to_rfc3339(), - end.to_rfc3339() + start.as_sqlite_datetime(), + end.as_sqlite_datetime() )); let events = query_paged_as!( UserEventDAO, diff --git a/ee/tabby-webserver/src/integrations/github.rs b/ee/tabby-webserver/src/integrations/github.rs index 99f8f4fbf0dd..d7fe630ac03d 100644 --- a/ee/tabby-webserver/src/integrations/github.rs +++ b/ee/tabby-webserver/src/integrations/github.rs @@ -62,7 +62,7 @@ pub fn routes( }; Router::new() .route("/connect/:id", routing::get(connect)) - // Routes defined past this point require authentication + // Routes defined above this point require authentication .layer(from_fn_with_state(auth, require_login_middleware)) .route("/callback", routing::get(callback)) .with_state(state) diff --git a/ee/tabby-webserver/src/service/analytic.rs b/ee/tabby-webserver/src/service/analytic.rs index d59473796fb8..568668c6f19d 100644 --- a/ee/tabby-webserver/src/service/analytic.rs +++ b/ee/tabby-webserver/src/service/analytic.rs @@ -90,7 +90,7 @@ pub fn new_analytic_service(db: DbConn) -> Arc { #[cfg(test)] mod tests { - use chrono::Days; + use chrono::{Days, Duration}; use super::*; use crate::service::AsID; @@ -200,7 +200,7 @@ mod tests { .unwrap(); let svc = new_analytic_service(db); - let end = Utc::now(); + let end = Utc::now() + Duration::days(1); let start = end.checked_sub_days(Days::new(100)).unwrap(); // Test that there is a single completion stat (1 day of history) with 1 completion and 1 select @@ -243,7 +243,7 @@ mod tests { .unwrap(); let service = new_analytic_service(db); - let end = Utc::now(); + let end = Utc::now() + Duration::days(1); let start = end.checked_sub_days(Days::new(100)).unwrap(); let stats = service @@ -288,7 +288,7 @@ mod tests { .unwrap() .is_empty()); - let end = Utc::now(); + let end = Utc::now() + Duration::days(1); let start = end.checked_sub_days(Days::new(100)).unwrap(); assert!(service