Minor cleanup (#1008)

- Add separators to long literal. - Use first() instead of get(0) - Move some SQL-making code to their own functions - Minor mathematics and rounding cleanup --------- Co-authored-by: Yuri Astrakhan <[email protected]>
maplibre · Nov 17, 2023 · a3b2bfd · a3b2bfd
1 parent cd375ff
commit a3b2bfd
Show file tree

Hide file tree

Showing 11 changed files with 221 additions and 180 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -11,7 +11,9 @@ readme = "README.md"
 homepage = "https://martin.maplibre.org/"
 
 [workspace.lints.rust]
-unsafe_code = "forbid"
+# Lints inheritance from workspace is not yet supported
+# Crate mbtiles uses unsafe code, so we can't forbid it here
+# unsafe_code = "forbid"
 
 [workspace.lints.clippy]
 pedantic = "warn"

diff --git a/martin-tile-utils/src/lib.rs b/martin-tile-utils/src/lib.rs
@@ -3,8 +3,12 @@
 // This code was partially adapted from https://github.com/maplibre/mbtileserver-rs
 // project originally written by Kaveh Karimi and licensed under MIT/Apache-2.0
 
+use std::f64::consts::PI;
 use std::fmt::Display;
 
+pub const EARTH_CIRCUMFERENCE: f64 = 40_075_016.7;
+pub const EARTH_RADIUS: f64 = EARTH_CIRCUMFERENCE / 2.0 / PI;
+
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
 pub enum Format {
     Gif,

diff --git a/martin/src/lib.rs b/martin/src/lib.rs
@@ -1,4 +1,5 @@
 #![doc = include_str!("../README.md")]
+#![forbid(unsafe_code)]
 
 pub mod args;
 mod config;

diff --git a/mbtiles/Cargo.toml b/mbtiles/Cargo.toml
@@ -1,3 +1,5 @@
+lints.workspace = true
+
 [package]
 name = "mbtiles"
 version = "0.7.4"
@@ -52,17 +54,3 @@ path = "src/lib.rs"
 name = "mbtiles"
 path = "src/bin/mbtiles.rs"
 required-features = ["cli"]
-
-# Lints inheritance from workspace is not yet supported
-# Copy/pasting the list, modifying the `unsafe_code` requirement
-[lints.rust]
-unsafe_code = "allow"
-
-[lints.clippy]
-# We are not ready to use pedantic clippy yet
-#pedantic = "warn"
-derive_partial_eq_without_eq = "allow"
-implicit_hasher = "allow"
-missing_errors_doc = "allow"
-missing_panics_doc = "allow"
-module_name_repetitions = "allow"
diff --git a/mbtiles/src/copier.rs b/mbtiles/src/copier.rs
@@ -6,7 +6,7 @@ use clap::{Args, ValueEnum};
 use enum_display::EnumDisplay;
 use log::{debug, info};
 use sqlite_hashes::rusqlite;
-use sqlite_hashes::rusqlite::params_from_iter;
+use sqlite_hashes::rusqlite::{params_from_iter, Connection};
 use sqlx::{query, Executor as _, Row, SqliteConnection};
 
 use crate::errors::MbtResult;
@@ -195,57 +195,45 @@ impl MbtileCopierInt {
         let (on_dupl, sql_cond) = self.get_on_duplicate_sql(dst_type);
 
         debug!("Copying tiles with 'INSERT {on_dupl}' {src_type} -> {dst_type} ({sql_cond})");
-        // 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();
 
-        // SAFETY: this is safe as long as handle_lock is valid. We will drop the lock.
-        let rusqlite_conn = unsafe { rusqlite::Connection::from_handle(handle) }?;
+        {
+            // 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();
+
+            // SAFETY: this is safe as long as handle_lock is valid. We will drop the lock.
+            let rusqlite_conn = unsafe { rusqlite::Connection::from_handle(handle) }?;
+
+            Self::copy_tiles(
+                &rusqlite_conn,
+                dst_type,
+                &query_args,
+                &on_dupl,
+                &select_from,
+                &sql_cond,
+            )?;
+
+            self.copy_metadata(&rusqlite_conn, &dif, &on_dupl)?;
+        }
 
-        match dst_type {
-            Flat => {
-                let sql = format!(
-                    "
-    INSERT {on_dupl} INTO tiles
-           (zoom_level, tile_column, tile_row, tile_data)
-    {select_from} {sql_cond}"
-                );
-                debug!("Copying to {dst_type} with {sql} {query_args:?}");
-                rusqlite_conn.execute(&sql, params_from_iter(query_args))?
-            }
-            FlatWithHash => {
-                let sql = format!(
-                    "
-    INSERT {on_dupl} INTO tiles_with_hash
-           (zoom_level, tile_column, tile_row, tile_data, tile_hash)
-    {select_from} {sql_cond}"
-                );
-                debug!("Copying to {dst_type} with {sql} {query_args:?}");
-                rusqlite_conn.execute(&sql, params_from_iter(query_args))?
-            }
-            Normalized { .. } => {
-                let sql = format!(
-                    "
-    INSERT OR IGNORE INTO images
-           (tile_id, tile_data)
-    SELECT tile_hash as tile_id, tile_data
-    FROM ({select_from})"
-                );
-                debug!("Copying to {dst_type} with {sql} {query_args:?}");
-                rusqlite_conn.execute(&sql, params_from_iter(&query_args))?;
+        if !self.options.skip_agg_tiles_hash {
+            dst_mbt.update_agg_tiles_hash(&mut conn).await?;
+        }
 
-                let sql = format!(
-                    "
-    INSERT {on_dupl} INTO map
-           (zoom_level, tile_column, tile_row, tile_id)
-    SELECT zoom_level, tile_column, tile_row, tile_hash as tile_id
-    FROM ({select_from} {sql_cond})"
-                );
-                debug!("Copying to {dst_type} with {sql} {query_args:?}");
-                rusqlite_conn.execute(&sql, params_from_iter(query_args))?
-            }
-        };
+        detach_db(&mut conn, "sourceDb").await?;
+        // Ignore error because we might not have attached diffDb
+        let _ = detach_db(&mut conn, "diffDb").await;
+
+        Ok(conn)
+    }
 
+    fn copy_metadata(
+        &self,
+        rusqlite_conn: &Connection,
+        dif: &Option<(Mbtiles, MbtType, MbtType)>,
+        on_dupl: &str,
+    ) -> Result<(), MbtError> {
         let sql;
         if dif.is_some() {
             // Insert all rows from diffDb.metadata if they do not exist or are different in sourceDb.metadata.
@@ -295,20 +283,59 @@ impl MbtileCopierInt {
             debug!("Copying metadata with {sql}");
         }
         rusqlite_conn.execute(&sql, [])?;
+        Ok(())
+    }
 
-        // SAFETY: must drop rusqlite_conn before handle_lock, or place the code since lock in a separate scope
-        drop(rusqlite_conn);
-        drop(handle_lock);
+    fn copy_tiles(
+        rusqlite_conn: &Connection,
+        dst_type: MbtType,
+        query_args: &Vec<u8>,
+        on_dupl: &str,
+        select_from: &str,
+        sql_cond: &str,
+    ) -> Result<(), MbtError> {
+        let sql = match dst_type {
+            Flat => {
+                format!(
+                    "
+    INSERT {on_dupl} INTO tiles
+           (zoom_level, tile_column, tile_row, tile_data)
+    {select_from} {sql_cond}"
+                )
+            }
+            FlatWithHash => {
+                format!(
+                    "
+    INSERT {on_dupl} INTO tiles_with_hash
+           (zoom_level, tile_column, tile_row, tile_data, tile_hash)
+    {select_from} {sql_cond}"
+                )
+            }
+            Normalized { .. } => {
+                let sql = format!(
+                    "
+    INSERT OR IGNORE INTO images
+           (tile_id, tile_data)
+    SELECT tile_hash as tile_id, tile_data
+    FROM ({select_from})"
+                );
+                debug!("Copying to {dst_type} with {sql} {query_args:?}");
+                rusqlite_conn.execute(&sql, params_from_iter(query_args))?;
 
-        if !self.options.skip_agg_tiles_hash {
-            dst_mbt.update_agg_tiles_hash(&mut conn).await?;
-        }
+                format!(
+                    "
+    INSERT {on_dupl} INTO map
+           (zoom_level, tile_column, tile_row, tile_id)
+    SELECT zoom_level, tile_column, tile_row, tile_hash as tile_id
+    FROM ({select_from} {sql_cond})"
+                )
+            }
+        };
 
-        detach_db(&mut conn, "sourceDb").await?;
-        // Ignore error because we might not have attached diffDb
-        let _ = detach_db(&mut conn, "diffDb").await;
+        debug!("Copying to {dst_type} with {sql} {query_args:?}");
+        rusqlite_conn.execute(&sql, params_from_iter(query_args))?;
 
-        Ok(conn)
+        Ok(())
     }
 
     /// Check if the detected destination file type matches the one given by the options

diff --git a/mbtiles/src/metadata.rs b/mbtiles/src/metadata.rs
@@ -23,6 +23,7 @@ pub struct Metadata {
     pub json: Option<JSONValue>,
 }
 
+#[allow(clippy::trivially_copy_pass_by_ref)]
 fn serialize_ti<S: Serializer>(ti: &TileInfo, serializer: S) -> Result<S::Ok, S::Error> {
     let mut s = serializer.serialize_struct("TileInfo", 2)?;
     s.serialize_field("format", &ti.format.to_string())?;

diff --git a/mbtiles/src/patcher.rs b/mbtiles/src/patcher.rs
@@ -5,7 +5,7 @@ use sqlx::query;
 
 use crate::queries::detach_db;
 use crate::MbtType::{Flat, FlatWithHash, Normalized};
-use crate::{MbtResult, Mbtiles, AGG_TILES_HASH, AGG_TILES_HASH_IN_DIFF};
+use crate::{MbtResult, MbtType, Mbtiles, AGG_TILES_HASH, AGG_TILES_HASH_IN_DIFF};
 
 pub async fn apply_patch(src_file: PathBuf, patch_file: PathBuf) -> MbtResult<()> {
     let src_mbt = Mbtiles::new(src_file)?;
@@ -17,7 +17,59 @@ pub async fn apply_patch(src_file: PathBuf, patch_file: PathBuf) -> MbtResult<()
     patch_mbt.attach_to(&mut conn, "patchDb").await?;
 
     info!("Applying patch file {patch_mbt} ({patch_type}) to {src_mbt} ({src_type})");
-    let select_from = if src_type == Flat {
+    let select_from = get_select_from(src_type, patch_type);
+    let (main_table, insert_sql) = get_insert_sql(src_type, select_from);
+
+    for statement in insert_sql {
+        query(&format!("{statement} WHERE tile_data NOTNULL"))
+            .execute(&mut conn)
+            .await?;
+    }
+
+    query(&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?;
+
+    if src_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?;
+    }
+
+    // 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!(
+        "
+    INSERT OR REPLACE INTO metadata (name, value)
+    SELECT IIF(name = '{AGG_TILES_HASH_IN_DIFF}', '{AGG_TILES_HASH}', name) as name,
+           value
+    FROM patchDb.metadata
+    WHERE name NOTNULL AND name != '{AGG_TILES_HASH}';"
+    ))
+    .execute(&mut conn)
+    .await?;
+
+    query(
+        "
+    DELETE FROM metadata
+    WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);",
+    )
+    .execute(&mut conn)
+    .await?;
+
+    detach_db(&mut conn, "patchDb").await
+}
+
+fn get_select_from(src_type: MbtType, patch_type: MbtType) -> &'static str {
+    if src_type == Flat {
         "SELECT zoom_level, tile_column, tile_row, tile_data FROM patchDb.tiles"
     } else {
         match patch_type {
@@ -39,9 +91,10 @@ pub async fn apply_patch(src_file: PathBuf, patch_file: PathBuf) -> MbtResult<()
             }
         }
     }
-    .to_string();
+}
 
