Change the /data view to load data from the platform API instead of from datasette #1133
Change the /data view to load data from the platform API instead of from datasette #1133pooleycodes merged 6 commits intomainfrom
Conversation
WalkthroughThis PR introduces platform API support for entity fetching by adding a new service module and middleware, while modifying the middleware stack to route requests to either datasette or platform based on configuration. It also adjusts schema validation permissiveness, updates dataset slug mapping initialisation to be awaitable, and enhances test utilities and UI navigation handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as Middleware Stack
participant MwBuilder as Middleware Builder
participant DataSource as Data Source
Note over Client,DataSource: Previous Flow: Datasette Only
Client->>Middleware: Request with dataset
Middleware->>MwBuilder: fetchEntities (datasette)
MwBuilder->>DataSource: datasette.runQuery()
DataSource-->>MwBuilder: Results
MwBuilder-->>Middleware: req.entities
Middleware-->>Client: Response
Note over Client,DataSource: New Flow: Platform Option
Client->>Middleware: Request with dataset
Middleware->>MwBuilder: fetchEntitiesPlatformDb (FetchOptions.platformDb)
alt Dataset = 'platform'
MwBuilder->>DataSource: platformApi.fetchEntities()
DataSource->>DataSource: REST GET /entity.json?params
DataSource-->>MwBuilder: { data, formattedData }
else Dataset = other
MwBuilder->>DataSource: datasette.runQuery()
DataSource-->>MwBuilder: Results
end
MwBuilder-->>Middleware: req.entities
Middleware-->>Client: Response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key areas requiring careful attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/middleware/dataview.middleware.js (1)
52-56: Remove unusedfetchEntitiesexport fromsrc/middleware/dataview.middleware.js(lines 52–56).The export is no longer used in this file's middleware chain (replaced by
fetchEntitiesPlatformDbon line 136) and is not imported elsewhere. The ripgrep and ast-grep searches found no imports of this specific export, indicating it is dead code.
🧹 Nitpick comments (2)
src/services/platformApi.js (2)
32-42: Add timeout and consider retry logic for external API calls.The axios request has no timeout configured, which could cause requests to hang indefinitely if the platform API is unresponsive. Additionally, there's no retry logic for transient failures.
Apply this diff to add a timeout:
- const response = await axios.get(url) + const response = await axios.get(url, { timeout: 10000 }) // 10 second timeoutConsider also implementing retry logic using a library like
axios-retryfor handling transient network failures, especially since this is a critical data source replacing datasette.
44-46: Usetypes.Externalfor external API logging.The error log uses
types.App, but since this is calling an external API (mainWebsiteUrl), it should usetypes.Externalfor consistency with logging conventions.Apply this diff:
logger.warn({ message: `platformApi.fetchEntities(): ${error.message}`, - type: types.App, + type: types.External, params, platformUrl: config.mainWebsiteUrl })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/middleware/dataview.middleware.js(2 hunks)src/middleware/middleware.builders.js(4 hunks)src/routes/schemas.js(1 hunks)src/services/platformApi.js(1 hunks)test/unit/services/platformApi.test.js(1 hunks)test/utils/mocker.js(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
test/unit/services/platformApi.test.jssrc/routes/schemas.js
📚 Learning: 2024-11-28T16:29:30.209Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 691
File: src/middleware/dataview.middleware.js:24-24
Timestamp: 2024-11-28T16:29:30.209Z
Learning: In `src/middleware/dataview.middleware.js`, when querying the database, interpolating variables directly into SQL queries is acceptable as the variables are sourced from the database.
Applied to files:
src/middleware/dataview.middleware.js
🧬 Code graph analysis (3)
src/services/platformApi.js (2)
src/utils/logger.js (1)
logger(21-28)src/utils/logging.js (1)
types(9-19)
src/middleware/dataview.middleware.js (1)
src/middleware/middleware.builders.js (3)
fetchMany(317-319)FetchOptions(33-46)FetchOptions(33-46)
src/middleware/middleware.builders.js (2)
src/middleware/common.middleware.js (20)
req(121-121)req(122-122)req(141-141)req(142-142)req(181-181)req(222-222)req(261-261)req(286-286)req(299-299)req(378-378)req(439-439)req(509-509)req(567-567)req(574-574)req(789-789)req(800-800)result(446-446)query(351-351)datasette(340-345)datasette(352-352)src/services/performanceDbApi.js (1)
result(483-483)
🔇 Additional comments (5)
test/utils/mocker.js (1)
6-6: LGTM! Correct support for looseObject schemas.The addition of
objectimport and theloose_objectcustom schema conversion correctly enables the mock data generator to handlelooseObjectschemas introduced in the codebase.Also applies to: 16-18
src/middleware/middleware.builders.js (1)
17-17: LGTM! Platform DB integration properly structured.The new
platformDbsymbol inFetchOptionsand correspondingdatasetOverridehandling correctly establish the platform-backed data source path.Also applies to: 41-45, 60-61
src/middleware/dataview.middleware.js (1)
35-50: LGTM! Platform DB middleware properly implemented.The new
fetchEntitiesPlatformDbmiddleware correctly:
- Uses
FetchOptions.platformDbto route through the platform API- Constructs a query object (not SQL) with the required parameters
- Includes clear documentation explaining the REST API usage
test/unit/services/platformApi.test.js (1)
1-157: LGTM! Comprehensive test coverage for platformApi.The test suite provides excellent coverage of the
fetchEntitiesfunction:
- URL construction with various parameter combinations
- Return value structure (data and formattedData)
- Edge cases (empty entities, missing entities field)
- Pagination behaviour
- Error propagation
The tests are well-structured, use proper mocking, and follow consistent patterns.
src/routes/schemas.js (1)
110-116: Verify the documentation of additional properties added to IssueSpecification objects by middleware.The change to
looseObjectwas made to fix a field mapping bug (commit a9f3bdf). Investigation confirms thataddDatabaseFieldToSpecificationmiddleware adds properties to field objects (e.g. via spread operators like{ datasetField, ...fieldObj }), whichstrictObjectwould reject. However, this validation now accepts any arbitrary properties.Consider documenting which additional properties middleware may add to field objects, or explicitly define them in the schema using
v.object()with additional properties specified. This maintains the flexibility needed for the fix whilst preserving validation clarity.
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/middleware/middleware.builders.js (1)
142-155: Platform branch infetchManyFnis clear; consider tightening query validation and updating docs.The split between Datasette and the platform API is straightforward, and the object/array guard is a good baseline. Two refinements would make this more robust and self-documenting:
- Align validation with the error message – you currently only check that
queryis a non-array object, but the error mentions{ organisation_entity, dataset, limit, offset }. Either validate the required keys (at leastorganisation_entityanddataset, optionallylimit/offset) or soften the message so it doesn’t imply field-level checks.- Update the JSDoc/comment for
fetchManyFn– it still describes fetching “from datasette”, but the middleware can now call either Datasette or the platform API. Refreshing the wording will help future readers understand the dual data sources.Example refinement for the key validation and error message (behaviour-preserving aside from earlier, clearer failures):
- if (database === 'platform') { - if (!query || typeof query !== 'object' || Array.isArray(query)) { - throw new Error(`Platform DB queries require an object with { organisation_entity, dataset, limit, offset }, received: ${typeof query}`) - } - result = await platformApi.fetchEntities(query) + if (database === 'platform') { + if (!query || typeof query !== 'object' || Array.isArray(query)) { + throw new Error(`Platform DB queries require a plain object, received: ${typeof query}`) + } + + const requiredKeys = ['organisation_entity', 'dataset'] + const missingKeys = requiredKeys.filter((key) => !(key in query)) + if (missingKeys.length) { + throw new Error(`Platform DB queries require keys ${requiredKeys.join(', ')}, missing: ${missingKeys.join(', ')}`) + } + + result = await platformApi.fetchEntities(query)These changes keep the overall control flow the same while making failures easier to diagnose.
🧹 Nitpick comments (1)
src/middleware/middleware.builders.js (1)
60-61:datasetOverridemapping for the platform DB looks correct; consider centralising dataset names.Returning
'platform'here aligns with thedatabase === 'platform'check infetchManyFn, so the routing is coherent. If you expect more platform-related logic, it may be worth introducing a shared constant (e.g.const PLATFORM_DATASET = 'platform') to avoid string duplication betweendatasetOverrideandfetchManyFn.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/middleware/middleware.builders.js(4 hunks)src/services/platformApi.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/platformApi.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
src/middleware/middleware.builders.js
🧬 Code graph analysis (1)
src/middleware/middleware.builders.js (2)
src/middleware/common.middleware.js (20)
req(121-121)req(122-122)req(141-141)req(142-142)req(181-181)req(222-222)req(261-261)req(286-286)req(299-299)req(378-378)req(439-439)req(509-509)req(567-567)req(574-574)req(789-789)req(800-800)result(446-446)query(351-351)datasette(340-345)datasette(352-352)src/services/performanceDbApi.js (1)
result(483-483)
🔇 Additional comments (2)
src/middleware/middleware.builders.js (2)
17-17: Platform API import is correctly wired.The import path and naming are consistent with the existing service imports and match the later usage in
fetchManyFn, so this addition looks sound.
41-45: NewFetchOptions.platformDbentry is consistent with existing options.The new symbol cleanly extends the enum-style
FetchOptionswithout changing existing behaviour and gives a clear hook for platform-backed fetches.
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/unit/services/platformApi.test.js (1)
149-159: LGTM! Consider additional error scenarios (optional).The error handling test correctly verifies that API errors propagate to the caller. As an optional enhancement, you might consider adding test cases for:
- Timeout errors specifically
- HTTP error responses (e.g., 404, 500)
- Invalid response formats
However, the current coverage is sufficient for the essential error path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/services/platformApi.test.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-14T16:38:49.883Z
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
Applied to files:
test/unit/services/platformApi.test.js
🔇 Additional comments (6)
test/unit/services/platformApi.test.js (6)
1-15: LGTM!The test setup correctly isolates the platformApi service with proper mocks for axios and the configuration module. The
beforeEachhook ensures test independence by clearing mocks between tests.
17-48: LGTM!This test correctly verifies that all parameters (organisation_entity, dataset, limit, offset) are properly encoded in the URL query string and that the return shape matches the expected pattern with both
dataandformattedDatafields.
50-77: LGTM!This test correctly verifies the minimal required parameter set, ensuring that optional parameters (limit, offset) can be omitted without affecting the function's behaviour.
79-99: LGTM!This test provides good edge case coverage for empty API responses, ensuring
formattedDatacorrectly returns an empty array.
101-119: Excellent defensive programming!This test ensures the implementation handles malformed API responses gracefully when the
entitiesfield is missing, preventing potential runtime errors.
121-147: LGTM!This test correctly verifies pagination parameter handling, ensuring
limitandoffsetare properly encoded in the URL and that results are returned in the expected format.
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
…ed click and hang timeout error
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/PageObjectModels/statusPage.js (2)
55-55: Consider removing or replacing console.log statements.Using
console.login test code can clutter test output. Consider using Playwright'stest.info()for structured logging or remove these statements if they're only for debugging.Also applies to: 62-62
46-73: Verify the necessity of this change and consider improving error handling.This polling-based approach is more robust for handling asynchronous navigation, but raises several concerns:
Relevance: This test change appears unrelated to the PR's stated objective of migrating from Datasette to the platform API. Please confirm this change is intentional and explain its connection to the PR's goals.
Error handling: If the button never becomes visible or navigation doesn't occur after 60 attempts, the final
waitForURLon line 70 has only a 5-second timeout. Consider either:
- Increasing the final timeout since 60 seconds have already elapsed
- Adding explicit error messaging to help diagnose why navigation failed
Silent failures: If
isVisible()returns false for all 60 attempts, there's no indication of what went wrong before the final timeout.Consider adding logging or assertions to help diagnose failures, and verify this change belongs in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/PageObjectModels/statusPage.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: rosado
Repo: digital-land/submit PR: 657
File: test/unit/middleware/issueDetails.middleware.test.js:43-43
Timestamp: 2024-11-14T16:38:49.883Z
Learning: In `test/unit/middleware/issueDetails.middleware.test.js`, template params are verified with a schema, so it's acceptable for the test expectations to use primitive values while the test input uses an object for `issueEntitiesCount`.
🧬 Code graph analysis (1)
test/PageObjectModels/statusPage.js (1)
test/PageObjectModels/resultsPage.js (1)
ResultsPage(9-80)
🪛 GitHub Actions: Code change pipeline to development by @Pooley3100
test/PageObjectModels/statusPage.js
[error] 49-49: Trailing spaces not allowed. (no-trailing-spaces)
[error] 52-52: Trailing spaces not allowed. (no-trailing-spaces)
[error] 58-58: Trailing spaces not allowed. (no-trailing-spaces)
[error] 64-64: Trailing spaces not allowed. (no-trailing-spaces)
[error] 68-68: Trailing spaces not allowed. (no-trailing-spaces)
[error] 71-71: Trailing spaces not allowed. (no-trailing-spaces)
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
66-69: Consider removing debug step once troubleshooting is complete.This debug step appears to be for temporary troubleshooting. If the acceptance test issues have been resolved, consider removing it to keep the workflow clean. Alternatively, if ongoing debugging is needed, consider adding a comment explaining its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/test.yml(1 hunks)index.js(1 hunks)src/utils/datasetSlugToReadableName.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-29T14:58:22.843Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 593
File: index.js:20-20
Timestamp: 2024-10-29T14:58:22.843Z
Learning: The function `initDatasetSlugToReadableNameFilter()` in `src/utils/datasetSlugToReadableName.js` already includes appropriate error handling and logging.
Applied to files:
src/utils/datasetSlugToReadableName.jsindex.js
📚 Learning: 2024-10-29T15:06:42.432Z
Learnt from: GeorgeGoodall-GovUk
Repo: digital-land/submit PR: 593
File: test/unit/lpaDetailsController.test.js:20-20
Timestamp: 2024-10-29T15:06:42.432Z
Learning: The function `initDatasetSlugToReadableNameFilter()` includes its own error handling, so additional try-catch blocks around it in test files are unnecessary.
Applied to files:
src/utils/datasetSlugToReadableName.jsindex.js
🧬 Code graph analysis (2)
src/utils/datasetSlugToReadableName.js (1)
index.js (1)
datasetSlugToReadableName(19-19)
index.js (2)
src/utils/datasetSlugToReadableName.js (2)
datasetSlugToReadableName(4-4)initDatasetSlugToReadableNameFilter(6-15)src/serverSetup/nunjucks.js (1)
setupNunjucks(34-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests / test
🔇 Additional comments (3)
src/utils/datasetSlugToReadableName.js (1)
6-15: LGTM! Returning the initialised function improves usability.The addition of the return statement enables callers to directly access the initialised
datasetSlugToReadableNamefunction, which is now used inindex.js. The existing error handling remains appropriate.Based on learnings
index.js (2)
19-19: LGTM! Top-level await ensures dataset mapping is ready before app setup.The change to await
initDatasetSlugToReadableNameFilterand store its result simplifies the initialisation flow and ensures the dataset mapping is ready before configuring Nunjucks. The error handling is already present in the initialisation function.Based on learnings
24-27: LGTM! Consistent usage of the initialised dataset mapping.Passing the pre-initialised
datasetSlugToReadableNameasdatasetNameMappingis consistent with the changes insrc/utils/datasetSlugToReadableName.jsand eliminates the need for a separate mapping fetch.
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
had a problem deploying
to
development
GitHub Actions
Error
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
force-pushed
the
spike-entities-platform
branch
from
b70f823 to
025d082
Compare
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
had a problem deploying
to
development
GitHub Actions
Failure
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
pooleycodes
dismissed
eveleighoj’s stale review
The merge-base changed after approval.
|
Merged in with PR 1136 |
pooleycodes
temporarily deployed
to
development
GitHub Actions
Inactive
|
A few edits around test criteria and slug fallback were not merged so reopen and merge now |
2 participants
Description
There is a long term goal to move more of the requests in this service to the platform API (planning.data.gov.uk) instead of from datasette, this is not only much faster but also data that represents what the service currently shows. This PR introduces a platformAPI.js file that allows querying to the planning.data.gov.uk/entiy.json endpoint.
Changes are also made to the fetchEntities in /data (with fetchEntitiesPlatformDb) function to now pull data in from platform instead of SQL, some adjustments are made to allow this while still working with the service structured around data being queried by SQL instead of REST API objects.
What type of PR is this? (check all applicable)
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
When comparing the data view in Doncaster Prod to Doncaster Dev the same data should be shown. (Not always the case i.e. if datasette is behind what is on planning platform)
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor
Schema
Tests
✏️ Tip: You can customize this high-level summary in your review settings.