Repository navigation
Conversation
…ion state Replace the LazyLock/task_local side channel used to smuggle storage and options into data migrations with SeaORM 2.x's MigratorTraitSelf trait. Migrator is now a struct carrying DispatchBackend and Options as fields. The instance method migrations(&self) constructs MigrationWithData wrappers directly from the migrator's state, removing the need for global statics, task-local scoping, and the run_with_test() helper. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's GuideRefactors migration execution from static SeaORM APIs plus global/task-local state to explicit Sequence diagram for explicit migration executionsequenceDiagram
participant Caller
participant Database
participant Migrator
participant SeaORM
participant DataMigration
Caller->>Migrator: new(storage, options)
Caller->>Database: migrate(migrator)
Database->>Migrator: up(database, None)
Migrator->>Migrator: migrations()
Migrator->>SeaORM: execute migrations
SeaORM->>DataMigration: up(manager)
DataMigration->>DataMigration: use storage and options
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="migration/src/main.rs" line_range="7" />
<code_context>
#[tokio::main]
async fn main() {
- cli::run_cli(migration::Migrator).await;
+ let storage = StorageConfig::parse()
+ .into_storage(false)
+ .await
</code_context>
<issue_to_address>
**Migration commands are rejected**
When the migration binary is invoked with a migration subcommand or its flags, `StorageConfig::parse()` parses the process arguments before `cli::run_cli` receives them, so migration commands such as `up` or `status` are rejected as unknown storage arguments and the migration CLI exits.
Parse storage settings without consuming the migration CLI arguments, or combine storage and migration arguments in one parser.
</issue_to_address>
### Comment 2
<location path="trustd/src/db.rs" line_range="15" />
<code_context>
pub(crate) command: Command,
#[command(flatten)]
pub(crate) database: Database,
+ /// Location of the storage
</code_context>
<issue_to_address>
**Data command flags are rejected**
When a caller supplies storage or migration-option flags after `trustify db data`, clap defines `StorageConfig` and `Options` on `Run` rather than `Data`, so previously accepted flags after `data` are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on `Data`, or make them global so existing command forms remain accepted.
Also at `trustd/src/db.rs:16-18`.
</issue_to_address>
### Comment 3
<location path="migration/src/main.rs" line_range="11" />
<code_context>
+ .into_storage(false)
+ .await
+ .expect("failed to initialize storage");
+ cli::run_cli(migration::Migrator::new(storage, ())).await;
}
</code_context>
<issue_to_address>
**Configured migration options are ignored**
When a `MIGRATION_DATA_*` variable is set for the standalone migration binary, `Migrator::new(storage, ())` uses default `Options`, so configured skip and runner settings are ignored and data migrations run with defaults, including migrations the operator intended to skip.
Construct `Options` from the environment and pass it to the migrator.
Also at `server/src/profile/api.rs:465-466`, `xtask/src/dataset.rs:131`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and the refactor changes which storage backend and options data migrations use, so an incorrect wiring could write incorrect derived values or leave migrations incomplete in the database. Reverting restores the previous runner, but any already-written values would need to be recomputed or the affected migration rerun.
Blocking findings: migration/src/main.rs:7, trustd/src/db.rs:15, migration/src/main.rs:11
| #[tokio::main] | ||
| async fn main() { | ||
| cli::run_cli(migration::Migrator).await; | ||
| let storage = StorageConfig::parse() |
Contributor
There was a problem hiding this comment.
🟠 High · Migration commands are rejected
When the migration binary is invoked with a migration subcommand or its flags, StorageConfig::parse() parses the process arguments before cli::run_cli receives them, so migration commands such as up or status are rejected as unknown storage arguments and the migration CLI exits.
Parse storage settings without consuming the migration CLI arguments, or combine storage and migration arguments in one parser.
Prompt for AI agents
In `migration/src/main.rs` at line 7:
**Migration commands are rejected**
When the migration binary is invoked with a migration subcommand or its flags, `StorageConfig::parse()` parses the process arguments before `cli::run_cli` receives them, so migration commands such as `up` or `status` are rejected as unknown storage arguments and the migration CLI exits.
Parse storage settings without consuming the migration CLI arguments, or combine storage and migration arguments in one parser.| #[command(flatten)] | ||
| pub(crate) database: Database, | ||
| /// Location of the storage | ||
| #[command(flatten)] |
Contributor
There was a problem hiding this comment.
🟡 Medium · Data command flags are rejected
When a caller supplies storage or migration-option flags after trustify db data, clap defines StorageConfig and Options on Run rather than Data, so previously accepted flags after data are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on Data, or make them global so existing command forms remain accepted.
Also at trustd/src/db.rs:16-18.
Prompt for AI agents
In `trustd/src/db.rs` at line 15:
**Data command flags are rejected**
When a caller supplies storage or migration-option flags after `trustify db data`, clap defines `StorageConfig` and `Options` on `Run` rather than `Data`, so previously accepted flags after `data` are rejected as unknown arguments and the requested data migration does not run.
Preserve the flattened storage and migration options on `Data`, or make them global so existing command forms remain accepted.
Also at `trustd/src/db.rs:16-18`.| .into_storage(false) | ||
| .await | ||
| .expect("failed to initialize storage"); | ||
| cli::run_cli(migration::Migrator::new(storage, ())).await; |
Contributor
There was a problem hiding this comment.
🟠 High · Configured migration options are ignored
When a MIGRATION_DATA_* variable is set for the standalone migration binary, Migrator::new(storage, ()) uses default Options, so configured skip and runner settings are ignored and data migrations run with defaults, including migrations the operator intended to skip.
Construct Options from the environment and pass it to the migrator.
Also at server/src/profile/api.rs:465-466, xtask/src/dataset.rs:131.
Prompt for AI agents
In `migration/src/main.rs` at line 11:
**Configured migration options are ignored**
When a `MIGRATION_DATA_*` variable is set for the standalone migration binary, `Migrator::new(storage, ())` uses default `Options`, so configured skip and runner settings are ignored and data migrations run with defaults, including migrations the operator intended to skip.
Construct `Options` from the environment and pass it to the migrator.
Also at `server/src/profile/api.rs:465-466`, `xtask/src/dataset.rs:131`.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migratoris now a struct carryingDispatchBackendandOptions, implementing SeaORM 2.x'sMigratorTraitSelf(instance methods) instead of the staticMigratorTraitLazyLockglobals,task_local!statics,init_storage()(withblock_on),MigrationWithData::new(), andrun_with_test()are all eliminatedDatabase::migrate(),refresh(),bootstrap()now take a&Migrator; CLI commands and tests construct migrators directlyMotivation
Data migrations need runtime state (storage backend, concurrency options) passed into their
up/downmethods. SeaORM 1.x'sMigratorTrait::migrations()was a static method with noself, so the project usedLazyLockglobals andtask_local!statics as a side channel to smuggle state into migrations. SeaORM 2.x addedMigratorTraitSelfwithfn migrations(&self), making this workaround unnecessary.Test plan
cargo xtask precommitpasses (schemas, openapi, clippy, fmt, check)cargo test -p migration)trustd db migratestill works with explicit storage config🤖 Generated with Claude Code
Summary by Sourcery
Adopt SeaORM's instance-based migration API to eliminate global migration state and pass runtime migration dependencies explicitly.
Enhancements:
Build:
Tests: