dsync icon indicating copy to clipboard operation
dsync copied to clipboard

Implement generator for `impl Default`

Open SamuelMarks opened this issue 1 year ago • 10 comments

SamuelMarks avatar Nov 27 '24 22:11 SamuelMarks

@Wulf Something weird is happening on my latest commit:

if update_struct.has_code() {
    ret_buffer.push('\n');
    ret_buffer.push_str(update_struct.code());
    if config.options.default_impl {
        ret_buffer.push('\n');
        ret_buffer.push_str(
            build_default_impl_fn(
                &format!("Update{struct_name}"),
                &update_struct.table.columns,
            )
            .as_str(),
        );
    }
    ret_buffer.push('\n');
}

The update_struct.table.columns has column names which are only present in the other structs; but not the update one. Which table should I use?

SamuelMarks avatar Nov 28 '24 03:11 SamuelMarks

btw, we dont have a CONTRIBUTING currently, but the commit style is angular/conventional-ish, see the commit history for examples.

hasezoey avatar Nov 28 '24 12:11 hasezoey

Good suggestions. Yeah I still need to add tests, but it should be ready now (aside from that).

SamuelMarks avatar Nov 28 '24 14:11 SamuelMarks

@hasezoey Hmm maybe I'm misusing something, generated_columns = [] always in generate_for_table of src/code.rs. The rest of the code works well though.

let generated_columns = table_options.get_autogenerated_columns();
println!("generated_columns = {generated_columns:#?}");

SamuelMarks avatar Sep 06 '25 03:09 SamuelMarks

Code looks mostly OK, but most importantly, tests need to be updated and new tests added for this new option.

Hmm maybe I'm misusing something, generated_columns = [] always in generate_for_table of src/code.rs. The rest of the code works well though.

I dont know how you are using it, but that will always be empty unless you specify the generated columns in TableOptions::autogenerated_columns / --autogenerated-columns on the CLI (MainOptions).

Yeah I am using those though. Maybe I'll try constructing a test so you can see.

SamuelMarks avatar Sep 06 '25 17:09 SamuelMarks

@hasezoey Darn… trying to get Default for Update to work and it's not working:

Attempt 0:

build_default_impl_fn(
                    &StructType::format(&StructType::Update, &struct_name),
                    update_struct
                        .table
                        .columns
                        .iter()

Attempt 1:

build_default_impl_fn(
                    &StructType::format(&StructType::Update, &struct_name),
                    update_struct
                        .fields()

Both attempts give the wrong fields, basically:

/// Update Struct for a row in table `users` for [`Users`]
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::AsChangeset, PartialEq)]
#[diesel(table_name=users)]
pub struct UpdateUsers {
    /// Field representing column `password_hash`
    pub password_hash: Option<String>,
    /// Field representing column `role`
    pub role: Option<String>,
    /// Field representing column `created_at`
    pub created_at: Option<chrono::NaiveDateTime>,
}

impl Default for UpdateUsers {
  fn default() -> Self {
    Self {
        username: String::new(),
        password_hash: String::new(),
        role: String::new()
    }
  }
}

FWIW: The other default is correct

/// Create Struct for a row in table `users` for [`Users`]
#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Insertable)]
#[diesel(table_name=users)]
pub struct CreateUsers {
    /// Field representing column `username`
    pub username: String,
    /// Field representing column `password_hash`
    pub password_hash: String,
    /// Field representing column `role`
    pub role: String,
}

impl Default for CreateUsers {
  fn default() -> Self {
    Self {
        username: String::new(),
        password_hash: String::new(),
        role: String::new()
    }
  }
}

So clearly the columns are not being properly set for this struct. How do I get the correctly filtered columns for the given table?

SamuelMarks avatar Sep 07 '25 01:09 SamuelMarks

Darn… trying to get Default for Update to work and it's not working:

I dont know what you are testing against as you still did not provide a test case with this new option, and with my slight refactor and previous suggestion, i dont see any problem, see the attached patch.

Patch that moves the generation into Struct::render and adds a test

To apply this patch put it into file "DIFF.patch" and run git am <DIFF.patch

Alternatively, i think i could also push this change directly into this PR if wanted.

From f9a3c50f2f2b09b2928c64152f7090df53bd9836 Mon Sep 17 00:00:00 2001
From: hasezoey <[email protected]>
Date: Sun, 7 Sep 2025 13:45:50 +0200
Subject: [PATCH] refactor(code): move "default_impl" code to "Struct::render"

