Fixed lazyRouting URLs 404'ing across the codebase#27938
Conversation
|
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:
WalkthroughThis PR extends lazy routing URL resolution by systematically enforcing tags and authors relation loading across serializers, endpoints, and services. A new forceUrlRelationsWhenLazy helper in both posts and pages serializers augments withRelated when lazy routing is active and URL is requested. API endpoints (email-post, search-index, search-index-public) and services (comments, batch-sending, email-controller, event-repository, email-renderer, mentions) are updated to load these relations during post/link queries. LazyUrlService gains a new _warnIfThin detection helper that warns when a resource lacks tags/authors relations but a router filter requires them. Tests validate serializer behavior under lazyRouting. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (4)
ghost/core/test/unit/api/canary/utils/serializers/input/posts.test.js (1)
142-258: ⚡ Quick winExtend this lazyRouting suite with
read()assertions.The new cases thoroughly cover
browse(), butread()now also forces URL relations in the serializer. Adding equivalentread()tests (admin/content withcolumnscontainingurl) would close that gap.🤖 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 `@ghost/core/test/unit/api/canary/utils/serializers/input/posts.test.js` around lines 142 - 258, Add mirror tests for serializers.input.posts.read matching the existing lazyRouting + thin columns browse suite: call serializers.input.posts.read(apiConfig, frame) for admin and content api frames (use frame.options.columns including 'url' and also a case omitting 'url'), set configUtils.set('lazyRouting', true/false as in the browse tests), and assert frame.options.withRelated is set and includes 'tags' and 'authors' when columns include 'url', assert it remains undefined when columns omit 'url' or lazyRouting is false, and add a test that when frame.options.withRelated already contains ['email'] the read() call preserves 'email' while merging in 'tags' and 'authors'.ghost/core/test/unit/api/canary/utils/serializers/input/pages.test.js (1)
77-130: ⚡ Quick winAdd
read()coverage for lazyRouting thin-column URL behavior.This suite validates
browse()well, but the serializer change also addsforceUrlRelationsWhenLazyinread(). Please mirror one admin and one contentread()case forcolumns: ['url', ...]to lock both entry points.🤖 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 `@ghost/core/test/unit/api/canary/utils/serializers/input/pages.test.js` around lines 77 - 130, Tests only cover browse(); add equivalent read() tests to cover forceUrlRelationsWhenLazy behavior: add one admin read test and one content read test that set configUtils.set('lazyRouting', true), construct a frame with apiType 'admin' (context {user:1}) and columns ['id','url',... ] and another with apiType 'content' (context {api_key:{id:1,type:'content'}}) and columns ['id','url'], call serializers.input.pages.read({}, frame) and assert frame.options.withRelated exists and includes 'tags' and 'authors'; mirror the existing browse() assertions to lock the read() entry point affected by forceUrlRelationsWhenLazy.ghost/core/test/unit/server/services/url/lazy-url-service.test.js (1)
164-223: ⚡ Quick winConsider adding edge case coverage for filter detection.
The current test suite covers the core scenarios well, but could be strengthened with tests for:
primary_tagandprimary_authorkeywords in filters (e.g.,filter: 'primary_tag:news')- Combined filters (e.g.,
filter: 'tag:news+author:jane') to verify both missing relations are reported- Verification that the router identifier appears in the warning message
- Different keyword forms:
tagvstags,authorvsauthorsThese tests would provide confidence that the regex patterns in
_warnIfThin(lines 160-161 of lazy-url-service.ts) correctly detect all supported NQL syntax variations.📋 Example test cases
it('warns when a primary_tag-filtered router is evaluated against a resource with no tags', function () { const service = new LazyUrlService({urlUtils}); service.onRouterAddedType('news', 'primary_tag:news', 'posts', '/:slug/'); service.getUrlForResource({type: 'posts', id: 'p1', slug: 'hello'}); sinon.assert.calledOnce(warnStub); assert.match(warnStub.firstCall.args[0], /primary_tag:news/); }); it('warns when a combined tag+author-filtered router evaluates a thin resource', function () { const service = new LazyUrlService({urlUtils}); service.onRouterAddedType('combo', 'tag:news+author:jane', 'posts', '/:slug/'); service.getUrlForResource({type: 'posts', id: 'p1', slug: 'hello'}); sinon.assert.calledOnce(warnStub); const msg = warnStub.firstCall.args[0]; assert.match(msg, /tags/); assert.match(msg, /authors/); }); it('includes the router identifier in the warning message', function () { const service = new LazyUrlService({urlUtils}); service.onRouterAddedType('my-custom-router', 'tag:news', 'posts', '/:slug/'); service.getUrlForResource({type: 'posts', id: 'p1', slug: 'hello'}); sinon.assert.calledOnce(warnStub); assert.match(warnStub.firstCall.args[0], /my-custom-router/); });🤖 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 `@ghost/core/test/unit/server/services/url/lazy-url-service.test.js` around lines 164 - 223, Tests are missing edge cases for filter detection used by LazyUrlService._warnIfThin; add unit tests in lazy-url-service.test.js that use onRouterAddedType and getUrlForResource to cover primary_tag/primary_author forms, combined filters like "tag:news+author:jane", plural/singular keyword variants (tag/tags, author/authors) and asserting the warnStub message contains the router identifier and mentions each missing relation (e.g., both "tags" and "authors") so the regexes in _warnIfThin are validated for all NQL variants.ghost/core/core/server/services/url/lazy-url-service.ts (1)
160-161: 💤 Low valueConsider edge cases in filter detection.
The regex patterns correctly detect tag/author keywords using word boundaries, but may not catch all NQL syntax variations. For example:
- Nested expressions:
(tag:news+featured:true),tag:updates- Negations:
-tag:news(currently warns, which is acceptable but could be refined)- Complex compound filters with parentheses
The current conservative approach (warn when keywords are present regardless of context) is safe and aligns with the stated goal, but consider documenting these edge cases if operators report false positives.
🤖 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 `@ghost/core/core/server/services/url/lazy-url-service.ts` around lines 160 - 161, The current detection using needsTags and needsAuthors (which test config.filter with /\btags?\b|\bprimary_tag\b/ and /\bauthors?\b|\bprimary_author\b/) can miss NQL forms like nested expressions, colon-operators, negations and parenthesized compounds; update the detection to also look for operator forms (e.g. token patterns like "tag:" or "author:") and common NQL punctuation (parentheses, plus/minus) so tests catch "tag:news", "-tag:news", and "(tag:news+featured:true)" variants, and add an explanatory comment next to the needsTags/needsAuthors definitions documenting remaining edge-case limitations and the rationale for conservative warnings.
🤖 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 `@ghost/core/core/server/services/url/lazy-url-service.ts`:
- Around line 160-161: The current detection using needsTags and needsAuthors
(which test config.filter with /\btags?\b|\bprimary_tag\b/ and
/\bauthors?\b|\bprimary_author\b/) can miss NQL forms like nested expressions,
colon-operators, negations and parenthesized compounds; update the detection to
also look for operator forms (e.g. token patterns like "tag:" or "author:") and
common NQL punctuation (parentheses, plus/minus) so tests catch "tag:news",
"-tag:news", and "(tag:news+featured:true)" variants, and add an explanatory
comment next to the needsTags/needsAuthors definitions documenting remaining
edge-case limitations and the rationale for conservative warnings.
In `@ghost/core/test/unit/api/canary/utils/serializers/input/pages.test.js`:
- Around line 77-130: Tests only cover browse(); add equivalent read() tests to
cover forceUrlRelationsWhenLazy behavior: add one admin read test and one
content read test that set configUtils.set('lazyRouting', true), construct a
frame with apiType 'admin' (context {user:1}) and columns ['id','url',... ] and
another with apiType 'content' (context {api_key:{id:1,type:'content'}}) and
columns ['id','url'], call serializers.input.pages.read({}, frame) and assert
frame.options.withRelated exists and includes 'tags' and 'authors'; mirror the
existing browse() assertions to lock the read() entry point affected by
forceUrlRelationsWhenLazy.
In `@ghost/core/test/unit/api/canary/utils/serializers/input/posts.test.js`:
- Around line 142-258: Add mirror tests for serializers.input.posts.read
matching the existing lazyRouting + thin columns browse suite: call
serializers.input.posts.read(apiConfig, frame) for admin and content api frames
(use frame.options.columns including 'url' and also a case omitting 'url'), set
configUtils.set('lazyRouting', true/false as in the browse tests), and assert
frame.options.withRelated is set and includes 'tags' and 'authors' when columns
include 'url', assert it remains undefined when columns omit 'url' or
lazyRouting is false, and add a test that when frame.options.withRelated already
contains ['email'] the read() call preserves 'email' while merging in 'tags' and
'authors'.
In `@ghost/core/test/unit/server/services/url/lazy-url-service.test.js`:
- Around line 164-223: Tests are missing edge cases for filter detection used by
LazyUrlService._warnIfThin; add unit tests in lazy-url-service.test.js that use
onRouterAddedType and getUrlForResource to cover primary_tag/primary_author
forms, combined filters like "tag:news+author:jane", plural/singular keyword
variants (tag/tags, author/authors) and asserting the warnStub message contains
the router identifier and mentions each missing relation (e.g., both "tags" and
"authors") so the regexes in _warnIfThin are validated for all NQL variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c3bc2be4-a9d9-4083-8e91-e369936b4578
📒 Files selected for processing (20)
ghost/core/core/server/api/endpoints/email-post.jsghost/core/core/server/api/endpoints/search-index-public.jsghost/core/core/server/api/endpoints/search-index.jsghost/core/core/server/api/endpoints/utils/serializers/input/pages.jsghost/core/core/server/api/endpoints/utils/serializers/input/posts.jsghost/core/core/server/services/comments/comments-service.jsghost/core/core/server/services/email-service/batch-sending-service.jsghost/core/core/server/services/email-service/email-controller.jsghost/core/core/server/services/members/members-api/repositories/event-repository.jsghost/core/core/server/services/url/lazy-url-service.tsghost/core/test/unit/api/canary/utils/serializers/input/pages.test.jsghost/core/test/unit/api/canary/utils/serializers/input/posts.test.jsghost/core/test/unit/api/endpoints/email-post.test.jsghost/core/test/unit/api/endpoints/search-index-public.test.jsghost/core/test/unit/api/endpoints/search-index.test.jsghost/core/test/unit/server/services/comments/comments-service.test.jsghost/core/test/unit/server/services/email-service/batch-sending-service.test.jsghost/core/test/unit/server/services/email-service/email-controller.test.jsghost/core/test/unit/server/services/members/members-api/repositories/event-repository.test.jsghost/core/test/unit/server/services/url/lazy-url-service.test.js
ref https://linear.app/tryghost/issue/HKG-1738/ The editor's bookmark / internal-link pickers (Admin and Content APIs) load posts with a hardcoded thin column list and don't request relations, so under lazyRouting the URL serializer evaluates each router's NQL filter against a resource with no .tags / .authors arrays. Every URL collapses to /404/ on a site with any tag- or author-filtered route — visible to users as bookmark cards that fail oembed lookup. Loading tags+authors here always (not just under lazyRouting) keeps the config-flag flip clean. The relations are stripped from the JSON response by the existing column-filter machinery, so the wire shape is unchanged.
bbc77fb to
bebaa31
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ghost/core/test/unit/api/canary/utils/serializers/input/posts.test.js`:
- Around line 139-177: The tests set global state via
configUtils.set('lazyRouting', true) but only call configUtils.restore() on the
success path, so a failing assertion can leak config; wrap the body of each
affected it(...) block in a try/finally (or move restore to an afterEach) to
ensure configUtils.restore() is always executed, referencing the existing calls
to configUtils.set/restore and the invocation of serializers.input.posts.browse
to locate the affected tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01653f2c-c613-47d7-ac8d-d7a360b6e004
📒 Files selected for processing (11)
ghost/core/core/server/api/endpoints/email-post.jsghost/core/core/server/api/endpoints/search-index-public.jsghost/core/core/server/api/endpoints/search-index.jsghost/core/core/server/api/endpoints/utils/serializers/input/pages.jsghost/core/core/server/api/endpoints/utils/serializers/input/posts.jsghost/core/core/server/services/comments/comments-service.jsghost/core/core/server/services/email-service/batch-sending-service.jsghost/core/core/server/services/email-service/email-controller.jsghost/core/core/server/services/members/members-api/repositories/event-repository.jsghost/core/core/server/services/url/lazy-url-service.tsghost/core/test/unit/api/canary/utils/serializers/input/posts.test.js
✅ Files skipped from review due to trivial changes (1)
- ghost/core/core/server/api/endpoints/search-index-public.js
🚧 Files skipped from review as they are similar to previous changes (8)
- ghost/core/core/server/api/endpoints/email-post.js
- ghost/core/core/server/services/comments/comments-service.js
- ghost/core/core/server/services/email-service/batch-sending-service.js
- ghost/core/core/server/services/members/members-api/repositories/event-repository.js
- ghost/core/core/server/services/url/lazy-url-service.ts
- ghost/core/core/server/api/endpoints/utils/serializers/input/pages.js
- ghost/core/core/server/api/endpoints/utils/serializers/input/posts.js
- ghost/core/core/server/api/endpoints/search-index.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
bebaa31 to
17e8461
Compare
ref https://linear.app/tryghost/issue/HKG-1738/ defaultRelations in the posts input serializer short-circuits whenever a caller passes thin columns (?fields=) without an explicit ?include=, which is the correct default for the eager URL service (URLs are precomputed at boot, the serializer just reads them by id). Under lazyRouting the URL is computed at serialization time from the resource's tags/authors, so a request like /admin/api/posts?fields=id,url silently 404s every URL for any tag- or author-filtered route. Force-load the relations whenever 'url' is in the requested columns, even if the caller already set withRelated via ?include= — their list may not include tags/authors. Mirrors the eager UrlService's behaviour of keeping a fully-loaded resource in scope when URL computation happens. The framework's column filter (_.pick on model.attributes) only strips scalar attributes — Bookshelf relations land on the JSON before the strip and would otherwise bleed into the response. forPost in the output utility strips the force-loaded relations so the wire shape matches what the caller asked for via ?fields= (also strips primary_tag / primary_author, which Post.toJSON computes from the relations). The search-index endpoint has its own thin-column path and is fixed separately in the previous commit.
ref https://linear.app/tryghost/issue/HKG-1738/ batch-sending-service loads the post that gets threaded through the email renderer to build the post URL embedded in the newsletter (via urlService.facade.getUrlForResource). The previous load only included posts_meta+authors; under lazyRouting the URL serializer evaluates each router's NQL filter against the loaded record and tag-filtered routes need 'tags' loaded too, otherwise every newsletter ships with /404/ links for sites using collection routing.
ref https://linear.app/tryghost/issue/HKG-1738/ email-controller loads the post for both preview and send-test endpoints, which then render through the email-renderer (same getUrlForResource path as batch-sending). Add 'tags' to the existing posts_meta+authors load so tag-filtered routes resolve under lazyRouting.
ref https://linear.app/tryghost/issue/HKG-1738/ The /email/<uuid>/ endpoint returns a post that goes through the email-posts output mapper → mappers.posts → url.forPost, which calls urlService.facade.getUrlForResource. Without tags+authors loaded the URL serializer can't evaluate tag- or author-filtered routes under lazyRouting and the rendered page embeds /404/ as the post URL. The fix merges with caller-supplied withRelated rather than overwriting, so ?include= options stay intact.
ref https://linear.app/tryghost/issue/HKG-1738/ getCommentEvents, getClickEvents and getFeedbackEvents all emit a post (or link.post) per row. The activity-feed mappers call url.forPost on that post, routing to urlService.facade.getUrlForResource. Without tags/authors loaded under lazyRouting every row's post URL is /404/ for tag- or author-filtered routes. Add post.tags / post.authors to the three withRelated lists (and link.post.tags / link.post.authors for clicks where the post is one relation deeper).
ref https://linear.app/tryghost/issue/HKG-1738/ getAdminAllComments emits a post per comment row for the admin moderation listing. mappers/comments.js calls url.forPost on the embedded post → urlService.facade.getUrlForResource. Without post.tags / post.authors loaded under lazyRouting each row's post URL resolves to /404/ for tag- or author-filtered routes.
ref https://linear.app/tryghost/issue/HKG-1738/ input/pages.js's defaultRelations had the same shape as the unfixed input/posts.js — a thin column request like ?fields=id,url short-circuits without loading any relations. Under lazyRouting the URL serializer needs tags+authors to evaluate filtered routes; without them a ?fields=url request resolves every URL to /404/. Mirrors the fix in input/posts.js: a forceUrlRelationsWhenLazy helper called from both the admin defaultRelations path and the Content API branch (which uses mapWithRelated rather than defaultRelations).
ref https://linear.app/tryghost/issue/HKG-1738/ The Content API branch of input/posts.js uses mapWithRelated rather than defaultRelations, so the lazyRouting fix in the previous PR (#27908) didn't cover Content API consumers passing ?fields=url. Same /404/ symptom: the URL serializer can't evaluate tag- or author-filtered routes against a thin record. Extracted the conditional into a forceUrlRelationsWhenLazy helper called from both the admin defaultRelations path and the Content API branch, so the two paths stay in sync.
ref https://linear.app/tryghost/issue/HKG-1738/ Operators flipping lazyRouting need a way to discover callers that pass thin resources (no tags/authors) against tag- or author-filtered routes — the URL silently falls through to /404/ and nothing in the logs points back to the bad caller. The previous commits in this stack fix every caller we found by audit, but a future regression in any of the ~20 call sites would silently break links again. Emit a warn log in LazyUrlService.getUrlForResource when: - the candidate router has a filter, and - the filter references tags / authors / primary_tag / primary_author, and - the resource doesn't have the matching relation loaded. The log identifies the resource (type#id), the router that triggered the check, and the filter expression — enough context to find the calling code and add the missing withRelated load. Detection is conservative: a tag-less router (filter: featured:true) against a tag-less resource doesn't log. Only fires when the filter genuinely needs a relation the caller didn't provide. Operators can alert on these warns once lazyRouting is on; they fire zero times under the eager service (this code path doesn't run with the flag off).
ref https://linear.app/tryghost/issue/HKG-1738/ mentions/service.js's getPostUrl passed an empty {} to url.forPost, so the resource reaching urlService.facade.getUrlForResource was {id, type} with no slug, tags, or authors. Every webmention URL fell through to /404/ under lazyRouting (and produced empty/broken URLs on the eager path too — the URL serializer's fallback to /404/ for missing slug masks the gap). Use post.toJSON() instead. The MentionSendingService.sendForPost path subscribes to post.published / page.published, both of which fire from Post.edit and arrive with tags+authors loaded via defaultRelations.
ref https://linear.app/tryghost/issue/HKG-1738/ email-renderer's show_latest_posts widget fetches the 3 most-recent posts via Post.findPage with no withRelated, then calls getPostUrl on each. Under lazyRouting the URL serializer evaluates router filters against the loaded record — tag- or author-filtered routes need the relations populated or each widget link resolves to /404/.
ref https://linear.app/tryghost/issue/HKG-1738/ The comments input serializer's `all` hook didn't extend `post` to `post.tags` / `post.authors` when callers asked for the post via `?include=post`. mappers/comments.js calls url.forPost on the embedded post; without the relations loaded, every comment row's `post.url` was /404/ for tag- or author-filtered routes. Covers the three comment fetch paths (getComments / getAdminComments / getReplies) at the serializer chokepoint, rather than each service method. getAdminAllComments was fixed separately because it bypasses the serializer with a hardcoded withRelated array.
e074149 to
f19a332
Compare
Summary
Re-opens #27908 (auto-closed when its branch was momentarily deleted during a stack rewrite).
Comprehensive fix for the
/404/regression on tag- and author-filtered routes underconfig.lazyRouting. The lazy URL service evaluates each registered router's NQL filter against the resource passed togetUrlForResource(resource); resources arriving withouttags/authorsarrays cause every filter to evaluate false and the URL falls through to/404/.This PR fixes every caller identified by audit, adds a warn log to catch any future regression, and ships the supporting tests.
Commits (11)
search-index.jsandsearch-index-public.jsalways loadtags+authors; needed by bookmark card and similar editor search flows.defaultRelationsin input/posts.js force-loadstags+authorswhen?fields=urlis requested (Admin API).tagsso the email-renderer's URL serialization works.tags+authors, merging with caller-supplied?include=.event-repository.getCommentEvents/getClickEvents/getFeedbackEventsloadpost.tags/post.authors(andlink.post.tags/link.post.authorsfor clicks).comments-service.getAdminAllCommentsloadspost.tags/post.authors.forceUrlRelationsWhenLazyhelper, called from both admin and Content API branches of input/posts.js.Known limitation
Under
?fields=url+lazyRouting: true, the response includes the force-loadedtagsandauthorsarrays (and computedprimary_tag/primary_author) even when the caller didn't request them. We considered building a marker-based output strip to drop the unrequested relations but the implementation overhead and added bug surface wasn't worth it for an experimental flag. Adding fields to an API response is not a breaking change (standard REST principle); a future PR can addwithRelatedFieldsto trim the response payload size if needed.Flag-off consumers see no change.
Test plan
pnpm test:single test/unit/api/canary/utils/serializers/input/posts.test.js— 29/31 (2 pre-existing html-to-mobiledoc env failures unrelated to these changes)pnpm test:single test/unit/api/canary/utils/serializers/input/pages.test.js— 22/22pnpm test:single test/unit/api/canary/utils/serializers/output/utils/url.test.js— 8/8pnpm test:single test/unit/api/endpoints/search-index.test.js— 4/4pnpm test:single test/unit/api/endpoints/search-index-public.test.js— 3/3pnpm test:single test/unit/api/endpoints/email-post.test.js— 1/1pnpm test:single test/unit/server/services/email-service/batch-sending-service.test.js— 55/55pnpm test:single test/unit/server/services/email-service/email-controller.test.js— 14/14pnpm test:single test/unit/server/services/members/members-api/repositories/event-repository.test.js— 35/35pnpm test:single test/unit/server/services/comments/comments-service.test.js— 1/1pnpm test:single test/unit/server/services/url/lazy-url-service.test.js— 33/33config.lazyRouting: truewith a tag-filtered collection in routes.yaml — every link (newsletter, email preview, admin activity feed, comment moderation, posts.browse, pages.browse, search-index, /email//) should resolve to the real URL