shuttle
shuttle copied to clipboard
shuttle-hq
refactor(auth): add postgresql enum type for account_tier
Description of change
Applied the recommendations here: https://docs.rs/sqlx/latest/sqlx/postgres/types/index.html#enumerations. Also important from a data integrity standpoint, to link PostgreSQL and Rust types.
There are also official PostgreSQL docs referring to type safety, ordering, and implementation details (e.g. how to alter the enum type after it is created): https://www.postgresql.org/docs/current/datatype-enum.html.
How has this been tested? (if applicable)
TODO on staging.
If it exists in postgres as well, it is type checked in the database, so mistakes in manual queries are rejected. I still think you have a good point. I like the path with less maintenance, considering this will be changing a lot.
Like @jonaro00 said, by having enums in the database, sqlx can be certain that the value coming in matches the enum and can reject invalid queries. The main reason for me is that it helps when you're adding or removing possible values in the database. Removing one value from the Rust enum would lead to failure down the line, while with an Postgres one SQLx will scream that you need to change it in the database first. And by experience it does help catch errors.
I'm in favor of the change. I don't think we're going to update the tiers that often, and when we do I don't feel like adding a one line migration when adding a new tier is that much of a burden.
Removing one value from the Rust enum would lead to failure down the line, while with an Postgres one SQLx will scream that you need to change it in the database first. And by experience it does help catch errors.
My experience has mostly been that code enums are better than DB enums. The reason being that the code enums are checked at compile time. But the DB enums only fail at runtime. And it's just never fun maintaining two pieces of code that does the same thing. I do agree though that the DB enums help with manual queries.
It will be interesting to know what failures you have seen from adding / removing an enum in Rust only?
I agree they're better (I would rather use the Rust one than the PSQL one), but they aren't mutually exclusive though. I'm advocating for using both.
It will be interesting to know what failures you have seen from adding / removing an enum in Rust only?
It wasn't with Rust but still went through an ORM of some sort (sqlx isn't an ORM, but you see what I mean). Basically they sometimes tried to remove one of the variant but forgot to make sure that this value wasn't used anymore. If you don't have an enum, they you will get a runtime error when you try to query (which might be way later in the future), instead of having the migration fail directly. Just like with static typing, you will get an error anyway, it will just happen before instead of later.
And it's just never fun maintaining two pieces of code that does the same thing.
Again, I don't think the cost of maintaining it is that big. In particular since there is no way for it to go wrong since SQLx will complain if you forgot to do it. And if this becomes a pain, we can always alter it to be a text field again.
Just like with static typing, you will get an error anyway, it will just happen before instead of later.
If I understand this correctly then we will get the error during the SQL migration. I still don't understand the adding enum case, but for deleting an enum you are expecting that the migration will fail if the enum variant is still in use. However for Postgres this won't be the case since Postgres does not support removing enums from the type after it was created. This means either having more complex migrations (adding a string column; copy over the enum values to the string column; dropping the enum column; dropping the enum type; recreate the enum type; add the enum column back; copy the string column to the enum; dropping the string column) or being happy with the PG enums not matching our Rust enums.
The migration isn't that complex (you rename the existing type, fix your column, create the new type, alter the column to use the new one, drop the old one, rename the type), but I was expecting it to be more straightforward. I won't push it further, I still think this is a better idea to have it. If we don't have any bug with it then fine, but when we do I'll link back to this discussion :p
From my end, I just want to make sure we all realize this is not a free lunch (or at least not as free as it seems). I've just had experiences where I wished the DB did not also have enums because they were a pain to update. So for me this is mostly a preference thing :smile:
Again, I don't think the cost of maintaining it is that big. In particular since there is no way for it to go wrong since SQLx will complain if you forgot to do it. And if this becomes a pain, we can always alter it to be a text field again.
What do you mean by SQLx will complain? Isn't that only the case if you use the compile-time checked sqlx macros?
Personally, and while I see the advantages, I am also leaning towards not having it as an enum in Postgres, at least not yet. The main reason for that is that I am not so confident in us not changing the account tier enum, it has been changed quite a few times since we started working on billing, and this would slow us down when making further changes.
