Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: MySQL support #75

Merged
merged 1 commit into from
Nov 25, 2024
Merged

feat: MySQL support #75

merged 1 commit into from
Nov 25, 2024

Conversation

m4tx
Copy link
Contributor

@m4tx m4tx commented Nov 20, 2024

This also deduplicates the database code and adds feature flags that can enable and disable the database backends (or remove the ORM altogether).

This commit also adds a cargo hack step to the CI that checks if the project can be compiled using any subset of the features.

@m4tx m4tx requested review from seqre and Iipin November 20, 2024 18:57
@m4tx m4tx force-pushed the mysql branch 3 times, most recently from e8c1b90 to 5226570 Compare November 20, 2024 18:59
Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 92.33227% with 24 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
flareon/src/db/fields.rs 54.54% 15 Missing ⚠️
flareon/src/auth.rs 83.33% 3 Missing ⚠️
flareon/src/db.rs 95.45% 2 Missing ⚠️
flareon/src/admin.rs 0.00% 1 Missing ⚠️
flareon/src/db/migrations.rs 95.65% 1 Missing ⚠️
flareon/src/lib.rs 85.71% 1 Missing ⚠️
flareon/tests/db.rs 75.00% 1 Missing ⚠️
Flag Coverage Δ
rust 83.18% <92.33%> (+0.29%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
flareon-macros/src/dbtest.rs 92.72% <100.00%> (+2.72%) ⬆️
flareon/src/config.rs 87.50% <100.00%> (+1.58%) ⬆️
flareon/src/db/impl_mysql.rs 100.00% <100.00%> (ø)
flareon/src/db/impl_postgres.rs 100.00% <100.00%> (ø)
flareon/src/db/impl_sqlite.rs 100.00% <100.00%> (+3.65%) ⬆️
flareon/src/db/sea_query_db.rs 100.00% <100.00%> (ø)
flareon/src/error.rs 89.83% <ø> (ø)
flareon/src/request.rs 80.26% <ø> (ø)
flareon/src/test.rs 95.12% <100.00%> (+0.70%) ⬆️
flareon/src/admin.rs 0.00% <0.00%> (ø)
... and 6 more

🚨 Try these New Features:

Copy link
Contributor

@seqre seqre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great job

@@ -6,6 +6,7 @@
//!
//! For the default way to store users in the database, see the [`db`] module.

#[cfg(any(feature = "sqlite", feature = "postgres", feature = "mysql"))]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to create an alias for it? We will repeat that in many places in the code.

One idea I have is to create an empty __db feature that each database feature would include as well, and then we could just check for it.

Copy link
Contributor Author

@m4tx m4tx Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's one way to resolve it. Except that I think it should actually just be named db (sadly there's no concept of "private" feature flags in Cargo as of now and special naming doesn't change much about this) and we should generate a compilation error in build.rs if db is enabled, but none of sqlite, postgres, and mysql are (if we don't do that, then seaquery-binder will fail to compile as far as I remember). But generally I think this is a decent idea

Comment on lines -824 to +915
Timestamp,
TimestampWithTimeZone,
DateTimeWithTimeZone,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no consensus on what does "datetime"/"timestamp" mean in database engines:

  • SQLite doesn't have a concept of a date(time), you just store it as text
  • Postgres doesn't have a dedicated datetime field; instead it just has timestamp with a wide range of possible values
  • MySQL has both: datetime is anything between year 1000 and 9999, while timestamp is 32-bit unix timestamp, so it's susceptible to Y2038 problem. MariaDB is slightly better in this regard, but it only supports timestamps with years up to 2106.

I thought it might be surprising for users who might want to store arbitrary chrono::DateTime objects and see them failing if they don't fall between 1970 and 2038. So I changed the naming so that it's clear it's not a timestamp, and we might eventually add a separate "timestamp" type in the future. For now, I just want to make sure that whatever (sensible) value the users might want to store, will work.

Comment on lines -223 to +231
let test_database_name = format!("test_flareon__{}", test_name);
let test_database_name = format!("test_flareon__{test_name}");
database
.raw(&format!("DROP DATABASE IF EXISTS {}", test_database_name))
.raw(&format!("DROP DATABASE IF EXISTS {test_database_name}"))
.await?;
database
.raw(&format!("CREATE DATABASE {}", test_database_name))
.raw(&format!("CREATE DATABASE {test_database_name}"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one can hide from Clippy's watchful eyes lol

This also deduplicates the database code and adds feature flags that can
enable and disable the database backends (or remove the ORM altogether).

This commit also adds a cargo hack step to the CI that checks if
the project can be compiled using any subset of the features.
@m4tx m4tx merged commit 764964c into master Nov 25, 2024
23 of 25 checks passed
@m4tx m4tx deleted the mysql branch November 25, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants