feat: add resource header prefix #1196

AVaksman · 2020-07-28T22:02:41Z

Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Method	Header
createInstance	project
listInstances	project
listInstancesStream	project
listInstanceConfigs	project
listInstanceConfigsStream	project
deleteInstance	instance
getInstance	instance
updateInstance	instance
createBackup	instance
getBackup	instance
updateBackup	instance
deleteBackup	instance
restoreDatabase	instance
listBackups	instance
listBackupOperations	instance
listDatabaseOperations	instance
createDatabase	instance
listDatabases	instance
dropDatabase	database
getDatabase	database
getDatabaseDdl	database
updateDatabaseDdl	database
partitionQuery	database
partitionRead	database
batchCreateSessions	database
createSession	database
listSessions	database
listSessionsStream	database
deleteSession	database
getSession	database
executeSql	database
beginTransaction	database
streamingRead	database
executeStreamingSql	database
executeBatchDml	database
commit	database
rollback	database

codecov · 2020-07-28T22:20:07Z

Codecov Report

Merging #1196 into master will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1196      +/-   ##
==========================================
+ Coverage   98.24%   98.58%   +0.34%     
==========================================
  Files          23       23              
  Lines       21191    21275      +84     
  Branches     1199     1233      +34     
==========================================
+ Hits        20820    20975     +155     
+ Misses        368      297      -71     
  Partials        3        3

Impacted Files	Coverage Δ
src/backup.ts	`99.62% <100.00%> (+<0.01%)`	⬆️
src/batch-transaction.ts	`100.00% <100.00%> (ø)`
src/common.ts	`100.00% <100.00%> (+100.00%)`	⬆️
src/database.ts	`99.92% <100.00%> (+<0.01%)`	⬆️
src/index.ts	`100.00% <100.00%> (ø)`
src/instance.ts	`100.00% <100.00%> (ø)`
src/session.ts	`100.00% <100.00%> (ø)`
src/transaction.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7412122...bea4300. Read the comment docs.

AVaksman · 2020-07-29T03:04:28Z

@skuruppu please verify the correctness of header to method

skuruppu

Thanks very much for making this change, especially with having to switch between the various values that could be set for this header.

src/database.ts

skuruppu · 2020-07-30T01:00:51Z

src/index.ts

-    delete reqOpts.gaxOptions;
+    const gaxOpts = addResourcePrefixHeader(
+      options.gaxOptions!,
+      'projects/' + this.projectId


Should we just add a this.formattedName_ field to this class?

skuruppu · 2020-07-30T01:21:20Z

test/batch-transaction.ts

+          (batchTransaction.session.parent as Database).formattedName_
+        )
+      );
+      console.log(gaxOpts);


nit: remove

test/batch-transaction.ts

stephenplusplus · 2020-07-30T15:34:58Z

Would it work to do this all in the request function, somewhere around here:

nodejs-spanner/src/index.ts

Line 856 in d835ba2

