From dd5df892e5527b9c08f7d00e8ecd0561393617ce Mon Sep 17 00:00:00 2001 From: Vasco Santos Date: Wed, 24 Jan 2024 10:16:09 +0100 Subject: [PATCH] fix: do upload get before remove --- packages/w3up-client/README.md | 2 +- packages/w3up-client/src/client.js | 48 +++++++++-- packages/w3up-client/test/client.test.js | 105 ++++++++++++++++++++++- 3 files changed, 144 insertions(+), 11 deletions(-) diff --git a/packages/w3up-client/README.md b/packages/w3up-client/README.md index 4fc5daefd..125f53125 100644 --- a/packages/w3up-client/README.md +++ b/packages/w3up-client/README.md @@ -603,7 +603,7 @@ function remove ( options: { shards?: boolean } = {} -): Promise +): Promise ``` Removes association of a content CID with the space. Optionally, also removes association of CAR shards with space. diff --git a/packages/w3up-client/src/client.js b/packages/w3up-client/src/client.js index d0b1b9f1e..a2db04f36 100644 --- a/packages/w3up-client/src/client.js +++ b/packages/w3up-client/src/client.js @@ -324,21 +324,30 @@ export class Client extends Base { * @param {boolean} [options.shards] */ async remove(contentCID, options = {}) { - // Remove association of content CID with selected space. + // Shortcut if there is no request to remove shards + if (!options.shards) { + // Remove association of content CID with selected space. + try { + await this.capability.upload.remove(contentCID) + /* c8 ignore start */ + } catch (/** @type {any} */ err) { + throw new Error( + `remove failed for ${contentCID}: ${err.message ?? err}` + ) + } + return + } + + // Get shards associated with upload. let upload try { - upload = await this.capability.upload.remove(contentCID) + upload = await this.capability.upload.get(contentCID) /* c8 ignore start */ } catch (/** @type {any} */ err) { throw new Error(`remove failed for ${contentCID}: ${err.message ?? err}`) } /* c8 ignore stop */ - // Shortcut if there is no request to remove shards - if (!options.shards) { - return - } - // Check if we have information about the shards to remove if (!upload.root) { throw new Error( @@ -353,7 +362,30 @@ export class Client extends Base { // Remove shards await Promise.allSettled( - upload.shards.map((shard) => this.capability.store.remove(shard)) + upload.shards.map(async (shard) => { + try { + await this.capability.store.remove(shard) + } catch (/** @type {any} */ error) { + /* c8 ignore start */ + // If not found, we can tolerate error as it may be a consecutive call for deletion where first failed + if (error?.cause?.name !== 'StoreItemNotFound') { + throw new Error( + `remove failed for shard ${shard.link()} from ${contentCID}: ${ + error?.message + }` + ) + } + /* c8 ignore stop */ + } + }) ) + + // Remove association of content CID with selected space. + try { + await this.capability.upload.remove(contentCID) + /* c8 ignore start */ + } catch (/** @type {any} */ err) { + throw new Error(`remove failed for ${contentCID}: ${err.message ?? err}`) + } } } diff --git a/packages/w3up-client/test/client.test.js b/packages/w3up-client/test/client.test.js index 5d04095bd..b3e8d7ffd 100644 --- a/packages/w3up-client/test/client.test.js +++ b/packages/w3up-client/test/client.test.js @@ -4,6 +4,7 @@ import { create as createServer, parseLink, provide, + error, } from '@ucanto/server' import * as CAR from '@ucanto/transport/car' import * as Signer from '@ucanto/principal/ed25519' @@ -11,6 +12,7 @@ import * as StoreCapabilities from '@web3-storage/capabilities/store' import * as UploadCapabilities from '@web3-storage/capabilities/upload' import * as UCANCapabilities from '@web3-storage/capabilities/ucan' import { AgentData } from '@web3-storage/access/agent' +import { StoreItemNotFound } from '../../upload-api/src/store/lib.js' import { randomBytes, randomCAR } from './helpers/random.js' import { toCAR } from './helpers/car.js' import { mockService, mockServiceConf } from './helpers/mocks.js' @@ -494,6 +496,16 @@ describe('Client', () => { }), }, upload: { + get: provide(UploadCapabilities.get, ({ invocation }) => { + return { + ok: { + root: uploadedCar.roots[0], + shards: [uploadedCar.cid], + insertedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + } + }), remove: provide(UploadCapabilities.remove, ({ invocation }) => { return { ok: { @@ -527,6 +539,8 @@ describe('Client', () => { alice.remove(contentCID, { shards: true }) ) + assert(service.upload.get.called) + assert.equal(service.upload.get.callCount, 1) assert(service.upload.remove.called) assert.equal(service.upload.remove.callCount, 1) assert(service.store.remove.called) @@ -540,6 +554,16 @@ describe('Client', () => { const service = mockService({ upload: { + get: provide(UploadCapabilities.get, ({ invocation }) => { + return { + ok: { + root: uploadedCar.roots[0], + shards: [uploadedCar.cid], + insertedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + } + }), remove: provide(UploadCapabilities.remove, ({ invocation }) => { return { ok: { @@ -549,6 +573,11 @@ describe('Client', () => { } }), }, + store: { + remove: provide(StoreCapabilities.remove, ({ invocation }) => { + return { ok: { size: uploadedCar.size } } + }), + }, }) const server = createServer({ @@ -584,7 +613,7 @@ describe('Client', () => { const service = mockService({ upload: { // @ts-expect-error root should be a link - remove: provide(UploadCapabilities.remove, ({ invocation }) => { + get: provide(UploadCapabilities.get, ({ invocation }) => { return { ok: { root: undefined, @@ -614,9 +643,81 @@ describe('Client', () => { await assert.rejects(alice.remove(contentCID, { shards: true })) + assert(service.upload.get.called) + assert.equal(service.upload.get.callCount, 1) + assert.equal(service.store.remove.callCount, 0) + assert.equal(service.upload.remove.callCount, 0) + }) + + it('should not fail to remove if shard is not found', async () => { + const bytesArray = [await randomBytes(128), await randomBytes(128)] + const uploadedCars = await Promise.all( + bytesArray.map((bytes) => toCAR(bytes)) + ) + const contentCID = uploadedCars[0].roots[0] + + const service = mockService({ + upload: { + get: provide(UploadCapabilities.get, ({ invocation }) => { + return { + ok: { + root: uploadedCars[0].roots[0], + shards: uploadedCars.map((car) => car.cid), + insertedAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + }, + } + }), + remove: provide(UploadCapabilities.remove, ({ invocation }) => { + return { + ok: { + root: uploadedCars[0].roots[0], + shards: uploadedCars.map((car) => car.cid), + }, + } + }), + }, + store: { + remove: provide( + StoreCapabilities.remove, + ({ invocation, capability }) => { + // Fail for first as not found) + if (capability.nb.link.equals(uploadedCars[0].cid)) { + return error( + new StoreItemNotFound('did:web:any', uploadedCars[0].cid) + ) + } + return { ok: { size: uploadedCars[1].size } } + } + ), + }, + }) + + const server = createServer({ + id: await Signer.generate(), + service, + codec: CAR.inbound, + validateAuthorization, + }) + + const alice = new Client(await AgentData.create(), { + // @ts-ignore + serviceConf: await mockServiceConf(server), + }) + + // setup space + const space = await alice.createSpace('upload-test') + const auth = await space.createAuthorization(alice) + await alice.addSpace(auth) + await alice.setCurrentSpace(space.did()) + + await assert.doesNotReject(() => + alice.remove(contentCID, { shards: true }) + ) + assert(service.upload.remove.called) assert.equal(service.upload.remove.callCount, 1) - assert.equal(service.store.remove.callCount, 0) + assert.equal(service.store.remove.callCount, 2) }) it('should not allow remove without a current space', async () => {