And add test for it
---
 src/code.rs                                 | 118 ++++-----------
 test/default_impl/Cargo.toml                |  18 +++
 test/default_impl/lib.rs                    |   6 +
 test/default_impl/models/mod.rs             |   1 +
 test/default_impl/models/todos/generated.rs | 151 ++++++++++++++++++++
 test/default_impl/models/todos/mod.rs       |   2 +
 test/default_impl/schema.rs                 |  16 +++
 test/default_impl/test.sh                   |   8 ++
 8 files changed, 233 insertions(+), 87 deletions(-)
 create mode 100644 test/default_impl/Cargo.toml
 create mode 100644 test/default_impl/lib.rs
 create mode 100644 test/default_impl/models/mod.rs
 create mode 100644 test/default_impl/models/todos/generated.rs
 create mode 100644 test/default_impl/models/todos/mod.rs
 create mode 100644 test/default_impl/schema.rs
 create mode 100755 test/default_impl/test.sh

diff --git a/src/code.rs b/src/code.rs
index f469c87..56f41b6 100644
--- a/src/code.rs
+++ b/src/code.rs
@@ -228,7 +228,7 @@ impl<'a> Struct<'a> {
                     derives_vec.push(derives::PARTIALEQ);
                 }
 
-                /*if !self.config.options.default_impl*/ {
+                if !self.config.options.default_impl {
                     derives_vec.push(derives::DEFAULT);
                 }
             }
@@ -299,7 +299,7 @@ impl<'a> Struct<'a> {
             .collect::<Vec<String>>()
             .join(" ");
 
-        let fields = self.fields();
+        let mut fields = self.fields();
 
         if fields.is_empty() {
             self.has_fields = Some(false);
@@ -332,18 +332,18 @@ impl<'a> Struct<'a> {
         };
 
         let mut lines = Vec::with_capacity(fields.len());
-        for mut f in fields.into_iter() {
+        for f in fields.iter_mut() {
             let field_name = &f.name;
 
             if f.base_type == "String" {
                 f.base_type = match self.ty {
-                    StructType::Read => f.base_type,
+                    StructType::Read => f.base_type.clone(),
                     StructType::Update => self.opts.get_update_str_type().as_str().to_string(),
                     StructType::Create => self.opts.get_create_str_type().as_str().to_string(),
                 }
             } else if f.base_type == "Vec<u8>" {
                 f.base_type = match self.ty {
-                    StructType::Read => f.base_type,
+                    StructType::Read => f.base_type.clone(),
                     StructType::Update => self.opts.get_update_bytes_type().as_str().to_string(),
                     StructType::Create => self.opts.get_create_bytes_type().as_str().to_string(),
                 }
@@ -382,7 +382,7 @@ impl<'a> Struct<'a> {
             ),
         };
 
-        let struct_code = formatdoc!(
+        let mut struct_code = formatdoc!(
             r#"
             {doccomment}
             {tsync_attr}{derive_attr}
@@ -409,6 +409,14 @@ impl<'a> Struct<'a> {
             lines = lines.join("\n"),
         );
 
+        if self.config.options.default_impl {
+            struct_code.push('\n');
+            struct_code.push_str(&build_default_impl_fn(
+                &ty.format(&table.struct_name),
+                &fields,
+            ));
+        }
+
         self.has_fields = Some(true);
         self.rendered_code = Some(struct_code);
     }
@@ -784,51 +792,40 @@ fn default_for_type(typ: &str) -> &'static str {
     }
 }
 
-struct NameTypNullable<'a> {
-    name: String,
-    typ: &'a str,
-    nullable: bool,
-}
-
 /// Generate default (insides of the `impl Default for StructName { fn default() -> Self {} }`)
-fn build_default_impl_fn<'a>(
-    struct_name: &str,
-    column_name_type_nullable: impl Iterator<Item = NameTypNullable<'a>>,
-) -> String {
-    let fields_to_defaults = column_name_type_nullable
+fn build_default_impl_fn<'a>(struct_name: &str, fields: &[StructField]) -> String {
+    let fields: Vec<String> = fields
+        .iter()
         .map(|name_typ_nullable| {
             format!(
-                "        {name}: {typ_default}",
+                "{name}: {typ_default},",
                 name = name_typ_nullable.name,
-                typ_default = if name_typ_nullable.nullable {
+                typ_default = if name_typ_nullable.is_optional {
                     "None"
                 } else {
-                    default_for_type(name_typ_nullable.typ)
+                    default_for_type(&name_typ_nullable.base_type)
                 }
             )
         })
-        .collect::<Vec<String>>()
-        .join(",\n");
+        .collect();
     formatdoc!(
-        r#"impl Default for {struct_name} {{
-  fn default() -> Self {{
-    Self {{
-{fields_to_defaults}
-    }}
-  }}
-}}"#
+        r#"
+        impl Default for {struct_name} {{
+            fn default() -> Self {{
+                Self {{
+                    {fields}
+                }}
+            }}
+        }}
+        "#,
+        fields = fields.join("\n            ")
     )
 }
 
 /// Generate a full file for a given diesel table
 pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) -> String {
     // early to ensure the table options are set for the current table
-    let struct_name = &table.struct_name;
     let table_options = config.table(&table.name.to_string());
-    let generated_columns = table_options.get_autogenerated_columns();
-    let not_generated = |col: &&ParsedColumnMacro| -> bool {
-        !generated_columns.contains(&col.column_name.as_str())
-    };
 
     let mut ret_buffer = format!("{FILE_SIGNATURE}\n\n");
 
@@ -844,25 +841,6 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) -
     if create_struct.has_code() {
         ret_buffer.push('\n');
         ret_buffer.push_str(create_struct.code());
-        if config.options.default_impl {
-            ret_buffer.push('\n');
-            ret_buffer.push_str(
-                build_default_impl_fn(
-                    &StructType::format(&StructType::Create, &struct_name),
-                    create_struct
-                        .table
-                        .columns
-                        .iter()
-                        .filter(not_generated)
-                        .map(|col| NameTypNullable {
-                            name: col.column_name.to_string(),
-                            typ: col.ty.as_str(),
-                            nullable: col.is_nullable,
-                        }),
-                )
-                .as_str(),
-            );
-        }
     }
 
     let update_struct = Struct::new(StructType::Update, table, config);
