feat(mbtiles) - Add validation to martin .mbtiles #741 #1689
Conversation
…to new_sources_url in case tile sources from URLs produce this error
| validation_error.to_string(), | ||
| )); | ||
| } | ||
| OnInvalid::Warn => { |
There was a problem hiding this comment.
We might want to do further processing of the validation_error to determine if this is a critical issue or not.
There was a problem hiding this comment.
I keep wondering if "fast" and "skip" are actually the same thing. We should always do some basic validation - like making sure the tiles table exists with the right columns - or else we can't even use it. The only difference is in what we do afterwards - ignore the source or continue with it... So perhaps:
- validation level is
fastvsthorough(no skip) - severity would be
good,warn,error - and the action could be
abortorignore sourceforerror, andabortorignore sourceorwarnonwarning? - we may even introduce
warnings-as-errorsparameter? (slowly building a compiler here :) )
There was a problem hiding this comment.
I really like where you are going with this! I left a few minor comments inline.
A few overall thoughts:
- we may have more than one type of "local source validation" (pmtiles, sprites, fonts, cog files, ...) - so we should keep CLI parameters simple and global (not mbtiles specific)
- all configuration sources may introduce some format-specific validation steps, but we should try to keep them all consistent from configuration file perspective
- we should have multi-level config: CLI / config file root / per source type / specific path/file
- lets cleanup error handling dup code somehow
- as i wrote more in depth in a comment, we may want to have
warning-as-errornon-default mode
overall, awesome work! I think it is pretty close to the working state. One last thing - usually you do not want to submit PRs from the main branch of your fork, but that's really a minor thing :)
| @@ -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.
minor nit: probably best to put the simple key: value things before the nested ones - i.e. move these two above paths:
| @@ -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.
Perhaps link to https://maplibre.org/martin/mbtiles-schema.html - and also update that page to include some information about performance? Note that tiles is the only official requirement of an mbtiles, but it could be either a table or a view - and it would be difficult to explain all this in a few lines.
| validation_error.to_string(), | ||
| )); | ||
| } | ||
| OnInvalid::Warn => { |
There was a problem hiding this comment.
I keep wondering if "fast" and "skip" are actually the same thing. We should always do some basic validation - like making sure the tiles table exists with the right columns - or else we can't even use it. The only difference is in what we do afterwards - ignore the source or continue with it... So perhaps:
- validation level is
fastvsthorough(no skip) - severity would be
good,warn,error - and the action could be
abortorignore sourceforerror, andabortorignore sourceorwarnonwarning? - we may even introduce
warnings-as-errorsparameter? (slowly building a compiler here :) )
| @@ -264,7 +270,11 @@ async fn resolve_int<T: SourceConfigExtras>( | |||
| 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?); | |||
| match cfg.custom.new_sources_url(id.clone(), url.clone()).await { | |||
There was a problem hiding this comment.
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?
closes #741