fix: Set emulator credentials on rest fallback #1812
Conversation
product-auto-label
bot
added
size: s
| }; | ||
| const emulatorClient = | ||
| // eslint-disable-next-line node/no-extraneous-require | ||
| new (require('google-auth-library').OAuth2Client)(); |
There was a problem hiding this comment.
Since google-auth-library is not a direct dependency (it is a transient dependency of google-gax) We get extraneous-require lint error here. I think disabling it for this line is okay... WDYT?
There was a problem hiding this comment.
If we added 'google-auth-library' to nodejs-firestore dependencies, would that also resolve the issue? Or am I missing some other context?
There was a problem hiding this comment.
Yep, adding google-auth-library to package.json would resolve this, but I am not sure if that is necessary here as google-gax should pull in google-auth-library as a dependency anyway.
If a
require()'s target is extraneous (it's not written inpackage.json), the program works in local, but will not work after dependencies are re-installed. It will cause troubles to your team/contributors. This rule disallowsrequire()of extraneous modules.
| }; | ||
| const emulatorClient = | ||
| // eslint-disable-next-line node/no-extraneous-require | ||
| new (require('google-auth-library').OAuth2Client)(); |
There was a problem hiding this comment.
If we added 'google-auth-library' to nodejs-firestore dependencies, would that also resolve the issue? Or am I missing some other context?
There was a problem hiding this comment.
I updated the system tests to run against the emulator. Unfortunately, they are failing with this change, but passing without this change. Need to rescind the approval until we can understand the issue.
| const emulatorCredentials = { | ||
| access_token: 'owner', | ||
| }; | ||
| const emulatorClient = |
There was a problem hiding this comment.
Root cause for failing tests. The emulatorClient (cached credentials) needs to specify the project ID. If not specified here and not specified in the firestore settings, then the project ID will not be available, causing request the Firestore instance to behave incorrectly.
There was a problem hiding this comment.
I see! How does the system tests obtain the project id? We can use the firestore settings and GCLOUD_PROJECT env var here, if that makes sense.
emulatorClient.projectId = settings.projectId ?? process.env.GCLOUD_PROJECT;WDYT?
|
I'm closing this PR for now to clean things up. We're waiting on a change in the gax-fallback, so that there will be consistent implementation of credentials used for the emulator regardless of using rest or grpc. Thanks for creating this @lahirumaramba, we can re-open it later if the other approach fails. |
|
Makes sense! Thanks @MarkDuckworth ! |
Setting Emulator credentials when REST fallback is enabled by setting
preferRest: true.I am not familiar with the codebase and looking for review from the experts :)
We already set emulator credentials on gRPC mode by setting custom headers at:
nodejs-firestore/dev/src/index.ts
Line 1453 in 653a8e9
Test coverage:
Tried to add unit tests with
sinon.spyonmodules.exports.v1andFirestoreClient, but couldn't get it to work. Appreciate any thoughts on adding test coverage to this.Fixes #1811 🦕