Fix SQL comments when func or table is pre-configured (#1045)

When a SQL comment is set on a table or a function to customize tilejson, and that tbl/func is pre-configured as part of the config file, the comment was silently ignored. Now both table and function cases are handled correctly. Also, update docs to not include function parameters - makes SQL example a bit simpler. Thanks @jjcfrancisco for reporting! Fixes: #1044
maplibre · Dec 7, 2023 · f88db05 · f88db05
1 parent ff62b3c
commit f88db05
Show file tree

Hide file tree

Showing 18 changed files with 151 additions and 44 deletions.
diff --git a/docs/src/sources-pg-functions.md b/docs/src/sources-pg-functions.md
@@ -122,7 +122,7 @@ To modify automatically generated `TileJSON`, you can add a valid JSON as an SQL
 
 ```sql
 DO $do$ BEGIN
-    EXECUTE 'COMMENT ON FUNCTION my_function_name(INT4, INT4, INT4) IS $tj$' || $$
+    EXECUTE 'COMMENT ON FUNCTION my_function_name IS $tj$' || $$
     {
         "description": "my new description",
         "attribution": "my attribution",

diff --git a/justfile b/justfile
@@ -155,7 +155,7 @@ test-int: clean-test install-sqlx
     fi
 
 # Run integration tests and save its output as the new expected output
-bless: restart clean-test bless-tests bless-insta-martin bless-insta-mbtiles
+bless: restart clean-test bless-insta-martin bless-insta-mbtiles bless-tests
     rm -rf tests/temp
     tests/test.sh
     rm -rf tests/expected

diff --git a/martin/src/pg/config_table.rs b/martin/src/pg/config_table.rs
@@ -28,11 +28,11 @@ pub struct TableInfo {
     pub geometry_column: String,
 
     /// Geometry column has a spatial index
-    #[serde(skip_deserializing, skip_serializing)]
+    #[serde(skip)]
     pub geometry_index: Option<bool>,
 
     /// Flag indicating if table is actually a view (PostgreSQL relkind = 'v')
-    #[serde(skip_deserializing, skip_serializing)]
+    #[serde(skip)]
     pub is_view: Option<bool>,
 
     /// Feature id column name
@@ -66,7 +66,7 @@ pub struct TableInfo {
     pub properties: Option<BTreeMap<String, String>>,
 
     /// Mapping of properties to the actual table columns
-    #[serde(skip_deserializing, skip_serializing)]
+    #[serde(skip)]
     pub prop_mapping: HashMap<String, String>,
 
     #[serde(flatten, skip_serializing)]

diff --git a/martin/src/pg/configurator.rs b/martin/src/pg/configurator.rs
@@ -9,7 +9,7 @@ use crate::args::BoundsCalcType;
 use crate::pg::config::{PgConfig, PgInfo};
 use crate::pg::config_function::{FuncInfoSources, FunctionInfo};
 use crate::pg::config_table::{TableInfo, TableInfoSources};
-use crate::pg::function_source::query_available_function;
+use crate::pg::function_source::{merge_func_info, query_available_function};
 use crate::pg::pg_source::{PgSource, PgSqlInfo};
 use crate::pg::pool::PgPool;
 use crate::pg::table_source::{
@@ -238,20 +238,22 @@ impl PgBuilder {
                 warn!("No functions found in schema {}. Only functions like (z,x,y) -> bytea and similar are considered. See README.md", cfg_inf.schema);
                 continue;
             }
-            let Some((pg_sql, _)) = find_info(db_funcs, &cfg_inf.function, "function", id) else {
+            let func_name = &cfg_inf.function;
+            let Some((pg_sql, db_inf)) = find_info(db_funcs, func_name, "function", id) else {
                 continue;
             };
 
-            let dup = !used.insert((&cfg_inf.schema, &cfg_inf.function));
-            let dup = if dup { "duplicate " } else { "" };
+            let merged_inf = merge_func_info(cfg_inf, db_inf);
 
-            let id2 = self.resolve_id(id, cfg_inf);
-            self.add_func_src(&mut res, id2.clone(), cfg_inf, pg_sql.clone());
+            let dup = !used.insert((&cfg_inf.schema, func_name));
+            let dup = if dup { "duplicate " } else { "" };
+            let id2 = self.resolve_id(id, &merged_inf);
+            self.add_func_src(&mut res, id2.clone(), &merged_inf, pg_sql.clone());
             warn_on_rename(id, &id2, "Function");
             let signature = &pg_sql.signature;
             info!("Configured {dup}source {id2} from the function {signature}");
             debug!("{id2} query: {}", pg_sql.query);
-            info_map.insert(id2, cfg_inf.clone());
+            info_map.insert(id2, merged_inf);
         }
 
         // Sort the discovered sources by schema and function name to ensure a consistent behavior

diff --git a/martin/src/pg/function_source.rs b/martin/src/pg/function_source.rs
@@ -118,6 +118,14 @@ pub async fn query_available_function(pool: &PgPool) -> PgResult<SqlFuncInfoMapM
     Ok(res)
 }
 
+pub fn merge_func_info(cfg_inf: &FunctionInfo, db_inf: &FunctionInfo) -> FunctionInfo {
+    FunctionInfo {
+        // TileJson does not need to be merged because it cannot be de-serialized from config
+        tilejson: db_inf.tilejson.clone(),
+        ..cfg_inf.clone()
+    }
+}
+
 fn jsonb_to_vec(jsonb: &Option<Value>) -> Option<Vec<String>> {
     jsonb.as_ref().map(|json| {
         json.as_array()

diff --git a/martin/src/pg/scripts/query_available_function.sql b/martin/src/pg/scripts/query_available_function.sql
@@ -37,7 +37,7 @@ WITH
         GROUP BY specific_name),
     --
     comments AS (
-        -- list of all comments associated with the comments
+        -- list of all comments associated with the function
         SELECT pg_namespace.nspname                    AS schema,
                pg_proc.proname                         AS name,
                obj_description(pg_proc.oid, 'pg_proc') AS description

diff --git a/martin/src/pg/table_source.rs b/martin/src/pg/table_source.rs
@@ -236,8 +236,11 @@ pub fn merge_table_info(
         schema: db_inf.schema.clone(),
         table: db_inf.table.clone(),
         geometry_column: db_inf.geometry_column.clone(),
+        // These values are not serialized, so copy auto-detected values from the database
         geometry_index: db_inf.geometry_index,
         is_view: db_inf.is_view,
+        tilejson: db_inf.tilejson.clone(),
+        // Srid requires some logic
         srid: calc_srid(&table_id, new_id, db_inf.srid, cfg_inf.srid, default_srid)?,
         prop_mapping: HashMap::new(),
         ..cfg_inf.clone()

diff --git a/martin/tests/pg_server_test.rs b/martin/tests/pg_server_test.rs
@@ -176,29 +176,24 @@ postgres:
         .insert_header(("x-rewrite-url", "/tiles/table_source?token=martin"))
         .to_request();
     let result: TileJSON = call_and_read_body_json(&app, req).await;
-    assert_eq!(
-        result,
-        serde_json::from_str(indoc! {r#"
-{
-  "name": "table_source",
-  "description": "public.table_source.geom",
-  "tilejson": "3.0.0",
-  "tiles": [
-    "http://localhost:8080/tiles/table_source/{z}/{x}/{y}?token=martin"
-  ],
-  "vector_layers": [
-    {
-      "id": "table_source",
-      "fields": {
-        "gid": "int4"
-      }
-    }
-  ],
-  "bounds": [-180.0, -90.0, 180.0, 90.0]
-}
-        "#})
-        .unwrap()
-    );
+    assert_yaml_snapshot!(result, @r###"
+    ---
+    tilejson: 3.0.0
+    tiles:
+      - "http://localhost:8080/tiles/table_source/{z}/{x}/{y}?token=martin"
+    vector_layers:
+      - id: table_source
+        fields:
+          gid: int4
+    bounds:
+      - -180
+      - -90
+      - 180
+      - 90
+    name: table_source
+    foo:
+      bar: foo
+    "###);
 }
 
 #[actix_rt::test]
@@ -1016,15 +1011,16 @@ tables:
     tilejson: 3.0.0
     tiles: []
     vector_layers:
-      - id: no_id
+      - id: MixPoints
         fields:
+          Gid: int4
           TABLE: text
     bounds:
       - -180
       - -90
       - 180
       - 90
-    description: MixedCase.MixPoints.Geom
+    description: a description from comment on table
     name: no_id
     "###);
 

diff --git a/tests/config.yaml b/tests/config.yaml
@@ -160,6 +160,11 @@ postgres:
       maxzoom: 30
       bounds: [-180.0, -90.0, 180.0, 90.0]
 
+    fnc_Mixed_Name:
+      schema: MixedCase
+      function: function_Mixed_Name
+
+
 pmtiles:
   sources:
     pmt: tests/fixtures/pmtiles/stamen_toner__raster_CC-BY+ODbL_z3.pmtiles

diff --git a/tests/expected/auto/fnc_comment.json b/tests/expected/auto/fnc_comment.json
@@ -0,0 +1,17 @@
+{
+  "tilejson": "3.0.0",
+  "tiles": [
+    "http://localhost:3111/function_Mixed_Name/{z}/{x}/{y}"
+  ],
+  "vector_layers": [
+    {
+      "id": "MixedCase.function_Mixed_Name",
+      "fields": {
+        "Geom": "",
+        "TABLE": ""
+      }
+    }
+  ],
+  "description": "a function source with MixedCase name",
+  "name": "function_Mixed_Name"
+}
diff --git a/tests/expected/auto/tbl_comment.json b/tests/expected/auto/tbl_comment.json
@@ -0,0 +1,23 @@
+{
+  "tilejson": "3.0.0",
+  "tiles": [
+    "http://localhost:3111/MixPoints/{z}/{x}/{y}"
+  ],
+  "vector_layers": [
+    {
+      "id": "MixPoints",
+      "fields": {
+        "Gid": "int4",
+        "TABLE": "text"
+      }
+    }
+  ],
+  "bounds": [
+    -170.94984639004662,
+    -84.20025580733805,
+    167.70892858284475,
+    74.23573284753762
+  ],
+  "description": "a description from comment on table",
+  "name": "MixPoints"
+}
diff --git a/tests/expected/configured/catalog_cfg.json b/tests/expected/configured/catalog_cfg.json
@@ -2,7 +2,7 @@
   "tiles": {
     "MixPoints": {
       "content_type": "application/x-protobuf",
-      "description": "MixedCase.MixPoints.Geom"
+      "description": "a description from comment on table"
     },
     "auto_table": {
       "content_type": "application/x-protobuf",
@@ -12,9 +12,12 @@
       "content_type": "application/x-protobuf",
       "description": "autodetect.bigint_table.geom"
     },
-    "function_zxy_query": {
+    "fnc_Mixed_Name": {
       "content_type": "application/x-protobuf",
-      "description": "public.function_zxy_query"
+      "description": "a function source with MixedCase name"
+    },
+    "function_zxy_query": {
+      "content_type": "application/x-protobuf"
     },
     "function_zxy_query_test": {
       "content_type": "application/x-protobuf",
@@ -40,8 +43,7 @@
       "description": "public.points3857.geom"
     },
     "table_source": {
-      "content_type": "application/x-protobuf",
-      "description": "public.table_source.geom"
+      "content_type": "application/x-protobuf"
     }
   },
   "sprites": {

diff --git a/tests/expected/configured/fnc_comment_cfg.json b/tests/expected/configured/fnc_comment_cfg.json
@@ -0,0 +1,17 @@
+{
+  "tilejson": "3.0.0",
+  "tiles": [
+    "http://localhost:3111/fnc_Mixed_Name/{z}/{x}/{y}"
+  ],
+  "vector_layers": [
+    {
+      "id": "MixedCase.function_Mixed_Name",
+      "fields": {
+        "Geom": "",
+        "TABLE": ""
+      }
+    }
+  ],
+  "description": "a function source with MixedCase name",
+  "name": "fnc_Mixed_Name"
+}
diff --git a/tests/expected/configured/save_config.yaml b/tests/expected/configured/save_config.yaml
@@ -137,6 +137,9 @@ postgres:
       properties:
         gid: int4
   functions:
+    fnc_Mixed_Name:
+      schema: MixedCase
+      function: function_Mixed_Name
     function_zxy_query:
       schema: public
       function: function_zxy_query

diff --git a/tests/expected/configured/tbl_comment_cfg.json b/tests/expected/configured/tbl_comment_cfg.json
@@ -0,0 +1,23 @@
+{
+  "tilejson": "3.0.0",
+  "tiles": [
+    "http://localhost:3111/MixPoints/{z}/{x}/{y}"
+  ],
+  "vector_layers": [
+    {
+      "id": "MixPoints",
+      "fields": {
+        "Gid": "int4",
+        "TABLE": "text"
+      }
+    }
+  ],
+  "bounds": [
+    -170.94984639004662,
+    -84.20025580733805,
+    167.70892858284475,
+    74.23573284753762
+  ],
+  "description": "a description from comment on table",
+  "name": "MixPoints"
+}
diff --git a/tests/fixtures/functions/function_Mixed_Name.sql b/tests/fixtures/functions/function_Mixed_Name.sql
@@ -15,7 +15,7 @@ RETURNS TABLE("mVt" bytea, key text) AS $$
 $$ LANGUAGE SQL IMMUTABLE STRICT PARALLEL SAFE;
 
 DO $do$ BEGIN
-    EXECUTE 'COMMENT ON FUNCTION "MixedCase"."function_Mixed_Name" (INT4, INT4, INT4) IS $tj$' || $$
+    EXECUTE 'COMMENT ON FUNCTION "MixedCase"."function_Mixed_Name" IS $tj$' || $$
     {
         "description": "a function source with MixedCase name",
         "vector_layers": [

diff --git a/tests/fixtures/mbtiles/world_cities.mbties b/tests/fixtures/mbtiles/world_cities.mbties
diff --git a/tests/test.sh b/tests/test.sh
@@ -286,6 +286,10 @@ test_pbf mb_mvt_2_3_1 world_cities/2/3/1
 >&2 echo "***** Test server response for table source with empty SRID *****"
 test_pbf points_empty_srid_0_0_0  points_empty_srid/0/0/0
 
+>&2 echo "***** Test server response for comments *****"
+test_jsn tbl_comment              MixPoints
+test_jsn fnc_comment              function_Mixed_Name
+
 kill_process $MARTIN_PROC_ID Martin
 
 test_log_has_str "$LOG_FILE" 'WARN  martin::pg::table_source] Table public.table_source has no spatial index on column geom'
@@ -366,6 +370,10 @@ test_font font_1      font/Overpass%20Mono%20Light/0-255
 test_font font_2      font/Overpass%20Mono%20Regular/0-255
 test_font font_3      font/Overpass%20Mono%20Regular,Overpass%20Mono%20Light/0-255
 
+# Test comments override
+test_jsn tbl_comment_cfg  MixPoints
+test_jsn fnc_comment_cfg  fnc_Mixed_Name
+
 kill_process $MARTIN_PROC_ID Martin
 test_log_has_str "$LOG_FILE" 'WARN  martin::pg::table_source] Table public.table_source has no spatial index on column geom'
 test_log_has_str "$LOG_FILE" 'WARN  martin::fonts] Ignoring duplicate font Overpass Mono Regular from tests'