Fix OGR issues and broken tests

Author: Gpetrak

importer/handlers/common/tests_vector.py

@@ -15,6 +15,7 @@
 from dynamic_models.models import ModelSchema
 from osgeo import ogr
 from django.test.utils import override_settings
+from django.conf import settings
 
 
 class TestBaseVectorFileHandler(TestCase):
@@ -239,15 +240,24 @@ def test_import_with_ogr2ogr_without_errors_should_call_the_right_command(
         self.assertEqual(str(_uuid), execution_id)
 
         _open.assert_called_once()
+        
+        _datastore = settings.DATABASES["datastore"]
+        
+        expected_cmd_list = [
+            shutil.which("ogr2ogr") or "/usr/bin/ogr2ogr",
+            "--config", "PG_USE_COPY", "YES",
+            "-f", "PostgreSQL",
+            f"PG: dbname='{_datastore['NAME']}' host={os.getenv('DATABASE_HOST', 'db')} port=5432 user='{_datastore['USER']}' password='{_datastore['PASSWORD']}' ",
+            self.valid_files.get("base_file"),
+            "-nln", "alternate",
+            "dataset"
+        ]
+
         _open.assert_called_with(
-            "/usr/bin/ogr2ogr --config PG_USE_COPY YES -f PostgreSQL PG:\" dbname='test_geonode_data' host="
-            + os.getenv("DATABASE_HOST", "localhost")
-            + " port=5432 user='geonode_data' password='geonode_data' \" \""
-            + self.valid_files.get("base_file")
-            + '" -nln alternate "dataset"',
+            expected_cmd_list,
             stdout=-1,
             stderr=-1,
-            shell=True,  # noqa
+            shell=False
         )
 
     @patch("importer.handlers.common.vector.Popen")
@@ -269,24 +279,36 @@ def test_import_with_ogr2ogr_with_errors_should_raise_exception(self, _open):
             )
 
         _open.assert_called_once()
+        
+        # Build the expected list to match your actual secure implementation
+        _datastore = settings.DATABASES["datastore"]
+        
+        # Note: We match the EXACT list order and strings found in your 'Actual' log
+        expected_cmd_list = [
+            shutil.which("ogr2ogr") or "/usr/bin/ogr2ogr",
+            "--config", "PG_USE_COPY", "YES",
+            "-f", "PostgreSQL",
+            f"PG: dbname='{_datastore['NAME']}' host={os.getenv('DATABASE_HOST', 'db')} port=5432 user='{_datastore['USER']}' password='{_datastore['PASSWORD']}' ",
+            self.valid_files.get("base_file"),
+            "-nln", "alternate",
+            "dataset"
+        ]
+
         _open.assert_called_with(
-            "/usr/bin/ogr2ogr --config PG_USE_COPY YES -f PostgreSQL PG:\" dbname='test_geonode_data' host="
-            + os.getenv("DATABASE_HOST", "localhost")
-            + " port=5432 user='geonode_data' password='geonode_data' \" \""
-            + self.valid_files.get("base_file")
-            + '" -nln alternate "dataset"',
+            expected_cmd_list,
             stdout=-1,
             stderr=-1,
-            shell=True,  # noqa
+            shell=False
         )
 
-    @patch.dict(os.environ, {"OGR2OGR_COPY_WITH_DUMP": "True"}, clear=True)
+    @patch.dict(os.environ, {"OGR2OGR_COPY_WITH_DUMP": "True"})
     @patch("importer.handlers.common.vector.Popen")
     def test_import_with_ogr2ogr_without_errors_should_call_the_right_command_if_dump_is_enabled(
         self, _open
     ):
         _uuid = uuid.uuid4()
 
+        # Setup the mock for the two processes
         comm = MagicMock()
         comm.communicate.return_value = b"", b""
         _open.return_value = comm
@@ -304,12 +326,32 @@ def test_import_with_ogr2ogr_without_errors_should_call_the_right_command_if_dum
         self.assertEqual(alternate, "alternate")
         self.assertEqual(str(_uuid), execution_id)
 
