Skip to content

Commit

Permalink
fix: StorageOptions.apiEndpoint overrides simple and resumable uploads (
Browse files Browse the repository at this point in the history 
#1161)

* fix: StorageOptions.apiEndpoint overrides Upload URL

* unit tests

* sanitize apiEndpoint

* assert apiEndpoint passed to resumableupload

* fix it

* apiEndpoint overrides STORAGE_EMULATOR_HOST

* test Storage::sanitizeEndpoint

* npm run fix

* pass apiEndpoint to startResumableUpload_

* fix lint

Co-authored-by: Benjamin E. Coe <bencoe@google.com>
Co-authored-by: Frank Natividad <frankyn@users.noreply.github.com>
  • Loading branch information
@jkwlui @bcoe @frankyn
3 people authored May 14, 2020
1 parent d680e9f commit cf8ea5c
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 21 deletions.
15 changes: 7 additions & 8 deletions src/file.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,13 +276,6 @@ class ResumableUploadError extends Error {
name = 'ResumableUploadError';
}

/**
* @const {string}
* @private
*/
const STORAGE_UPLOAD_BASE_URL =
'https://storage.googleapis.com/upload/storage/v1/b';

/**
* @const {string}
* @private
Expand Down Expand Up @@ -1514,6 +1507,7 @@ class File extends ServiceObject<File> {
resumableUpload.createURI(
{
authClient: this.storage.authClient,
apiEndpoint: this.storage.apiEndpoint,
bucket: this.bucket.name,
configPath: options.configPath,
file: this.name,
Expand Down Expand Up @@ -3529,6 +3523,7 @@ class File extends ServiceObject<File> {

const uploadStream = resumableUpload.upload({
authClient: this.storage.authClient,
apiEndpoint: this.storage.apiEndpoint,
bucket: this.bucket.name,
configPath: options.configPath,
file: this.name,
Expand Down Expand Up @@ -3580,11 +3575,15 @@ class File extends ServiceObject<File> {
options
);

const apiEndpoint = this.storage.apiEndpoint;
const bucketName = this.bucket.name;
const uri = `${apiEndpoint}/upload/storage/v1/b/${bucketName}/o`;

const reqOpts: DecorateRequestOptions = {
qs: {
name: this.name,
},
uri: `${STORAGE_UPLOAD_BASE_URL}/${this.bucket.name}/o`,
uri: uri,
};

if (this.generation !== undefined) {
Expand Down
32 changes: 25 additions & 7 deletions src/storage.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ export interface GetHmacKeysCallback {

export type GetHmacKeysResponse = [HmacKey[]];

export const PROTOCOL_REGEX = /^(\w*):\/\//;

/*! Developer Documentation
*
* Invoke this method to create a new Storage object bound with pre-determined
Expand Down Expand Up @@ -385,15 +387,24 @@ export class Storage extends Service {
* @param {StorageOptions} [options] Configuration options.
*/
constructor(options: StorageOptions = {}) {
options = Object.assign({}, options, {
apiEndpoint: options.apiEndpoint || 'storage.googleapis.com',
});
const url =
process.env.STORAGE_EMULATOR_HOST ||
`https://${options.apiEndpoint}/storage/v1`;
let apiEndpoint = 'https://storage.googleapis.com';

const EMULATOR_HOST = process.env.STORAGE_EMULATOR_HOST;
if (typeof EMULATOR_HOST === 'string') {
apiEndpoint = Storage.sanitizeEndpoint(EMULATOR_HOST);
}

if (options.apiEndpoint) {
apiEndpoint = Storage.sanitizeEndpoint(options.apiEndpoint);
}

options = Object.assign({}, options, {apiEndpoint});

const baseUrl = EMULATOR_HOST || `${options.apiEndpoint}/storage/v1`;

const config = {
apiEndpoint: options.apiEndpoint!,
baseUrl: url,
baseUrl,
projectIdRequired: false,
scopes: [
'https://www.googleapis.com/auth/iam',
Expand All @@ -417,6 +428,13 @@ export class Storage extends Service {
this.getHmacKeysStream = paginator.streamify('getHmacKeys');
}

private static sanitizeEndpoint(url: string) {
if (!PROTOCOL_REGEX.test(url)) {
url = `https://${url}`;
}
return url.replace(/\/+$/, ''); // Remove trailing slashes
}

/**
* Get a reference to a Cloud Storage bucket.
*
Expand Down
3 changes: 3 additions & 0 deletions test/file.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ describe('File', () => {
STORAGE = {
createBucket: util.noop,
request: util.noop,
apiEndpoint: 'https://storage.googleapis.com',
// eslint-disable-next-line @typescript-eslint/no-explicit-any
makeAuthenticatedRequest(req: {}, callback: any) {
if (callback) {
Expand Down Expand Up @@ -1610,6 +1611,7 @@ describe('File', () => {
const storage = bucket.storage;

assert.strictEqual(opts.authClient, storage.authClient);
assert.strictEqual(opts.apiEndpoint, storage.apiEndpoint);
assert.strictEqual(opts.bucket, bucket.name);
assert.strictEqual(opts.configPath, options.configPath);
assert.strictEqual(opts.file, file.name);
Expand Down Expand Up @@ -4008,6 +4010,7 @@ describe('File', () => {
const authClient = storage.makeAuthenticatedRequest.authClient;

assert.strictEqual(opts.authClient, authClient);
assert.strictEqual(opts.apiEndpoint, storage.apiEndpoint);
assert.strictEqual(opts.bucket, bucket.name);
assert.strictEqual(opts.configPath, options.configPath);
assert.strictEqual(opts.file, file.name);
Expand Down
120 changes: 114 additions & 6 deletions test/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,14 @@ import {
import {PromisifyAllOptions} from '@google-cloud/promisify';
import arrify = require('arrify');
import * as assert from 'assert';
import {describe, it, before, beforeEach, afterEach} from 'mocha';
import {describe, it, before, beforeEach, after, afterEach} from 'mocha';
import * as proxyquire from 'proxyquire';
// eslint-disable-next-line @typescript-eslint/no-unused-vars
import {Bucket} from '../src';
import {GetFilesOptions} from '../src/bucket';
import sinon = require('sinon');
import {HmacKey} from '../src/hmacKey';
import {HmacKeyResourceResponse} from '../src/storage';
import {HmacKeyResourceResponse, PROTOCOL_REGEX} from '../src/storage';

// eslint-disable-next-line @typescript-eslint/no-var-requires
const hmacKeyModule = require('../src/hmacKey');
Expand Down Expand Up @@ -147,7 +147,7 @@ describe('Storage', () => {
projectId: PROJECT_ID,
};
const expectedCalledWith = Object.assign({}, options, {
apiEndpoint: 'storage.googleapis.com',
apiEndpoint: 'https://storage.googleapis.com',
});
storage = new Storage(options);
const calledWith = storage.calledWith_[1];
Expand All @@ -157,17 +157,93 @@ describe('Storage', () => {
});

it('should propagate the apiEndpoint option', () => {
const apiEndpoint = 'some.fake.endpoint';
const apiEndpoint = 'https://some.fake.endpoint';
storage = new Storage({
projectId: PROJECT_ID,
apiEndpoint,
});
const calledWith = storage.calledWith_[0];
assert.strictEqual(calledWith.baseUrl, `${apiEndpoint}/storage/v1`);
assert.strictEqual(calledWith.apiEndpoint, `${apiEndpoint}`);
});

it('should prepend apiEndpoint with default protocol', () => {
const protocollessApiEndpoint = 'some.fake.endpoint';
storage = new Storage({
projectId: PROJECT_ID,
apiEndpoint: protocollessApiEndpoint,
});
const calledWith = storage.calledWith_[0];
assert.strictEqual(
calledWith.baseUrl,
`https://${apiEndpoint}/storage/v1`
`https://${protocollessApiEndpoint}/storage/v1`
);
assert.strictEqual(
calledWith.apiEndpoint,
`https://${protocollessApiEndpoint}`
);
assert.strictEqual(calledWith.apiEndpoint, apiEndpoint);
});

it('should strip trailing slash from apiEndpoint', () => {
const apiEndpoint = 'https://some.fake.endpoint/';
storage = new Storage({
projectId: PROJECT_ID,
apiEndpoint,
});
const calledWith = storage.calledWith_[0];
assert.strictEqual(calledWith.baseUrl, `${apiEndpoint}storage/v1`);
assert.strictEqual(calledWith.apiEndpoint, 'https://some.fake.endpoint');
});

describe('STORAGE_EMULATOR_HOST', () => {
const EMULATOR_HOST = 'https://internal.benchmark.com/path';
before(() => {
process.env.STORAGE_EMULATOR_HOST = EMULATOR_HOST;
});

after(() => {
delete process.env.STORAGE_EMULATOR_HOST;
});

it('should set baseUrl to env var STORAGE_EMULATOR_HOST', () => {
storage = new Storage({
projectId: PROJECT_ID,
});

const calledWith = storage.calledWith_[0];
assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
assert.strictEqual(
calledWith.apiEndpoint,
'https://internal.benchmark.com/path'
);
});

it('should be overriden by apiEndpoint', () => {
storage = new Storage({
projectId: PROJECT_ID,
apiEndpoint: 'https://some.api.com',
});

const calledWith = storage.calledWith_[0];
assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
assert.strictEqual(calledWith.apiEndpoint, 'https://some.api.com');
});

it('should prepend default protocol and strip trailing slash', () => {
const EMULATOR_HOST = 'internal.benchmark.com/path/';
process.env.STORAGE_EMULATOR_HOST = EMULATOR_HOST;

storage = new Storage({
projectId: PROJECT_ID,
});

const calledWith = storage.calledWith_[0];
assert.strictEqual(calledWith.baseUrl, EMULATOR_HOST);
assert.strictEqual(
calledWith.apiEndpoint,
'https://internal.benchmark.com/path'
);
});
});
});

Expand Down Expand Up @@ -979,4 +1055,36 @@ describe('Storage', () => {
});
});
});

describe('#sanitizeEndpoint', () => {
const USER_DEFINED_SHORT_API_ENDPOINT = 'myapi.com:8080';
const USER_DEFINED_PROTOCOL = 'myproto';
const USER_DEFINED_FULL_API_ENDPOINT = `${USER_DEFINED_PROTOCOL}://myapi.com:8080`;

it('should default protocol to https', () => {
const endpoint = Storage.sanitizeEndpoint(
USER_DEFINED_SHORT_API_ENDPOINT
);
assert.strictEqual(endpoint.match(PROTOCOL_REGEX)![1], 'https');
});

it('should not override protocol', () => {
const endpoint = Storage.sanitizeEndpoint(USER_DEFINED_FULL_API_ENDPOINT);
assert.strictEqual(
endpoint.match(PROTOCOL_REGEX)![1],
USER_DEFINED_PROTOCOL
);
});

it('should remove trailing slashes from URL', () => {
const endpointsWithTrailingSlashes = [
`${USER_DEFINED_FULL_API_ENDPOINT}/`,
`${USER_DEFINED_FULL_API_ENDPOINT}//`,
];
for (const endpointWithTrailingSlashes of endpointsWithTrailingSlashes) {
const endpoint = Storage.sanitizeEndpoint(endpointWithTrailingSlashes);
assert.strictEqual(endpoint.endsWith('/'), false);
}
});
});
});

0 comments on commit cf8ea5c

Please sign in to comment.