Make it impossible to not assign completed date for completed benchmark requests

Also create benchmark requests with a struct literal in the Postgres code, to make it impossible to forget setting some fields.

Author: Kobzol

database/src/lib.rs

@@ -309,6 +309,7 @@ impl Scenario {
     }
 }
 
+use anyhow::anyhow;
 use std::cmp::Ordering;
 use std::str::FromStr;
 
@@ -802,7 +803,7 @@ pub enum BenchmarkRequestStatus {
     WaitingForArtifacts,
     ArtifactsReady,
     InProgress,
-    Completed,
+    Completed { completed_at: DateTime<Utc> },
 }
 
 const WAITING_FOR_ARTIFACTS_STR: &str = "waiting_for_artifacts";
@@ -811,44 +812,48 @@ const IN_PROGRESS_STR: &str = "in_progress";
 const COMPLETED_STR: &str = "completed";
 
 impl BenchmarkRequestStatus {
-    pub fn as_str(&self) -> &str {
+    pub(crate) fn as_str(&self) -> &str {
         match self {
-            BenchmarkRequestStatus::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR,
-            BenchmarkRequestStatus::ArtifactsReady => ARTIFACTS_READY_STR,
-            BenchmarkRequestStatus::InProgress => IN_PROGRESS_STR,
-            BenchmarkRequestStatus::Completed { .. } => COMPLETED_STR,
+            Self::WaitingForArtifacts => WAITING_FOR_ARTIFACTS_STR,
+            Self::ArtifactsReady => ARTIFACTS_READY_STR,
+            Self::InProgress => IN_PROGRESS_STR,
+            Self::Completed { .. } => COMPLETED_STR,
         }
     }
-}
-
-impl fmt::Display for BenchmarkRequestStatus {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        f.write_str(self.as_str())
-    }
-}
-
-impl<'a> tokio_postgres::types::FromSql<'a> for BenchmarkRequestStatus {
-    fn from_sql(
-        ty: &tokio_postgres::types::Type,
-        raw: &'a [u8],
-    ) -> Result<Self, Box<dyn std::error::Error + Sync + Send>> {
-        // Decode raw bytes into &str with Postgres' own text codec
-        let s: &str = <&str as tokio_postgres::types::FromSql>::from_sql(ty, raw)?;
 
-        match s {
+    pub(crate) fn from_str_and_completion_date(
+        text: &str,
+        completion_date: Option<DateTime<Utc>>,
+    ) -> anyhow::Result<Self> {
+        match text {
             WAITING_FOR_ARTIFACTS_STR => Ok(Self::WaitingForArtifacts),
             ARTIFACTS_READY_STR => Ok(Self::ArtifactsReady),
             IN_PROGRESS_STR => Ok(Self::InProgress),
-            COMPLETED_STR => Ok(Self::Completed),
-            other => Err(format!("unknown benchmark_request_status '{other}'").into()),
+            COMPLETED_STR => Ok(Self::Completed {
+                completed_at: completion_date.ok_or_else(|| {
+                    anyhow!("No completion date for a completed BenchmarkRequestStatus")
+                })?,
+            }),
+            _ => Err(anyhow!("Unknown BenchmarkRequestStatus `{text}`")),
         }
     }
 
-    fn accepts(ty: &tokio_postgres::types::Type) -> bool {
-        <&str as tokio_postgres::types::FromSql>::accepts(ty)
+    pub(crate) fn completed_at(&self) -> Option<DateTime<Utc>> {
+        match self {
+            Self::Completed { completed_at } => Some(*completed_at),
+            _ => None,
+        }
     }
 }
 
+impl fmt::Display for BenchmarkRequestStatus {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        f.write_str(self.as_str())
+    }
+}
+
+
+
 const BENCHMARK_REQUEST_TRY_STR: &str = "try";
 const BENCHMARK_REQUEST_MASTER_STR: &str = "master";
 const BENCHMARK_REQUEST_RELEASE_STR: &str = "release";
@@ -885,7 +890,6 @@ impl fmt::Display for BenchmarkRequestType {
 pub struct BenchmarkRequest {
     pub commit_type: BenchmarkRequestType,
     pub created_at: DateTime<Utc>,
-    pub completed_at: Option<DateTime<Utc>>,
     pub status: BenchmarkRequestStatus,
     pub backends: String,
     pub profiles: String,
@@ -896,18 +900,15 @@ impl BenchmarkRequest {
         tag: &str,
         created_at: DateTime<Utc>,
         status: BenchmarkRequestStatus,
-        backends: &str,
-        profiles: &str,
     ) -> Self {
         Self {
             commit_type: BenchmarkRequestType::Release {
                 tag: tag.to_string(),
             },
             created_at,
-            completed_at: None,
             status,
-            backends: backends.to_string(),
-            profiles: profiles.to_string(),
+            backends: String::new(),
+            profiles: String::new(),
         }
     }
 
@@ -927,7 +928,6 @@ impl BenchmarkRequest {
                 parent_sha: parent_sha.map(|it| it.to_string()),
             },
             created_at,
-            completed_at: None,
             status,
             backends: backends.to_string(),
             profiles: profiles.to_string(),
@@ -950,7 +950,6 @@ impl BenchmarkRequest {
                 parent_sha: parent_sha.to_string(),
             },
             created_at,
-            completed_at: None,
             status,
             backends: backends.to_string(),
             profiles: profiles.to_string(),

database/src/pool.rs

@@ -434,8 +434,6 @@ mod tests {
                 "1.8.0",
                 time,
                 BenchmarkRequestStatus::ArtifactsReady,
-                "cranelift,llvm",
-                "",
             );
 
             let db = db.connection().await;
@@ -490,8 +488,6 @@ mod tests {
                 "1.8.0",
                 time,
                 BenchmarkRequestStatus::ArtifactsReady,
-                "cranelift,llvm",
-                "",
             );
 
             let db = db.connection().await;

database/src/pool/postgres.rs

@@ -1453,58 +1453,56 @@ where
         let benchmark_requests = rows
             .iter()
             .map(|row| {
-                let tag = row.get::<_, Option<&str>>(0);
-                let parent_sha = row.get::<_, Option<&str>>(1);
+                let tag = row.get::<_, Option<String>>(0);
+                let parent_sha = row.get::<_, Option<String>>(1);
                 let pr = row.get::<_, Option<i32>>(2);
                 let commit_type = row.get::<_, &str>(3);
-                let status = row.get::<_, BenchmarkRequestStatus>(4);
+                let status = row.get::<_, &str>(4);
                 let created_at = row.get::<_, DateTime<Utc>>(5);
                 let completed_at = row.get::<_, Option<DateTime<Utc>>>(6);
-                let backends = row.get::<_, &str>(7);
-                let profiles = row.get::<_, &str>(8);
+                let backends = row.get::<_, String>(7);
+                let profiles = row.get::<_, String>(8);
+
+                let pr = pr.map(|v| v as u32);
+
+                let status =
+                    BenchmarkRequestStatus::from_str_and_completion_date(status, completed_at)
+                        .expect("Invalid BenchmarkRequestStatus data in the database");
 
                 match commit_type {
-                    BENCHMARK_REQUEST_TRY_STR => {
-                        let mut try_benchmark = BenchmarkRequest::create_try(
-                            tag,
+                    BENCHMARK_REQUEST_TRY_STR => BenchmarkRequest {
+                        commit_type: BenchmarkRequestType::Try {
+                            sha: tag,
                             parent_sha,
-                            pr.unwrap() as u32,
-                            created_at,
-                            status,
-                            backends,
-                            profiles,
-                        );
-                        try_benchmark.completed_at = completed_at;
-                        try_benchmark
-                    }
-                    BENCHMARK_REQUEST_MASTER_STR => {
-                        let mut master_benchmark = BenchmarkRequest::create_master(
-                            tag.expect("Master commit in DB without SHA"),
-                            parent_sha.unwrap(),
-                            pr.unwrap() as u32,
-                            created_at,
-                            status,
-                            backends,
-                            profiles,
-                        );
-                        master_benchmark.completed_at = completed_at;
-                        master_benchmark
-                    }
-                    BENCHMARK_REQUEST_RELEASE_STR => {
-                        let mut release_benchmark = BenchmarkRequest::create_release(
-                            tag.expect("Release commit in DB witohut SHA"),
-                            created_at,
-                            status,
-                            backends,
-                            profiles,
-                        );
-                        release_benchmark.completed_at = completed_at;
-                        release_benchmark
-                    }
-                    _ => panic!(
-                        "Invalid `commit_type` for `BenchmarkRequest` {}",
-                        commit_type
-                    ),
+                            pr: pr.expect("Try commit in the DB without a PR"),
+                        },
+                        created_at,
+                        status,
+                        backends,
+                        profiles,
+                    },
+                    BENCHMARK_REQUEST_MASTER_STR => BenchmarkRequest {
+                        commit_type: BenchmarkRequestType::Master {
+                            sha: tag.expect("Master commit in the DB without a SHA"),
+                            parent_sha: parent_sha
+                                .expect("Master commit in the DB without a parent SHA"),
+                            pr: pr.expect("Master commit in the DB without a PR"),
+                        },
+                        created_at,
+                        status,
+                        backends,
+                        profiles,
+                    },
+                    BENCHMARK_REQUEST_RELEASE_STR => BenchmarkRequest {
+                        commit_type: BenchmarkRequestType::Release {
+                            tag: tag.expect("Release commit in the DB without a SHA"),
+                        },
+                        created_at,
+                        status,
+                        backends,
+                        profiles,
+                    },
+                    _ => panic!("Invalid `commit_type` for `BenchmarkRequest` {commit_type}",),
                 }
             })
             .collect();
@@ -1513,23 +1511,25 @@ where
 
     async fn update_benchmark_request_status(
         &mut self,
-        benchmark_request: &BenchmarkRequest,
-        benchmark_request_status: BenchmarkRequestStatus,
+        request: &BenchmarkRequest,
+        status: BenchmarkRequestStatus,
     ) -> anyhow::Result<()> {
-        let tx = self
-            .conn_mut()
-            .transaction()
-            .await
-            .context("failed to start transaction")?;
-
-        tx.execute(
-            "UPDATE benchmark_request SET status = $1 WHERE tag = $2;",
-            &[&benchmark_request_status, &benchmark_request.tag()],
-        )
-        .await
-        .context("failed to execute UPDATE benchmark_request")?;
+        let tag = request
+            .tag()
+            .expect("Cannot update status of a benchmark request without a tag. Use `attach_shas_to_try_benchmark_request` instead.");
 
-        tx.commit().await.context("failed to commit transaction")?;
+        let status_str = status.as_str();
+        let completed_at = status.completed_at();
+        self.conn()
+            .execute(
+                r#"
+                UPDATE benchmark_request
+                SET status = $1, completed_at = $2
+                WHERE tag = $3;"#,
+                &[&status_str, &completed_at, &tag],
+            )
+            .await
+            .context("failed to benchmark request status update")?;
 
         Ok(())
     }

site/src/job_queue.rs

@@ -129,8 +129,6 @@ async fn create_benchmark_request_releases(
                 &name,
                 date_time,
                 BenchmarkRequestStatus::ArtifactsReady,
-                "",
-                "",
             );
             if let Err(e) = conn.insert_benchmark_request(&release_request).await {
                 log::error!("Failed to insert release benchmark request: {}", e);
@@ -395,8 +393,6 @@ mod tests {
             tag,
             days_ago(age_days),
             BenchmarkRequestStatus::ArtifactsReady,
-            "",
-            "",
         )
     }