Implement more verbose output (or better yet: logging) when SQL queries fail

[GutZuFusss]

(If you give me a quick feedback to this or any issue, I will fix them with pleasure, especially ones like this that need more time than skill :D)

Here is an example of what I did for the Quest insertion query:

for i in 0..3 {
	let quest_id = match sqlx::query_scalar!(
		"INSERT INTO QUEST (monster, location, length) VALUES \
		 ($1, $2, $3) returning ID",
		139,
		1,
		60,
	)
	.fetch_one(&mut *tx)
	.await {
		Ok(quest_id) => quest_id, // AAAAAAAAAAAAAAAAAAAAAAAAAA this is probably bullshit, right? not sure how to handle this case, otherwise I would have already made a PR xd
		Err(e) => {
			println!("Error inserting quest: {:?}", e);
			_ = tx.rollback().await;
			return INTERNAL_ERR;
		}
	};
	quests[i] = quest_id;
}

Was previously:

for i in 0..3 {
    let Ok(quest_id) = sqlx::query_scalar!(
        "INSERT INTO QUEST (monster, location, length) VALUES \
         ($1, $2, $3) returning ID",
        139,
        1,
        60,
    )
    .fetch_one(&mut *tx)
    .await
    else {
        _ = tx.rollback().await;
        return INTERNAL_ERR;
    };
    quests[i] = quest_id;
 }

[the-marenga]

The best way would probably be to put a log in the new request_wrapper, since that would catch all errors

[the-marenga]

To give a bit more extensive feedback, yes, the

match x {
Ok(o) => o,
Err(e) => // Handle error
}

pattern is very annoying, but there is not really a better solution. The let Ok(o) = x else drops the error andif let Ok(o) = x {} else {} does the same, but even worse.

A good solution would be to .map_err(|e|// handle & map err) this and then ? propagate the error. The problem becomes, that such a closure does not work with async functions and you need the function to return some generic error. Assuming the transaction is going to be rolled back on drop, a good solution using thiserror could have been:

#[derive(Debug, thiserror::Error)]
enum ServerError {
    #[error("Error when {0}: {1}")]
    DBError(&'static str, sqlx::Error),
}

...

fn do_work() -> Result<(), ServerError> {

  let quest_id = sqlx::query_scalar!(
          "INSERT INTO QUEST (monster, location, length) VALUES \
           ($1, $2, $3) returning ID",
          139, 1, 60,
      )
      .fetch_one(&mut *tx)
      .await
      .map_err(|e| ServerError::DBError("inserting quest", e))?;

The calling function can then log all errors in one place with the custom format, specified in the enum ("Error when inserting quest: Invalid DB Table QUEST")

and can itself decide how it would like to proceed (shutdown server on any db errors, but not on invalid player names or something)

That would probably be the "correct" way how to handle this, but as I said, this sadly does not work, if you have to do async stuff. If you do not care about the actual cause, or can infer the cause, you could even:

#[derive(Debug, thiserror::Error)]
enum ServerError {
    #[error("DBError: {0}")]
    DBError(#[from]  sqlx::Error),
}

...
fn do_work() -> Result<(), ServerError> {

  let quest_id = sqlx::query_scalar!(
          "INSERT INTO QUEST (monster, location, length) VALUES \
           ($1, $2, $3) returning ID",
          139, 1, 60,
      ).fetch_one(&mut *tx).await?;

This is pretty much what I do now, since db errors can be identified by the sql error and it removes tons of boilerplate around error handling. In this case rust will automatically .into() the db error into the custom error enum. If you do not want to use the thiserror crate, the #[from] would just be a:

impl From<sqlx::Error> for ServerError {
// ...
}

and you would have to impl:

impl Error for ServerError {
   //
}

impl Display for ServerError {
  //
}

for the custom error messages yourself