fix: migration-process #55
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,4 @@ | ||||||||||||||||||||||||||||||||||||||||||||||
| use crate::domain::mongodb::connection::{extract_db_name, get_mongo_uri, select_mongo_path}; | ||||||||||||||||||||||||||||||||||||||||||||||
| use crate::services::config::DatabaseConfig; | ||||||||||||||||||||||||||||||||||||||||||||||
| use anyhow::{Context, Result}; | ||||||||||||||||||||||||||||||||||||||||||||||
| use std::path::PathBuf; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -12,18 +12,41 @@ pub async fn run(cfg: DatabaseConfig, restore_file: PathBuf) -> Result<()> { | |||||||||||||||||||||||||||||||||||||||||||||
| let mongorestore = select_mongo_path().join("mongorestore"); | ||||||||||||||||||||||||||||||||||||||||||||||
| let uri = get_mongo_uri(cfg.clone())?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| let dry_run = Command::new(&mongorestore) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!( | ||||||||||||||||||||||||||||||||||||||||||||||
| "--uri={}", | ||||||||||||||||||||||||||||||||||||||||||||||
| format!( | ||||||||||||||||||||||||||||||||||||||||||||||
| "mongodb://{}:{}@{}:{}/?authSource=admin", | ||||||||||||||||||||||||||||||||||||||||||||||
| cfg.username, cfg.password, cfg.host, cfg.port | ||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+15
to
+22
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query:
💡 Result: When MongoDB authentication is truly disabled, you should use an anonymous connection string that does not include username/password material. So, use: mongodb://host:port/ The URI you proposed, mongodb://: Citations:
🏁 Script executed: cat -n src/domain/mongodb/restore.rs | head -50Repository: Portabase/agent Length of output: 2471 🏁 Script executed: cat -n src/domain/mongodb/connection.rsRepository: Portabase/agent Length of output: 1832 Use consistent URI construction for no-auth MongoDB configs in dry-run command. At lines 15–22, the dry-run command always injects Apply the same conditional logic from Proposed fix- let dry_run = Command::new(&mongorestore)
+ let dry_uri = if cfg.username.is_empty() || cfg.password.is_empty() {
+ format!("mongodb://{}:{}/", cfg.host, cfg.port)
+ } else {
+ format!(
+ "mongodb://{}:{}@{}:{}/?authSource=admin",
+ cfg.username, cfg.password, cfg.host, cfg.port
+ )
+ };
+
+ let dry_run = Command::new(&mongorestore)
.arg(format!(
"--uri={}",
- format!(
- "mongodb://{}:{}@{}:{}/?authSource=admin",
- cfg.username, cfg.password, cfg.host, cfg.port
- )
+ dry_uri
))📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--archive={}", restore_file.display())) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg("--gzip") | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg("--dryRun") | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg("--verbose") | ||||||||||||||||||||||||||||||||||||||||||||||
| .output()?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| let dry_output = String::from_utf8_lossy(&dry_run.stderr); | ||||||||||||||||||||||||||||||||||||||||||||||
| let source_db = extract_db_name(&dry_output) | ||||||||||||||||||||||||||||||||||||||||||||||
| .context("Could not detect source database name from archive")?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+29
to
+31
There was a problem hiding this comment. 🧩 Analysis chain🌐 Web query:
💡 Result: Non-zero exit status: treat it as authoritative failure. Don’t base correctness on whether archive “prelude” details appear on stderr vs stdout. stderr vs stdout for archive prelude details - MongoDB’s docs state that the database tools (including mongorestore) print logs to stderr. - The documentation for --dryRun + --verbose describes verbosity affecting “internal reporting returned on standard output or in log files” (i.e., you should not assume anything about where specific messages land). - In the mongorestore code, archive prelude metadata is parsed/used internally (and failures in parsing/reading return an error), rather than being something a caller should infer from stderr-only output. Should callers parse output and ignore exit code? - No: callers should key off mongorestore’s process exit status. If mongorestore encounters errors such as inability to parse/validate options or read archive/prelude metadata, the tool returns an error result and exits non-zero. - In general, callers should not treat “message seen on stderr” as the only signal. Use the exit code to decide whether it is safe to parse any summary output. Practical guidance 1) Run mongorestore with --dryRun --verbose and capture both streams if you need logs. 2) If the command’s exit code is non-zero, treat the run as failed and do not trust/parse success summaries that may still have been printed. 3) Only parse success/summary output when exit code is 0. Grounding notes: This applies to archive prelude handling because prelude metadata is not merely informational; mongorestore reads it as part of establishing the archive/dump format and may abort the restore when it can’t be processed, independent of whether any particular detail was emitted on stderr. Citations:
🏁 Script executed: cat -n src/domain/mongodb/restore.rs | sed -n '1,50p'Repository: Portabase/agent Length of output: 2471 🏁 Script executed: wc -l src/domain/mongodb/restore.rsRepository: Portabase/agent Length of output: 92 🏁 Script executed: cat -n src/domain/mongodb/restore.rs | sed -n '15,45p'Repository: Portabase/agent Length of output: 1501 Check dry-run exit status before parsing stderr. The dry-run command exits at line 27 without verifying success. If Fix let dry_output = String::from_utf8_lossy(&dry_run.stderr);
+ if !dry_run.status.success() {
+ anyhow::bail!("mongorestore dry-run failed for {}: {}", cfg.name, dry_output);
+ }
let source_db = extract_db_name(&dry_output)
.context("Could not detect source database name from archive")?;The actual restore command at line 46 correctly checks 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| info!("Detected source database in archive: {}", source_db); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| let output = Command::new(&mongorestore) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--uri={}", uri)) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--archive={}", restore_file.display())) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg("--gzip") | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg("--drop") | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--nsInclude={}.*", source_db)) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--nsFrom={}.*", source_db)) | ||||||||||||||||||||||||||||||||||||||||||||||
| .arg(format!("--nsTo={}.*", cfg.database)) | ||||||||||||||||||||||||||||||||||||||||||||||
| .output() | ||||||||||||||||||||||||||||||||||||||||||||||
| .with_context(|| format!("Failed to run mongorestore for {}", cfg.name))?; | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| if !output.status.success() { | ||||||||||||||||||||||||||||||||||||||||||||||
| let stderr = String::from_utf8_lossy(&output.stderr); | ||||||||||||||||||||||||||||||||||||||||||||||
| error!("MongoDB restore failed for {}: {}", cfg.name, stderr); | ||||||||||||||||||||||||||||||||||||||||||||||
| anyhow::bail!("MongoDB restore failed for: {}", cfg.name); | ||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
| info!("MongoDB restore completed for {}", cfg.name); | ||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mariadb_dumpis resolved by version but never used — falls back to$PATH.Lines 33–34 compute and log a version-specific path, then line 37 spawns the bare string
"mariadb-dump", which makesselect_mariadb_patheffectively dead code and breaks the per-version targeting. If the host has multiplemariadb-dumpbinaries (or none on$PATH), behavior diverges from what the log line claims.🔧 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents