feat: add remove high level function to client #1248

vasco-santos · 2024-01-04T12:26:52Z

This PR adds high level function for removing to the client. It essentially follows what has been done in the CLI https://github.com/web3-storage/w3cli/blob/main/index.js#L189-L237

We currently provide this feature according to the docs https://web3.storage/docs/how-to/remove/#using-the-js-client-or-cli but it is not there.

It is annoying that we have docs for something that does not exist. Other option would be to change the docs to use low level capabilities, even though that means explaining more to the users what shards are, which I think is not the best direction.

Two discussion points:

Not performing this in an atomic operation may lead to inconsistencies, where for instance part of the shards could be removed but then other operations fail... This is also the current behaviour in the CLI. We could return the result of Promise.allSettled if that would be better.
- but honestly, I think we should consider not facilitating a high level function to client as it is not an atomic op, may have mutations on partial failure, and as pointed out when part of multiple uploads may also be an issue.
CLI and docs have defaults as implemented here, but I think it would make more sense to delete shards by default in this high level function, any thoughts?

Closes #1246

Gozala · 2024-01-04T16:58:50Z

packages/w3up-client/src/client.js

+    // Remove association of content CID with selected space.
+    let upload
+    try {
+      upload = await this.capability.upload.remove(contentCID)


I would expect this to be run only after we remove all the shards instead of doing it before we remove the shards. That is because is one of the operations fails, caller will still have an opportunity to re-try. On the other hand if we remove the upload and fail to remove one of the shards, there will be no way for the user to retry or clear the shards.

If there is a good reason to do it this way can you please add code comments elaborating it. Otherwise I suggest to try removing all the shards and if they succeed only after remove the root.

There is no real reason, I basically copied what was done in CLI given that functionality is there. I will see where did the change go introduced and ask for feedback to align on it

Ok, so this code was added over a year ago storacha/w3cli#20

I guess we would be ok with changing for suggested, but let me ask @olizilla if there is any reason on their head

Honestly, I think we could consider not facilitating a high level function to client as it is not an atomic op, may have mutations on partial failure, and as pointed out when part of multiple uploads may also be an issue.

Leaving this for the advanced function prefixed by capabilities could be a way to deal with this, but we would need to explain users upload and store, and update the docs (and of course make CLI no delete like this, but also in a granular way)

Gozala

I agree it makes more sense to have this functionality inside a as opposed to just CLI. I do worry however about the data corruption and would love us to do something on the backend to prevent that from happening e.g. instead of store/remove we could have store/unlink that could remove a link between upload and shard and delete actual CAR only if whet number of links is 0. That would require changing our DB so that shard to upload relations are stored separate from uploads, but that is something we have been considering already due to other issues we encountered with the current design (where vast number of shards can lead to record size limit).

In the immediate future we could probably just add big warning sign around this API and make it a user problem.

I have requested changes, but I do not think we need to re-review this. The change I'm referring to is in the inline comments tldr; lets remove shards first so that on failure user still can retry or if there is a reason not to lets document it in the code so it's clear to the code maintainer.

packages/w3up-client/src/client.js

Gozala · 2024-01-04T17:49:50Z

This inspired me to write up following proposal #1249

alanshaw · 2024-01-23T11:48:16Z

@vasco-santos can you fix this up as @Gozala requested and deploy (in next CIC)? We'll redesign the relations later.

vasco-santos · 2024-01-23T11:52:32Z

@alanshaw I am more tempted to move with my latest thoughts:

#1248 (comment)

Remove this function and the docs for it should be updated for low level capabilities until we figure this out. However, maybe CLI should also not enable this directly. What do you think?

alanshaw · 2024-01-23T12:28:34Z

The upload functions are also non-atomic - they make multiple invocations. The issue IMHO is the chance of removing a shard used in a different upload, not failing to perform the operation atomically.

This functionality should always have been in the client and removing the upload after the stores is the better method.

If we don't add it to the client then developers are just going to do it themselves and might get it wrong, so we should give them a method that does it right.

We can then improve on the design later!

Either way I would like either the code to be updated or the docs to be fixed asap, since it has been 3 weeks since it was opened! 🙏

vasco-santos · 2024-01-23T15:59:39Z

The upload functions are also non-atomic - they make multiple invocations. The issue IMHO is the chance of removing a shard used in a different upload, not failing to perform the operation atomically.

Yes, but they are not destructive 😬 here if it fails mid-way, there is no way to retry because Upload is gone... Upload is iterative adds, so is fine. Anyway, we can deal with this later on the redesign given CLI already offers this behaviour anyway,

Co-authored-by: Irakli Gozalishvili <[email protected]>

packages/w3up-client/README.md

packages/w3up-client/src/client.js

alanshaw · 2024-01-24T09:29:22Z

packages/w3up-client/src/client.js

+      // Remove association of content CID with selected space.
+      try {
+        await this.capability.upload.remove(contentCID)
+        /* c8 ignore start */


Where does it stop?

alanshaw · 2024-01-24T09:29:42Z

packages/w3up-client/src/client.js

+    // Remove association of content CID with selected space.
+    try {
+      await this.capability.upload.remove(contentCID)
+      /* c8 ignore start */


Where does it stop?

alanshaw · 2024-01-24T09:30:18Z

packages/w3up-client/src/client.js

+      /* c8 ignore start */
+    } catch (/** @type {any} */ err) {
+      throw new Error(`remove failed for ${contentCID}: ${err.message ?? err}`)
+    }


Personally I'd wrap a conditional around the shard deleting bit and not repeat this try/catch twice in this same function.

alanshaw · 2024-01-24T09:31:38Z

packages/w3up-client/src/client.js

+            throw new Error(
+              `remove failed for shard ${shard.link()} from ${contentCID}: ${
+                error?.message
+              }`
+            )


🙏 don't smother the stack and the cause.

Suggested change

throw new Error(

`remove failed for shard ${shard.link()} from ${contentCID}: ${

error?.message

}`

)

throw new Error(`failed to remove shard: ${shard}`, { cause: error })

IMHO, only catch errors and re-throw to add information that it is not possible for the caller to know already. This is the perfect case where this needs to happen because the caller won't be able to identify which shard failed to be removed. However we don't need to include contentCID in the error message, since that is what the caller asked to be removed. The caller should never rely on context they pass to the function to be part of the error message, since the function may throw for other reasons that weren't annotated this way. So it is on the caller to add their context to the error (or log it with appropriate context).

alanshaw · 2024-01-24T09:38:34Z

packages/w3up-client/src/client.js

+      upload = await this.capability.upload.get(contentCID)
+      /* c8 ignore start */
+    } catch (/** @type {any} */ err) {
+      throw new Error(`remove failed for ${contentCID}: ${err.message ?? err}`)


I would just let this throw - we don't have anything to add here that the caller does not already know.

alanshaw · 2024-01-24T09:39:22Z

packages/w3up-client/src/client.js

+    }
+    /* c8 ignore start */
+    if (!upload.shards || !upload.shards.length) {
+      return


We still need to remove the upload.

alanshaw · 2024-01-24T09:40:14Z

packages/w3up-client/src/client.js

+    /* c8 ignore stop */
+
+    // Check if we have information about the shards to remove
+    if (!upload.root) {


Why would we not have root? That field is mandatory no?

copied from CLI code, bot looks like not needed. Going to remove

alanshaw · 2024-01-24T09:41:11Z

packages/w3up-client/src/client.js

+      await this.capability.upload.remove(contentCID)
+      /* c8 ignore start */
+    } catch (/** @type {any} */ err) {
+      throw new Error(`remove failed for ${contentCID}: ${err.message ?? err}`)


Again, I would just let this throw - we don't have anything to add here that the caller does not already know.

Co-authored-by: Alan Shaw <[email protected]>

alanshaw

Thank you ❤️

🤖 I have created a release *beep* *boop* --- ## [12.2.0](w3up-client-v12.1.0...w3up-client-v12.2.0) (2024-01-24) ### Features * add remove high level function to client ([#1248](#1248)) ([104b8de](104b8de)) ### Fixes * update "byo agent" example to match website snippet ([#1268](#1268)) ([e34eed1](e34eed1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

vasco-santos force-pushed the feat/add-remove-high-level-function-to-client branch 2 times, most recently from bc2a0a9 to 25f300d Compare January 4, 2024 13:03

vasco-santos marked this pull request as ready for review January 4, 2024 13:12

vasco-santos requested review from travis and Gozala January 4, 2024 13:12

vasco-santos mentioned this pull request Jan 4, 2024

Property 'remove' does not exist on type 'Client' #1246

Closed

Gozala reviewed Jan 4, 2024

View reviewed changes

Gozala requested changes Jan 4, 2024

View reviewed changes

packages/w3up-client/src/client.js Show resolved Hide resolved

vasco-santos and others added 3 commits January 23, 2024 16:59

feat: add remove high level function to client

af2d67d

chore: add description warning

884c8bf

Co-authored-by: Irakli Gozalishvili <[email protected]>

docs: update readme

ef2c6f2

vasco-santos force-pushed the feat/add-remove-high-level-function-to-client branch from c86d96c to ef2c6f2 Compare January 23, 2024 16:00

vasco-santos requested review from Gozala and alanshaw January 23, 2024 16:04

vasco-santos force-pushed the feat/add-remove-high-level-function-to-client branch 3 times, most recently from 14c9858 to 2fdc7e6 Compare January 24, 2024 09:19

fix: do upload get before remove

dd5df89

vasco-santos force-pushed the feat/add-remove-high-level-function-to-client branch from 2fdc7e6 to dd5df89 Compare January 24, 2024 09:21

alanshaw requested changes Jan 24, 2024

View reviewed changes

vasco-santos and others added 3 commits January 24, 2024 13:17

chore: apply suggestions from code review

122b6e8

Co-authored-by: Alan Shaw <[email protected]>

chore: address review comments

58da6ec

chore: appease lint

4293a48

vasco-santos requested a review from alanshaw January 24, 2024 13:03

alanshaw approved these changes Jan 24, 2024

View reviewed changes

vasco-santos merged commit 104b8de into main Jan 24, 2024
3 checks passed

vasco-santos deleted the feat/add-remove-high-level-function-to-client branch January 24, 2024 16:12

it-dag-house mentioned this pull request Jan 24, 2024

chore(main): release w3up-client 12.2.0 #1270

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add remove high level function to client #1248

feat: add remove high level function to client #1248

vasco-santos commented Jan 4, 2024 •

edited

Loading

Gozala Jan 4, 2024

vasco-santos Jan 4, 2024 •

edited

Loading

vasco-santos Jan 4, 2024

vasco-santos Jan 4, 2024

Gozala left a comment

Gozala commented Jan 4, 2024

alanshaw commented Jan 23, 2024 •

edited

Loading

vasco-santos commented Jan 23, 2024

alanshaw commented Jan 23, 2024

vasco-santos commented Jan 23, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw Jan 24, 2024

vasco-santos Jan 24, 2024

alanshaw Jan 24, 2024

alanshaw left a comment

feat: add remove high level function to client #1248

feat: add remove high level function to client #1248

Conversation

vasco-santos commented Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

vasco-santos Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Gozala commented Jan 4, 2024

alanshaw commented Jan 23, 2024 • edited Loading

vasco-santos commented Jan 23, 2024

alanshaw commented Jan 23, 2024

vasco-santos commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw left a comment

Choose a reason for hiding this comment

vasco-santos commented Jan 4, 2024 •

edited

Loading

vasco-santos Jan 4, 2024 •

edited

Loading

alanshaw commented Jan 23, 2024 •

edited

Loading