-    let (main_table, insert_sql) = match src_type {
+fn get_insert_sql(src_type: MbtType, select_from: &str) -> (&'static str, Vec<String>) {
+    match src_type {
         Flat => (
             "tiles",
             vec![format!(
@@ -75,54 +128,7 @@ pub async fn apply_patch(src_file: PathBuf, patch_file: PathBuf) -> MbtResult<()
                 ),
             ],
         ),
-    };
-
-    for statement in insert_sql {
-        query(&format!("{statement} WHERE tile_data NOTNULL"))
-            .execute(&mut conn)
-            .await?;
-    }
-
-    query(&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?;
-
-    if src_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?;
     }
-
-    // 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!(
-        "
-    INSERT OR REPLACE INTO metadata (name, value)
-    SELECT IIF(name = '{AGG_TILES_HASH_IN_DIFF}', '{AGG_TILES_HASH}', name) as name,
-           value
-    FROM patchDb.metadata
-    WHERE name NOTNULL AND name != '{AGG_TILES_HASH}';"
-    ))
-    .execute(&mut conn)
-    .await?;
-
-    query(
-        "
-    DELETE FROM metadata
-    WHERE name IN (SELECT name FROM patchDb.metadata WHERE value ISNULL);",
-    )
-    .execute(&mut conn)
-    .await?;
-
-    detach_db(&mut conn, "patchDb").await
 }
 
 #[cfg(test)]