Skip to content

Commit 590e73b

Browse files
stephenplusplusstephenplusplus
authored
fix: favor user options over defaults (#353)
1 parent f06cc4e commit 590e73b

2 files changed

Lines changed: 70 additions & 62 deletions

File tree

‎core/common/src/service-object.ts‎

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -265,13 +265,11 @@ class ServiceObject<T = any> extends EventEmitter {
265265
const methodConfig =
266266
(typeof this.methods.delete === 'object' && this.methods.delete) || {};
267267

268-
const reqOpts = extend(
269-
{
270-
method: 'DELETE',
271-
uri: '',
272-
qs: options,
273-
},
274-
methodConfig.reqOpts);
268+
const reqOpts = extend(true, methodConfig.reqOpts, {
269+
method: 'DELETE',
270+
uri: '',
271+
qs: options,
272+
});
275273

276274
// The `request` method may have been overridden to hold any special
277275
// behavior. Ensure we call the original `request` method.
@@ -384,12 +382,10 @@ class ServiceObject<T = any> extends EventEmitter {
384382
const methodConfig = (typeof this.methods.getMetadata === 'object' &&
385383
this.methods.getMetadata) ||
386384
{};
387-
const reqOpts = extend(
388-
{
389-
uri: '',
390-
qs: options,
391-
},
392-
methodConfig.reqOpts);
385+
const reqOpts = extend(true, methodConfig.reqOpts, {
386+
uri: '',
387+
qs: options,
388+
});
393389

394390
// The `request` method may have been overridden to hold any special
395391
// behavior. Ensure we call the original `request` method.
@@ -427,14 +423,12 @@ class ServiceObject<T = any> extends EventEmitter {
427423
this.methods.setMetadata) ||
428424
{};
429425

430-
const reqOpts = extend(
431-
true, {
432-
method: 'PATCH',
433-
uri: '',
434-
json: metadata,
435-
qs: options,
436-
},
437-
methodConfig.reqOpts);
426+
const reqOpts = extend(true, methodConfig.reqOpts, {
427+
method: 'PATCH',
428+
uri: '',
429+
json: metadata,
430+
qs: options,
431+
});
438432

439433
// The `request` method may have been overridden to hold any special
440434
// behavior. Ensure we call the original `request` method.

‎core/common/test/service-object.ts‎

Lines changed: 55 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -249,37 +249,43 @@ describe('ServiceObject', () => {
249249
});
250250

251251
it('should accept options', (done) => {
252-
const options = {};
252+
const options = {queryOptionProperty: true};
253253
sandbox.stub(ServiceObject.prototype, 'request')
254254
.callsFake((reqOpts, callback) => {
255-
assert.strictEqual(reqOpts.qs, options);
255+
assert.deepStrictEqual(reqOpts.qs, options);
256256
done();
257257
callback(null, null, {} as r.Response);
258258
});
259259
serviceObject.delete(options, assert.ifError);
260260
});
261261

262-
it('should extend the request options with defaults', (done) => {
263-
const method = {
262+
it('should extend the defaults with request options', (done) => {
263+
const methodConfig = {
264264
reqOpts: {
265-
method: 'override',
266265
qs: {
267-
custom: true,
266+
defaultProperty: true,
267+
thisPropertyWasOverridden: false,
268268
},
269269
},
270270
};
271271

272272
sandbox.stub(ServiceObject.prototype, 'request')
273273
.callsFake((reqOpts_, callback) => {
274-
assert.strictEqual(reqOpts_.method, method.reqOpts.method);
275-
assert.deepStrictEqual(reqOpts_.qs, method.reqOpts.qs);
274+
assert.deepStrictEqual(reqOpts_.qs, {
275+
defaultProperty: true,
276+
optionalProperty: true,
277+
thisPropertyWasOverridden: true,
278+
});
276279
done();
277280
callback(null, null, null!);
278281
});
279282

280283
const serviceObject = new ServiceObject(CONFIG) as FakeServiceObject;
281-
serviceObject.methods.delete = method;
282-
serviceObject.delete();
284+
serviceObject.methods.delete = methodConfig;
285+
serviceObject.delete({
286+
optionalProperty: true,
287+
thisPropertyWasOverridden: true,
288+
});
283289
});
284290

285291
it('should not require a callback', () => {
@@ -309,10 +315,10 @@ describe('ServiceObject', () => {
309315
});
310316

311317
it('should accept options', (done) => {
312-
const options = {};
318+
const options = {queryOptionProperty: true};
313319
sandbox.stub(ServiceObject.prototype, 'get')
314320
.callsFake((options_, callback) => {
315-
assert.strictEqual(options_, options);
321+
assert.deepStrictEqual(options_, options);
316322
done();
317323
callback(null, null, {} as r.Response);
318324
});
@@ -514,37 +520,43 @@ describe('ServiceObject', () => {
514520
});
515521

516522
it('should accept options', (done) => {
517-
const options = {};
523+
const options = {queryOptionProperty: true};
518524
sandbox.stub(ServiceObject.prototype, 'request')
519525
.callsFake(function(this: ServiceObject, reqOpts, callback) {
520-
assert.strictEqual(reqOpts.qs, options);
526+
assert.deepStrictEqual(reqOpts.qs, options);
521527
done();
522528
callback(null, null, {} as r.Response);
523529
});
524530
serviceObject.getMetadata(options, assert.ifError);
525531
});
526532

527-
it('should extend the request options with defaults', (done) => {
528-
const method = {
533+
it('should extend the defaults with request options', (done) => {
534+
const methodConfig = {
529535
reqOpts: {
530-
method: 'override',
531536
qs: {
532-
custom: true,
537+
defaultProperty: true,
538+
thisPropertyWasOverridden: false,
533539
},
534540
},
535541
};
536542

537543
sandbox.stub(ServiceObject.prototype, 'request')
538544
.callsFake((reqOpts_, callback) => {
539-
assert.strictEqual(reqOpts_.method, method.reqOpts.method);
540-
assert.deepStrictEqual(reqOpts_.qs, method.reqOpts.qs);
545+
assert.deepStrictEqual(reqOpts_.qs, {
546+
defaultProperty: true,
547+
optionalProperty: true,
548+
thisPropertyWasOverridden: true,
549+
});
541550
done();
542-
callback(null, undefined, {} as r.Response);
551+
callback(null, null, null!);
543552
});
544553

545554
const serviceObject = new ServiceObject(CONFIG) as FakeServiceObject;
546-
serviceObject.methods.getMetadata = method;
547-
serviceObject.getMetadata(() => {});
555+
serviceObject.methods.getMetadata = methodConfig;
556+
serviceObject.getMetadata({
557+
optionalProperty: true,
558+
thisPropertyWasOverridden: true,
559+
});
548560
});
549561

550562
it('should execute callback with error & apiResponse', (done) => {
@@ -583,13 +595,13 @@ describe('ServiceObject', () => {
583595

584596
describe('setMetadata', () => {
585597
it('should make the correct request', (done) => {
586-
const metadata = {};
598+
const metadata = {metadataProperty: true};
587599
sandbox.stub(ServiceObject.prototype, 'request')
588600
.callsFake(function(this: ServiceObject, reqOpts, callback) {
589601
assert.strictEqual(this, serviceObject);
590602
assert.strictEqual(reqOpts.method, 'PATCH');
591603
assert.strictEqual(reqOpts.uri, '');
592-
assert.strictEqual(reqOpts.json, metadata);
604+
assert.deepStrictEqual(reqOpts.json, metadata);
593605
done();
594606
callback(null, null, {} as r.Response);
595607
});
@@ -598,41 +610,43 @@ describe('ServiceObject', () => {
598610

599611
it('should accept options', (done) => {
600612
const metadata = {};
601-
const options = {};
613+
const options = {queryOptionProperty: true};
602614
sandbox.stub(ServiceObject.prototype, 'request')
603615
.callsFake(function(this: ServiceObject, reqOpts, callback) {
604-
assert.strictEqual(reqOpts.qs, options);
616+
assert.deepStrictEqual(reqOpts.qs, options);
605617
done();
606618
callback(null, null, {} as r.Response);
607619
});
608620
serviceObject.setMetadata(metadata, options, () => {});
609621
});
610622

611-
it('should extend the request options with defaults', (done) => {
612-
const metadataDefault = {a: 'b'};
613-
const metadata = {c: 'd'};
614-
const method = {
623+
it('should extend the defaults with request options', (done) => {
624+
const methodConfig = {
615625
reqOpts: {
616-
method: 'override',
617626
qs: {
618-
custom: true,
627+
defaultProperty: true,
628+
thisPropertyWasOverridden: false,
619629
},
620-
json: metadataDefault,
621630
},
622631
};
623632

624-
const expectedJson = extend(true, {}, metadataDefault, metadata);
625-
const serviceObject = new ServiceObject(CONFIG);
626-
asInternal(serviceObject).methods.setMetadata = method;
627633
sandbox.stub(ServiceObject.prototype, 'request')
628634
.callsFake((reqOpts_, callback) => {
629-
assert.deepStrictEqual(reqOpts_.method, method.reqOpts.method);
630-
assert.deepStrictEqual(reqOpts_.qs, method.reqOpts.qs);
631-
assert.deepStrictEqual(reqOpts_.json, expectedJson);
635+
assert.deepStrictEqual(reqOpts_.qs, {
636+
defaultProperty: true,
637+
optionalProperty: true,
638+
thisPropertyWasOverridden: true,
639+
});
632640
done();
633641
callback(null, null, null!);
634642
});
635-
serviceObject.setMetadata(metadata);
643+
644+
const serviceObject = new ServiceObject(CONFIG) as FakeServiceObject;
645+
serviceObject.methods.setMetadata = methodConfig;
646+
serviceObject.setMetadata({}, {
647+
optionalProperty: true,
648+
thisPropertyWasOverridden: true,
649+
});
636650
});
637651

638652
it('should execute callback with error & apiResponse', (done) => {

0 commit comments

Comments
 (0)