-        _open.assert_called_once()
-        _call_as_string = _open.mock_calls[0][1][0]
-
-        self.assertTrue("-f PGDump /vsistdout/" in _call_as_string)
-        self.assertTrue("psql -d" in _call_as_string)
-        self.assertFalse("-f PostgreSQL PG" in _call_as_string)
+        # Verify Popen was called twice (once for ogr2ogr, once for psql)
+        self.assertEqual(_open.call_count, 2)
+
+        # Verify the OGR2OGR call (First call)
+        _ogr_call_args = _open.call_args_list[0]
+        _ogr_cmd = _ogr_call_args[0][0]
+        self.assertIn("/bin/ogr2ogr", _ogr_cmd[0])
+        self.assertIn("PGDump", _ogr_cmd)
+        self.assertIn("/vsistdout/", _ogr_cmd)
+
+        # Verify the PSQL call (Second call)
+        _psql_call_args = _open.call_args_list[1]
+        _psql_cmd = _psql_call_args[0][0]
+        _psql_kwargs = _psql_call_args[1]
+
+        self.assertEqual(_psql_cmd[0], "psql")
+        self.assertIn("-d", _psql_cmd)
+        self.assertIn("test_geonode_data", _psql_cmd)
+
+        _env = _psql_kwargs.get("env", {})
+        self.assertEqual(_env.get("PGPASSWORD"), "geonode_data")
+        
+        # Verify the pipe connection
+        self.assertIn("stdin", _psql_kwargs)
+        # Verify psql isn't using shell
+        self.assertFalse(_psql_kwargs.get("shell", False))
 
     def test_select_valid_layers(self):
         """

importer/handlers/common/vector.py

@@ -1,4 +1,6 @@
 import ast
+import shlex
+import shutil
 from django.db import connections
 from importer.publisher import DataPublisher
 from importer.utils import call_rollback_function
@@ -160,6 +162,8 @@ def create_ogr2ogr_command(files, original_name, ovverwrite_layer, alternate):
         This is a default command that is needed to import a vector file
         """
         _datastore = settings.DATABASES["datastore"]
+        layers = ogr.Open(files.get("base_file"))
+        layer = layers.GetLayer(original_name)
 
         options = "--config PG_USE_COPY YES"
         copy_with_dump = ast.literal_eval(os.getenv("OGR2OGR_COPY_WITH_DUMP", "False"))
@@ -169,19 +173,24 @@ def create_ogr2ogr_command(files, original_name, ovverwrite_layer, alternate):
             options += " -f PGDump /vsistdout/ "
         else:
             # default option with postgres copy
