Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
fe9d002
feat(source): delete moves to trash
bosschaert Apr 2, 2026
1cbfdc2
test: more unit tests
bosschaert Apr 2, 2026
73ae591
test: more unit tests
bosschaert Apr 2, 2026
5457497
fix: move the unique suffix inside the file extension
bosschaert Apr 2, 2026
cc20b84
PR review improvement
bosschaert Apr 13, 2026
c3fee05
Merge branch 'main' into trash
bosschaert Apr 13, 2026
141a505
refactor: move common source bus access methods to source-client.js
bosschaert Apr 13, 2026
fd0a5b7
Merge remote-tracking branch 'origin/trash' into trash
bosschaert Apr 13, 2026
c903e01
refactor: consolidate copyDocument/copyFolder args into CopyOptions c…
bosschaert Apr 13, 2026
53ce6b1
refactor: extract S3 path functions into s3-path-utils.js
bosschaert Apr 13, 2026
4ee29d1
Merge branch 'main' into trash
bosschaert Apr 16, 2026
f01ff51
Update test to use nock.listObjects()
bosschaert Apr 16, 2026
f920383
refactor: small tweak
bosschaert Apr 16, 2026
0e82e1a
refactor: split up copyWithRetry()
bosschaert Apr 20, 2026
5b2a03a
refactor: use authInfo.resolveEmail()
bosschaert Apr 20, 2026
ab932b3
Merge branch 'main' into trash
bosschaert Apr 20, 2026
18589bd
test: increase line coverage from 99.96% to 100%
bosschaert Apr 21, 2026
c00a35b
chore: small rename
bosschaert Apr 21, 2026
dab66df
refactor: function renames
bosschaert Apr 23, 2026
5d1ae05
refactor: split copy and move into separate methods
bosschaert Apr 24, 2026
4656c49
trivial: small tweak
bosschaert Apr 24, 2026
c3309f9
fix: preserve existing metadata of copy options
bosschaert Apr 24, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 58 additions & 13 deletions src/source/delete.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,31 +12,76 @@
import { Response } from '@adobe/fetch';
import { HelixStorage } from '@adobe/helix-shared-storage';
import { createErrorResponse } from '../contentbus/utils.js';
import { deleteFolder } from './folder.js';
import { getS3KeyFromInfo } from './utils.js';
import { RequestInfo } from '../support/RequestInfo.js';
import { CopyOptions, moveDocument, moveFolder } from './source-client.js';
import { getDocPathFromS3Key, getS3Key, getS3KeyFromInfo } from './s3-path-utils.js';

