Switch to the DuckDB Expression API to mitigate sql injection risk (#777)

## What I am changing
<!-- What were the high-level goals of the change? -->
Using string concatenation in
[_geoarrow/_duckdb.py](https://github.com/developmentseed/lonboard/blob/main/lonboard/_geoarrow/_duckdb.py)
triggered the warning [hardcoded-sql-expression
(S608)](https://docs.astral.sh/ruff/rules/hardcoded-sql-expression/),
and rightfully so, e.g. surfacing in the earlier issue
#768 . Luckily, the
[DuckDB Expression
API](https://duckdb.org/docs/stable/clients/python/expression.html)
allows us to rewrite these.

A nice side effect of this is that the Expression API does not need the
connection object to process a DuckDBPyRelation.

## How I did it
<!-- How did you go about achieving these goals? Any considerations made
along the way? -->
The core is to
[rewrite](6d41d02)
`SELECT ST_AsWKB( {geom_col_name} ) as {geom_col_name} FROM rel;` into:
```python
rel.select(
            FunctionExpression("st_aswkb", ColumnExpression(geom_col_name)).alias(
                geom_col_name,
            )
```
, the rest just cascaded from there.

## How you can test it
<!-- How might a reviewer test your changes to verify that they work as
expected? -->
- I edited the related tests too.

## Related Issues
<!-- Reference any issues that inspired this PR. Use a keyword to
auto-close any issues when this PR is merged (eg "closes #1") -->
- n/a

Author: danielsparing

lonboard/_geoarrow/_duckdb.py

@@ -1,6 +1,3 @@
-# ruff: noqa: S608
-# Possible SQL injection vector through string-based query construction
-
 from __future__ import annotations
 
 import json
@@ -17,6 +14,7 @@
     list_array,
     struct_field,
 )
+from duckdb import ColumnExpression, FunctionExpression
 
 from lonboard._constants import EXTENSION_NAME
 
@@ -37,7 +35,6 @@
 def from_duckdb(
     rel: duckdb.DuckDBPyRelation,
     *,
-    con: duckdb.DuckDBPyConnection | None = None,
     crs: str | pyproj.CRS | None = None,
 ) -> Table:
     geom_col_idxs = [
@@ -67,7 +64,7 @@ def from_duckdb(
             crs=crs,
         )
     if geom_type == "GEOMETRY":
-        return _from_geometry(rel, con=con, geom_col_idx=geom_col_idx, crs=crs)
+        return _from_geometry(rel, geom_col_idx=geom_col_idx, crs=crs)
     if geom_type == "POINT_2D":
         return _from_geoarrow(
             rel,
@@ -101,7 +98,6 @@ def from_duckdb(
 def _from_geometry(
     rel: duckdb.DuckDBPyRelation,
     *,
-    con: duckdb.DuckDBPyConnection | None = None,
     geom_col_idx: int,
     crs: str | pyproj.CRS | None = None,
 ) -> Table:
@@ -120,43 +116,13 @@ def _from_geometry(
         geom_col_name,
     ), f"Expected geometry column name to match regex: {re_match}"
 
-    if con is not None:
-        geom_table = Table.from_arrow(
-            con.sql(f"""
-        SELECT ST_AsWKB( {geom_col_name} ) as {geom_col_name} FROM rel;
-        """).arrow(),
-        )
-    else:
-        import duckdb
-
-        # We need to re-import the spatial extension because this is a different context
-        # as the user's context.
-        # It would be nice to re-use the user's context, but in the case of `viz` where
-        # we want to visualize a single input object, we want to accept a
-        # DuckDBPyRelation as input.
-        sql = f"""
-            INSTALL spatial;
-            LOAD spatial;
-            SELECT ST_AsWKB( {geom_col_name} ) as {geom_col_name} FROM rel;
-            """
-        try:
-            # duckdb.default_connection changed from attribute to callable in duckdb 1.2
-            default_con = (
-                duckdb.default_connection()
-                if callable(duckdb.default_connection)
-                else duckdb.default_connection
-            )
-            geom_table = Table.from_arrow(
-                duckdb.execute(sql, connection=default_con).arrow(),
-            )
-        except duckdb.CatalogException as err:
-            msg = (
-                "Could not coerce type GEOMETRY to WKB.\n"
-                "This often happens from using a custom DuckDB connection object.\n"
-                "Either pass in a `con` object containing the DuckDB connection or "
-                "cast to WKB manually with `ST_AsWKB`."
-            )
-            raise ValueError(msg) from err
+    geom_table = Table.from_arrow(
+        rel.select(
+            FunctionExpression("st_aswkb", ColumnExpression(geom_col_name)).alias(
+                geom_col_name,
+            ),
+        ).arrow(),
+    )
 
     metadata = _make_geoarrow_field_metadata(EXTENSION_NAME.WKB, crs)
     geom_field = geom_table.schema.field(0).with_metadata(metadata)

lonboard/_layer.py

@@ -420,8 +420,7 @@ def from_duckdb(
             sql: The SQL input to visualize. This can either be a string containing a
                 SQL query or the output of the duckdb `sql` function.
             con: The current DuckDB connection. This is required when passing a `str` to
-                the `sql` parameter or when using a non-global DuckDB connection.
-                Defaults to None.
+                the `sql` parameter.
 
         Keyword Args:
             crs: The CRS of the input data. This can either be a string passed to
@@ -436,9 +435,9 @@ def from_duckdb(
             assert con is not None, "con must be provided when sql is a str"
 
             rel = con.sql(sql)
-            table = _from_duckdb(rel, con=con, crs=crs)
+            table = _from_duckdb(rel, crs=crs)
         else:
-            table = _from_duckdb(sql, con=con, crs=crs)
+            table = _from_duckdb(sql, crs=crs)
 
         return cls(table=table, **kwargs)
 

lonboard/_viz.py

@@ -5,6 +5,7 @@
 from __future__ import annotations
 
 import json
+import warnings
 from textwrap import dedent
 from typing import (
     TYPE_CHECKING,
@@ -126,18 +127,6 @@ def viz(
             viz(query)
             ```
 
-            If you're using a custom connection, ensure you pass in the `con` parameter:
-
-            ```py
-            import duckdb
-            from lonboard import viz
-
-            con = duckdb.connect()
-            sql = "SELECT * FROM spatial_table;"
-            query = con.sql(sql)
-            viz(query, con=con)
-            ```
-
             You can also render an entire table by using the `table()` method:
 
             ```py
@@ -146,7 +135,7 @@ def viz(
 
             con = duckdb.connect()
             con.execute("CREATE TABLE spatial_table AS ...;")
-            viz(con.table(), con=con)
+            viz(con.table())
             ```
 
         !!! warning
@@ -179,9 +168,8 @@ def viz(
             [`PolygonLayer`][lonboard.PolygonLayer]s.
         map_kwargs: a `dict` of parameters to pass down to the generated
             [`Map`][lonboard.Map].
-        con: the active DuckDB connection. This is necessary in some cases when passing
-            in a DuckDB query. In particular, if you're using a non-global DuckDB
-            connection and if your SQL query outputs the default `GEOMETRY` type.
+        con: Deprecated: the active DuckDB connection. This argument has no effect and
+            might be removed in the future.
 
     For more control over rendering, construct [`Map`][lonboard.Map] and `Layer` objects
     directly.
@@ -192,6 +180,14 @@ def viz(
     """
     global COLOR_COUNTER  # noqa: PLW0603 Using the global statement to update `COLOR_COUNTER` is discouraged
 
+    if con is not None:
+        warnings.warn(
+            "The 'con' argument is deprecated and may be removed in a future version. "
+            "It has no effect.",
+            category=DeprecationWarning,
+            stacklevel=2,
+        )
+
     if isinstance(data, (list, tuple)):
         layers: list[ScatterplotLayer | PathLayer | PolygonLayer] = []
         for i, item in enumerate(data):
@@ -201,7 +197,6 @@ def viz(
                 scatterplot_kwargs=scatterplot_kwargs,
                 path_kwargs=path_kwargs,
                 polygon_kwargs=polygon_kwargs,
-                con=con,
             )
             layers.extend(ls)
 
@@ -213,7 +208,6 @@ def viz(
             scatterplot_kwargs=scatterplot_kwargs,
             path_kwargs=path_kwargs,
             polygon_kwargs=polygon_kwargs,
-            con=con,
         )
         COLOR_COUNTER += 1
 
@@ -228,27 +222,13 @@ def viz(
 DUCKDB_PY_CONN_ERROR = dedent("""\
     Must pass in DuckDBPyRelation object, not DuckDBPyConnection.
 
-    Instead of using `duckdb.execute()` or `con.execute()`, use `duckdb.sql()` or
-    `con.sql()`.
-
-    If using `con.sql()`, ensure you pass the `con` into the `viz()` function:
-
-    ```
-    viz(con.sql("SELECT * FROM table;", con=con))
-    ```
-
-    Alternatively, you can call the `table()` method of `con`:
-
-    ```
-    viz(con.table("table_name", con=con))
-    ```
+    Instead of using `duckdb.execute()` or `con.execute()`, use `duckdb.sql()`,
+    `con.sql()` or `con.table()`.
     """)
 
 
 def create_layers_from_data_input(
     data: VizDataInput,
-    *,
-    con: duckdb.DuckDBPyConnection | None = None,
     **kwargs: Any,
 ) -> list[ScatterplotLayer | PathLayer | PolygonLayer]:
     """Create one or more renderable layers from data input.
@@ -274,7 +254,7 @@ def create_layers_from_data_input(
         data.__class__.__module__.startswith("duckdb")
         and data.__class__.__name__ == "DuckDBPyRelation"
     ):
-        return _viz_duckdb_relation(data, con=con, **kwargs)  # type: ignore
+        return _viz_duckdb_relation(data, **kwargs)  # type: ignore
 
     if (
         data.__class__.__module__.startswith("duckdb")
@@ -354,12 +334,11 @@ def _viz_geopandas_geoseries(
 
 def _viz_duckdb_relation(
     data: duckdb.DuckDBPyRelation,
-    con: duckdb.DuckDBPyConnection | None = None,
     **kwargs: Any,
 ) -> list[ScatterplotLayer | PathLayer | PolygonLayer]:
     from lonboard._geoarrow._duckdb import from_duckdb
 
-    table = from_duckdb(data, con=con)
+    table = from_duckdb(data)
     return _viz_geoarrow_table(table, **kwargs)
 
 

tests/test_duckdb.py

@@ -18,6 +18,22 @@
 cities_gdal_path = f"/vsizip/{cities_path}"
 
 
+def test_viz_geometry_default_con():
+    # For WKB parsing
+    pytest.importorskip("shapely")
+
+    sql = f"""
+        INSTALL spatial;
+        LOAD spatial;
+        SELECT * FROM ST_Read("{cities_gdal_path}");
+        """
+    rel = duckdb.sql(sql)
+    assert rel.types[-1] == "GEOMETRY"
+
+    m = viz(rel)
+    assert isinstance(m.layers[0], ScatterplotLayer)
+
+
 def test_viz_geometry():
     # For WKB parsing
     pytest.importorskip("shapely")
@@ -30,7 +46,12 @@ def test_viz_geometry():
         """
     rel = con.sql(sql)
     assert rel.types[-1] == "GEOMETRY"
-    m = viz(rel, con=con)
+    m = viz(rel)
+    assert isinstance(m.layers[0], ScatterplotLayer)
+
+    # Tolerates legacy con parameter
+    with pytest.warns(DeprecationWarning, match="The 'con' argument is deprecated"):
+        m = viz(rel, con=con)
     assert isinstance(m.layers[0], ScatterplotLayer)
 
 
@@ -198,14 +219,12 @@ def test_create_table_as_custom_con():
     con = duckdb.connect()
     con.execute(sql)
 
-    with pytest.raises(
-        duckdb.InvalidInputException,
-        match="object was created by another Connection",
-    ):
-        m = viz(con.table("test"))
+    m = viz(con.table("test"))
+    assert isinstance(m.layers[0], ScatterplotLayer)
 
-    # Succeeds when passing in con object
-    m = viz(con.table("test"), con=con)
+    # Tolerates legacy con parameter
+    with pytest.warns(DeprecationWarning, match="The 'con' argument is deprecated"):
+        m = viz(con.table("test"), con=con)
     assert isinstance(m.layers[0], ScatterplotLayer)
 
 
@@ -221,7 +240,12 @@ def test_geometry_only_column():
 
     _layer = ScatterplotLayer.from_duckdb(con.table("data"), con)
 
-    m = viz(con.table("data"), con=con)
+    m = viz(con.table("data"))
+    assert isinstance(m.layers[0], ScatterplotLayer)
+
+    # Tolerates legacy con parameter
+    with pytest.warns(DeprecationWarning, match="The 'con' argument is deprecated"):
+        m = viz(con.table("data"), con=con)
     assert isinstance(m.layers[0], ScatterplotLayer)
 
 
@@ -261,4 +285,4 @@ def test_sanitize_column_name():
         AssertionError,
         match="Expected geometry column name to match regex:",
     ):
-        viz(rel, con=con)
+        viz(rel)