Refactor location detail endpoints to use website_data cache - #196
Refactor location detail endpoints to use website_data cache#196jbeard4 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Not safe to approve yet: the new website_data cache introduces behavior changes and stale-data failure modes that can break API correctness.
Blocking issues
getInfoBySlugno longer does exact slug matching; it matches the last path segment via SQLsplit_part(...). This can return the wrong location when multiple slugs share the same leaf, which is a broken API behavior regression from the previous exactwhere: { slug }lookup.- Cached reads (
getWebsiteDataByLocationId/getWebsiteDataBySlug) trustwebsite_datarows without validating source record existence/freshness. If upstream records are deleted or cache rows become stale, endpoints can serve outdated data instead of 404/current state. parseWebsiteDataRowdoes rawJSON.parsewith no guard/rebuild path. A single malformedwebsite_data.datavalue will throw and fail requests (500) rather than recovering by regenerating data, creating an avoidable availability regression.- No tests are included for this risky cache layer (slug lookup semantics, cache invalidation/refresh on mutations/deletes, malformed cache payload handling). Given the API-path refactor, missing regression coverage is blocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 980f1ddfce
ℹ️ 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".
| if (websiteData) { | ||
| return parseWebsiteDataRow(websiteData); |
There was a problem hiding this comment.
Invalidate cache when organization data changes
This read path returns cached JSON unconditionally once a website_data row exists, but the commit only refreshes cache rows from locations/services mutation handlers. Organization edits (for example PATCH /organizations/:organizationId in src/controllers/organizations.js) update fields that are embedded in the location payload (organization name and derived slug), so after one cached read, GET /locations/:locationId can keep serving stale organization/slug data indefinitely.
Useful? React with 👍 / 👎.
| throw new NotFoundError('Location not found'); | ||
| } | ||
|
|
||
| return buildLocationResponse(location, locationWithServices, false); |
There was a problem hiding this comment.
Keep slug endpoint payload in its legacy format
The cache builder hard-codes excludeMetadata to false, and getInfoBySlug now serves this cached payload too. Previously /locations-by-slug/:slug used the excludeMetadata path (returning lastValidatedDateForLocation and omitting metadata blobs), so this change silently alters the response contract for slug lookups and can break clients that rely on the old schema.
Useful? React with 👍 / 👎.
Motivation
website_datatable.GET /locations/:locationIdandGET /locations-by-slug/:slugcan be fast and avoid expensive queries.Description
WebsiteDataSequelize model atsrc/models/website-data.jsto represent the existingwebsite_datatable.src/services/location-website-data.jswhich centralizes building the full location detail payload, parsing/storing JSON intowebsite_data,getWebsiteDataByLocationId,getWebsiteDataBySlug,upsertWebsiteDataForLocation, andupsertWebsiteDataForService(which refreshes all locations affected by a service change).GET /locations/:locationIdandGET /locations-by-slug/:slug(insrc/controllers/locations.js) to usegetWebsiteDataByLocationIdandgetWebsiteDataBySlugrespectively, with lazy backfill when cache rows are missing.locations.create/update,locations.addPhone/updatePhone/deletePhone, andservices.create/update/deletenow call the appropriateupsertWebsiteData*functions, and slug lookup in cache supports matching the final slash-delimited segment.Testing
npx eslint src/controllers/locations.js src/controllers/services.js src/services/location-website-data.js src/models/website-data.jsand the lint check for those modified files completed successfully.npm test -- --runInBandand it failed in this environment due to the Node test runtime missing thefetchshim required by the installedopenaipackage (not related to the location cache changes).Codex Task