Abstraction is leacking from data access layer to api-controller

[Mart-Bogdan]

In the pointed line of code

lemmy/crates/api/src/post.rs

Line 107 in 3a05817

let err_type = if e.to_string() == "value too long for type character varying(200)" {

logic in controller is checking error string specific to concrete RDBMS implementation.

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[Mart-Bogdan]

P.S. it also has hardcoded number 200, it could be made into regex, to be future schema-change proof.

[dessalines]

concrete RDBMS implementation

We will never allow a different DB other than postgres.

Could you do a PR for this?

[Mart-Bogdan]

We will never allow a different DB other than postgres.

Anyway, it's a broken abstraction, and hardcoded length.

Could you do a PR for this?

I'm not sure. I haven't written in Rust anything serious.

[Mart-Bogdan]

Perhaps I should try making PR.
Already managed to build Lemmy on a Windows box.

Would get to it on next week.

But I find getting started documentation lacking. I have no idea how to set-up admin user, for example.

[Nutomic]

@Mart-Bogdan It depends how you start Lemmy. If you use the docker/dev/ setup, open localhost:1235 and login with user and password both set to admin.

[Mart-Bogdan]

@Nutomic I'm back.

Was hoping to make use of DatabaseErrorInformation::column_name but it seems now that Postgres don't provide column name. It's a type error that happens on type biding level or something(which is irrelevant why it don't show column name).

I would like to change the column type to text as Postgres stores varchar(N) the same way. And add CHECK constraint (with constraint name same as error code used in Lemmy):

ALTER TABLE post ALTER COLUMN name type text;

ALTER TABLE post ADD CONSTRAINT post_title_too_long CHECK ( char_length(name) <= 200 ) ;

It's actually not recommended to use legacy SQL string types in postgres (unless compatibility with other DBMS is required, which you state is not needed).

From docs:

Tip
There is no performance difference among these three types, apart from increased storage space when using the blank-padded type, and a few extra CPU cycles to check the length when storing into a length-constrained column. While character(n) has performance advantages in some other database systems, there is no such advantage in PostgreSQL; in fact character(n) is usually the slowest of the three because of its additional storage costs. In most situations text or character varying should be used instead.

P.S. I've read a long time ago this SO answer about domains. Dpmains would definitely be overkill, but we could use simple check constraints. That way we can change constraints without locking the whole table in the future.

So how do you think @Nutomic, would it be ok to add CHECK CONSTRAINT in migration instead of VARCHAR(200)?

P.P.S. In my most Postgres development I use just text type and check length inside application code. And I had a lot of problems with fixed limited length columns in MS SQL Server.

[dessalines]

I do most of the DB work.

would it be ok to add CHECK CONSTRAINT in migration instead of VARCHAR(200)?

I def prefer varchar fixed limit lengths over extra unnecessary constraints. They work fine in postgres.

[Mart-Bogdan]

I think then it's not many that could be done.

I hoped something useful could be xtracted from diesel Error::DatabaseError.

My initial assumption was to create custom struct/enum like LemmyDbError, and wrap it around diesel's error, but that won'd to mutch at this point, what do you think?

P.S. What do you think about Keats/validator for proactive validation instead of DB type constraints? Because currently I've noticed a lot of columns with limited length in DB, but it's not possible to detect from code -- which one failed.

But using validator has two drawbacks:

1. we would need to specify the same constraint in two places
2. it must be in sync
3. What to do with an error response and backward compatibility(could be worked around by returning only the first error in current error field)?

[Nutomic]

would it be ok to add CHECK CONSTRAINT in migration instead of VARCHAR(200)?

I def prefer varchar fixed limit lengths over extra unnecessary constraints. They work fine in postgres.

I think we will have to move the length checks to Rust anyway, because of #1447. Otherwise they need to be defined in 2 different places (Postgres and Rust).

[dessalines]

I'm good with adding different constraint checks to rust, but I don't want to ever remove them from the DB. But also I don't see what's wrong with checking error messages coming back from diesel like we do now.

[Mart-Bogdan]

Hm, we are checking for exact message with a specified length

let err_type = if e.to_string() == "value too long for type character varying(200)" {
  "post_title_too_long"
} else {
  "couldnt_create_post"
};

and then inferring field name.

But my initial objection was about that code is tightly coupled to diesel error, code inside "controllers".
But I wasn't able to find out a more clever way to wrap errors and detect failed field.

Perhaps I'll look at it again later. But for now I would like to focus on auth improvements.

P.S.

In my C# project, we are using following error handling in Data Access Layer

catch (DbUpdateException dbException)
{
    if (dbException.InnerException != null)
    {
        //Postgres generated error
        //Sql State 23505 a unique value constraint is violated
        if ((string)dbException.InnerException.Data["SqlState"] == "23505")
        {
            throw new $MyCompany$UniquenessErrorException(dbException);
        }
    }
    throw;
}

So in this particular case upper lying code don't bother to know that under the hood we are using Entity Framework or even SQL. It only needs to care about our domain Exception.

But in case if we don't detect SqlState -- we are using catch-all that rethrows the original error with throw; And it would be caught by generic logic, logged, and returned to users as 500 error. But particular UniquenessErrorException could be handled more correctly. (actually returning error code ErrorCode.UniquenessError )

[dessalines]

We're in a sense doing a similar thing here, checking to see if its a specific type of diesel error, then returning our own ApiError if it is.

        let err_type = if e.to_string()
          == "duplicate key value violates unique constraint \"user__email_key\""
        {
          "email_already_exists"
        } else {
          "user_already_exists"
        };

        return Err(ApiError::err(err_type).into());

I think we've tried before, but have been unable to get cleaner errors out of that diesel error type, but you could try to see if you could get anything more concrete than the string checking we have.

[Mart-Bogdan]

Unfortunately, diesel do not export SQLState,
They've just merged exposing of error position in SQL, but that's pretty useless for us.

diesel-rs/diesel#2517

I was asking if SQLState could also be included, but it didn't get into PR.

@dessalines I've just run a search for if e.to_string

How do you think it's only places with similar checks?

[dessalines]

How do you think it's only places with similar checks?

Could you clarify, I don't understand.

[Mart-Bogdan]

I mean co you know of other similar cases in codebase?

I could wrap these cases into an enum

enum LemmyDataAccessError{
VarcharLenTooLong(max_len:u32),
UniqueConstraint(constraint_name;String),
OtherDataError(anyhow::Error)
}

or plain diesel::Error in the last case.

[Mart-Bogdan]

I mean apply some regex and create helper function that would be called inside db_queries crate

[Nutomic]

Could you open an issue about this in the Diesel repo and link it here? I think this should be handled in the library, and not by us.

[Mart-Bogdan]

He-he. Nice fix. Indeed this can be done by validation. 🎉