Firebase App Auto Init by avishalom · Pull Request #164 · firebase/firebase-admin-node

avishalom · 2017-12-18T15:54:18Z

Adding the auto_init mechanism.
The env variable pointing to the config file is blanked by the config file when running the tests.

hiranya911

This is a start. But it seems to be missing some of the key elements. For instance I don't see the changes to initializeApp(), which makes it possible to call it without any arguments. I also think the logic for parsing the JSON file should be inside or close to initializeApp(), as opposed to the constructor of FirebaseApp.

hiranya911 · 2017-12-18T19:42:01Z

 account key file from the "Settings > Service Accounts" page of the project, and copy it to
 `test/resources/key.json`. Also obtain the API key for the same project from "Settings > General",
-and save it to `test/resource/apikey.txt`. Finally, to run the integration test suite:
+and save it to `test/resources/apikey.txt`. Finally, to run the integration test suite:


Duplicate of #163

hiranya911 · 2017-12-18T19:43:59Z

+ let contents;
+ try {
+ contents = fs.readFileSync(process.env[this.firebaseInternals_.CONFIG_FILE_VAR], 'utf8');
+ } catch (ignored) {


When will this return an error? Does it make sense to expose this to the developer?

hiranya911 · 2017-12-18T19:45:11Z

 * Internals of a FirebaseNamespace instance.
 */
 export class FirebaseNamespaceInternals {
+ public CONFIG_FILE_VAR: string = 'FIREBASE_CONFIG';


Please make this a constant. Also lets rename to something like FIREBASE_CONFIG_ENV_VAR.

hiranya911 · 2017-12-18T19:46:34Z

@@ -0,0 +1,5 @@
+{
+ "databaseURL": "https://hipster-chat.firebaseio.com",
+ "projectId": "hipster-chat",


Please use some other obviously fake project ID. Same for other parameters in these files. See mock.key.json for an example.

hiranya911

Looks mostly good. Some suggestions to clean up and improve. Also need to cover a couple more cases in testes.

hiranya911 · 2017-12-19T19:14:37Z

-let globalCertCreds: {[key: string]: CertCredential} = {};
-let globalRefreshTokenCreds: {[key: string]: RefreshTokenCredential} = {};
+let globalCertCreds: {[key: string]: CertCredential} = {};
+let globalRefreshTokenCreds: {[key: string]: RefreshTokenCredential} = {};


Please revert these changes and keep the diff small.

hiranya911 · 2017-12-19T19:18:21Z

 */
- public initializeApp(options: FirebaseAppOptions, appName = DEFAULT_APP_NAME): FirebaseApp {
+ public initializeApp(
+ options: FirebaseAppOptions = { credential: new ApplicationDefaultCredential() },


it is a little weird to be using an entire object as a default argument. Will the following make this a bit more readable?
public initializeApp(options?: FirebaseAppOptions, appName?: string): FirebaseApp { if (typeof options === 'undefined') { options = {credential: new ApplicationDefaultCredential()}; } }

hiranya911 · 2017-12-19T19:19:00Z

+ /**
+ * Parse the file pointed to by the FIREBASE_CONFIG_FILE_VAR, if it exists 
+ */
+ private useConfigEnvVar_(): FirebaseAppOptions {


Lets rename this to loadOptionsFromEnvironment(). Also this function doesn't return anything, so the signature should be updated.

hiranya911 · 2017-12-19T19:20:01Z

+ * Parse the file pointed to by the FIREBASE_CONFIG_FILE_VAR, if it exists 
+ */
+ private useConfigEnvVar_(): FirebaseAppOptions {
+ const FIREBASE_CONFIG_FILE_VAR: string = 'FIREBASE_CONFIG';


Define this at module level, so it can be used in tests.

hiranya911 · 2017-12-19T19:23:58Z

+ this.options_.storageBucket !== undefined) {
+ return;
+ }
+ if (process.env[FIREBASE_CONFIG_FILE_VAR]) {


Return early if the variable is not set.
if (!process.env[FIREBASE_CONFIG_FILE_VAR]) { return; }

hiranya911 · 2017-12-19T19:49:58Z

 });
+
+ it('should overwrite the config values with the ones in the config file', () => {
+ process.env[FIREBASE_CONFIG_FILE_VAR] = './test/resources/firebase_config.json';


You should first unset this variable in the beforeEach hook, and restore it to the original value in afterEach hook. Then you don't have to delete it in the test cases. See Firestore unit tests for some examples.

hiranya911 · 2017-12-19T19:53:12Z

+ expect(app.options.storageBucket).to.equal('bucketName.appspot.com');
+ });
+
+ it('should use the existing values when some of them aren\'t overwritten', () => {


aren't --> are not

hiranya911 · 2017-12-19T19:53:58Z

+ expect(app.options.storageBucket).to.equal(undefined);
+ });
+
+ it('should init without problem when the config env var isn\'t set', () => {


should not throw when the config environment variable is not set

Also add a test case when env variable is not set, and options are not explicitly specified. Should result in an error.

hiranya911 · 2017-12-19T19:54:45Z

+ expect(app.options.storageBucket).to.equal("hipster-chat.appspot.mock");
+ });
+
+ it('should complain about a non existant file', () => {


should throw when the environment variable points to non existing file

hiranya911 · 2017-12-19T20:59:35Z

+ expect(app.options.storageBucket).to.equal(undefined);
+ });
+
+ it('should init with no params setting default credential', () => {


Add another test case where the environment variable is set.

Ok, it wasn't clear what's going on here. Shall we update the description to something like 'should init with application default credentials when no options provided and the environment variable is not set'?

hiranya911

Looks good. A few nits to address, and then I can lgtm

hiranya911 · 2017-12-20T18:47:48Z

+/**
+ * Constant holding the enviromnet variable that holds the default config.
+ */
+


Redundant empty line

hiranya911 · 2017-12-20T18:47:58Z

+/**
+ * List of keys expected in the config file. 
+ */
+


Please remove

hiranya911 · 2017-12-20T18:48:57Z

+ if (allSpecified) {
+ return;
+ }
+ if (process.env[FIREBASE_CONFIG_FILE_VAR] === undefined) {


This can be OR'ed with the previous check for allSpecified

hiranya911 · 2017-12-20T18:49:24Z

+ );
+ }
+ this.options_ = Object.assign(jsonContent, this.options_);
+ // for (let field of FIREBASE_CONFIG_KEYS) {


Please remove

hiranya911 · 2017-12-20T18:54:44Z

 afterEach(() => {
 this.clock.restore();
-
+ if (firebaseConfigVar == undefined || firebaseConfigVar == 'undefined') {


This check looks weird. Do we need it at all?

setting the process.env[FIREBASE_CONFIG_FILE_VAR] to the value of firebaseConfigVar : string when that is undefined results in the assignment of the string 'undefined'
the second check is moot though

hiranya911 · 2017-12-20T18:57:28Z

+ });
+
+ it('should throw when the environment variable points to non parsable file', () => {
+ process.env[FIREBASE_CONFIG_FILE_VAR] = './test/resources/firebase_config_bad.json' ;


Is this file empty? I would recommend adding some obviously malformed content in there to make that obvious.

hiranya911 · 2017-12-20T18:58:19Z

+ }).to.throw(`Failed to parse app options file`);
+ });
+
+ it('should use the existing values when some of them exist', () => {


'should use explicitly specified options when available'

hiranya911 · 2017-12-20T18:58:55Z

+ expect(app.options.storageBucket).to.equal('bucketName.appspot.com');
+ });
+
+ it('should use the existing values when some of them are not overwritten', () => {


Not clear what we are testing here. Please revise the description.

hiranya911 · 2017-12-20T18:59:35Z

+ expect(app.options.storageBucket).to.equal(undefined);
+ });
+
+ it('should init with params from the config file, setting default credential', () => {


'should init when no init arguments are provided'

hiranya911 · 2017-12-20T19:00:24Z

+ });
+
+ it('should init with params from the config file, setting default credential', () => {
+ process.env[FIREBASE_CONFIG_FILE_VAR] = './test/resources/firebase_config.json';


Need one more test case for when options are not specified, and neither the env variable is set.

that is the previous case

…s unset

…est is not necessary

hiranya911

Looks mostly good. A few minor nits, and then I can approve.

hiranya911 · 2018-01-02T18:37:53Z


 if (typeof this.options_ !== 'object' || this.options_ === null) {
 // Ensure the options are a non-null object
+ // This shouldn't hapen 


Please remove

hiranya911 · 2018-01-02T18:41:01Z

+ * Parse the file pointed to by the FIREBASE_CONFIG_FILE_VAR, if it exists 
+ */
+ private loadOptionsFromEnvVar(): FirebaseAppOptions {
+ if (typeof process.env[FIREBASE_CONFIG_FILE_VAR] === 'undefined' ||


let filePath = process.env[FIREBASE_CONFIG_FILE_VAR]; if (!validator.isNonEmptyString(filePath)) { return {}; }

hiranya911 · 2018-01-02T18:43:05Z

+ return {};
+ }
+ let contents;
+ try {


Merge these try-catch blocks into one:
try { let contents = fs.readFileSync(filePath, 'utf8'); return JSON.parse(contents) as FirebaseAppOptions; } catch (error) { // error message: 'Failed to load app options from environment: ' + error }

hiranya911 · 2018-01-02T18:45:26Z

 * @return {FirebaseApp} A new FirebaseApp instance.
 */
- public initializeApp(options: FirebaseAppOptions, appName?: string): FirebaseApp {
+ public initializeApp(options?: FirebaseAppOptions, appName?: string): FirebaseApp {


Update the argument description for options.

hiranya911 · 2018-01-02T18:47:04Z

+ expect(app.options.databaseURL).to.undefined;
+ expect(app.options.projectId).to.be.undefined;
+ expect(app.options.storageBucket).to.undefined;
+ });


Add empty line

hiranya911 · 2018-01-02T18:49:12Z

@@ -0,0 +1,4 @@
+{
+ "databaseUrl": "https://hipster-chat.firebaseio.mock",


Please add another key that is obviously bad. Something like "notSupported": "some value"

hiranya911 · 2018-01-02T18:49:56Z

+ expect(app.options.storageBucket).to.equal('bucketName.appspot.com');
+ });
+
+ it('should ignore the config file even if some fields are missing', () => {


should not throw if some...

hiranya911 · 2018-01-02T18:50:42Z

+ });
+
+ it('should not throw when the config environment variable is not set', () => {
+ const app = firebaseNamespace.initializeApp(mocks.appOptionsNoDatabaseUrl, mocks.appName);


No options should be passed here -- I think.

that is the following case.
(this case can be deleted, but no harm in leaving it in)

avishalom · 2018-01-02T22:38:40Z

ok, done + added the json from env var option

hiranya911

LGTM with a couple of nits. No further reviews necessary. Thanks for putting it all together.

hiranya911 · 2018-01-03T04:32:15Z

+ if (!validator.isNonEmptyString(config)) {
+ return {};
+ }
+ let contents;


Nit: Move the declaration of contents into the try block.

hiranya911 · 2018-01-03T04:32:55Z

+ // Assume filename.
+ contents = fs.readFileSync(config, 'utf8');
+ }
+ jsonContent = JSON.parse(contents);


Nit: Return here and remove the jsonContent variable: return JSON.parse(contents) as FirebaseAppOptions

avishalom added 4 commits December 15, 2017 17:06

path typo
dc79777

add overwrite defaults for config
dcc5e16

parametrize the env var name
4362594

Adding test cases for firebase_config
e0798d0

avishalom requested a review from hiranya911 December 18, 2017 15:54

avishalom assigned hiranya911 Dec 18, 2017

avishalom added 7 commits December 18, 2017 11:42

remove whitespaces
890a734

more whitespace
04f307f

remove more whitespace
43efe2f

revert package and package-lock.json
eeb91aa

clean output
1123778

Partial test
d4f2966

typo fix
0534968

hiranya911 suggested changes Dec 18, 2017
View reviewed changes

avishalom added 3 commits December 19, 2017 07:50

PR comments, invocation with no params.
b44bdfa

lint
1275819

add a bad json test
eec3a6e

hiranya911 suggested changes Dec 19, 2017
View reviewed changes

avishalom added 8 commits December 20, 2017 10:27

PR comments
166cc64

whitespace (undo auto formatting)
774e23a

whitespace
d37709f

whitespace and after hook
4e19460

whitespace undo auto format
c15ad1a

add test for empty invocation
ce590d7

fix error message to be cakwards compatible
e201c4e

whitespace
5b9e528

hiranya911 suggested changes Dec 20, 2017
View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Dec 20, 2017

PR comments, add test
f321672

override the FIREBASE_CONFIG env var for tests, and treat empty var a…
8b95550
…s unset

avishalom changed the title ~~Auto init~~Firebase App Auto InitDec 23, 2017

avishalom added 3 commits December 29, 2017 00:01

whitespace
8f4c4eb

revert the update behaviour, use FIREBASE_CONFIG only with no options
f4db9f5

remove protective wrapper code, as ignoring the FIREBASE_CONFIG for t…
05eeb2c
…est is not necessary

hiranya911 approved these changes Jan 2, 2018
View reviewed changes

hiranya911 assigned avishalom and unassigned bojeil-google Jan 2, 2018

avishalom added 2 commits January 2, 2018 16:48

address comments on PR
725f904

Adding the optioon for the config var to holds json , + tests.
3ed4257

avishalom assigned hiranya911 and unassigned avishalom Jan 2, 2018

avishalom added 2 commits January 2, 2018 21:25

fix error message for js 5 compatibility
06b5fdc

merge master
61a6ad4

hiranya911 approved these changes Jan 3, 2018
View reviewed changes

hiranya911 assigned avishalom and unassigned hiranya911 Jan 3, 2018

avishalom added 2 commits January 9, 2018 10:01

inline return into try block
0684ed7

whitespace
a90f1da

avishalom merged commit 76a17f0 into masterJan 10, 2018

hiranya911 deleted the auto_init branch January 11, 2018 00:38

snyk-bot mentioned this pull request Nov 12, 2020
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#15
Open

snyk-bot mentioned this pull request May 4, 2021
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#16
Open

snyk-bot mentioned this pull request Aug 5, 2021
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#18
Open

snyk-bot mentioned this pull request Aug 12, 2021
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#19
Open

snyk-bot mentioned this pull request Sep 1, 2021
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#20
Open

snyk-bot mentioned this pull request Jun 30, 2022
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#26
Open

takeratta mentioned this pull request Mar 23, 2024
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#37
Open

takeratta mentioned this pull request May 23, 2024
[Snyk] Security upgrade @google-cloud/firestore from 0.8.2 to 0.13.1 takeratta/firebase-admin-node#38
Open

		@@ -0,0 +1,4 @@
		{
		"databaseUrl": "https://hipster-chat.firebaseio.mock",

Uh oh!

Conversation

avishalom commented Dec 18, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

avishalom commented Dec 18, 2017•
edited
Loading