Diskless redirects: introduce storage abstraction (Phase 1)#27697
Draft
Diskless redirects: introduce storage abstraction (Phase 1)#27697
Conversation
ref https://linear.app/ghost/project/diskless-redirects-config-c4c5916f50a8 Adds characterization tests covering observable behaviour of the redirects service before extracting it into a Repository-pattern store + service. Locks in: - init() loads parsed redirects into the redirect manager when a file exists (existing tests only covered the no-file case) - init() logs and swallows errors when the redirects file is unparseable (graceful degradation that boot.js relies on) - setFromFilePath() does not attempt a backup when no existing file is on disk (the conditional backup branch was previously untested) - setFromFilePath() runs validation before any write or activation, and a validation failure short-circuits with no filesystem or manager mutation These tests will be migrated/adapted as part of the upcoming RedirectsService + FileStore split, but they're added here in a separate commit so the refactor is provably behaviour-preserving.
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
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:
WalkthroughCustom redirects were refactored from a file-path-based CustomRedirectsAPI to a RedirectsService backed by a new FileStore. FileStore reads legacy 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/core/server/services/custom-redirects/file-store.js`:
- Around line 50-57: replaceAll currently calls _backup() before the new file is
durably written, which can remove the live redirects if the subsequent
_writeAtomic/rename fails; change the flow in replaceAll (and the same pattern
at the other block) to first write the new JSON to a temporary/atomic path using
_writeAtomic (ensuring the write+rename to the canonical targetPath succeeds)
while leaving the existing file in place, then once the new file has been
committed/successfully renamed into place, call _backup(existingPath) to move
the previous canonical file to its backup location; reference replaceAll,
_findExistingFile, _writeAtomic and _backup to implement: write new -> commit ->
only then backup old.
In `@ghost/core/core/server/services/custom-redirects/redirect-config-parser.js`:
- Around line 49-54: Currently the parser accepts any plain mapping
(isPlainMapping(configYaml)) including mappings with no recognized status
sections, which later results in silently returning [] and wiping redirects;
update the validation after the isPlainMapping check to assert that the
top-level mapping contains at least one of the recognized status keys '301' or
'302' and throw the same errors.BadRequestError (using tpl(messages.yamlInvalid)
and tpl(messages.redirectsHelp)) if neither key is present so malformed YAML
like "foo: bar" is rejected; look for isPlainMapping, errors.BadRequestError,
tpl(messages.yamlInvalid) and messages.redirectsHelp in
redirect-config-parser.js to implement this guard.
In `@ghost/core/core/server/services/custom-redirects/redirects-service.js`:
- Around line 78-81: The replace() flow can persist redirects via
store.replaceAll() then call _loadIntoManager(), which swallows per-item
failures (via addRedirect()), leading to a persisted-but-not-activated state;
fix by making activation strict: before calling this.store.replaceAll(redirects)
or persisting, attempt to load all entries into a throwaway manager (e.g.,
instantiate the same RedirectsManager used by _loadIntoManager() or call
addRedirect() against a temp instance) and fail immediately on the first
rejected addRedirect() so replace() throws; only if the throwaway load succeeds,
proceed to this.store.replaceAll(redirects) and then load into the real manager
(or, alternatively, change _loadIntoManager() to throw on the first
addRedirect() error and have replace() propagate that error). Ensure the change
affects replace() (and the same pattern used in the similar block around lines
88-99) so persistence is all-or-nothing.
In `@ghost/core/core/server/services/custom-redirects/validation.js`:
- Around line 12-16: The validator currently only checks truthiness but needs to
enforce non-empty strings for `from` and `to` as promised by the JSDoc; update
the validation in the function (e.g., validateRedirects / the batch validator in
this file) to assert typeof redirect.from === 'string' &&
redirect.from.trim().length > 0 and typeof redirect.to === 'string' &&
redirect.to.trim().length > 0 before attempting to compile the `from` RegExp,
and throw the existing error with a clear message when these type checks fail;
ensure the RegExp compilation only runs on a confirmed string `from`.
In `@ghost/core/test/unit/server/services/custom-redirects/file-store.test.js`:
- Around line 151-166: The test is nondeterministic because it relies on
wall-clock seconds to collide backup filenames; modify the test to freeze the
clock so both replaceAll calls generate the same timestamped backup name. Use
the test suite's fake-timer utility (e.g., sinon.useFakeTimers) to set and
freeze time before creating the FileStore and performing the fs.writeFile/
store.replaceAll calls, then restore the clock at the end; this guarantees the
collision path in FileStore.replaceAll is exercised deterministically.
🪄 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: d66456f7-8c2a-4606-8ba3-665ccc819e68
📒 Files selected for processing (17)
ghost/core/core/server/api/endpoints/redirects.jsghost/core/core/server/services/custom-redirects/custom-redirects-api.jsghost/core/core/server/services/custom-redirects/file-store.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirect-config-parser.jsghost/core/core/server/services/custom-redirects/redirects-service.jsghost/core/core/server/services/custom-redirects/types.jsghost/core/core/server/services/custom-redirects/validation.jsghost/core/test/e2e-api/admin/redirects.test.jsghost/core/test/legacy/api/admin/redirects.test.jsghost/core/test/unit/server/services/custom-redirects/api.test.jsghost/core/test/unit/server/services/custom-redirects/file-store.test.jsghost/core/test/unit/server/services/custom-redirects/helpers/in-memory-store.jsghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.jsghost/core/test/unit/server/services/custom-redirects/in-memory-store.test.jsghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.jsghost/core/test/unit/server/services/custom-redirects/redirects-service.test.js
💤 Files with no reviewable changes (2)
- ghost/core/core/server/services/custom-redirects/custom-redirects-api.js
- ghost/core/test/unit/server/services/custom-redirects/api.test.js
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
1ce5558 to
2c48126
Compare
Collaborator
Author
|
Addressed CodeRabbit's 5 findings. Force-pushed. @coderabbitai review |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
[review] |
Collaborator
Author
|
@coderabbitai — for visibility, here's where each of the round-1 findings was addressed in the force-pushed stack (head 2c48126):
Please verify when convenient. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27927
The implementation looks solid. The only minor observation worth noting: in |
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
2c48126 to
469b70c
Compare
Collaborator
Author
|
Addressed the round-2 nit on _loadIntoManager null-return logging. Force-pushed to head 469b70c. @coderabbitai review |
Contributor
🧩 Analysis chainLet me verify the fix before triggering the review. 🏁 Script executed: Length of output: 8141
if (id === null) {
throw new errors.ValidationError({
message: tpl(messages.rejectedRedirect, { from: redirect.from, to: redirect.to }),
context: redirect
});
}The ✅ Actions performedReview triggered.
|
Contributor
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)
ghost/core/core/server/services/custom-redirects/validation.js (1)
26-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard redirect entry shape before property access.
At Line 27, malformed entries like
nullcan throw a rawTypeError(redirect.from) instead of the intendedValidationError. Add a null/object guard first so bad payloads consistently return the redirects-format error.Proposed fix
- _.each(redirects, function (redirect) { - if (!isNonEmptyString(redirect.from) || !isNonEmptyString(redirect.to)) { + _.each(redirects, function (redirect) { + if (!redirect || typeof redirect !== 'object' || !isNonEmptyString(redirect.from) || !isNonEmptyString(redirect.to)) { throw new errors.ValidationError({ message: tpl(messages.redirectsWrongFormat), context: redirect, help: tpl(messages.redirectsHelp) }); }🤖 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/custom-redirects/validation.js` around lines 26 - 33, The loop over redirects accesses redirect.from/redirect.to without ensuring redirect is an object; update the _.each callback to first guard that redirect is a non-null object (e.g. redirect && typeof redirect === 'object') before calling isNonEmptyString, and if that guard fails treat it as invalid and throw the same errors.ValidationError using tpl(messages.redirectsWrongFormat) and tpl(messages.redirectsHelp); this touches the _.each(redirects, function (redirect) { ... }) block and uses isNonEmptyString, errors.ValidationError, tpl and messages.redirectsWrongFormat/messages.redirectsHelp.
🤖 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/core/server/api/endpoints/redirects.js`:
- Around line 37-41: The query(frame) logic only detects '.yaml' exactly,
causing '.yml' and mixed-case extensions to be misrouted; change the check to
normalize the extension (e.g., const ext = (frame.file.ext || '').toLowerCase())
and call parseYaml when ext === '.yaml' || ext === '.yml', otherwise call
parseJson; update the conditional around frame.file.ext used in query to use
this normalized ext so '.YML' and '.YAML' are handled correctly by parseYaml.
In `@ghost/core/core/server/services/custom-redirects/redirects-service.js`:
- Around line 108-129: In _verifyAllLoadable(), wrap calls to the dry-run
manager's addRedirect (created via createDryRunManager()) in a try/catch so any
exception thrown by DynamicRedirectManager.addRedirect is caught and rethrown as
an errors.ValidationError with the same tpl(messages.rejectedRedirect, {from:
redirect.from, to: redirect.to}) message, include context: redirect and help:
tpl(messages.redirectsHelp); keep the existing null-check behavior but unify
thrown errors into the same ValidationError shape so replace() surfaces a
user-facing validation failure instead of a raw 500.
---
Outside diff comments:
In `@ghost/core/core/server/services/custom-redirects/validation.js`:
- Around line 26-33: The loop over redirects accesses redirect.from/redirect.to
without ensuring redirect is an object; update the _.each callback to first
guard that redirect is a non-null object (e.g. redirect && typeof redirect ===
'object') before calling isNonEmptyString, and if that guard fails treat it as
invalid and throw the same errors.ValidationError using
tpl(messages.redirectsWrongFormat) and tpl(messages.redirectsHelp); this touches
the _.each(redirects, function (redirect) { ... }) block and uses
isNonEmptyString, errors.ValidationError, tpl and
messages.redirectsWrongFormat/messages.redirectsHelp.
🪄 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: 4d730685-c4fa-4d1a-b47b-f0a62a108d3d
📒 Files selected for processing (18)
ghost/core/core/server/api/endpoints/redirects.jsghost/core/core/server/services/custom-redirects/custom-redirects-api.jsghost/core/core/server/services/custom-redirects/file-store.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirect-config-parser.jsghost/core/core/server/services/custom-redirects/redirects-service.jsghost/core/core/server/services/custom-redirects/types.jsghost/core/core/server/services/custom-redirects/validation.jsghost/core/test/e2e-api/admin/redirects.test.jsghost/core/test/legacy/api/admin/redirects.test.jsghost/core/test/unit/server/services/custom-redirects/api.test.jsghost/core/test/unit/server/services/custom-redirects/file-store.test.jsghost/core/test/unit/server/services/custom-redirects/helpers/in-memory-store.jsghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.jsghost/core/test/unit/server/services/custom-redirects/in-memory-store.test.jsghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.jsghost/core/test/unit/server/services/custom-redirects/redirects-service.test.jsghost/core/test/unit/server/services/custom-redirects/validation.test.js
💤 Files with no reviewable changes (2)
- ghost/core/core/server/services/custom-redirects/custom-redirects-api.js
- ghost/core/test/unit/server/services/custom-redirects/api.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.js
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
469b70c to
1513dbd
Compare
Collaborator
Author
|
Round-3 fixes pushed (head 1513dbd):
@coderabbitai review |
Contributor
🧩 Analysis chainLet me verify the round-3 fixes before triggering the review. 🏁 Script executed: Length of output: 25847
All round-1, round-2, and round-3 findings are accounted for. Triggering a fresh review of the force-pushed stack. ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.js (1)
23-29: ⚡ Quick winAdd a contract check that
getAll()returns a defensive copy.The suite verifies input mutation safety after
replaceAll, but not mutation safety of the array/objects returned bygetAll(). Without this, a store can still leak internal state to callers.Proposed test addition
describe('getAll', function () { it('returns an empty array when no redirects have been stored', async function () { const result = await store.getAll(); assert.deepEqual(result, []); }); + + it('returns data that callers cannot mutate in-place', async function () { + await store.replaceAll([{from: '/a', to: '/b', permanent: true}]); + + const firstRead = await store.getAll(); + firstRead.push({from: '/x', to: '/y', permanent: false}); + firstRead[0].to = '/mutated'; + + assert.deepEqual(await store.getAll(), [ + {from: '/a', to: '/b', permanent: true} + ]); + }); });Also applies to: 31-70
🤖 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/custom-redirects/helpers/store-contract.js` around lines 23 - 29, Add a contract test ensuring getAll returns a defensive copy: call store.replaceAll with sample redirects, call store.getAll to obtain the array, mutate the returned array (e.g. push/pop) and mutate an item object inside it, then call store.getAll again and assert the store's internal data was not affected; reference the store.getAll and store.replaceAll methods and ensure both array-level and object-level mutations are tested so callers cannot mutate internal state via the returned value.
🤖 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/core/server/services/custom-redirects/file-store.js`:
- Around line 74-75: The backup step (this._backup(existingPath)) can throw
after replaceAll() has already committed redirects.json (targetPath), leaving a
stale redirects.yaml and an unreachable new JSON; wrap the backup call in a
try/catch inside replaceAll() (or the containing function) and on any error
delete/unlink the newly written targetPath (redirects.json) to roll back the
committed JSON, then rethrow the original error so callers still see the
failure; reference existingPath, targetPath, this._backup(), and redirects.json
when locating where to add the try/catch and cleanup.
---
Nitpick comments:
In
`@ghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.js`:
- Around line 23-29: Add a contract test ensuring getAll returns a defensive
copy: call store.replaceAll with sample redirects, call store.getAll to obtain
the array, mutate the returned array (e.g. push/pop) and mutate an item object
inside it, then call store.getAll again and assert the store's internal data was
not affected; reference the store.getAll and store.replaceAll methods and ensure
both array-level and object-level mutations are tested so callers cannot mutate
internal state via the returned value.
🪄 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: 0e3923be-b42b-4a9a-b4dd-08abbf040919
📒 Files selected for processing (18)
ghost/core/core/server/api/endpoints/redirects.jsghost/core/core/server/services/custom-redirects/custom-redirects-api.jsghost/core/core/server/services/custom-redirects/file-store.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirect-config-parser.jsghost/core/core/server/services/custom-redirects/redirects-service.jsghost/core/core/server/services/custom-redirects/types.jsghost/core/core/server/services/custom-redirects/validation.jsghost/core/test/e2e-api/admin/redirects.test.jsghost/core/test/legacy/api/admin/redirects.test.jsghost/core/test/unit/server/services/custom-redirects/api.test.jsghost/core/test/unit/server/services/custom-redirects/file-store.test.jsghost/core/test/unit/server/services/custom-redirects/helpers/in-memory-store.jsghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.jsghost/core/test/unit/server/services/custom-redirects/in-memory-store.test.jsghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.jsghost/core/test/unit/server/services/custom-redirects/redirects-service.test.jsghost/core/test/unit/server/services/custom-redirects/validation.test.js
💤 Files with no reviewable changes (2)
- ghost/core/test/unit/server/services/custom-redirects/api.test.js
- ghost/core/core/server/services/custom-redirects/custom-redirects-api.js
🚧 Files skipped from review as they are similar to previous changes (3)
- ghost/core/test/unit/server/services/custom-redirects/file-store.test.js
- ghost/core/test/legacy/api/admin/redirects.test.js
- ghost/core/test/unit/server/services/custom-redirects/in-memory-store.test.js
ref https://linear.app/ghost/issue/HKG-1692 Establishes the seam where storage backends become swappable. Without a shared contract, FileStore (existing local-disk behaviour) and the upcoming GCSStore would inevitably drift on edge cases like empty state, overwrite semantics, and reference handling. - types.js: canonical JSDoc typedefs for RedirectConfig and RedirectsStore. RedirectConfig fields (from, to, permanent) verified against current validation.js and DynamicRedirectManager.addRedirect call sites — the store handles only those, no extras. - helpers/store-contract.js: a parameterised describe block any store implementation can plug into via a createStore factory. - helpers/in-memory-store.js: a reference implementation used to validate the contract suite itself, plus a useful test double for higher-level service tests later in the milestone. - in-memory-store.test.js: wires the in-memory implementation into the contract. Backups, file paths, and other implementation-specific concerns are deliberately not in the contract — they live in each store's own tests.
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
1513dbd to
c173d01
Compare
Collaborator
Author
|
Round-4 fixes pushed (head c173d01):
@coderabbitai review |
Contributor
🧩 Analysis chainLet me verify the round-4 fixes before triggering the review. 🏁 Script executed: Length of output: 18689
All round-1 through round-4 findings are accounted for. Triggering a fresh review of the force-pushed stack. ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ghost/core/core/server/services/custom-redirects/redirect-config-parser.js (1)
85-91: 💤 Low valueConsider validating that
section[from]is a string before pushing.The
tovalue extracted from YAML could be a non-string (number, object, etc.). While downstreamvalidation.validate()catches this, rejecting early inparseYamlwould provide clearer error context (pointing to the YAML structure rather than the redirect entry).🤖 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/custom-redirects/redirect-config-parser.js` around lines 85 - 91, The loop that builds redirect entries currently pushes section[from] into the redirect object without type-checking; update the loop in parseYaml (the block that iterates "for (const from in section)" and calls redirects.push(...)) to validate that section[from] is a string before pushing—if it's not a string, throw a clear Error (or return a validation error) that includes the redirect key (from) and the actual typeof section[from] so the error points to the YAML structure (e.g., "Invalid redirect target for '<from>': expected string but got <type>"). Ensure you reference the same redirects array and maintain the existing permanent/statusCode logic.ghost/core/test/unit/server/services/custom-redirects/file-store.test.js (1)
151-174: ⚡ Quick winThis collision test never proves the stale backup was replaced.
Right now it only checks that the second write succeeds and the canonical state is updated. Because the recreated YAML content is identical on both passes, an implementation that leaves
fixedBackupuntouched would still pass. Make the second YAML payload distinct and assertfixedBackupcontains that second payload.💡 Tighten the collision assertion
await fs.writeFile(yamlPath, '301:\n /a/: /b/\n', 'utf-8'); await store.replaceAll([{from: '/first', to: '/x', permanent: true}]); // Re-create a yaml so the next replaceAll has something to // back up onto the same fixedBackup target. - await fs.writeFile(yamlPath, '301:\n /a/: /b/\n', 'utf-8'); + await fs.writeFile(yamlPath, '301:\n /second-backup/: /z/\n', 'utf-8'); await store.replaceAll([{from: '/second', to: '/y', permanent: true}]); + assert.equal( + await fs.readFile(fixedBackup, 'utf-8'), + '301:\n /second-backup/: /z/\n' + ); assert.deepEqual(await store.getAll(), [ {from: '/second', to: '/y', permanent: true} ]);🤖 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/custom-redirects/file-store.test.js` around lines 151 - 174, The test for FileStore's collision behavior should verify the fixed backup file was actually overwritten: when using the injected getBackupFilePath (fixedBackup) make the second fs.writeFile to yamlPath write a distinct payload (different from the first), then after the second store.replaceAll read the fixedBackup file and assert its contents match the second payload (in addition to the existing assert on store.getAll); reference FileStore, getBackupFilePath, replaceAll, fixedBackup, yamlPath and store.getAll to locate where to change the writes and add the backup-file read/assertion.ghost/core/test/e2e-api/admin/redirects.test.js (1)
55-71: ⚡ Quick winAdd an end-to-end upload case for YAML input.
This suite now locks in the YAML download change, but it still only exercises JSON uploads. The new logic in this PR is the extension-based YAML/JSON parsing path, so a
.yaml/.ymlupload should be covered here too or this e2e can stay green while the main changed path regresses.💡 Suggested test shape
+ it('upload yaml', function () { + const filePath = path.join(config.get('paths:contentPath'), 'redirects-init.yml'); + fs.writeFileSync(filePath, '301:\n /k: /l\n'); + + return request + .post(localUtils.API.getApiQuery('redirects/upload/')) + .set('Origin', config.get('url')) + .attach('redirects', filePath) + .expect('Content-Type', /application\/json/) + .expect(200) + .expect((res) => { + assert.deepEqual(res.body, {}); + }); + });🤖 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/e2e-api/admin/redirects.test.js` around lines 55 - 71, Update the e2e test in redirects.test.js to also exercise YAML uploads: in the existing it('upload', ...) test (and/or add a new it block) write a redirects-init.yaml (or .yml) file into the contentPath using YAML syntax for the same redirect array, then call request.post(localUtils.API.getApiQuery('redirects/upload/')) and .attach('redirects', path.join(config.get('paths:contentPath'), 'redirects-init.yaml')) (or .yml), keeping the same headers and assertions (expect JSON response 200 and assert.deepEqual(res.body, {})) so the extension-driven YAML/JSON parsing path is covered.
🤖 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/custom-redirects/redirect-config-parser.js`:
- Around line 85-91: The loop that builds redirect entries currently pushes
section[from] into the redirect object without type-checking; update the loop in
parseYaml (the block that iterates "for (const from in section)" and calls
redirects.push(...)) to validate that section[from] is a string before
pushing—if it's not a string, throw a clear Error (or return a validation error)
that includes the redirect key (from) and the actual typeof section[from] so the
error points to the YAML structure (e.g., "Invalid redirect target for '<from>':
expected string but got <type>"). Ensure you reference the same redirects array
and maintain the existing permanent/statusCode logic.
In `@ghost/core/test/e2e-api/admin/redirects.test.js`:
- Around line 55-71: Update the e2e test in redirects.test.js to also exercise
YAML uploads: in the existing it('upload', ...) test (and/or add a new it block)
write a redirects-init.yaml (or .yml) file into the contentPath using YAML
syntax for the same redirect array, then call
request.post(localUtils.API.getApiQuery('redirects/upload/')) and
.attach('redirects', path.join(config.get('paths:contentPath'),
'redirects-init.yaml')) (or .yml), keeping the same headers and assertions
(expect JSON response 200 and assert.deepEqual(res.body, {})) so the
extension-driven YAML/JSON parsing path is covered.
In `@ghost/core/test/unit/server/services/custom-redirects/file-store.test.js`:
- Around line 151-174: The test for FileStore's collision behavior should verify
the fixed backup file was actually overwritten: when using the injected
getBackupFilePath (fixedBackup) make the second fs.writeFile to yamlPath write a
distinct payload (different from the first), then after the second
store.replaceAll read the fixedBackup file and assert its contents match the
second payload (in addition to the existing assert on store.getAll); reference
FileStore, getBackupFilePath, replaceAll, fixedBackup, yamlPath and store.getAll
to locate where to change the writes and add the backup-file read/assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d9befa2-3d8d-47d5-92e9-1d644274005b
📒 Files selected for processing (18)
ghost/core/core/server/api/endpoints/redirects.jsghost/core/core/server/services/custom-redirects/custom-redirects-api.jsghost/core/core/server/services/custom-redirects/file-store.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirect-config-parser.jsghost/core/core/server/services/custom-redirects/redirects-service.jsghost/core/core/server/services/custom-redirects/types.jsghost/core/core/server/services/custom-redirects/validation.jsghost/core/test/e2e-api/admin/redirects.test.jsghost/core/test/legacy/api/admin/redirects.test.jsghost/core/test/unit/server/services/custom-redirects/api.test.jsghost/core/test/unit/server/services/custom-redirects/file-store.test.jsghost/core/test/unit/server/services/custom-redirects/helpers/in-memory-store.jsghost/core/test/unit/server/services/custom-redirects/helpers/store-contract.jsghost/core/test/unit/server/services/custom-redirects/in-memory-store.test.jsghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.jsghost/core/test/unit/server/services/custom-redirects/redirects-service.test.jsghost/core/test/unit/server/services/custom-redirects/validation.test.js
💤 Files with no reviewable changes (2)
- ghost/core/test/unit/server/services/custom-redirects/api.test.js
- ghost/core/core/server/services/custom-redirects/custom-redirects-api.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.js
ref https://linear.app/ghost/issue/HKG-1693 Pulls the YAML/JSON parsing logic out of CustomRedirectsAPI (lines 54-114 of custom-redirects-api.js) into a standalone module so that the upcoming FileStore and the API endpoint can both reuse it without depending on the legacy class. Also adds the YAML serialisation that HKG-1697 will need on the download path — it does not exist anywhere in the current codebase. - parseJson / parseYaml: behaviour preserved from the existing parseRedirectsFile, including the BadRequestError shape and the ordering quirk where 302 entries appear before 301 entries (matches current init() output, so nothing observable changes). - parseYaml additionally rejects top-level YAML lists, scalars, and non-mapping `301:` / `302:` sections. Without these guards an arbitrary list-shaped upload would parse to zero redirects and silently wipe the live config via replaceAll([]) on the upload path. Empty status-code sections (`301:` followed by nothing, common in hand-edited files) are tolerated and contribute zero redirects. - Empty input throws BadRequestError. The legacy parseRedirectsFile threw on JSON.parse('') and yaml.load('')→null, and FileStore.getAll reaches the parser only when the file actually exists, so this preserves the legacy behaviour where a zero-byte redirects file surfaces as a logged error at boot rather than a silent zero-redirect state. - serializeToYaml: section headers (`301:` / `302:`) are emitted by hand because js-yaml quotes numeric-string keys to disambiguate them from integers, which would give us `"301":` instead of `301:` and break visual parity with every YAML redirects file Ghost has shipped to date. Each `from: to` pair underneath is dumped via js-yaml so escaping, block scalars, and quoting of special characters in the redirect fields (colons, hashes, leading dashes, multi-line values) are handled correctly. custom-redirects-api.js is not yet updated to use this module; that swap happens later in the milestone after the service/store split is in place. Keeping this commit purely additive means the legacy class keeps passing its tests unchanged.
ref https://linear.app/ghost/issue/HKG-1694 The first concrete implementation of the RedirectsStore contract. Self- hosters use this permanently, so backward compatibility with existing yaml/json files on disk is non-negotiable. The contract suite from HKG-1692 plugs in directly to verify behaviour. - getAll(): reads `redirects.yaml` if present, falling back to `redirects.json`. Same precedence as the existing CustomRedirectsAPI, so a self-hosted upgrade is an in-place swap with no on-disk migration. - replaceAll(): always writes `redirects.json` regardless of what was previously on disk. The legacy yaml file (if any) is moved to a timestamped backup. After the first upload on a yaml-based instance, subsequent reads come from the json file. YAML comments are lost on that one-shot conversion — accepted tradeoff per HKG-1694. - Atomic writes via temp-file + rename. fs.rename is atomic on POSIX within the same filesystem, so a process crash mid-write either leaves the previous file intact or the new file fully written. The scratch file is best-effort cleaned up on rename failure. - Backup collisions inside the same second overwrite the older backup rather than failing — matches the legacy behaviour and avoids a surprising error on rapid successive uploads. CustomRedirectsAPI still exists and is still wired in via index.js; it's swapped out in a later commit once the orchestrating service is in place.
ref https://linear.app/ghost/issue/HKG-1695 Splits storage from activation so the in-memory router can be rebuilt from the store without writing a file. Today's setFromFilePath couples the two: there is no way to say "reload from disk" without an upload. The future cross-instance notification project (out of scope for this milestone) needs activation as a standalone operation, and laying the seam now means it can be wired up without touching this service. - activate(): read from store, then clear+rebuild the DynamicRedirectManager. Per-item validation: a single bad redirect (bad regex, missing field) is logged and skipped without aborting the rest of the batch. Today's behaviour is harsher — one bad rule kills the whole config — so this is a deliberate shift toward resilience on the read path. Store-level errors bubble up so callers can decide. - replace(): batch-validates first (so a publisher upload either succeeds entirely or rejects without persisting), then store.replaceAll + activate. - init(): the boot wrapper. Catches every error, distinguishing Ghost errors from unwrapped ones for consistent logging shape, matching CustomRedirectsAPI.init's existing graceful-degradation policy. - The DynamicRedirectManager instance is mutated, never replaced, because site.js wires `redirectManager.handleRequest` into Express at boot — swapping the manager would dangle that reference. The service is not yet wired into index.js — the swap is the next commit.
ref https://linear.app/ghost/issue/HKG-1696 Switches the Ghost runtime from CustomRedirectsAPI to the new RedirectsService + FileStore pair. boot.js and site.js are not touched — the module still exports `{ init(), api, middleware }` so the blast radius stays inside this directory. - index.js: composes FileStore + DynamicRedirectManager + validate into a RedirectsService, drops the import of the legacy class. The legacy file itself is left in the tree until the dead-code sweep that follows; nothing imports it now. - api/endpoints/redirects.js: switches to the service's `getAll`/`replace` methods and parses uploads at the boundary using the parser util. The download path is intentionally left as JSON-with-`redirects.json`- filename for this commit; HKG-1697 will flip it to YAML in the next commit and update the e2e test alongside. - test/legacy/api/admin/redirects.test.js: the existing yaml-on-disk assertion no longer holds because FileStore writes JSON regardless of upload format. Updated to assert that the canonical redirects.json contains the parsed config and that a timestamped backup is created on overwrite — the observable contract for self-hosters. Verified by running the redirects unit tests, the test/e2e-api/admin/redirects.test.js download/upload pair, and the legacy integration test.
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
c173d01 to
1962ffb
Compare
Collaborator
Author
|
Round-5 nits pushed (head 1962ffb):
@coderabbitai review |
Contributor
🧩 Analysis chainLet me verify the round-5 fixes before triggering the review. 🏁 Script executed: Length of output: 32408
All round-1 through round-5 findings are accounted for. Triggering a fresh review of the force-pushed stack. ✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ghost/core/test/e2e-api/admin/redirects.test.js (1)
80-88: ⚡ Quick winAssert upload side effects, not just status/body.
At Line 86, this test only checks
200+{}. A silent no-op upload could still pass. Please add a follow-up read assertion (e.g., download and confirm/yaml-from/ -> /yaml-to/exists) so the YAML path is functionally validated.Suggested test hardening
- it('upload (yaml)', function () { + it('upload (yaml)', async function () { // The endpoint chooses parser by file extension. Cover the // YAML branch explicitly so a regression on the JSON path // doesn't quietly hide the missing YAML coverage. const filePath = path.join(config.get('paths:contentPath'), 'redirects-init.yaml'); fs.writeFileSync(filePath, '301:\n /yaml-from/: /yaml-to/\n'); - return request + await request .post(localUtils.API.getApiQuery('redirects/upload/')) .set('Origin', config.get('url')) .attach('redirects', filePath) .expect('Content-Type', /application\/json/) .expect(200) .expect((res) => { assert.deepEqual(res.body, {}); }); + + await request + .get(localUtils.API.getApiQuery('redirects/download/')) + .set('Origin', config.get('url')) + .expect(200) + .expect((res) => { + const redirects = parseYaml(res.text); + assert.equal( + redirects.some(r => r.from === '/yaml-from/' && r.to === '/yaml-to/'), + true + ); + }); });🤖 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/e2e-api/admin/redirects.test.js` around lines 80 - 88, After the POST to redirects/upload/ (the request.post(localUtils.API.getApiQuery('redirects/upload/')) call and its existing assertions), add a follow-up read to verify the uploaded redirect actually exists: perform a GET (e.g., request.get(localUtils.API.getApiQuery('redirects/')) or download endpoint) and assert that an entry mapping "/yaml-from/" to "/yaml-to/" is present in the returned redirects list/body; update the test around the .expect((res) => { assert.deepEqual(res.body, {}); }) block to include this additional GET and assertion so the upload side effect is validated, not just the 200 and empty body.
🤖 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/test/e2e-api/admin/redirects.test.js`:
- Around line 80-88: After the POST to redirects/upload/ (the
request.post(localUtils.API.getApiQuery('redirects/upload/')) call and its
existing assertions), add a follow-up read to verify the uploaded redirect
actually exists: perform a GET (e.g.,
request.get(localUtils.API.getApiQuery('redirects/')) or download endpoint) and
assert that an entry mapping "/yaml-from/" to "/yaml-to/" is present in the
returned redirects list/body; update the test around the .expect((res) => {
assert.deepEqual(res.body, {}); }) block to include this additional GET and
assertion so the upload side effect is validated, not just the 200 and empty
body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd25e5d5-13a4-4e8c-91cf-476ae9c0f8d5
📒 Files selected for processing (12)
ghost/core/core/server/api/endpoints/redirects.jsghost/core/core/server/services/custom-redirects/custom-redirects-api.jsghost/core/core/server/services/custom-redirects/file-store.jsghost/core/core/server/services/custom-redirects/index.jsghost/core/core/server/services/custom-redirects/redirect-config-parser.jsghost/core/core/server/services/custom-redirects/redirects-service.jsghost/core/test/e2e-api/admin/redirects.test.jsghost/core/test/legacy/api/admin/redirects.test.jsghost/core/test/unit/server/services/custom-redirects/api.test.jsghost/core/test/unit/server/services/custom-redirects/file-store.test.jsghost/core/test/unit/server/services/custom-redirects/redirect-config-parser.test.jsghost/core/test/unit/server/services/custom-redirects/redirects-service.test.js
💤 Files with no reviewable changes (2)
- ghost/core/test/unit/server/services/custom-redirects/api.test.js
- ghost/core/core/server/services/custom-redirects/custom-redirects-api.js
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/core/test/unit/server/services/custom-redirects/file-store.test.js
ref https://linear.app/ghost/issue/HKG-1697 Changelog: when downloading the custom redirects config, Ghost now always returns a YAML file (\`redirects.yaml\`) regardless of which format was originally uploaded. Previously, JSON uploads downloaded as \`redirects.json\` and YAML uploads downloaded as \`redirects.yaml\`, which made the download ↔ edit ↔ re-upload loop awkward when the format varied. YAML is the recommended format per Ghost docs and the only format the existing CLI tooling generates examples for, so making it consistent removes a small papercut. Note: any comments, anchors, or whitespace in a hand-edited \`redirects.yaml\` on disk are lost as soon as the publisher uploads or downloads via the admin API. The on-disk file is now the canonical \`redirects.json\` (FileStore writes JSON regardless of upload format), and downloads reconstruct YAML from the parsed structure. Self-hosters who manage \`redirects.yaml\` directly on disk and never use the admin upload endpoint are unaffected. Endpoint changes: - download: serializes the service result via serializeToYaml, Content-Disposition is \`redirects.yaml\`, response format is plain so the framework writes the string verbatim, Content-Type is application/yaml via the existing yaml disposition helper. - The service layer no longer receives file paths or raw file content on either path — it only ever sees \`RedirectConfig[]\`. That seals the boundary the milestone has been working toward. Test updates: - e2e admin redirects download test now asserts the YAML Content-Disposition + Content-Type and round-trips the body through parseYaml for a structural compare. Comparing byte-for-byte would bake in our specific YAML formatting (key quoting, section order) which is incidental, not contract.
ref https://linear.app/ghost/issue/HKG-1698 All call sites moved to RedirectsService + FileStore in earlier commits. The legacy class and its unit-test file are now dead weight in the redirects service directory and confuse future readers. - core/server/services/custom-redirects/custom-redirects-api.js: gone. - test/unit/server/services/custom-redirects/api.test.js: gone. Its characterization tests were behaviour-level (init activates from disk, validate runs before write, no backup when no existing file, validation failure short-circuits) and the equivalent assertions now live in redirects-service.test.js / file-store.test.js against the new abstractions. - utils.js (getBackupRedirectsFilePath) is preserved — FileStore uses it. \`grep -r CustomRedirectsAPI\` and \`grep -r custom-redirects-api\` both return zero results across the codebase post-deletion.
allouis
force-pushed
the
fabien/diskless-redirects-phase-1
branch
from
1962ffb to
2ed05af
Compare
Collaborator
Author
|
Round-6 nit pushed (head 2ed05af):
@coderabbitai review |
Contributor
🧩 Analysis chainLet me verify the round-6 fix before triggering the review. 🏁 Script executed: Length of output: 5229
One small observation: All round-1 through round-6 findings are accounted for. Triggering a fresh review of the force-pushed stack. ✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 of the Diskless Redirects Config project. Replaces the legacy
CustomRedirectsAPIwith a Repository-pattern split —RedirectsServiceorchestrating aRedirectsStore(FileStore for now, GCSStore in Phase 2). No infrastructure change, only a refactor and one user-visible flip.The store boundary is the seam Phase 2 plugs into.
activate()is independently callable so the future cross-instance notifier can rebuild in-memory state without re-persisting.User-visible change
The admin redirects download endpoint now always returns YAML (
redirects.yaml) regardless of the original upload format. Previously JSON uploads downloaded as JSON, YAML uploads as YAML — making the download → edit → re-upload loop awkward when the format varied. YAML is the recommended format per the docs and the only one the existing tooling generates examples for.Self-hosters who SSH-edit
redirects.yamldirectly on disk are unaffected. Self-hosters who use the admin upload endpoint will see their YAML comments / anchors / formatting stripped after the first upload (FileStore writes JSON regardless of input format). The replaced YAML is preserved as a timestamped backup incontent/data/.Stack (best reviewed commit-by-commit)
Test plan
🤖 Generated with Claude Code