fix(spp_api_v2_gis): repair aborted-transaction fallbacks and geofence/incident scope actions - #375
Open
jeremi wants to merge 8 commits into
Open
fix(spp_api_v2_gis): repair aborted-transaction fallbacks and geofence/incident scope actions#375jeremi wants to merge 8 commits into
jeremi wants to merge 8 commits into
Conversation
The filter placeholders live inside {where_clause}, which precedes the
geometry placeholder, but the params list was built geometry-first and
then rebuilt into the identical list. With an is_group filter the GeoJSON
string was bound to p.is_group and PostgreSQL rejected the statement.
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
query_statistics catches a failing coordinate query and retries with the area query on the same cursor. Without a savepoint the first failure had already aborted the transaction, so the fallback raised InFailedSqlTransaction and the endpoint returned 500 instead of degrading. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
routers/geofence.py gates create and delete on
has_scope("gis", "geofence"), but geofence was not a selectable action,
so only clients holding gis:all could reach those endpoints and no
client could ever be granted the intended scope.
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
No migration script: selection_add only widens the allowed set of action values, every stored value stays valid, the column stays varchar and there is no SQL constraint on it. The version bump is what makes Odoo re-register the selection on update. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
query_proximity has the same defect as query_statistics: it catches a failing coordinate query and retries with the area query on the same cursor, but the first failure had already aborted the transaction, so _create_proximity_temp_table raised InFailedSqlTransaction and the endpoint returned 500 instead of degrading. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
query_statistics wrapped its coordinate attempt in a savepoint but left the area fallback's raw SQL unguarded. A genuine database error there (not the ValueError path used when res.partner.coordinates is absent) aborted the whole transaction, and since opening a savepoint itself requires a live transaction, every later geometry in the same query_statistics_batch call failed too. One bad geometry degraded the entire batch response to total_count: 0, query_method: "error" for everything after it, with nothing indicating why. This completes the savepoint coverage the branch already started for the coordinate-query fallbacks in query_statistics and query_proximity. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
query_proximity wraps its coordinate attempt in a savepoint but left the area fallback's raw SQL unguarded. A genuine database error there aborted the whole transaction, leaving every later query on the same cursor unusable, since even opening a new savepoint requires a live transaction. This completes the savepoint coverage started in 2bfefcb4 (coordinate query) and 9ff662f1 (query_statistics's own area fallback), applying the same fix to query_proximity's area fallback. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #375 +/- ##
==========================================
- Coverage 74.28% 69.24% -5.04%
==========================================
Files 372 133 -239
Lines 25385 12178 -13207
==========================================
- Hits 18857 8433 -10424
+ Misses 6528 3745 -2783
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The committed README.rst and index.html were generated on a developer
machine whose docutils/pandoc renders RST tables one column wider than
CI's, so pre-commit rewrote both files on every run and failed the job.
Take CI's output verbatim: the OGC endpoints table's Description column
is 29 characters, matching its widest cell ("QGIS style file
(extension)", 27) plus padding.
The generator is not reproducible across machines even though
.pre-commit-config.yaml pins docutils and markdown-it-py, because pandoc
is a system binary the pin cannot reach. Tracked separately.
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9d6923846
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| super().setUpClass() | ||
|
|
||
| # Mirrors the geometry(Point, 4326) column created by GeoPointField. | ||
| cls.env.cr.execute("ALTER TABLE res_partner ADD COLUMN coordinates geometry(Point, 4326)") |
There was a problem hiding this comment.
When these tests run in a database where spp_registrant_gis is already installed, this unconditional ALTER TABLE raises PostgreSQL DuplicateColumn because that module defines the same res_partner.coordinates column. This occurs in combined installations such as spp_mis_demo_v2, which depends on both modules, and prevents the GIS API tests from running; check for the field/column first or use ADD COLUMN IF NOT EXISTS.
AGENTS.md reference: AGENTS.md:L107-L115
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes five independent defects in
spp_api_v2_gis's spatial-query and access-scope code, all found while auditing the module for the aborted-transaction bug class.query_statistics,query_statistics_batchandquery_proximityeach retry a failed coordinate-based lookup with an area-based fallback on the same database cursor. Three of the four fallback paths ran outside any savepoint, so a genuine database error in the coordinate attempt left the transaction aborted, and the fallback (plus every later query on that cursor) failed withInFailedSqlTransactioninstead of degrading gracefully.A fourth, pre-existing bug bound SQL parameters out of order when an
is_groupfilter was present, feeding a GeoJSON string into a numeric column position.Separately,
routers/geofence.pygated create/delete onhas_scope("gis", "geofence"), butgeofencewas never added as a selectable client scope action, so no client could ever be granted that scope short ofgis:all. The same gap existed forincident.11 files changed, 647 insertions(+), 34 deletions(-) versus
19.0.What it removes, renames, or breaks
Nothing. All fixes are additive: a savepoint around an existing query, a parameter-order correction, and two new selectable scope-action values.
The manifest bumps
19.0.2.0.0→19.0.2.0.1only so Odoo re-registers the widenedSelectionfor scope actions. No field is added, removed, or retyped.Two related items are explicitly out of scope here and handled elsewhere:
_compute_statisticsstill has its own unprotected raw-SQL sites, and a broader inventory found roughly 20 further aborted-transaction-shaped sites across the codebase. Please do not read this branch as having closed the bug class, only its own four sites.Migrations
Manifest bump only, no migration script.
selection_addwidens the allowed set ofactionvalues; every stored value stays valid and there is no SQL constraint on the column, so the bump exists solely to trigger re-registration on-u.Known gap: nobody has exercised this specific
19.0.2.0.0→19.0.2.0.1upgrade end to end against a database that already holdsspp.api.client.scoperows. The "no migration needed" conclusion rests on the reasoning above, not on an executed upgrade.This PR must merge before #281, or
spp_api_v2_gisregresses in a way that cannot be recovered.spp_api_v2_gisversion19.019.0.2.0.019.0.2.0.119.0.3.0.0If any part of #281 merges first, this PR then drives the manifest backwards from
19.0.3.xto19.0.2.0.1. Odoo still performs the upgrade, because a lower version is still a different version, but the dispatch windowinstalled < V <= manifestbecomes empty for every conceivableVfrom that point on, so no migration for this module can ever run again until some future bump climbs back above19.0.3.1.0. Nothing warns, the module keeps working, and the next schema change it ships silently does not migrate. That is precisely the failure that forced the #76 revert.Merging in the documented order costs nothing:
2.0.0 → 2.0.1 → 3.0.0, monotonic throughout. If for any reason this PR ends up merging second, re-bump its manifest above whatever is on19.0at that moment, rather than merging as-is.Second, semantic constraint: #281 independently fixed the same parameter-order bug but carries zero savepoints. A merge that resolves in #281's favour silently reintroduces both aborted-transaction paths. Both sides look correct in isolation and the conflict is semantic rather than textual, so this one needs a human check at merge time. The scope-action fix is unaffected;
models/api_client_scope.pyis byte-identical across the branches.Verification
./spp test spp_api_v2_gison this branch at0a11b276, read from the run log rather than the console summary:12 of those 209 skipped, and they are worth naming rather than hiding behind the green:
TestCatalogService.test_get_catalog_report_driven_data_layerTestCatalogService.test_get_catalog_with_data_layersTestLayersService.test_build_layer_stylingTestLayersService.test_fetch_layer_features_limitTestLayersService.test_fetch_layer_features_without_geometryTestLayersService.test_get_data_layer_as_geojsonTestOGCService.test_get_collection_by_layer_idTestOGCService.test_get_collections_contains_data_layersTestOGCService.test_model_driven_data_layer_collection_has_no_qml_linkTestOGCService.test_report_driven_data_layer_collection_has_qml_linkTestOGCHTTP.test_qml_for_model_driven_data_layer_returns_404TestOGCHTTP.test_qml_for_report_driven_data_layer_returns_200These skips are pre-existing on
19.0and not introduced by this branch, but they are the bad kind: each one skips itself precisely when the geo field it exists to exercise is missing, so the suite goes green in exactly the situation the test was written to catch. Converting this family fromskipTestto hard assertions is tracked separately and is expected to land after #281, which is what makes the geo fields reliably present. Flagging it here so nobody reads0 failedon this module as full coverage of the data-layer paths.Historical context for the fixes themselves: the coordinate savepoint fix took the query-parameter-order test class from
2 failed, 9 error(s)of 14 tests to0 failed, 0 error(s)of 4 (the other 10 were unaffected by this bug), and the full suite from2 failed, 10 error(s)of 205 tests to clean.All four savepoints use
cr.savepoint(flush=False). This is a deliberate deviation from the default flushing behaviour, with the rationale inline at each site: it keeps unrelated pending ORM writes out of the rollback. Worth a reviewer's second look, because it means only the raw SQL inside thewithblock is protected, not any not-yet-flushed ORM writes made earlier in the same request.Assumptions
geofenceandincidentscope actions were added as part of this fix rather than split into a separate PR, on the grounds that the gate without the selectable value is not shippable on its own. Flagged here in case you would rather they had been left alone.Signed-off-by:.