@@ -870,25 +848,6 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) -
     if update_struct.has_code() {
         ret_buffer.push('\n');
         ret_buffer.push_str(update_struct.code());
-        /*if config.options.default_impl {
-            ret_buffer.push('\n');
-            ret_buffer.push_str(
-                build_default_impl_fn(
-                    &StructType::format(&StructType::Update, &struct_name),
-                    update_struct
-                        .table
-                        .columns
-                        .iter()
-                        .filter(not_generated)
-                        .map(|col| NameTypNullable {
-                            name: col.column_name.to_string(),
-                            typ: col.ty.as_str(),
-                            nullable: col.is_nullable,
-                        }),
-                )
-                .as_str(),
-            );
-        }*/
     }
 
     // third, push functions - if enabled
@@ -897,20 +856,5 @@ pub fn generate_for_table(table: &ParsedTableMacro, config: &GenerationConfig) -
         ret_buffer.push_str(build_table_fns(table, config, create_struct, update_struct).as_str());
     }
 
-    if config.options.default_impl {
-        ret_buffer.push('\n');
-        ret_buffer.push_str(
-            build_default_impl_fn(
-                &struct_name,
-                table.columns.iter().map(|col| NameTypNullable {
-                    name: col.name.to_string().to_owned(),
-                    typ: col.ty.as_str(),
-                    nullable: col.is_nullable,
-                }),
-            )
-            .as_str(),
-        );
-    }
-
     ret_buffer
 }
