feat: handle multiple organisation per resource#577
Conversation
tombrooks248
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe changes partition duplicate-reference detection by entity and add organisation-aware entity resolution, including multi-organisation fan-out, deduplication, issue logging, pipeline wiring, and unit tests. Duplicate reference partitioning
Organisation-aware entity lookup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Pipeline
participant EntityLookupPhase
participant LookupPhase
Pipeline->>EntityLookupPhase: pass configured organisations
EntityLookupPhase->>LookupPhase: resolve each block for each organisation
LookupPhase-->>EntityLookupPhase: return entity or missing reference
EntityLookupPhase-->>Pipeline: emit deduplicated organisation-stamped blocks
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (2)
digital_land/phase/lookup.py (2)
219-223: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable default arguments flagged by Ruff B006.
lookups={},redirect_lookups={}, andentity_range=[]are mutable defaults. This follows the existing pattern inLookupPhase.__init__, but the static analyser flags it. Consider usingNoneand initialising inside the body if you want to address the lint warnings.🤖 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 `@digital_land/phase/lookup.py` around lines 219 - 223, Update the affected initializer signature, including LookupPhase.__init__, to replace mutable defaults for lookups, redirect_lookups, and entity_range with None. Initialize each value inside the function body while preserving the existing behavior and avoiding shared mutable state.Source: Linters/SAST tools
184-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
_log_unknown_entityinLookupPhase.processto eliminate duplication.The newly added
_log_unknown_entitymethod faithfully extracts the inline logging at lines 184–206 but is only called from_process_multi_org. RefactoringLookupPhase.processto call it as well would remove ~20 lines of duplicated logic and ensure both paths stay in sync.♻️ Proposed refactor
if prefix: if not row.get(self.entity_field, ""): row[self.entity_field] = self.get_entity(block) if not row[self.entity_field]: - if self.issues: - if not reference: - self.issues.log_issue( - "entity", - "unknown entity - missing reference", - curie, - line_number=line_number, - ) - else: - self.issues.log_issue( - "entity", - "unknown entity", - curie, - line_number=line_number, - ) - if self.operational_issues: - self.operational_issues.log_issue( - "entity", - "unknown entity", - curie, - line_number=line_number, - ) + self._log_unknown_entity(reference, curie, line_number) else: row[self.entity_field] = self.redirect_entity( row[self.entity_field] )🤖 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 `@digital_land/phase/lookup.py` around lines 184 - 206, Update LookupPhase.process to replace the duplicated missing/unknown entity issue-logging block with a call to the existing _log_unknown_entity method, passing the same relevant values such as curie, reference, and line_number. Preserve the current behavior for missing references and operational_issues while ensuring both process paths use the shared helper.
🤖 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 `@digital_land/check.py`:
- Around line 25-29: Update the duplicate-processing flow after the query
grouped by "entity" so the subsequent issues.log_issue(...) call passes
entity=row["entity"]. Preserve the existing issue fields and use the
IssueLog.log_issue entity parameter to retain entity attribution.
In `@digital_land/phase/lookup.py`:
- Around line 247-249: Update the single-organization call to
issues.record_entity_map in LookupPhase.process to access the entity value
defensively with the same row.get(self.entity_field, "") pattern used by the
multi-organization path, preserving an empty-string fallback when the field is
absent.
- Around line 289-290: Update the issue-recording flow in apply_entity_map to
preserve the resolved entity or organisation directly on each issue row. Do not
use issues.record_entity_map with the shared entry-number map for
multi-organisation entries; ensure each issue receives its own attribution so
later issues for the same entry-number are not overwritten by the first entity.
---
Nitpick comments:
In `@digital_land/phase/lookup.py`:
- Around line 219-223: Update the affected initializer signature, including
LookupPhase.__init__, to replace mutable defaults for lookups, redirect_lookups,
and entity_range with None. Initialize each value inside the function body while
preserving the existing behavior and avoiding shared mutable state.
- Around line 184-206: Update LookupPhase.process to replace the duplicated
missing/unknown entity issue-logging block with a call to the existing
_log_unknown_entity method, passing the same relevant values such as curie,
reference, and line_number. Preserve the current behavior for missing references
and operational_issues while ensuring both process paths use the shared helper.
🪄 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: 41b2cdb5-7171-4c8c-b9c4-6a5c614b0642
📒 Files selected for processing (5)
digital_land/check.pydigital_land/phase/lookup.pydigital_land/pipeline/main.pytests/unit/phase/test_lookup.pytests/unit/test_check.py
6edfdd6 to
b2c9449
Compare
b2c9449 to
11c327b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
digital_land/phase/lookup.py (1)
247-252: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFix potential
KeyErroronblock["row"]["entity"]in the single-organisation path.When
LookupPhase.processprocesses a row with noprefix, it yields the block without settingrow[self.entity_field]. Accessingblock["row"]["entity"]directly here will cause aKeyErrorand crash the pipeline.Use the same defensive access pattern
.get(self.entity_field, "")that is successfully used in the multi-organisation path.🔧 Proposed fix
for block in super().process(stream): if self.issues: self.issues.record_entity_map( - block["entry-number"], block["row"]["entity"] + block["entry-number"], block["row"].get(self.entity_field, "") ) yield block🤖 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 `@digital_land/phase/lookup.py` around lines 247 - 252, Update the entity lookup in LookupPhase.process to use the row’s defensive .get(self.entity_field, "") access when recording the entity map, matching the multi-organisation path and allowing rows without a prefix to proceed without KeyError.
🧹 Nitpick comments (1)
digital_land/phase/lookup.py (1)
146-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
_log_unknown_entityto eliminate duplicated logic.This centralises the issue-logging perfectly. You can also reuse this new method inside
LookupPhase.processto eliminate the remaining duplicated logic.You can apply the following clean-up to
LookupPhase.process(around line 184):if not row[self.entity_field]: self._log_unknown_entity(reference, curie, line_number) else: row[self.entity_field] = self.redirect_entity( row[self.entity_field] )🤖 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 `@digital_land/phase/lookup.py` around lines 146 - 170, Update LookupPhase.process to call _log_unknown_entity(reference, curie, line_number) when the entity field is missing, replacing its duplicated issue-logging branch; retain the existing redirect_entity assignment for rows with an entity value.
🤖 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 `@digital_land/phase/lookup.py`:
- Around line 217-236: Update the __init__ method’s mutable parameters—lookups,
redirect_lookups, and entity_range—to default to None, then initialize each to a
fresh empty dictionary or list before passing them to super().__init__. Preserve
the existing organisations fallback and behavior for explicitly supplied values.
---
Duplicate comments:
In `@digital_land/phase/lookup.py`:
- Around line 247-252: Update the entity lookup in LookupPhase.process to use
the row’s defensive .get(self.entity_field, "") access when recording the entity
map, matching the multi-organisation path and allowing rows without a prefix to
proceed without KeyError.
---
Nitpick comments:
In `@digital_land/phase/lookup.py`:
- Around line 146-170: Update LookupPhase.process to call
_log_unknown_entity(reference, curie, line_number) when the entity field is
missing, replacing its duplicated issue-logging branch; retain the existing
redirect_entity assignment for rows with an entity value.
🪄 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: aa5f039a-83c7-4c38-bf4f-402852cdb9d1
📒 Files selected for processing (5)
digital_land/check.pydigital_land/phase/lookup.pydigital_land/pipeline/main.pytests/unit/phase/test_lookup.pytests/unit/test_check.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/test_check.py
- digital_land/pipeline/main.py
- digital_land/check.py
- tests/unit/phase/test_lookup.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
digital_land/phase/lookup.py (1)
198-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
_log_unknown_entityto remove duplicated logic.The newly introduced
_log_unknown_entityhelper (lines 159-184) duplicates this exact logic. You can use it here to keep the issue logging centralised and DRY.♻️ Proposed refactor
- if not row[self.entity_field]: - if self.issues: - if not reference: - self.issues.log_issue( - "entity", - "unknown entity - missing reference", - curie, - line_number=line_number, - ) - else: - self.issues.log_issue( - "entity", - "unknown entity", - curie, - line_number=line_number, - ) - if self.operational_issues: - self.operational_issues.log_issue( - "entity", - "unknown entity", - curie, - line_number=line_number, - ) + if not row[self.entity_field]: + self._log_unknown_entity(reference, curie, line_number)🤖 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 `@digital_land/phase/lookup.py` around lines 198 - 220, Replace the duplicated missing-reference and unknown-entity logging block in the row-processing flow with a call to the existing _log_unknown_entity helper. Pass the same entity identifier, reference, and line number so issue and operational-issue logging behavior remains unchanged.
🤖 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.
Nitpick comments:
In `@digital_land/phase/lookup.py`:
- Around line 198-220: Replace the duplicated missing-reference and
unknown-entity logging block in the row-processing flow with a call to the
existing _log_unknown_entity helper. Pass the same entity identifier, reference,
and line number so issue and operational-issue logging behavior remains
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62e67758-20c7-4056-ba63-4f5f7d44ddb1
📒 Files selected for processing (3)
digital_land/phase/lookup.pydigital_land/pipeline/main.pytests/unit/phase/test_lookup.py
🚧 Files skipped from review as they are similar to previous changes (2)
- digital_land/pipeline/main.py
- tests/unit/phase/test_lookup.py
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)
digital_land/phase/lookup.py (1)
159-184: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass the current entry number into unknown-entity logging. In
digital_land/phase/lookup.py#L159-L184and#L364-L367,_log_unknown_entity(...)falls back toIssueLog.entry_number, so the fan-out path can record the issue against the wrong entry. Threadblock["entry-number"]through and assert"entry-number": 1intests/unit/phase/test_lookup.py#L414-L438.🤖 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 `@digital_land/phase/lookup.py` around lines 159 - 184, Update _log_unknown_entity and its call site in digital_land/phase/lookup.py:159-184 and 364-367 to accept and pass the current block["entry-number"] explicitly, ensuring both issue logs use that entry instead of falling back to IssueLog.entry_number. In tests/unit/phase/test_lookup.py:435-438, assert the fan-out result records "entry-number": 1.
🧹 Nitpick comments (1)
tests/unit/phase/test_lookup.py (1)
435-438: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the unknown issue’s entry attribution.
Also verify
issues.rows[0]["entry-number"] == 1. This currently exposes whether the multi-provider path correctly propagates the block’s entry number when logging the issue.🤖 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 `@tests/unit/phase/test_lookup.py` around lines 435 - 438, Extend the assertions in the unknown-entity test to verify that the single issue row has an entry-number of 1, alongside the existing issue-type assertion. This should validate entry attribution for the multi-provider lookup path without changing the output or issue-count expectations.
🤖 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 `@digital_land/phase/lookup.py`:
- Around line 159-184: Update _log_unknown_entity and its call site in
digital_land/phase/lookup.py:159-184 and 364-367 to accept and pass the current
block["entry-number"] explicitly, ensuring both issue logs use that entry
instead of falling back to IssueLog.entry_number. In
tests/unit/phase/test_lookup.py:435-438, assert the fan-out result records
"entry-number": 1.
---
Nitpick comments:
In `@tests/unit/phase/test_lookup.py`:
- Around line 435-438: Extend the assertions in the unknown-entity test to
verify that the single issue row has an entry-number of 1, alongside the
existing issue-type assertion. This should validate entry attribution for the
multi-provider lookup path without changing the output or issue-count
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b574fcec-a820-4e78-9e1e-678e8ba2c718
📒 Files selected for processing (2)
digital_land/phase/lookup.pytests/unit/phase/test_lookup.py
2 participants
What type of PR is this? (check all applicable)
Description
Changing the way the pipeline generates entities from a resource in a situation where multiple endpoints from different organisations provide identical data and therefore produce the same resource.
Before - No entities are produced from resources with multiple orgs.
After - now generate entities per organisation per resource.
Why
Because we are currently missing entities in our data and this is causing a bunch of problems particularly for local-plan data.
How the bug works
A resource is identified only by the hash of its contents. When two organisations publish the same URL, they produce identical bytes, so the collection merges them into a single resource that carries both organisation codes.
To generate entities, the pipeline stamps a default organisation onto every row of a resource (most data files don't say which authority they belong to). But it only does this when there is exactly one organisation:
Worked example — South & Vale Joint Local Plan 2045
South Oxfordshire and Vale of White Horse jointly published one local plan timetable at a single URL:
https://www.southandvale.gov.uk/app/uploads/2026/06/Local-Plan-Timetable-CSV.csvIn
source.csvthat one endpoint was attributed to both authorities:local-authority:SOXlocal-authority:VALlookup.csvalready defines separate entities for each authority against the same references, for example:Joint-Local-Plan-2045-scoping-consultation-startBefore: the resource carries two organisations, the guard is skipped, and neither authority's entities are created — both show zero records supplied.
After: entity lookup resolves the reference once per organisation and emits a row per distinct entity, so both authorities get their entities.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Tested in DEV
DEV's collected
resource.csvhas 4 active multi-org resources (organisations column containing;). I tested by looking at this one:ae8f59326ef53ede176817221ff512c00d3bc860874e8b8add2fb84dbc06e1f5local-authority:ADU;local-authority:WOT(Adur + Worthing)ea98ea4d156ee47f…Adur and Worthing publish a single shared brownfield-land CSV.
Result — the fix works
local-authority:ADU— 23 entitieslocal-authority:WOT— 22 entitiespriority=2(authoritative)Before the change this file was empty — no organisation meant no entity, and every row was pruned. Both councils showed zero records supplied. Both councils' data is now created from the single shared resource.
Why this is safer than it first looks
Changing how resources map to organisations sounds like it touches a core assumption, but the pipeline is already entity-centric below the resource level — facts, dataset assembly and tasks are all keyed on the entity, not the resource. So "process a resource once per organisation" fits the existing model rather than fighting it: each organisation resolves to its own entity, and from that point on everything treats the two organisations exactly as if they'd come from separate resources.
The load-bearing detail is that a fact's identity is a hash of
entity : field : value, so two organisations producing identical rows still create distinct facts (different entities → different hashes). That's why nothing collapses or overwrites. I also confirmed the resource-level logs are set, not accumulated, so nothing double-counts.The change is also naturally bounded: resources with one organisation (or none) follow exactly the same path as before — only resources with two or more organisations hit the new behaviour. Which is a relatively small subset of the total data.
Impact on issues generated
an unknown-entity issue is raised only when no organisation resolves an entity
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Summary of changes
New Features
Bug Fixes
Documentation
Tests