Skip to content

Mirror eager's excluded cache columns in the lazy URL filter evaluation - #29517

Merged
allouis merged 1 commit into
mainfrom
lazy-url-excluded-column-filter
Jul 22, 2026
Merged

Mirror eager's excluded cache columns in the lazy URL filter evaluation#29517
allouis merged 1 commit into
mainfrom
lazy-url-excluded-column-filter

Conversation

@allouis

@allouis allouis commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

Part of the eager → lazy URL service migration (compare mode). Shadow comparison flagged published posts on sites whose collection filter references a column the eager service drops from its cache (e.g. custom_template:null): eager serves the post, the lazy service returned /404/.

The eager service excludes a set of columns from its in-memory URL cache and evaluates each router's collection filter against that reduced record. An excluded column reads as absent, and NQL treats absent as null — so custom_template:null matches and eager serves the post. The lazy service loads full records and saw the real value, so the filter failed and it 404'd posts eager routes. Since the migration must preserve eager's live URLs, the lazy service now strips eager's excluded columns before evaluating router filters (and neither requires nor force-loads them), reproducing eager's blind spot exactly where eager has one. The base filter is untouched — it still runs against the full record, since eager applies status/type/visibility at query time rather than in the cache.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 04a2a73d-cc5f-4268-bef6-90a449ca9ebf

📥 Commits

Reviewing files that changed from the base of the PR and between 35e0a71 and 8811f19.

📒 Files selected for processing (2)
  • ghost/core/core/server/services/url/lazy-url-service.ts
  • ghost/core/test/unit/server/services/url/lazy-url-service.test.js

Walkthrough

LazyUrlService now derives eagerly excluded fields from URL configuration and removes them from router-filter records. Required-field calculation and thin-resource validation ignore excluded scalar fields. URL lookup, ownership checks, and URL resolution use the transformed records for filter matching. Tests cover custom_template null matching, missing excluded fields, and required-field output.

Possibly related PRs

  • TryGhost/Ghost#29416: Coordinates required URL columns for search-index endpoints with lazy URL required-field behavior.
  • TryGhost/Ghost#29436: Updates URL building to preload required relations for thin-resource handling.

Suggested reviewers: vershwal

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch lazy-url-excluded-column-filter

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

@nx-cloud

nx-cloud Bot commented Jul 22, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit adaaeef

Command Status Duration Result
nx run ghost:test:ci:integration ✅ Succeeded 3m 10s View ↗
nx run ghost:test:integration ✅ Succeeded 3m 3s View ↗
nx run ghost:test:legacy ✅ Succeeded 2m 23s View ↗
nx run ghost:test:e2e ✅ Succeeded 2m 34s View ↗
nx run-many --target=build --projects=tag:publi... ✅ Succeeded 1s View ↗
nx run-many -t test:unit -p ghost ✅ Succeeded 4s View ↗
nx run ghost-monorepo:lint:boundaries ✅ Succeeded <1s View ↗
nx run-many -t lint -p ghost,ghost-monorepo ✅ Succeeded 1s View ↗
nx run @tryghost/admin:build ✅ Succeeded 5s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-07-22 02:48:27 UTC

ref https://linear.app/ghost/issue/HKG-1937

Eager drops a set of columns from its in-memory URL cache (the exclude
lists in services/url/config.js) and evaluates each router's collection
filter against that reduced record. An excluded column therefore reads
as absent, and NQL treats absent as null — so a filter like
custom_template:null matches and eager serves the post.

Lazy loads full records and saw the real column value, so the same
filter failed to match and it 404'd posts eager routes. Since we are
preserving eager's behaviour, lazy now strips eager's excluded columns
before evaluating router filters (and neither requires nor force-loads
them), reproducing eager's blind spot exactly where eager has one. The
base filter is untouched: it still runs against the full record, as
eager applies status/type/visibility at query time rather than in the
cache.
@allouis
allouis force-pushed the lazy-url-excluded-column-filter branch from adaaeef to 8811f19 Compare July 22, 2026 02:35
@codecov

codecov Bot commented Jul 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.57%. Comparing base (35e0a71) to head (8811f19).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
.../core/core/server/services/url/lazy-url-service.ts 88.88% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29517      +/-   ##
==========================================
+ Coverage   74.54%   74.57%   +0.03%     
==========================================
  Files        1609     1609              
  Lines      141454   141521      +67     
  Branches    17266    17287      +21     
==========================================
+ Hits       105443   105538      +95     
+ Misses      34945    34917      -28     
  Partials     1066     1066              
Flag Coverage Δ
e2e-tests 76.64% <88.88%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@allouis
allouis marked this pull request as ready for review July 22, 2026 03:54
@allouis
allouis merged commit a398294 into main Jul 22, 2026
47 of 48 checks passed
@allouis
allouis deleted the lazy-url-excluded-column-filter branch July 22, 2026 03:54
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.

1 participant