-
-
Notifications
You must be signed in to change notification settings - Fork 230
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(mbtiles) - Add validation to martin .mbtiles #741 #1689
base: main
Are you sure you want to change the base?
Changes from 11 commits
03e9115
6aa51e8
81a8d17
f39d553
00e2e62
1b78512
1a69d0e
ff32c1b
3df7fae
b4a9fe3
6251d86
5bc0bb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
use log::info; | ||
use mbtiles::ValidationLevel; | ||
|
||
use crate::mbtiles::{MbtConfig, OnInvalid}; | ||
|
||
#[derive(clap::Args, Debug, PartialEq, Default)] | ||
#[command(about, version)] | ||
pub struct MbtArgs { | ||
/// Level of validation to apply to .mbtiles | ||
#[arg(long)] | ||
pub validate_mbtiles: Option<ValidationLevel>, | ||
/// How to handle invalid .mbtiles | ||
#[arg(long)] | ||
pub on_invalid_mbtiles: Option<OnInvalid>, | ||
} | ||
|
||
impl MbtArgs { | ||
/// Apply CLI parameters from `self` to the configuration loaded from the config file `mbtiles` | ||
pub fn override_config(self, mbt_config: &mut MbtConfig) { | ||
// This ensures that if a new parameter is added to the struct, it will not be forgotten here | ||
let Self { | ||
validate_mbtiles, | ||
on_invalid_mbtiles, | ||
} = self; | ||
|
||
if let Some(value) = validate_mbtiles { | ||
info!("Overriding configured default mbtiles.validate to {value}"); | ||
mbt_config.validate = validate_mbtiles.unwrap_or_default(); | ||
} | ||
|
||
if let Some(value) = on_invalid_mbtiles { | ||
info!("Overriding configured default mbtiles.on_invalid to {value}"); | ||
mbt_config.on_invalid = on_invalid_mbtiles.unwrap_or_default(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,23 +4,47 @@ use std::path::PathBuf; | |
use std::sync::Arc; | ||
|
||
use async_trait::async_trait; | ||
use log::trace; | ||
use clap::ValueEnum; | ||
use enum_display::EnumDisplay; | ||
use log::{trace, warn}; | ||
use martin_tile_utils::{TileCoord, TileInfo}; | ||
use mbtiles::MbtilesPool; | ||
use mbtiles::{MbtResult, MbtilesPool, ValidationLevel}; | ||
use serde::{Deserialize, Serialize}; | ||
use tilejson::TileJSON; | ||
use url::Url; | ||
|
||
use crate::config::UnrecognizedValues; | ||
use crate::file_config::FileError::{AcquireConnError, InvalidMetadata, IoError}; | ||
use crate::file_config::FileError::{self, AcquireConnError, InvalidMetadata, IoError}; | ||
use crate::file_config::{ConfigExtras, FileResult, SourceConfigExtras}; | ||
use crate::source::{TileData, TileInfoSource, UrlQuery}; | ||
use crate::{MartinResult, Source}; | ||
|
||
#[derive( | ||
PartialEq, Eq, Debug, Clone, Copy, Default, Serialize, Deserialize, ValueEnum, EnumDisplay, | ||
)] | ||
#[serde(rename_all = "lowercase")] | ||
pub enum OnInvalid { | ||
/// Print warning message, and abort if the error is critical | ||
#[default] | ||
Warn, | ||
|
||
/// Skip this source | ||
Ignore, | ||
|
||
/// Do not start Martin on any warnings | ||
Abort, | ||
} | ||
|
||
#[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] | ||
pub struct MbtConfig { | ||
#[serde(flatten)] | ||
pub unrecognized: UnrecognizedValues, | ||
|
||
#[serde(default)] | ||
pub validate: ValidationLevel, | ||
|
||
#[serde(default)] | ||
pub on_invalid: OnInvalid, | ||
} | ||
|
||
impl ConfigExtras for MbtConfig { | ||
|
@@ -31,7 +55,31 @@ impl ConfigExtras for MbtConfig { | |
|
||
impl SourceConfigExtras for MbtConfig { | ||
async fn new_sources(&self, id: String, path: PathBuf) -> FileResult<TileInfoSource> { | ||
Ok(Box::new(MbtSource::new(id, path).await?)) | ||
let source = MbtSource::new(id, path.clone()).await?; | ||
if let Err(validation_error) = source.validate(self.validate).await { | ||
match self.on_invalid { | ||
OnInvalid::Abort => { | ||
return Err(FileError::AbortOnInvalid( | ||
path, | ||
validation_error.to_string(), | ||
)); | ||
} | ||
OnInvalid::Ignore => { | ||
return Err(FileError::IgnoreOnInvalid( | ||
path, | ||
validation_error.to_string(), | ||
)); | ||
} | ||
OnInvalid::Warn => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might want to do further processing of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I keep wondering if "fast" and "skip" are actually the same thing. We should always do some basic validation - like making sure the
|
||
warn!( | ||
"Source {} failed validation, this may cause performance issues: {}", | ||
path.display(), | ||
validation_error.to_string() | ||
); | ||
} | ||
} | ||
} | ||
Ok(Box::new(source)) | ||
} | ||
|
||
// TODO: Remove #[allow] after switching to Rust/Clippy v1.78+ in CI | ||
|
@@ -80,6 +128,10 @@ impl MbtSource { | |
tile_info: meta.tile_info, | ||
}) | ||
} | ||
|
||
async fn validate(&self, validation_level: ValidationLevel) -> MbtResult<()> { | ||
self.mbtiles.validate(validation_level).await | ||
} | ||
} | ||
|
||
#[async_trait] | ||
|
@@ -131,13 +183,15 @@ mod tests { | |
use std::path::PathBuf; | ||
|
||
use indoc::indoc; | ||
use mbtiles::ValidationLevel; | ||
|
||
use crate::file_config::{FileConfigEnum, FileConfigSource, FileConfigSrc}; | ||
use crate::mbtiles::MbtConfig; | ||
use crate::mbtiles::{MbtConfig, OnInvalid}; | ||
|
||
#[test] | ||
fn parse() { | ||
let cfg = serde_yaml::from_str::<FileConfigEnum<MbtConfig>>(indoc! {" | ||
let cfg: FileConfigEnum<MbtConfig> = | ||
serde_yaml::from_str::<FileConfigEnum<MbtConfig>>(indoc! {" | ||
paths: | ||
- /dir-path | ||
- /path/to/file2.ext | ||
|
@@ -149,8 +203,10 @@ mod tests { | |
pm-src3: https://example.org/file3.ext | ||
pm-src4: | ||
path: https://example.org/file4.ext | ||
validate: thorough | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit: probably best to put the simple |
||
on_invalid: abort | ||
"}) | ||
.unwrap(); | ||
.unwrap(); | ||
let res = cfg.finalize(""); | ||
assert!(res.is_empty(), "unrecognized config: {res:?}"); | ||
let FileConfigEnum::Config(cfg) = cfg else { | ||
|
@@ -190,5 +246,7 @@ mod tests { | |
), | ||
])) | ||
); | ||
assert_eq!(cfg.custom.validate, ValidationLevel::Thorough); | ||
assert_eq!(cfg.custom.on_invalid, OnInvalid::Abort); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,7 @@ pub enum MbtError { | |
#[error("The destination file {0} is not empty. Some operations like creating a diff file require the destination file to be non-existent or empty.")] | ||
NonEmptyTargetFile(PathBuf), | ||
|
||
#[error("The file {0} does not have the required uniqueness constraint")] | ||
#[error("The file {0} does not have the required uniqueness constraint. Try adding a unique index on the `map` or `tiles` table.")] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps link to https://maplibre.org/martin/mbtiles-schema.html - and also update that page to include some information about performance? Note that |
||
NoUniquenessConstraint(String), | ||
|
||
#[error("Could not copy MBTiles file: {reason}")] | ||
|
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.
i'm not a big fan of all this dup code for each call to
new_sources
- i wonder if we can do this logic elsewhere, and here keep the simple?
operator as before?