Merge pull request #187 from storytold/brandon-fix-sqlx-query-builder

Fix QueryBuilder with respect to column names and type juggling

crates/_types/enums/src/by_table/media_files/media_file_origin_category.rs

@@ -1,4 +1,5 @@
 use std::collections::BTreeSet;
+
 use serde::Deserialize;
 use serde::Serialize;
 #[cfg(test)]
@@ -29,6 +30,7 @@ pub enum MediaFileOriginCategory {
 // TODO(bt, 2022-12-21): This desperately needs MySQL integration tests!
 impl_enum_display_and_debug_using_to_str!(MediaFileOriginCategory);
 impl_mysql_enum_coders!(MediaFileOriginCategory);
+impl_mysql_from_row!(MediaFileOriginCategory);
 
 /// NB: Legacy API for older code.
 impl MediaFileOriginCategory {

crates/_types/enums/src/by_table/media_files/media_file_origin_model_type.rs

@@ -24,6 +24,7 @@ pub enum MediaFileOriginModelType {
   /// so-vits-svc voice conversion models
   #[serde(rename = "so_vits_svc")]
   SoVitsSvc,
+
   #[serde(rename = "vall_e_x")]
   VallEX,
 }
@@ -31,6 +32,7 @@ pub enum MediaFileOriginModelType {
 // TODO(bt, 2022-12-21): This desperately needs MySQL integration tests!
 impl_enum_display_and_debug_using_to_str!(MediaFileOriginModelType);
 impl_mysql_enum_coders!(MediaFileOriginModelType);
+impl_mysql_from_row!(MediaFileOriginModelType);
 
 /// NB: Legacy API for older code.
 impl MediaFileOriginModelType {

crates/_types/enums/src/by_table/media_files/media_file_origin_product_category.rs

@@ -39,6 +39,7 @@ pub enum MediaFileOriginProductCategory {
 // TODO(bt, 2022-12-21): This desperately needs MySQL integration tests!
 impl_enum_display_and_debug_using_to_str!(MediaFileOriginProductCategory);
 impl_mysql_enum_coders!(MediaFileOriginProductCategory);
+impl_mysql_from_row!(MediaFileOriginProductCategory);
 
 /// NB: Legacy API for older code.
 impl MediaFileOriginProductCategory {

crates/_types/enums/src/by_table/media_files/media_file_type.rs

@@ -25,6 +25,7 @@ pub enum MediaFileType {
 // TODO(bt, 2022-12-21): This desperately needs MySQL integration tests!
 impl_enum_display_and_debug_using_to_str!(MediaFileType);
 impl_mysql_enum_coders!(MediaFileType);
+impl_mysql_from_row!(MediaFileType);
 
 /// NB: Legacy API for older code.
 impl MediaFileType {

crates/_types/enums/src/common/visibility.rs

@@ -38,6 +38,7 @@ pub enum Visibility {
 
 
 impl_enum_display_and_debug_using_to_str!(Visibility);
+impl_mysql_from_row!(Visibility);
 
 // For reference, here's what the serde implementation might be if manually written.
 // This may be useful for designing composite types in the future:

crates/_types/enums/src/lib.rs

@@ -32,3 +32,4 @@ mod macros;
 
 pub mod by_table;
 pub mod common;
+pub mod traits;

crates/_types/enums/src/macros/impl_mysql_from_row.rs

@@ -0,0 +1,63 @@
+
+/// Implement `MySqlFromRow` on a type.
+macro_rules! impl_mysql_from_row {
+  ($t:ident) => {
+
+    // Try to convert a MySQL row and named field into the value type (for non-nullable fields).
+    impl crate::traits::mysql_from_row::MySqlFromRow<$t> for $t {
+      fn try_from_mysql_row(row: &sqlx_mysql::MySqlRow, field_name: &str) -> Result<$t, sqlx::Error> {
+        use sqlx::{Error, Row};
+
+        // NB(bt,2023-12-05): For now only string encodings are considered.
+        // We may want to revisit in the future if we deal with binary data.
+        let value : String = row.try_get(field_name)?;
+
+        let output_type = Self::from_str(&value)
+            .map_err(|err| {
+              Error::ColumnDecode {
+                index: format!("mysql_from_row failure on field {}: {:?}", field_name, err),
+                source: format!("mysql_from_row failure on field {}: {:?}", field_name, err).into(),
+              }
+            })?;
+
+        Ok(output_type)
+      }
+
+      // Try to convert a MySQL row and named field into the value type (for nullable fields).
+      fn try_from_mysql_row_nullable(row: &sqlx_mysql::MySqlRow, field_name: &str) -> Result<Option<$t>, sqlx::Error> {
+        use sqlx::{Error, Row};
+
+        // NB(bt,2023-12-05): For now only string encodings are considered.
+        // We may want to revisit in the future if we deal with binary data.
+        // NB(2): Nullable fields decode as Option<T>.
+        let maybe_value : Option<String> = row.try_get(field_name)?;
+
+        let value = match maybe_value {
+          Some(value) => value,
+          None => return Ok(None),
+        };
+
+        let output_type = Self::from_str(&value)
+            .map_err(|err| {
+              Error::ColumnDecode {
+                index: format!("mysql_from_row failure on field {}: {:?}", field_name, err),
+                source: format!("mysql_from_row failure on field {}: {:?}", field_name, err).into(),
+              }
+            })?;
+
+        Ok(Some(output_type))
+      }
+    }
+
+    //// TODO: Tests.
+    //#[cfg(test)]
+    //#[test]
+    //fn test_name() {
+    //  use strum::IntoEnumIterator;
+    //  for variant in $t::iter() {
+    //    assert_eq!(format!("{}", variant), variant.to_str());
+    //  }
+    //}
+
+  }
+}

crates/_types/enums/src/macros/mod.rs

@@ -1,3 +1,4 @@
 #[macro_use] pub mod impl_enum_display_and_debug_using_to_str;
 #[macro_use] pub mod impl_mysql_enum_coders;
+#[macro_use] pub mod impl_mysql_from_row;
 #[macro_use] pub mod impl_sqlite_enum_coders;

crates/_types/enums/src/traits/mod.rs

@@ -0,0 +1 @@
+pub mod mysql_from_row;

crates/_types/enums/src/traits/mysql_from_row.rs

@@ -0,0 +1,11 @@
+use sqlx_mysql::MySqlRow;
+
+/// This is a trait (in conjunction with the macro implementation) that makes up for sqlx's
+/// QueryBuilder not playing well with FromRow implementation or derivation of named types.
+pub trait MySqlFromRow<T> {
+
+  /// Attempt to parse a named column from a MySqlRow into the type.
+  fn try_from_mysql_row(row: &MySqlRow, field_name: &str) -> Result<T, sqlx::Error>;
+
+  fn try_from_mysql_row_nullable(row: &MySqlRow, field_name: &str) -> Result<Option<T>, sqlx::Error>;
+}

crates/lib/mysql_queries/src/queries/media_files/list_media_files_for_user.rs

@@ -1,11 +1,13 @@
 use chrono::{DateTime, Utc};
-use sqlx::{MySql, MySqlPool, QueryBuilder};
+use sqlx::{FromRow, MySql, MySqlPool, QueryBuilder, Row};
+use sqlx::mysql::MySqlRow;
 
 use enums::by_table::media_files::media_file_origin_category::MediaFileOriginCategory;
 use enums::by_table::media_files::media_file_origin_model_type::MediaFileOriginModelType;
 use enums::by_table::media_files::media_file_origin_product_category::MediaFileOriginProductCategory;
 use enums::by_table::media_files::media_file_type::MediaFileType;
 use enums::common::visibility::Visibility;
+use enums::traits::mysql_from_row::MySqlFromRow;
 use errors::AnyhowResult;
 use tokens::tokens::media_files::MediaFileToken;
 
@@ -119,21 +121,21 @@ fn query_builder<'a>(
     r#"
 SELECT
   m.id,
-  m.token as `token: tokens::tokens::media_files::MediaFileToken`,
+  m.token,
 
-  m.origin_category as `origin_category: enums::by_table::media_files::media_file_origin_category::MediaFileOriginCategory`,
-  m.origin_product_category as `origin_product_category: enums::by_table::media_files::media_file_origin_product_category::MediaFileOriginProductCategory`,
+  m.origin_category,
+  m.origin_product_category,
 
   m.maybe_origin_model_type,
   m.maybe_origin_model_token,
 
-  m.media_type as `media_type: enums::by_table::media_files::media_file_type::MediaFileType`,
+  m.media_type,
 
   m.public_bucket_directory_hash,
   m.maybe_public_bucket_prefix,
   m.maybe_public_bucket_extension,
 
-  m.creator_set_visibility as `creator_set_visibility: enums::common::visibility::Visibility`,
+  m.creator_set_visibility,
   m.created_at,
   m.updated_at
 
@@ -141,24 +143,30 @@ FROM media_files AS m
 LEFT OUTER JOIN users AS u
     ON m.maybe_creator_user_token = u.token
 
-WHERE u.username = ?
-AND m.user_deleted_at IS NULL
-AND m.mod_deleted_at IS NULL
+WHERE m.user_deleted_at IS NULL
+  AND m.mod_deleted_at IS NULL
     "#
   );
 
+  query_builder.push(" AND u.username = ");
   query_builder.push_bind(username);
 
   if let Some(media_type) = maybe_filter_media_type {
-    query_builder.push(" AND m.media_file_type = ? ");
-    query_builder.push_bind(media_type);
+    // FIXME: Binding shouldn't require to_str().
+    //  Otherwise, it's calling the Display trait on the raw type which is resulting in an
+    //  incorrect binding and runtime error.
+    query_builder.push(" AND m.media_file_type = ");
   }
 
   match view_as {
     ViewAs::Author => {}
     ViewAs::Moderator => {}
     ViewAs::AnotherUser => {
-      query_builder.push(" AND m.creator_set_visibility = 'public' ");
+      // FIXME: Binding shouldn't require to_str().
+      //  Otherwise, it's calling the Display trait on the raw type which is resulting in an
+      //  incorrect binding and runtime error.
+      query_builder.push(" AND m.creator_set_visibility = ");
+      query_builder.push_bind(Visibility::Public.to_str());
     }
   }
 
@@ -181,7 +189,6 @@ AND m.mod_deleted_at IS NULL
   query_builder
 }
 
-#[derive(sqlx::FromRow)]
 struct MediaFileListItemInternal {
   id: i64,
   token: MediaFileToken,
@@ -201,3 +208,35 @@ struct MediaFileListItemInternal {
   created_at: DateTime<Utc>,
   updated_at: DateTime<Utc>,
 }
+
+// NB(bt,2023-12-05): There's an issue with type hinting in the `as` clauses with QueryBuilder (or
+// raw query strings) and sqlx::FromRow, regardless of whether it is derived of manually
+// implemented. Perhaps this will improve in the future, but for now manually constructed queries
+// cannot have type hints, eg. the following:
+//
+//    m.token as `token: tokens::tokens::media_files::MediaFileToken`,
+//    m.origin_category as `origin_category: enums::by_table::media_files::media_file_origin_category::MediaFileOriginCategory`,
+//    m.creator_set_visibility as `creator_set_visibility: enums::common::visibility::Visibility`,
+//
+// This results in the automatic mapping not being able to be found by name (for macro derive), and
+// in the manual case `row.try_get()` etc. won't have the correct column name (since the name is the
+// full "as" clause).
+impl FromRow<'_, MySqlRow> for MediaFileListItemInternal {
+  fn from_row(row: &MySqlRow) -> Result<Self, sqlx::Error> {
+    Ok(Self {
+      id: row.try_get("id")?,
+      token: MediaFileToken::new(row.try_get("token")?),
+      origin_category: MediaFileOriginCategory::try_from_mysql_row(row, "origin_category")?,
+      origin_product_category: MediaFileOriginProductCategory::try_from_mysql_row(row, "origin_product_category")?,
+      maybe_origin_model_type: MediaFileOriginModelType::try_from_mysql_row_nullable(row, "maybe_origin_model_type")?,
+      maybe_origin_model_token: row.try_get("maybe_origin_model_token")?,
+      media_type: MediaFileType::try_from_mysql_row(row, "media_type")?,
+      public_bucket_directory_hash: row.try_get("public_bucket_directory_hash")?,
+      maybe_public_bucket_prefix: row.try_get("maybe_public_bucket_prefix")?,
+      maybe_public_bucket_extension: row.try_get("maybe_public_bucket_extension")?,
+      creator_set_visibility: Visibility::try_from_mysql_row(row, "creator_set_visibility")?,
+      created_at: row.try_get("created_at")?,
+      updated_at: row.try_get("updated_at")?,
+    })
+  }
+}