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

CONTRIBUTING.md

 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

src/firebase-app.ts

+      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

src/firebase-namespace.ts

 * 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

test/resources/firebase_config.json

@@ -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

src/firebase-namespace.ts

-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

src/firebase-namespace.ts

   */
-  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

src/firebase-app.ts

+  /**
+   * 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

src/firebase-app.ts

+   * 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

src/firebase-app.ts

+      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

test/unit/firebase-app.spec.ts

    });
+
+    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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+      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

src/firebase-app.ts

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


Redundant empty line

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

src/firebase-app.ts

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


Please remove

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

src/firebase-app.ts

+    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

src/firebase-app.ts

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


Please remove

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

test/unit/firebase-app.spec.ts

  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

test/unit/firebase-app.spec.ts

+    });
+
+    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

test/unit/firebase-app.spec.ts

+      }).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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+    });
+
+    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

src/firebase-app.ts


    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

src/firebase-namespace.ts

+   * 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

src/firebase-namespace.ts

+      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

src/firebase-namespace.ts

   * @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

test/unit/firebase-app.spec.ts

+      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

test/resources/firebase_config_bad_key.json

@@ -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

test/unit/firebase-app.spec.ts

+      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

test/unit/firebase-app.spec.ts

+    });
+
+    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

src/firebase-namespace.ts

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


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

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

src/firebase-namespace.ts

+        // 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 Init Dec 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 master Jan 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",

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

Uh oh!

avishalom commented Dec 18, 2017 •

edited

Loading