diff --git a/spp_api_v2_gis/README.rst b/spp_api_v2_gis/README.rst index 961e531d3..08d131107 100644 --- a/spp_api_v2_gis/README.rst +++ b/spp_api_v2_gis/README.rst @@ -96,6 +96,11 @@ Scopes and Data Privacy | ``gis:geofence`` | Read + Write | Create and archive geofences (also | | | | requires ``gis:read`` for listing) | +------------------+--------------+------------------------------------+ +| ``gis:incident`` | Reserved | No endpoint checks it yet; | +| | | selectable so incident-scoped | +| | | clients can be provisioned ahead | +| | | of the incidents API | ++------------------+--------------+------------------------------------+ **What data is exposed** @@ -156,6 +161,24 @@ Dependencies Changelog ========= +19.0.2.0.1 +~~~~~~~~~~ + +- fix: bind coordinate query parameters in the order the SQL expects +- fix: run the coordinate statistics query inside a savepoint so the + area fallback stays reachable +- fix: run the coordinate proximity query inside a savepoint so the area + fallback stays reachable +- fix: run the area fallback queries inside savepoints so a failed + geometry cannot abort the transaction +- fix: run each batch geometry and the batch summary inside savepoints + so one failed geometry cannot poison the rest of the batch +- fix: propagate statistics failures instead of mislabelling them as + coordinate-query failures and retrying via the area fallback +- fix: add ``geofence`` and ``incident`` scope actions so geofence + endpoints can be granted (``incident`` prepares for the incidents API + re-land) + 19.0.2.0.0 ~~~~~~~~~~ diff --git a/spp_api_v2_gis/__manifest__.py b/spp_api_v2_gis/__manifest__.py index 2aad21224..e8616fc42 100644 --- a/spp_api_v2_gis/__manifest__.py +++ b/spp_api_v2_gis/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP GIS API", "category": "OpenSPP/Integration", - "version": "19.0.2.0.0", + "version": "19.0.2.0.1", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_api_v2_gis/models/api_client_scope.py b/spp_api_v2_gis/models/api_client_scope.py index a9c767de1..40ce1ae51 100644 --- a/spp_api_v2_gis/models/api_client_scope.py +++ b/spp_api_v2_gis/models/api_client_scope.py @@ -16,3 +16,11 @@ class ApiClientScope(models.Model): ], ondelete={"gis": "cascade", "statistics": "cascade"}, ) + + action = fields.Selection( + selection_add=[ + ("geofence", "Geofence Management"), + ("incident", "Incident Management"), + ], + ondelete={"geofence": "cascade", "incident": "cascade"}, + ) diff --git a/spp_api_v2_gis/readme/DESCRIPTION.md b/spp_api_v2_gis/readme/DESCRIPTION.md index b9bfd937f..c1b29ac76 100644 --- a/spp_api_v2_gis/readme/DESCRIPTION.md +++ b/spp_api_v2_gis/readme/DESCRIPTION.md @@ -48,6 +48,7 @@ Follows thin client architecture where QGIS displays data and OpenSPP performs a |-------|--------|-------------| | `gis:read` | Read-only | View collections, layers, statistics, export data | | `gis:geofence` | Read + Write | Create and archive geofences (also requires `gis:read` for listing) | +| `gis:incident` | Reserved | No endpoint checks it yet; selectable so incident-scoped clients can be provisioned ahead of the incidents API | **What data is exposed** diff --git a/spp_api_v2_gis/readme/HISTORY.md b/spp_api_v2_gis/readme/HISTORY.md index 4aaf9afef..bfdcdc497 100644 --- a/spp_api_v2_gis/readme/HISTORY.md +++ b/spp_api_v2_gis/readme/HISTORY.md @@ -1,3 +1,13 @@ +### 19.0.2.0.1 + +- fix: bind coordinate query parameters in the order the SQL expects +- fix: run the coordinate statistics query inside a savepoint so the area fallback stays reachable +- fix: run the coordinate proximity query inside a savepoint so the area fallback stays reachable +- fix: run the area fallback queries inside savepoints so a failed geometry cannot abort the transaction +- fix: run each batch geometry and the batch summary inside savepoints so one failed geometry cannot poison the rest of the batch +- fix: propagate statistics failures instead of mislabelling them as coordinate-query failures and retrying via the area fallback +- fix: add `geofence` and `incident` scope actions so geofence endpoints can be granted (`incident` prepares for the incidents API re-land) + ### 19.0.2.0.0 - Initial migration to OpenSPP2 diff --git a/spp_api_v2_gis/services/spatial_query_service.py b/spp_api_v2_gis/services/spatial_query_service.py index 6c49aba85..29d8d96c3 100644 --- a/spp_api_v2_gis/services/spatial_query_service.py +++ b/spp_api_v2_gis/services/spatial_query_service.py @@ -53,11 +53,17 @@ def query_statistics_batch(self, geometries, filters=None, variables=None): geometry = item["geometry"] try: - result = self.query_statistics( - geometry=geometry, - filters=filters, - variables=variables, - ) + # Each iteration runs inside its own savepoint: the spatial + # legs guard themselves, but a statistics failure would + # otherwise abort the transaction for every later geometry + # and for the summary below. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + result = self.query_statistics( + geometry=geometry, + filters=filters, + variables=variables, + ) # Collect registrant IDs for deduplication in summary registrant_ids = result.pop("registrant_ids", []) all_registrant_ids.update(registrant_ids) @@ -92,7 +98,15 @@ def query_statistics_batch(self, geometries, filters=None, variables=None): # Compute summary by aggregating unique registrants with metadata summary_stats_with_metadata = {"statistics": {}} if all_registrant_ids: - summary_stats_with_metadata = self._compute_statistics(list(all_registrant_ids), variables or []) + try: + # Same guard as the loop: a failure degrades to an empty + # summary instead of discarding the per-geometry results. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + summary_stats_with_metadata = self._compute_statistics(list(all_registrant_ids), variables or []) + except Exception as e: + _logger.warning("Batch summary statistics failed: %s", e) + summary_stats_with_metadata = {"statistics": {}} summary = { "total_count": len(all_registrant_ids), @@ -133,25 +147,38 @@ def query_statistics(self, geometry, filters=None, variables=None): geometry_json = json.dumps(geometry) # Try coordinate-based query first (preferred method) + result = None try: - result = self._query_by_coordinates(geometry_json, filters) - if result["total_count"] > 0: - _logger.info( - "Spatial query using coordinates: %s registrants found", - result["total_count"], - ) - # Compute statistics for the matched registrants with metadata - stats_with_metadata = self._compute_statistics(result["registrant_ids"], variables) - result.update(stats_with_metadata) - return result + # A failed statement aborts the whole transaction, which would make + # the area fallback below fail too. The savepoint contains it. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + result = self._query_by_coordinates(geometry_json, filters) except Exception as e: _logger.warning( "Coordinate-based query failed: %s, falling back to area-based query", e, ) + # Only the coordinate query itself is retried via the fallback; a + # statistics failure is not a spatial failure and must propagate. + if result is not None and result["total_count"] > 0: + _logger.info( + "Spatial query using coordinates: %s registrants found", + result["total_count"], + ) + # Compute statistics for the matched registrants with metadata + stats_with_metadata = self._compute_statistics(result["registrant_ids"], variables) + result.update(stats_with_metadata) + return result + # Fall back to area-based query - result = self._query_by_area(geometry_json, filters) + # A failed statement here would otherwise leave the transaction aborted + # for every subsequent query on this cursor, including later geometries + # in query_statistics_batch. The savepoint contains it. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + result = self._query_by_area(geometry_json, filters) _logger.info( f"Spatial query using area fallback: {result['total_count']} registrants in {result['areas_matched']} areas" ) @@ -176,11 +203,11 @@ def _query_by_coordinates(self, geometry_json, filters): """ # Build WHERE clause from filters where_clauses = ["p.is_registrant = true"] - params = [geometry_json] + filter_params = [] if filters.get("is_group") is not None: where_clauses.append("p.is_group = %s") - params.append(filters["is_group"]) + filter_params.append(filters["is_group"]) if filters.get("disabled") is not None: if filters["disabled"]: @@ -209,8 +236,8 @@ def _query_by_coordinates(self, geometry_json, filters): ) """ # nosec B608 - SQL clauses built from hardcoded fragments, data uses %s params - # Add geometry parameter at the beginning - params = [geometry_json] + params[1:] + # Params ordered to match SQL: filter params (in where_clause) then geometry + params = filter_params + [geometry_json] self.env.cr.execute(query, params) registrant_ids = [row[0] for row in self.env.cr.fetchall()] @@ -484,30 +511,42 @@ def query_proximity(self, reference_points, radius_km, relation="within", filter radius_meters = radius_km * 1000 # Try coordinate-based query first + result = None try: - result = self._proximity_by_coordinates(reference_points, radius_meters, relation, filters) - if result["total_count"] > 0: - _logger.info( - "Proximity query (%s, %.1f km) using coordinates: %s registrants found", - relation, - radius_km, - result["total_count"], - ) - registrant_ids = result["registrant_ids"] - stats_with_metadata = self._compute_statistics(registrant_ids, variables) - result.update(stats_with_metadata) - result["reference_points_count"] = len(reference_points) - result["radius_km"] = radius_km - result["relation"] = relation - return result + # A failed statement aborts the whole transaction, which would make + # the area fallback below fail too. The savepoint contains it. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + result = self._proximity_by_coordinates(reference_points, radius_meters, relation, filters) except Exception as e: _logger.warning( "Coordinate-based proximity query failed: %s, falling back to area-based", e, ) + # Only the proximity query itself is retried via the fallback; a + # statistics failure is not a spatial failure and must propagate. + if result is not None and result["total_count"] > 0: + _logger.info( + "Proximity query (%s, %.1f km) using coordinates: %s registrants found", + relation, + radius_km, + result["total_count"], + ) + registrant_ids = result["registrant_ids"] + stats_with_metadata = self._compute_statistics(registrant_ids, variables) + result.update(stats_with_metadata) + result["reference_points_count"] = len(reference_points) + result["radius_km"] = radius_km + result["relation"] = relation + return result + # Fall back to area-based query - result = self._proximity_by_area(reference_points, radius_meters, relation, filters) + # A failed statement here would otherwise leave the transaction aborted + # for every subsequent query on this cursor. The savepoint contains it. + # flush=False keeps unrelated pending ORM writes out of the rollback. + with self.env.cr.savepoint(flush=False): + result = self._proximity_by_area(reference_points, radius_meters, relation, filters) _logger.info( "Proximity query (%s, %.1f km) using area fallback: %s registrants in %s areas", relation, diff --git a/spp_api_v2_gis/static/description/index.html b/spp_api_v2_gis/static/description/index.html index b7c6f7e27..5e78ccec4 100644 --- a/spp_api_v2_gis/static/description/index.html +++ b/spp_api_v2_gis/static/description/index.html @@ -501,6 +501,13 @@

