From e3fbc59d8c59f02f65883eb440b164348a1e71f9 Mon Sep 17 00:00:00 2001 From: david catalan Date: Fri, 8 May 2026 13:37:34 +0200 Subject: [PATCH] fix(content): correctness fixes for push, merge, and delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * push: --path now uses exact-or-trailing-slash matching, so `--path /blog` no longer matches `/blog2/post.html`. * push: do not advance refs/da/synced when a --path filter excludes other ahead changes — those would otherwise be silently considered already synced on the next run. * push: rely on the error counts returned by _uploadFiles and _deleteFiles instead of re-reading the input arrays. processQueue consumes its input via shift(), so putTargets.every(...) and deleted.every(...) were vacuously true and the synced ref was advanced on every push even when uploads or deletes failed. * merge: pass the project root (not content/) to getValidToken so the cached login is reused across content subcommands and .hlx/.da-token.json is not written inside the content repo. * da-api: deleteSource throws on non-2xx (matching putSource) and treats 404 as success (idempotent delete). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/content/da-api.js | 8 ++- src/content/merge.cmd.js | 2 +- src/content/push.cmd.js | 57 +++++++++------- test/content/da-api.test.js | 14 ++++ test/content/merge.cmd.test.js | 26 ++++++++ test/content/push.cmd.test.js | 115 +++++++++++++++++++++++++++++++++ 6 files changed, 194 insertions(+), 28 deletions(-) diff --git a/src/content/da-api.js b/src/content/da-api.js index 8d697c40f..fa6029ed1 100644 --- a/src/content/da-api.js +++ b/src/content/da-api.js @@ -162,7 +162,8 @@ export class DaClient { } /** - * Deletes a file or folder. Idempotent. + * Deletes a file or folder. Idempotent: 404 is treated as success. + * Throws on transport or server errors so callers don't silently treat them as success. */ async deleteSource(org, repo, daPath) { const url = `${DA_ADMIN}/source/${org}/${repo}${daPath}`; @@ -173,7 +174,10 @@ export class DaClient { if (res.status === 401) { throw new Error('Unauthorized: invalid or missing token'); } - return res.ok || res.status === 204; + if (res.ok || res.status === 204 || res.status === 404) { + return true; + } + throw new Error(`DELETE failed for ${daPath}: ${res.status} ${res.statusText}`); } /** diff --git a/src/content/merge.cmd.js b/src/content/merge.cmd.js index 181b56eef..731bb9688 100644 --- a/src/content/merge.cmd.js +++ b/src/content/merge.cmd.js @@ -96,7 +96,7 @@ export default class MergeCommand { return; } - const token = await getValidToken(log, this._token, contentDir); + const token = await getValidToken(log, this._token, this._dir); const client = new DaClient(token); // Get the HEAD commit to read base blobs diff --git a/src/content/push.cmd.js b/src/content/push.cmd.js index 1df7aafc5..fc84f231c 100644 --- a/src/content/push.cmd.js +++ b/src/content/push.cmd.js @@ -108,16 +108,16 @@ export default class PushCommand { * @param {string} repo * @param {string} contentDir * @param {string[]} targets - * @returns {Promise<{ pushed: number, errors: number, successfullyPushed: Set }>} + * @returns {Promise<{ pushed: number, errors: number }>} */ async _uploadFiles(client, org, repo, contentDir, targets) { const { log } = this; let pushed = 0; let errors = 0; - const successfullyPushed = new Set(); + // processQueue consumes its input array via shift(); copy so the caller's list survives. const results = await processQueue( - targets, + [...targets], async (daPath) => { const localPath = path.join(contentDir, ...daPath.split('/').filter(Boolean)); const ext = daPath.split('.').pop(); @@ -125,7 +125,7 @@ export default class PushCommand { const buffer = await fse.readFile(localPath); await client.putSource(org, repo, daPath, buffer, getContentType(ext)); log.info(` ✓ ${daPath}`); - return { ok: true, daPath }; + return { ok: true }; } catch (err) { log.warn(` ✗ ${daPath}: ${err.message}`); return { ok: false }; @@ -136,13 +136,12 @@ export default class PushCommand { for (const r of results) { if (r.ok) { pushed += 1; - successfullyPushed.add(r.daPath); } else { errors += 1; } } - return { pushed, errors, successfullyPushed }; + return { pushed, errors }; } /** @@ -151,21 +150,21 @@ export default class PushCommand { * @param {string} org * @param {string} repo * @param {string[]} deleted - * @returns {Promise<{ pushed: number, errors: number, successfullyDeleted: Set }>} + * @returns {Promise<{ pushed: number, errors: number }>} */ async _deleteFiles(client, org, repo, deleted) { const { log } = this; let pushed = 0; let errors = 0; - const successfullyDeleted = new Set(); + // processQueue consumes its input array via shift(); copy so the caller's list survives. const results = await processQueue( - deleted, + [...deleted], async (daPath) => { try { await client.deleteSource(org, repo, daPath); log.info(` ✓ deleted ${daPath}`); - return { ok: true, daPath }; + return { ok: true }; } catch (err) { log.warn(` ✗ ${daPath}: ${err.message}`); return { ok: false }; @@ -176,13 +175,12 @@ export default class PushCommand { for (const r of results) { if (r.ok) { pushed += 1; - successfullyDeleted.add(r.daPath); } else { errors += 1; } } - return { pushed, errors, successfullyDeleted }; + return { pushed, errors }; } async run() { @@ -207,12 +205,21 @@ export default class PushCommand { const syncedOid = await resolveSyncedOid(fs, contentDir); const lastSyncTime = await getCommitCommitterTimeMs(fs, contentDir, syncedOid); - let { added, modified, deleted } = await diffCommitTrees(fs, contentDir, syncedOid, headOid); + const fullChanges = await diffCommitTrees(fs, contentDir, syncedOid, headOid); + + const scope = this._pushPath ? this._pushPath.replace(/\/+$/, '') : null; + const inScope = (daPath) => scope === null + || daPath === scope + || daPath.startsWith(`${scope}/`); + const added = fullChanges.added.filter(inScope); + const modified = fullChanges.modified.filter(inScope); + const deleted = fullChanges.deleted.filter(inScope); - const inScope = (daPath) => !this._pushPath || daPath.startsWith(this._pushPath); - added = added.filter(inScope); - modified = modified.filter(inScope); - deleted = deleted.filter(inScope); + const fullCount = fullChanges.added.length + + fullChanges.modified.length + + fullChanges.deleted.length; + const scopeCount = added.length + modified.length + deleted.length; + const scopeIsComplete = fullCount === scopeCount; if (added.length === 0 && modified.length === 0 && deleted.length === 0) { log.info('Nothing to push. No commits ahead of the last da.live sync.'); @@ -261,27 +268,27 @@ export default class PushCommand { return; } - const putTargets = [...added, ...modified]; const { pushed: putPushed, errors: putErrors, - successfullyPushed, - } = await this._uploadFiles(client, org, repo, contentDir, putTargets); + } = await this._uploadFiles(client, org, repo, contentDir, [...added, ...modified]); const { pushed: deletePushed, errors: deleteErrors, - successfullyDeleted, } = await this._deleteFiles(client, org, repo, deleted); const pushed = putPushed + deletePushed; const pushErrors = putErrors + deleteErrors; + const allOk = pushErrors === 0; - const allPutsOk = putTargets.every((p) => successfullyPushed.has(p)); - const allDeletesOk = deleted.every((p) => successfullyDeleted.has(p)); - - if (allPutsOk && allDeletesOk) { + if (allOk && scopeIsComplete) { await writeSyncedRef(fs, contentDir, headOid); + } else if (allOk) { + log.info( + '\n--path filter held back other changes; sync ref not advanced. ' + + 'Run \'aem content push\' (without --path) to fully sync.', + ); } else { log.warn('\nSync ref not updated: fix errors and push again to finish syncing this commit.'); } diff --git a/test/content/da-api.test.js b/test/content/da-api.test.js index 2cdb4af8b..dc7be64e0 100644 --- a/test/content/da-api.test.js +++ b/test/content/da-api.test.js @@ -343,6 +343,20 @@ describe('DaClient', () => { await assert.rejects(() => client.deleteSource('org', 'repo', '/file.html'), /Unauthorized/); }); + it('throws on 5xx so callers cannot treat server failure as success', async () => { + client.fetch = async () => mockResponse(500, 'Server Error', false); + await assert.rejects( + () => client.deleteSource('org', 'repo', '/file.html'), + /DELETE failed/, + ); + }); + + it('treats 404 as success (idempotent)', async () => { + client.fetch = async () => mockResponse(404, 'Not Found', false); + const result = await client.deleteSource('org', 'repo', '/missing.html'); + assert.strictEqual(result, true); + }); + it('calls correct source URL', async () => { let calledUrl; client.fetch = async (url) => { diff --git a/test/content/merge.cmd.test.js b/test/content/merge.cmd.test.js index 6511cef0f..7b28f5a05 100644 --- a/test/content/merge.cmd.test.js +++ b/test/content/merge.cmd.test.js @@ -201,5 +201,31 @@ describe('MergeCommand', () => { const cmd = new Cmd(makeLogger()).withDirectory(testRoot).withFilePath('index.html'); await cmd.run(); }); + + it('passes the project root (not content/) to getValidToken so the cached login is reused', async () => { + const contentDir = await setupContentDir(testRoot); + await fse.writeFile( + path.join(contentDir, 'index.html'), + '

index

local extra

', + ); + + let capturedProjectDir; + const mod = await esmock('../../src/content/merge.cmd.js', { + '../../src/content/da-auth.js': { + getValidToken: async (_log, _override, projectDir) => { + capturedProjectDir = projectDir; + return 'token'; + }, + }, + '../../src/content/da-api.js': { + DaClient: createDaClientClass({ sourceContent: '

remote

' }), + }, + }); + const Cmd = mod.default; + const cmd = new Cmd(makeLogger()).withDirectory(testRoot); + await cmd.run(); + + assert.strictEqual(capturedProjectDir, testRoot); + }); }); }); diff --git a/test/content/push.cmd.test.js b/test/content/push.cmd.test.js index 0e1448f0f..924c1da53 100644 --- a/test/content/push.cmd.test.js +++ b/test/content/push.cmd.test.js @@ -12,10 +12,13 @@ /* eslint-env mocha */ import assert from 'assert'; +import fs from 'fs'; import path from 'path'; import fse from 'fs-extra'; +import git from 'isomorphic-git'; import esmock from 'esmock'; import { createTestRoot } from '../utils.js'; +import { DA_SYNCED_REF } from '../../src/content/content-git.js'; import { makeLogger, setupContentDir, @@ -249,6 +252,118 @@ describe('PushCommand', () => { assert.ok(!pushed.includes('/index.html')); }); + it('--path /blog does not match /blog2/* (no prefix bleed)', async () => { + const contentDir = await setupContentDir(testRoot); + await fse.ensureDir(path.join(contentDir, 'blog2')); + await fse.writeFile(path.join(contentDir, 'blog', 'post.html'), 'changed blog'); + await fse.writeFile(path.join(contentDir, 'blog2', 'post.html'), 'new blog2'); + await stageAllAndCommit(contentDir, 'edit blog and add blog2'); + + const pushed = []; + const DaClientClass = createDaClientClass({ onPut: (p) => pushed.push(p) }); + const log = makeLogger(); + const mod = await esmock('../../src/content/push.cmd.js', { + '../../src/content/da-auth.js': { getValidToken: async () => 'token' }, + '../../src/content/da-api.js': { + DaClient: DaClientClass, + getContentType: () => 'text/html', + }, + }); + const Cmd = mod.default; + const cmd = new Cmd(log).withDirectory(testRoot).withPath('/blog'); + await cmd.run(); + + assert.ok(pushed.includes('/blog/post.html')); + assert.ok(!pushed.includes('/blog2/post.html')); + }); + + it('does not advance synced ref when --path filter excludes other ahead changes', async () => { + const contentDir = await setupContentDir(testRoot); + await fse.writeFile(path.join(contentDir, 'index.html'), 'changed'); + await fse.writeFile(path.join(contentDir, 'blog', 'post.html'), 'changed blog'); + await stageAllAndCommit(contentDir, 'edit pages'); + + const pushed = []; + const DaClientClass = createDaClientClass({ onPut: (p) => pushed.push(p) }); + const log = makeLogger(); + const mod = await esmock('../../src/content/push.cmd.js', { + '../../src/content/da-auth.js': { getValidToken: async () => 'token' }, + '../../src/content/da-api.js': { + DaClient: DaClientClass, + getContentType: () => 'text/html', + }, + }); + const Cmd = mod.default; + const cmd = new Cmd(log).withDirectory(testRoot).withPath('/blog'); + await cmd.run(); + + // The in-scope file must still be uploaded successfully… + assert.ok(pushed.includes('/blog/post.html')); + assert.ok(!pushed.includes('/index.html')); + // …but the synced ref must not advance to HEAD, otherwise the unfiltered + // /index.html change would be silently considered already-synced next time. + const headOid = await git.resolveRef({ fs, dir: contentDir, ref: 'HEAD' }); + let syncedOid; + try { + syncedOid = await git.resolveRef({ fs, dir: contentDir, ref: DA_SYNCED_REF }); + } catch { + syncedOid = null; + } + assert.notStrictEqual(syncedOid, headOid); + assert.ok(log.logs.some((l) => l.msg.includes('held back') || l.msg.includes('without --path'))); + }); + + it('advances synced ref to HEAD when --path covers all ahead changes', async () => { + const contentDir = await setupContentDir(testRoot); + await fse.writeFile(path.join(contentDir, 'blog', 'post.html'), 'changed blog only'); + await stageAllAndCommit(contentDir, 'edit blog'); + + const mod = await esmock('../../src/content/push.cmd.js', { + '../../src/content/da-auth.js': { getValidToken: async () => 'token' }, + '../../src/content/da-api.js': { + DaClient: createDaClientClass(), + getContentType: () => 'text/html', + }, + }); + const Cmd = mod.default; + const cmd = new Cmd(makeLogger()).withDirectory(testRoot).withPath('/blog'); + await cmd.run(); + + const headOid = await git.resolveRef({ fs, dir: contentDir, ref: 'HEAD' }); + const syncedOid = await git.resolveRef({ fs, dir: contentDir, ref: DA_SYNCED_REF }); + assert.strictEqual(syncedOid, headOid); + }); + + it('does not advance synced ref when a delete fails on the server', async () => { + const contentDir = await setupContentDir(testRoot); + await fse.remove(path.join(contentDir, 'index.html')); + await stageAllAndCommit(contentDir, 'drop index'); + + const DaClientClass = createDaClientClass({ deleteFails: true }); + const log = makeLogger(); + const mod = await esmock('../../src/content/push.cmd.js', { + '../../src/content/da-auth.js': { getValidToken: async () => 'token' }, + '../../src/content/da-api.js': { + DaClient: DaClientClass, + getContentType: () => 'text/html', + }, + }); + const Cmd = mod.default; + const cmd = new Cmd(log).withDirectory(testRoot); + await cmd.run(); + + const headOid = await git.resolveRef({ fs, dir: contentDir, ref: 'HEAD' }); + let syncedOid; + try { + syncedOid = await git.resolveRef({ fs, dir: contentDir, ref: DA_SYNCED_REF }); + } catch { + syncedOid = null; + } + assert.notStrictEqual(syncedOid, headOid); + assert.ok(log.logs.some((l) => l.msg.includes('Sync ref not updated'))); + assert.ok(log.logs.some((l) => l.msg.includes('error') || l.msg.includes('✗'))); + }); + it('shows dry-run added/modified/deleted sections', async () => { const contentDir = await setupContentDir(testRoot); await fse.writeFile(path.join(contentDir, 'index.html'), 'changed');