From 5ffa6127b1a0c1ff2fffd1df2331e850afdc9dcd Mon Sep 17 00:00:00 2001 From: Matthew Gabeler-Lee Date: Tue, 20 Sep 2022 11:10:59 -0400 Subject: [PATCH 1/3] fix: correct STORAGE_EMULATOR_HOST handling (#2069, #1314) credit to @jpambrun for identifying the fix --- src/storage.ts | 2 +- test/index.ts | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index a40d1b1bb..cb47c9521 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -666,7 +666,7 @@ export class Storage extends Service { options = Object.assign({}, options, {apiEndpoint}); // Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead. - const baseUrl = EMULATOR_HOST || `${options.apiEndpoint}/storage/v1`; + const baseUrl = (EMULATOR_HOST || options.apiEndpoint) + '/storage/v1'; const config = { apiEndpoint: options.apiEndpoint!, diff --git a/test/index.ts b/test/index.ts index 56d08e3db..85b8b64b6 100644 --- a/test/index.ts +++ b/test/index.ts @@ -437,13 +437,13 @@ describe('Storage', () => { delete process.env.STORAGE_EMULATOR_HOST; }); - it('should set baseUrl to env var STORAGE_EMULATOR_HOST', () => { + it('should set baseUrl to env var STORAGE_EMULATOR_HOST plus standard path', () => { const storage = new Storage({ projectId: PROJECT_ID, }); const calledWith = storage.calledWith_[0]; - assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST); + assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1'); assert.strictEqual( calledWith.apiEndpoint, 'https://internal.benchmark.com/path' @@ -457,7 +457,7 @@ describe('Storage', () => { }); const calledWith = storage.calledWith_[0]; - assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST); + assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1'); assert.strictEqual(calledWith.apiEndpoint, 'https://some.api.com'); }); @@ -470,7 +470,7 @@ describe('Storage', () => { }); const calledWith = storage.calledWith_[0]; - assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST); + assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1'); assert.strictEqual( calledWith.apiEndpoint, 'https://internal.benchmark.com/path' From dcd583c9d36c20622ff11d6895e418cad3746ffd Mon Sep 17 00:00:00 2001 From: Matthew Gabeler-Lee Date: Wed, 5 Oct 2022 14:27:08 -0400 Subject: [PATCH 2/3] fix: normalize baseUrl Co-authored-by: Daniel Bankhead --- src/storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage.ts b/src/storage.ts index cb47c9521..851282227 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -666,7 +666,7 @@ export class Storage extends Service { options = Object.assign({}, options, {apiEndpoint}); // Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead. - const baseUrl = (EMULATOR_HOST || options.apiEndpoint) + '/storage/v1'; + const baseUrl = new URL('/storage/v1', EMULATOR_HOST || options.apiEndpoint).href; const config = { apiEndpoint: options.apiEndpoint!, From 04940e00fe0a5540bb3381569c0e84f394638516 Mon Sep 17 00:00:00 2001 From: Matthew Gabeler-Lee Date: Wed, 5 Oct 2022 14:49:29 -0400 Subject: [PATCH 3/3] fix: adjust URL normalization & tests for consistency --- src/storage.ts | 8 ++++++-- test/index.ts | 18 +++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/storage.ts b/src/storage.ts index 851282227..75c6b9b8e 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -665,8 +665,12 @@ export class Storage extends Service { options = Object.assign({}, options, {apiEndpoint}); - // Note: EMULATOR_HOST is an experimental configuration variable. Use apiEndpoint instead. - const baseUrl = new URL('/storage/v1', EMULATOR_HOST || options.apiEndpoint).href; + // Note: EMULATOR_HOST, if present and not overridden, has been applied to + // `options` at this point. Also, this uses string concatenation because the + // endpoint may contain a base path, and any trailing slash on that will + // have been removed, so using the two-arg URL constructor for relative path + // resolution won't work. + const baseUrl = new URL(options.apiEndpoint + '/storage/v1').href; const config = { apiEndpoint: options.apiEndpoint!, diff --git a/test/index.ts b/test/index.ts index 85b8b64b6..08befeefc 100644 --- a/test/index.ts +++ b/test/index.ts @@ -450,14 +450,20 @@ describe('Storage', () => { ); }); - it('should be overriden by apiEndpoint', () => { + it('should be overridden by apiEndpoint', () => { const storage = new Storage({ projectId: PROJECT_ID, apiEndpoint: 'https://some.api.com', }); const calledWith = storage.calledWith_[0]; - assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1'); + // NOTE: this used to assert partially the opposite of what the test + // says: it checked that baseUrl was _not_ overridden, but apiEndpoint + // was. + assert.strictEqual( + calledWith.baseUrl, + 'https://some.api.com/storage/v1' + ); assert.strictEqual(calledWith.apiEndpoint, 'https://some.api.com'); }); @@ -470,7 +476,13 @@ describe('Storage', () => { }); const calledWith = storage.calledWith_[0]; - assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST + '/storage/v1'); + // NOTE: this used to assert partially the opposite of what the test + // says: it checked that baseUrl was _not_ overridden, but apiEndpoint + // was. + assert.strictEqual( + calledWith.baseUrl, + 'https://internal.benchmark.com/path/storage/v1' + ); assert.strictEqual( calledWith.apiEndpoint, 'https://internal.benchmark.com/path'