Skip to content

fix: fetch every resource for the deleted entities expectation - #611

Merged
tombrooks248 merged 1 commit into
mainfrom
fix/deleted-entities-resource-fetch
Oct 19, 2026
Merged

tombrooks248 merged 1 commit into
mainfrom
fix/deleted-entities-resource-fetch

Conversation

@tombrooks248

@tombrooks248 tombrooks248 commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Fixes two very minor problems in how the count_deleted_entities expectation fetches each organisation's resources from Datasette which result in very slightly off expectation counts:

  • The active-resources query now ignores reporting_historic_endpoints rows with no resource (log entries for days an endpoint returned nothing).
  • Both queries now page through the results 1,000 rows at a time, with a stable order by, instead of making one request. The retry loop has moved into _read_datasette_csv so that each page is retried on its own.

Why

Blank active resources. The active query keeps rows where resource_end_date is blank or null. Log rows with no resource have a null resource_end_date, so they pass that filter. group by o.entity, rhe.endpoint then picks an arbitrary row per endpoint. When the pick is a blank row, the organisation's active resource is lost, and every entity it supplies is counted as deleted. In prod on 17 Oct this failed central-activities-zone for LBH (entity 2200001), although its resource b7cd7941… is still active. In the same night's results, organisations with a blank in their own active list made up 9 failing rows in conservation-area, 3 in article-4-direction-area and 2 in tree-preservation-zone. Which row gets picked varies between runs, so the failures come and go.

Truncated historic resources. Datasette returns at most 1,000 rows (max_returned_rows) and gives no error when it cuts a result short. The dataset-wide historic query is over that for listed-building-outline (2,857 rows), conservation-area (2,451), tree-preservation-zone (2,821) and article-4-direction-area (3,321). Organisations whose rows fall past the cut get an empty historic list, so the check silently passes them with 0. Adding _stream=on doesn't help: Datasette only streams table and view exports, not custom SQL.

Related Tickets & Documents

  • Ticket Link
  • Related Issue #
  • Closes #

QA Instructions, Screenshots, Recordings

Ran the dataset checkpoint locally against live prod Datasette, using the prod conservation-area sqlite from 17 Oct 01:22Z, with main and with this branch:

main this branch
active resources fetched 390 (79 blank) 313 (0 blank)
historic resources fetched 1,000 2,451
organisations failing count_deleted_entities 10 15
  • The five newly failing organisations (MDW, NED, SWK, WAE, national-park-authority:Q72617158) all returned 0 on main because their resources were past the 1,000-row cut. I traced NED: its own resource ended in December 2025 and its entities are missing from its active resources, so it fails under the expectation's existing definition.
  • listed-building-outline fetches went from 122 active (24 blank) to 98 active, and from 1,000 historic to 2,857.
  • Each page takes under 0.2 s in prod.
  • Expect more count_deleted_entities notice tasks after merge, because organisations that were silently skipped now get checked. They are expectation tasks, so they stay off LPA dashboards.
  • DLP main is installed at runtime by the collection tasks, so this goes live on the next nightly run with no publish step.

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.

  • Yes
  • No, and this is why: please replace this line with details on why tests
    have not been included
  • I need help with writing tests

[optional] Are there any post deployment tasks we need to perform?

[optional] Are there any dependencies on other PRs or Work?

Summary by CodeRabbit

  • Bug Fixes
    • Resource results now exclude entries without a resource value and appear in a consistent order.
    • Large resource lists are now fully retrieved, including results beyond the previous 1,000-row limit.

@coderabbitai

coderabbitai Bot commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 58ad3b88-e2c3-4cc9-912a-92d3e9e9ae9b
📥 Commits

Reviewing files that changed from the base of the PR and between 7ec8cbc and b63a618.

📒 Files selected for processing: 2
  • digital_land/expectations/operations/dataset.py
  • tests/integration/expectations/operations/test_dataset.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Dataset resource retrieval now filters and orders query results. It fetches results in pages of up to 1,000 rows, combines the pages, and builds the organisation-to-resources cache. Integration tests cover empty active resources and retrieval of 2,500 resources.

Changes

Dataset resource retrieval

Layer / File(s) Summary
Resource query and CSV reader
digital_land/expectations/operations/dataset.py
The active-resource query excludes empty resources and orders by organisation and endpoint. The historical-resource query orders by organisation and resource. _read_datasette_csv returns the CSV data directly.
Paged retrieval and cache construction
digital_land/expectations/operations/dataset.py, tests/integration/expectations/operations/test_dataset.py
_fetch_resources_for_dataset retrieves pages of up to 1,000 rows, combines them, and builds the cache. Integration tests check the empty-resource filter and retrieval of all 2,500 resources across pages.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Fetch as _fetch_resources_for_dataset
  participant Reader as _read_datasette_csv
  participant Pandas as pandas.read_csv
  participant Datasette
  loop Until a page has fewer than 1,000 rows
    Fetch->>Reader: Query URL with page limit and offset
    Reader->>Pandas: Read CSV from query URL
    Pandas->>Datasette: Request CSV page
    Datasette-->>Pandas: Return CSV page
    Pandas-->>Reader: Return page data
    Reader-->>Fetch: Return page data
  end
  Fetch->>Fetch: Combine pages and build cache
Loading

Suggested reviewers: sianteesdale

Merge Risk: ⚪ Minimal · up to b63a6

The changed path retrieves resources beyond the previous 1,000-row limit. No actionable merge-blocking risk was established; merge after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly summarises the main change: fetching every resource for the deleted-entities expectation.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
  • Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@tombrooks248
tombrooks248 marked this pull request as ready for review October 19, 2026 08:22

@sianteesdale sianteesdale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Tom!

@tombrooks248
tombrooks248 merged commit f761751 into main Oct 19, 2026
9 checks passed
@tombrooks248
tombrooks248 deleted the fix/deleted-entities-resource-fetch branch October 19, 2026 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants