From 6f27c4e9e59d101a8e09a4cfdd8be1c2f44e8900 Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:26:13 -0600 Subject: [PATCH 1/5] feat: Implement bucket entry size summation and deletion by bucket. Add error handling for shard-backed buckets in the use case. Update HTTP gateway to support bucket deletion endpoint. Enhance tests for bucket deletion scenarios. --- .../MongoDBBucketEntriesRepository.ts | 18 ++ lib/core/bucketEntries/Repository.ts | 2 + lib/core/bucketEntries/usecase.ts | 83 ++++++- lib/server/http/gateway/controller.ts | 49 +++- lib/server/http/gateway/index.ts | 1 + tests/lib/core/bucketentries/usecase.test.ts | 179 ++++++++++++++- tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts | 214 ++++++++++++++++++ 7 files changed, 542 insertions(+), 4 deletions(-) diff --git a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts index db9f8134..22851567 100644 --- a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts +++ b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts @@ -90,6 +90,24 @@ export class MongoDBBucketEntriesRepository implements BucketEntriesRepository { return bucketEntries.map(formatFromMongoToBucketEntry); } + async sumSizeByBucket(bucketId: string): Promise { + const [result] = await this.model + .aggregate([ + { $match: { bucket: new ObjectId(bucketId) } }, + { $group: { _id: null, total: { $sum: '$size' } } }, + ]) + .read('primary') + .exec(); + + return result?.total ?? 0; + } + + async deleteByBucket(bucketId: string): Promise { + const { deletedCount } = await this.model.deleteMany({ bucket: bucketId }); + + return deletedCount ?? 0; + } + async findByIds(ids: string[]): Promise { const bucketEntries = await this.model.find({ _id: { $in: ids } }); diff --git a/lib/core/bucketEntries/Repository.ts b/lib/core/bucketEntries/Repository.ts index 4cc7c926..9ce49857 100644 --- a/lib/core/bucketEntries/Repository.ts +++ b/lib/core/bucketEntries/Repository.ts @@ -6,6 +6,8 @@ export interface BucketEntriesRepository { count(where: Partial): Promise; findOne(where: Partial): Promise; findByBucket(bucketId: Bucket['id'], limit: number, offset: number): Promise; + sumSizeByBucket(bucketId: Bucket['id']): Promise; + deleteByBucket(bucketId: Bucket['id']): Promise; findByIds(ids: BucketEntry['id'][]): Promise; findOneWithFrame(where: Partial): Promise & { frame?: Frame } | null>; findByIdsWithFrames(ids: BucketEntry['id'][]): Promise<(Omit & { frame?: Frame })[]>; diff --git a/lib/core/bucketEntries/usecase.ts b/lib/core/bucketEntries/usecase.ts index f1ec0c93..3eb9227e 100644 --- a/lib/core/bucketEntries/usecase.ts +++ b/lib/core/bucketEntries/usecase.ts @@ -16,6 +16,27 @@ import { User } from '../users/User'; import { Bucket } from '../buckets/Bucket'; import { FileStateRepository } from '../fileState/Repository'; + + +/** + * Raised when a bucket turns out to hold entries backed by shards. + * + * removeBucketAndEntries() drops entries wholesale without walking their + * shards, which is only safe for the shard-less buckets this route exists to + * purge. Doing it to a Drive bucket would strand its shards, mirrors and the + * bytes on the farmers with nothing left pointing at them, so it refuses. + */ +export class ShardBackedBucketError extends Error { + constructor() { + super('Bucket holds shard-backed entries and cannot be purged by this route'); + + Object.setPrototypeOf(this, ShardBackedBucketError.prototype); + } +} + +const isShardBackedEntry = (entry: BucketEntry): boolean => + Boolean(entry.frame) || Boolean(entry.index) || Boolean(entry.hmac); + export class BucketEntryVersionNotFoundError extends Error { constructor() { super('BucketEntryVersion not found'); @@ -179,8 +200,8 @@ export class BucketEntriesUsecase { await this.bucketEntryShardsRepository.deleteByIds(bucketEntryShardsIds); } - await this.bucketEntriesRepository.deleteByIds(fileIds); await this.fileStateRepository.deleteByBucketEntryIds(fileIds); + await this.bucketEntriesRepository.deleteByIds(fileIds); } private async findBucketOwner( @@ -254,4 +275,64 @@ export class BucketEntriesUsecase { totalUsedSpaceBytes, }; } + + /** + * Removes a bucket and every entry in it. + * + * The entries this purges are metadata only: createEntry() writes a row with + * a bucket, a size and a version, and nothing else. Nothing downstream of a + * bucket entry exists for them, which is what makes a wholesale delete + * possible instead of walking each entry and its shards. Anything else in + * the bucket is refused, see ShardBackedBucketError. + * + * The whole thing is three commands regardless of how many entries the + * bucket holds + */ + async removeBucketAndEntries( + userUuid: User['uuid'], + bucketId: Bucket['id'] + ): Promise { + const user = await this.usersRepository.findByUuid(userUuid); + + if (!user) { + throw new UserNotFoundError(userUuid); + } + + const bucket = await this.bucketsRepository.findOne({ id: bucketId }); + + if (bucket && bucket.userId !== userUuid) { + throw new BucketForbiddenError(); + } + + const sample = await this.bucketEntriesRepository.findOne({ bucket: bucketId }); + if (sample && isShardBackedEntry(sample)) { + throw new ShardBackedBucketError(); + } + + if (bucket) { + await this.bucketsRepository.removeByIdAndUser(bucketId, userUuid); + } + + const releasedBytes = await this.bucketEntriesRepository.sumSizeByBucket(bucketId); + + await this.bucketEntriesRepository.deleteByBucket(bucketId); + + if (releasedBytes === 0) { + return { + maxSpaceBytes: user.maxSpaceBytes, + totalUsedSpaceBytes: user.totalUsedSpaceBytes, + }; + } + + const totalUsedSpaceBytes = await this.usersRepository.addTotalUsedSpaceBytes( + userUuid, + -releasedBytes + ); + + return { + maxSpaceBytes: user.maxSpaceBytes, + totalUsedSpaceBytes, + }; + } + } diff --git a/lib/server/http/gateway/controller.ts b/lib/server/http/gateway/controller.ts index 5c1a180d..71647fb0 100644 --- a/lib/server/http/gateway/controller.ts +++ b/lib/server/http/gateway/controller.ts @@ -2,8 +2,8 @@ import { Request, Response } from 'express'; import { validate as uuidValidate } from 'uuid'; import { Logger } from 'winston'; import { EmailIsAlreadyInUseError, InvalidDataFormatError, UserAlreadyExistsError, UserNotFoundError, UserSpaceSnapshot, UsersUsecase } from '../../../core'; -import { BucketEntriesUsecase } from '../../../core/bucketEntries/usecase'; -import { BucketNotFoundError } from '../../../core/buckets/usecase'; +import { BucketEntriesUsecase, ShardBackedBucketError } from '../../../core/bucketEntries/usecase'; +import { BucketForbiddenError, BucketNotFoundError } from '../../../core/buckets/usecase'; import { GatewayUsecase } from '../../../core/gateway/Usecase'; import { EventBus, EventBusEvents, UserStorageChangedPayload } from '../../eventBus'; @@ -205,6 +205,51 @@ export class HTTPGatewayController { } } + async deleteUserBucket( + req: Request<{ uuid: string; id: string }>, + res: Response + ) { + const { uuid, id } = req.params; + + if (!uuid || !uuidValidate(uuid) || !id || !OBJECT_ID_PATTERN.test(id)) { + return res.status(400).send({ message: 'Invalid params' }); + } + + try { + const snapshot = await this.bucketEntriesUsecase.removeBucketAndEntries(uuid, id); + + return res.status(200).send(snapshot); + } catch (err) { + if (err instanceof UserNotFoundError) { + return res.status(404).send({ message: err.message }); + } + + if (err instanceof BucketForbiddenError) { + return res.status(403).send({ message: err.message }); + } + + if (err instanceof ShardBackedBucketError) { + this.logger.warn( + '[GATEWAY/DELETE_BUCKET] Refused to purge shard-backed bucket %s of user %s', + id, + uuid + ); + + return res.status(409).send({ message: err.message }); + } + + this.logger.error( + '[GATEWAY/DELETE_BUCKET] Error deleting bucket %s of user %s: %s. %s', + id, + uuid, + (err as Error).message, + (err as Error).stack || 'NO STACK' + ); + + return res.status(500).send({ message: 'Internal server error' }); + } + } + async createBucketEntry( req: Request<{ uuid: string; id: string }, {}, Partial, {}>, res: Response diff --git a/lib/server/http/gateway/index.ts b/lib/server/http/gateway/index.ts index 52acc390..f971873b 100644 --- a/lib/server/http/gateway/index.ts +++ b/lib/server/http/gateway/index.ts @@ -12,6 +12,7 @@ export const createGatewayHTTPRouter = ( router.patch('/users/:uuid', jwtMiddleware, controller.updateUserEmail.bind(controller)); router.put('/storage/users/:uuid', jwtMiddleware, controller.changeStorage.bind(controller)); router.post('/users/:uuid/buckets', jwtMiddleware, controller.createUserBucket.bind(controller)); + router.delete('/users/:uuid/buckets/:id', jwtMiddleware, controller.deleteUserBucket.bind(controller)); router.post('/users/:uuid/buckets/:id/entries', jwtMiddleware, controller.createBucketEntry.bind(controller)); router.delete('/users/:uuid/buckets/:id/entries/:entryId', jwtMiddleware, controller.deleteBucketEntry.bind(controller)); router.delete('/storage/files', jwtMiddleware, controller.deleteFilesInBulk.bind(controller)); diff --git a/tests/lib/core/bucketentries/usecase.test.ts b/tests/lib/core/bucketentries/usecase.test.ts index 6cac4614..5f188af8 100644 --- a/tests/lib/core/bucketentries/usecase.test.ts +++ b/tests/lib/core/bucketentries/usecase.test.ts @@ -12,7 +12,7 @@ import { UsersRepository } from '../../../../lib/core/users/Repository'; import { MongoDBBucketsRepository } from '../../../../lib/core/buckets/MongoDBBucketsRepository'; import { MongoDBBucketEntriesRepository } from '../../../../lib/core/bucketEntries/MongoDBBucketEntriesRepository'; -import { BucketEntriesUsecase, BucketEntryVersionNotFoundError } from '../../../../lib/core/bucketEntries/usecase'; +import { BucketEntriesUsecase, BucketEntryVersionNotFoundError, ShardBackedBucketError } from '../../../../lib/core/bucketEntries/usecase'; import { BucketEntryNotFoundError, BucketForbiddenError, BucketNotFoundError } from '../../../../lib/core/buckets/usecase'; import { MongoDBFramesRepository } from '../../../../lib/core/frames/MongoDBFramesRepository'; import { MongoDBMirrorsRepository } from '../../../../lib/core/mirrors/MongoDBMirrorsRepository'; @@ -753,4 +753,181 @@ describe('BucketEntriesUsecase', function () { expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 3500 }); }); }); + + describe('removeBucketAndEntries()', () => { + it('When the user does not exist, then it throws UserNotFoundError', async () => { + stub(usersRepository, 'findByUuid').resolves(null); + const findBucket = stub(bucketsRepository, 'findOne'); + + try { + await bucketEntriesUsecase.removeBucketAndEntries('unknown-uuid', 'bucket-id'); + expect(true).toBeFalsy(); + } catch (err) { + expect(err).toBeInstanceOf(UserNotFoundError); + } + + expect(findBucket.called).toBeFalsy(); + }); + + it('When the bucket belongs to another user, then it throws BucketForbiddenError and removes nothing', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket(); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket'); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); + + try { + await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + expect(true).toBeFalsy(); + } catch (err) { + expect(err).toBeInstanceOf(BucketForbiddenError); + } + + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); + }); + + it.each([ + ['a frame', { frame: 'frame-id', index: undefined, hmac: undefined }], + ['an index', { frame: undefined, index: 'file-index' }], + ['an hmac', { frame: undefined, index: undefined, hmac: { type: 'sha512' as const, value: 'v' } }], + ])( + 'When the bucket holds entries with %s, then it refuses and leaves the bucket alone', + async (_case, entryFields) => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket({ userId: user.uuid }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves( + fixtures.getBucketEntry({ bucket: bucket.id, ...entryFields }) + ); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket'); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); + + try { + await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + expect(true).toBeFalsy(); + } catch (err) { + expect(err).toBeInstanceOf(ShardBackedBucketError); + } + + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); + } + ); + + it('When the bucket is empty, then it is removed and the total is untouched', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket({ userId: user.uuid }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves(null); + stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(0); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(0); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + + expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(addUsage.called).toBeFalsy(); + expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + }); + + it('When the bucket has entries, then they go in one delete and their total size is released', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket({ userId: user.uuid }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves( + fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: 500 }) + ); + const sumSizes = stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(2000); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(2); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(2000); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + + expect(sumSizes.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(addUsage.calledOnceWithExactly(user.uuid, -2000)).toBeTruthy(); + expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 2000 }); + }); + + it('When the bucket has entries, then the bucket document is removed before the sum is taken', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket({ userId: user.uuid }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves( + fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined }) + ); + const sumSizes = stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(2000); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(2); + stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(2000); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); + + await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + + // An entry created between the sum and the delete would be removed + // without its size ever being released. + expect(removeBucket.calledBefore(sumSizes)).toBeTruthy(); + expect(sumSizes.calledBefore(deleteEntries)).toBeTruthy(); + }); + + it('When the bucket document is already gone, then it still drains the entries it left behind', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(null); + stub(bucketEntriesRepository, 'findOne').resolves( + fixtures.getBucketEntry({ bucket: 'bucket-id', frame: undefined, index: undefined }) + ); + stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(1000); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(1); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(3000); + const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id'); + + expect(deleteEntries.calledOnceWithExactly('bucket-id')).toBeTruthy(); + expect(addUsage.calledOnceWithExactly(user.uuid, -1000)).toBeTruthy(); + expect(removeBucket.called).toBeFalsy(); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 3000 }); + }); + + it('When the entries carry no size, then the total is left where it was', async () => { + const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const bucket = fixtures.getBucket({ userId: user.uuid }); + + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves( + fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: undefined }) + ); + stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(0); + const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(1); + const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); + stub(bucketsRepository, 'removeByIdAndUser').resolves(); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + + expect(deleteEntries.calledOnce).toBeTruthy(); + expect(addUsage.called).toBeFalsy(); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + }); + }); }); diff --git a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts index 171188db..a6514b1c 100644 --- a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts +++ b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts @@ -368,6 +368,220 @@ describe('Gateway V2 e2e tests', () => { }) }) + describe('Deleting a user bucket', () => { + const createBucketForUser = async (userUuid: string, jwt: string) => { + const { body } = await testServer + .post(`/v2/gateway/users/${userUuid}/buckets`) + .set('Authorization', `Bearer ${jwt}`) + .send({ name: `mail-account-${crypto.randomUUID()}` }) + + return body as { id: string; name: string } + } + + const createEntryForBucket = async (userUuid: string, bucketId: string, jwt: string, size: number) => { + const { body } = await testServer + .post(`/v2/gateway/users/${userUuid}/buckets/${bucketId}/entries`) + .set('Authorization', `Bearer ${jwt}`) + .send({ size }) + + return body as { id: string } + } + + it('When deleting a bucket, then its entries go with it and the user total shrinks by their sum', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + + await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + await createEntryForBucket(testUser.uuid, bucket.id, jwt, 3000) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(200) + + const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) + expect(entriesInDatabase.length).toBe(0) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).toBeNull() + + const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + expect(response.body).toEqual({ + maxSpaceBytes: userAfter.maxSpaceBytes, + totalUsedSpaceBytes: userAfter.totalUsedSpaceBytes, + }) + }) + + it('When deleting an empty bucket, then it is removed and the user total is unchanged', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(200) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).toBeNull() + + const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + }) + + it('When deleting the same bucket twice, then the second call succeeds and the total does not move again', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + + const first = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + const userAfterFirst = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + + const second = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(first.status).toBe(200) + expect(second.status).toBe(200) + + const userAfterSecond = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + expect(userAfterSecond.totalUsedSpaceBytes).toBe(userAfterFirst.totalUsedSpaceBytes) + }) + + it('When deleting a bucket of another user, then it returns 403 and the bucket is left alone', async () => { + const owner = await createTestUser() + const otherUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(owner.uuid, jwt) + + await createEntryForBucket(owner.uuid, bucket.id, jwt, 5000) + + const ownerBefore = await databaseConnection.models.User.findOne({ uuid: owner.uuid }) + + const response = await testServer + .delete(`/v2/gateway/users/${otherUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(403) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).not.toBeNull() + + const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) + expect(entriesInDatabase.length).toBe(1) + + const ownerAfter = await databaseConnection.models.User.findOne({ uuid: owner.uuid }) + expect(ownerAfter.totalUsedSpaceBytes).toBe(ownerBefore.totalUsedSpaceBytes) + }) + + it('When deleting a bucket of an unknown user, then it returns 404', async () => { + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + + const response = await testServer + .delete(`/v2/gateway/users/${crypto.randomUUID()}/buckets/${'a'.repeat(24)}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(404) + }) + + it('When entries are left behind by a bucket that no longer exists, then they are drained anyway', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + + await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + + await databaseConnection.models.Bucket.deleteOne({ _id: bucket.id }) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(200) + + const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) + expect(entriesInDatabase.length).toBe(0) + + const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + }) + + it('When the bucket holds shard-backed entries, then it refuses and touches nothing', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + + await databaseConnection.models.BucketEntry.updateMany( + { bucket: bucket.id }, + { $set: { index: 'a'.repeat(64) } } + ) + + const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(409) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).not.toBeNull() + + const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) + expect(entriesInDatabase.length).toBe(1) + + const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + }) + + it('When the user uuid is malformed, then it returns 400', async () => { + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + + const response = await testServer + .delete(`/v2/gateway/users/not-a-uuid/buckets/${'a'.repeat(24)}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(400) + }) + + it('When the bucket id is malformed, then it returns 400', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/not-an-object-id`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(400) + }) + + it('When no auth token is provided, then it returns 401', async () => { + const testUser = await createTestUser() + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${'a'.repeat(24)}`) + + expect(response.status).toBe(401) + }) + }) + describe('Deleting user files', () => { let axiosGetStub: sinon.SinonStub const FAKE_UPLOAD_URL = 'http://fake-upload-url' From a3993e08dd07da8a4fb31d5904946118a0f1beff Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Fri, 21 Aug 2026 08:21:40 -0600 Subject: [PATCH 2/5] refactor: Update deleteByBucket method to return void and adjust related repository interface. Simplify bucket entry removal logic in the use case and enhance test stubs for better clarity and maintainability. --- .../MongoDBBucketEntriesRepository.ts | 6 +- lib/core/bucketEntries/Repository.ts | 2 +- lib/core/bucketEntries/usecase.ts | 39 ++--- tests/lib/core/bucketentries/usecase.test.ts | 142 +++++++----------- tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts | 114 ++++++-------- 5 files changed, 109 insertions(+), 194 deletions(-) diff --git a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts index 22851567..f86f0c62 100644 --- a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts +++ b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts @@ -102,10 +102,8 @@ export class MongoDBBucketEntriesRepository implements BucketEntriesRepository { return result?.total ?? 0; } - async deleteByBucket(bucketId: string): Promise { - const { deletedCount } = await this.model.deleteMany({ bucket: bucketId }); - - return deletedCount ?? 0; + async deleteByBucket(bucketId: string): Promise { + await this.model.deleteMany({ bucket: bucketId }); } async findByIds(ids: string[]): Promise { diff --git a/lib/core/bucketEntries/Repository.ts b/lib/core/bucketEntries/Repository.ts index 9ce49857..35f49935 100644 --- a/lib/core/bucketEntries/Repository.ts +++ b/lib/core/bucketEntries/Repository.ts @@ -7,7 +7,7 @@ export interface BucketEntriesRepository { findOne(where: Partial): Promise; findByBucket(bucketId: Bucket['id'], limit: number, offset: number): Promise; sumSizeByBucket(bucketId: Bucket['id']): Promise; - deleteByBucket(bucketId: Bucket['id']): Promise; + deleteByBucket(bucketId: Bucket['id']): Promise; findByIds(ids: BucketEntry['id'][]): Promise; findOneWithFrame(where: Partial): Promise & { frame?: Frame } | null>; findByIdsWithFrames(ids: BucketEntry['id'][]): Promise<(Omit & { frame?: Frame })[]>; diff --git a/lib/core/bucketEntries/usecase.ts b/lib/core/bucketEntries/usecase.ts index 3eb9227e..c9d97bc1 100644 --- a/lib/core/bucketEntries/usecase.ts +++ b/lib/core/bucketEntries/usecase.ts @@ -16,15 +16,11 @@ import { User } from '../users/User'; import { Bucket } from '../buckets/Bucket'; import { FileStateRepository } from '../fileState/Repository'; - - /** * Raised when a bucket turns out to hold entries backed by shards. * - * removeBucketAndEntries() drops entries wholesale without walking their - * shards, which is only safe for the shard-less buckets this route exists to - * purge. Doing it to a Drive bucket would strand its shards, mirrors and the - * bytes on the farmers with nothing left pointing at them, so it refuses. + * Dropping those entries wholesale would strand their shards, mirrors and the + * bytes on the farmers with nothing left pointing at them. */ export class ShardBackedBucketError extends Error { constructor() { @@ -284,55 +280,42 @@ export class BucketEntriesUsecase { * bucket entry exists for them, which is what makes a wholesale delete * possible instead of walking each entry and its shards. Anything else in * the bucket is refused, see ShardBackedBucketError. - * - * The whole thing is three commands regardless of how many entries the - * bucket holds */ async removeBucketAndEntries( userUuid: User['uuid'], bucketId: Bucket['id'] ): Promise { - const user = await this.usersRepository.findByUuid(userUuid); + const [user, bucket, sample] = await Promise.all([ + this.usersRepository.findByUuid(userUuid), + this.bucketsRepository.findOne({ id: bucketId }), + this.bucketEntriesRepository.findOne({ bucket: bucketId }), + ]); if (!user) { throw new UserNotFoundError(userUuid); } - const bucket = await this.bucketsRepository.findOne({ id: bucketId }); - if (bucket && bucket.userId !== userUuid) { throw new BucketForbiddenError(); } - const sample = await this.bucketEntriesRepository.findOne({ bucket: bucketId }); if (sample && isShardBackedEntry(sample)) { throw new ShardBackedBucketError(); } - if (bucket) { - await this.bucketsRepository.removeByIdAndUser(bucketId, userUuid); - } + await this.bucketsRepository.removeByIdAndUser(bucketId, userUuid); const releasedBytes = await this.bucketEntriesRepository.sumSizeByBucket(bucketId); await this.bucketEntriesRepository.deleteByBucket(bucketId); - if (releasedBytes === 0) { - return { - maxSpaceBytes: user.maxSpaceBytes, - totalUsedSpaceBytes: user.totalUsedSpaceBytes, - }; - } - - const totalUsedSpaceBytes = await this.usersRepository.addTotalUsedSpaceBytes( - userUuid, - -releasedBytes - ); + const totalUsedSpaceBytes = releasedBytes === 0 + ? user.totalUsedSpaceBytes + : await this.usersRepository.addTotalUsedSpaceBytes(userUuid, -releasedBytes); return { maxSpaceBytes: user.maxSpaceBytes, totalUsedSpaceBytes, }; } - } diff --git a/tests/lib/core/bucketentries/usecase.test.ts b/tests/lib/core/bucketentries/usecase.test.ts index 5f188af8..af2a4a0a 100644 --- a/tests/lib/core/bucketentries/usecase.test.ts +++ b/tests/lib/core/bucketentries/usecase.test.ts @@ -25,6 +25,7 @@ import { ShardsUsecase } from '../../../../lib/core/shards/usecase'; import fixtures from '../fixtures'; import { BucketEntry } from '../../../../lib/core/bucketEntries/BucketEntry'; import { Bucket } from '../../../../lib/core/buckets/Bucket'; +import { User } from '../../../../lib/core/users/User'; import { ContactsRepository } from '../../../../lib/core/contacts/Repository'; import { MongoDBContactsRepository } from '../../../../lib/core/contacts/MongoDBContactsRepository'; import { FileStateRepository } from '../../../../lib/core/fileState/Repository'; @@ -755,9 +756,28 @@ describe('BucketEntriesUsecase', function () { }); describe('removeBucketAndEntries()', () => { + const stubRepositories = ( + user: User | null, + bucket: Bucket | null, + entry: BucketEntry | null, + { releasedBytes = 0, newTotal = 0 } = {} + ) => { + stub(usersRepository, 'findByUuid').resolves(user); + stub(bucketsRepository, 'findOne').resolves(bucket); + stub(bucketEntriesRepository, 'findOne').resolves(entry); + + return { + sumSizes: stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(releasedBytes), + deleteEntries: stub(bucketEntriesRepository, 'deleteByBucket').resolves(), + addUsage: stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(newTotal), + removeBucket: stub(bucketsRepository, 'removeByIdAndUser').resolves(), + }; + }; + + const getOwner = () => fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + it('When the user does not exist, then it throws UserNotFoundError', async () => { - stub(usersRepository, 'findByUuid').resolves(null); - const findBucket = stub(bucketsRepository, 'findOne'); + const { deleteEntries, removeBucket } = stubRepositories(null, null, null); try { await bucketEntriesUsecase.removeBucketAndEntries('unknown-uuid', 'bucket-id'); @@ -766,21 +786,16 @@ describe('BucketEntriesUsecase', function () { expect(err).toBeInstanceOf(UserNotFoundError); } - expect(findBucket.called).toBeFalsy(); + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); }); it('When the bucket belongs to another user, then it throws BucketForbiddenError and removes nothing', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); - const bucket = fixtures.getBucket(); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket'); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); + const user = getOwner(); + const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, fixtures.getBucket(), null); try { - await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id'); expect(true).toBeFalsy(); } catch (err) { expect(err).toBeInstanceOf(BucketForbiddenError); @@ -798,17 +813,13 @@ describe('BucketEntriesUsecase', function () { ])( 'When the bucket holds entries with %s, then it refuses and leaves the bucket alone', async (_case, entryFields) => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const user = getOwner(); const bucket = fixtures.getBucket({ userId: user.uuid }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves( + const { deleteEntries, removeBucket, addUsage } = stubRepositories( + user, + bucket, fixtures.getBucketEntry({ bucket: bucket.id, ...entryFields }) ); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket'); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); try { await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); @@ -823,17 +834,17 @@ describe('BucketEntriesUsecase', function () { } ); - it('When the bucket is empty, then it is removed and the total is untouched', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + it.each([ + ['the bucket is empty', null], + ['the entries carry no size', { size: undefined }], + ])('When %s, then it is removed and the total is untouched', async (_case, entryFields) => { + const user = getOwner(); const bucket = fixtures.getBucket({ userId: user.uuid }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves(null); - stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(0); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(0); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); + const { deleteEntries, addUsage, removeBucket } = stubRepositories( + user, + bucket, + entryFields && fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, ...entryFields }) + ); const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); @@ -844,18 +855,14 @@ describe('BucketEntriesUsecase', function () { }); it('When the bucket has entries, then they go in one delete and their total size is released', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const user = getOwner(); const bucket = fixtures.getBucket({ userId: user.uuid }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves( - fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: 500 }) + const { sumSizes, deleteEntries, addUsage, removeBucket } = stubRepositories( + user, + bucket, + fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: 500 }), + { releasedBytes: 2000, newTotal: 2000 } ); - const sumSizes = stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(2000); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(2); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(2000); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); @@ -864,23 +871,6 @@ describe('BucketEntriesUsecase', function () { expect(addUsage.calledOnceWithExactly(user.uuid, -2000)).toBeTruthy(); expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 2000 }); - }); - - it('When the bucket has entries, then the bucket document is removed before the sum is taken', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); - const bucket = fixtures.getBucket({ userId: user.uuid }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves( - fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined }) - ); - const sumSizes = stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(2000); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(2); - stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(2000); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser').resolves(); - - await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); // An entry created between the sum and the delete would be removed // without its size ever being released. @@ -889,45 +879,19 @@ describe('BucketEntriesUsecase', function () { }); it('When the bucket document is already gone, then it still drains the entries it left behind', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(null); - stub(bucketEntriesRepository, 'findOne').resolves( - fixtures.getBucketEntry({ bucket: 'bucket-id', frame: undefined, index: undefined }) + const user = getOwner(); + const { deleteEntries, addUsage } = stubRepositories( + user, + null, + fixtures.getBucketEntry({ bucket: 'bucket-id', frame: undefined, index: undefined }), + { releasedBytes: 1000, newTotal: 3000 } ); - stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(1000); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(1); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(3000); - const removeBucket = stub(bucketsRepository, 'removeByIdAndUser'); const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id'); expect(deleteEntries.calledOnceWithExactly('bucket-id')).toBeTruthy(); expect(addUsage.calledOnceWithExactly(user.uuid, -1000)).toBeTruthy(); - expect(removeBucket.called).toBeFalsy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 3000 }); }); - - it('When the entries carry no size, then the total is left where it was', async () => { - const user = fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); - const bucket = fixtures.getBucket({ userId: user.uuid }); - - stub(usersRepository, 'findByUuid').resolves(user); - stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves( - fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: undefined }) - ); - stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(0); - const deleteEntries = stub(bucketEntriesRepository, 'deleteByBucket').resolves(1); - const addUsage = stub(usersRepository, 'addTotalUsedSpaceBytes'); - stub(bucketsRepository, 'removeByIdAndUser').resolves(); - - const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); - - expect(deleteEntries.calledOnce).toBeTruthy(); - expect(addUsage.called).toBeFalsy(); - expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); - }); }); }); diff --git a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts index a6514b1c..def8f6c3 100644 --- a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts +++ b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts @@ -12,6 +12,15 @@ const MB100 = 100 * 1024 * 1024; describe('Gateway V2 e2e tests', () => { const databaseConnection = new StorageDbManager(); + const createBucketForUser = async (userUuid: string, jwt: string) => { + const { body } = await testServer + .post(`/v2/gateway/users/${userUuid}/buckets`) + .set('Authorization', `Bearer ${jwt}`) + .send({ name: `mail-account-${crypto.randomUUID()}` }) + + return body as { id: string; name: string } + } + beforeEach(async () => { await databaseConnection.connect(); jest.clearAllMocks() @@ -170,15 +179,6 @@ describe('Gateway V2 e2e tests', () => { }) describe('Bucket entries', () => { - const createBucketForUser = async (userUuid: string, jwt: string) => { - const { body } = await testServer - .post(`/v2/gateway/users/${userUuid}/buckets`) - .set('Authorization', `Bearer ${jwt}`) - .send({ name: `mail-account-${crypto.randomUUID()}` }) - - return body as { id: string; name: string } - } - const randomObjectId = () => crypto.randomBytes(12).toString('hex') it('When creating an entry, then it is persisted as a shard-less v2 entry and the user total grows by its size', async () => { @@ -369,14 +369,15 @@ describe('Gateway V2 e2e tests', () => { }) describe('Deleting a user bucket', () => { - const createBucketForUser = async (userUuid: string, jwt: string) => { - const { body } = await testServer - .post(`/v2/gateway/users/${userUuid}/buckets`) + const VALID_OBJECT_ID = 'a'.repeat(24) + + const deleteBucket = (userUuid: string, bucketId: string, jwt: string) => + testServer + .delete(`/v2/gateway/users/${userUuid}/buckets/${bucketId}`) .set('Authorization', `Bearer ${jwt}`) - .send({ name: `mail-account-${crypto.randomUUID()}` }) - return body as { id: string; name: string } - } + const usedSpaceOf = async (uuid: string): Promise => + (await databaseConnection.models.User.findOne({ uuid })).totalUsedSpaceBytes const createEntryForBucket = async (userUuid: string, bucketId: string, jwt: string, size: number) => { const { body } = await testServer @@ -392,14 +393,12 @@ describe('Gateway V2 e2e tests', () => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) - const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + const usedBefore = await usedSpaceOf(testUser.uuid) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 3000) - const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt) expect(response.status).toBe(200) @@ -410,7 +409,7 @@ describe('Gateway V2 e2e tests', () => { expect(bucketInDatabase).toBeNull() const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) - expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + expect(userAfter.totalUsedSpaceBytes).toBe(usedBefore) expect(response.body).toEqual({ maxSpaceBytes: userAfter.maxSpaceBytes, totalUsedSpaceBytes: userAfter.totalUsedSpaceBytes, @@ -422,19 +421,16 @@ describe('Gateway V2 e2e tests', () => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) - const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + const usedBefore = await usedSpaceOf(testUser.uuid) - const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt) expect(response.status).toBe(200) const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) expect(bucketInDatabase).toBeNull() - const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) - expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) it('When deleting the same bucket twice, then the second call succeeds and the total does not move again', async () => { @@ -444,21 +440,16 @@ describe('Gateway V2 e2e tests', () => { await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) - const first = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const first = await deleteBucket(testUser.uuid, bucket.id, jwt) - const userAfterFirst = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + const usedAfterFirst = await usedSpaceOf(testUser.uuid) - const second = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const second = await deleteBucket(testUser.uuid, bucket.id, jwt) expect(first.status).toBe(200) expect(second.status).toBe(200) - const userAfterSecond = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) - expect(userAfterSecond.totalUsedSpaceBytes).toBe(userAfterFirst.totalUsedSpaceBytes) + expect(await usedSpaceOf(testUser.uuid)).toBe(usedAfterFirst) }) it('When deleting a bucket of another user, then it returns 403 and the bucket is left alone', async () => { @@ -469,11 +460,9 @@ describe('Gateway V2 e2e tests', () => { await createEntryForBucket(owner.uuid, bucket.id, jwt, 5000) - const ownerBefore = await databaseConnection.models.User.findOne({ uuid: owner.uuid }) + const ownerUsedBefore = await usedSpaceOf(owner.uuid) - const response = await testServer - .delete(`/v2/gateway/users/${otherUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(otherUser.uuid, bucket.id, jwt) expect(response.status).toBe(403) @@ -483,16 +472,13 @@ describe('Gateway V2 e2e tests', () => { const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) expect(entriesInDatabase.length).toBe(1) - const ownerAfter = await databaseConnection.models.User.findOne({ uuid: owner.uuid }) - expect(ownerAfter.totalUsedSpaceBytes).toBe(ownerBefore.totalUsedSpaceBytes) + expect(await usedSpaceOf(owner.uuid)).toBe(ownerUsedBefore) }) it('When deleting a bucket of an unknown user, then it returns 404', async () => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) - const response = await testServer - .delete(`/v2/gateway/users/${crypto.randomUUID()}/buckets/${'a'.repeat(24)}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(crypto.randomUUID(), VALID_OBJECT_ID, jwt) expect(response.status).toBe(404) }) @@ -502,23 +488,20 @@ describe('Gateway V2 e2e tests', () => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) - const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + const usedBefore = await usedSpaceOf(testUser.uuid) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) await databaseConnection.models.Bucket.deleteOne({ _id: bucket.id }) - const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt) expect(response.status).toBe(200) const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) expect(entriesInDatabase.length).toBe(0) - const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) - expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) it('When the bucket holds shard-backed entries, then it refuses and touches nothing', async () => { @@ -533,11 +516,9 @@ describe('Gateway V2 e2e tests', () => { { $set: { index: 'a'.repeat(64) } } ) - const userBefore = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) + const usedBefore = await usedSpaceOf(testUser.uuid) - const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt) expect(response.status).toBe(409) @@ -547,27 +528,16 @@ describe('Gateway V2 e2e tests', () => { const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) expect(entriesInDatabase.length).toBe(1) - const userAfter = await databaseConnection.models.User.findOne({ uuid: testUser.uuid }) - expect(userAfter.totalUsedSpaceBytes).toBe(userBefore.totalUsedSpaceBytes) + expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) - it('When the user uuid is malformed, then it returns 400', async () => { + it.each([ + ['the user uuid is malformed', 'not-a-uuid', VALID_OBJECT_ID], + ['the bucket id is malformed', crypto.randomUUID(), 'not-an-object-id'], + ])('When %s, then it returns 400', async (_case, userUuid, bucketId) => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) - const response = await testServer - .delete(`/v2/gateway/users/not-a-uuid/buckets/${'a'.repeat(24)}`) - .set('Authorization', `Bearer ${jwt}`) - - expect(response.status).toBe(400) - }) - - it('When the bucket id is malformed, then it returns 400', async () => { - const testUser = await createTestUser() - const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) - - const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/not-an-object-id`) - .set('Authorization', `Bearer ${jwt}`) + const response = await deleteBucket(userUuid, bucketId, jwt) expect(response.status).toBe(400) }) @@ -576,7 +546,7 @@ describe('Gateway V2 e2e tests', () => { const testUser = await createTestUser() const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${'a'.repeat(24)}`) + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${VALID_OBJECT_ID}`) expect(response.status).toBe(401) }) From 3fe9df14e64dbb880c2f935ea9568bd0ca048e0a Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Wed, 26 Aug 2026 19:12:56 -0600 Subject: [PATCH 3/5] feat: Enhance MongoDBBucketEntriesRepository with new methods for checking shard-backed and metadata-only entries. Update repository interface and adjust bucket entry deletion logic in the use case to include bucket name validation. Improve HTTP gateway controller to require bucket name on deletion requests. Update tests to cover new functionality and edge cases. --- .../MongoDBBucketEntriesRepository.ts | 52 +++- lib/core/bucketEntries/Repository.ts | 7 +- lib/core/bucketEntries/usecase.ts | 64 +++-- lib/server/http/gateway/controller.ts | 11 +- tests/lib/core/bucketentries/usecase.test.ts | 224 +++++++++++------- tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts | 104 +++++--- 6 files changed, 317 insertions(+), 145 deletions(-) diff --git a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts index f86f0c62..a67db2b3 100644 --- a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts +++ b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts @@ -1,8 +1,21 @@ import { Frame } from '../frames/Frame'; import { BucketEntry, BucketEntryWithFrame } from './BucketEntry'; -import { BucketEntriesRepository } from './Repository'; +import { BucketEntriesRepository, MetadataOnlyEntry } from './Repository'; import { ObjectId } from 'mongodb'; +/** + * The fields that mark an entry as backed by storage: `frame` on v1 entries, + * `index` and `hmac` on v2 uploads. Gateway entries (createEntry) set none of + * them, which is what makes them safe to drop wholesale. + */ +const SHARD_MARKERS = ['frame', 'index', 'hmac.value']; + +const markersExist = (exists: boolean) => + SHARD_MARKERS.map((field) => ({ [field]: { $exists: exists } })); + +const SHARD_BACKED = { $or: markersExist(true) }; +const METADATA_ONLY = { $and: markersExist(false) }; + interface BucketEntryModel extends Omit { _id: string; created: Date; @@ -90,20 +103,39 @@ export class MongoDBBucketEntriesRepository implements BucketEntriesRepository { return bucketEntries.map(formatFromMongoToBucketEntry); } - async sumSizeByBucket(bucketId: string): Promise { - const [result] = await this.model - .aggregate([ - { $match: { bucket: new ObjectId(bucketId) } }, - { $group: { _id: null, total: { $sum: '$size' } } }, - ]) + private async existsByBucket(bucketId: string, filter = {}): Promise { + const found = await this.model + .countDocuments({ bucket: bucketId, ...filter }, { limit: 1 }) .read('primary') .exec(); - return result?.total ?? 0; + return found > 0; } - async deleteByBucket(bucketId: string): Promise { - await this.model.deleteMany({ bucket: bucketId }); + hasEntriesByBucket(bucketId: string): Promise { + return this.existsByBucket(bucketId); + } + + hasShardBackedEntriesByBucket(bucketId: string): Promise { + return this.existsByBucket(bucketId, SHARD_BACKED); + } + + async findMetadataOnlyByBucket( + bucketId: string, + limit: number + ): Promise { + const bucketEntries = await this.model + .find({ bucket: bucketId, ...METADATA_ONLY }) + .select('_id size') + .limit(limit) + .read('primary') + .lean() + .exec(); + + return bucketEntries.map((entry: { _id: unknown; size?: number }) => ({ + id: String(entry._id), + size: entry.size, + })); } async findByIds(ids: string[]): Promise { diff --git a/lib/core/bucketEntries/Repository.ts b/lib/core/bucketEntries/Repository.ts index 35f49935..26e5e02f 100644 --- a/lib/core/bucketEntries/Repository.ts +++ b/lib/core/bucketEntries/Repository.ts @@ -2,12 +2,15 @@ import { Bucket } from "../buckets/Bucket"; import { Frame } from "../frames/Frame"; import { BucketEntry, BucketEntryWithFrame } from "./BucketEntry"; +export type MetadataOnlyEntry = Pick; + export interface BucketEntriesRepository { count(where: Partial): Promise; findOne(where: Partial): Promise; findByBucket(bucketId: Bucket['id'], limit: number, offset: number): Promise; - sumSizeByBucket(bucketId: Bucket['id']): Promise; - deleteByBucket(bucketId: Bucket['id']): Promise; + hasEntriesByBucket(bucketId: Bucket['id']): Promise; + hasShardBackedEntriesByBucket(bucketId: Bucket['id']): Promise; + findMetadataOnlyByBucket(bucketId: Bucket['id'], limit: number): Promise; findByIds(ids: BucketEntry['id'][]): Promise; findOneWithFrame(where: Partial): Promise & { frame?: Frame } | null>; findByIdsWithFrames(ids: BucketEntry['id'][]): Promise<(Omit & { frame?: Frame })[]>; diff --git a/lib/core/bucketEntries/usecase.ts b/lib/core/bucketEntries/usecase.ts index c9d97bc1..b1678e57 100644 --- a/lib/core/bucketEntries/usecase.ts +++ b/lib/core/bucketEntries/usecase.ts @@ -30,8 +30,7 @@ export class ShardBackedBucketError extends Error { } } -const isShardBackedEntry = (entry: BucketEntry): boolean => - Boolean(entry.frame) || Boolean(entry.index) || Boolean(entry.hmac); +const PURGE_BATCH_SIZE = 500; export class BucketEntryVersionNotFoundError extends Error { constructor() { @@ -278,40 +277,75 @@ export class BucketEntriesUsecase { * The entries this purges are metadata only: createEntry() writes a row with * a bucket, a size and a version, and nothing else. Nothing downstream of a * bucket entry exists for them, which is what makes a wholesale delete - * possible instead of walking each entry and its shards. Anything else in - * the bucket is refused, see ShardBackedBucketError. + * possible instead of walking each entry and its shards. + * + * A Drive bucket reaching this method would lose its files and strand their + * shards with nothing left to enumerate them by. The guards below read + * themselves, but note that the last of them - the delete only ever matching + * metadata-only entries - holds even if every check above it is wrong. + * + * BucketsUsecase.deleteBucketByIdAndUser is the legacy-stack equivalent; it + * drops the bucket document alone and does no usage accounting. */ async removeBucketAndEntries( userUuid: User['uuid'], - bucketId: Bucket['id'] + bucketId: Bucket['id'], + bucketName: Bucket['name'] ): Promise { - const [user, bucket, sample] = await Promise.all([ + const [user, bucket] = await Promise.all([ this.usersRepository.findByUuid(userUuid), this.bucketsRepository.findOne({ id: bucketId }), - this.bucketEntriesRepository.findOne({ bucket: bucketId }), ]); if (!user) { throw new UserNotFoundError(userUuid); } - if (bucket && bucket.userId !== userUuid) { + if (!bucket) { + throw new BucketNotFoundError(); + } + + if (bucket.userId !== userUuid) { throw new BucketForbiddenError(); } - if (sample && isShardBackedEntry(sample)) { + if (bucket.name !== bucketName) { + throw new BucketNotFoundError(); + } + + if (await this.bucketEntriesRepository.hasShardBackedEntriesByBucket(bucketId)) { throw new ShardBackedBucketError(); } - await this.bucketsRepository.removeByIdAndUser(bucketId, userUuid); + let totalUsedSpaceBytes = user.totalUsedSpaceBytes; + + for (;;) { + const entries = await this.bucketEntriesRepository.findMetadataOnlyByBucket( + bucketId, + PURGE_BATCH_SIZE + ); + + if (entries.length === 0) { + break; + } - const releasedBytes = await this.bucketEntriesRepository.sumSizeByBucket(bucketId); + const releasedBytes = entries.reduce((total, entry) => total + (entry.size ?? 0), 0); - await this.bucketEntriesRepository.deleteByBucket(bucketId); + await this.bucketEntriesRepository.deleteByIds(entries.map((entry) => entry.id)); - const totalUsedSpaceBytes = releasedBytes === 0 - ? user.totalUsedSpaceBytes - : await this.usersRepository.addTotalUsedSpaceBytes(userUuid, -releasedBytes); + if (releasedBytes > 0) { + totalUsedSpaceBytes = await this.usersRepository.addTotalUsedSpaceBytes( + userUuid, + -releasedBytes + ); + } + } + + if (await this.bucketEntriesRepository.hasEntriesByBucket(bucketId)) { + throw new ShardBackedBucketError(); + } + + await this.bucketsRepository.removeByIdAndUser(bucketId, userUuid); return { maxSpaceBytes: user.maxSpaceBytes, diff --git a/lib/server/http/gateway/controller.ts b/lib/server/http/gateway/controller.ts index 71647fb0..cd3cc826 100644 --- a/lib/server/http/gateway/controller.ts +++ b/lib/server/http/gateway/controller.ts @@ -206,21 +206,26 @@ export class HTTPGatewayController { } async deleteUserBucket( - req: Request<{ uuid: string; id: string }>, + req: Request<{ uuid: string; id: string }, {}, {}, { name?: unknown }>, res: Response ) { const { uuid, id } = req.params; + const { name } = req.query; if (!uuid || !uuidValidate(uuid) || !id || !OBJECT_ID_PATTERN.test(id)) { return res.status(400).send({ message: 'Invalid params' }); } + if (typeof name !== 'string' || name.length === 0) { + return res.status(400).send({ message: 'name is required' }); + } + try { - const snapshot = await this.bucketEntriesUsecase.removeBucketAndEntries(uuid, id); + const snapshot = await this.bucketEntriesUsecase.removeBucketAndEntries(uuid, id, name); return res.status(200).send(snapshot); } catch (err) { - if (err instanceof UserNotFoundError) { + if (err instanceof UserNotFoundError || err instanceof BucketNotFoundError) { return res.status(404).send({ message: err.message }); } diff --git a/tests/lib/core/bucketentries/usecase.test.ts b/tests/lib/core/bucketentries/usecase.test.ts index af2a4a0a..4558e14f 100644 --- a/tests/lib/core/bucketentries/usecase.test.ts +++ b/tests/lib/core/bucketentries/usecase.test.ts @@ -1,7 +1,7 @@ import { restore, stub } from 'sinon'; import { Model } from 'mongoose'; -import { BucketEntriesRepository } from '../../../../lib/core/bucketEntries/Repository'; +import { BucketEntriesRepository, MetadataOnlyEntry } from '../../../../lib/core/bucketEntries/Repository'; import { FramesRepository } from '../../../../lib/core/frames/Repository'; import { MirrorsRepository } from '../../../../lib/core/mirrors/Repository'; import { ShardsRepository } from '../../../../lib/core/shards/Repository'; @@ -759,138 +759,188 @@ describe('BucketEntriesUsecase', function () { const stubRepositories = ( user: User | null, bucket: Bucket | null, - entry: BucketEntry | null, - { releasedBytes = 0, newTotal = 0 } = {} + { + shardBacked = false, + pages = [] as MetadataOnlyEntry[][], + remaining = false, + newTotal = 0, + } = {} ) => { stub(usersRepository, 'findByUuid').resolves(user); stub(bucketsRepository, 'findOne').resolves(bucket); - stub(bucketEntriesRepository, 'findOne').resolves(entry); + + const findEntries = stub(bucketEntriesRepository, 'findMetadataOnlyByBucket'); + pages.forEach((page, call) => findEntries.onCall(call).resolves(page)); + findEntries.resolves([]); return { - sumSizes: stub(bucketEntriesRepository, 'sumSizeByBucket').resolves(releasedBytes), - deleteEntries: stub(bucketEntriesRepository, 'deleteByBucket').resolves(), + findEntries, + hasShardBacked: stub(bucketEntriesRepository, 'hasShardBackedEntriesByBucket').resolves(shardBacked), + hasRemaining: stub(bucketEntriesRepository, 'hasEntriesByBucket').resolves(remaining), + deleteEntries: stub(bucketEntriesRepository, 'deleteByIds').resolves(), addUsage: stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(newTotal), removeBucket: stub(bucketsRepository, 'removeByIdAndUser').resolves(), }; }; const getOwner = () => fixtures.getUser({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + const getMailBucket = (user: User) => + fixtures.getBucket({ userId: user.uuid, name: 'address-uuid' }); it('When the user does not exist, then it throws UserNotFoundError', async () => { - const { deleteEntries, removeBucket } = stubRepositories(null, null, null); + const { deleteEntries, removeBucket, addUsage } = stubRepositories(null, null); - try { - await bucketEntriesUsecase.removeBucketAndEntries('unknown-uuid', 'bucket-id'); - expect(true).toBeFalsy(); - } catch (err) { - expect(err).toBeInstanceOf(UserNotFoundError); - } + await expect( + bucketEntriesUsecase.removeBucketAndEntries('unknown-uuid', 'bucket-id', 'address-uuid') + ).rejects.toBeInstanceOf(UserNotFoundError); expect(deleteEntries.called).toBeFalsy(); expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); }); - it('When the bucket belongs to another user, then it throws BucketForbiddenError and removes nothing', async () => { + it('When the bucket document does not exist, then it throws BucketNotFoundError and drains nothing', async () => { const user = getOwner(); - const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, fixtures.getBucket(), null); + const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, null); - try { - await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id'); - expect(true).toBeFalsy(); - } catch (err) { - expect(err).toBeInstanceOf(BucketForbiddenError); - } + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id', 'address-uuid') + ).rejects.toBeInstanceOf(BucketNotFoundError); expect(deleteEntries.called).toBeFalsy(); expect(removeBucket.called).toBeFalsy(); expect(addUsage.called).toBeFalsy(); }); - it.each([ - ['a frame', { frame: 'frame-id', index: undefined, hmac: undefined }], - ['an index', { frame: undefined, index: 'file-index' }], - ['an hmac', { frame: undefined, index: undefined, hmac: { type: 'sha512' as const, value: 'v' } }], - ])( - 'When the bucket holds entries with %s, then it refuses and leaves the bucket alone', - async (_case, entryFields) => { - const user = getOwner(); - const bucket = fixtures.getBucket({ userId: user.uuid }); - const { deleteEntries, removeBucket, addUsage } = stubRepositories( - user, - bucket, - fixtures.getBucketEntry({ bucket: bucket.id, ...entryFields }) - ); - - try { - await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); - expect(true).toBeFalsy(); - } catch (err) { - expect(err).toBeInstanceOf(ShardBackedBucketError); - } - - expect(deleteEntries.called).toBeFalsy(); - expect(removeBucket.called).toBeFalsy(); - expect(addUsage.called).toBeFalsy(); - } - ); - - it.each([ - ['the bucket is empty', null], - ['the entries carry no size', { size: undefined }], - ])('When %s, then it is removed and the total is untouched', async (_case, entryFields) => { + it('When the bucket belongs to another user, then it throws BucketForbiddenError and removes nothing', async () => { const user = getOwner(); - const bucket = fixtures.getBucket({ userId: user.uuid }); - const { deleteEntries, addUsage, removeBucket } = stubRepositories( + const { deleteEntries, removeBucket, addUsage } = stubRepositories( user, - bucket, - entryFields && fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, ...entryFields }) + fixtures.getBucket({ name: 'address-uuid' }) ); - const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id', 'address-uuid') + ).rejects.toBeInstanceOf(BucketForbiddenError); - expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); expect(addUsage.called).toBeFalsy(); - expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); - expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); }); - it('When the bucket has entries, then they go in one delete and their total size is released', async () => { + it('When the name does not match the bucket, then it throws BucketNotFoundError and removes nothing', async () => { const user = getOwner(); - const bucket = fixtures.getBucket({ userId: user.uuid }); - const { sumSizes, deleteEntries, addUsage, removeBucket } = stubRepositories( - user, - bucket, - fixtures.getBucketEntry({ bucket: bucket.id, frame: undefined, index: undefined, size: 500 }), - { releasedBytes: 2000, newTotal: 2000 } - ); + const driveBucket = fixtures.getBucket({ userId: user.uuid, name: 'Bucket-a1b2c3' }); + const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, driveBucket); + + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, driveBucket.id, 'address-uuid') + ).rejects.toBeInstanceOf(BucketNotFoundError); + + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); + }); + + it('When a single shard-backed entry exists among many, then it refuses and leaves the bucket alone', async () => { + const user = getOwner(); + const bucket = getMailBucket(user); + const { findEntries, deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { + shardBacked: true, + pages: [[{ id: 'entry-1', size: 500 }]], + }); - const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id); + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) + ).rejects.toBeInstanceOf(ShardBackedBucketError); - expect(sumSizes.calledOnceWithExactly(bucket.id)).toBeTruthy(); - expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); - expect(addUsage.calledOnceWithExactly(user.uuid, -2000)).toBeTruthy(); + expect(findEntries.called).toBeFalsy(); + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); + }); + + it('When entries survive the drain, then the bucket document is kept', async () => { + const user = getOwner(); + const bucket = getMailBucket(user); + const { removeBucket } = stubRepositories(user, bucket, { + pages: [[{ id: 'entry-1', size: 500 }]], + remaining: true, + newTotal: 3500, + }); + + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) + ).rejects.toBeInstanceOf(ShardBackedBucketError); + + expect(removeBucket.called).toBeFalsy(); + }); + + it('When the bucket is empty, then it is removed and the total is untouched', async () => { + const user = getOwner(); + const bucket = getMailBucket(user); + const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries( + user.uuid, + bucket.id, + bucket.name + ); + + expect(deleteEntries.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); - expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 2000 }); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); + }); - // An entry created between the sum and the delete would be removed - // without its size ever being released. - expect(removeBucket.calledBefore(sumSizes)).toBeTruthy(); - expect(sumSizes.calledBefore(deleteEntries)).toBeTruthy(); + it('When the entries carry no size, then they are deleted and nothing is charged back', async () => { + const user = getOwner(); + const bucket = getMailBucket(user); + const entry = { id: 'entry-1', size: undefined }; + const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { + pages: [[entry]], + }); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries( + user.uuid, + bucket.id, + bucket.name + ); + + expect(deleteEntries.calledOnceWithExactly([entry.id])).toBeTruthy(); + expect(addUsage.called).toBeFalsy(); + expect(removeBucket.called).toBeTruthy(); + expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); }); - it('When the bucket document is already gone, then it still drains the entries it left behind', async () => { + it('When the entries span several batches, then each batch releases exactly its own bytes', async () => { const user = getOwner(); - const { deleteEntries, addUsage } = stubRepositories( - user, - null, - fixtures.getBucketEntry({ bucket: 'bucket-id', frame: undefined, index: undefined }), - { releasedBytes: 1000, newTotal: 3000 } + const bucket = getMailBucket(user); + const first = [{ id: 'entry-1', size: 500 }, { id: 'entry-2', size: 300 }]; + const second = [{ id: 'entry-3', size: 200 }]; + + const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { + pages: [first, second], + newTotal: 3000, + }); + + const snapshot = await bucketEntriesUsecase.removeBucketAndEntries( + user.uuid, + bucket.id, + bucket.name ); - const snapshot = await bucketEntriesUsecase.removeBucketAndEntries(user.uuid, 'bucket-id'); + expect(deleteEntries.callCount).toBe(2); + expect(deleteEntries.firstCall.calledWithExactly(first.map((e) => e.id))).toBeTruthy(); + expect(deleteEntries.secondCall.calledWithExactly(second.map((e) => e.id))).toBeTruthy(); + + expect(addUsage.callCount).toBe(2); + expect(addUsage.firstCall.calledWithExactly(user.uuid, -800)).toBeTruthy(); + expect(addUsage.secondCall.calledWithExactly(user.uuid, -200)).toBeTruthy(); + + expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); + expect(deleteEntries.secondCall.calledBefore(removeBucket.firstCall)).toBeTruthy(); - expect(deleteEntries.calledOnceWithExactly('bucket-id')).toBeTruthy(); - expect(addUsage.calledOnceWithExactly(user.uuid, -1000)).toBeTruthy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 3000 }); }); }); diff --git a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts index def8f6c3..ac2adced 100644 --- a/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts +++ b/tests/lib/e2e/gateway/gateway-v2.e2e-spec.ts @@ -12,6 +12,8 @@ const MB100 = 100 * 1024 * 1024; describe('Gateway V2 e2e tests', () => { const databaseConnection = new StorageDbManager(); + const randomObjectId = () => crypto.randomBytes(12).toString('hex') + const createBucketForUser = async (userUuid: string, jwt: string) => { const { body } = await testServer .post(`/v2/gateway/users/${userUuid}/buckets`) @@ -179,8 +181,6 @@ describe('Gateway V2 e2e tests', () => { }) describe('Bucket entries', () => { - const randomObjectId = () => crypto.randomBytes(12).toString('hex') - it('When creating an entry, then it is persisted as a shard-less v2 entry and the user total grows by its size', async () => { const testUser = await createTestUser() const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) @@ -371,9 +371,9 @@ describe('Gateway V2 e2e tests', () => { describe('Deleting a user bucket', () => { const VALID_OBJECT_ID = 'a'.repeat(24) - const deleteBucket = (userUuid: string, bucketId: string, jwt: string) => + const deleteBucket = (userUuid: string, bucketId: string, jwt: string, name: string) => testServer - .delete(`/v2/gateway/users/${userUuid}/buckets/${bucketId}`) + .delete(`/v2/gateway/users/${userUuid}/buckets/${bucketId}?name=${encodeURIComponent(name)}`) .set('Authorization', `Bearer ${jwt}`) const usedSpaceOf = async (uuid: string): Promise => @@ -398,7 +398,9 @@ describe('Gateway V2 e2e tests', () => { await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 3000) - const response = await deleteBucket(testUser.uuid, bucket.id, jwt) + expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore + 8000) + + const response = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) expect(response.status).toBe(200) @@ -423,7 +425,7 @@ describe('Gateway V2 e2e tests', () => { const usedBefore = await usedSpaceOf(testUser.uuid) - const response = await deleteBucket(testUser.uuid, bucket.id, jwt) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) expect(response.status).toBe(200) @@ -433,21 +435,21 @@ describe('Gateway V2 e2e tests', () => { expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) - it('When deleting the same bucket twice, then the second call succeeds and the total does not move again', async () => { + it('When deleting the same bucket twice, then the second call reports it gone and the total does not move again', async () => { const testUser = await createTestUser() const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) - const first = await deleteBucket(testUser.uuid, bucket.id, jwt) + const first = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) const usedAfterFirst = await usedSpaceOf(testUser.uuid) - const second = await deleteBucket(testUser.uuid, bucket.id, jwt) + const second = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) expect(first.status).toBe(200) - expect(second.status).toBe(200) + expect(second.status).toBe(404) expect(await usedSpaceOf(testUser.uuid)).toBe(usedAfterFirst) }) @@ -462,7 +464,7 @@ describe('Gateway V2 e2e tests', () => { const ownerUsedBefore = await usedSpaceOf(owner.uuid) - const response = await deleteBucket(otherUser.uuid, bucket.id, jwt) + const response = await deleteBucket(otherUser.uuid, bucket.id, jwt, bucket.name) expect(response.status).toBe(403) @@ -478,49 +480,44 @@ describe('Gateway V2 e2e tests', () => { it('When deleting a bucket of an unknown user, then it returns 404', async () => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) - const response = await deleteBucket(crypto.randomUUID(), VALID_OBJECT_ID, jwt) + const response = await deleteBucket(crypto.randomUUID(), VALID_OBJECT_ID, jwt, 'any-name') expect(response.status).toBe(404) }) - it('When entries are left behind by a bucket that no longer exists, then they are drained anyway', async () => { + it('When entries are left behind by a bucket that no longer exists, then they are left alone', async () => { const testUser = await createTestUser() const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) - const usedBefore = await usedSpaceOf(testUser.uuid) - await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + const usedBefore = await usedSpaceOf(testUser.uuid) + await databaseConnection.models.Bucket.deleteOne({ _id: bucket.id }) - const response = await deleteBucket(testUser.uuid, bucket.id, jwt) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) - expect(response.status).toBe(200) + expect(response.status).toBe(404) const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) - expect(entriesInDatabase.length).toBe(0) + expect(entriesInDatabase.length).toBe(1) expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) - it('When the bucket holds shard-backed entries, then it refuses and touches nothing', async () => { + it('When the name does not match the bucket, then it refuses and touches nothing', async () => { const testUser = await createTestUser() const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) const bucket = await createBucketForUser(testUser.uuid, jwt) await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) - await databaseConnection.models.BucketEntry.updateMany( - { bucket: bucket.id }, - { $set: { index: 'a'.repeat(64) } } - ) - const usedBefore = await usedSpaceOf(testUser.uuid) - const response = await deleteBucket(testUser.uuid, bucket.id, jwt) + const response = await deleteBucket(testUser.uuid, bucket.id, jwt, 'Bucket-a1b2c3') - expect(response.status).toBe(409) + expect(response.status).toBe(404) const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) expect(bucketInDatabase).not.toBeNull() @@ -531,22 +528,73 @@ describe('Gateway V2 e2e tests', () => { expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) }) + it.each([ + ['an index', { index: 'a'.repeat(64) }], + ['a frame', { frame: randomObjectId() }], + ['an hmac', { hmac: { type: 'sha512', value: 'a'.repeat(128) } }], + ])( + 'When one entry among several carries %s, then it refuses and every entry survives', + async (_case, shardBackedFields) => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + const metadataOnly = await createEntryForBucket(testUser.uuid, bucket.id, jwt, 5000) + const shardBacked = await createEntryForBucket(testUser.uuid, bucket.id, jwt, 3000) + + await databaseConnection.models.BucketEntry.updateOne( + { _id: shardBacked.id }, + { $set: shardBackedFields } + ) + + const usedBefore = await usedSpaceOf(testUser.uuid) + + const response = await deleteBucket(testUser.uuid, bucket.id, jwt, bucket.name) + + expect(response.status).toBe(409) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).not.toBeNull() + + const entriesInDatabase = await databaseConnection.models.BucketEntry.find({ bucket: bucket.id }) + expect(entriesInDatabase.map((e: { _id: unknown }) => String(e._id)).sort()) + .toEqual([metadataOnly.id, shardBacked.id].sort()) + + expect(await usedSpaceOf(testUser.uuid)).toBe(usedBefore) + } + ) + it.each([ ['the user uuid is malformed', 'not-a-uuid', VALID_OBJECT_ID], ['the bucket id is malformed', crypto.randomUUID(), 'not-an-object-id'], ])('When %s, then it returns 400', async (_case, userUuid, bucketId) => { const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) - const response = await deleteBucket(userUuid, bucketId, jwt) + const response = await deleteBucket(userUuid, bucketId, jwt, 'any-name') expect(response.status).toBe(400) }) + it('When the name is missing, then it returns 400', async () => { + const testUser = await createTestUser() + const jwt = signRS256JWT('5m', engine._config.gateway.SIGN_JWT_SECRET) + const bucket = await createBucketForUser(testUser.uuid, jwt) + + const response = await testServer + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${bucket.id}`) + .set('Authorization', `Bearer ${jwt}`) + + expect(response.status).toBe(400) + + const bucketInDatabase = await databaseConnection.models.Bucket.findOne({ _id: bucket.id }) + expect(bucketInDatabase).not.toBeNull() + }) + it('When no auth token is provided, then it returns 401', async () => { const testUser = await createTestUser() const response = await testServer - .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${VALID_OBJECT_ID}`) + .delete(`/v2/gateway/users/${testUser.uuid}/buckets/${VALID_OBJECT_ID}?name=any-name`) expect(response.status).toBe(401) }) From eba1f0a7a3d26a703f0dd05e804e4a76e2f63dc6 Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Thu, 27 Aug 2026 17:59:19 -0600 Subject: [PATCH 4/5] feat: Refactor MongoDBBucketEntriesRepository to include methods for summarizing bucket entries and deleting metadata-only entries. Update the repository interface accordingly. Enhance bucket entry use case to utilize new summary method and improve logging during bucket deletion. Update tests to reflect changes in repository methods and ensure proper functionality. --- .../MongoDBBucketEntriesRepository.ts | 70 ++++++++++------ lib/core/bucketEntries/Repository.ts | 8 +- lib/core/bucketEntries/usecase.ts | 39 ++++----- tests/lib/core/bucketentries/usecase.test.ts | 79 +++++++++++-------- 4 files changed, 116 insertions(+), 80 deletions(-) diff --git a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts index a67db2b3..6dad8dc4 100644 --- a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts +++ b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts @@ -1,6 +1,6 @@ import { Frame } from '../frames/Frame'; import { BucketEntry, BucketEntryWithFrame } from './BucketEntry'; -import { BucketEntriesRepository, MetadataOnlyEntry } from './Repository'; +import { BucketEntriesRepository, BucketEntriesSummary } from './Repository'; import { ObjectId } from 'mongodb'; /** @@ -10,11 +10,19 @@ import { ObjectId } from 'mongodb'; */ const SHARD_MARKERS = ['frame', 'index', 'hmac.value']; -const markersExist = (exists: boolean) => - SHARD_MARKERS.map((field) => ({ [field]: { $exists: exists } })); +const METADATA_ONLY = { + $and: SHARD_MARKERS.map((field) => ({ [field]: { $exists: false } })), +}; + +const SHARD_BACKED = { + $or: SHARD_MARKERS.map((field) => ({ [field]: { $exists: true } })), +}; -const SHARD_BACKED = { $or: markersExist(true) }; -const METADATA_ONLY = { $and: markersExist(false) }; +const IS_SHARD_BACKED = { + $or: SHARD_MARKERS.map((field) => ({ + $ne: [{ $type: `$${field}` }, 'missing'], + })), +}; interface BucketEntryModel extends Omit { _id: string; @@ -103,39 +111,51 @@ export class MongoDBBucketEntriesRepository implements BucketEntriesRepository { return bucketEntries.map(formatFromMongoToBucketEntry); } - private async existsByBucket(bucketId: string, filter = {}): Promise { + async hasEntriesByBucket(bucketId: string): Promise { const found = await this.model - .countDocuments({ bucket: bucketId, ...filter }, { limit: 1 }) + .countDocuments({ bucket: bucketId }, { limit: 1 }) .read('primary') .exec(); return found > 0; } - hasEntriesByBucket(bucketId: string): Promise { - return this.existsByBucket(bucketId); - } + async hasShardBackedEntriesByBucket(bucketId: string): Promise { + const found = await this.model + .countDocuments({ bucket: bucketId, ...SHARD_BACKED }, { limit: 1 }) + .read('primary') + .exec(); - hasShardBackedEntriesByBucket(bucketId: string): Promise { - return this.existsByBucket(bucketId, SHARD_BACKED); + return found > 0; } - async findMetadataOnlyByBucket( - bucketId: string, - limit: number - ): Promise { - const bucketEntries = await this.model - .find({ bucket: bucketId, ...METADATA_ONLY }) - .select('_id size') - .limit(limit) + async summarizeByBucket(bucketId: string): Promise { + const [summary] = await this.model + .aggregate([ + { $match: { bucket: new ObjectId(bucketId) } }, + { + $group: { + _id: null, + shardBackedCount: { $sum: { $cond: [IS_SHARD_BACKED, 1, 0] } }, + metadataOnlyBytes: { + $sum: { + $cond: [IS_SHARD_BACKED, 0, { $ifNull: ['$size', 0] }], + }, + }, + }, + }, + ]) .read('primary') - .lean() .exec(); - return bucketEntries.map((entry: { _id: unknown; size?: number }) => ({ - id: String(entry._id), - size: entry.size, - })); + return { + shardBackedCount: summary?.shardBackedCount ?? 0, + metadataOnlyBytes: summary?.metadataOnlyBytes ?? 0, + }; + } + + async deleteMetadataOnlyByBucket(bucketId: string): Promise { + await this.model.deleteMany({ bucket: bucketId, ...METADATA_ONLY }); } async findByIds(ids: string[]): Promise { diff --git a/lib/core/bucketEntries/Repository.ts b/lib/core/bucketEntries/Repository.ts index 26e5e02f..531f59d3 100644 --- a/lib/core/bucketEntries/Repository.ts +++ b/lib/core/bucketEntries/Repository.ts @@ -2,7 +2,10 @@ import { Bucket } from "../buckets/Bucket"; import { Frame } from "../frames/Frame"; import { BucketEntry, BucketEntryWithFrame } from "./BucketEntry"; -export type MetadataOnlyEntry = Pick; +export interface BucketEntriesSummary { + shardBackedCount: number; + metadataOnlyBytes: number; +} export interface BucketEntriesRepository { count(where: Partial): Promise; @@ -10,7 +13,8 @@ export interface BucketEntriesRepository { findByBucket(bucketId: Bucket['id'], limit: number, offset: number): Promise; hasEntriesByBucket(bucketId: Bucket['id']): Promise; hasShardBackedEntriesByBucket(bucketId: Bucket['id']): Promise; - findMetadataOnlyByBucket(bucketId: Bucket['id'], limit: number): Promise; + summarizeByBucket(bucketId: Bucket['id']): Promise; + deleteMetadataOnlyByBucket(bucketId: Bucket['id']): Promise; findByIds(ids: BucketEntry['id'][]): Promise; findOneWithFrame(where: Partial): Promise & { frame?: Frame } | null>; findByIdsWithFrames(ids: BucketEntry['id'][]): Promise<(Omit & { frame?: Frame })[]>; diff --git a/lib/core/bucketEntries/usecase.ts b/lib/core/bucketEntries/usecase.ts index b1678e57..8bbdbf32 100644 --- a/lib/core/bucketEntries/usecase.ts +++ b/lib/core/bucketEntries/usecase.ts @@ -1,5 +1,6 @@ import lodash from 'lodash'; +import log from '../../logger'; import { BucketsRepository } from '../buckets/Repository'; import { BucketEntriesRepository } from './Repository'; import { BucketNotFoundError, BucketForbiddenError, BucketEntryNotFoundError } from '../buckets/usecase'; @@ -30,8 +31,6 @@ export class ShardBackedBucketError extends Error { } } -const PURGE_BATCH_SIZE = 500; - export class BucketEntryVersionNotFoundError extends Error { constructor() { super('BucketEntryVersion not found'); @@ -317,30 +316,32 @@ export class BucketEntriesUsecase { throw new ShardBackedBucketError(); } - let totalUsedSpaceBytes = user.totalUsedSpaceBytes; + const { shardBackedCount, metadataOnlyBytes } = + await this.bucketEntriesRepository.summarizeByBucket(bucketId); - for (;;) { - const entries = await this.bucketEntriesRepository.findMetadataOnlyByBucket( - bucketId, - PURGE_BATCH_SIZE - ); + if (shardBackedCount > 0) { + throw new ShardBackedBucketError(); + } - if (entries.length === 0) { - break; - } + let totalUsedSpaceBytes = user.totalUsedSpaceBytes; - const releasedBytes = entries.reduce((total, entry) => total + (entry.size ?? 0), 0); + log.info( + `[removeBucketAndEntries] purging bucket - userUuid: ${userUuid}, bucketId: ${bucketId}, metadataOnlyBytes: ${metadataOnlyBytes}` + ); - await this.bucketEntriesRepository.deleteByIds(entries.map((entry) => entry.id)); + await this.bucketEntriesRepository.deleteMetadataOnlyByBucket(bucketId); - if (releasedBytes > 0) { - totalUsedSpaceBytes = await this.usersRepository.addTotalUsedSpaceBytes( - userUuid, - -releasedBytes - ); - } + if (metadataOnlyBytes > 0) { + totalUsedSpaceBytes = await this.usersRepository.addTotalUsedSpaceBytes( + userUuid, + -metadataOnlyBytes + ); } + log.info( + `[removeBucketAndEntries] usage credited - userUuid: ${userUuid}, bucketId: ${bucketId}, releasedBytes: ${metadataOnlyBytes}, totalUsedSpaceBytes: ${totalUsedSpaceBytes}` + ); + if (await this.bucketEntriesRepository.hasEntriesByBucket(bucketId)) { throw new ShardBackedBucketError(); } diff --git a/tests/lib/core/bucketentries/usecase.test.ts b/tests/lib/core/bucketentries/usecase.test.ts index 4558e14f..8b62467c 100644 --- a/tests/lib/core/bucketentries/usecase.test.ts +++ b/tests/lib/core/bucketentries/usecase.test.ts @@ -1,7 +1,7 @@ import { restore, stub } from 'sinon'; import { Model } from 'mongoose'; -import { BucketEntriesRepository, MetadataOnlyEntry } from '../../../../lib/core/bucketEntries/Repository'; +import { BucketEntriesRepository } from '../../../../lib/core/bucketEntries/Repository'; import { FramesRepository } from '../../../../lib/core/frames/Repository'; import { MirrorsRepository } from '../../../../lib/core/mirrors/Repository'; import { ShardsRepository } from '../../../../lib/core/shards/Repository'; @@ -761,7 +761,8 @@ describe('BucketEntriesUsecase', function () { bucket: Bucket | null, { shardBacked = false, - pages = [] as MetadataOnlyEntry[][], + shardBackedCount = 0, + metadataOnlyBytes = 0, remaining = false, newTotal = 0, } = {} @@ -769,15 +770,16 @@ describe('BucketEntriesUsecase', function () { stub(usersRepository, 'findByUuid').resolves(user); stub(bucketsRepository, 'findOne').resolves(bucket); - const findEntries = stub(bucketEntriesRepository, 'findMetadataOnlyByBucket'); - pages.forEach((page, call) => findEntries.onCall(call).resolves(page)); - findEntries.resolves([]); - return { - findEntries, - hasShardBacked: stub(bucketEntriesRepository, 'hasShardBackedEntriesByBucket').resolves(shardBacked), + hasShardBacked: stub(bucketEntriesRepository, 'hasShardBackedEntriesByBucket').resolves( + shardBacked + ), + summarize: stub(bucketEntriesRepository, 'summarizeByBucket').resolves({ + shardBackedCount, + metadataOnlyBytes, + }), hasRemaining: stub(bucketEntriesRepository, 'hasEntriesByBucket').resolves(remaining), - deleteEntries: stub(bucketEntriesRepository, 'deleteByIds').resolves(), + deleteEntries: stub(bucketEntriesRepository, 'deleteMetadataOnlyByBucket').resolves(), addUsage: stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(newTotal), removeBucket: stub(bucketsRepository, 'removeByIdAndUser').resolves(), }; @@ -799,7 +801,7 @@ describe('BucketEntriesUsecase', function () { expect(addUsage.called).toBeFalsy(); }); - it('When the bucket document does not exist, then it throws BucketNotFoundError and drains nothing', async () => { + it('When the bucket document does not exist, then it throws BucketNotFoundError and purges nothing', async () => { const user = getOwner(); const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, null); @@ -842,29 +844,46 @@ describe('BucketEntriesUsecase', function () { expect(addUsage.called).toBeFalsy(); }); - it('When a single shard-backed entry exists among many, then it refuses and leaves the bucket alone', async () => { + it('When a single shard-backed entry exists among many, then it refuses without scanning the bucket', async () => { const user = getOwner(); const bucket = getMailBucket(user); - const { findEntries, deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { + const { summarize, deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { shardBacked: true, - pages: [[{ id: 'entry-1', size: 500 }]], + metadataOnlyBytes: 500, }); await expect( bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) ).rejects.toBeInstanceOf(ShardBackedBucketError); - expect(findEntries.called).toBeFalsy(); + expect(summarize.called).toBeFalsy(); expect(deleteEntries.called).toBeFalsy(); expect(removeBucket.called).toBeFalsy(); expect(addUsage.called).toBeFalsy(); }); - it('When entries survive the drain, then the bucket document is kept', async () => { + it('When the summary still reports shard-backed entries, then it refuses before deleting anything', async () => { + const user = getOwner(); + const bucket = getMailBucket(user); + const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { + shardBackedCount: 1, + metadataOnlyBytes: 500, + }); + + await expect( + bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) + ).rejects.toBeInstanceOf(ShardBackedBucketError); + + expect(deleteEntries.called).toBeFalsy(); + expect(removeBucket.called).toBeFalsy(); + expect(addUsage.called).toBeFalsy(); + }); + + it('When entries survive the purge, then the bucket document is kept', async () => { const user = getOwner(); const bucket = getMailBucket(user); const { removeBucket } = stubRepositories(user, bucket, { - pages: [[{ id: 'entry-1', size: 500 }]], + metadataOnlyBytes: 500, remaining: true, newTotal: 3500, }); @@ -879,7 +898,7 @@ describe('BucketEntriesUsecase', function () { it('When the bucket is empty, then it is removed and the total is untouched', async () => { const user = getOwner(); const bucket = getMailBucket(user); - const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket); + const { addUsage, removeBucket } = stubRepositories(user, bucket); const snapshot = await bucketEntriesUsecase.removeBucketAndEntries( user.uuid, @@ -887,7 +906,6 @@ describe('BucketEntriesUsecase', function () { bucket.name ); - expect(deleteEntries.called).toBeFalsy(); expect(addUsage.called).toBeFalsy(); expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); @@ -896,9 +914,8 @@ describe('BucketEntriesUsecase', function () { it('When the entries carry no size, then they are deleted and nothing is charged back', async () => { const user = getOwner(); const bucket = getMailBucket(user); - const entry = { id: 'entry-1', size: undefined }; const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { - pages: [[entry]], + metadataOnlyBytes: 0, }); const snapshot = await bucketEntriesUsecase.removeBucketAndEntries( @@ -907,20 +924,18 @@ describe('BucketEntriesUsecase', function () { bucket.name ); - expect(deleteEntries.calledOnceWithExactly([entry.id])).toBeTruthy(); + expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); expect(addUsage.called).toBeFalsy(); expect(removeBucket.called).toBeTruthy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 4000 }); }); - it('When the entries span several batches, then each batch releases exactly its own bytes', async () => { + it('When the bucket holds many entries, then one delete and one charge cover all of them', async () => { const user = getOwner(); const bucket = getMailBucket(user); - const first = [{ id: 'entry-1', size: 500 }, { id: 'entry-2', size: 300 }]; - const second = [{ id: 'entry-3', size: 200 }]; - const { deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { - pages: [first, second], + const { summarize, deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { + metadataOnlyBytes: 1000, newTotal: 3000, }); @@ -930,16 +945,12 @@ describe('BucketEntriesUsecase', function () { bucket.name ); - expect(deleteEntries.callCount).toBe(2); - expect(deleteEntries.firstCall.calledWithExactly(first.map((e) => e.id))).toBeTruthy(); - expect(deleteEntries.secondCall.calledWithExactly(second.map((e) => e.id))).toBeTruthy(); - - expect(addUsage.callCount).toBe(2); - expect(addUsage.firstCall.calledWithExactly(user.uuid, -800)).toBeTruthy(); - expect(addUsage.secondCall.calledWithExactly(user.uuid, -200)).toBeTruthy(); + expect(summarize.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(addUsage.calledOnceWithExactly(user.uuid, -1000)).toBeTruthy(); expect(removeBucket.calledOnceWithExactly(bucket.id, user.uuid)).toBeTruthy(); - expect(deleteEntries.secondCall.calledBefore(removeBucket.firstCall)).toBeTruthy(); + expect(deleteEntries.firstCall.calledBefore(removeBucket.firstCall)).toBeTruthy(); expect(snapshot).toStrictEqual({ maxSpaceBytes: 10000, totalUsedSpaceBytes: 3000 }); }); From be92f9a287c4936f59151b85bd2d84d3f4713d09 Mon Sep 17 00:00:00 2001 From: jzunigax2 <125698953+jzunigax2@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:20:38 -0600 Subject: [PATCH 5/5] refactor: Replace summarizeByBucket method with sumMetadataOnlyBytesByBucket in MongoDBBucketEntriesRepository. Update repository interface and adjust BucketEntriesUsecase to utilize the new method. Modify tests to reflect these changes and ensure proper functionality. --- .../MongoDBBucketEntriesRepository.ts | 31 ++++------------- lib/core/bucketEntries/Repository.ts | 7 +--- lib/core/bucketEntries/usecase.ts | 8 ++--- tests/lib/core/bucketentries/usecase.test.ts | 33 ++++--------------- 4 files changed, 16 insertions(+), 63 deletions(-) diff --git a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts index 6dad8dc4..fa9a7586 100644 --- a/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts +++ b/lib/core/bucketEntries/MongoDBBucketEntriesRepository.ts @@ -1,6 +1,6 @@ import { Frame } from '../frames/Frame'; import { BucketEntry, BucketEntryWithFrame } from './BucketEntry'; -import { BucketEntriesRepository, BucketEntriesSummary } from './Repository'; +import { BucketEntriesRepository } from './Repository'; import { ObjectId } from 'mongodb'; /** @@ -18,12 +18,6 @@ const SHARD_BACKED = { $or: SHARD_MARKERS.map((field) => ({ [field]: { $exists: true } })), }; -const IS_SHARD_BACKED = { - $or: SHARD_MARKERS.map((field) => ({ - $ne: [{ $type: `$${field}` }, 'missing'], - })), -}; - interface BucketEntryModel extends Omit { _id: string; created: Date; @@ -129,29 +123,16 @@ export class MongoDBBucketEntriesRepository implements BucketEntriesRepository { return found > 0; } - async summarizeByBucket(bucketId: string): Promise { - const [summary] = await this.model + async sumMetadataOnlyBytesByBucket(bucketId: string): Promise { + const [result] = await this.model .aggregate([ - { $match: { bucket: new ObjectId(bucketId) } }, - { - $group: { - _id: null, - shardBackedCount: { $sum: { $cond: [IS_SHARD_BACKED, 1, 0] } }, - metadataOnlyBytes: { - $sum: { - $cond: [IS_SHARD_BACKED, 0, { $ifNull: ['$size', 0] }], - }, - }, - }, - }, + { $match: { bucket: new ObjectId(bucketId), ...METADATA_ONLY } }, + { $group: { _id: null, bytes: { $sum: '$size' } } }, ]) .read('primary') .exec(); - return { - shardBackedCount: summary?.shardBackedCount ?? 0, - metadataOnlyBytes: summary?.metadataOnlyBytes ?? 0, - }; + return result?.bytes ?? 0; } async deleteMetadataOnlyByBucket(bucketId: string): Promise { diff --git a/lib/core/bucketEntries/Repository.ts b/lib/core/bucketEntries/Repository.ts index 531f59d3..fd847ad7 100644 --- a/lib/core/bucketEntries/Repository.ts +++ b/lib/core/bucketEntries/Repository.ts @@ -2,18 +2,13 @@ import { Bucket } from "../buckets/Bucket"; import { Frame } from "../frames/Frame"; import { BucketEntry, BucketEntryWithFrame } from "./BucketEntry"; -export interface BucketEntriesSummary { - shardBackedCount: number; - metadataOnlyBytes: number; -} - export interface BucketEntriesRepository { count(where: Partial): Promise; findOne(where: Partial): Promise; findByBucket(bucketId: Bucket['id'], limit: number, offset: number): Promise; hasEntriesByBucket(bucketId: Bucket['id']): Promise; hasShardBackedEntriesByBucket(bucketId: Bucket['id']): Promise; - summarizeByBucket(bucketId: Bucket['id']): Promise; + sumMetadataOnlyBytesByBucket(bucketId: Bucket['id']): Promise; deleteMetadataOnlyByBucket(bucketId: Bucket['id']): Promise; findByIds(ids: BucketEntry['id'][]): Promise; findOneWithFrame(where: Partial): Promise & { frame?: Frame } | null>; diff --git a/lib/core/bucketEntries/usecase.ts b/lib/core/bucketEntries/usecase.ts index 8bbdbf32..01254f41 100644 --- a/lib/core/bucketEntries/usecase.ts +++ b/lib/core/bucketEntries/usecase.ts @@ -316,12 +316,8 @@ export class BucketEntriesUsecase { throw new ShardBackedBucketError(); } - const { shardBackedCount, metadataOnlyBytes } = - await this.bucketEntriesRepository.summarizeByBucket(bucketId); - - if (shardBackedCount > 0) { - throw new ShardBackedBucketError(); - } + const metadataOnlyBytes = + await this.bucketEntriesRepository.sumMetadataOnlyBytesByBucket(bucketId); let totalUsedSpaceBytes = user.totalUsedSpaceBytes; diff --git a/tests/lib/core/bucketentries/usecase.test.ts b/tests/lib/core/bucketentries/usecase.test.ts index 8b62467c..c0e30bfc 100644 --- a/tests/lib/core/bucketentries/usecase.test.ts +++ b/tests/lib/core/bucketentries/usecase.test.ts @@ -761,7 +761,6 @@ describe('BucketEntriesUsecase', function () { bucket: Bucket | null, { shardBacked = false, - shardBackedCount = 0, metadataOnlyBytes = 0, remaining = false, newTotal = 0, @@ -774,10 +773,9 @@ describe('BucketEntriesUsecase', function () { hasShardBacked: stub(bucketEntriesRepository, 'hasShardBackedEntriesByBucket').resolves( shardBacked ), - summarize: stub(bucketEntriesRepository, 'summarizeByBucket').resolves({ - shardBackedCount, - metadataOnlyBytes, - }), + sumBytes: stub(bucketEntriesRepository, 'sumMetadataOnlyBytesByBucket').resolves( + metadataOnlyBytes + ), hasRemaining: stub(bucketEntriesRepository, 'hasEntriesByBucket').resolves(remaining), deleteEntries: stub(bucketEntriesRepository, 'deleteMetadataOnlyByBucket').resolves(), addUsage: stub(usersRepository, 'addTotalUsedSpaceBytes').resolves(newTotal), @@ -847,7 +845,7 @@ describe('BucketEntriesUsecase', function () { it('When a single shard-backed entry exists among many, then it refuses without scanning the bucket', async () => { const user = getOwner(); const bucket = getMailBucket(user); - const { summarize, deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { + const { sumBytes, deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { shardBacked: true, metadataOnlyBytes: 500, }); @@ -856,24 +854,7 @@ describe('BucketEntriesUsecase', function () { bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) ).rejects.toBeInstanceOf(ShardBackedBucketError); - expect(summarize.called).toBeFalsy(); - expect(deleteEntries.called).toBeFalsy(); - expect(removeBucket.called).toBeFalsy(); - expect(addUsage.called).toBeFalsy(); - }); - - it('When the summary still reports shard-backed entries, then it refuses before deleting anything', async () => { - const user = getOwner(); - const bucket = getMailBucket(user); - const { deleteEntries, removeBucket, addUsage } = stubRepositories(user, bucket, { - shardBackedCount: 1, - metadataOnlyBytes: 500, - }); - - await expect( - bucketEntriesUsecase.removeBucketAndEntries(user.uuid, bucket.id, bucket.name) - ).rejects.toBeInstanceOf(ShardBackedBucketError); - + expect(sumBytes.called).toBeFalsy(); expect(deleteEntries.called).toBeFalsy(); expect(removeBucket.called).toBeFalsy(); expect(addUsage.called).toBeFalsy(); @@ -934,7 +915,7 @@ describe('BucketEntriesUsecase', function () { const user = getOwner(); const bucket = getMailBucket(user); - const { summarize, deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { + const { sumBytes, deleteEntries, addUsage, removeBucket } = stubRepositories(user, bucket, { metadataOnlyBytes: 1000, newTotal: 3000, }); @@ -945,7 +926,7 @@ describe('BucketEntriesUsecase', function () { bucket.name ); - expect(summarize.calledOnceWithExactly(bucket.id)).toBeTruthy(); + expect(sumBytes.calledOnceWithExactly(bucket.id)).toBeTruthy(); expect(deleteEntries.calledOnceWithExactly(bucket.id)).toBeTruthy(); expect(addUsage.calledOnceWithExactly(user.uuid, -1000)).toBeTruthy();