From 1abd111d43a595524fe7d92c64dca5a5682a89f4 Mon Sep 17 00:00:00 2001 From: rstanciu Date: Wed, 30 Aug 2023 18:56:54 -0700 Subject: [PATCH] Address review comments --- ...5917961ef736c030ac577a487091a9724049d.json | 12 ++ martin-mbtiles/src/bin/main.rs | 12 +- martin-mbtiles/src/errors.rs | 7 +- martin-mbtiles/src/mbtiles.rs | 124 +++++++++--------- martin-mbtiles/src/tile_copier.rs | 17 ++- .../files/invalid_zoomed_world_cities.mbtiles | Bin 14848 -> 14848 bytes .../files/zoomed_world_cities.mbtiles | Bin 14848 -> 14848 bytes 7 files changed, 98 insertions(+), 74 deletions(-) create mode 100644 martin-mbtiles/.sqlx/query-8450267c60816a13cc70ee0ff635917961ef736c030ac577a487091a9724049d.json diff --git a/martin-mbtiles/.sqlx/query-8450267c60816a13cc70ee0ff635917961ef736c030ac577a487091a9724049d.json b/martin-mbtiles/.sqlx/query-8450267c60816a13cc70ee0ff635917961ef736c030ac577a487091a9724049d.json new file mode 100644 index 000000000..74adbbdcd --- /dev/null +++ b/martin-mbtiles/.sqlx/query-8450267c60816a13cc70ee0ff635917961ef736c030ac577a487091a9724049d.json @@ -0,0 +1,12 @@ +{ + "db_name": "SQLite", + "query": "INSERT OR REPLACE INTO metadata(name, value) VALUES('global_hash', ?)", + "describe": { + "columns": [], + "parameters": { + "Right": 1 + }, + "nullable": [] + }, + "hash": "8450267c60816a13cc70ee0ff635917961ef736c030ac577a487091a9724049d" +} diff --git a/martin-mbtiles/src/bin/main.rs b/martin-mbtiles/src/bin/main.rs index 509eed81d..6e5e663a5 100644 --- a/martin-mbtiles/src/bin/main.rs +++ b/martin-mbtiles/src/bin/main.rs @@ -69,7 +69,7 @@ enum Commands { integrity_check: IntegrityCheck, /// Generate a hash of the tile data hashes and store under the 'global_hash' key in metadata #[arg(long)] - generate_global_hash: bool, + regenerate_global_hash: bool, }, } @@ -96,9 +96,9 @@ async fn main() -> Result<()> { Commands::Validate { file, integrity_check, - generate_global_hash, + regenerate_global_hash, } => { - validate_mbtiles(file.as_path(), integrity_check, generate_global_hash).await?; + validate_mbtiles(file.as_path(), integrity_check, regenerate_global_hash).await?; } } @@ -126,13 +126,13 @@ async fn meta_set_value(file: &Path, key: &str, value: Option) -> Result async fn validate_mbtiles( file: &Path, integrity_check: IntegrityCheck, - generate_global_hash: bool, + regenerate_global_hash: bool, ) -> Result<()> { let mbt = Mbtiles::new(file)?; let opt = SqliteConnectOptions::new().filename(file).read_only(true); let mut conn = SqliteConnection::connect_with(&opt).await?; mbt.validate_mbtiles(integrity_check, &mut conn).await?; - if generate_global_hash { + if regenerate_global_hash { mbt.generate_global_hash(&mut conn).await?; } Ok(()) @@ -427,7 +427,7 @@ mod tests { command: Validate { file: PathBuf::from("src_file"), integrity_check: IntegrityCheck::Quick, - generate_global_hash: false + regenerate_global_hash: false } } ); diff --git a/martin-mbtiles/src/errors.rs b/martin-mbtiles/src/errors.rs index 3f196c5e0..96f46d883 100644 --- a/martin-mbtiles/src/errors.rs +++ b/martin-mbtiles/src/errors.rs @@ -20,12 +20,15 @@ pub enum MbtError { #[error("Invalid data format for MBTile file {0}")] InvalidDataFormat(String), - #[error("Integrity check failed for MBTile file {0}")] - FailedIntegrityCheck(String), + #[error("Integrity check failed for MBTile file {0} for the following reasons: \n {1:?}")] + FailedIntegrityCheck(String, Vec), #[error("Invalid tile data for MBTile file {0}")] InvalidTileData(String), + #[error("Expected global_hash value in metadata table for MBTiles file {0}")] + GlobalHashValueNotFound(String), + #[error(r#"Filename "{0}" passed to SQLite must be valid UTF-8"#)] InvalidFilenameType(PathBuf), diff --git a/martin-mbtiles/src/mbtiles.rs b/martin-mbtiles/src/mbtiles.rs index 026ca8923..4c6db100e 100644 --- a/martin-mbtiles/src/mbtiles.rs +++ b/martin-mbtiles/src/mbtiles.rs @@ -16,15 +16,15 @@ use martin_tile_utils::{Format, TileInfo}; use serde_json::{Value as JSONValue, Value}; use sqlite_hashes::register_md5_function; use sqlite_hashes::rusqlite::Connection as RusqliteConnection; +use sqlx::sqlite::SqliteRow; use sqlx::{query, Row, SqliteExecutor}; use tilejson::{tilejson, Bounds, Center, TileJSON}; use crate::errors::{MbtError, MbtResult}; -use crate::mbtiles::MbtType::Flat; use crate::mbtiles_queries::{ is_flat_tables_type, is_flat_with_hash_tables_type, is_normalized_tables_type, }; -use crate::MbtError::{FailedIntegrityCheck, InvalidTileData}; +use crate::MbtError::{FailedIntegrityCheck, GlobalHashValueNotFound, InvalidTileData}; #[derive(Clone, Debug, PartialEq)] pub struct Metadata { @@ -388,29 +388,38 @@ impl Mbtiles { Err(MbtError::NoUniquenessConstraint(self.filepath.clone())) } - pub async fn generate_global_hash(&self, conn: &mut T) -> MbtResult<()> + async fn get_global_hash(&self, conn: &mut T) -> MbtResult> where for<'e> &'e mut T: SqliteExecutor<'e>, { + let rusqlite_conn = RusqliteConnection::open(Path::new(&self.filepath()))?; + register_md5_function(&rusqlite_conn)?; let mbttype = self.detect_type(&mut *conn).await?; - let select_from = match mbttype { - Flat => { + let sql = match mbttype { + MbtType::Flat => { println!("Cannot generate global hash, no hash column in flat table format. Skipping global_hash generation..."); - return Ok(()); + return Ok(None); } - MbtType::FlatWithHash => "SELECT 'global_hash', hex(md5_concat(tile_hash)) FROM tiles_with_hash ORDER BY zoom_level, tile_column, tile_row;", - MbtType::Normalized => "SELECT 'global_hash', hex(md5_concat(images.tile_id)) FROM images JOIN map ON images.tile_id=map.tile_id ORDER BY zoom_level, tile_column, tile_row;" + MbtType::FlatWithHash => "SELECT hex(md5_concat(cast(zoom_level AS text), cast(tile_column AS text), cast(tile_row AS text), tile_hash)) FROM tiles_with_hash ORDER BY zoom_level, tile_column, tile_row;", + MbtType::Normalized => "SELECT hex(md5_concat(cast(zoom_level AS text), cast(tile_column AS text), cast(tile_row AS text), tile_id)) FROM map ORDER BY zoom_level, tile_column, tile_row;" + }; - }.to_string(); + Ok(Some(rusqlite_conn.query_row_and_then(sql, [], |row| { + row.get::<_, String>(0) + })?)) + } - let rusqlite_conn = RusqliteConnection::open(Path::new(&self.filepath()))?; - register_md5_function(&rusqlite_conn)?; - rusqlite_conn.execute( - format!("INSERT OR REPLACE INTO metadata(name, value) {select_from}").as_str(), - [], - )?; - Ok(()) + pub async fn generate_global_hash(&self, conn: &mut T) -> MbtResult<()> + where + for<'e> &'e mut T: SqliteExecutor<'e>, + { + if let Some(global_hash) = self.get_global_hash(&mut *conn).await? { + self.set_metadata_value(conn, "global_hash", Some(global_hash)) + .await + } else { + Ok(()) + } } pub async fn validate_mbtiles( @@ -422,20 +431,26 @@ impl Mbtiles { for<'e> &'e mut T: SqliteExecutor<'e>, { // SQLite Integrity check - if "ok" - != match integrity_check { - IntegrityCheck::Full => query("PRAGMA integrity_check;") - .fetch_one(&mut *conn) - .await? - .get::(0), - IntegrityCheck::Quick => query("PRAGMA integrity_check;") - .fetch_one(&mut *conn) - .await? - .get::(0), - IntegrityCheck::Off => "ok".to_string(), + if integrity_check != IntegrityCheck::Off { + let sql = if integrity_check == IntegrityCheck::Full { + "PRAGMA integrity_check;" + } else { + "PRAGMA quick_check;" + }; + + let result = query(sql) + .map(|row: SqliteRow| row.get::(0)) + .fetch_all(&mut *conn) + .await?; + + if result.len() > 1 + || result.get(0).ok_or(FailedIntegrityCheck( + self.filepath().to_string(), + vec!["SQLite could not perform integrity check".to_string()], + ))? != "ok" + { + return Err(FailedIntegrityCheck(self.filepath().to_string(), result)); } - { - return Err(FailedIntegrityCheck(self.filepath().to_string())); } let mbttype = self.detect_type(&mut *conn).await?; @@ -451,40 +466,24 @@ impl Mbtiles { register_md5_function(&rusqlite_conn)?; // Global hash check - if self - .get_metadata_value(&mut *conn, "global_hash") - .await? - .is_none() - { - println!( - "No value for 'global_hash' key found in metadata, skipping global hash validation step..." - ); - } else { - let sql = if mbttype == MbtType::FlatWithHash { - "SELECT * FROM metadata - WHERE name='global_hash' - AND value!=(SELECT hex(md5_concat(tile_hash)) FROM tiles_with_hash ORDER BY zoom_level, tile_column, tile_row);" - } else { - "SELECT * FROM metadata - WHERE name='global_hash' - AND value!=(hex(md5_concat(images.tile_id)) FROM images JOIN map ON images.tile_id=map.tile_id ORDER BY zoom_level, tile_column, tile_row);" - } - .to_string(); - - if rusqlite_conn.prepare(&sql)?.exists(())? { - return Err(InvalidTileData(self.filepath().to_string())); + if let Some(global_hash) = self.get_metadata_value(&mut *conn, "global_hash").await? { + if let Some(new_global_hash) = self.get_global_hash(&mut *conn).await? { + if global_hash != new_global_hash { + return Err(InvalidTileData(self.filepath().to_string())); + } } + } else { + return Err(GlobalHashValueNotFound(self.filepath().to_string())); } // Per-tile hash check let sql = if mbttype == MbtType::FlatWithHash { - "SELECT * FROM tiles_with_hash WHERE tile_hash!=hex(md5(tile_data)) LIMIT 1;" + "SELECT 1 FROM tiles_with_hash WHERE tile_hash != hex(md5(tile_data)) LIMIT 1;" } else { - "SELECT * FROM images WHERE tile_id!=hex(md5(tile_data)) LIMIT 1;" - } - .to_string(); + "SELECT 1 FROM images WHERE tile_id != hex(md5(tile_data)) LIMIT 1;" + }; - if rusqlite_conn.prepare(&sql)?.exists(())? { + if rusqlite_conn.prepare(sql)?.exists(())? { return Err(InvalidTileData(self.filepath().to_string())); } @@ -659,6 +658,10 @@ mod tests { let (mut conn, mbt) = open("../tests/fixtures/files/invalid_zoomed_world_cities.mbtiles").await; + print!( + "VLAUE {:?}", + mbt.validate_mbtiles(IntegrityCheck::Quick, &mut conn).await + ); assert!(matches!( mbt.validate_mbtiles(IntegrityCheck::Quick, &mut conn) .await @@ -666,13 +669,4 @@ mod tests { MbtError::InvalidTileData(..) )); } - - #[actix_rt::test] - async fn validate_file_with_global_hash() { - let (mut conn, mbt) = open("../tests/fixtures/files/zoomed_world_cities.mbtiles").await; - - mbt.validate_mbtiles(IntegrityCheck::Quick, &mut conn) - .await - .unwrap(); - } } diff --git a/martin-mbtiles/src/tile_copier.rs b/martin-mbtiles/src/tile_copier.rs index 9d6155491..45707a383 100644 --- a/martin-mbtiles/src/tile_copier.rs +++ b/martin-mbtiles/src/tile_copier.rs @@ -31,7 +31,7 @@ pub struct TileCopierOptions { src_file: PathBuf, /// MBTiles file to write to dst_file: PathBuf, - /// Output format of the destination file, ignored if the file exists. if not specified, defaults to the type of source + /// Output format of the destination file, ignored if the file exists. If not specified, defaults to the type of source #[cfg_attr(feature = "cli", arg(long, value_enum))] dst_mbttype: Option, /// Specify copying behaviour when tiles with duplicate (zoom_level, tile_column, tile_row) values are found @@ -49,6 +49,9 @@ pub struct TileCopierOptions { /// Compare source file with this file, and only copy non-identical tiles to destination #[cfg_attr(feature = "cli", arg(long))] diff_with_file: Option, + /// Skip generating a global hash for mbtiles validation. By default, if dst_mbttype is flat-with-hash or normalized, generate a global hash and store in the metadata table + #[cfg_attr(feature = "cli", arg(long))] + skip_global_hash: bool, } #[cfg(feature = "cli")] @@ -101,6 +104,7 @@ impl TileCopierOptions { min_zoom: None, max_zoom: None, diff_with_file: None, + skip_global_hash: false, } } @@ -133,6 +137,11 @@ impl TileCopierOptions { self.diff_with_file = Some(diff_with_file); self } + + pub fn skip_global_hash(mut self, skip_global_hash: bool) -> Self { + self.skip_global_hash = skip_global_hash; + self + } } impl TileCopier { @@ -226,6 +235,12 @@ impl TileCopier { } }; + if !self.options.skip_global_hash + && (dst_mbttype == FlatWithHash || dst_mbttype == Normalized) + { + self.dst_mbtiles.generate_global_hash(&mut conn).await?; + } + Ok(conn) } diff --git a/tests/fixtures/files/invalid_zoomed_world_cities.mbtiles b/tests/fixtures/files/invalid_zoomed_world_cities.mbtiles index 9e0e31c0ccf57784db8b1361815f7ca48f0d2afe..3012ef4b175fb2f2f6e8d1dd12ddef0d1d034e2c 100644 GIT binary patch delta 289 zcmZoDX(*YH#FX}GW6AftjO|rK6jPiIJtLxr>FPlevkZk+Y+Vp|honvzwFA=BrHlKrYLP-j{3xQ1CbcQ;Xj?nk+0}kTXdy{SUZuL&4o*^N%;utt zI0%Ad>h2FHNH;eZe}M=tCS5MiIUJrlAb3BB1{=MkzxbHWE6aEBWVaDp5%U^EYM z;xwEK`_TeE;T2PS;~qZVF~cvO@EPXBQ}~nYb&_S#^Bh}I*ecE(*J|=q-0{o4nT)%Z zb8dG$ImWhWbJLZU*czxF!!rq{~Vyk6A{mAa-C skF-KKmbsFKN+KDAF*JH6!yheFYUEZIDoNSfyiLZxNMJ!e)K+G@|LK2Fng9R* diff --git a/tests/fixtures/files/zoomed_world_cities.mbtiles b/tests/fixtures/files/zoomed_world_cities.mbtiles index a79942a59982db2ed2eeb72fb2640108522c11c7..bc33764dbec84c776939e67eaadf5c207d104d8e 100644 GIT binary patch delta 222 zcmZoDX(-vSj7gt|S=l!|CqF4MCq5&wIK#ro$*}%Zm z)X2!m%x&{kCKE={uZ*u4Z!w-{+|Rg;aTVh{#%YYbjE$QWSrQoQ85kJ2nWY)aQj3Z+ z^YfTFm_->&DhpCM*qKE+^AdAYL6W(dc~$xOxh!CIVg-oJ3R0L=oS(Aj*N@{U(QD#92&~PS@RB~!wNoo-rh?$gMn#Y_{%)W@t1OR-rJqG{) delta 222 zcmZoDX(-vSj7guHS=l!|CqF4MCq5&wIK$b<$;iUh(agxm(cIX~+||Ox*~r|;(%Hq? z!o|eY(QNZoCKE={b&NY1Uog&MY-F6sc!co+;}6EAjCVFGvLrCpGcYjlGb=M98OhDT zEXtTwoS(cSi%VsE2%6<rVVr4|)u=I1ektVu~NPA