feat: Implement duplicate entity redirection logic and add correspond… - #123
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
request-processor/src/application/core/duplicates.py (1)
1-333: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI: Black formatting check fails on this file.
Pipeline logs report Black would reformat this file. Run
python -m black --write --exclude "digital-land" ./src ./testsbefore merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/src/application/core/duplicates.py` around lines 1 - 333, Black formatting is out of sync in duplicates.py, so re-run Black on the module and keep the file formatted consistently with the rest of the codebase. Focus on the affected symbols such as _organisation_row_for_provider, _build_candidate, and find_duplicate_redirect_candidates, then verify the diff contains only Black-driven formatting changes and no logic changes.Source: Pipeline failures
request-processor/tests/unit/src/test_duplicates.py (1)
1-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCI: Black formatting check fails on this file.
Pipeline logs report Black would reformat this file. Run
python -m black --write --exclude "digital-land" ./src ./testsbefore merging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/tests/unit/src/test_duplicates.py` around lines 1 - 249, The Black formatting check is failing for this test module, so reformat the file to match Black’s style. Run the repository Black formatter over the affected tests, then verify the updated test functions and helper definitions in this file still read cleanly and preserve behavior.Source: Pipeline failures
🧹 Nitpick comments (3)
request-processor/tests/unit/src/test_duplicates.py (2)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable dict as class attribute.
FakeOrganisationIndex.organisationis a mutable dict defined at class level, shared across all instances/tests. Harmless here since it's read-only in these tests, but worth moving into__init__to avoid latent shared-state bugs if the fixture is ever mutated in a future test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/tests/unit/src/test_duplicates.py` around lines 14 - 21, The FakeOrganisationIndex.organisation attribute is a mutable class-level dict shared across instances and tests; move it into the FakeOrganisationIndex initializer so each test gets its own copy. Update the FakeOrganisationIndex class to set organisation on self in __init__, and keep the existing test references working through the instance attribute to avoid latent shared-state bugs.Source: Linters/SAST tools
66-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the old/new organisation_entity mapping swap.
All current tests exercise only the branch where
entity_ais the platform (old) entity andentity_bis the provision (new) entity. Consider adding a case where the match hasentity_aas the new/provision entity andentity_bas the old/platform entity (withold_entitymissing an explicitorganisation_entitykey) to catch the_build_candidatefallback mapping issue flagged induplicates.py.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/tests/unit/src/test_duplicates.py` around lines 66 - 132, Add a test that covers the reversed match shape in find_duplicate_redirect_candidates/_build_candidate, where entity_a is the provision/new entity and entity_b is the old/platform entity, and the old entity does not include an organisation_entity field. Use fake_run_duplicate_check and fake_fetch_platform_entities to assert the fallback mapping still assigns old_organisation and new_organisation correctly, so the candidate is built with the swapped old/new fields in this branch.request-processor/src/application/core/duplicates.py (1)
252-297: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate temp SQLite DB + spatialite connection is created twice per request.
_run_duplicate_checkis invoked once perspatial_field(geometry, thenpoint), each creating and tearing down its own temp file and spatialite connection/table. Since both calls operate on the samecombined_rows, this could be consolidated into a single connection/table setup that runsduplicate_geometry_checkfor both fields, halving the per-request I/O and spatialite/temp-file overhead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/src/application/core/duplicates.py` around lines 252 - 297, The duplicate scan in find_duplicate_redirect_candidates creates a temp SQLite/spatialite setup twice because _run_duplicate_check is called separately for "geometry" and "point" over the same combined_rows. Refactor the flow so the shared setup is created once and duplicate_geometry_check runs for both spatial fields within a single connection/table lifecycle, reusing the same combined dataset instead of rebuilding it per field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@request-processor/src/application/core/duplicates.py`:
- Around line 199-245: The fallback mapping for organisation entities in
_build_candidate is wrong for matches produced by the first branch in
find_duplicate_redirect_candidates. Update the call site to pass the resolved
old/new organisation_entity values (or explicitly pass the correct a/b mapping
for each branch) instead of relying on match.get("organisation_entity_a") and
match.get("organisation_entity_b") inside _build_candidate. Keep the logic
aligned with the existing _build_candidate and
find_duplicate_redirect_candidates symbols so old_organisation_entity and
new_organisation_entity always correspond to old_entity and new_entity.
- Around line 38-46: The allowlist in duplicates.py drops organisation_entity,
so provision rows never retain the resolved organisation entity and
_build_candidate ends up producing a blank new_organisation_entity. Update the
entity_fields set and the related candidate-building flow so the value resolved
by _resolve_organisation_entity is preserved into _create_entity_table and then
passed through _build_candidate for provision-side rows instead of relying only
on match.get("organisation_entity_b").
- Around line 153-177: The _run_duplicate_check function leaves the SpatiaLite
connection open because the with spatialite.connect(path) block only handles
transaction scope, not cleanup. Update this flow to explicitly close the
connection in a try/finally around the duplicate_geometry_check call, using the
conn symbol from _run_duplicate_check, and ensure conn.close() runs before the
temp file is removed in the existing cleanup block.
---
Outside diff comments:
In `@request-processor/src/application/core/duplicates.py`:
- Around line 1-333: Black formatting is out of sync in duplicates.py, so re-run
Black on the module and keep the file formatted consistently with the rest of
the codebase. Focus on the affected symbols such as
_organisation_row_for_provider, _build_candidate, and
find_duplicate_redirect_candidates, then verify the diff contains only
Black-driven formatting changes and no logic changes.
In `@request-processor/tests/unit/src/test_duplicates.py`:
- Around line 1-249: The Black formatting check is failing for this test module,
so reformat the file to match Black’s style. Run the repository Black formatter
over the affected tests, then verify the updated test functions and helper
definitions in this file still read cleanly and preserve behavior.
---
Nitpick comments:
In `@request-processor/src/application/core/duplicates.py`:
- Around line 252-297: The duplicate scan in find_duplicate_redirect_candidates
creates a temp SQLite/spatialite setup twice because _run_duplicate_check is
called separately for "geometry" and "point" over the same combined_rows.
Refactor the flow so the shared setup is created once and
duplicate_geometry_check runs for both spatial fields within a single
connection/table lifecycle, reusing the same combined dataset instead of
rebuilding it per field.
In `@request-processor/tests/unit/src/test_duplicates.py`:
- Around line 14-21: The FakeOrganisationIndex.organisation attribute is a
mutable class-level dict shared across instances and tests; move it into the
FakeOrganisationIndex initializer so each test gets its own copy. Update the
FakeOrganisationIndex class to set organisation on self in __init__, and keep
the existing test references working through the instance attribute to avoid
latent shared-state bugs.
- Around line 66-132: Add a test that covers the reversed match shape in
find_duplicate_redirect_candidates/_build_candidate, where entity_a is the
provision/new entity and entity_b is the old/platform entity, and the old entity
does not include an organisation_entity field. Use fake_run_duplicate_check and
fake_fetch_platform_entities to assert the fallback mapping still assigns
old_organisation and new_organisation correctly, so the candidate is built with
the swapped old/new fields in this branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb1b9966-86a4-47c6-863d-4ef8ce022930
📒 Files selected for processing (6)
request-processor/Dockerfilerequest-processor/requirements/requirements.inrequest-processor/requirements/requirements.txtrequest-processor/src/application/core/duplicates.pyrequest-processor/src/application/core/pipeline.pyrequest-processor/tests/unit/src/test_duplicates.py
… add tests for organisation entity mapping
…rove test readability for name similarity
…g and update tests
…resources, including duplicate candidate analysis
…call in issue logs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
request-processor/src/application/core/pipeline.py (1)
396-439: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDuplicate detection should include every transformed resource. This path already processes every file in
input_dir, but_find_duplicate_candidates(...)runs once after the loop against a singleoutput_path, so earlier resource files are not included. If multi-file add-data requests are supported here, collect the transformed outputs or run duplicate detection per file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@request-processor/src/application/core/pipeline.py` around lines 396 - 439, Duplicate detection is only run once after the file-processing loop, so it can miss transformed resources from earlier files. Update the add-data flow in pipeline.py around _process_add_data_resource and _find_duplicate_candidates so duplicate checking covers every processed resource, either by running it per file inside the loop or by aggregating all transformed outputs before calling it. Make sure the fix preserves the existing handling of transformed_entities, new_lookups, and entity_org_mapping for multi-file requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@request-processor/src/application/core/pipeline.py`:
- Around line 396-439: Duplicate detection is only run once after the
file-processing loop, so it can miss transformed resources from earlier files.
Update the add-data flow in pipeline.py around _process_add_data_resource and
_find_duplicate_candidates so duplicate checking covers every processed
resource, either by running it per file inside the loop or by aggregating all
transformed outputs before calling it. Make sure the fix preserves the existing
handling of transformed_entities, new_lookups, and entity_org_mapping for
multi-file requests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a6991d9-9c1e-43d9-ab06-ed97aea1ddc4
📒 Files selected for processing (3)
request-processor/requirements/test_requirements.txtrequest-processor/src/application/core/pipeline.pyrequest-processor/tests/unit/src/test_duplicates.py
✅ Files skipped from review due to trivial changes (1)
- request-processor/requirements/test_requirements.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- request-processor/tests/unit/src/test_duplicates.py
…k_pipeline function
…d mandatory_fields parameters
…es and related tests
2 participants
What type of PR is this? (check all applicable)
Description
Adds duplicate entity redirection support for spatial add-data requests. The request processor now identifies potential duplicate conservation-area entities by comparing newly transformed provision entities against existing platform entities using geometry/point matching, then includes
duplicate-candidatesin the add-data response for use in Assign Entities.This also adds partial-ratio name similarity using
rapidfuzz, installs Spatialite support in the processor image, and adds unit coverage for duplicate candidate generation, Datasette fetching, name similarity, and skip conditions.Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Run the duplicate candidate unit tests:
cd request-processor python3 -m pytest tests/unit/src/test_duplicates.pyAlso verify an add-data response for a conservation-area resource includes a
duplicate-candidatesarray when a new provision entity spatially matches an existing platform entity.Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
None.
[optional] Are there any dependencies on other PRs or Work?
None known.
Summary by CodeRabbit