From c27fd91b6602adcd1ed447ebdd2a9041b7dea376 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 4 Jun 2026 11:23:06 +0000 Subject: [PATCH 1/3] feat(storage): add deleteSourceObjects option to bucket.combine and introduce ComposeCleanupError for failed source deletions --- handwritten/storage/src/bucket.ts | 112 ++++++++++++++++++++------ handwritten/storage/src/index.ts | 1 + handwritten/storage/test/bucket.ts | 124 ++++++++++++++++++++++++++++- 3 files changed, 208 insertions(+), 29 deletions(-) diff --git a/handwritten/storage/src/bucket.ts b/handwritten/storage/src/bucket.ts index b003b546540d..8a6676a1d6bf 100644 --- a/handwritten/storage/src/bucket.ts +++ b/handwritten/storage/src/bucket.ts @@ -190,6 +190,7 @@ export interface CombineOptions extends PreconditionOptions { [key: string]: ContextValue; } | null; }; + deleteSourceObjects?: boolean; } export interface CombineCallback { @@ -198,6 +199,24 @@ export interface CombineCallback { export type CombineResponse = [File, unknown]; +export class ComposeCleanupError extends Error { + errors: Error[]; + newFile: File; + apiResponse: unknown; + constructor( + message: string, + errors: Error[], + newFile: File, + apiResponse: unknown + ) { + super(message); + this.name = 'ComposeCleanupError'; + this.errors = errors; + this.newFile = newFile; + this.apiResponse = apiResponse; + } +} + export interface CreateChannelConfig extends WatchAllOptions { address: string; } @@ -1579,7 +1598,9 @@ class Bucket extends ServiceObject { * metadata's `kms_key_name` value, if any. * @property {string} [userProject] The ID of the project which will be * billed for the request. - */ + * @property {boolean} [deleteSourceObjects] If true, the source objects + * will be permanently deleted after a successful compose operation. + */ /** * @callback CombineCallback * @param {?Error} err Request error, if any. @@ -1612,7 +1633,8 @@ class Bucket extends ServiceObject { * metadata's `kms_key_name` value, if any. * @param {string} [options.userProject] The ID of the project which will be * billed for the request. - + * @param {boolean} [options.deleteSourceObjects] If true, the source objects + * will be permanently deleted after a successful compose operation. * @param {CombineCallback} [callback] Callback function. * @returns {Promise} * @@ -1709,8 +1731,17 @@ class Bucket extends ServiceObject { maxRetries = 0; } - if (options.ifGenerationMatch === undefined) { - Object.assign(options, destinationFile.instancePreconditionOpts, options); + const deleteSourceObjects = options.deleteSourceObjects; + + const requestQueryObject = Object.assign({}, options); + delete requestQueryObject.deleteSourceObjects; + + if (requestQueryObject.ifGenerationMatch === undefined) { + Object.assign( + requestQueryObject, + destinationFile.instancePreconditionOpts, + requestQueryObject + ); } // Make the request from the destination File object. @@ -1719,37 +1750,66 @@ class Bucket extends ServiceObject { method: 'POST', uri: '/compose', maxRetries, - json: { - destination: { - contentType: destinationFile.metadata.contentType, - contentEncoding: destinationFile.metadata.contentEncoding, - contexts: options.contexts || destinationFile.metadata.contexts, + json: Object.assign( + { + destination: { + contentType: destinationFile.metadata.contentType, + contentEncoding: destinationFile.metadata.contentEncoding, + contexts: + requestQueryObject.contexts || destinationFile.metadata.contexts, + }, + sourceObjects: (sources as File[]).map(source => { + const sourceObject = { + name: source.name, + } as SourceObject; + + if (source.metadata && source.metadata.generation) { + sourceObject.generation = parseInt( + source.metadata.generation.toString() + ); + } + + return sourceObject; + }), }, - sourceObjects: (sources as File[]).map(source => { - const sourceObject = { - name: source.name, - } as SourceObject; - - if (source.metadata && source.metadata.generation) { - sourceObject.generation = parseInt( - source.metadata.generation.toString(), - ); - } - - return sourceObject; - }), - }, - qs: options, + deleteSourceObjects !== undefined ? {deleteSourceObjects} : {} + ), + qs: requestQueryObject, }, - (err, resp) => { + async (err, resp) => { this.storage.retryOptions.autoRetry = this.instanceRetryValue; if (err) { callback!(err, null, resp); return; } + if (deleteSourceObjects) { + const deletePromises = (sources as File[]).map(async source => { + try { + await source.delete(); + } catch (deleteErr) { + return deleteErr as Error; + } + return null; + }); + + const results = await Promise.all(deletePromises); + const errors = results.filter((res): res is Error => res !== null); + + if (errors.length > 0) { + const cleanupErr = new ComposeCleanupError( + `Compose operation succeeded, but cleaning up source objects failed. Failed to delete ${errors.length} source object(s).`, + errors, + destinationFile, + resp + ); + callback!(cleanupErr, destinationFile, resp); + return; + } + } + callback!(null, destinationFile, resp); - }, + } ); } diff --git a/handwritten/storage/src/index.ts b/handwritten/storage/src/index.ts index 32d2728bdeb2..f93ef6002e73 100644 --- a/handwritten/storage/src/index.ts +++ b/handwritten/storage/src/index.ts @@ -111,6 +111,7 @@ export { CombineCallback, CombineOptions, CombineResponse, + ComposeCleanupError, CreateChannelCallback, CreateChannelConfig, CreateChannelOptions, diff --git a/handwritten/storage/test/bucket.ts b/handwritten/storage/test/bucket.ts index 6e14bec68cf4..5fb231a42d4d 100644 --- a/handwritten/storage/test/bucket.ts +++ b/handwritten/storage/test/bucket.ts @@ -191,6 +191,8 @@ describe('Bucket', () => { let Bucket: any; // eslint-disable-next-line @typescript-eslint/no-explicit-any let bucket: any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let ComposeCleanupError: any; const STORAGE = { createBucket: util.noop, @@ -211,7 +213,7 @@ describe('Bucket', () => { const BUCKET_NAME = 'test-bucket'; before(() => { - Bucket = proxyquire('../src/bucket.js', { + const bucketModule = proxyquire('../src/bucket.js', { fs: fakeFs, 'p-limit': fakePLimit, '@google-cloud/promisify': fakePromisify, @@ -225,7 +227,9 @@ describe('Bucket', () => { './iam.js': {Iam: FakeIam}, './notification.js': {Notification: FakeNotification}, './signer.js': fakeSigner, - }).Bucket; + }); + Bucket = bucketModule.Bucket; + ComposeCleanupError = bucketModule.ComposeCleanupError; }); beforeEach(() => { @@ -806,7 +810,7 @@ describe('Bucket', () => { const destination = bucket.file('destination.txt'); destination.request = (reqOpts: DecorateRequestOptions) => { - assert.strictEqual(reqOpts.qs, options); + assert.deepStrictEqual(reqOpts.qs, options); done(); }; @@ -916,6 +920,120 @@ describe('Bucket', () => { bucket.combine(sources, destination, done); }); + + it('should delete source objects if deleteSourceObjects is true', done => { + const sources = [bucket.file('1.foo'), bucket.file('2.foo')]; + const destination = bucket.file('destination.foo'); + + let deletedCount = 0; + sources.forEach(source => { + source.delete = async () => { + deletedCount++; + return [{}]; + }; + }); + + destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { + assert.strictEqual(reqOpts.qs.deleteSourceObjects, undefined); + assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + callback(null, {}); + }; + + bucket.combine(sources, destination, {deleteSourceObjects: true}, (err: any) => { + assert.ifError(err); + assert.strictEqual(deletedCount, 2); + done(); + }); + }); + + it('should not delete source objects if deleteSourceObjects is false/omitted', done => { + const sources = [bucket.file('1.foo'), bucket.file('2.foo')]; + const destination = bucket.file('destination.foo'); + + let deletedCount = 0; + sources.forEach(source => { + source.delete = async () => { + deletedCount++; + return [{}]; + }; + }); + + destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { + assert.strictEqual(reqOpts.json.deleteSourceObjects, undefined); + callback(null, {}); + }; + + bucket.combine(sources, destination, (err: any) => { + assert.ifError(err); + assert.strictEqual(deletedCount, 0); + done(); + }); + }); + + it('should not delete source objects if compose operation fails', done => { + const sources = [bucket.file('1.foo'), bucket.file('2.foo')]; + const destination = bucket.file('destination.foo'); + const composeError = new Error('Compose failed.'); + + let deletedCount = 0; + sources.forEach(source => { + source.delete = async () => { + deletedCount++; + return [{}]; + }; + }); + + destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { + assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + callback(composeError); + }; + + bucket.combine(sources, destination, {deleteSourceObjects: true}, (err: any) => { + assert.strictEqual(err, composeError); + assert.strictEqual(deletedCount, 0); + done(); + }); + }); + + it('should return ComposeCleanupError if deleting source objects fails', done => { + const sources = [bucket.file('1.foo'), bucket.file('2.foo')]; + const destination = bucket.file('destination.foo'); + const deleteError = new Error('Delete failed.'); + + sources[0].delete = async () => { + throw deleteError; + }; + sources[1].delete = async () => { + return [{}]; + }; + + destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { + assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + callback(null, {success: true}); + }; + + bucket.combine( + sources, + destination, + {deleteSourceObjects: true}, + (err: any, newFile: any, apiResponse: any) => { + try { + assert.ok(err instanceof ComposeCleanupError); + assert.strictEqual(err.name, 'ComposeCleanupError'); + assert.deepStrictEqual((err as any).errors, [deleteError]); + assert.strictEqual((err as any).newFile, destination); + assert.deepStrictEqual((err as any).apiResponse, {success: true}); + + // Also check callback arguments + assert.strictEqual(newFile, destination); + assert.deepStrictEqual(apiResponse, {success: true}); + done(); + } catch (assertErr) { + done(assertErr); + } + } + ); + }); }); describe('createChannel', () => { From e8922db5ae8ae56120a35409cf5bdb0d9b335efc Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Thu, 4 Jun 2026 11:41:13 +0000 Subject: [PATCH 2/3] feat(storage): support userProject in bucket.combine cleanup and update delete options interface --- handwritten/storage/src/bucket.ts | 47 +++++++++---------- .../src/nodejs-common/service-object.ts | 1 + handwritten/storage/test/bucket.ts | 32 ++++++++----- 3 files changed, 44 insertions(+), 36 deletions(-) diff --git a/handwritten/storage/src/bucket.ts b/handwritten/storage/src/bucket.ts index 8a6676a1d6bf..cfa47d7cf972 100644 --- a/handwritten/storage/src/bucket.ts +++ b/handwritten/storage/src/bucket.ts @@ -1750,30 +1750,27 @@ class Bucket extends ServiceObject { method: 'POST', uri: '/compose', maxRetries, - json: Object.assign( - { - destination: { - contentType: destinationFile.metadata.contentType, - contentEncoding: destinationFile.metadata.contentEncoding, - contexts: - requestQueryObject.contexts || destinationFile.metadata.contexts, - }, - sourceObjects: (sources as File[]).map(source => { - const sourceObject = { - name: source.name, - } as SourceObject; - - if (source.metadata && source.metadata.generation) { - sourceObject.generation = parseInt( - source.metadata.generation.toString() - ); - } - - return sourceObject; - }), + json: { + destination: { + contentType: destinationFile.metadata.contentType, + contentEncoding: destinationFile.metadata.contentEncoding, + contexts: + requestQueryObject.contexts || destinationFile.metadata.contexts, }, - deleteSourceObjects !== undefined ? {deleteSourceObjects} : {} - ), + sourceObjects: (sources as File[]).map(source => { + const sourceObject = { + name: source.name, + } as SourceObject; + + if (source.metadata && source.metadata.generation) { + sourceObject.generation = parseInt( + source.metadata.generation.toString() + ); + } + + return sourceObject; + }), + }, qs: requestQueryObject, }, async (err, resp) => { @@ -1786,7 +1783,9 @@ class Bucket extends ServiceObject { if (deleteSourceObjects) { const deletePromises = (sources as File[]).map(async source => { try { - await source.delete(); + await source.delete({ + userProject: options.userProject, + }); } catch (deleteErr) { return deleteErr as Error; } diff --git a/handwritten/storage/src/nodejs-common/service-object.ts b/handwritten/storage/src/nodejs-common/service-object.ts index 4f83189d525a..842ada149abc 100644 --- a/handwritten/storage/src/nodejs-common/service-object.ts +++ b/handwritten/storage/src/nodejs-common/service-object.ts @@ -111,6 +111,7 @@ export interface CreateCallback { export type DeleteOptions = { ignoreNotFound?: boolean; + userProject?: string; ifGenerationMatch?: number | string; ifGenerationNotMatch?: number | string; ifMetagenerationMatch?: number | string; diff --git a/handwritten/storage/test/bucket.ts b/handwritten/storage/test/bucket.ts index 5fb231a42d4d..6b8691733def 100644 --- a/handwritten/storage/test/bucket.ts +++ b/handwritten/storage/test/bucket.ts @@ -927,7 +927,8 @@ describe('Bucket', () => { let deletedCount = 0; sources.forEach(source => { - source.delete = async () => { + source.delete = async (opts?: any) => { + assert.strictEqual(opts?.userProject, 'user-project-id'); deletedCount++; return [{}]; }; @@ -935,15 +936,20 @@ describe('Bucket', () => { destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { assert.strictEqual(reqOpts.qs.deleteSourceObjects, undefined); - assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + assert.strictEqual(reqOpts.json.deleteSourceObjects, undefined); callback(null, {}); }; - bucket.combine(sources, destination, {deleteSourceObjects: true}, (err: any) => { - assert.ifError(err); - assert.strictEqual(deletedCount, 2); - done(); - }); + bucket.combine( + sources, + destination, + {deleteSourceObjects: true, userProject: 'user-project-id'}, + (err: any) => { + assert.ifError(err); + assert.strictEqual(deletedCount, 2); + done(); + } + ); }); it('should not delete source objects if deleteSourceObjects is false/omitted', done => { @@ -984,7 +990,7 @@ describe('Bucket', () => { }); destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { - assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + assert.strictEqual(reqOpts.json.deleteSourceObjects, undefined); callback(composeError); }; @@ -1000,22 +1006,24 @@ describe('Bucket', () => { const destination = bucket.file('destination.foo'); const deleteError = new Error('Delete failed.'); - sources[0].delete = async () => { + sources[0].delete = async (opts?: any) => { + assert.strictEqual(opts?.userProject, 'user-project-id'); throw deleteError; }; - sources[1].delete = async () => { + sources[1].delete = async (opts?: any) => { + assert.strictEqual(opts?.userProject, 'user-project-id'); return [{}]; }; destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { - assert.strictEqual(reqOpts.json.deleteSourceObjects, true); + assert.strictEqual(reqOpts.json.deleteSourceObjects, undefined); callback(null, {success: true}); }; bucket.combine( sources, destination, - {deleteSourceObjects: true}, + {deleteSourceObjects: true, userProject: 'user-project-id'}, (err: any, newFile: any, apiResponse: any) => { try { assert.ok(err instanceof ComposeCleanupError); From 6cfc6c047a234f58bfbf18dd22cfb6d22de60e73 Mon Sep 17 00:00:00 2001 From: Thiyagu K Date: Fri, 5 Jun 2026 09:28:51 +0000 Subject: [PATCH 3/3] fix(storage): update bucket combine method to respect file generation and ignore missing sources during deletion --- handwritten/storage/src/bucket.ts | 15 ++++++++++++--- handwritten/storage/test/bucket.ts | 26 +++++++++++++++++++------- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/handwritten/storage/src/bucket.ts b/handwritten/storage/src/bucket.ts index cfa47d7cf972..13ce52e92a1a 100644 --- a/handwritten/storage/src/bucket.ts +++ b/handwritten/storage/src/bucket.ts @@ -67,6 +67,7 @@ import {CRC32CValidatorGenerator} from './crc32c.js'; import {URL} from 'url'; import { BaseMetadata, + DeleteOptions, SetMetadataOptions, } from './nodejs-common/service-object.js'; @@ -1782,10 +1783,18 @@ class Bucket extends ServiceObject { if (deleteSourceObjects) { const deletePromises = (sources as File[]).map(async source => { + const deleteOptions: DeleteOptions = { + ignoreNotFound: true, + userProject: options.userProject, + }; + + const generation = source.generation ?? source.metadata?.generation; + if (generation !== undefined) { + deleteOptions.ifGenerationMatch = generation; + } + try { - await source.delete({ - userProject: options.userProject, - }); + await source.delete(deleteOptions); } catch (deleteErr) { return deleteErr as Error; } diff --git a/handwritten/storage/test/bucket.ts b/handwritten/storage/test/bucket.ts index 6b8691733def..0b02e33de6ad 100644 --- a/handwritten/storage/test/bucket.ts +++ b/handwritten/storage/test/bucket.ts @@ -925,14 +925,24 @@ describe('Bucket', () => { const sources = [bucket.file('1.foo'), bucket.file('2.foo')]; const destination = bucket.file('destination.foo'); + // Set generation on the first file and leave second file without generation + sources[0].generation = 12345; + let deletedCount = 0; - sources.forEach(source => { - source.delete = async (opts?: any) => { - assert.strictEqual(opts?.userProject, 'user-project-id'); - deletedCount++; - return [{}]; - }; - }); + sources[0].delete = async (opts?: any) => { + assert.strictEqual(opts?.userProject, 'user-project-id'); + assert.strictEqual(opts?.ignoreNotFound, true); + assert.strictEqual(opts?.ifGenerationMatch, 12345); + deletedCount++; + return [{}]; + }; + sources[1].delete = async (opts?: any) => { + assert.strictEqual(opts?.userProject, 'user-project-id'); + assert.strictEqual(opts?.ignoreNotFound, true); + assert.strictEqual(opts?.ifGenerationMatch, undefined); + deletedCount++; + return [{}]; + }; destination.request = (reqOpts: DecorateRequestOptions, callback: Function) => { assert.strictEqual(reqOpts.qs.deleteSourceObjects, undefined); @@ -1008,10 +1018,12 @@ describe('Bucket', () => { sources[0].delete = async (opts?: any) => { assert.strictEqual(opts?.userProject, 'user-project-id'); + assert.strictEqual(opts?.ignoreNotFound, true); throw deleteError; }; sources[1].delete = async (opts?: any) => { assert.strictEqual(opts?.userProject, 'user-project-id'); + assert.strictEqual(opts?.ignoreNotFound, true); return [{}]; };