diff --git a/test/default_impl/Cargo.toml b/test/default_impl/Cargo.toml
new file mode 100644
index 0000000..c8b0b90
--- /dev/null
+++ b/test/default_impl/Cargo.toml
@@ -0,0 +1,18 @@
+[lib]
+path = "lib.rs"
+
+[package]
+name = "default_impl"
+version = "0.1.0"
+edition = "2021"
+
+[dependencies]
+diesel = { version = "*", default-features = false, features = [
+    "sqlite",
+    "r2d2",
+    "chrono",
+    "returning_clauses_for_sqlite_3_35",
+] }
+r2d2.workspace = true
+chrono.workspace = true
+serde.workspace = true
diff --git a/test/default_impl/lib.rs b/test/default_impl/lib.rs
new file mode 100644
index 0000000..fdea3f5
--- /dev/null
+++ b/test/default_impl/lib.rs
@@ -0,0 +1,6 @@
+pub mod models;
+pub mod schema;
+
+pub mod diesel {
+    pub use diesel::*;
+}
diff --git a/test/default_impl/models/mod.rs b/test/default_impl/models/mod.rs
new file mode 100644
index 0000000..015a6a2
--- /dev/null
+++ b/test/default_impl/models/mod.rs
@@ -0,0 +1 @@
+pub mod todos;
diff --git a/test/default_impl/models/todos/generated.rs b/test/default_impl/models/todos/generated.rs
new file mode 100644
index 0000000..aac3981
--- /dev/null
+++ b/test/default_impl/models/todos/generated.rs
@@ -0,0 +1,151 @@
+/* @generated and managed by dsync */
+
+#[allow(unused)]
+use crate::diesel::*;
+use crate::schema::*;
+
+pub type ConnectionType = diesel::r2d2::PooledConnection<diesel::r2d2::ConnectionManager<diesel::sqlite::SqliteConnection>>;
+
+/// Struct representing a row in table `todos`
+#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Queryable, diesel::Selectable, diesel::QueryableByName, PartialEq, diesel::Identifiable)]
+#[diesel(table_name=todos, primary_key(id))]
+pub struct Todos {
+    /// Field representing column `id`
+    pub id: i32,
+    /// Field representing column `text`
+    pub text: String,
+    /// Field representing column `completed`
+    pub completed: bool,
+    /// Field representing column `type`
+    pub type_: String,
+    /// Field representing column `smallint`
+    pub smallint: i16,
+    /// Field representing column `bigint`
+    pub bigint: i64,
+    /// Field representing column `created_at`
+    pub created_at: chrono::NaiveDateTime,
+    /// Field representing column `updated_at`
+    pub updated_at: chrono::NaiveDateTime,
+}
+
+impl Default for Todos {
+    fn default() -> Self {
+        Self {
+            id: 0,
+            text: String::new(),
+            completed: false,
+            type_: String::new(),
+            smallint: 0,
+            bigint: 0,
+            created_at: Default::default(),
+            updated_at: Default::default(),
+        }
+    }
+}
+
+/// Create Struct for a row in table `todos` for [`Todos`]
+#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::Insertable)]
+#[diesel(table_name=todos)]
+pub struct CreateTodos {
+    /// Field representing column `text`
+    pub text: String,
+    /// Field representing column `completed`
+    pub completed: bool,
+    /// Field representing column `type`
+    pub type_: String,
+    /// Field representing column `smallint`
+    pub smallint: i16,
+    /// Field representing column `bigint`
+    pub bigint: i64,
+}
+
+impl Default for CreateTodos {
+    fn default() -> Self {
+        Self {
+            text: String::new(),
+            completed: false,
+            type_: String::new(),
+            smallint: 0,
+            bigint: 0,
+        }
+    }
+}
+
+/// Update Struct for a row in table `todos` for [`Todos`]
+#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, diesel::AsChangeset, PartialEq)]
+#[diesel(table_name=todos)]
+pub struct UpdateTodos {
+    /// Field representing column `text`
+    pub text: Option<String>,
+    /// Field representing column `completed`
+    pub completed: Option<bool>,
+    /// Field representing column `type`
+    pub type_: Option<String>,
+    /// Field representing column `smallint`
+    pub smallint: Option<i16>,
+    /// Field representing column `bigint`
+    pub bigint: Option<i64>,
+    /// Field representing column `created_at`
+    pub created_at: Option<chrono::NaiveDateTime>,
+    /// Field representing column `updated_at`
+    pub updated_at: Option<chrono::NaiveDateTime>,
+}
+
+impl Default for UpdateTodos {
+    fn default() -> Self {
+        Self {
+            text: String::new(),
+            completed: false,
+            type_: String::new(),
+            smallint: 0,
+            bigint: 0,
+            created_at: Default::default(),
+            updated_at: Default::default(),
+        }
+    }
+}
+
+/// Result of a `.paginate` function
+#[derive(Debug, serde::Serialize)]
+pub struct PaginationResult<T> {
+    /// Resulting items that are from the current page
+    pub items: Vec<T>,
+    /// The count of total items there are
+    pub total_items: i64,
+    /// Current page, 0-based index
+    pub page: i64,
+    /// Size of a page
+    pub page_size: i64,
+    /// Number of total possible pages, given the `page_size` and `total_items`
+    pub num_pages: i64,
+}
+
+impl Todos {
+    /// Insert a new row into `todos` with a given [`CreateTodos`]
+    pub fn create(db: &mut ConnectionType, item: &CreateTodos) -> diesel::QueryResult<Self> {
+        use crate::schema::todos::dsl::*;
+
+        diesel::insert_into(todos).values(item).get_result::<Self>(db)
+    }
+
+    /// Get a row from `todos`, identified by the primary key
+    pub fn read(db: &mut ConnectionType, param_id: i32) -> diesel::QueryResult<Self> {
+        use crate::schema::todos::dsl::*;
+
+        todos.filter(id.eq(param_id)).first::<Self>(db)
+    }
+
+    /// Update a row in `todos`, identified by the primary key with [`UpdateTodos`]
+    pub fn update(db: &mut ConnectionType, param_id: i32, item: &UpdateTodos) -> diesel::QueryResult<Self> {
+        use crate::schema::todos::dsl::*;
+
+        diesel::update(todos.filter(id.eq(param_id))).set(item).get_result(db)
+    }
+
+    /// Delete a row in `todos`, identified by the primary key
+    pub fn delete(db: &mut ConnectionType, param_id: i32) -> diesel::QueryResult<usize> {
+        use crate::schema::todos::dsl::*;
+
+        diesel::delete(todos.filter(id.eq(param_id))).execute(db)
+    }
+}
diff --git a/test/default_impl/models/todos/mod.rs b/test/default_impl/models/todos/mod.rs
new file mode 100644
index 0000000..a5bb9b9
--- /dev/null
+++ b/test/default_impl/models/todos/mod.rs
@@ -0,0 +1,2 @@
+pub use generated::*;
+pub mod generated;
diff --git a/test/default_impl/schema.rs b/test/default_impl/schema.rs
new file mode 100644
index 0000000..c91a5c4
--- /dev/null
+++ b/test/default_impl/schema.rs
@@ -0,0 +1,16 @@
+diesel::table! {
+    todos (id) {
+        id -> Int4,
+        // unsigned -> Unsigned<Integer>,
+        // unsigned_nullable -> Nullable<Unsigned<Integer>>,
+        text -> Text,
+        completed -> Bool,
+        #[sql_name = "type"]
+        #[max_length = 255]
+        type_ -> Varchar,
+        smallint -> Int2,
+        bigint -> Int8,
+        created_at -> Timestamp,
+        updated_at -> Timestamp,
+    }
+}
diff --git a/test/default_impl/test.sh b/test/default_impl/test.sh
new file mode 100755
index 0000000..7c33aaf
--- /dev/null
+++ b/test/default_impl/test.sh
@@ -0,0 +1,8 @@
+#!/bin/bash
+
+SCRIPT_DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )"
+
+cd $SCRIPT_DIR
+
+cargo run --manifest-path ../../Cargo.toml -- \
+-i schema.rs -o models -g id -g created_at -g updated_at --default-impl -c "diesel::r2d2::PooledConnection<diesel::r2d2::ConnectionManager<diesel::sqlite::SqliteConnection>>"
-- 
2.51.0

hasezoey avatar Sep 07 '25 11:09 hasezoey

@Wulf Thanks. Made an edit to handle the Update properly, what do you think now, can we get this merged in? - Then the other PR?

Then I've got some big ideas to try, but likely beyond dsync scope…

SamuelMarks avatar Sep 08 '25 17:09 SamuelMarks

@hasezoey Can you help this get over the line?

Then the other PR? - Then I want to send a new PR to implement From<CreateStruct> for UpdateStruct

SamuelMarks avatar Sep 17 '25 18:09 SamuelMarks

Can you help this get over the line?

I dont know what to help you with aside from repeating what i said earlier in this PR (also applies to your other PR):

the tests still need to be updated.

In case you had not seen or done it before in this project, to update the tests you need to follow these steps (relative to the project root):

  • run ./test/test_all.sh
    • on error, fix the error, then re-run until no error (compile test of the tests)
  • review the changes generated
  • commit the changes
  • push the changes
  • review the CI result

Also due to the amount of changes the PartialEq will generate, i would like to have that as a separate PR

hasezoey avatar Sep 18 '25 09:09 hasezoey