Enable 'strictNullChecks' ts compiler option. #676

rsgowman · 2019-10-18T16:01:54Z

In the majority of cases, I simply turned stuff like this:

function fname(param: T) {
  if (!param) throw Error("invalid param; must not be nullish");
  ...
}

into this:

function fname(param: T|null|undefined) {
  if (!param) throw Error("invalid param; must not be nullish");
  ...
}

A more sensible approach would be eliminate the possibility of
null/undef and eliminate the checks too, but that's a bit more involved
and this patch is already enormous. But I think these sorts of
improvements could be taken advantage of opportunistically as the code
is worked on in the future.

In the majority of cases, I simply turned stuff like this: ``` function fname(param: T) { if (!param) throw Error("invalid param; must not be nullish"); ... } ``` into this: ``` function fname(param: T|null|undefined) { if (!param) throw Error("invalid param; must not be nullish"); ... } ``` A more sensible approach would be eliminate the possibility of null/undef and eliminate the checks too, but that's a bit more involved and this patch is already enormous. But I think these sorts of improvements could be taken advantage of opportunistically as the code is worked on in the future.

And also update @firebase/auth to most recent to match.

hiranya911

Thanks for doing this @rsgowman. Much overdue improvements. It looks mostly good. I just added a few suggestions and questions for clarity.

src/auth/action-code-settings-builder.ts

hiranya911 · 2019-11-08T22:46:04Z

src/auth/auth-api-request.ts

@@ -683,7 +683,7 @@ const LIST_INBOUND_SAML_CONFIGS = new ApiSettings('/inboundSamlConfigs', 'GET')
 * Class that provides the mechanism to send requests to the Firebase Auth backend endpoints.
 */
 export abstract class AbstractAuthRequestHandler {
-  protected readonly projectId: string;
+  protected readonly projectId: string | null;


Should never be null I think.

It's set via utils.getProjectId (src/utils/index.ts:66), which returns null if all other options fail. (And is documented as such too, so it seems deliberate.)

firebase-admin-node/src/utils/index.ts

Line 66 in 01a01a9

export function getProjectId(app: FirebaseApp): string {

So I think there's three possibilities:
a) This is a (legitimately) nullable variable.
b) utils.getProjectId() returns a nullable project id, which is correct in general, but we have reason to believe here in this specific case that it should be impossible/illegal. In which case, we can check the return value and throw if null.
c) utils.getProjectId() should never return null. In which case, we can change it to throw if all possibilities are exhausted.

I don't know which is correct though. Tracing through how it's called, it's clear that we're going to send some silly urls if it's not present, so (b) sounds like the most likely. However, I suppose if this is never actually used, then we could defer that error check?

Is it possible to use admin.auth without a valid projectId? If yes, then I suppose the projectId variables should be left as nullable until the cases where they're definitely required. If no, then we could move the check to the auth ctor and then drop the nullable-ness from these variables. (Which would be a behaviour change, but if it's actually required, then it's not a particularly meaningful behaviour change.)

There are some admin.auth() APIs that doesn't require project ID (custom token creation for example). Option (b) with deferred validation sounds like the right thing to do. We do a similar thing in FirebaseTokenVerifier. But that may be a larger refactor than we wish to do right now. I'm ok with keeping this as is, and changing it later.

sg; I've left it as is for now. I think AuthResourceUrlBuilder::getUrl() would need to change to check the projectId before using it. We'd also need to trace through to find other potential problems.

I've added a TODO to remind us.

src/auth/auth-api-request.ts

src/auth/auth.ts

src/auth/credential.ts

test/integration/auth.spec.ts

hiranya911 · 2019-11-08T23:36:05Z

test/unit/auth/auth.spec.ts

@@ -1236,7 +1242,7 @@ AUTH_CONFIGS.forEach((testConfig) => {
      });

      it('should be rejected given invalid properties', () => {
-        return auth.createUser(null)
+        return auth.createUser(null as unknown as CreateRequest)


Can we null as any?

We can. But this: null as CreateRequest generated the following error message:

error TS2352: Conversion of type 'null' to type 'CreateRequest' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

So I've assumed that's a typescript convention and used 'unknown'. But I have no reason other than that error message to prefer 'unknown' here. If you still prefer 'any', let me know, and I'll switch it.

I have a slight preference towards as any since it's less things to type. But this ok too.

Switched to as any.

test/unit/auth/auth.spec.ts

test/unit/auth/tenant-manager.spec.ts

test/unit/messaging/messaging.spec.ts

…trictNullChecks

hiranya911

Thanks @rsgowman. This looks great. Just pointed out a couple of things to consider. But I think we are almost there.

hiranya911 · 2019-12-05T18:28:49Z

test/unit/auth/auth.spec.ts

-              .should.eventually.be.rejected.and.have.property('code', 'auth/argument-error');
+            expect(() => {
+              (auth as any)[emailActionFlow.api](email, undefined);
+            }).to.throw(FirebaseAuthError).with.property('code', 'auth/argument-error');


This looks like a breaking change. Instead of rejecting the API now throws. We probably don't want that.

I wouldn't call it a breaking change; I'd call it a bug fix. :) If you violate the api contract, you expect it to fail. (See also https://g3doc.corp.google.com/firebase/jscore/g3doc/contributing/styleguide.md#public-apis-should-sanitize-their-inputs-aggressively which I'm definitely in favour of.)

But you're right that this is a behaviour change, and I'm trying to minimize those in this PR. I've reverted this for now, but I think this is worth discussing:
a) whether we should change this throughout (though obviously not during this PR)
b) if so, whether we consider it a breaking change

