From 7107db5887f6233f3066077513978cbfe2757c46 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 17 Mar 2024 21:17:30 -0400 Subject: [PATCH] Fix apply-patch documentation (#1261) --- docs/src/mbtiles-diff.md | 4 ++-- mbtiles/src/bin/mbtiles.rs | 7 ++++--- mbtiles/src/copier.rs | 29 +++++++++++++-------------- mbtiles/src/patcher.rs | 41 ++++++++++++++++---------------------- mbtiles/src/validation.rs | 5 ++--- mbtiles/tests/copy.rs | 6 ++---- 6 files changed, 41 insertions(+), 51 deletions(-) diff --git a/docs/src/mbtiles-diff.md b/docs/src/mbtiles-diff.md index 607b8df0..b2d627b2 100644 --- a/docs/src/mbtiles-diff.md +++ b/docs/src/mbtiles-diff.md @@ -9,7 +9,7 @@ insertions, and deletions as `NULL` values), for both the tile and metadata tabl There is one exception: `agg_tiles_hash` metadata value will be renamed to `agg_tiles_hash_after_apply`, and a new `agg_tiles_hash` will be generated for the diff file itself. This is done to avoid confusion when applying the diff -file to the original file, as the `agg_tiles_hash` value will be different after the diff is applied. The `apply-diff` +file to the original file, as the `agg_tiles_hash` value will be different after the diff is applied. The `apply-patch` command will automatically rename the `agg_tiles_hash_after_apply` value back to `agg_tiles_hash` when applying the diff. @@ -18,7 +18,7 @@ diff. mbtiles diff file1.mbtiles file2.mbtiles diff.mbtiles # If diff.mbtiles is applied to file1.mbtiles, it will produce file2.mbtiles -mbtiles apply-diff file1.mbtiles diff.mbtiles file2a.mbtiles +mbtiles apply-patch file1.mbtiles diff.mbtiles file2a.mbtiles # file2.mbtiles and file2a.mbtiles should now be the same # Validate both files and see that their hash values are identical diff --git a/mbtiles/src/bin/mbtiles.rs b/mbtiles/src/bin/mbtiles.rs index fc3d7228..6776bcbe 100644 --- a/mbtiles/src/bin/mbtiles.rs +++ b/mbtiles/src/bin/mbtiles.rs @@ -102,11 +102,12 @@ pub struct CopyArgs { pub options: SharedCopyOpts, /// Compare source file with this file, and only copy non-identical tiles to destination. /// Use `mbtiles diff` as a more convenient way to generate this file. - /// It should be later possible to run `mbtiles apply-diff` to merge it in. + /// Use `mbtiles apply-patch` or `mbtiles copy --apply-patch` to apply the diff file. #[arg(long, conflicts_with("apply_patch"))] diff_with_file: Option, - /// Compare source file with this file, and only copy non-identical tiles to destination. - /// It should be later possible to run `mbtiles apply-diff SRC_FILE DST_FILE` to get the same DIFF file. + /// Apply a patch file while copying src to dst. + /// Use `mbtiles diff` or `mbtiles copy --diff-with-file` to generate the patch file. + /// Use `mbtiles apply-patch` to apply the patch file in-place, without making a copy of the original. #[arg(long, conflicts_with("diff_with_file"))] apply_patch: Option, } diff --git a/mbtiles/src/copier.rs b/mbtiles/src/copier.rs index ebe8b76c..c3b450fb 100644 --- a/mbtiles/src/copier.rs +++ b/mbtiles/src/copier.rs @@ -63,11 +63,8 @@ pub struct MbtilesCopier { /// Bounding box to copy, in the format `min_lon,min_lat,max_lon,max_lat`. Can be used multiple times. pub bbox: Vec, /// Compare source file with this file, and only copy non-identical tiles to destination. - /// Use `mbtiles diff` as a more convenient way to generate this file. - /// It should be later possible to run `mbtiles apply-diff` to merge it in. pub diff_with_file: Option, - /// Compare source file with this file, and only copy non-identical tiles to destination. - /// It should be later possible to run `mbtiles apply-diff SRC_FILE DST_FILE` to get the same DIFF file. + /// Apply a patch file while copying src to dst. pub apply_patch: Option, /// Skip generating a global hash for mbtiles validation. By default, `mbtiles` will compute `agg_tiles_hash` metadata value. pub skip_agg_tiles_hash: bool, @@ -128,7 +125,7 @@ impl MbtileCopierInt { (Some(dif_file), None) | (None, Some(dif_file)) => { let dif_mbt = Mbtiles::new(dif_file)?; let dif_type = dif_mbt.open_and_detect_type().await?; - Some((dif_mbt, dif_type, dif_type)) + Some((dif_mbt, dif_type)) } (Some(_), Some(_)) => unreachable!(), // validated in the Self::new _ => None, @@ -157,7 +154,7 @@ impl MbtileCopierInt { CopyType::Metadata => "metadata ", }; let dst_type: MbtType; - if let Some((dif_mbt, dif_type, _)) = &dif { + if let Some((dif_mbt, dif_type)) = &dif { if !is_empty_db { return Err(MbtError::NonEmptyTargetFile(self.options.dst_file)); } @@ -183,9 +180,9 @@ impl MbtileCopierInt { self.init_new_schema(&mut conn, src_type, dst_type).await?; } + // SAFETY: This must be scoped to make sure the handle is dropped before we continue using conn + // Make sure not to execute any other queries while the handle is locked { - // SAFETY: This must be scoped to make sure the handle is dropped before we continue using conn - // Make sure not to execute any other queries while the handle is locked let mut handle_lock = conn.lock_handle().await?; let handle = handle_lock.as_raw_handle().as_ptr(); @@ -205,7 +202,7 @@ impl MbtileCopierInt { } if self.options.copy.copy_metadata() { - self.copy_metadata(&rusqlite_conn, dif.as_ref(), on_duplicate)?; + self.copy_metadata(&rusqlite_conn, dif.is_some(), on_duplicate)?; } else { debug!("Skipping copying metadata"); } @@ -215,9 +212,11 @@ impl MbtileCopierInt { dst_mbt.update_agg_tiles_hash(&mut conn).await?; } + if dif.is_some() { + detach_db(&mut conn, "diffDb").await?; + } + detach_db(&mut conn, "sourceDb").await?; - // Ignore error because we might not have attached diffDb - let _ = detach_db(&mut conn, "diffDb").await; Ok(conn) } @@ -225,12 +224,12 @@ impl MbtileCopierInt { fn copy_metadata( &self, rusqlite_conn: &Connection, - dif: Option<&(Mbtiles, MbtType, MbtType)>, + is_diff: bool, on_duplicate: CopyDuplicateMode, ) -> Result<(), MbtError> { let on_dupl = on_duplicate.to_sql(); let sql; - if dif.is_some() { + if is_diff { // Insert all rows from diffDb.metadata if they do not exist or are different in sourceDb.metadata. // Also insert all names from sourceDb.metadata that do not exist in diffDb.metadata, with their value set to NULL. // Rename agg_tiles_hash to agg_tiles_hash_after_apply because agg_tiles_hash will be auto-added later @@ -282,14 +281,14 @@ impl MbtileCopierInt { fn copy_tiles( &self, rusqlite_conn: &Connection, - dif: Option<&(Mbtiles, MbtType, MbtType)>, + dif: Option<&(Mbtiles, MbtType)>, src_type: MbtType, dst_type: MbtType, on_duplicate: CopyDuplicateMode, ) -> Result<(), MbtError> { let on_dupl = on_duplicate.to_sql(); - let select_from = if let Some((_, dif_type, _)) = &dif { + let select_from = if let Some((_, dif_type)) = &dif { if self.options.diff_with_file.is_some() { Self::get_select_from_with_diff(*dif_type, dst_type) } else { diff --git a/mbtiles/src/patcher.rs b/mbtiles/src/patcher.rs index 3ae43163..6e983ffe 100644 --- a/mbtiles/src/patcher.rs +++ b/mbtiles/src/patcher.rs @@ -14,60 +14,53 @@ pub async fn apply_patch(base_file: PathBuf, patch_file: PathBuf) -> MbtResult<( let mut conn = base_mbt.open().await?; let base_type = base_mbt.detect_type(&mut conn).await?; - patch_mbt.attach_to(&mut conn, "patchDb").await?; info!("Applying patch file {patch_mbt} ({patch_type}) to {base_mbt} ({base_type})"); + + patch_mbt.attach_to(&mut conn, "patchDb").await?; let select_from = get_select_from(base_type, patch_type); let (main_table, insert1, insert2) = get_insert_sql(base_type, select_from); - query(&format!("{insert1} WHERE tile_data NOTNULL")) - .execute(&mut conn) - .await?; + let sql = format!("{insert1} WHERE tile_data NOTNULL"); + query(&sql).execute(&mut conn).await?; if let Some(insert2) = insert2 { - query(&format!("{insert2} WHERE tile_data NOTNULL")) - .execute(&mut conn) - .await?; + let sql = format!("{insert2} WHERE tile_data NOTNULL"); + query(&sql).execute(&mut conn).await?; } - query(&format!( + let sql = format!( " DELETE FROM {main_table} WHERE (zoom_level, tile_column, tile_row) IN ( SELECT zoom_level, tile_column, tile_row FROM ({select_from} WHERE tile_data ISNULL) )" - )) - .execute(&mut conn) - .await?; + ); + query(&sql).execute(&mut conn).await?; if base_type.is_normalized() { debug!("Removing unused tiles from the images table (normalized schema)"); - query("DELETE FROM images WHERE tile_id NOT IN (SELECT tile_id FROM map)") - .execute(&mut conn) - .await?; + let sql = "DELETE FROM images WHERE tile_id NOT IN (SELECT tile_id FROM map)"; + query(sql).execute(&mut conn).await?; } // Copy metadata from patchDb to the destination file, replacing existing values // Convert 'agg_tiles_hash_in_patch' into 'agg_tiles_hash' // Delete metadata entries if the value is NULL in patchDb - query(&format!( + let sql = format!( " INSERT OR REPLACE INTO metadata (name, value) SELECT IIF(name = '{AGG_TILES_HASH_AFTER_APPLY}', '{AGG_TILES_HASH}', name) as name, value FROM patchDb.metadata WHERE name NOTNULL AND name != '{AGG_TILES_HASH}';" - )) - .execute(&mut conn) - .await?; + ); + query(&sql).execute(&mut conn).await?; - query( - " + let sql = " DELETE FROM metadata - WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);", - ) - .execute(&mut conn) - .await?; + WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);"; + query(sql).execute(&mut conn).await?; detach_db(&mut conn, "patchDb").await } diff --git a/mbtiles/src/validation.rs b/mbtiles/src/validation.rs index d583af07..1ac919f2 100644 --- a/mbtiles/src/validation.rs +++ b/mbtiles/src/validation.rs @@ -24,8 +24,7 @@ use crate::{invert_y_value, Mbtiles}; /// Metadata key for the aggregate tiles hash value pub const AGG_TILES_HASH: &str = "agg_tiles_hash"; -/// Metadata key for a diff file, -/// describing the eventual [`AGG_TILES_HASH`] value once the diff is applied +/// Metadata key for a diff file, describing the eventual [`AGG_TILES_HASH`] value of the resulting tileset once the diff is applied pub const AGG_TILES_HASH_AFTER_APPLY: &str = "agg_tiles_hash_after_apply"; #[derive(Debug, Clone, Copy, Hash, PartialEq, Eq, EnumDisplay, Serialize)] @@ -116,7 +115,7 @@ impl Mbtiles { tested_zoom = r.zoom_level.unwrap_or(-1); } - // Afterwards, iterate over tiles in all allowed zooms and check for consistency + // Afterward, iterate over tiles in all allowed zooms and check for consistency for z in tilejson.minzoom.unwrap_or(0)..=tilejson.maxzoom.unwrap_or(18) { if i64::from(z) == tested_zoom { continue; diff --git a/mbtiles/tests/copy.rs b/mbtiles/tests/copy.rs index f81438d8..97efcc64 100644 --- a/mbtiles/tests/copy.rs +++ b/mbtiles/tests/copy.rs @@ -5,7 +5,6 @@ use std::str::from_utf8; use ctor::ctor; use insta::{allow_duplicates, assert_snapshot}; use itertools::Itertools as _; -use log::info; use martin_tile_utils::xyz_to_bbox; use mbtiles::AggHashType::Verify; use mbtiles::IntegrityCheckType::Off; @@ -105,14 +104,14 @@ async fn open(file: &str) -> MbtResult<(Mbtiles, SqliteConnection)> { /// Run [`MbtilesCopier`], the first two params are source and destination [`Mbtiles`] refs, the rest are optional (key => val)* params. macro_rules! copy { - ($src_path:expr, $dst_path:expr $( , $key:tt => $val:expr )* $(,)?) => {{ + ($src_path:expr, $dst_path:expr $( , $key:tt => $val:expr )* $(,)?) => { MbtilesCopier { src_file: $src_path, dst_file: $dst_path $(, $key : $val)*, ..Default::default() }.run().await.unwrap() - }}; + }; } /// Same as the copy! macro, but with the result dumped. @@ -509,7 +508,6 @@ async fn patch_on_copy( let v2 = v2_type.map_or("dflt", shorten); let prefix = format!("{v1}+{dif}={v2}"); - info!("TEST: Compare v1 with v2, and copy anything that's different (i.e. mathematically: v2-v1=diff)"); let (v2_mbt, mut v2_cn) = open!(patch_on_copy, "{prefix}__v2"); copy! { databases.path("v1", v1_type),