const requestFn = gaxClient[config.method].bind(

?

To handle the different values that are used for the header, the value could be passed to the function call, e.g.:

this.request({
  client: 'InstanceAdminClient',
  method: 'listInstanceConfigs',
  reqOpts,
  gaxOpts,
  resourcePrefixHeader: 'projects/' + this.projectId,
});

Or even:

this.request({
  client: 'InstanceAdminClient',
  method: 'listInstanceConfigs',
  reqOpts,
  gaxOpts,
  headers: {
    'google-cloud-resource-prefix': 'projects/' + this.projectId,
  },
});

Reducing it to this format should save a lot of code, and be a bit more future proof should some other requirement come along to set a property on the gaxOptions, like:

gaxOpts: addTrackingIdHeader(addResourcePrefixHeader({}, this.formattedName_)),

Co-authored-by: skuruppu <[email protected]>

AVaksman · 2020-08-03T15:13:59Z

Different methods require a different level of granularity, project, instance, database.
The desired data can come in the reqOpts object under different properties, parent, name, database, backup.name, instance.name, session (in case of session, for header purposes the path would have to be shortened to database).

stephenplusplus · 2020-08-03T15:18:59Z

I'm sorry if I'm not catching on quickly enough, but how does that invalidate my suggestion for code simplification? I'm just looking at replacing all of the calls to addResourcePrefixHeader(theValue) with something more generic and adaptable in the future.

export function addResourcePrefixHeader(
  gaxOpts: CallOptions,
  headerValue: string
): CallOptions {
  return extend(true, {}, gaxOpts, {
    otherArgs: {
      headers: {[CLOUD_RESOURCE_HEADER]: headerValue},
    },
  });
}

AVaksman · 2020-08-04T21:12:53Z

@stephenplusplus PTAL
Is it closer to what you had in mind?

stephenplusplus

👍 Thank you! I had some small tweaks to suggest in this review, but I appreciate you taking the time to make the changes.

src/index.ts

stephenplusplus · 2020-08-10T14:06:26Z

@AVaksman thank you for all of the changes! I think the resource header is set very nicely. There are a lot of seemingly unrelated changes to how reqOpts combines with gaxOpts. I wouldn't mind a separate PR for those, unless they are actually tied to this change?

AVaksman · 2020-08-10T15:44:30Z

Not tied to this change.
Will move it in a separate PR.

src/backup.ts

test/batch-transaction.ts

src/common.ts

stephenplusplus · 2020-08-10T15:58:26Z

test/database.ts

@@ -999,7 +1010,7 @@ describe('Database', () => {
    it('should accept gaxOptions', done => {
      const gaxOptions = {};
      database.request = config => {
-        assert.strictEqual(config.gaxOpts, gaxOptions);
+        assert.deepStrictEqual(config.gaxOpts, gaxOptions);


In the cases where our src/ code directly passes the gaxOptions object that the user gives, I think "strictEqual" is the right case. When the src/ code modifies the object, we should add some properties to the test's "gaxOptions" and "deepStrictEqual" that they're still there.

src/index.ts

test/backup.ts

AVaksman · 2020-08-17T17:59:18Z

@stephenplusplus PTAL

stephenplusplus

Thank you! Just these few gaxOptions lingering issues.

stephenplusplus · 2020-08-17T18:56:32Z

test/database.ts

@@ -993,7 +1006,7 @@ describe('Database', () => {
    it('should accept gaxOptions', done => {
      const gaxOptions = {};
      database.request = config => {
-        assert.strictEqual(config.gaxOpts, gaxOptions);
+        assert.deepStrictEqual(config.gaxOpts, gaxOptions);


This is a test that would be a false positive, unless we put in fake properties or revert to comparing that the input object is strictly equal.

stephenplusplus · 2020-08-17T18:58:12Z

test/database.ts

@@ -1648,7 +1665,7 @@ describe('Database', () => {
    it('should accept gaxOptions', done => {
      const gaxOptions = {};
      database.request = config => {
-        assert.strictEqual(config.gaxOpts, gaxOptions);
+        assert.deepStrictEqual(config.gaxOpts, gaxOptions);


This is a test that would be a false positive, unless we put in fake properties or revert to comparing that the input object is strictly equal.

AVaksman · 2020-08-17T20:37:19Z

Done, thank you!

stephenplusplus · 2020-08-19T14:59:40Z

src/session.ts

@@ -36,7 +36,9 @@ import {
  CreateSessionOptions,
 } from './database';
 import {ServiceObjectConfig} from '@google-cloud/common';
-import {NormalCallback} from './common';
+import {NormalCallback, CLOUD_RESOURCE_HEADER} from './common';
+// import {ServiceObjectConfig} from '@google-cloud/common';


Suggested change

// import {ServiceObjectConfig} from '@google-cloud/common';

// import {ServiceObjectConfig} from '@google-cloud/common';

I think these comments can be removed. (This and the next line)

stephenplusplus · 2020-08-19T14:59:52Z

src/session.ts

-import {NormalCallback} from './common';
+import {NormalCallback, CLOUD_RESOURCE_HEADER} from './common';
+// import {ServiceObjectConfig} from '@google-cloud/common';
+// import {NormalCallback} from './common';


Suggested change

// import {NormalCallback} from './common';

// import {NormalCallback} from './common';

stephenplusplus · 2020-08-19T15:03:15Z

test/batch-transaction.ts

@@ -181,7 +190,7 @@ describe('BatchTransaction', () => {
    const RESPONSE = {partitions: PARTITIONS};

    const QUERY = {a: 'b'};
-    const CONFIG = {reqOpts: QUERY};
+    const CONFIG = {reqOpts: QUERY, gaxOpts: {timeout: 1000}};


Suggested change

const CONFIG = {reqOpts: QUERY, gaxOpts: {timeout: 1000}};

const CONFIG = {reqOpts: QUERY};

It doesn't look like this function is doing anything with gaxOpts, or that we're testing it anywhere, so having this defined here could be confusing.

stephenplusplus · 2020-08-19T15:06:40Z

test/database.ts

@@ -1017,7 +1032,7 @@ describe('Database', () => {
    it('should accept gaxOptions', done => {
      const gaxOptions = {};
      database.request = config => {
-        assert.strictEqual(config.gaxOpts, gaxOptions);
+        assert.deepStrictEqual(config.gaxOpts, gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, gaxOptions);

assert.strictEqual(config.gaxOpts, gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on gaxOptions.

stephenplusplus · 2020-08-19T15:14:00Z

test/instance.ts

@@ -258,7 +269,7 @@ describe('Instance', () => {
    it('should accept gaxOptions', done => {
      const options = Object.assign({}, OPTIONS, {gaxOptions: {}});
      instance.request = config => {
-        assert.strictEqual(config.gaxOpts, options.gaxOptions);
+        assert.deepStrictEqual(config.gaxOpts, options.gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, options.gaxOptions);

assert.strictEqual(config.gaxOpts, options.gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on options.gaxOptions.

stephenplusplus · 2020-08-19T15:17:34Z

test/transaction.ts

@@ -558,7 +575,7 @@ describe('Transaction', () => {
        const {gaxOpts, reqOpts} = REQUEST_STREAM.lastCall.args[0];

        assert.strictEqual(reqOpts.gaxOptions, undefined);
-        assert.strictEqual(gaxOpts, fakeQuery.gaxOptions);
+        assert.deepStrictEqual(gaxOpts, fakeQuery.gaxOptions);


Suggested change

assert.deepStrictEqual(gaxOpts, fakeQuery.gaxOptions);

assert.strictEqual(gaxOpts, fakeQuery.gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on fakeQuery.gaxOptions.

stephenplusplus · 2020-08-19T15:17:57Z

test/transaction.ts

@@ -1022,7 +1039,7 @@ describe('Transaction', () => {
      it('should accept gaxOptions', done => {
        const gaxOptions = {};
        transaction.request = config => {
-          assert.strictEqual(config.gaxOpts, gaxOptions);
+          assert.deepStrictEqual(config.gaxOpts, gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, gaxOptions);

assert.strictEqual(config.gaxOpts, gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on gaxOptions.

stephenplusplus · 2020-08-19T15:18:18Z

test/transaction.ts

      });

      it('should accept gaxOptions', done => {
        const gaxOptions = {};
        transaction.request = config => {
-          assert.strictEqual(config.gaxOpts, gaxOptions);
+          assert.deepStrictEqual(config.gaxOpts, gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, gaxOptions);

assert.strictEqual(config.gaxOpts, gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on gaxOptions.

stephenplusplus · 2020-08-19T15:18:27Z

test/transaction.ts

      });

      it('should accept gaxOptions', done => {
        const gaxOptions = {};
        transaction.request = config => {
-          assert.strictEqual(config.gaxOpts, gaxOptions);
+          assert.deepStrictEqual(config.gaxOpts, gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, gaxOptions);

assert.strictEqual(config.gaxOpts, gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on gaxOptions.

stephenplusplus · 2020-08-19T15:18:36Z

test/transaction.ts

      });

      it('should accept gaxOptions', done => {
        const gaxOptions = {};
        transaction.request = config => {
-          assert.strictEqual(config.gaxOpts, gaxOptions);
+          assert.deepStrictEqual(config.gaxOpts, gaxOptions);


Suggested change

assert.deepStrictEqual(config.gaxOpts, gaxOptions);

assert.strictEqual(config.gaxOpts, gaxOptions);

If after making that change, the test fails, then we would want to keep deepStrictEqual, but also set a property on gaxOptions.

AVaksman · 2020-08-21T14:57:38Z

I believe I addressed all latest comments PTAL @stephenplusplus

stephenplusplus · 2020-08-25T14:19:12Z

src/session.ts

@@ -36,7 +36,9 @@ import {
  CreateSessionOptions,
 } from './database';
 import {ServiceObjectConfig} from '@google-cloud/common';
-import {NormalCallback} from './common';
+import {NormalCallback, CLOUD_RESOURCE_HEADER} from './common';
+// import {ServiceObjectConfig} from '@google-cloud/common';


I think these comments can be removed. (This and the next line)

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 28, 2020

AVaksman force-pushed the resource_header branch from dccd125 to a9b5b65 Compare July 28, 2020 22:12

AVaksman marked this pull request as ready for review July 29, 2020 03:04

AVaksman requested review from a team as code owners July 29, 2020 03:04

feat: add resource header prefix

33ba958

AVaksman force-pushed the resource_header branch from a9b5b65 to 33ba958 Compare July 29, 2020 03:33

skuruppu approved these changes Jul 30, 2020

View reviewed changes

AVaksman and others added 5 commits July 30, 2020 16:29

test: typo

1c3b849

Co-authored-by: skuruppu <[email protected]>

chore: clean up

1c920db

chore: add projectFormattedName_ to index class

a2d23d5

refactor: use projectFormattedName_

1a56146

Merge branch 'master' into resource_header

32d9c19

Merge branch 'master' into resource_header

0e5c7bf

refactor: refactor

a834cb5

stephenplusplus reviewed Aug 5, 2020

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

AVaksman added 2 commits August 6, 2020 18:13

chore: revert Instance.formatName_

33d4f49

chore: refactor

2c3f3d0

chore: revert unrelated refactoring

b100d28

stephenplusplus suggested changes Aug 10, 2020

View reviewed changes

AVaksman added 2 commits August 17, 2020 11:45

Merge branch master into resource_header

09fc866

chore: improve unit tests

83d6e10

stephenplusplus suggested changes Aug 17, 2020

View reviewed changes

test: improve test 2

06384ed

stephenplusplus suggested changes Aug 19, 2020

View reviewed changes

fix: improve gaxOptions tests

0514abc

product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Aug 21, 2020

Merge branch 'master' into resource_header

7fe70be

AVaksman added 2 commits August 22, 2020 22:11

Merge branch 'master' into resource_header

5d94a61

Merge branch 'master' into resource_header

6f07b76

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 24, 2020

stephenplusplus suggested changes Aug 25, 2020

View reviewed changes

chore: remove commented out imports

73abe18

stephenplusplus approved these changes Aug 25, 2020

View reviewed changes

AVaksman added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 25, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 25, 2020

Merge branch 'master' into resource_header

bea4300

AVaksman merged commit 99744b6 into googleapis:master Aug 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add resource header prefix #1196

feat: add resource header prefix #1196

AVaksman commented Jul 28, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading

AVaksman commented Jul 29, 2020

skuruppu left a comment

skuruppu Jul 30, 2020

skuruppu Jul 30, 2020

stephenplusplus commented Jul 30, 2020

AVaksman commented Aug 3, 2020

stephenplusplus commented Aug 3, 2020

AVaksman commented Aug 4, 2020

stephenplusplus left a comment

stephenplusplus commented Aug 10, 2020

AVaksman commented Aug 10, 2020 •

edited

Loading

stephenplusplus Aug 10, 2020

AVaksman commented Aug 17, 2020

stephenplusplus left a comment

stephenplusplus Aug 17, 2020

stephenplusplus Aug 17, 2020

AVaksman commented Aug 17, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 25, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

stephenplusplus Aug 19, 2020

AVaksman commented Aug 21, 2020

stephenplusplus Aug 25, 2020

	// import {ServiceObjectConfig} from '@google-cloud/common';
	// import {ServiceObjectConfig} from '@google-cloud/common';

	// import {NormalCallback} from './common';
	// import {NormalCallback} from './common';

	const CONFIG = {reqOpts: QUERY, gaxOpts: {timeout: 1000}};
	const CONFIG = {reqOpts: QUERY};

	assert.deepStrictEqual(config.gaxOpts, gaxOptions);
	assert.strictEqual(config.gaxOpts, gaxOptions);

	assert.deepStrictEqual(config.gaxOpts, options.gaxOptions);
	assert.strictEqual(config.gaxOpts, options.gaxOptions);

	assert.deepStrictEqual(gaxOpts, fakeQuery.gaxOptions);
	assert.strictEqual(gaxOpts, fakeQuery.gaxOptions);

feat: add resource header prefix #1196

feat: add resource header prefix #1196

Conversation

AVaksman commented Jul 28, 2020 • edited Loading

codecov bot commented Jul 28, 2020 • edited Loading

Codecov Report

AVaksman commented Jul 29, 2020

skuruppu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephenplusplus commented Jul 30, 2020

AVaksman commented Aug 3, 2020

stephenplusplus commented Aug 3, 2020

AVaksman commented Aug 4, 2020

stephenplusplus left a comment

Choose a reason for hiding this comment

stephenplusplus commented Aug 10, 2020

AVaksman commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

AVaksman commented Aug 17, 2020

stephenplusplus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVaksman commented Aug 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AVaksman commented Aug 21, 2020

Choose a reason for hiding this comment

AVaksman commented Jul 28, 2020 •

edited

Loading

codecov bot commented Jul 28, 2020 •

edited

Loading

AVaksman commented Aug 10, 2020 •

edited

Loading