-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Add Spanner + Cloud Functions sample #371
Conversation
Codecov Report
@@ Coverage Diff @@
## master #371 +/- ##
=======================================
Coverage 83.88% 83.88%
=======================================
Files 4 4
Lines 422 422
=======================================
Hits 354 354
Misses 68 68 Continue to review full report at Codecov.
|
functions/spanner/index.js
Outdated
const Spanner = require('@google-cloud/spanner'); | ||
|
||
// Your Google Cloud Platform project ID | ||
const projectId = 'YOUR_PROJECT_ID'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users should not be hard-coding projectId
variables inside Cloud Functions (the environment provides the project ID. Remove this line, and don't pass project ID into the client library constructor.
functions/spanner/index.js
Outdated
var data = []; | ||
rows.forEach((row) => data.push(row.toJSON())); | ||
res.send(data); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling.
functions/spanner/test/index.test.js
Outdated
const sample = getSample(); | ||
const mocks = sample.mocks; | ||
|
||
const err = await sample.program.get(mocks.req, mocks.res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no point using async/await in this test function, as program.get
does not return a promise (and shouldn't).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so change line to:
sample.program.get(mocks.req, mocks.res);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI - this causes the test to fail. IIUC, the test itself needs to complete before the stubs can be checked.
(It's probably possible to de-promisify the test itself - but unless we do that, we have to keep this await
here.)
functions/spanner/test/index.test.js
Outdated
const mocks = sample.mocks; | ||
|
||
const err = await sample.program.get(mocks.req, mocks.res); | ||
t.falsy(err, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line
}; | ||
} | ||
|
||
test(`get: Gets albums`, async (t) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove async
functions/spanner/index.js
Outdated
const rows = results[0]; | ||
var data = []; | ||
rows.forEach((row) => data.push(row.toJSON())); | ||
res.send(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to res.send(rows.map((row) => row.toJSON()));
and delete the redundant lines
functions/spanner/test/index.test.js
Outdated
const sample = getSample(); | ||
const mocks = sample.mocks; | ||
|
||
const err = sample.program.get(mocks.req, mocks.res); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fails lint, plus, program.get
does not have a return value.
functions/spanner/index.js
Outdated
database.run(query) | ||
.then((results) => { | ||
const rows = results[0]; | ||
res.send(rows.map((row) => row.toJSON())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just be explicit:
res
.status(200)
.send(rows.map((row) => row.toJSON()))
.end();
functions/spanner/index.js
Outdated
}) | ||
.catch((err) => { | ||
res.status(500); | ||
res.send(`Error querying Spanner: ${err}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same:
res
.status(500)
.send(`Error querying Spanner: ${err}`)
.end();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the end()
necessary if we're calling send()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a convention I follow in all our App Engine and Cloud Functions samples. end()
is just saying: "we're done formulating the response".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha - will update accordingly.
functions/spanner/index.js
Outdated
.then((results) => { | ||
const rows = results[0].map((row) => row.toJSON()); | ||
rows.forEach((row) => { | ||
res.write(`SingerId: ${row.SingerId.value}, AlbumId: ${row.AlbumId.value}, AlbumTitle: ${row.AlbumTitle}</br>`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to set the Content-Type
response header? (If you're using res.write
, I'm wondering if it will correctly detect the content-type as text/html
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, it sets the Content-Type
to text/plain
- which IMO should be fine unless we're specifically trying to show HTML.
(If I convert the </br>
s to \n
s, I can get the same desired result with text/plain
.)
functions/spanner/index.js
Outdated
}; | ||
|
||
// Execute the query | ||
database.run(query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To eliminate possible race condition/flakiness in the test:
s/database.run(query)
/return database.run(query)
/
🤖 I have created a release \*beep\* \*boop\* --- ## [1.9.0](https://www.github.com/googleapis/nodejs-tasks/compare/v1.8.0...v1.9.0) (2020-03-06) ### Features * deferred client initialization ([#370](https://www.github.com/googleapis/nodejs-tasks/issues/370)) ([05fdd69](https://www.github.com/googleapis/nodejs-tasks/commit/05fdd6987c916da04c62193fe0f1081c23b85cbe)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
🤖 I have created a release \*beep\* \*boop\* --- ## [1.9.0](https://www.github.com/googleapis/nodejs-tasks/compare/v1.8.0...v1.9.0) (2020-03-06) ### Features * deferred client initialization ([#370](https://www.github.com/googleapis/nodejs-tasks/issues/370)) ([05fdd69](https://www.github.com/googleapis/nodejs-tasks/commit/05fdd6987c916da04c62193fe0f1081c23b85cbe)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
…e in region tag (#371) - [ ] Regenerate this pull request now. PiperOrigin-RevId: 399287285 Source-Link: googleapis/googleapis@1575986 Source-Link: https://github.com/googleapis/googleapis-gen/commit/b27fff623a5d8d586b703b5e4919856abe7c2eb3 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjI3ZmZmNjIzYTVkOGQ1ODZiNzAzYjVlNDkxOTg1NmFiZTdjMmViMyJ9
…e in region tag (#371) - [ ] Regenerate this pull request now. PiperOrigin-RevId: 399287285 Source-Link: googleapis/googleapis@1575986 Source-Link: googleapis/googleapis-gen@b27fff6 Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiYjI3ZmZmNjIzYTVkOGQ1ODZiNzAzYjVlNDkxOTg1NmFiZTdjMmViMyJ9
🤖 I have created a release \*beep\* \*boop\* --- ## [2.7.0](https://www.github.com/googleapis/nodejs-video-intelligence/compare/v2.6.5...v2.7.0) (2020-02-13) ### Features * face and person detection samples ([#362](https://www.github.com/googleapis/nodejs-video-intelligence/issues/362)) ([cff2f36](https://www.github.com/googleapis/nodejs-video-intelligence/commit/cff2f36a4e6294908a4e26587ed840c1ec1b03f8)) ### Bug Fixes * adds spaces to region tags, other fixes ([#369](https://www.github.com/googleapis/nodejs-video-intelligence/issues/369)) ([2b6943e](https://www.github.com/googleapis/nodejs-video-intelligence/commit/2b6943ee0685761a0076c7b8023eed4f12f93d64)) * fixes face and people detection region tags ([#367](https://www.github.com/googleapis/nodejs-video-intelligence/issues/367)) ([ab039b5](https://www.github.com/googleapis/nodejs-video-intelligence/commit/ab039b56b3bea27edf93c4db7c97241599d38419)) * refactors person and face detection samples into separate files ([#370](https://www.github.com/googleapis/nodejs-video-intelligence/issues/370)) ([eb9b400](https://www.github.com/googleapis/nodejs-video-intelligence/commit/eb9b400c24bdf306d8263ec402922b3235754034)) * updates README file with correct links ([#371](https://www.github.com/googleapis/nodejs-video-intelligence/issues/371)) ([fb2701a](https://www.github.com/googleapis/nodejs-video-intelligence/commit/fb2701a81c7476ef06ab279a8d4572f006abe831)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
🤖 I have created a release \*beep\* \*boop\* --- ## [2.7.0](https://www.github.com/googleapis/nodejs-video-intelligence/compare/v2.6.5...v2.7.0) (2020-02-13) ### Features * face and person detection samples ([#362](https://www.github.com/googleapis/nodejs-video-intelligence/issues/362)) ([cff2f36](https://www.github.com/googleapis/nodejs-video-intelligence/commit/cff2f36a4e6294908a4e26587ed840c1ec1b03f8)) ### Bug Fixes * adds spaces to region tags, other fixes ([#369](https://www.github.com/googleapis/nodejs-video-intelligence/issues/369)) ([2b6943e](https://www.github.com/googleapis/nodejs-video-intelligence/commit/2b6943ee0685761a0076c7b8023eed4f12f93d64)) * fixes face and people detection region tags ([#367](https://www.github.com/googleapis/nodejs-video-intelligence/issues/367)) ([ab039b5](https://www.github.com/googleapis/nodejs-video-intelligence/commit/ab039b56b3bea27edf93c4db7c97241599d38419)) * refactors person and face detection samples into separate files ([#370](https://www.github.com/googleapis/nodejs-video-intelligence/issues/370)) ([eb9b400](https://www.github.com/googleapis/nodejs-video-intelligence/commit/eb9b400c24bdf306d8263ec402922b3235754034)) * updates README file with correct links ([#371](https://www.github.com/googleapis/nodejs-video-intelligence/issues/371)) ([fb2701a](https://www.github.com/googleapis/nodejs-video-intelligence/commit/fb2701a81c7476ef06ab279a8d4572f006abe831)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
🤖 I have created a release \*beep\* \*boop\* --- ## [2.7.0](https://www.github.com/googleapis/nodejs-video-intelligence/compare/v2.6.5...v2.7.0) (2020-02-13) ### Features * face and person detection samples ([#362](https://www.github.com/googleapis/nodejs-video-intelligence/issues/362)) ([cff2f36](https://www.github.com/googleapis/nodejs-video-intelligence/commit/cff2f36a4e6294908a4e26587ed840c1ec1b03f8)) ### Bug Fixes * adds spaces to region tags, other fixes ([#369](https://www.github.com/googleapis/nodejs-video-intelligence/issues/369)) ([2b6943e](https://www.github.com/googleapis/nodejs-video-intelligence/commit/2b6943ee0685761a0076c7b8023eed4f12f93d64)) * fixes face and people detection region tags ([#367](https://www.github.com/googleapis/nodejs-video-intelligence/issues/367)) ([ab039b5](https://www.github.com/googleapis/nodejs-video-intelligence/commit/ab039b56b3bea27edf93c4db7c97241599d38419)) * refactors person and face detection samples into separate files ([#370](https://www.github.com/googleapis/nodejs-video-intelligence/issues/370)) ([eb9b400](https://www.github.com/googleapis/nodejs-video-intelligence/commit/eb9b400c24bdf306d8263ec402922b3235754034)) * updates README file with correct links ([#371](https://www.github.com/googleapis/nodejs-video-intelligence/issues/371)) ([fb2701a](https://www.github.com/googleapis/nodejs-video-intelligence/commit/fb2701a81c7476ef06ab279a8d4572f006abe831)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
* build!: Update library to use Node 12 Co-authored-by: Summer Ji <[email protected]> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [2.3.0](googleapis/nodejs-ai-platform@v2.2.0...v2.3.0) (2022-09-21) ### Features * Add deleteFeatureValues in aiplatform v1beta1 featurestore_service.proto ([#371](googleapis/nodejs-ai-platform#371)) ([e1c5cd6](googleapis/nodejs-ai-platform@e1c5cd6)) * Add timestamp_outside_retention_rows_count, RemoveContextChildren, order_by, InputArtifact, read_mask, TransferLearningConfig ([#450](googleapis/nodejs-ai-platform#450)) ([3a3f71f](googleapis/nodejs-ai-platform@3a3f71f)) ### Bug Fixes * Allow passing gax instance to client constructor ([#365](googleapis/nodejs-ai-platform#365)) ([6200e38](googleapis/nodejs-ai-platform@6200e38)) * Preserve default values in x-goog-request-params header ([#370](googleapis/nodejs-ai-platform#370)) ([6860cfd](googleapis/nodejs-ai-platform@6860cfd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
* chore(main): release 3.1.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
* chore(main): release 3.1.0 * 🦉 Updates from OwlBot post-processor See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com> Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [2.3.0](googleapis/nodejs-ai-platform@v2.2.0...v2.3.0) (2022-09-21) ### Features * Add deleteFeatureValues in aiplatform v1beta1 featurestore_service.proto ([#371](googleapis/nodejs-ai-platform#371)) ([e1c5cd6](googleapis/nodejs-ai-platform@e1c5cd6)) * Add timestamp_outside_retention_rows_count, RemoveContextChildren, order_by, InputArtifact, read_mask, TransferLearningConfig ([#450](googleapis/nodejs-ai-platform#450)) ([3a3f71f](googleapis/nodejs-ai-platform@3a3f71f)) ### Bug Fixes * Allow passing gax instance to client constructor ([#365](googleapis/nodejs-ai-platform#365)) ([6200e38](googleapis/nodejs-ai-platform@6200e38)) * Preserve default values in x-goog-request-params header ([#370](googleapis/nodejs-ai-platform#370)) ([6860cfd](googleapis/nodejs-ai-platform@6860cfd)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
FYI @WalterHub - this is the sample you wanted me to add.
@jmdobry: the sample code itself has (presumably) already been reviewed (given that it was pasted directly into a docs page). The test code, however, is new and has **not** been reviewed yet.