/**
* Delete from the source bus.
* Trash a folder by moving all of its contents to the trash in the same folder structure.
* If the trash already contains a folder with this name, a base-36 encoded timestamp is appended.
*
* @param {import('../support/AdminContext').AdminContext} context context
* @param {import('../support/RequestInfo').RequestInfo} info request info
* @return {Promise<Response>} response
* @returns {Promise<Response>} response, status 204 if successful.
*/
export async function deleteSource(context, info) {
async function trashFolder(context, info) {
const bucket = HelixStorage.fromContext(context).sourceBus();

const destDir = `/.trash/${info.rawPath.split('/').at(-2)}`;

// Ensure that there is no folder in the trash with this name yet
const listResp = await bucket.list(`${getS3Key(info.org, info.site, destDir)}/`, { shallow: true });
const destPath = listResp.length > 0 ? `${destDir}-${Date.now().toString(36)}/` : `${destDir}/`;

const srcKey = getS3Key(info.org, info.site, info.rawPath);
const newInfo = RequestInfo.clone(info, { path: destPath });
const copyOpts = (sKey) => ({ addMetadata: { 'doc-path': getDocPathFromS3Key(sKey) } });

try {
await moveFolder(context, new CopyOptions({
src: srcKey, info: newInfo, fnOpts: copyOpts, collOpts: { collision: 'unique' },
}));
return new Response('', { status: 204 });
} catch (e) {
const opts = { e, log: context.log };
opts.status = e.$metadata?.httpStatusCode;
return createErrorResponse(opts);
}
}

/**
* Delete from the source bus, which means moving it to the trash. Both
* documents and folders are supported. The trashed documents gets an extra
* metadata field 'doc-path' which is the path where it was deleted from.
*
* @param {import('../support/AdminContext').AdminContext} context context
* @param {import('../support/RequestInfo').RequestInfo} info request info
* @return {Promise<Response>} response, status 204 if successful.
*/
export async function trashSource(context, info) {
if (info.rawPath.endsWith('/')) {
return deleteFolder(context, info);
return trashFolder(context, info);
}
const { log } = context;

const bucket = HelixStorage.fromContext(context).sourceBus();
const key = getS3KeyFromInfo(info);
// Trash a document.
const docName = info.rawPath.split('/').pop();
const srcKey = getS3KeyFromInfo(info);
const newInfo = RequestInfo.clone(info, { path: `/.trash/${docName}` });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this desired? why don't you include the path somehow? otherwise, deleting common files, like index.html will end up overwriting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to how a Trash works in an OS environment, e.g. on Mac OS individually deleted files end up in the root of the trash, if there is already a file with that name, they get a unique suffix:

Image

We do a similar thing. So files are not overwritten, but if one with the name already exists, it will get a unique suffix (alpha sortable to get the order in which they are deleted):

Image

const copyOpts = {
addMetadata: {
'doc-path': info.resourcePath,
},
};
const copyOptions = new CopyOptions({
src: srcKey, info: newInfo, opts: copyOpts, collOpts: { collision: 'unique' },
});
Comment on lines +75 to +77
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a late remark, but I wonder whether the deletion of the source in a move shouldn't be done by this caller instead of passing it down to an implementation that looks at move and then deletes the source. Or is the protocol for copying - depending on whether it should be a move - completely different? Then may be use 2 methods instead of one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment!
I've changed the implementation so that move is no longer a parameter (which is nice as it reduces the amount of options) and now calls copy and then delete.
One thing was a little interesting, on a copy the document gets a new document ID, but on a move this doesn't happen, so the outer copy now does this and the move calls a copy one level after.


try {
const resp = await bucket.remove(key);
return new Response('', { status: resp.$metadata?.httpStatusCode });
await moveDocument(context, copyOptions);
return new Response('', { status: 204 });
} catch (e) {
const opts = { e, log };
opts.status = e.status;
const opts = { e, log: context.log };
opts.status = e.$metadata?.httpStatusCode;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dominique-pfister since delete now doesn't do bucket.remove() any more but rather a copy, we need to go back to checking e.$metadata?.httpStatusCode am I correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so: AFAICS, Bucket operations catch all errors thrown and rethrow an exception where the information in $metadata is available in the exception, e.g.:
https://github.com/adobe/helix-shared/blob/726a38139542da81fc0a1b26a414339b8ba6562b/packages/helix-shared-storage/src/storage.js#L430-L438

return createErrorResponse(opts);
}
}
4 changes: 3 additions & 1 deletion src/source/folder.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import { sanitizePath } from '@adobe/helix-shared-string';
import { createErrorResponse } from '../contentbus/utils.js';
import { splitExtension } from '../support/RequestInfo.js';
import { StatusCodeError } from '../support/StatusCodeError.js';
import { getS3Key, storeSourceFile, CONTENT_TYPES } from './utils.js';
import { getS3Key } from './s3-path-utils.js';
import { storeSourceFile } from './source-client.js';
import { CONTENT_TYPES } from './utils.js';

/**
* A folder is marked by a marker file. This allows folder to show up in bucket
Expand Down
5 changes: 3 additions & 2 deletions src/source/get.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@
*/
import { createErrorResponse } from '../contentbus/utils.js';
import { listFolder } from './folder.js';
import { accessSourceFile, getS3KeyFromInfo } from './utils.js';
import { getOrListVersions, VERSION_FOLDER } from './versions.js';
import { accessSourceFile, VERSION_FOLDER } from './source-client.js';
import { getS3KeyFromInfo } from './s3-path-utils.js';
import { getOrListVersions } from './versions.js';

async function accessSource(context, info, headRequest) {
if (info.rawPath.endsWith('/')) {
Expand Down
4 changes: 2 additions & 2 deletions src/source/handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* OF ANY KIND, either express or implied. See the License for the specific language
* governing permissions and limitations under the License.
*/
import { deleteSource } from './delete.js';
import { trashSource } from './delete.js';
import { getSource, headSource } from './get.js';
import { postSource } from './post.js';
import { putSource } from './put.js';
Expand All @@ -32,7 +32,7 @@ export default async function handle(context, info) {
case 'HEAD':
return headSource(context, info);
case 'DELETE':
return deleteSource(context, info);
return trashSource(context, info);
default:
return new Response('method not allowed', { status: 405 });
}
Expand Down
12 changes: 4 additions & 8 deletions src/source/post.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,9 @@
import { createErrorResponse } from '../contentbus/utils.js';
import { createFolder } from './folder.js';
import { checkConditionals } from './header-utils.js';
import {
contentTypeFromExtension,
getS3KeyFromInfo,
getValidPayload,
storeSourceFile,
} from './utils.js';
import { postVersion, VERSION_FOLDER } from './versions.js';
import { getS3KeyFromInfo } from './s3-path-utils.js';
import { storeSourceFile, createVersion, VERSION_FOLDER } from './source-client.js';
import { contentTypeFromExtension, getValidPayload } from './utils.js';

/**
* Handle POST requests to the source bus.
Expand All @@ -38,7 +34,7 @@ export async function postSource(context, info) {
const operation = String(context.data.operation || '');
const comment = String(context.data.comment || '');

return postVersion(context, baseKey, operation, comment);
return createVersion(context, baseKey, operation, comment);
}

try {
Expand Down
201 changes: 28 additions & 173 deletions src/source/put.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,172 +10,13 @@
* governing permissions and limitations under the License.
*/
import { Response } from '@adobe/fetch';
import processQueue from '@adobe/helix-shared-process-queue';
import { HelixStorage } from '@adobe/helix-shared-storage';
import { ulid } from 'ulid';
import { createErrorResponse } from '../contentbus/utils.js';
import { StatusCodeError } from '../support/StatusCodeError.js';
import { checkConditionals } from './header-utils.js';
import { getDocPathFromS3Key, getS3Key, getS3KeyFromInfo } from './s3-path-utils.js';
import {
contentTypeFromExtension,
getS3KeyFromInfo,
getS3Key,
getDocID,
getValidPayload,
storeSourceFile,
MAX_SOURCE_BUCKET_RETRY,
} from './utils.js';
import { postVersion } from './versions.js';

/**
* Copy an S3 object and handle conflichts.
*
* @param {import('../support/AdminContext').AdminContext} context context
* @param {string} srcKey source S3 key
* @param {string} destKey destination S3 key
* @param {boolean} move true if this is a move operation
* @param {object} initialOpts metadata options for the copy operation
* @param {object} collOpts collision options (e.g { copy: 'overwrite' } )
*/
async function copyWithRetry(
context,
srcKey,
destKey,
move,
initialOpts,
collOpts,
) {
const bucket = HelixStorage.fromContext(context).sourceBus();
let opts = initialOpts;

// We start with assuming that there is nothing at the destination, the happy path
let copyOpts = { IfNoneMatch: '*' };

const maxRetry = context.attributes.maxSourceBucketRetry ?? MAX_SOURCE_BUCKET_RETRY;
let attempt = 0;
while (true) {
try {
const allOpts = { copyOpts, ...opts };
// eslint-disable-next-line no-await-in-loop
await bucket.copy(srcKey, destKey, allOpts);

break; // copy was successful, break out of the loop - we're done!
} catch (e) {
attempt += 1;
if (attempt > maxRetry) {
throw e;
}

const status = e.$metadata?.httpStatusCode;

// As per S3 docs, retry on a 409
if (status !== 409) {
if (status !== 412) {
throw e;
}
// 412: precondition failed - something is at the destination already.

if (move) {
// TODO add move collision handling
throw new StatusCodeError('Collision: something is at the destination already', 409);
} else {
if (collOpts.copy !== 'overwrite') {
throw new StatusCodeError('Collision: something is at the destination already, no overwrite option provided', 409);
}

// eslint-disable-next-line no-await-in-loop
const dest = await bucket.head(destKey);

// version what's there before overwriting it, provide the destination ETag so that we
// know we're versioning what we just did a head() of.
// eslint-disable-next-line no-await-in-loop
const versionResp = await postVersion(context, destKey, 'copy', 'Version created before overwrite', dest.ETag);
if (versionResp.status !== 201) {
if (versionResp.status !== 412 && versionResp.status !== 409) {
throw new StatusCodeError('Failed to version the destination', versionResp.status);
}
} else {
// Creating the version was successful, so we can now copy over the destination.

const getDestDocId = getDocID(dest);

// If something is at the destination already, we copy over that file, but keep
// the doc ID from the destination as-is so that the destination keeps its history.
opts = { ...initialOpts, addMetadata: { 'doc-id': getDestDocId } };

// Now only copy over the destination if it's still the same as what we did a head() of
copyOpts = { IfMatch: dest.ETag };
}
}
}
}
}

if (move) {
const resp = await bucket.remove(srcKey);
if (resp.$metadata?.httpStatusCode !== 204) {
throw new StatusCodeError(`Failed to remove source: ${srcKey}`, resp.$metadata?.httpStatusCode);
}
}
}

async function copyFile(context, srcKey, destKey, move, collOpts) {
const opts = {};
if (!move) {
opts.addMetadata = { 'doc-id': ulid() };
}
await copyWithRetry(context, srcKey, destKey, move, opts, collOpts);
}

/**
* Copies a document from the source to the destination.
*
* @param {import('../support/AdminContext').AdminContext} context context
* @param {string} src source S3 key
* @param {import('../support/RequestInfo').RequestInfo} info destination info
* @param {boolean} move whether to move the source
* @param {object} collOpts collision options
* @returns {Promise<Array<{src: string, dst: string}>>} the copied file details
*/
async function copyDocument(context, src, info, move, collOpts) {
const dst = getS3KeyFromInfo(info);
await copyFile(context, src, dst, move, collOpts);
return [{ src, dst }];
}

/**
* Copies a folder from the source to the destination.
*
* @param {import('../support/AdminContext').AdminContext} context context
* @param {string} srcKey source S3 key
* @param {import('../support/RequestInfo').RequestInfo} info destination info
* @param {boolean} move whether to move the source
* @param {object} collOpts collision options
* @returns {Promise<Array<{src: string, dst: string}>>} the copied files
*/
async function copyFolder(context, srcKey, info, move, collOpts) {
const tasks = [];
const destKey = getS3Key(info.org, info.site, info.rawPath);

if (destKey.startsWith(srcKey)) {
throw new StatusCodeError('Destination cannot be a subfolder of source', 400);
}

const bucket = HelixStorage.fromContext(context).sourceBus();
(await bucket.list(srcKey)).forEach((obj) => {
tasks.push({
src: obj.key,
dst: `${destKey}${obj.path}`,
});
});

const copied = [];
await processQueue(tasks, async (task) => {
await copyFile(context, task.src, task.dst, move, collOpts);
copied.push({ src: task.src, dst: task.dst });
});
return copied;
}
CopyOptions, copyFolder, copyDocument, moveDocument, moveFolder, storeSourceFile,
} from './source-client.js';
import { contentTypeFromExtension, getValidPayload } from './utils.js';

/**
* Copies a resource of a folder to the destination folder. If a folder is
Expand All @@ -198,13 +39,30 @@ async function copySource(context, info, move, collOpts) {
return createErrorResponse({ status: 400, msg: 'Source and destination type mismatch', log });
}

const copied = isFolder
? await copyFolder(context, srcKey, info, move, collOpts)
: await copyDocument(context, srcKey, info, move, collOpts);
const copyOpts = new CopyOptions({ src: srcKey, info, collOpts });

let copied;
if (isFolder) {
if (move) {
copied = await moveFolder(context, copyOpts);
} else {
copied = await copyFolder(context, copyOpts);
}
} else if (move) {
copied = await moveDocument(context, copyOpts);
} else {
copied = await copyDocument(context, copyOpts);
}

// The copied paths returned are without the org and site segments
const copiedPaths = copied.map((c) => ({
src: getDocPathFromS3Key(c.src),
dst: getDocPathFromS3Key(c.dst),
}));

const operation = move ? 'moved' : 'copied';
return new Response({
[operation]: copied,
[operation]: copiedPaths,
});
} catch (e) {
const opts = { e, log };
Expand All @@ -223,12 +81,9 @@ async function copySource(context, info, move, collOpts) {
export async function putSource(context, info) {
if (context.data.source) {
const move = String(context.data.move) === 'true';
const collOpts = {};
if (move) {
collOpts.move = context.data.collision;
} else {
collOpts.copy = context.data.collision;
}
const collOpts = {
collision: context.data.collision,
};
return copySource(context, info, move, collOpts);
}

Expand Down
Loading
Loading