feat: add SeaORM entities for database abstraction #43
No reviewers
Labels
No labels
operations
simulator api
tests
web app
No milestone
No project
No assignees
4 participants
Due date
No due date set.
Dependencies
No dependencies set
Reference
DevOpps/minitwit!43
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/issue-33-orm"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
feat: add SeaORM entities for database abstractionto WIP: feat: add SeaORM entities for database abstractionWIP: feat: add SeaORM entities for database abstractionto feat: add SeaORM entities for database abstractionfeat: add SeaORM entities for database abstractionto WIP: feat: add SeaORM entities for database abstractionWIP: feat: add SeaORM entities for database abstractionto feat: add SeaORM entities for database abstractionThis 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.Suggested changes.
Make sure to format all changed files as well.
@ -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❓ 🤔
Yeah, you are re right. SeaORM uses it under the hood as its database driver, so we have to keep this feature enabled.
@ -21,3 +21,3 @@pub text: String,pub email: String,pub pub_date: i64,pub pub_date: Option<i64>,pub_dateshould always be set, why is it changed to an Option here?@ -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
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
@ -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
Fixed as part of fix to last comment
@ -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
This one is a bit wonky, so I have moved the limit cast to make it easier for future removal.
@ -174,2 +192,2 @@.execute(pool).await?;message::ActiveModel {author_id: Set(author_id as i32),change parameter type to i32 instead of casting
@ -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
Previously the user_ids were 64 bit, so changed author_id to match
@ -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
@ -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_missingis missing.As a consequence, running
make testno 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.dbto"DATABASE=sqlite::memory:"(including quotes).@ -16,3 +17,4 @@mod state;mod web_app;run the rust formatter on all of your files
@ -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
@ -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_manyto update one value might be a bit awkward (and not as performance optimised). I suggest doing something like this instead:(you would still need to address the i32/i64 comment.)
@ -46,2 +48,2 @@.bind(val).execute(&state.pool)let _ = latest::Entity::update_many().col_expr(latest::Column::Value, Expr::value(val as i32))valfrom the query parameters is i64 butvalin the model is i32. I do think that we only store i32s in the database, so instead of castingval as i32, we should change the query parameter model (pub struct ApiQuery) to holdi32instead ofi64.@ -65,1 +69,4 @@.await.ok().flatten().map(|m| m.value as i64)same concern here with the i64/i32
6d282a6f42to50cc4075eb50cc4075ebtoe74f99dfc6It 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?
It coincides with Omids, Omid was more critical than I, and after a second review I concur.
Resolve conflicts by running
git rebase origin/mainfrom your branch