Repository: geofileops/geofileops
Pull request #650 (2025-03-07 19:44:00)

ENH: in add_column, don't add column if update expression is invalid
If adding the column succeeds but the update fails, the output files is in a state that is not ideal: when the `add_column` is ran again, the column won't be "fixed". It would be better that this is transactional: if the update fails, the add should be rolled back well.

This PR adds using transactions for this case.

Mind: transactions are not supported for all file types! E.g. 
- for .gpkg this will work fine.
- for .shp the column will still stay added as transactions are not supported.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/geofileops/geofileops/pull/650?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) Report
Attention: Patch coverage is `93.33333%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 85.71%. Comparing base [(`5f08842`)](https://app.codecov.io/gh/geofileops/geofileops/commit/5f08842e475a5aaff2f4ace75e7bc77c9c2fe023?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) to head [(`f3242b6`)](https://app.codecov.io/gh/geofileops/geofileops/commit/f3242b6562d36b7dc3a540642e922fbbe8a9283b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).
> Report is 119 commits behind head on main.

| [Files with missing lines](https://app.codecov.io/gh/geofileops/geofileops/pull/650?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) | Patch % | Lines |
|---|---|---|
| [geofileops/util/\_ogr\_util.py](https://app.codecov.io/gh/geofileops/geofileops/pull/650?src=pr&el=tree&filepath=geofileops%2Futil%2F_ogr_util.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops#diff-Z2VvZmlsZW9wcy91dGlsL19vZ3JfdXRpbC5weQ==) | 91.66% | [2 Missing :warning: ](https://app.codecov.io/gh/geofileops/geofileops/pull/650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops) |

> :exclamation:  There is a different number of reports uploaded between BASE (5f08842) and HEAD (f3242b6). Click for more details.
> 
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (5f08842) | HEAD (f3242b6) |
>|------|------|------|
>||6|5|
></details>

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #650      +/-   ##
==========================================
- Coverage   93.11%   85.71%   -7.41%     
==========================================
  Files          21       22       +1     
  Lines        4113     4556     +443     
==========================================
+ Hits         3830     3905      +75     
- Misses        283      651     +368     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/geofileops/geofileops/pull/650?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=geofileops).

<details><summary>🚀 New features to boost your workflow: </summary>

- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0821ce2..b6c9625 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -29,6 +29,7 @@
 - Enable "CURVE" geometrytype files to be processed in the general file and
   layer operations (#558)
 - Don't convert to multi-part geometries by default in `copy_layer`,... (#570)
+- In `add_column`, don't add column if update expression is invalid (#650)
 - Add configuration option to only warn on dissolve errors (#561)
 - Add some pre-flight checks when geofileops is imported (#573, #627)
 - For `select_two_layers`, add the `gpkg_ogr_contents` table + fill out extents in the
diff --git a/geofileops/fileops.py b/geofileops/fileops.py
index 29738ee..bb2a62a 100644
--- a/geofileops/fileops.py
+++ b/geofileops/fileops.py
@@ -1028,8 +1028,8 @@ def add_column(
                 f'ALTER TABLE "{layer}" ADD COLUMN "{name}" {type_str}{width_str}'
             )
             datasource = gdal.OpenEx(str(path), nOpenFlags=gdal.OF_UPDATE)
-            result = datasource.ExecuteSQL(sql_stmt)
-            datasource.ReleaseResultSet(result)
+            _ogr_util.StartTransaction(datasource)
+            datasource.ExecuteSQL(sql_stmt)
         else:
             logger.warning(f"Column {name} existed already in {path}, layer {layer}")
 
@@ -1039,11 +1039,14 @@ def add_column(
         ):
             if datasource is None:
                 datasource = gdal.OpenEx(str(path), nOpenFlags=gdal.OF_UPDATE)
+                _ogr_util.StartTransaction(datasource)
             sql_stmt = f'UPDATE "{layer}" SET "{name}" = {expression}'
-            result = datasource.ExecuteSQL(sql_stmt, dialect=expression_dialect)
-            datasource.ReleaseResultSet(result)
+            datasource.ExecuteSQL(sql_stmt, dialect=expression_dialect)
+
+        _ogr_util.CommitTransaction(datasource)
 
     except Exception as ex:
+        _ogr_util.RollbackTransaction(datasource)
         ex.args = (f"add_column error for {path}#{layer}:\n  {ex}",)
         raise
     finally:
diff --git a/geofileops/util/_ogr_util.py b/geofileops/util/_ogr_util.py
index 5c9b297..b1615fc 100644
--- a/geofileops/util/_ogr_util.py
+++ b/geofileops/util/_ogr_util.py
@@ -166,6 +166,65 @@ def read_cpl_log(path: Path) -> tuple[list[str], list[str]]:
     return (lines_cleaned, lines_error)
 
 
+def StartTransaction(datasource: gdal.Dataset) -> bool:
+    """Starts a transaction on an open datasource.
+
+    Args:
+        datasource (gdal.Dataset): the datasource to start the transaction on.
+
+    Raises:
+        ValueError: if datasource is None.
+
+    Returns:
+        bool: True if the transaction was started successfully.
+    """
+    if datasource is None:
+        raise ValueError("datasource is None")
+
+    if datasource.TestCapability(ogr.ODsCTransactions):
+        datasource.StartTransaction()
+
+    return True
+
+
+def CommitTransaction(datasource: Optional[gdal.Dataset]) -> bool:
+    """Commits a transaction on an open datasource.
+
+    Args:
+        datasource (gdal.Dataset): the datasource to commit the transaction on. If None,
+            no commit is executed.
+
+    Returns:
+        bool: True if the transaction was committed successfully.
+    """
+    if datasource is None:
+        return False
+
+    if datasource.TestCapability(ogr.ODsCTransactions):
+        datasource.CommitTransaction()
+
+    return True
+
+
+def RollbackTransaction(datasource: Optional[gdal.Dataset]) -> bool:
+    """Rolls back a transaction on an open datasource.
+
+    Args:
+        datasource (gdal.Dataset): the datasource to roll back the transaction on. If
+            None, no rollback is executed.
+
+    Returns:
+        bool: True if the transaction was rolled back successfully.
+    """
+    if datasource is None:
+        return False
+
+    if datasource.TestCapability(ogr.ODsCTransactions):
+        datasource.RollbackTransaction()
+
+    return True
+
+
 class VectorTranslateInfo:
     def __init__(
         self,


Test patch:
diff --git a/tests/test_geofile.py b/tests/test_geofile.py
index 64cd2ab..98096f5 100644
--- a/tests/test_geofile.py
+++ b/tests/test_geofile.py
@@ -123,6 +123,32 @@ def test_add_column_gpkg(tmp_path):
     assert gdf["HFDTLT"][0] == "5"
 
 
+@pytest.mark.parametrize(
+    "suffix, transaction_supported", [(".gpkg", True), (".shp", False)]
+)
+def test_add_column_update_error(tmp_path, suffix, transaction_supported):
+    """Test on the result of an invalid update expression.
+
+    If the file format supports transactions, the column should not be added.
+    """
+    test_path = test_helper.get_testfile(
+        "polygon-parcel", dst_dir=tmp_path, suffix=suffix
+    )
+
+    # Add a column with an invalid expression
+    with pytest.raises(RuntimeError, match="add_column error for"):
+        gfo.add_column(
+            test_path, name="ERROR_COL", type="TEXT", expression="invalid_expression"
+        )
+
+    # For formats that support transactions, the column should not be there
+    info = gfo.get_layerinfo(test_path)
+    if transaction_supported:
+        assert "ERROR_COL" not in list(info.columns)
+    else:
+        assert "ERROR_COL" in list(info.columns)
+
+
 def test_append_different_layer(tmp_path):
     # Prepare test data
     src_path = test_helper.get_testfile("polygon-parcel", dst_dir=tmp_path)
diff --git a/tests/test_ogr_util.py b/tests/test_ogr_util.py
index 1a28ce8..526be69 100644
--- a/tests/test_ogr_util.py
+++ b/tests/test_ogr_util.py
@@ -149,6 +149,19 @@ def test_set_config_options():
     assert gdal.GetConfigOption(test3_config_envset) == "test3_new_env_value"
 
 
+def test_StartTransaction_None():
+    with pytest.raises(Exception, match="datasource is None"):
+        _ogr_util.StartTransaction(None)
+
+
+def test_CommitTransaction_None():
+    assert not _ogr_util.CommitTransaction(None)
+
+
+def test_RollbackTransaction_None():
+    assert not _ogr_util.RollbackTransaction(None)
+
+
 @pytest.mark.parametrize(
     "output_geometrytype, exp_geometrytype",
     [