Scopes and Data Privacy

Create and archive geofences (also requires gis:read for listing) +gis:incident +Reserved +No endpoint checks it yet; +selectable so incident-scoped +clients can be provisioned ahead +of the incidents API +

What data is exposed

@@ -557,24 +564,44 @@

Dependencies

Changelog

-

19.0.2.0.0

+

19.0.2.0.1

+ +
+
+

19.0.2.0.0

-

Bug Tracker

+

Bug Tracker

Bugs are tracked on GitHub Issues. In case of trouble, please check there if your issue has already been reported. If you spotted it first, help us to smash it by providing a detailed and welcomed @@ -582,7 +609,7 @@

Bug Tracker

Do not contact contributors directly about support or help with technical issues.

-

Credits

+

Credits

diff --git a/spp_api_v2_gis/tests/__init__.py b/spp_api_v2_gis/tests/__init__.py index 06c22da07..bb18c8162 100644 --- a/spp_api_v2_gis/tests/__init__.py +++ b/spp_api_v2_gis/tests/__init__.py @@ -1,4 +1,5 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. +from . import test_api_client_scope from . import test_catalog_service from . import test_export_service from . import test_geofence_model @@ -6,6 +7,8 @@ from . import test_ogc_features from . import test_ogc_http from . import test_qml_template_service +from . import test_spatial_query_coordinates +from . import test_spatial_query_fallback from . import test_spatial_query_service from . import test_statistics_endpoint from . import test_batch_query diff --git a/spp_api_v2_gis/tests/test_api_client_scope.py b/spp_api_v2_gis/tests/test_api_client_scope.py new file mode 100644 index 000000000..645cc5968 --- /dev/null +++ b/spp_api_v2_gis/tests/test_api_client_scope.py @@ -0,0 +1,101 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for the GIS-specific actions on spp.api.client.scope. + +``routers/geofence.py`` gates create/delete on ``has_scope("gis", "geofence")``, +so ``geofence`` has to be a selectable value of the ``action`` field. Otherwise +the only client able to reach those endpoints is one holding ``action = all``. +""" + +from uuid import uuid4 + +from odoo.tests.common import TransactionCase + + +class TestGisActionScopes(TransactionCase): + """The GIS module must contribute its own scope actions.""" + + @classmethod + def setUpClass(cls): + """Set up an organization and type usable by API clients.""" + super().setUpClass() + cls.ApiClient = cls.env["spp.api.client"] + cls.ApiScope = cls.env["spp.api.client.scope"] + + cls.test_partner = cls.env["res.partner"].create({"name": "Geofence Scope Organization"}) + + cls.org_type = cls.env.ref("spp_consent.org_type_government", raise_if_not_found=False) + if not cls.org_type: + cls.org_type = cls.env["spp.consent.org.type"].search([("code", "=", "government")], limit=1) + if not cls.org_type: + cls.org_type = cls.env["spp.consent.org.type"].create({"name": "Government", "code": "government"}) + + def _create_client_with_scopes(self, scopes): + """Create an API client holding the given (resource, action) scopes.""" + # uuid4, not id(scopes): CPython reuses freed addresses, so two + # throwaway scope lists can collide on the unique client_id. + unique = uuid4().hex + client = self.ApiClient.create( + { + "name": f"Geofence Scope Client {unique}", + "client_id": f"test_geofence_client_{unique}", + "partner_id": self.test_partner.id, + "organization_type_id": self.org_type.id, + } + ) + for resource, action in scopes: + self.ApiScope.create( + { + "client_id": client.id, + "resource": resource, + "action": action, + } + ) + return client + + def test_geofence_action_is_selectable(self): + """The geofence action is offered by the action selection.""" + selection = dict(self.ApiScope.fields_get(["action"])["action"]["selection"]) + self.assertIn("geofence", selection) + + def test_incident_action_is_selectable(self): + """The incident action is offered by the action selection.""" + selection = dict(self.ApiScope.fields_get(["action"])["action"]["selection"]) + self.assertIn("incident", selection) + + def test_geofence_scope_can_be_stored(self): + """A gis:geofence scope record can be created.""" + client = self._create_client_with_scopes([("gis", "geofence")]) + + scope = client.scope_ids.filtered(lambda s: s.resource == "gis") + self.assertEqual(scope.action, "geofence") + + def test_geofence_scope_grants_access(self): + """A client holding gis:geofence passes the endpoint check.""" + client = self._create_client_with_scopes([("gis", "geofence")]) + + self.assertTrue(client.has_scope("gis", "geofence")) + + def test_read_scope_does_not_grant_geofence_access(self): + """gis:read is not enough to manage geofences.""" + client = self._create_client_with_scopes([("gis", "read")]) + + self.assertFalse(client.has_scope("gis", "geofence")) + + def test_all_action_still_grants_geofence_access(self): + """gis:all keeps granting geofence access.""" + client = self._create_client_with_scopes([("gis", "all")]) + + self.assertTrue(client.has_scope("gis", "geofence")) + + def test_geofence_scope_does_not_grant_other_actions(self): + """gis:geofence does not widen into unrelated actions.""" + client = self._create_client_with_scopes([("gis", "geofence")]) + + self.assertFalse(client.has_scope("gis", "read")) + self.assertFalse(client.has_scope("gis", "delete")) + + def test_incident_scope_can_be_stored(self): + """A gis:incident scope record can be created.""" + client = self._create_client_with_scopes([("gis", "incident")]) + + self.assertTrue(client.has_scope("gis", "incident")) diff --git a/spp_api_v2_gis/tests/test_batch_query.py b/spp_api_v2_gis/tests/test_batch_query.py index 19cb9fa66..9d2d97d6f 100644 --- a/spp_api_v2_gis/tests/test_batch_query.py +++ b/spp_api_v2_gis/tests/test_batch_query.py @@ -1,9 +1,20 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Tests for batch spatial query service.""" +import json from datetime import date +from unittest.mock import patch from odoo.tests.common import TransactionCase +from odoo.tools import mute_logger + +SERVICE_LOGGER = "odoo.addons.spp_api_v2_gis.services.spatial_query_service" + +# Polygon covering roughly lon 27.9..28.1 / lat -2.1..-1.9 (East Africa). +QUERY_POLYGON = { + "type": "Polygon", + "coordinates": [[[27.9, -2.1], [28.1, -2.1], [28.1, -1.9], [27.9, -1.9], [27.9, -2.1]]], +} class TestBatchSpatialQueryService(TransactionCase): @@ -235,6 +246,214 @@ def test_batch_query_empty_geometries_list(self): self.assertEqual(result["summary"]["geometries_queried"], 0) +class TestBatchQueryDoesNotPoisonLaterGeometries(TransactionCase): + """A geometry that fails inside PostgreSQL must not abort the rest of the batch. + + Both spatial legs of ``query_statistics`` run raw SQL, so a genuine + database error (not a Python-level ``ValueError``) aborts the transaction + unless the failing statement ran inside a savepoint. Without that guard, + every later geometry in the same batch call fails too, since even opening + a new savepoint requires a live transaction. + """ + + @classmethod + def setUpClass(cls): + """Create an area covering the query polygon plus a registrant in it.""" + super().setUpClass() + + cls.area = cls.env["spp.area"].create( + { + "draft_name": "Batch Poison Test Area", + "code": "BATCH-POISON-001", + } + ) + cls.env.cr.execute( + """ + UPDATE spp_area + SET geo_polygon = ST_SetSRID(ST_GeomFromGeoJSON(%s), 4326) + WHERE id = %s + """, + [json.dumps(QUERY_POLYGON), cls.area.id], + ) + + cls.group = cls.env["res.partner"].create( + { + "name": "Batch Poison Test Household", + "is_registrant": True, + "is_group": True, + "area_id": cls.area.id, + } + ) + + def test_invalid_geometry_does_not_abort_later_geometries(self): + """A DB-level failure on one geometry must not poison the geometries after it.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + geometries = [ + { + "id": "invalid", + # Genuinely invalid GeoJSON: ST_GeomFromGeoJSON rejects the type at + # the database level, so this fails the same way a real bad request + # would, rather than being simulated with a mock. + "geometry": {"type": "InvalidType", "coordinates": []}, + }, + { + "id": "valid", + "geometry": QUERY_POLYGON, + }, + ] + + with mute_logger("odoo.sql_db"): + result = service.query_statistics_batch(geometries=geometries) + + self.assertEqual(len(result["results"]), 2) + + invalid_result = next(r for r in result["results"] if r["id"] == "invalid") + self.assertEqual(invalid_result["query_method"], "error") + + valid_result = next(r for r in result["results"] if r["id"] == "valid") + self.assertEqual( + valid_result["query_method"], + "area_fallback", + "a DB error on an earlier geometry aborted the transaction for the rest of the batch", + ) + self.assertEqual(valid_result["total_count"], 1) + self.assertEqual(valid_result["areas_matched"], 1) + + +class TestBatchStatisticsFailureDoesNotPoisonBatch(TransactionCase): + """A statistics failure on one geometry must not abort the rest of the batch. + + The spatial legs guard themselves with savepoints, but the statistics + computation the matched registrants are handed to can also fail inside + PostgreSQL. Each batch iteration (and the batch summary) therefore runs + inside its own savepoint, so a poisoned transaction is rolled back before + the next geometry — or the summary — touches the cursor. + """ + + @classmethod + def setUpClass(cls): + """Create an area covering the query polygon plus a registrant in it.""" + super().setUpClass() + + cls.area = cls.env["spp.area"].create( + { + "draft_name": "Batch Stats Poison Test Area", + "code": "BATCH-STATS-POISON-001", + } + ) + cls.env.cr.execute( + """ + UPDATE spp_area + SET geo_polygon = ST_SetSRID(ST_GeomFromGeoJSON(%s), 4326) + WHERE id = %s + """, + [json.dumps(QUERY_POLYGON), cls.area.id], + ) + + cls.group = cls.env["res.partner"].create( + { + "name": "Batch Stats Poison Test Household", + "is_registrant": True, + "is_group": True, + "area_id": cls.area.id, + } + ) + + def _failing_then_real_statistics(self, failing_call_numbers): + """Build a ``_compute_statistics`` stand-in that dies on the given calls. + + The failure is a genuine database error, so the transaction is aborted + exactly the way a real statistics failure would abort it. + """ + from ..services.spatial_query_service import SpatialQueryService + + original = SpatialQueryService._compute_statistics + calls = [] + + def compute_statistics(service_self, registrant_ids, variables): + calls.append(len(registrant_ids)) + if len(calls) in failing_call_numbers: + service_self.env.cr.execute("SELECT id FROM spp_table_that_does_not_exist") + return original(service_self, registrant_ids, variables) + + return compute_statistics + + def test_statistics_failure_does_not_abort_later_geometries(self): + """A DB error while computing one geometry's statistics spares the rest.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + geometries = [ + {"id": "poisoned", "geometry": QUERY_POLYGON}, + {"id": "healthy", "geometry": QUERY_POLYGON}, + ] + + with ( + patch.object( + SpatialQueryService, + "_compute_statistics", + self._failing_then_real_statistics({1}), + ), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING") as captured, + ): + result = service.query_statistics_batch(geometries=geometries) + + poisoned = next(r for r in result["results"] if r["id"] == "poisoned") + self.assertEqual(poisoned["query_method"], "error") + + healthy = next(r for r in result["results"] if r["id"] == "healthy") + self.assertEqual( + healthy["query_method"], + "area_fallback", + "a statistics DB error on an earlier geometry aborted the transaction for the rest of the batch", + ) + self.assertEqual(healthy["total_count"], 1) + self.assertTrue( + any("Batch query failed for geometry 'poisoned'" in message for message in captured.output), + f"expected a batch failure warning, got {captured.output}", + ) + + def test_summary_statistics_failure_degrades_to_empty_summary(self): + """A DB error in the summary statistics keeps the per-geometry results.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + geometries = [{"id": "only", "geometry": QUERY_POLYGON}] + + # Call 1 computes the geometry's own statistics; call 2 is the summary. + with ( + patch.object( + SpatialQueryService, + "_compute_statistics", + self._failing_then_real_statistics({2}), + ), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING") as captured, + ): + result = service.query_statistics_batch(geometries=geometries) + + only = next(r for r in result["results"] if r["id"] == "only") + self.assertEqual(only["query_method"], "area_fallback") + self.assertEqual(only["total_count"], 1) + + self.assertEqual(result["summary"]["total_count"], 1) + self.assertEqual(result["summary"]["statistics"], {}) + self.assertTrue( + any("Batch summary statistics failed" in message for message in captured.output), + f"expected a summary failure warning, got {captured.output}", + ) + + # The cursor must still be usable after the degraded summary. + self.env.cr.execute("SELECT id FROM res_partner WHERE id = %s", [self.group.id]) + self.assertEqual(self.env.cr.fetchall(), [(self.group.id,)]) + + class TestBatchSpatialQuerySchemas(TransactionCase): """Test batch query Pydantic schemas.""" diff --git a/spp_api_v2_gis/tests/test_spatial_query_coordinates.py b/spp_api_v2_gis/tests/test_spatial_query_coordinates.py new file mode 100644 index 000000000..29d295fc7 --- /dev/null +++ b/spp_api_v2_gis/tests/test_spatial_query_coordinates.py @@ -0,0 +1,206 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for the coordinate-based branch of the spatial query service. + +``res.partner.coordinates`` is added by ``spp_registrant_gis``, which is not a +dependency of ``spp_api_v2_gis``. These tests create the PostGIS column and +declare the field on the model for the duration of the test so the coordinate +branch can be exercised without adding a module dependency. +""" + +import json +from contextlib import contextmanager +from types import MappingProxyType +from unittest.mock import patch + +from odoo import fields +from odoo.tests.common import TransactionCase + +SERVICE_LOGGER = "odoo.addons.spp_api_v2_gis.services.spatial_query_service" + +# Polygon covering roughly lon 27.9..28.1 / lat -2.1..-1.9 (East Africa). +QUERY_POLYGON = { + "type": "Polygon", + "coordinates": [[[27.9, -2.1], [28.1, -2.1], [28.1, -1.9], [27.9, -1.9], [27.9, -2.1]]], +} + + +@contextmanager +def declared_coordinates_field(env): + """Declare ``coordinates`` on res.partner as ``spp_registrant_gis`` would. + + ``_query_by_coordinates`` refuses to run when the field is absent, so the + field has to be visible on the model while the query runs. ``_fields`` is a + read-only mapping, so the whole mapping is swapped for a widened copy. + + When ``spp_registrant_gis`` is installed (e.g. the full SP-MIS stack), the + real field is already there and must not be shadowed by an un-set-up copy. + """ + partner_cls = type(env["res.partner"]) + if "coordinates" in partner_cls._fields: + yield + return + widened = MappingProxyType({**partner_cls._fields, "coordinates": fields.GeoPointField()}) + with patch.object(partner_cls, "_fields", widened): + yield + + +class TestQueryByCoordinates(TransactionCase): + """Coordinate query must bind its parameters in the order the SQL expects.""" + + @classmethod + def setUpClass(cls): + """Add the coordinates column and create registrants inside/outside the polygon.""" + super().setUpClass() + + # Mirrors the geometry(Point, 4326) column created by GeoPointField. + # IF NOT EXISTS: the column is real when spp_registrant_gis is installed. + cls.env.cr.execute("ALTER TABLE res_partner ADD COLUMN IF NOT EXISTS coordinates geometry(Point, 4326)") + + cls.group_inside = cls.env["res.partner"].create( + { + "name": "Coordinates Household Inside", + "is_registrant": True, + "is_group": True, + } + ) + cls.individual_inside = cls.env["res.partner"].create( + { + "name": "Coordinates Individual Inside", + "is_registrant": True, + "is_group": False, + } + ) + cls.group_outside = cls.env["res.partner"].create( + { + "name": "Coordinates Household Outside", + "is_registrant": True, + "is_group": True, + } + ) + + cls._set_coordinates(cls.group_inside, 28.0, -2.0) + cls._set_coordinates(cls.individual_inside, 28.01, -2.01) + cls._set_coordinates(cls.group_outside, 32.0, -5.0) + + @classmethod + def _set_coordinates(cls, partner, longitude, latitude): + """Write a point into the raw coordinates column.""" + cls.env.cr.execute( + "UPDATE res_partner SET coordinates = ST_SetSRID(ST_MakePoint(%s, %s), 4326) WHERE id = %s", + [longitude, latitude, partner.id], + ) + + def _get_service(self): + """Create a SpatialQueryService instance.""" + from ..services.spatial_query_service import SpatialQueryService + + return SpatialQueryService(self.env) + + def test_query_without_filters(self): + """Without filters, every registrant inside the polygon is returned.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service._query_by_coordinates(json.dumps(QUERY_POLYGON), {}) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.group_inside.id, result["registrant_ids"]) + self.assertIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + + def test_is_group_true_filter(self): + """The is_group filter must be bound to p.is_group, not to the geometry.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service._query_by_coordinates(json.dumps(QUERY_POLYGON), {"is_group": True}) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.group_inside.id, result["registrant_ids"]) + self.assertNotIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + + def test_is_group_false_filter(self): + """is_group=False returns individuals inside the polygon only.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service._query_by_coordinates(json.dumps(QUERY_POLYGON), {"is_group": False}) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + + def test_is_group_filter_combined_with_disabled_filter(self): + """The disabled filter adds no placeholder and must not shift the params.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service._query_by_coordinates( + json.dumps(QUERY_POLYGON), + {"is_group": True, "disabled": False}, + ) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.group_inside.id, result["registrant_ids"]) + self.assertNotIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + + def test_query_statistics_uses_coordinates(self): + """End to end, query_statistics prefers the coordinate method.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service.query_statistics(geometry=QUERY_POLYGON) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.group_inside.id, result["registrant_ids"]) + self.assertIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + + def test_query_proximity_uses_coordinates(self): + """End to end, query_proximity prefers the coordinate method.""" + service = self._get_service() + + with declared_coordinates_field(self.env): + result = service.query_proximity( + reference_points=[{"longitude": 28.0, "latitude": -2.0}], + radius_km=10, + ) + + self.assertEqual(result["query_method"], "coordinates") + self.assertIn(self.group_inside.id, result["registrant_ids"]) + self.assertIn(self.individual_inside.id, result["registrant_ids"]) + self.assertNotIn(self.group_outside.id, result["registrant_ids"]) + self.assertEqual(result["relation"], "within") + self.assertEqual(result["radius_km"], 10) + + def test_statistics_failure_propagates_instead_of_retrying_via_fallback(self): + """A statistics failure after a successful coordinate query propagates. + + It is not a spatial failure, so it must not be logged as a + coordinate-query failure and silently retried through the area + fallback (which would recompute the same statistics anyway). + """ + from ..services.spatial_query_service import SpatialQueryService + + service = self._get_service() + + def exploding_statistics(service_self, registrant_ids, variables): + raise RuntimeError("statistics exploded") + + # Deliberately not self.assertRaises: TransactionCase overrides it to + # wrap the block in a *flushing* savepoint, and the flush runs pending + # precommit hooks against the temporarily widened _fields mapping. + raised = None + with ( + declared_coordinates_field(self.env), + patch.object(SpatialQueryService, "_compute_statistics", exploding_statistics), + self.assertNoLogs(SERVICE_LOGGER, level="WARNING"), + ): + try: + service.query_statistics(geometry=QUERY_POLYGON) + except RuntimeError as exc: + raised = exc + self.assertIsNotNone(raised, "a statistics failure must propagate to the caller") diff --git a/spp_api_v2_gis/tests/test_spatial_query_fallback.py b/spp_api_v2_gis/tests/test_spatial_query_fallback.py new file mode 100644 index 000000000..12e56392c --- /dev/null +++ b/spp_api_v2_gis/tests/test_spatial_query_fallback.py @@ -0,0 +1,232 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Tests for the area fallbacks taken when a coordinate query fails. + +``query_statistics`` and ``query_proximity`` both catch failures from their +coordinate-based query and retry with the area-based query on the same cursor. +A failed statement aborts the PostgreSQL transaction, so the coordinate query +has to run inside a savepoint for either fallback to be reachable at all. +""" + +import json +from unittest.mock import patch + +from odoo.tests.common import TransactionCase +from odoo.tools import mute_logger + +SERVICE_LOGGER = "odoo.addons.spp_api_v2_gis.services.spatial_query_service" + +# Polygon covering roughly lon 27.9..28.1 / lat -2.1..-1.9 (East Africa). +QUERY_POLYGON = { + "type": "Polygon", + "coordinates": [[[27.9, -2.1], [28.1, -2.1], [28.1, -1.9], [27.9, -1.9], [27.9, -2.1]]], +} + + +# Centre of QUERY_POLYGON, used as the proximity reference point. +REFERENCE_POINTS = [{"longitude": 28.0, "latitude": -2.0}] + + +def _failing_coordinate_query(self, geometry_json, filters): + """Stand-in for a coordinate query that dies inside PostgreSQL.""" + self.env.cr.execute("SELECT id FROM spp_table_that_does_not_exist") + + +def _failing_proximity_query(self, reference_points, radius_meters, relation, filters): + """Stand-in for a proximity query that dies inside PostgreSQL.""" + self.env.cr.execute("SELECT id FROM spp_table_that_does_not_exist") + + +class TestCoordinateQueryFallback(TransactionCase): + """The failed coordinate attempt must not poison the area fallback.""" + + @classmethod + def setUpClass(cls): + """Create an area covering the query polygon plus a registrant in it.""" + super().setUpClass() + + cls.area = cls.env["spp.area"].create( + { + "draft_name": "Fallback Test Area", + "code": "FALLBACK-AREA-001", + } + ) + cls.env.cr.execute( + """ + UPDATE spp_area + SET geo_polygon = ST_SetSRID(ST_GeomFromGeoJSON(%s), 4326) + WHERE id = %s + """, + [json.dumps(QUERY_POLYGON), cls.area.id], + ) + + cls.group = cls.env["res.partner"].create( + { + "name": "Fallback Test Household", + "is_registrant": True, + "is_group": True, + "area_id": cls.area.id, + } + ) + + def test_area_fallback_runs_after_failed_coordinate_query(self): + """A SQL error in the coordinate query degrades to the area query.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + with ( + patch.object(SpatialQueryService, "_query_by_coordinates", _failing_coordinate_query), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING") as captured, + ): + result = service.query_statistics(geometry=QUERY_POLYGON) + + self.assertEqual(result["query_method"], "area_fallback") + self.assertIn(self.group.id, result["registrant_ids"]) + self.assertTrue( + any("Coordinate-based query failed" in message for message in captured.output), + f"expected a fallback warning, got {captured.output}", + ) + + def test_cursor_stays_usable_after_failed_coordinate_query(self): + """The transaction is still usable once the fallback has completed.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + with ( + patch.object(SpatialQueryService, "_query_by_coordinates", _failing_coordinate_query), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING"), + ): + service.query_statistics(geometry=QUERY_POLYGON) + + self.env.cr.execute("SELECT id FROM res_partner WHERE id = %s", [self.group.id]) + self.assertEqual(self.env.cr.fetchall(), [(self.group.id,)]) + + +class TestProximityQueryFallback(TransactionCase): + """query_proximity has the same fallback, and needs the same savepoint.""" + + @classmethod + def setUpClass(cls): + """Create an area covering the reference point plus a registrant in it.""" + super().setUpClass() + + cls.area = cls.env["spp.area"].create( + { + "draft_name": "Proximity Fallback Test Area", + "code": "FALLBACK-AREA-002", + } + ) + cls.env.cr.execute( + """ + UPDATE spp_area + SET geo_polygon = ST_SetSRID(ST_GeomFromGeoJSON(%s), 4326) + WHERE id = %s + """, + [json.dumps(QUERY_POLYGON), cls.area.id], + ) + + cls.group = cls.env["res.partner"].create( + { + "name": "Proximity Fallback Test Household", + "is_registrant": True, + "is_group": True, + "area_id": cls.area.id, + } + ) + + def test_area_fallback_runs_after_failed_proximity_query(self): + """A SQL error in the proximity query degrades to the area query.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + with ( + patch.object(SpatialQueryService, "_proximity_by_coordinates", _failing_proximity_query), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING") as captured, + ): + result = service.query_proximity(reference_points=REFERENCE_POINTS, radius_km=10) + + self.assertEqual(result["query_method"], "area_fallback") + self.assertIn(self.group.id, result["registrant_ids"]) + self.assertTrue( + any("Coordinate-based proximity query failed" in message for message in captured.output), + f"expected a fallback warning, got {captured.output}", + ) + + def test_cursor_stays_usable_after_failed_proximity_query(self): + """The transaction is still usable once the fallback has completed.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + with ( + patch.object(SpatialQueryService, "_proximity_by_coordinates", _failing_proximity_query), + mute_logger("odoo.sql_db"), + self.assertLogs(SERVICE_LOGGER, level="WARNING"), + ): + service.query_proximity(reference_points=REFERENCE_POINTS, radius_km=10) + + self.env.cr.execute("SELECT id FROM res_partner WHERE id = %s", [self.group.id]) + self.assertEqual(self.env.cr.fetchall(), [(self.group.id,)]) + + +class TestProximityAreaFallbackDoesNotPoisonTransaction(TransactionCase): + """query_proximity's own area fallback must not leave the transaction aborted. + + Both legs of ``query_proximity`` run raw SQL. The area fallback + (``_proximity_by_area``) has to run inside a savepoint just like the + coordinate attempt: a genuine database error there still raises out of + query_proximity, but the savepoint keeps the cursor usable for whatever + runs next, instead of every later query failing with + ``InFailedSqlTransaction``. + """ + + def test_non_finite_radius_does_not_abort_the_transaction(self): + """A DB-level failure in the area fallback must not poison later queries.""" + from ..services.spatial_query_service import SpatialQueryService + + service = SpatialQueryService(self.env) + + # The coordinate attempt fails whether or not res.partner has a real + # "coordinates" field: without one (this module's own test stack) it + # raises a plain ValueError before touching the database, with one + # (e.g. spp_registrant_gis installed) ST_Buffer rejects the + # non-finite radius. Either way the coordinate leg's savepoint + # recovers it, exactly as intended. + # + # query_proximity then falls back to _proximity_by_area, which + # shares the same temp-table helper and is therefore handed the same + # non-finite radius. ST_Buffer rejects a non-finite distance + # argument at the database level ("distance must be a finite + # value"), a genuine, deterministic PostGIS error, not a simulated + # one: unlike an out-of-range or non-finite *coordinate*, which + # PostGIS/GEOS only coerces or fails on inconsistently depending on + # the exact computation involved, a non-finite *buffer distance* is + # rejected by a straightforward argument check every time. Without a + # savepoint around the fallback, this second failure would abort the + # transaction. + reference_points = [{"longitude": 28.0, "latitude": -2.0}] + + # Deliberately not self.assertRaises: TransactionCase overrides + # assertRaises (see BaseCase._assertRaises in odoo/tests/common.py) + # to wrap the block in its own savepoint and roll it back on the + # expected exception. That would recover the transaction as a side + # effect of the assertion itself, hiding exactly the bug this test + # exists to catch. A plain try/except leaves the transaction exactly + # as query_proximity left it, for the assertion below to see. + raised = None + with mute_logger("odoo.sql_db"): + try: + service.query_proximity(reference_points=reference_points, radius_km=float("nan")) + except Exception as exc: + raised = exc + self.assertIsNotNone(raised, "query_proximity should raise when the area fallback hits a DB error") + + # An ordinary ORM query must still work; without the savepoint around + # the area fallback, the aborted transaction raises + # InFailedSqlTransaction here instead. + self.env["res.partner"].search_count([]) diff --git a/spp_api_v2_gis/tests/test_statistics_endpoint.py b/spp_api_v2_gis/tests/test_statistics_endpoint.py index 3a4e7ca8e..b57c39467 100644 --- a/spp_api_v2_gis/tests/test_statistics_endpoint.py +++ b/spp_api_v2_gis/tests/test_statistics_endpoint.py @@ -1,6 +1,8 @@ # Part of OpenSPP. See LICENSE file for full copyright and licensing details. """Tests for statistics discovery endpoint.""" +from uuid import uuid4 + from odoo.tests.common import TransactionCase @@ -250,7 +252,9 @@ def _create_client_with_scopes(self, scopes): client = self.ApiClient.create( { "name": f"Test Client {len(scopes)}", - "client_id": f"test_client_{id(scopes)}", + # uuid4, not id(scopes): CPython reuses freed addresses, so two + # throwaway scope lists can collide on the unique client_id. + "client_id": f"test_client_{uuid4().hex}", "partner_id": self.test_partner.id, "organization_type_id": self.org_type.id, }