Skip to content

feat: ability to configure foreign_keys pragma for SQLite #1346

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

Merged
merged 5 commits into from
Apr 28, 2025

Conversation

ryands17
Copy link
Contributor

@ryands17 ryands17 commented Apr 3, 2025

This PR allows configuring the foreign_keys pragma for SQLite. Some projects work without foreign keys so this would be a nice addition to the configuration

Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

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

Thank you, @ryands17, for your pull request.

Instead of configuring each PRAGMA setting individually, perhaps we could introduce a more general mechanism. For instance, changing the enable_foreign_keys setting to run_on_start would allow users to configure any PRAGMA settings they desire. This approach would offer greater flexibility and accommodate a wider range of configurations.

WDYT?

@ryands17
Copy link
Contributor Author

ryands17 commented Apr 7, 2025

@kaplanelad I think that's a better approach! Would this be through the config itself? So instead of maybe a single value, we could add the options specified currently

@kaplanelad
Copy link
Contributor

If we add this setup for each property, it will be irrelevant for Postgres. I think a generic configuration would be a better approach.

@ryands17
Copy link
Contributor Author

ryands17 commented Apr 8, 2025

@kaplanelad Is there a DB specific config that can be added? I can look into that and add these configurable setting there with defaults if not passed in.

@kaplanelad
Copy link
Contributor

You can retain the existing configuration; however, if a new configuration is provided, it will override the existing one.

Configuration Example

...
# Database Configuration
database:
  # Database connection URI
  uri: {{ get_env(name="DATABASE_URL", default="sqlite://myapp_development.sqlite?mode=rwc") }}

  # Commands to run when the database starts
  run_on_start:
    PRAGMA foreign_keys = ON;
    PRAGMA journal_mode = WAL;
    PRAGMA synchronous = NORMAL;
    PRAGMA mmap_size = 134217728;
    PRAGMA journal_size_limit = 67108864;
    PRAGMA cache_size = 2000;
...

Behavior

  • If the run_on_start field is defined in the configuration, it will be used as-is.
  • If run_on_start is not provided, the application will use the default commands defined in the code.

WDYT?

@ryands17
Copy link
Contributor Author

@kaplanelad Makes sense! I'm guessing this would just be for SQLite so I can update this PR to contain this if that's ok?

@kaplanelad
Copy link
Contributor

Yes, sounds good!

@ryands17 ryands17 force-pushed the feat/sqlite-fk-config branch 2 times, most recently from 4042fa3 to a8c6995 Compare April 15, 2025 13:04
@ryands17
Copy link
Contributor Author

@kaplanelad made the changes, wdyt?

src/config.rs Outdated
@@ -218,6 +218,25 @@ pub struct Database {
/// various things in development.
#[serde(default)]
pub dangerously_recreate: bool,

// sqlite run on start commands
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, this value should be supported for other cases as well.
So maybe add a comment explaining what it does?
For example: Execute query after initializing the DB

@kaplanelad
Copy link
Contributor

@kaplanelad made the changes, wdyt?

Can you check my note?

@ryands17 ryands17 force-pushed the feat/sqlite-fk-config branch from b19f1f2 to 6ebc685 Compare April 22, 2025 22:51
@ryands17
Copy link
Contributor Author

@kaplanelad Does this look good?

@ryands17 ryands17 force-pushed the feat/sqlite-fk-config branch from 36c5457 to 121713b Compare April 25, 2025 16:53
@kaplanelad kaplanelad merged commit 2604720 into loco-rs:master Apr 28, 2025
5 checks passed
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