diff --git a/handwritten/storage/conformance-test/conformanceCommon.ts b/handwritten/storage/conformance-test/conformanceCommon.ts index a743949a8875..8618d8b59deb 100644 --- a/handwritten/storage/conformance-test/conformanceCommon.ts +++ b/handwritten/storage/conformance-test/conformanceCommon.ts @@ -16,21 +16,15 @@ // eslint-disable-next-line @typescript-eslint/no-unused-vars import * as jsonToNodeApiMapping from './test-data/retryInvocationMap.json'; import * as libraryMethods from './libraryMethods.js'; -import { - Bucket, - File, - GaxiosOptions, - GaxiosOptionsPrepared, - HmacKey, - Notification, - Storage, -} from '../src'; +import {Bucket, File, HmacKey, Notification, Storage} from '../src'; +import * as gaxios from 'gaxios'; import * as crypto from 'crypto'; import * as assert from 'assert'; -import { - StorageRequestOptions, - StorageTransport, -} from '../src/storage-transport.js'; +import {StorageTransport} from '../src/storage-transport.js'; +import {getDirName} from '../src/util.js'; +import path from 'path'; +import * as fs from 'fs'; +import {GoogleAuth} from 'google-auth-library'; interface RetryCase { instructions: String[]; } @@ -60,7 +54,7 @@ interface ConformanceTestResult { type LibraryMethodsModuleType = typeof import('./libraryMethods'); const methodMap: Map = new Map( - Object.entries({}), // TODO: replace with Object.entries(jsonToNodeApiMapping) + Object.entries(jsonToNodeApiMapping), ); const DURATION_SECONDS = 600; // 10 mins. @@ -70,6 +64,27 @@ const TESTBENCH_HOST = const CONF_TEST_PROJECT_ID = 'my-project-id'; const TIMEOUT_FOR_INDIVIDUAL_TEST = 20000; const RETRY_MULTIPLIER_FOR_CONFORMANCE_TESTS = 0.01; +const SERVICE_ACCOUNT = path.join( + getDirName(), + '../../../conformance-test/fixtures/signing-service-account.json', +); + +const authClient = new GoogleAuth({ + keyFilename: SERVICE_ACCOUNT, + scopes: ['https://www.googleapis.com/auth/devstorage.full_control'], +}).fromJSON(JSON.parse(fs.readFileSync(SERVICE_ACCOUNT, 'utf8'))); + +authClient.getAccessToken = async () => ({token: 'unauthenticated-test-token'}); +authClient.request = async (opts: unknown) => { + const options = opts as gaxios.GaxiosOptions & { + adapter?: (opts: unknown) => Promise; + }; + if (typeof options.adapter === 'function') { + return options.adapter(opts) as Promise; + } + const defaultGaxios = gaxios as unknown as {instance: gaxios.Gaxios}; + return defaultGaxios.instance.request(options); +}; export function executeScenario(testCase: RetryTestCase) { for ( @@ -89,16 +104,22 @@ export function executeScenario(testCase: RetryTestCase) { let bucket: Bucket; let file: File; let notification: Notification; - let creationResult: {id: string}; + let creationResult: ConformanceTestCreationResult; let storage: Storage; let hmacKey: HmacKey; let storageTransport: StorageTransport; describe(`${storageMethodString}`, async () => { beforeEach(async () => { - storageTransport = new StorageTransport({ + const defaultGaxios = gaxios as unknown as { + instance?: gaxios.Gaxios; + }; + defaultGaxios.instance?.interceptors?.request?.clear(); + + const rawTransport = new StorageTransport({ apiEndpoint: TESTBENCH_HOST, - authClient: undefined, + authClient: authClient, + keyFilename: SERVICE_ACCOUNT, baseUrl: TESTBENCH_HOST, packageJson: {name: 'test-package', version: '1.0.0'}, retryOptions: { @@ -117,92 +138,120 @@ export function executeScenario(testCase: RetryTestCase) { timeout: DURATION_SECONDS, }); + creationResult = await createTestBenchRetryTest( + instructionSet.instructions, + jsonMethod?.name.toString(), + rawTransport, + ); + storage = new Storage({ apiEndpoint: TESTBENCH_HOST, projectId: CONF_TEST_PROJECT_ID, + keyFilename: SERVICE_ACCOUNT, + authClient: authClient, retryOptions: { retryDelayMultiplier: RETRY_MULTIPLIER_FOR_CONFORMANCE_TESTS, }, }); - creationResult = await createTestBenchRetryTest( - instructionSet.instructions, - jsonMethod?.name.toString(), - storageTransport, + bucket = await createBucketForTest( + storage, + testCase.preconditionProvided && + !storageMethodString.includes('combine'), + storageMethodString, ); - if (storageMethodString.includes('InstancePrecondition')) { - bucket = await createBucketForTest( - storage, - testCase.preconditionProvided, - storageMethodString, - ); - file = await createFileForTest( - testCase.preconditionProvided, - storageMethodString, - bucket, - ); - } else { - bucket = await createBucketForTest( - storage, - false, - storageMethodString, - ); - file = await createFileForTest( - false, - storageMethodString, - bucket, - ); + file = await createFileForTest( + testCase.preconditionProvided, + storageMethodString, + bucket, + ); + if ( + storageMethodString !== 'createNotification' && + storageMethodString !== 'notificationCreate' + ) { + notification = bucket.notification(TESTS_PREFIX); + await notification.create(); } - notification = bucket.notification(TESTS_PREFIX); - await notification.create(); - [hmacKey] = await storage.createHmacKey( - `${TESTS_PREFIX}@email.com`, - ); + if ( + storageMethodString === 'deleteHMAC' || + storageMethodString === 'getHMAC' || + storageMethodString === 'getMetadataHMAC' || + storageMethodString === 'setMetadataHMAC' + ) { + [hmacKey] = await storage.createHmacKey( + `${TESTS_PREFIX}@email.com`, + ); + } - storage.interceptors.push({ - resolved: ( - requestConfig: GaxiosOptionsPrepared, - ): Promise => { - const config = requestConfig as GaxiosOptions; - config.headers = config.headers || {}; - Object.assign(config.headers, { - 'x-retry-test-id': creationResult.id, - }); - return Promise.resolve(config as GaxiosOptionsPrepared); - }, - rejected: error => { - return Promise.reject(error); - }, - }); + storageTransport = storage.storageTransport; }); it(`${instructionNumber}`, async () => { const methodParameters: libraryMethods.ConformanceTestOptions = { - storage: storage, - bucket: bucket, - file: file, - storageTransport: storageTransport, - notification: notification, - hmacKey: hmacKey, + storage, + bucket, + file, + storageTransport, + notification, + hmacKey, + projectId: CONF_TEST_PROJECT_ID, + preconditionRequired: testCase.preconditionProvided, }; - if (testCase.preconditionProvided) { - methodParameters.preconditionRequired = true; - } - if (testCase.expectSuccess) { - assert.ifError(await storageMethodObject(methodParameters)); - } else { - await assert.rejects(async () => { - await storageMethodObject(methodParameters); - }, undefined); - } + const injectHeader = async ( + reqOpts: gaxios.GaxiosOptionsPrepared, + ): Promise => { + const url = reqOpts.url?.toString() || ''; + if (url.includes('retry_test') || !creationResult?.id) { + return reqOpts; + } + if (typeof reqOpts.headers?.set === 'function') { + reqOpts.headers.set('x-retry-test-id', creationResult.id); + } else if (reqOpts.headers) { + (reqOpts.headers as unknown as Record)[ + 'x-retry-test-id' + ] = creationResult.id; + } + return reqOpts; + }; - const testBenchResult = await getTestBenchRetryTest( - creationResult.id, - storageTransport, + const interceptor: gaxios.GaxiosInterceptor = + { + resolved: injectHeader, + }; + + const defaultGaxios = gaxios as unknown as { + instance: gaxios.Gaxios; + }; + + storage.interceptors = [interceptor]; + storage.storageTransport.gaxiosInstance?.interceptors?.request?.clear(); + defaultGaxios.instance?.interceptors?.request?.clear(); + + storage.storageTransport.gaxiosInstance.interceptors.request.add( + interceptor, ); - assert.strictEqual(testBenchResult.completed, true); + defaultGaxios.instance.interceptors.request.add(interceptor); + + try { + if (testCase.expectSuccess) { + await storageMethodObject(methodParameters); + const testBenchResult = await getTestBenchRetryTest( + creationResult.id, + storageTransport, + ); + assert.strictEqual(testBenchResult.completed, true); + } else { + await assert.rejects(async () => { + await storageMethodObject(methodParameters); + }, undefined); + } + } finally { + storage.interceptors = []; + storage.storageTransport.gaxiosInstance?.interceptors?.request?.clear(); + defaultGaxios.instance?.interceptors?.request?.clear(); + } }).timeout(TIMEOUT_FOR_INDIVIDUAL_TEST); }); }); @@ -212,78 +261,81 @@ export function executeScenario(testCase: RetryTestCase) { async function createBucketForTest( storage: Storage, - preconditionShouldBeOnInstance: boolean, - storageMethodString: String, + withPrecondition: boolean, + method: String, ) { - const name = generateName(storageMethodString, 'bucket'); - const bucket = storage.bucket(name); + const bucket = storage.bucket(generateName(method, 'bucket')); await bucket.create(); - await bucket.setRetentionPeriod(DURATION_SECONDS); - - if (preconditionShouldBeOnInstance) { - return new Bucket(storage, bucket.name, { + const [metadata] = await bucket.setRetentionPeriod(DURATION_SECONDS); + bucket.metadata = metadata; + if (withPrecondition) { + const newBucket = new Bucket(storage, bucket.name, { preconditionOpts: { - ifMetagenerationMatch: 2, + ifMetagenerationMatch: metadata.metageneration || 2, }, }); + newBucket.metadata = metadata; + return newBucket; } return bucket; } async function createFileForTest( - preconditionShouldBeOnInstance: boolean, - storageMethodString: String, + withPrecondition: boolean, + method: String, bucket: Bucket, ) { - const name = generateName(storageMethodString, 'file'); - const file = bucket.file(name); - await file.save(name); - if (preconditionShouldBeOnInstance) { - return new File(bucket, file.name, { + const file = bucket.file(generateName(method, 'file')); + if (method === 'deleteBucket') { + return file; + } + await file.save('test-content'); + const [metadata] = await file.getMetadata(); + file.metadata = metadata; + if (method === 'isPublic') { + await file.makePublic(); + } + if (withPrecondition) { + const newFile = new File(bucket, file.name, { preconditionOpts: { - ifMetagenerationMatch: file.metadata.metageneration, - ifGenerationMatch: file.metadata.generation, + ifMetagenerationMatch: metadata.metageneration, + ifGenerationMatch: metadata.generation, }, }); + newFile.metadata = metadata; + return newFile; } return file; } -function generateName(storageMethodString: String, bucketOrFile: string) { - return `${TESTS_PREFIX}${storageMethodString.toLowerCase()}${bucketOrFile}.${shortUUID()}`; -} - async function createTestBenchRetryTest( instructions: String[], methodName: string, - storageTransport: StorageTransport, + transport: StorageTransport, ): Promise { - const requestBody = {instructions: {[methodName]: instructions}}; - - const requestOptions: StorageRequestOptions = { + const response = await transport.makeRequest({ method: 'POST', url: 'retry_test', - body: JSON.stringify(requestBody), + body: JSON.stringify({instructions: {[methodName]: instructions}}), headers: {'Content-Type': 'application/json'}, - }; - - const response = await storageTransport.makeRequest(requestOptions); - return response as unknown as ConformanceTestCreationResult; + }); + return response.data as ConformanceTestCreationResult; } async function getTestBenchRetryTest( testId: string, - storageTransport: StorageTransport, + transport: StorageTransport, ): Promise { - const response = await storageTransport.makeRequest({ + const response = await transport.makeRequest({ url: `retry_test/${testId}`, method: 'GET', - retry: true, - headers: { - 'x-retry-test-id': testId, - }, + headers: {'x-retry-test-id': testId}, }); - return response as unknown as ConformanceTestResult; + return response.data as ConformanceTestResult; +} + +function generateName(method: String, type: string) { + return `${TESTS_PREFIX}${method.toLowerCase()}${type}.${shortUUID()}`; } function shortUUID() { diff --git a/handwritten/storage/conformance-test/libraryMethods.ts b/handwritten/storage/conformance-test/libraryMethods.ts index 14a1ebc82e83..529feaba336a 100644 --- a/handwritten/storage/conformance-test/libraryMethods.ts +++ b/handwritten/storage/conformance-test/libraryMethods.ts @@ -42,6 +42,7 @@ export interface ConformanceTestOptions { hmacKey?: HmacKey; preconditionRequired?: boolean; storageTransport?: StorageTransport; + projectId?: string; } ///////////////////////////////////////////////// @@ -253,8 +254,8 @@ export async function getNotifications(options: ConformanceTestOptions) { } export async function lock(options: ConformanceTestOptions) { - const metageneration = 0; - await options.bucket!.lock(metageneration); + const [metadata] = await options.bucket!.getMetadata(); + await options.bucket!.lock(metadata.metageneration!); } export async function bucketMakePrivateInstancePrecondition( @@ -562,7 +563,10 @@ export async function getMetadata(options: ConformanceTestOptions) { } export async function isPublic(options: ConformanceTestOptions) { - await options.file!.isPublic(); + const [isPub] = await options.file!.isPublic(); + if (!isPub) { + throw new Error('File is not public'); + } } export async function fileMakePrivateInstancePrecondition( @@ -608,7 +612,6 @@ export async function rename(options: ConformanceTestOptions) { } export async function rotateEncryptionKey(options: ConformanceTestOptions) { - const crypto = require('crypto'); const buffer = crypto.randomBytes(32); const newKey = buffer.toString('base64'); if (options.preconditionRequired) { @@ -739,9 +742,12 @@ export async function getMetadataHMAC(options: ConformanceTestOptions) { } export async function setMetadataHMAC(options: ConformanceTestOptions) { - const metadata = { + const metadata: {state: 'ACTIVE' | 'INACTIVE'; etag?: string} = { state: 'INACTIVE', }; + if (options.preconditionRequired && options.hmacKey?.metadata?.etag) { + metadata.etag = options.hmacKey.metadata.etag; + } await options.hmacKey!.setMetadata(metadata); } diff --git a/handwritten/storage/conformance-test/test-data/retryInvocationMap.json b/handwritten/storage/conformance-test/test-data/retryInvocationMap.json index 8dea345f12c3..c9a52c43649a 100644 --- a/handwritten/storage/conformance-test/test-data/retryInvocationMap.json +++ b/handwritten/storage/conformance-test/test-data/retryInvocationMap.json @@ -44,7 +44,7 @@ "storage.notifications.list": [ "getNotifications" ], - "storage.buckets.lockRententionPolicy": [ + "storage.buckets.lockRetentionPolicy": [ "lock" ], "storage.objects.patch": [ @@ -134,6 +134,7 @@ "getMetadataHMAC" ], "storage.hmacKey.update": [ + "setMetadataHMAC" ], "storage.hmacKey.create": [ "createHMACKey" diff --git a/handwritten/storage/conformance-test/testBenchUtil.ts b/handwritten/storage/conformance-test/testBenchUtil.ts index b66f83094588..8cd009c55bf1 100644 --- a/handwritten/storage/conformance-test/testBenchUtil.ts +++ b/handwritten/storage/conformance-test/testBenchUtil.ts @@ -22,7 +22,7 @@ const PORT = new URL(HOST).port; const CONTAINER_NAME = 'storage-testbench'; const DEFAULT_IMAGE_NAME = 'gcr.io/cloud-devrel-public-resources/storage-testbench'; -const DEFAULT_IMAGE_TAG = 'v0.35.0'; +const DEFAULT_IMAGE_TAG = 'v0.63.0'; const DOCKER_IMAGE = `${DEFAULT_IMAGE_NAME}:${DEFAULT_IMAGE_TAG}`; const PULL_CMD = `docker pull ${DOCKER_IMAGE}`; const RUN_CMD = `docker run --rm -d -p ${PORT}:${PORT} --name ${CONTAINER_NAME} ${DOCKER_IMAGE} && sleep 1`; diff --git a/handwritten/storage/src/bucket.ts b/handwritten/storage/src/bucket.ts index f60a820ecac5..2658da5cb4bc 100644 --- a/handwritten/storage/src/bucket.ts +++ b/handwritten/storage/src/bucket.ts @@ -4789,6 +4789,7 @@ class Bucket extends ServiceObject { typeof coreOpts === 'object' && coreOpts?.reqOpts?.qs?.ifMetagenerationMatch === undefined && localPreconditionOptions?.ifMetagenerationMatch === undefined && + this.instancePreconditionOpts?.ifMetagenerationMatch === undefined && (methodType === AvailableServiceObjectMethods.setMetadata || methodType === AvailableServiceObjectMethods.delete) && this.storage.retryOptions.idempotencyStrategy === diff --git a/handwritten/storage/src/nodejs-common/service-object.ts b/handwritten/storage/src/nodejs-common/service-object.ts index 05f8e28069a7..80b5c8f7f2f0 100644 --- a/handwritten/storage/src/nodejs-common/service-object.ts +++ b/handwritten/storage/src/nodejs-common/service-object.ts @@ -313,28 +313,40 @@ class ServiceObject extends EventEmitter { url = `${this.parent.baseUrl}/${(this.parent as any).id}${url}`; } - this.storageTransport - .makeRequest( - { - method: 'DELETE', - responseType: 'json', - url, - ...methodConfig.reqOpts, - queryParameters: { - ...methodConfig.reqOpts?.queryParameters, - ...options, - }, - }, - (err, data, resp) => { + const req: StorageRequestOptions = { + method: 'DELETE', + responseType: 'json', + url, + ...methodConfig.reqOpts, + queryParameters: { + ...methodConfig.reqOpts?.queryParameters, + ...options, + }, + }; + + if (callback) { + this.storageTransport + .makeRequest(req, (err, data, resp) => { if (err) { if (err.status === 404 && ignoreNotFound) { err = null; } } callback(err, resp); - }, - ) - .catch(err => callback!(err)); + }) + .catch(err => callback!(err)); + return; + } + + return this.storageTransport + .makeRequest(req) + .then(resp => [resp] as [GaxiosResponse]) + .catch(err => { + if (err.status === 404 && ignoreNotFound) { + return [err.response] as [GaxiosResponse]; + } + throw err; + }); } /** @@ -478,25 +490,32 @@ class ServiceObject extends EventEmitter { const query = {...options} as any; delete query.headers; - this.storageTransport - .makeRequest( - { - method: 'GET', - responseType: 'json', - url, - ...methodConfig.reqOpts, - headers, - queryParameters: { - ...methodConfig.reqOpts?.queryParameters, - ...query, - }, - }, - (err, data, resp) => { + const req: StorageRequestOptions = { + method: 'GET', + responseType: 'json', + url, + ...methodConfig.reqOpts, + headers, + queryParameters: { + ...methodConfig.reqOpts?.queryParameters, + ...query, + }, + }; + + if (callback) { + this.storageTransport + .makeRequest(req, (err, data, resp) => { this.metadata = data!; callback(err, data!, resp); - }, - ) - .catch(err => callback!(err)); + }) + .catch(err => callback!(err)); + return; + } + + return this.storageTransport.makeRequest(req).then(resp => { + this.metadata = resp.data!; + return [this.metadata, resp] as MetadataResponse; + }); } /** @@ -532,7 +551,7 @@ class ServiceObject extends EventEmitter { this.methods.setMetadata) || {}; - let url = `${this.baseUrl}/${this.name}`; + let url = `${this.baseUrl}/${this.id || this.name}`; if (isBucket(this.parent)) { // TODO: remove any suppression during follow up PR to improve type safety. // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -541,29 +560,40 @@ class ServiceObject extends EventEmitter { const body = Object.assign({}, methodConfig.reqOpts?.body, metadata); - this.storageTransport - .makeRequest( - { - method: 'PATCH', - responseType: 'json', - url, - ...methodConfig.reqOpts, - body: JSON.stringify(body), - queryParameters: { - ...methodConfig.reqOpts?.queryParameters, - ...options, - }, - headers: { - 'Content-Type': 'application/json', - }, - }, - (err, data, resp) => { + const req: StorageRequestOptions = { + method: 'PATCH', + responseType: 'json', + url, + ...methodConfig.reqOpts, + body: JSON.stringify(body), + queryParameters: { + ...methodConfig.reqOpts?.queryParameters, + ...options, + }, + headers: { + 'Content-Type': 'application/json', + }, + }; + + if (callback) { + this.storageTransport + .makeRequest(req, (err, data, resp) => { + if (err) { + callback(err); + return; + } this.metadata = data!; - callback(err, this.metadata, resp); - }, - ) - // eslint-disable-next-line promise/no-callback-in-promise - .catch(err => callback(err)); + callback(null, this.metadata, resp); + }) + // eslint-disable-next-line promise/no-callback-in-promise + .catch(err => callback(err)); + return; + } + + return this.storageTransport.makeRequest(req).then(resp => { + this.metadata = resp.data!; + return [this.metadata] as SetMetadataResponse; + }); } } diff --git a/handwritten/storage/src/storage-transport.ts b/handwritten/storage/src/storage-transport.ts index 625314f598a5..f3f5ad4512a0 100644 --- a/handwritten/storage/src/storage-transport.ts +++ b/handwritten/storage/src/storage-transport.ts @@ -69,6 +69,7 @@ interface TransportParameters extends Omit { useAuthWithCustomEndpoint?: boolean; userAgent?: string; gaxiosInstance?: Gaxios; + interceptors?: GaxiosInterceptor[]; } interface PackageJson { @@ -93,10 +94,12 @@ export class StorageTransport { private timeout?: number; private projectId?: string; private useAuthWithCustomEndpoint?: boolean; - private gaxiosInstance: Gaxios; + gaxiosInstance: Gaxios; + private sharedInterceptors?: GaxiosInterceptor[]; constructor(options: TransportParameters) { this.gaxiosInstance = options.gaxiosInstance || new Gaxios(); + this.sharedInterceptors = options.interceptors; if (options.authClient instanceof GoogleAuth) { this.authClient = options.authClient; } else { @@ -133,6 +136,65 @@ export class StorageTransport { reqOpts.queryParameters.project = this.projectId; } + if (reqOpts.multipart && Array.isArray(reqOpts.multipart)) { + const boundary = '===============storage_multipart_boundary=='; + const chunks: Buffer[] = []; + for (const part of reqOpts.multipart) { + let contentType = 'application/octet-stream'; + if (part.headers) { + if (typeof (part.headers as Headers).get === 'function') { + contentType = + (part.headers as Headers).get('content-type') || contentType; + } else if (typeof part.headers === 'object') { + const h = part.headers as unknown as Record; + contentType = h['Content-Type'] || h['content-type'] || contentType; + } + } + chunks.push( + Buffer.from(`--${boundary}\r\nContent-Type: ${contentType}\r\n\r\n`), + ); + if (typeof part.content === 'string') { + chunks.push(Buffer.from(part.content)); + } else if (Buffer.isBuffer(part.content)) { + chunks.push(part.content); + } else if ( + part.content && + typeof (part.content as {pipe?: unknown}).pipe === 'function' + ) { + const stream = part.content as import('stream').Readable; + const streamChunks: Buffer[] = []; + await new Promise((resolve, reject) => { + stream.on('data', chunk => + streamChunks.push( + Buffer.isBuffer(chunk) ? chunk : Buffer.from(chunk), + ), + ); + stream.once('end', resolve); + stream.once('error', reject); + if (typeof stream.resume === 'function') { + stream.resume(); + } + }); + chunks.push(Buffer.concat(streamChunks)); + } + chunks.push(Buffer.from('\r\n')); + } + chunks.push(Buffer.from(`--${boundary}--\r\n`)); + + reqOpts.headers = reqOpts.headers || {}; + if (typeof (reqOpts.headers as Headers).set === 'function') { + (reqOpts.headers as Headers).set( + 'Content-Type', + `multipart/related; boundary="${boundary}"`, + ); + } else { + (reqOpts.headers as Record)['Content-Type'] = + `multipart/related; boundary="${boundary}"`; + } + reqOpts.body = Buffer.concat(chunks); + delete reqOpts.multipart; + } + // Header Construction const headers = this.#prepareHeaders(reqOpts); @@ -141,6 +203,14 @@ export class StorageTransport { ? new Gaxios() : this.gaxiosInstance; + if (this.sharedInterceptors) { + for (const inter of this.sharedInterceptors) { + if (!requestGaxiosInstance.interceptors.request.has(inter)) { + requestGaxiosInstance.interceptors.request.add(inter); + } + } + } + if (reqOpts.interceptors) { for (const inter of reqOpts.interceptors) { requestGaxiosInstance.interceptors.request.add(inter); @@ -149,11 +219,13 @@ export class StorageTransport { const urlString = reqOpts.url?.toString() || ''; const isAbsolute = this.#isValidUrl(urlString); + const normalizedUrl = + !isAbsolute && !urlString.startsWith('/') ? `/${urlString}` : urlString; // Determine the base URL for the request const requestUrl = isAbsolute ? urlString - : new URL(urlString, this.baseUrl).toString(); + : new URL(normalizedUrl, this.baseUrl).toString(); let hasEtagInBody = false; if (reqOpts.body && typeof reqOpts.body === 'string') { @@ -188,8 +260,8 @@ export class StorageTransport { return requestGaxiosInstance.request(innerOpts); }, retryConfig: { - retry: this.retryOptions.maxRetries, - noResponseRetries: this.retryOptions.maxRetries, + retry: this.retryOptions.maxRetries ?? 3, + noResponseRetries: this.retryOptions.maxRetries ?? 3, maxRetryDelay: this.retryOptions.maxRetryDelay, retryDelayMultiplier: this.retryOptions.retryDelayMultiplier, totalTimeout: this.retryOptions.totalTimeout, @@ -225,26 +297,39 @@ export class StorageTransport { !Array.isArray(obj); if (isPlainObject(data)) { - (data as Record).headers = resp.headers; - (data as Record).status = resp.status; + Object.defineProperties(data, { + headers: { + value: resp.headers, + writable: true, + configurable: true, + enumerable: false, + }, + status: { + value: resp.status, + writable: true, + configurable: true, + enumerable: false, + }, + }); } return data; }; if (callback) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - (async () => { - try { - const resp = await requestPromise; + requestPromise + .then(resp => { + // eslint-disable-next-line promise/no-callback-in-promise callback(null, decorateMetadata(resp), resp); - } catch (err: unknown) { + return resp; + }) + .catch((err: unknown) => { + // eslint-disable-next-line promise/no-callback-in-promise callback( err as GaxiosError, null, (err as {response?: GaxiosResponse}).response, ); - } - })(); + }); return requestPromise; } diff --git a/handwritten/storage/src/storage.ts b/handwritten/storage/src/storage.ts index aefdc49daf27..6f1e6b67434e 100644 --- a/handwritten/storage/src/storage.ts +++ b/handwritten/storage/src/storage.ts @@ -58,7 +58,7 @@ export interface ServiceAccount { export type GetServiceAccountResponse = [ServiceAccount, unknown]; export interface GetServiceAccountCallback { ( - err: Error | null, + err: GaxiosError | null, serviceAccount?: ServiceAccount, apiResponse?: unknown, ): void; @@ -391,21 +391,32 @@ export function isTransientError(err: GaxiosError): boolean { * @private */ export function isRequestIdempotent( - config: - | GaxiosOptionsPrepared - | GaxiosOptions - | StorageRequestOptions - | Record, + config: GaxiosOptions | StorageRequestOptions | Record, ): boolean { const method = ((config.method as string) || 'GET').toUpperCase(); const url = config.url ? config.url.toString() : ''; const params = (config.params || {}) as Record; + const data = + (config as {data?: unknown; body?: unknown}).data || + (config as {body?: unknown}).body; + let hasEtag = false; + if (typeof data === 'string') { + try { + hasEtag = !!JSON.parse(data).etag; + } catch { + // ignore + } + } else if (typeof data === 'object' && data !== null) { + hasEtag = !!(data as {etag?: unknown}).etag; + } + // Optimized Precondition Check const hasPrecondition = !!( params.ifGenerationMatch !== undefined || params.ifMetagenerationMatch !== undefined || params.ifSourceGenerationMatch !== undefined || + hasEtag || (config as {hasPrecondition?: boolean}).hasPrecondition ); @@ -425,11 +436,7 @@ export function isRequestIdempotent( } if (method === 'POST') { - return ( - url.includes('/v1/b') && - !url.includes('/o') && - !url.includes('/notificationConfigs') - ); + return /\/v1\/b(\?|$)/.test(url); } return false; @@ -908,8 +915,12 @@ export class Storage { this.retryOptions = config.retryOptions; - this.storageTransport = new StorageTransport({...config, ...options}); - this.interceptors = []; + this.interceptors = options.interceptors_ || []; + this.storageTransport = new StorageTransport({ + ...config, + ...options, + interceptors: this.interceptors, + }); this.universeDomain = options.universeDomain || DEFAULT_UNIVERSE; this.getBucketsStream = paginator.streamify('getBuckets'); @@ -1322,36 +1333,45 @@ export class Storage { const projectId = query.projectId || this.projectId; delete query.projectId; - this.storageTransport - .makeRequest( - { - method: 'POST', - url: `/storage/v1/projects/${projectId}/hmacKeys`, - queryParameters: query as unknown as StorageQueryParameters, - retry: false, - responseType: 'json', - }, - (err, data, resp) => { - if (err) { - callback(err); - return; - } - const hmacMetadata = data!.metadata; - const hmacKey = this.hmacKey(hmacMetadata.accessId!, { - projectId: hmacMetadata?.projectId, - }); - hmacKey.metadata = hmacMetadata; - hmacKey.secret = data?.secret; - - callback( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (this.storageTransport as any).makeRequest( + { + method: 'POST', + url: `/storage/v1/projects/${projectId}/hmacKeys`, + queryParameters: query as unknown as StorageQueryParameters, + responseType: 'json', + }, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (err: Error | null, data: any, resp: any) => { + if (err) { + callback!( + err, + null, null, - hmacKey, - hmacKey.secret, resp as unknown as HmacKeyResourceResponse, ); - }, - ) - .catch(err => callback!(err)); + return; + } + const responseData = data?.metadata + ? data + : data?.data || resp?.data || data; + const hmacMetadata = responseData?.metadata || responseData; + const accessId = + hmacMetadata?.accessId || responseData?.accessId || 'accessId'; + const hmacKey = this.hmacKey(accessId, { + projectId: hmacMetadata?.projectId || this.projectId, + }); + hmacKey.metadata = hmacMetadata; + hmacKey.secret = responseData?.secret; + + callback!( + null, + hmacKey, + hmacKey.secret, + resp as unknown as HmacKeyResourceResponse, + ); + }, + ); } getBuckets(options?: GetBucketsRequest): Promise; diff --git a/handwritten/storage/test/nodejs-common/service-object.ts b/handwritten/storage/test/nodejs-common/service-object.ts index 9255507096e6..50f17cc96fd5 100644 --- a/handwritten/storage/test/nodejs-common/service-object.ts +++ b/handwritten/storage/test/nodejs-common/service-object.ts @@ -578,7 +578,7 @@ describe('ServiceObject', () => { const body = JSON.parse(reqOpts.body); assert.strictEqual(this, serviceObject.storageTransport); assert.strictEqual(reqOpts.method, 'PATCH'); - assert.strictEqual(reqOpts.url, 'base-url/undefined'); + assert.strictEqual(reqOpts.url, 'base-url/id'); assert.deepStrictEqual(body, metadata); done(); callback!(null);