feat(source): delete moves to trash by bosschaert · Pull Request #170 · adobe/helix-api-service

bosschaert · 2026-04-02T12:24:45Z

Deleting a document moves it to the .trash folder at the root of the site storage. If there is already a document with the same name a unique suffix is added to the document name.
Deleting a folder moves all of its contents recursively to the .trash under a folder with the same name. If a folder with this name already exists in the trash a unique suffix is added.

Each document moved to the trash has a new metadata property 'doc-path' with the path it was originally located at.

Also included here, the returned value from a copy operation now doesn't include org and site components of the paths any more, as these are not in the caller space:

So:

{"copied: [{"src":"bosschaert/my-eds-site/mypath/sub2/foo.html","dst":"bosschaert/my-eds-site/mypath/sub4/foo.html"}]}

is now

{"copied: [{"src":"/mypath/sub2/foo.html","dst":"/mypath/sub4/foo.html"}]}

Related Issues

Contributes to: #169

* Deleting a document moves it to the .trash folder at the root of the site storage. If there is already a document with the same name a unique suffix is added to the document name. * Deleting a folder moves all of its content to the .trash under a folder with the same name. If a folder with this name already exists in the trash a unique suffix is added. Each document moved to the trash has a new metadata property 'doc-path' with the path it was originally located at. Contributes to: #169

github-actions · 2026-04-02T15:49:45Z

This PR will trigger a minor release when merged.

When a file is moved (or copied) with the 'unique' collision option configured, it will be given a unique name by adding a suffix. This commit moves that suffix inside the extension rather than appending it, so for example if the move is done on page1.html to the .trash, and there is already a file called page1.html in the trash, the newly trashed document will be called page1-abfsg7h1.html

tripodsan · 2026-04-07T09:44:44Z

+        delimiter: '/',
+        prefix: 'org1/site2/.trash/b/',
+      })
+      .reply(200, Buffer.from(BUCKET_LIST_EMPTY_TRASH));


why don't use you nock.listObjects() ? or nock.listFolder() ?

You mean add listObjects() or listFolder() to the Nock implementation? Because I don't see them there at this time...

I think you need to merge main to see them, they were added some time after you started working on your branch.

Ah thanks! I have updated the test to use nock.listObjects() 👍

There are some existing tests that use the old approach. Maybe we should convert them in a seperate PR to avoid this one growing too big...?

tripodsan

i have the feeling the code get more and morel over the place. you start exporting common methods in put that are used else where.

maybe it would make sense to separate the source-bus operations from the method handlers. i.e. create a source-bus client.

I would also create a source bus "path" class, that encapsulates all the from/to key,path,name, etc methods.

I also don't like the every growing argument list for the copy operations. maybe this can be consolidated in a combined "CopyOptions" argument (including the "move" flag)

Co-authored-by: Tobias Bocanegra <tripod@bocanegra.ch>

…lass Replace individual parameters (src, info, move, opts/fnOpts, collOpts) with a single CopyOptions object for both copyDocument and copyFolder, keeping only context as a separate argument. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Move getS3Key, getS3KeyFromInfo, and getDocPathFromS3Key out of utils.js into a dedicated s3-path-utils module. Add tests for all three functions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

bosschaert · 2026-04-16T11:23:38Z

Hi @tripodsan , thanks for the comments!

i have the feeling the code get more and morel over the place. you start exporting common methods in put that are used else where.

maybe it would make sense to separate the source-bus operations from the method handlers. i.e. create a source-bus client.

I have created source-client.js that now contains common methods dealing directly with the source bus.

I would also create a source bus "path" class, that encapsulates all the from/to key,path,name, etc methods.

Created s3-paths-utils.js with the S3 path conversion functions.

I also don't like the every growing argument list for the copy operations. maybe this can be consolidated in a combined "CopyOptions" argument (including the "move" flag)

I have added a CopyOptions class to source-client.js for this purpose.

tripodsan · 2026-04-20T07:44:12Z

+  // Trash a document.
+  const docName = info.rawPath.split('/').pop();
+  const srcKey = getS3KeyFromInfo(info);
+  const newInfo = RequestInfo.clone(info, { path: `/.trash/${docName}` });


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

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:

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):

tripodsan · 2026-04-20T10:14:44Z

+          // We do this by appending a ms timestamp to the destination key.
+
+          const ext = `.${destKey.split('.').pop()}`;
+          const destWithoutExt = destKey.slice(0, -ext.length);


isn't this the same as path.basename ?

We just want the path to the file, without the extension (so we can insert the unique string). So if we had o1/s1/to.html we want o1/s1/to. With path:

path.basename would give me just to.html

path.dirname gives me o1/s1

path.extname gives me .html
I guess I could use these to get the same, but it wouldn't be much simpler I think and I would have to import path...

tripodsan

break up copyWithRetry

tripodsan · 2026-04-20T10:16:26Z

