feat: add SeaORM entities for database abstraction #43

Merged
marfavi merged 13 commits from feature/issue-33-orm into main 2026-03-27 15:06:47 +01:00
Owner

I have set up the SeaORM database entity files for all our tables and linked them to main.rs. Syntax is checked and passing. Next step will be replacing the raw SQL queries.

Fixes #33

I have set up the SeaORM database entity files for all our tables and linked them to main.rs. Syntax is checked and passing. Next step will be replacing the raw SQL queries. Fixes #33
imaznaseem changed title from feat: add SeaORM entities for database abstraction to WIP: feat: add SeaORM entities for database abstraction 2026-03-14 19:34:39 +01:00
imaznaseem changed title from WIP: feat: add SeaORM entities for database abstraction to feat: add SeaORM entities for database abstraction 2026-03-14 19:34:54 +01:00
imaznaseem changed title from feat: add SeaORM entities for database abstraction to WIP: feat: add SeaORM entities for database abstraction 2026-03-14 19:35:01 +01:00
Replace all raw SQL queries and sqlx pool usage with SeaORM entity API.
imaznaseem changed title from WIP: feat: add SeaORM entities for database abstraction to feat: add SeaORM entities for database abstraction 2026-03-17 12:33:44 +01:00
Owner

This issue required more work than I anticipated so I used Claude Sonnet 4.6 to help rewrite some of the code in domain/services.rs. So a review is definitely needed.

This issue required more work than I anticipated so I used Claude Sonnet 4.6 to help rewrite some of the code in `domain/services.rs`. So a review is definitely needed.
marfavi requested changes 2026-03-17 14:49:58 +01:00
Dismissed
marfavi left a comment

Suggested changes.

Make sure to format all changed files as well.

Suggested changes. Make sure to format all changed files as well.
Cargo.toml Outdated
@ -17,3 +16,4 @@ bcrypt = "0.17"
md-5 = "0.10"
chrono = "0.4"
thiserror = "2"
sea-orm = { version = "0.12", features = ["sqlx-sqlite", "runtime-tokio-rustls", "macros"] }

do we need feature "sqlx-sqlite" when we no longer use sqlx❓ or is it because it uses sqlx under the hood❓ 🤔

do we need feature "sqlx-sqlite" when we no longer use sqlx❓ or is it because it uses sqlx under the hood❓ 🤔
Author
Owner

Yeah, you are re right. SeaORM uses it under the hood as its database driver, so we have to keep this feature enabled.

Yeah, you are re right. SeaORM uses it under the hood as its database driver, so we have to keep this feature enabled.
marfavi marked this conversation as resolved
@ -21,3 +21,3 @@
pub text: String,
pub email: String,
pub pub_date: i64,
pub pub_date: Option<i64>,

pub_date should always be set, why is it changed to an Option here?

`pub_date` should always be set, why is it changed to an Option here?
marfavi marked this conversation as resolved
@ -24,1 +28,4 @@
.filter(user::Column::Username.eq(username))
.one(db)
.await?
.map(|m| m.user_id as i64)

user_id is i32 in the model. either change the return type to i32 or change the model to contain i64. don't cast the type here, though

user_id is i32 in the model. either change the return type to i32 or change the model to contain i64. don't cast the type here, though

I changed it in the model to be type i64, since previously the domain user_id was also i64 and removed the associated casts

I changed it in the model to be type i64, since previously the domain user_id was also i64 and removed the associated casts
@ -33,1 +38,4 @@
.filter(user::Column::Username.eq(username))
.one(db)
.await?
.map(|m| User { user_id: m.user_id as i64, username: m.username, email: m.email })

user_id is i32 in the model, see other comment

user_id is i32 in the model, see other comment
balthazar marked this conversation as resolved
@ -153,0 +167,4 @@
.to(user::Column::UserId)
.into(),
)
.filter(follower::Column::WhoId.eq(user_id as i32))

change parameter type to i32 instead of casting

change parameter type to i32 instead of casting

Fixed as part of fix to last comment

Fixed as part of fix to last comment
marfavi marked this conversation as resolved
@ -153,0 +168,4 @@
.into(),
)
.filter(follower::Column::WhoId.eq(user_id as i32))
.limit(limit as u64)

change parameter type to u64 instead of casting

change parameter type to u64 instead of casting

This one is a bit wonky, so I have moved the limit cast to make it easier for future removal.