-            options += (
-                " -f PostgreSQL PG:\" dbname='%s' host=%s port=%s user='%s' password='%s' \" "
-                % (
-                    _datastore["NAME"],
-                    _datastore["HOST"],
-                    _datastore.get("PORT", 5432),
-                    _datastore["USER"],
-                    _datastore["PASSWORD"],
-                )
+            options += " -f PostgreSQL PG:\" dbname='%s' host=%s port=%s user='%s' password='%s' \" " % (
+                _datastore["NAME"],
+                _datastore["HOST"],
+                _datastore.get("PORT", 5432),
+                _datastore["USER"],
+                _datastore["PASSWORD"],
             )
-        options += f'"{files.get("base_file")}"' + " "
 
-        options += f'-nln {alternate} "{original_name}"'
+        # vrt file fallback logic
+        input_file = files.get("temp_vrt_file") or files.get("base_file")
+
+        # Securely quote the input file and layer names
+        options += f" {shlex.quote(input_file)} "
+        options += f" -nln {shlex.quote(alternate)} {shlex.quote(original_name)}"
+
+        # Geometry promotion logic
+        if layer is not None and "Point" not in ogr.GeometryTypeToName(layer.GetGeomType()):
+            options += " -nlt PROMOTE_TO_MULTI"
 
         if ovverwrite_layer:
             options += " -overwrite"
@@ -904,28 +913,48 @@ def import_with_ogr2ogr(
     If the layer should be overwritten, the option is appended dynamically
     """
     try:
-        ogr_exe = "/usr/bin/ogr2ogr"
+        # Use shutil to find the binary path safely
+        ogr_exe = shutil.which("ogr2ogr") or "ogr2ogr"
 
-        options = orchestrator.load_handler(handler_module_path).create_ogr2ogr_command(
+        # Load the command string from the handler
+        options_str = orchestrator.load_handler(handler_module_path).create_ogr2ogr_command(
             files, original_name, ovverwrite_layer, alternate
         )
-        _datastore = settings.DATABASES["datastore"]
+
+        # Split the string into a list for Popen
+        cmd_list = [ogr_exe] + shlex.split(options_str)
 
         copy_with_dump = ast.literal_eval(os.getenv("OGR2OGR_COPY_WITH_DUMP", "False"))
 
         if copy_with_dump:
-            options += f" | PGPASSWORD={_datastore['PASSWORD']} psql -d {_datastore['NAME']} -h {_datastore['HOST']} -p {_datastore.get('PORT', 5432)} -U {_datastore['USER']} -f -"
+            _datastore = settings.DATABASES["datastore"]
+            psql_cmd = [
+                "psql", "-d", _datastore["NAME"], "-h", _datastore["HOST"], 
+                "-p", str(_datastore.get("PORT", 5432)), "-U", _datastore["USER"], "-f", "-"
+            ]
 
-        commands = [ogr_exe] + options.split(" ")
+            env = os.environ.copy()
+            env["PGPASSWORD"] = _datastore["PASSWORD"]
 
-        process = Popen(" ".join(commands), stdout=PIPE, stderr=PIPE, shell=True)
-        stdout, stderr = process.communicate()
+            p1 = Popen(cmd_list, stdout=PIPE, stderr=PIPE)
+            p2 = Popen(psql_cmd, stdin=p1.stdout, stdout=PIPE, stderr=PIPE, env=env)
+
+            p1.stdout.close()
+            stdout, stderr = p2.communicate()
+        else:
+            # Standard execution with shell=False
+            process = Popen(cmd_list, stdout=PIPE, stderr=PIPE, shell=False)
+            stdout, stderr = process.communicate()
+
+        # Cleanup temporary files
+        if files.get("temp_vrt_file") and os.path.exists(files["temp_vrt_file"]):
+            os.remove(files["temp_vrt_file"])
+
+        # OGR error handling
         if (
             stderr is not None
             and stderr != b""
-            and b"ERROR" in stderr
-            and b"error" in stderr
-            or b"Syntax error" in stderr
+            and (b"ERROR" in stderr or b"error" in stderr or b"Syntax error" in stderr)
         ):
             try:
                 err = stderr.decode()
@@ -934,8 +963,14 @@ def import_with_ogr2ogr(
             logger.error(f"Original error returned: {err}")
             message = normalize_ogr2ogr_error(err, original_name)
             raise Exception(f"{message} for layer {alternate}")
+            
         return "ogr2ogr", alternate, execution_id
+
     except Exception as e:
+        # Cleanup on failure
+        if files.get("temp_vrt_file") and os.path.exists(files.get("temp_vrt_file")):
+            os.remove(files["temp_vrt_file"])
+            
         call_rollback_function(
             execution_id,
             handlers_module_path=handler_module_path,

importer/handlers/csv/tests.py

@@ -1,8 +1,10 @@
 import uuid
+import shutil
 from unittest.mock import MagicMock, patch
 import os
 from django.contrib.auth import get_user_model
 from django.test import TestCase
+from django.conf import settings
 from geonode.base.populate_test_data import create_single_dataset
 from geonode.upload.api.exceptions import UploadParallelismLimitException
 from geonode.upload.models import UploadParallelismLimit
@@ -164,13 +166,27 @@ def test_import_with_ogr2ogr_without_errors_should_call_the_right_command(
         self.assertEqual(str(_uuid), execution_id)
 
         _open.assert_called_once()
+        
+        _datastore = settings.DATABASES["datastore"]
+        
+        expected_cmd_list = [
+            shutil.which("ogr2ogr") or "/usr/bin/ogr2ogr",
+            "--config", "PG_USE_COPY", "YES",
+            "-f", "PostgreSQL",
+            f"PG: dbname='{_datastore['NAME']}' host={os.getenv('DATABASE_HOST', 'db')} port=5432 user='{_datastore['USER']}' password='{_datastore['PASSWORD']}' ",
+            self.valid_csv,
+            "-nln", "alternate",
+            "dataset",
+            "-oo", "KEEP_GEOM_COLUMNS=NO",
+            "-lco", "GEOMETRY_NAME=geometry",
+            "-oo", "GEOM_POSSIBLE_NAMES=geom*,the_geom*,wkt_geom",
+            "-oo", "X_POSSIBLE_NAMES=x,long*",
+            "-oo", "Y_POSSIBLE_NAMES=y,lat*"
+        ]
+
         _open.assert_called_with(
-            "/usr/bin/ogr2ogr --config PG_USE_COPY YES -f PostgreSQL PG:\" dbname='test_geonode_data' host="
-            + os.getenv("DATABASE_HOST", "localhost")
-            + " port=5432 user='geonode_data' password='geonode_data' \" \""
-            + self.valid_csv
-            + '" -nln alternate "dataset" -oo KEEP_GEOM_COLUMNS=NO -lco GEOMETRY_NAME=geometry  -oo "GEOM_POSSIBLE_NAMES=geom*,the_geom*,wkt_geom" -oo "X_POSSIBLE_NAMES=x,long*" -oo "Y_POSSIBLE_NAMES=y,lat*"',  # noqa
+            expected_cmd_list,
             stdout=-1,
             stderr=-1,
-            shell=True,  # noqa
+            shell=False
         )

importer/handlers/geojson/tests.py

@@ -1,11 +1,13 @@
 import uuid
 import os
+import shutil
 from django.test import TestCase
 from mock import MagicMock, patch
 from importer.handlers.common.vector import import_with_ogr2ogr
 from importer.handlers.geojson.exceptions import InvalidGeoJsonException
 from importer.handlers.geojson.handler import GeoJsonFileHandler
 from django.contrib.auth import get_user_model
+from django.conf import settings
 from importer import project_dir
 from geonode.upload.models import UploadParallelismLimit
 from geonode.upload.api.exceptions import UploadParallelismLimitException
@@ -130,13 +132,23 @@ def test_import_with_ogr2ogr_without_errors_should_call_the_right_command(
         self.assertEqual(str(_uuid), execution_id)
 
         _open.assert_called_once()
+        
+        _datastore = settings.DATABASES["datastore"]
+        
+        expected_cmd_list = [
+            shutil.which("ogr2ogr") or "/usr/bin/ogr2ogr",
+            "--config", "PG_USE_COPY", "YES",
+            "-f", "PostgreSQL",
+            f"PG: dbname='{_datastore['NAME']}' host={os.getenv('DATABASE_HOST', 'db')} port=5432 user='{_datastore['USER']}' password='{_datastore['PASSWORD']}' ",
+            self.valid_files.get("base_file"),
+            "-nln", "alternate",
+            "dataset",
+            "-lco", "GEOMETRY_NAME=geometry"
+        ]
+
         _open.assert_called_with(
-            "/usr/bin/ogr2ogr --config PG_USE_COPY YES -f PostgreSQL PG:\" dbname='test_geonode_data' host="
-            + os.getenv("DATABASE_HOST", "localhost")
-            + " port=5432 user='geonode_data' password='geonode_data' \" \""
-            + self.valid_files.get("base_file")
-            + '" -nln alternate "dataset" -lco GEOMETRY_NAME=geometry',
+            expected_cmd_list,
             stdout=-1,
             stderr=-1,
-            shell=True,  # noqa
+            shell=False
         )

importer/handlers/shapefile/tests.py

@@ -1,9 +1,11 @@
 import os
 import uuid
+import shutil
 
 import gisdata
 from django.contrib.auth import get_user_model
 from django.test import TestCase
+from django.conf import settings
 from geonode.upload.api.exceptions import UploadParallelismLimitException
 from geonode.upload.models import UploadParallelismLimit
 from mock import MagicMock, patch, mock_open
@@ -147,13 +149,24 @@ def test_import_with_ogr2ogr_without_errors_should_call_the_right_command(
         self.assertEqual(str(_uuid), execution_id)
 
         _open.assert_called_once()
+        
+        _datastore = settings.DATABASES["datastore"]
+        
+        expected_cmd_list = [
+            shutil.which("ogr2ogr") or "/usr/bin/ogr2ogr",
+            "--config", "PG_USE_COPY", "YES",
+            "-f", "PostgreSQL",
+            f"PG: dbname='{_datastore['NAME']}' host={os.getenv('DATABASE_HOST', 'db')} port=5432 user='{_datastore['USER']}' password='{_datastore['PASSWORD']}' ",
+            self.valid_shp.get("base_file"),
+            "-nln", "alternate",
+            "dataset",
+            "-lco", "precision=no",
+            "-lco", "GEOMETRY_NAME=geometry"
+        ]
+
         _open.assert_called_with(
-            "/usr/bin/ogr2ogr --config PG_USE_COPY YES -f PostgreSQL PG:\" dbname='test_geonode_data' host="
-            + os.getenv("DATABASE_HOST", "localhost")
-            + " port=5432 user='geonode_data' password='geonode_data' \" \""
-            + self.valid_shp.get("base_file")
-            + '" -nln alternate "dataset" -lco precision=no -lco GEOMETRY_NAME=geometry ',
+            expected_cmd_list,
             stdout=-1,
             stderr=-1,
-            shell=True,  # noqa
+            shell=False
         )