Skip to content

core: only recurse relevant request objects #2100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stephenplusplus merged 1 commit into from
Mar 24, 2017
Merged

core: only recurse relevant request objects #2100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions packages/common-grpc/src/service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ GrpcService.prototype.request = function(protoOpts, reqOpts, callback) {
}

try {
reqOpts = util.decorateRequest(reqOpts, { projectId: this.projectId });
reqOpts = this.decorateRequest_(reqOpts);
} catch(e) {
callback(e);
return;
Expand Down Expand Up @@ -362,7 +362,7 @@ GrpcService.prototype.requestStream = function(protoOpts, reqOpts) {
}

try {
reqOpts = util.decorateRequest(reqOpts, { projectId: this.projectId });
reqOpts = this.decorateRequest_(reqOpts);
} catch(e) {
setImmediate(function() {
stream.destroy(e);
Expand Down Expand Up @@ -443,7 +443,7 @@ GrpcService.prototype.requestWritableStream = function(protoOpts, reqOpts) {
}

try {
reqOpts = util.decorateRequest(reqOpts, { projectId: this.projectId });
reqOpts = this.decorateRequest_(reqOpts);
} catch (e) {
setImmediate(function() {
stream.destroy(e);
Expand Down Expand Up @@ -687,6 +687,22 @@ GrpcService.structToObj_ = function(struct) {
return convertedObject;
};

/**
* Assign a projectId if one is specified to all request options.
*
* @param {object} reqOpts - The request options.
* @return {object} - The decorated request object.
*/
GrpcService.prototype.decorateRequest_ = function(reqOpts) {
reqOpts = extend({}, reqOpts);

delete reqOpts.autoPaginate;
delete reqOpts.autoPaginateVal;
delete reqOpts.objectMode;

return util.replaceProjectIdToken(reqOpts, this.projectId);
};

/**
* To authorize requests through gRPC, we must get the raw google-auth-library
* auth client object.
Expand All @@ -711,7 +727,7 @@ GrpcService.prototype.getGrpcCredentials_ = function(callback) {
);

if (!self.projectId || self.projectId === '{{projectId}}') {
self.projectId = authClient.projectId;
self.projectId = self.authClient.projectId;
}

callback(null, credentials);
Expand Down
57 changes: 45 additions & 12 deletions packages/common-grpc/test/service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ describe('GrpcService', function() {

describe('request', function() {
var PROTO_OPTS = { service: 'service', method: 'method', timeout: 3000 };
var REQ_OPTS = {};
var REQ_OPTS = { reqOpts: true };
var GRPC_CREDENTIALS = {};

function ProtoService() {}
Expand Down Expand Up @@ -747,9 +747,8 @@ describe('GrpcService', function() {
it('should decorate the request', function(done) {
var decoratedRequest = {};

fakeUtil.decorateRequest = function(reqOpts, config) {
assert.strictEqual(reqOpts, REQ_OPTS);
assert.deepEqual(config, { projectId: grpcService.projectId });
grpcService.decorateRequest_ = function(reqOpts) {
assert.deepEqual(reqOpts, REQ_OPTS);
return decoratedRequest;
};

Expand All @@ -770,7 +769,7 @@ describe('GrpcService', function() {
var error = new Error('Error.');

it('should return a thrown error to the callback', function(done) {
fakeUtil.decorateRequest = function() {
grpcService.decorateRequest_ = function() {
throw error;
};

Expand All @@ -787,7 +786,7 @@ describe('GrpcService', function() {
grpcService.getService_ = function() {
return {
method: function(reqOpts) {
assert.strictEqual(reqOpts, REQ_OPTS);
assert.deepEqual(reqOpts, REQ_OPTS);
done();
}
};
Expand Down Expand Up @@ -1063,9 +1062,8 @@ describe('GrpcService', function() {
it('should decorate the request', function(done) {
var decoratedRequest = {};

fakeUtil.decorateRequest = function(reqOpts, config) {
grpcService.decorateRequest_ = function(reqOpts) {
assert.strictEqual(reqOpts, REQ_OPTS);
assert.deepEqual(config, { projectId: grpcService.projectId });
return decoratedRequest;
};

Expand All @@ -1084,7 +1082,7 @@ describe('GrpcService', function() {
it('should end stream with a thrown error', function(done) {
var error = new Error('Error.');

fakeUtil.decorateRequest = function() {
grpcService.decorateRequest_ = function() {
throw error;
};

Expand Down Expand Up @@ -1340,9 +1338,8 @@ describe('GrpcService', function() {
it('should decorate the request', function(done) {
var decoratedRequest = {};

fakeUtil.decorateRequest = function(reqOpts, config) {
grpcService.decorateRequest_ = function(reqOpts) {
assert.strictEqual(reqOpts, REQ_OPTS);
assert.deepEqual(config, { projectId: grpcService.projectId });
return decoratedRequest;
};

Expand All @@ -1360,7 +1357,7 @@ describe('GrpcService', function() {
var error = new Error('Error.');

it('should end stream with a thrown error', function(done) {
fakeUtil.decorateRequest = function() {
grpcService.decorateRequest_ = function() {
throw error;
};

Expand Down Expand Up @@ -1660,6 +1657,41 @@ describe('GrpcService', function() {
});
});

describe('decorateRequest_', function() {
it('should delete custom API values without modifying object', function() {
var reqOpts = {
autoPaginate: true,
autoPaginateVal: true,
objectMode: true
};

var originalReqOpts = extend({}, reqOpts);

assert.deepEqual(grpcService.decorateRequest_(reqOpts), {});
assert.deepEqual(reqOpts, originalReqOpts);
});

it('should execute and return replaceProjectIdToken', function() {
var reqOpts = {
a: 'b',
c: 'd'
};

var replacedReqOpts = {};

fakeUtil.replaceProjectIdToken = function(reqOpts_, projectId) {
assert.deepEqual(reqOpts_, reqOpts);
assert.strictEqual(projectId, grpcService.projectId);
return replacedReqOpts;
};

assert.strictEqual(
grpcService.decorateRequest_(reqOpts),
replacedReqOpts
);
});
});

describe('getGrpcCredentials_', function() {
it('should get credentials from the auth client', function(done) {
grpcService.authClient = {
Expand Down Expand Up @@ -1698,6 +1730,7 @@ describe('GrpcService', function() {
beforeEach(function() {
grpcService.authClient = {
getAuthClient: function(callback) {
grpcService.authClient = AUTH_CLIENT;
callback(null, AUTH_CLIENT);
}
};
Expand Down
6 changes: 5 additions & 1 deletion packages/common/src/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,14 +487,18 @@ function decorateRequest(reqOpts, config) {
if (is.object(reqOpts.qs)) {
delete reqOpts.qs.autoPaginate;
delete reqOpts.qs.autoPaginateVal;
reqOpts.qs = util.replaceProjectIdToken(reqOpts.qs, config.projectId);
}

if (is.object(reqOpts.json)) {
delete reqOpts.json.autoPaginate;
delete reqOpts.json.autoPaginateVal;
reqOpts.json = util.replaceProjectIdToken(reqOpts.json, config.projectId);
}

return util.replaceProjectIdToken(reqOpts, config.projectId);
reqOpts.uri = util.replaceProjectIdToken(reqOpts.uri, config.projectId);

return reqOpts;
}

util.decorateRequest = decorateRequest;
Expand Down
58 changes: 51 additions & 7 deletions packages/common/test/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1276,22 +1276,66 @@ describe('common/util', function() {
assert.strictEqual(decoratedReqOpts.json.autoPaginateVal, undefined);
});

it('should replace project ID tokens for qs object', function() {
var config = {
projectId: 'project-id'
};
var reqOpts = {
uri: 'http://',
qs: {}
};
var decoratedQs = {};

utilOverrides.replaceProjectIdToken = function(qs, projectId) {
utilOverrides = {};
assert.strictEqual(qs, reqOpts.qs);
assert.strictEqual(projectId, config.projectId);
return decoratedQs;
};

var decoratedRequest = util.decorateRequest(reqOpts, config);
assert.strictEqual(decoratedRequest.qs, decoratedQs);
});

it('should replace project ID tokens for json object', function() {
var config = {
projectId: 'project-id'
};
var reqOpts = {
uri: 'http://',
json: {}
};
var decoratedJson = {};

utilOverrides.replaceProjectIdToken = function(json, projectId) {
utilOverrides = {};
assert.strictEqual(reqOpts.json, json);
assert.strictEqual(projectId, config.projectId);
return decoratedJson;
};

var decoratedRequest = util.decorateRequest(reqOpts, config);
assert.strictEqual(decoratedRequest.json, decoratedJson);
});

it('should decorate the request', function() {
var config = {
projectId: 'project-id'
};
var reqOpts = {};
var decoratedReqOpts = {};
var reqOpts = {
uri: 'http://'
};
var decoratedUri = 'http://decorated';

utilOverrides.replaceProjectIdToken = function(reqOpts_, projectId) {
assert.strictEqual(reqOpts_, reqOpts);
utilOverrides.replaceProjectIdToken = function(uri, projectId) {
assert.strictEqual(uri, reqOpts.uri);
assert.strictEqual(projectId, config.projectId);
return decoratedReqOpts;
return decoratedUri;
};

assert.strictEqual(
assert.deepEqual(
util.decorateRequest(reqOpts, config),
decoratedReqOpts
{ uri: decoratedUri }
);
});
});
Expand Down
3 changes: 2 additions & 1 deletion packages/compute/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@
"concat-stream": "^1.5.0",
"mocha": "^3.0.1",
"propprop": "^0.3.0",
"proxyquire": "^1.7.10"
"proxyquire": "^1.7.10",
"uuid": "^3.0.1"
},
"scripts": {
"publish-module": "node ../../scripts/publish.js compute",
Expand Down
7 changes: 6 additions & 1 deletion packages/compute/src/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,11 @@ VM.prototype.detachDisk = function(disk, callback) {
return;
}

var diskName = common.util.replaceProjectIdToken(
disk.formattedName,
self.zone.compute.authClient.projectId
);

var deviceName;
var baseUrl = 'https://www.googleapis.com/compute/v1/';
var disks = metadata.disks || [];
Expand All @@ -352,7 +357,7 @@ VM.prototype.detachDisk = function(disk, callback) {
var attachedDisk = disks[i];
var source = attachedDisk.source.replace(baseUrl, '');

if (source === disk.formattedName) {
if (source === diskName) {
deviceName = attachedDisk.deviceName;
}
}
Expand Down
42 changes: 40 additions & 2 deletions packages/compute/test/vm.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ var proxyquire = require('proxyquire');
var ServiceObject = require('@google-cloud/common').ServiceObject;
var util = require('@google-cloud/common').util;

var utilCached = extend({}, util);

var promisified = false;
var fakeUtil = extend({}, util, {
promisifyAll: function(Class) {
Expand All @@ -46,7 +48,12 @@ describe('VM', function() {
var Disk;
var DISK;

var COMPUTE = { projectId: 'project-id' };
var COMPUTE = {
authClient: {
projectId: 'project-id'
},
projectId: 'project-id'
};
var ZONE = {
compute: COMPUTE,
name: 'us-central1-a',
Expand All @@ -70,6 +77,7 @@ describe('VM', function() {
});

beforeEach(function() {
extend(fakeUtil, utilCached);
vm = new VM(ZONE, VM_NAME);
DISK = new Disk(ZONE, 'disk-name');
});
Expand Down Expand Up @@ -229,7 +237,7 @@ describe('VM', function() {
beforeEach(function() {
DEVICE_NAME = DISK.formattedName;

METADATA = METADATA = {
METADATA = {
disks: [
{
source: DEVICE_NAME,
Expand All @@ -254,6 +262,36 @@ describe('VM', function() {
});
});

it('should replace projectId token in disk name', function(done) {
var REPLACED_DEVICE_NAME = 'replaced-device-name';

fakeUtil.replaceProjectIdToken = function(value, projectId) {
assert.strictEqual(value, DISK.formattedName);
assert.strictEqual(projectId, COMPUTE.authClient.projectId);
return REPLACED_DEVICE_NAME;
};

vm.getMetadata = function(callback) {
var metadata = {
disks: [
{
source: REPLACED_DEVICE_NAME,
deviceName: REPLACED_DEVICE_NAME
}
]
};

callback(null, metadata, metadata);
};

vm.request = function(reqOpts) {
assert.strictEqual(reqOpts.qs.deviceName, REPLACED_DEVICE_NAME);
done();
};

vm.detachDisk(DISK, assert.ifError);
});

it('should return an error if device name not found', function(done) {
var metadata = {
disks: [
Expand Down