hiranya911 · 2019-12-05T18:43:09Z

src/utils/api-request.ts

@@ -459,6 +470,11 @@ class AsyncHttpCall {
    try {
      this.config = new HttpRequestConfigImpl(config);
      this.options = this.config.buildRequestOptions();
+      if (!this.options.headers) {


I see. The method buildEntity() also has code that modifies headers in-place. I think we can just do this:

this.entity = this.config.buildEntity(this.options.headers!);

If buildRequestOptions did not set headers, this will trigger a runtime error in buildEntity() when it attempts to modify headers. That sounds like an ok position for us to be in. WDYT?

hiranya911 · 2019-12-05T18:44:17Z

test/unit/auth/auth.spec.ts

@@ -1236,7 +1242,7 @@ AUTH_CONFIGS.forEach((testConfig) => {
      });

      it('should be rejected given invalid properties', () => {
-        return auth.createUser(null)
+        return auth.createUser(null as unknown as CreateRequest)


I have a slight preference towards as any since it's less things to type. But this ok too.

hiranya911

LGTM with a couple of nits.

hiranya911 · 2019-12-05T22:34:51Z

test/unit/auth/auth.spec.ts

@@ -1243,7 +1243,7 @@ AUTH_CONFIGS.forEach((testConfig) => {
      });

      it('should be rejected given invalid properties', () => {
-        return auth.createUser(null as unknown as CreateRequest)
+        return auth.createUser(null as any as CreateRequest)


null as any should be sufficient here right?

Yup. I've s/as any as/as any/ throughout this file (and the other files you've tagged.)

hiranya911 · 2019-12-05T22:35:15Z

test/unit/auth/tenant-manager.spec.ts

@@ -401,7 +401,7 @@ describe('TenantManager', () => {
    });

    it('should be rejected given invalid TenantOptions', () => {
-      return tenantManager.createTenant(null as unknown as TenantOptions)
+      return tenantManager.createTenant(null as any as TenantOptions)


Again null as any should be sufficient?

hiranya911 · 2019-12-05T22:39:03Z

test/unit/messaging/messaging.spec.ts

@@ -398,10 +398,10 @@ describe('Messaging', () => {
  describe('send()', () => {
    it('should throw given no message', () => {
      expect(() => {
-        messaging.send(undefined as unknown as Message);
+        messaging.send(undefined as any as Message);


I'd expect just undefined as any to be enough here.

…trictNullChecks

rsgowman self-assigned this Oct 18, 2019

rsgowman force-pushed the rsgowman/tsconfig_strictFunctionTypes branch from c7c9b14 to 2605cf1 Compare October 21, 2019 13:52

rsgowman force-pushed the rsgowman/tsconfig_strictNullChecks branch from db95eec to 9c009a4 Compare October 21, 2019 13:53

rsgowman force-pushed the rsgowman/tsconfig_strictFunctionTypes branch from 2605cf1 to d1e7e84 Compare October 21, 2019 18:52

rsgowman force-pushed the rsgowman/tsconfig_strictNullChecks branch 3 times, most recently from 28767f7 to 0daa443 Compare October 22, 2019 17:48

rsgowman changed the base branch from rsgowman/tsconfig_strictFunctionTypes to master October 22, 2019 17:49

rsgowman added 2 commits October 23, 2019 15:20

Add @firebase/auth-types to dev dependencies

ad16d69

And also update @firebase/auth to most recent to match.

rsgowman force-pushed the rsgowman/tsconfig_strictNullChecks branch from 0daa443 to ad16d69 Compare October 23, 2019 19:20

rsgowman changed the title ~~Enable 'structNullChecks' ts compiler option.~~ Enable 'strictNullChecks' ts compiler option. Oct 23, 2019

rsgowman requested a review from hiranya911 October 23, 2019 20:39

rsgowman assigned hiranya911 and unassigned rsgowman Oct 23, 2019

hiranya911 suggested changes Nov 8, 2019

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Nov 8, 2019

rsgowman added 3 commits November 25, 2019 12:26

review feedback

63a1eb6

Merge remote-tracking branch 'origin/master' into rsgowman/tsconfig_s…

8521b7c

…trictNullChecks

More fixes resulting from merge master

f3dcaa0

rsgowman assigned hiranya911 and unassigned rsgowman Nov 25, 2019

hiranya911 suggested changes Dec 5, 2019

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Dec 5, 2019

review feedback pt2

e0aea12

hiranya911 approved these changes Dec 5, 2019

View reviewed changes

rsgowman added 2 commits December 6, 2019 10:57

s/as any as/as any/ in a few of the test files

af48b2c

Merge remote-tracking branch 'origin/master' into rsgowman/tsconfig_s…

5987a32

…trictNullChecks

rsgowman merged commit 45905e4 into master Dec 6, 2019

rsgowman deleted the rsgowman/tsconfig_strictNullChecks branch December 6, 2019 16:53

lahirumaramba mentioned this pull request Jul 3, 2020

Add null to databaseAuthVariableOverride #926

Merged

Enable 'strictNullChecks' ts compiler option. #676

Enable 'strictNullChecks' ts compiler option. #676

Uh oh!

Conversation

rsgowman commented Oct 18, 2019

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!