+ * - 'unique' - append a (base-36) encoded timestamp to the destination key to make it unique.
+ * These timestamps are alphabetically sortable.
+ */
+async function copyWithRetry(


this is very unwieldy. I would break this:

one function that does the retry and one function that does the copy

the copy should be further split in all the collision handling functions

dominique-pfister · 2026-04-20T11:03:58Z

+function getUser(context) {
+  const email = context.attributes.authInfo?.profile?.email;
+
+  return email || 'anonymous';
+}


there is no way the authInfo is not set. aren't you looking for:

const { authInfo } = context; return authInfo.resolveEmail() || 'anonymous';

?

Good point - I changed this.

bosschaert · 2026-04-20T16:32:40Z

-    const opts = { e, log };
-    opts.status = e.status;
+    const opts = { e, log: context.log };
+    opts.status = e.$metadata?.httpStatusCode;


@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?

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

dominique-pfister · 2026-04-23T12:34:32Z

+  }
+
+  if (collOpts.collision === 'overwrite') {
+    const { mdOpts: mdo, copyOpts } = await versionBeforeOverwrite(context, destinationKey, mdOpts);


It seems counterintuitive that a method called getCopyRetryOpts silently creates a version before returning.

Good point - i've renamed it to handleCopyRetry(), hope that works?

dominique-pfister · 2026-04-23T12:35:28Z

+ * @param {import('../support/RequestInfo').RequestInfo} info request info
+ * @return {Promise<Response>} response, status 204 if successful.
 */
 export async function deleteSource(context, info) {


I wouldn't call it deleteSource, as it's actually a moving a source to trash, may be call it trashSource?

I actually had this for a while and then changed it back. Let me re-apply that name change.

dominique-pfister · 2026-04-23T12:37:44Z

+  const copyOptions = new CopyOptions({
+    src: srcKey, info: newInfo, move: true, opts: copyOpts, collOpts: { collision: 'unique' },
+  });


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?

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.

* rename deleteSource() to trashSource() * rename getCopyRetryOpts() to handleCopyRetry()

As suggested in the feedback comment.

bosschaert added 2 commits April 2, 2026 13:18

test: more unit tests

1cbfdc2

test: more unit tests

73ae591

bosschaert marked this pull request as ready for review April 2, 2026 15:54

bosschaert requested review from dominique-pfister and tripodsan April 7, 2026 08:13

tripodsan reviewed Apr 7, 2026

View reviewed changes

Comment thread src/source/delete.js Outdated

tripodsan reviewed Apr 7, 2026

View reviewed changes

tripodsan requested changes Apr 7, 2026

View reviewed changes

bosschaert and others added 9 commits April 13, 2026 13:43

PR review improvement

cc20b84

Co-authored-by: Tobias Bocanegra <tripod@bocanegra.ch>

Merge branch 'main' into trash

c3fee05

refactor: move common source bus access methods to source-client.js

141a505

Merge remote-tracking branch 'origin/trash' into trash

fd0a5b7

refactor: extract S3 path functions into s3-path-utils.js

53ce6b1

Move getS3Key, getS3KeyFromInfo, and getDocPathFromS3Key out of utils.js into a dedicated s3-path-utils module. Add tests for all three functions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Merge branch 'main' into trash

4ee29d1

Update test to use nock.listObjects()

f01ff51

refactor: small tweak

f920383

bosschaert requested a review from tripodsan April 16, 2026 11:26

tripodsan reviewed Apr 20, 2026

View reviewed changes

tripodsan requested changes Apr 20, 2026

View reviewed changes

dominique-pfister reviewed Apr 20, 2026

View reviewed changes

bosschaert added 3 commits April 20, 2026 16:06

refactor: split up copyWithRetry()

0e82e1a

refactor: use authInfo.resolveEmail()

5b2a03a

Merge branch 'main' into trash

ab932b3

bosschaert commented Apr 20, 2026

View reviewed changes

bosschaert added 2 commits April 21, 2026 12:11

test: increase line coverage from 99.96% to 100%

18589bd

chore: small rename

c00a35b

bosschaert requested review from dominique-pfister and tripodsan April 21, 2026 12:23

dominique-pfister reviewed Apr 23, 2026

View reviewed changes

bosschaert added 4 commits April 23, 2026 15:17

refactor: function renames

dab66df

* rename deleteSource() to trashSource() * rename getCopyRetryOpts() to handleCopyRetry()

refactor: split copy and move into separate methods

5d1ae05

As suggested in the feedback comment.

trivial: small tweak

4656c49

fix: preserve existing metadata of copy options

c3309f9

bosschaert requested a review from dominique-pfister April 24, 2026 14:22

Conversation

bosschaert commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

Uh oh!

github-actions Bot commented Apr 2, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bosschaert Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tripodsan left a comment

Choose a reason for hiding this comment

Uh oh!

bosschaert commented Apr 16, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tripodsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominique-pfister Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bosschaert commented Apr 2, 2026 •

edited

Loading

bosschaert Apr 16, 2026 •

edited

Loading

dominique-pfister Apr 20, 2026 •

edited

Loading