From 9d8aca9837870706261fd63db8cc8f3f2ad7ef9a Mon Sep 17 00:00:00 2001 From: gabeschine Date: Wed, 3 Dec 2025 14:38:44 -0500 Subject: [PATCH 1/5] feat(config): warn! on bad source paths instead of hard fail --- martin/src/config/file/file_config.rs | 195 ++++++++++++++++---------- 1 file changed, 123 insertions(+), 72 deletions(-) diff --git a/martin/src/config/file/file_config.rs b/martin/src/config/file/file_config.rs index 0d9b93879..c63f554ed 100644 --- a/martin/src/config/file/file_config.rs +++ b/martin/src/config/file/file_config.rs @@ -273,6 +273,111 @@ pub async fn resolve_files( resolve_int(config, idr, extension).await } +#[cfg(feature = "_tiles")] +async fn resolve_one_source_int( + custom: &T, + idr: &IdResolver, + id: &String, + source: FileConfigSrc, + files: &mut HashSet, + configs: &mut BTreeMap, +) -> MartinResult> { + let mut results = Vec::new(); + + if let Some(url) = parse_url(T::parse_urls(), source.get_path())? { + let dup = !files.insert(source.get_path().clone()); + let dup = if dup { "duplicate " } else { "" }; + let id = idr.resolve(&id, url.to_string()); + configs.insert(id.clone(), source); + results.push(custom.new_sources_url(id.clone(), url.clone()).await?); + info!("Configured {dup}source {id} from {}", sanitize_url(&url)); + } else { + let can = source.abs_path()?; + if !can.is_file() { + log::warn!("The file: {} does not exist", can.display()); + } + + let dup = !files.insert(can.clone()); + let dup = if dup { "duplicate " } else { "" }; + let id = idr.resolve(&id, can.to_string_lossy().to_string()); + info!("Configured {dup}source {id} from {}", can.display()); + configs.insert(id.clone(), source.clone()); + results.push(custom.new_sources(id, source.into_path()).await?); + } + + Ok(results) +} + +#[cfg(feature = "_tiles")] +async fn resolve_one_path_int( + custom: &T, + idr: &IdResolver, + extension: &[&str], + path: PathBuf, + files: &mut HashSet, + directories: &mut Vec, + configs: &mut BTreeMap, +) -> MartinResult> { + let mut results = Vec::new(); + + if let Some(url) = parse_url(T::parse_urls(), &path)? { + let target_ext = extension.iter().find(|&e| url.to_string().ends_with(e)); + let id = if let Some(ext) = target_ext { + url.path_segments() + .and_then(Iterator::last) + .and_then(|s| { + // Strip extension and trailing dot, or keep the original string + s.strip_suffix(ext) + .and_then(|s| s.strip_suffix('.')) + .or(Some(s)) + }) + .unwrap_or("web_source") + } else { + "web_source" + }; + + let id = idr.resolve(id, url.to_string()); + configs.insert(id.clone(), FileConfigSrc::Path(path)); + results.push(custom.new_sources_url(id.clone(), url.clone()).await?); + info!("Configured source {id} from URL {}", sanitize_url(&url)); + } else { + let is_dir = path.is_dir(); + let dir_files = if is_dir { + // directories will be kept in the config just in case there are new files + directories.push(path.clone()); + collect_files_with_extension(&path, extension)? + } else if path.is_file() { + vec![path] + } else { + return Err(MartinError::from(ConfigFileError::InvalidFilePath( + path.canonicalize().unwrap_or(path), + ))); + }; + for path in dir_files { + let can = path + .canonicalize() + .map_err(|e| ConfigFileError::IoError(e, path.clone()))?; + if files.contains(&can) { + if !is_dir { + warn!("Ignoring duplicate MBTiles path: {}", can.display()); + } + continue; + } + let id = path.file_stem().map_or_else( + || "_unknown".to_string(), + |s| s.to_string_lossy().to_string(), + ); + let id = idr.resolve(&id, can.to_string_lossy().to_string()); + info!("Configured source {id} from {}", can.display()); + files.insert(can); + configs.insert(id.clone(), FileConfigSrc::Path(path.clone())); + results.push(custom.new_sources(id, path).await?); + } + } + + Ok(results) +} + #[cfg(feature = "_tiles")] async fn resolve_int( config: &mut FileConfigEnum, @@ -290,83 +395,29 @@ async fn resolve_int( if let Some(sources) = cfg.sources { for (id, source) in sources { - if let Some(url) = parse_url(T::parse_urls(), source.get_path())? { - let dup = !files.insert(source.get_path().clone()); - let dup = if dup { "duplicate " } else { "" }; - let id = idr.resolve(&id, url.to_string()); - configs.insert(id.clone(), source); - results.push(cfg.custom.new_sources_url(id.clone(), url.clone()).await?); - info!("Configured {dup}source {id} from {}", sanitize_url(&url)); - } else { - let can = source.abs_path()?; - if !can.is_file() { - log::warn!("The file: {} does not exist", can.display()); - } - - let dup = !files.insert(can.clone()); - let dup = if dup { "duplicate " } else { "" }; - let id = idr.resolve(&id, can.to_string_lossy().to_string()); - info!("Configured {dup}source {id} from {}", can.display()); - configs.insert(id.clone(), source.clone()); - results.push(cfg.custom.new_sources(id, source.into_path()).await?); + match resolve_one_source_int(&cfg.custom, idr, &id, source, &mut files, &mut configs) + .await + { + Ok(mut sources) => results.append(&mut sources), + Err(e) => warn!("Failed to resolve source {}: {}", id, e), } } } for path in cfg.paths { - if let Some(url) = parse_url(T::parse_urls(), &path)? { - let target_ext = extension.iter().find(|&e| url.to_string().ends_with(e)); - let id = if let Some(ext) = target_ext { - url.path_segments() - .and_then(Iterator::last) - .and_then(|s| { - // Strip extension and trailing dot, or keep the original string - s.strip_suffix(ext) - .and_then(|s| s.strip_suffix('.')) - .or(Some(s)) - }) - .unwrap_or("web_source") - } else { - "web_source" - }; - - let id = idr.resolve(id, url.to_string()); - configs.insert(id.clone(), FileConfigSrc::Path(path)); - results.push(cfg.custom.new_sources_url(id.clone(), url.clone()).await?); - info!("Configured source {id} from URL {}", sanitize_url(&url)); - } else { - let is_dir = path.is_dir(); - let dir_files = if is_dir { - // directories will be kept in the config just in case there are new files - directories.push(path.clone()); - collect_files_with_extension(&path, extension)? - } else if path.is_file() { - vec![path] - } else { - return Err(MartinError::from(ConfigFileError::InvalidFilePath( - path.canonicalize().unwrap_or(path), - ))); - }; - for path in dir_files { - let can = path - .canonicalize() - .map_err(|e| ConfigFileError::IoError(e, path.clone()))?; - if files.contains(&can) { - if !is_dir { - warn!("Ignoring duplicate MBTiles path: {}", can.display()); - } - continue; - } - let id = path.file_stem().map_or_else( - || "_unknown".to_string(), - |s| s.to_string_lossy().to_string(), - ); - let id = idr.resolve(&id, can.to_string_lossy().to_string()); - info!("Configured source {id} from {}", can.display()); - files.insert(can); - configs.insert(id.clone(), FileConfigSrc::Path(path.clone())); - results.push(cfg.custom.new_sources(id, path).await?); - } + match resolve_one_path_int( + &cfg.custom, + idr, + extension, + path, + &mut files, + &mut directories, + &mut configs, + ) + .await + { + Ok(mut sources) => results.append(&mut sources), + Err(e) => warn!("Failed to resolve sources from path: {}", e), } } From 0d682f8d0b1b423ce6556718fd7d9b607a2056e3 Mon Sep 17 00:00:00 2001 From: gabeschine Date: Wed, 3 Dec 2025 14:57:43 -0500 Subject: [PATCH 2/5] chore: just clippy --- martin/src/config/file/file_config.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/martin/src/config/file/file_config.rs b/martin/src/config/file/file_config.rs index c63f554ed..e2ff28ff2 100644 --- a/martin/src/config/file/file_config.rs +++ b/martin/src/config/file/file_config.rs @@ -277,7 +277,7 @@ pub async fn resolve_files( async fn resolve_one_source_int( custom: &T, idr: &IdResolver, - id: &String, + id: &str, source: FileConfigSrc, files: &mut HashSet, configs: &mut BTreeMap, @@ -287,7 +287,7 @@ async fn resolve_one_source_int( if let Some(url) = parse_url(T::parse_urls(), source.get_path())? { let dup = !files.insert(source.get_path().clone()); let dup = if dup { "duplicate " } else { "" }; - let id = idr.resolve(&id, url.to_string()); + let id = idr.resolve(id, url.to_string()); configs.insert(id.clone(), source); results.push(custom.new_sources_url(id.clone(), url.clone()).await?); info!("Configured {dup}source {id} from {}", sanitize_url(&url)); @@ -299,7 +299,7 @@ async fn resolve_one_source_int( let dup = !files.insert(can.clone()); let dup = if dup { "duplicate " } else { "" }; - let id = idr.resolve(&id, can.to_string_lossy().to_string()); + let id = idr.resolve(id, can.to_string_lossy().to_string()); info!("Configured {dup}source {id} from {}", can.display()); configs.insert(id.clone(), source.clone()); results.push(custom.new_sources(id, source.into_path()).await?); @@ -399,7 +399,7 @@ async fn resolve_int( .await { Ok(mut sources) => results.append(&mut sources), - Err(e) => warn!("Failed to resolve source {}: {}", id, e), + Err(e) => warn!("Failed to resolve source {id}: {e}"), } } } @@ -417,7 +417,7 @@ async fn resolve_int( .await { Ok(mut sources) => results.append(&mut sources), - Err(e) => warn!("Failed to resolve sources from path: {}", e), + Err(e) => warn!("Failed to resolve sources from path: {e}"), } } From 6f3e3980d1cf0461251e78b9addb50916f9c5f3a Mon Sep 17 00:00:00 2001 From: gabeschine Date: Thu, 4 Dec 2025 16:30:26 -0500 Subject: [PATCH 3/5] feedback(copilot): add path to error --- martin/src/config/file/file_config.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/martin/src/config/file/file_config.rs b/martin/src/config/file/file_config.rs index e2ff28ff2..c6848b976 100644 --- a/martin/src/config/file/file_config.rs +++ b/martin/src/config/file/file_config.rs @@ -409,7 +409,7 @@ async fn resolve_int( &cfg.custom, idr, extension, - path, + path.clone(), &mut files, &mut directories, &mut configs, @@ -417,7 +417,10 @@ async fn resolve_int( .await { Ok(mut sources) => results.append(&mut sources), - Err(e) => warn!("Failed to resolve sources from path: {e}"), + Err(e) => warn!( + "Failed to resolve sources from path {}: {e}", + path.display(), + ), } } From 761b65dcaa748c6a09607f558472eb2427a18794 Mon Sep 17 00:00:00 2001 From: gabeschine Date: Thu, 4 Dec 2025 16:43:32 -0500 Subject: [PATCH 4/5] tests: basic tests to validate no failures with invalid paths --- martin/src/config/file/file_config.rs | 68 +++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/martin/src/config/file/file_config.rs b/martin/src/config/file/file_config.rs index c6848b976..a15cc650f 100644 --- a/martin/src/config/file/file_config.rs +++ b/martin/src/config/file/file_config.rs @@ -497,3 +497,71 @@ pub fn copy_unrecognized_keys_from_config( ) { result.extend(unrecognized.keys().map(|k| format!("{prefix}{k}"))); } + +#[cfg(all(test, feature = "mbtiles"))] +mod mbtiles_tests { + use martin_core::config::IdResolver; + + use super::*; + use crate::config::file::tiles::mbtiles::MbtConfig; + + #[tokio::test] + async fn test_invalid_path_warns_instead_of_failing() { + let _ = env_logger::builder().is_test(true).try_init(); + + let invalid_path = PathBuf::from("/nonexistent/path/"); + let invalid_source = PathBuf::from("/nonexistent/path/to/file.mbtiles"); + let mut sources = BTreeMap::new(); + sources.insert( + "test_source".to_string(), + FileConfigSrc::Path(invalid_source.clone()), + ); + let mut config = FileConfigEnum::::Config(FileConfig { + paths: OptOneMany::One(invalid_path.clone()), + sources: Some(sources), + custom: MbtConfig::default(), + }); + + let idr = IdResolver::new(&[]); + let result = resolve_files(&mut config, &idr, &["mbtiles"]).await; + + // Should succeed despite invalid paths + assert!(result.is_ok()); + let sources = result.unwrap(); + assert_eq!(sources.len(), 0); + } +} + +#[cfg(all(test, feature = "pmtiles"))] +mod pmtiles_tests { + use martin_core::config::IdResolver; + + use super::*; + use crate::config::file::tiles::pmtiles::PmtConfig; + + #[tokio::test] + async fn test_invalid_path_warns_instead_of_failing() { + let _ = env_logger::builder().is_test(true).try_init(); + + let invalid_path = PathBuf::from("/nonexistent/path/"); + let invalid_source = PathBuf::from("/nonexistent/path/to/file.pmtiles"); + let mut sources = BTreeMap::new(); + sources.insert( + "test_source".to_string(), + FileConfigSrc::Path(invalid_source.clone()), + ); + let mut config = FileConfigEnum::::Config(FileConfig { + paths: OptOneMany::One(invalid_path.clone()), + sources: Some(sources), + custom: PmtConfig::default(), + }); + + let idr = IdResolver::new(&[]); + let result = resolve_files(&mut config, &idr, &["pmtiles"]).await; + + // Should succeed despite invalid paths + assert!(result.is_ok()); + let sources = result.unwrap(); + assert_eq!(sources.len(), 0); + } +} From 2a5f34f379e311c33c35032cdab816f70ec753ec Mon Sep 17 00:00:00 2001 From: Frank Elsinga Date: Fri, 5 Dec 2025 10:32:49 +0100 Subject: [PATCH 5/5] apply doc comments and method ordering from upstream --- martin/src/config/file/file_config.rs | 121 ++++++++++++++------------ 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/martin/src/config/file/file_config.rs b/martin/src/config/file/file_config.rs index a15cc650f..9067a5b56 100644 --- a/martin/src/config/file/file_config.rs +++ b/martin/src/config/file/file_config.rs @@ -273,6 +273,62 @@ pub async fn resolve_files( resolve_int(config, idr, extension).await } +#[cfg(feature = "_tiles")] +async fn resolve_int( + config: &mut FileConfigEnum, + idr: &IdResolver, + extension: &[&str], +) -> MartinResult> { + let Some(cfg) = config.extract_file_config() else { + return Ok(Vec::new()); + }; + + let mut results = Vec::new(); + let mut configs = BTreeMap::new(); + let mut files = HashSet::new(); + let mut directories = Vec::new(); + + if let Some(sources) = cfg.sources { + for (id, source) in sources { + match resolve_one_source_int(&cfg.custom, idr, &id, source, &mut files, &mut configs) + .await + { + Ok(src) => results.push(src), + Err(e) => warn!("Failed to resolve source {id}: {e}"), + } + } + } + + for path in cfg.paths { + match resolve_one_path_int( + &cfg.custom, + idr, + extension, + path.clone(), + &mut files, + &mut directories, + &mut configs, + ) + .await + { + Ok(mut sources) => results.append(&mut sources), + Err(e) => warn!( + "Failed to resolve sources from path {}: {e}", + path.display(), + ), + } + } + + *config = FileConfigEnum::new_extended(directories, configs, cfg.custom); + + Ok(results) +} + +/// Resolves a single tile source configuration and returns a boxed source for further processing. +/// +/// This function processes a tile source configuration using a given custom implementation of +/// `TileSourceConfiguration` and resolves its ID using `IdResolver`. +/// It determines if the source is a URL or a file path, configures the source accordingly. #[cfg(feature = "_tiles")] async fn resolve_one_source_int( custom: &T, @@ -281,15 +337,15 @@ async fn resolve_one_source_int( source: FileConfigSrc, files: &mut HashSet, configs: &mut BTreeMap, -) -> MartinResult> { - let mut results = Vec::new(); +) -> MartinResult { + let mut result; if let Some(url) = parse_url(T::parse_urls(), source.get_path())? { let dup = !files.insert(source.get_path().clone()); let dup = if dup { "duplicate " } else { "" }; let id = idr.resolve(id, url.to_string()); configs.insert(id.clone(), source); - results.push(custom.new_sources_url(id.clone(), url.clone()).await?); + result = custom.new_sources_url(id.clone(), url.clone()).await?; info!("Configured {dup}source {id} from {}", sanitize_url(&url)); } else { let can = source.abs_path()?; @@ -302,12 +358,16 @@ async fn resolve_one_source_int( let id = idr.resolve(id, can.to_string_lossy().to_string()); info!("Configured {dup}source {id} from {}", can.display()); configs.insert(id.clone(), source.clone()); - results.push(custom.new_sources(id, source.into_path()).await?); + result = custom.new_sources(id, source.into_path()).await?; } - Ok(results) + Ok(result) } +/// Resolves a single path, configuring sources based on the given tile source configuration. +/// +/// This function processes a given `PathBuf`, checking if it represents a file, directory, +/// or a URL, and then it performs the necessary steps to configure tile sources. #[cfg(feature = "_tiles")] async fn resolve_one_path_int( custom: &T, @@ -378,57 +438,6 @@ async fn resolve_one_path_int( Ok(results) } -#[cfg(feature = "_tiles")] -async fn resolve_int( - config: &mut FileConfigEnum, - idr: &IdResolver, - extension: &[&str], -) -> MartinResult> { - let Some(cfg) = config.extract_file_config() else { - return Ok(Vec::new()); - }; - - let mut results = Vec::new(); - let mut configs = BTreeMap::new(); - let mut files = HashSet::new(); - let mut directories = Vec::new(); - - if let Some(sources) = cfg.sources { - for (id, source) in sources { - match resolve_one_source_int(&cfg.custom, idr, &id, source, &mut files, &mut configs) - .await - { - Ok(mut sources) => results.append(&mut sources), - Err(e) => warn!("Failed to resolve source {id}: {e}"), - } - } - } - - for path in cfg.paths { - match resolve_one_path_int( - &cfg.custom, - idr, - extension, - path.clone(), - &mut files, - &mut directories, - &mut configs, - ) - .await - { - Ok(mut sources) => results.append(&mut sources), - Err(e) => warn!( - "Failed to resolve sources from path {}: {e}", - path.display(), - ), - } - } - - *config = FileConfigEnum::new_extended(directories, configs, cfg.custom); - - Ok(results) -} - /// Returns a vector of file paths matching any `allowed_extension` within the given directory. /// /// # Errors