This one is a bit wonky, so I have moved the limit cast to make it easier for future removal.
marfavi marked this conversation as resolved
@ -174,2 +192,2 @@
.execute(pool)
.await?;
message::ActiveModel {
author_id: Set(author_id as i32),

change parameter type to i32 instead of casting

change parameter type to i32 instead of casting
balthazar marked this conversation as resolved
@ -245,2 +272,2 @@
if bcrypt::verify(password, &row.pw_hash).unwrap_or(false) {
Ok(row.user_id)
if bcrypt::verify(password, &model.pw_hash).unwrap_or(false) {
Ok(model.user_id as i64)

don't upcast to i64; change return type to i32. im pretty sure our database only stores i32 ints anyway, so it's the more correct thing to do

don't upcast to i64; change return type to i32. im pretty sure our database only stores i32 ints anyway, so it's the more correct thing to do

Previously the user_ids were 64 bit, so changed author_id to match

Previously the user_ids were 64 bit, so changed author_id to match
marfavi marked this conversation as resolved
@ -256,3 +283,3 @@
.map(|row| MessageView {
gravatar: gravatar_url(&row.email, 48),
pub_date: format_datetime(row.pub_date),
pub_date: format_datetime(row.pub_date.unwrap_or(0)),

pub_date should not be an Option

pub_date should not be an Option
balthazar marked this conversation as resolved
src/main.rs Outdated
@ -85,4 +79,1 @@
let database_url = std::env::var("DATABASE").unwrap_or_else(|_| DEFAULT_DATABASE.into());
let options = SqliteConnectOptions::from_str(&database_url)
.expect("Invalid DATABASE URL")
.create_if_missing(true);

Some form of equivalence for create_if_missing is missing.

As a consequence, running make test no longer works because it uses a temporary database file, but because it doesn't exist, Rust panics.

Some form of equivalence for `create_if_missing` is missing. As a consequence, running `make test` no longer works because it uses a temporary database file, but because it doesn't exist, Rust panics.

Actually the easiest way to fix this is to change the docker-compose.test.yml file to use an in-memory database instead of a temporary file:

change DATABASE=sqlite:minitwit_test.db to "DATABASE=sqlite::memory:" (including quotes).

Actually the easiest way to fix this is to change the docker-compose.test.yml file to use an in-memory database instead of a temporary file: change `DATABASE=sqlite:minitwit_test.db` to `"DATABASE=sqlite::memory:"` (including quotes).
marfavi marked this conversation as resolved
src/main.rs Outdated
@ -16,3 +17,4 @@
mod state;
mod web_app;

run the rust formatter on all of your files

run the rust formatter on all of your files
marfavi marked this conversation as resolved
src/main.rs Outdated
@ -111,3 +107,3 @@
// Migrate: add `latest` table if it doesn't exist yet (safe on existing DBs)
sqlx::query(
db.execute_unprepared(

Remove the "migrate"/"seed" code, it is no longer needed

Remove the "migrate"/"seed" code, it is no longer needed
marfavi marked this conversation as resolved
@ -45,3 +48,1 @@
let _ = sqlx::query("UPDATE latest SET value = ?1 WHERE id = 1")
.bind(val)
.execute(&state.pool)
let _ = latest::Entity::update_many()

Using update_many to update one value might be a bit awkward (and not as performance optimised). I suggest doing something like this instead:

      if let Some(val) = query.latest {
        if let Ok(Some(model)) = latest::Entity::find_by_id(1).one(&state.db).await {
            let mut active = latest::ActiveModel::from(model);
            active.value = Set(val as i32);
            let _ = active.update(&state.db).await;
        }
      }

(you would still need to address the i32/i64 comment.)

Using `update_many` to update one value might be a bit awkward (and not as performance optimised). I suggest doing something like this instead: ```rs if let Some(val) = query.latest { if let Ok(Some(model)) = latest::Entity::find_by_id(1).one(&state.db).await { let mut active = latest::ActiveModel::from(model); active.value = Set(val as i32); let _ = active.update(&state.db).await; } } ``` (you would still need to address the i32/i64 comment.)
marfavi marked this conversation as resolved
@ -46,2 +48,2 @@
.bind(val)
.execute(&state.pool)
let _ = latest::Entity::update_many()
.col_expr(latest::Column::Value, Expr::value(val as i32))

val from the query parameters is i64 but val in the model is i32. I do think that we only store i32s in the database, so instead of casting val as i32, we should change the query parameter model (pub struct ApiQuery) to hold i32 instead of i64.

`val` from the query parameters is i64 but `val` in the model is i32. I do think that we only store i32s in the database, so instead of casting `val as i32`, we should change the query parameter model (`pub struct ApiQuery`) to hold `i32` instead of `i64`.
marfavi marked this conversation as resolved
@ -65,1 +69,4 @@
.await
.ok()
.flatten()
.map(|m| m.value as i64)

same concern here with the i64/i32

same concern here with the i64/i32
marfavi marked this conversation as resolved
marfavi force-pushed feature/issue-33-orm from 6d282a6f42 to 50cc4075eb 2026-03-17 14:50:33 +01:00 Compare
marfavi force-pushed feature/issue-33-orm from 50cc4075eb to e74f99dfc6 2026-03-17 18:05:19 +01:00 Compare
balthazar approved these changes 2026-03-20 16:25:36 +01:00
Dismissed
balthazar left a comment

It already looks way nicer with the OMR, I was a bit stumped on the per_page choice, but honestly maybe that is just me? This looks good to me.

It already looks way nicer with the OMR, I was a bit stumped on the per_page choice, but honestly maybe that is just me? This looks good to me.
@ -96,3 +105,1 @@
.bind(limit)
.fetch_all(pool)
.await?;
let limit = limit.unwrap_or(PER_PAGE as i64) as u64;

What is going on here? Earlier we changed PER_PAGE to U64 from I64, but now we are casting it back and forth?

What is going on here? Earlier we changed PER_PAGE to U64 from I64, but now we are casting it back and forth?
marfavi marked this conversation as resolved
balthazar dismissed balthazar's review 2026-03-20 16:26:50 +01:00
Reason:

It coincides with Omids, Omid was more critical than I, and after a second review I concur.

Owner

Resolve conflicts by running git rebase origin/main from your branch

Resolve conflicts by running `git rebase origin/main` from your branch
Replace all raw SQL queries and sqlx pool usage with SeaORM entity API.
Merge branch 'feature/issue-33-orm' of https://git.marfavi.com/DevOpps/minitwit into feature/issue-33-orm
All checks were successful
Lint / Clippy (push) Successful in 2m52s
Lint / Rustfmt (push) Successful in 29s
Lint / Hadolint (push) Successful in 5s
Lint / Clippy (pull_request) Successful in 41s
Lint / Rustfmt (pull_request) Successful in 30s
Lint / Hadolint (pull_request) Successful in 4s
469d3e7f50
use i32 for user ids
Some checks failed
Lint / Clippy (push) Failing after 41s
Lint / Rustfmt (push) Successful in 27s
Lint / Hadolint (push) Successful in 5s
Lint / Clippy (pull_request) Failing after 41s
Lint / Rustfmt (pull_request) Successful in 27s
Lint / Hadolint (pull_request) Successful in 5s
a485e6b448
set pub_date to be an i32
Some checks failed
Lint / Clippy (push) Failing after 43s
Lint / Rustfmt (push) Successful in 28s
Lint / Hadolint (push) Successful in 4s
Lint / Clippy (pull_request) Failing after 41s
Lint / Rustfmt (pull_request) Has been cancelled
Lint / Hadolint (pull_request) Has been cancelled
fb1279f1ac
remove "migrate"/"seed" code in main.rs
Some checks failed
Lint / Clippy (push) Failing after 42s
Lint / Rustfmt (push) Successful in 27s
Lint / Hadolint (push) Successful in 4s
Lint / Clippy (pull_request) Failing after 40s
Lint / Rustfmt (pull_request) Has been cancelled
Lint / Hadolint (pull_request) Has been cancelled
e9a1874966
make pipeline happy
All checks were successful
Lint / Clippy (push) Successful in 43s
Lint / Rustfmt (push) Successful in 28s
Lint / Hadolint (push) Successful in 5s
Lint / Clippy (pull_request) Successful in 51s
Lint / Rustfmt (pull_request) Successful in 30s
Lint / Hadolint (pull_request) Successful in 5s
7ad1e9adf7
ci: restrict static analysis jobs to PR to main branch and combine Rust jobs (#52)
Some checks are pending
Static analysis / Check Rust code with Clippy and rustfmt (pull_request) Waiting to run
Static analysis / Check Dockerfile with Hadolint (pull_request) Waiting to run
74679171f7
Reviewed-on: #52
Reviewed-by: mudiloodi <[email protected]>
Co-authored-by: Omid Marfavi <[email protected]>
Co-committed-by: Omid Marfavi <[email protected]>
Merge branch 'main' into feature/issue-33-orm
All checks were successful
Static analysis / Check Rust code with Clippy and rustfmt (pull_request) Successful in 45s
Static analysis / Check Dockerfile with Hadolint (pull_request) Successful in 5s
489f90c451
marfavi approved these changes 2026-03-27 15:06:22 +01:00
marfavi deleted branch feature/issue-33-orm 2026-03-27 15:06:47 +01:00
Commenting is not possible because the repository is archived.
No reviewers
No milestone
No project
No assignees
4 participants
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
DevOpps/minitwit!43
No description provided.