Running Postgres version for a postgres branch?

[FabianAster]

Somehow it was a bit of debugging to get it running with postgres, might be worth to create a branch for it.
Im new to axum and sqlx, so it might also be some error on my side, but I couldn't get it to work with u64 ids.

Also I'm not sure if there is a way for it, but I couldn't propose the PR to go into a new branch, so I chose main

[thanipro]

Thanks for taking time to work on this. I never really tested it with Postgres and might have overlooked the amount of changes required to get it to work. I also agree with you that we should have this on a separate branch, and we can update the README so people can easily locate the Postgres version.​​​​​​​​​​​​​​​​

[thanipro]

Somehow it was a bit of debugging to get it running with postgres, might be worth to create a branch for it. Im new to axum and sqlx, so it might also be some error on my side, but I couldn't get it to work with u64 ids.

Also I'm not sure if there is a way for it, but I couldn't propose the PR to go into a new branch, so I chose main

@FabianAster is this fine to merge ? should we change the target branch to point to a postgres named branch

[FabianAster]

Yes its ready, we should also change the target branch. But i dont think i have the rights to do that.

[admwrd]

I saw this repo mentioned by the owner on Reddit. Thought I'd take a look and noticed a Postgresql branch discussion; so offering advice. Feel free to use or discard as you see fit :)

[admwrd]

There's no need to do RETURNING id and then immediately follow up with a SELET * FROM user WHERE id = $1.

Instead, leave it as the query_as! macro with the User struct passed, and do a RETURNING * with fetch_one.

Something like:

sqlx::query_as!(User, r#"
    INSERT INTO user (first_name, last_name, user_name, email, password, is_active)
    VALUES ($1, $2, $3, $4, $5, $6) 
    RETURNING * "#,
            payload.first_name,
            payload.last_name,
            payload.user_name,
            payload.email,
            bcrypt::hash(payload.password, 4).unwrap(),
            true
)
.fetch_one(self.db_conn.get_pool())
.await
.map_err(|err| {
// Handle things like existing username, existing email, invalid input for names or passwords
})

Also, we shouldn't be doing an unwrap, even if that's in the original branch. We definitely shouldn't be doing an unwrap in our query statement.

We need to handle the Error result for bcrypt::hash earlier and error out in case we get an error.

let hashed_password = bcrypt::hash(payload.password, 4).map_err(|err| {
    log::warning!("Cannot hash password due to {:?}", err);
    err
})?;
// sqlx stuff, but use `hashed_password` instead, now.

[admwrd]

This Result error type is lossy.I We have several potential errors that can occur, not just from sqlx.

I think it would be better to either use something like anyhow or create a new enum that can hold both the sqlx errors and the bcrypt errors.

pub enum UserCreationError {
    Sqlx(SqlxError),
    Bcrypt(BcryptError)
}

[admwrd]

You could leave the default as CURRENT_TIMESTAMP, but it's more common to use NOW()

[admwrd]

It's best to avoid reserved words likeuser for tables and fields. You could make this users to avoid that, and it would also fit since it is a table storing a list of users. The debate between singular and plural is ongoing, but here the singular is reserved (hence the need to quote the name).

Speaking of quoting, none of the column names are reserved (not even password), so there's no need to quote them.

[FabianAster]

Thanks for the review!
Alway nice to see ways to improve my code, I dont actively use this anymore, but if I find time ill integrate it.