Differentiate between hardAsserts and debugAsserts #2859

schmidt-sebastian · 2020-04-02T23:50:10Z

This PR aims to differentiate between asserts that we should handle in production builds (e.g. we received invalid data) and asserts we handle only during testing (our local state is corrupt). Some of the distinctions are really obvious, and other are anything but. In a lot of cases, we actually have multiple asserts for a single condition. I opted to only make the initial assertion a hardAssert (e.g. when we receive mutation results, we assert multiple times that the number of results matches the mutation count).

Asserts based on User Input are treated as soft asserts as we only use them after validating the input.

softAssert is a temporary name that I will rename back to assert once this is done. It is meant to make the decisions here more obvious.

This PR aims to differentiate between asserts that we should handle in production builds (e.g. we received invalid data) and asserts we handle only during testing (our local state is corrupt). Some of the distinctions are really obvious, and other are anything but. In a lot of cases, we actually have multiple asserts for a single condition. I opted to only make the initial assertion a hardAssert (e.g. when we reveive mutation results, we assert multiple times that the number of results matches the mutation count). softAssert is a temporary name that I will rename back to assert once this is done. It is meant to make the decisions here more obvious

google-oss-bot · 2020-04-03T21:54:30Z

Binary Size Report

Affected SDKs

SDK	Type	Base (`6a4c00f`)	Head (`2c99fae`)	Diff
@firebase/firestore/memory	browser	209948.00	209974.00	+26.00 (+0.01%)
	module	208351.00	208377.00	+26.00 (+0.01%)
	esm2017	167413.00	167439.00	+26.00 (+0.02%)
	main	373416.00	374458.00	+1042.00 (+0.28%)
@firebase/firestore	browser	266659.00	266685.00	+26.00 (+0.01%)
	module	264645.00	264671.00	+26.00 (+0.01%)
	esm2017	213291.00	213317.00	+26.00 (+0.01%)
	main	484875.00	486146.00	+1271.00 (+0.26%)
firebase	firebase.js	840339.00	840365.00	+26.00 (+0.00%)
	firebase-firestore.memory.js	253054.00	253080.00	+26.00 (+0.01%)
	firebase-firestore.js	308442.00	308468.00	+26.00 (+0.01%)

Metric Unit: byte

Test Logs

wilhuff · 2020-04-06T20:09:35Z

packages/firestore/src/core/query.ts

@@ -449,7 +455,7 @@ export class Query {
  }

  private assertValidBound(bound: Bound): void {
-    assert(
+    softAssert(


Have you confirmed elsewhere that these conditions are actually validated (i.e. that we reject user input that would have failed this assertion?

I have not, but I would assume that our customers would let us know if that was the case.

wilhuff · 2020-04-06T20:17:00Z

packages/firestore/src/local/indexeddb_schema.ts

@@ -72,7 +72,7 @@ export class SchemaConverter implements SimpleDbSchemaConverter {
    fromVersion: number,
    toVersion: number
  ): PersistencePromise<void> {
-    assert(
+    softAssert(


The fromVersion here comes from the IDBVersionChangeEvent so it's not entirely under our control. Keep this one a hardAssert?

wilhuff · 2020-04-06T20:28:16Z

packages/firestore/src/local/memory_mutation_queue.ts

@@ -302,7 +302,7 @@ export class MemoryMutationQueue implements MutationQueue {
    // Find the position of the first batch for removal. This need not be the
    // first entry in the queue.
    const batchIndex = this.indexOfExistingBatchId(batch.batchId, 'removed');
-    assert(
+    softAssert(


I'm wary of this one because we don't actually use the batch index to remove the value below.

I'm on the fence as to whether or not to recommend switching back to hardAssert here. The consequences seem dire (corrupting the mutation queue). On the other hand, this assertion really only mattered when we were transitioning away from held write acks.

Incidentally, we could remove the indexOfExistingBatchId call too if we had a facility to mark arbitrary code as strippable. Something like:

if (debugBuild) { const batchIndex = this.indexOfExistingBatchId(batch.batchId, 'removed'); softAssert(...); }

In this particular case, maybe just inline the call?

Also, the comment above is out of date ("This need not be the first entry in the queue" is no longer true).

I looked through the callsites. The batch ID here and in acknowledgeBadge is from the backend and no other validation is performed. As such, I made this a hardAssert and changed acknowledgeBatch to a hardAssert as well.

wilhuff · 2020-04-06T20:34:24Z

packages/firestore/src/remote/datastore.ts

@@ -114,7 +114,7 @@ export class Datastore {
      const result: MaybeDocument[] = [];
      keys.forEach(key => {
        const doc = docs.get(key);
-        assert(!!doc, 'Missing entity in write response for ' + key);
+        softAssert(!!doc, 'Missing entity in write response for ' + key);


This seems like a candidate for keeping as a hardAssert because the docs come from the server response.

wilhuff · 2020-04-06T20:39:55Z

packages/firestore/src/util/assert.ts

 }

 /**
 * Fails if the given assertion condition is false, throwing an Error with the
 * given message if it did.
+ *
+ * This is stripped out in production builds.


Maybe clarify that the source is removed? Of particular importance is that any side effects of any code inline in the argument is removed--it's not just that the condition is ignored. Suggestion:

The code of callsites invoking this function are stripped out in production builds. Any side-effects of code within the softAssert invocation will not happen in this case.

Done. Thanks for the suggestion.

wilhuff · 2020-04-06T20:42:24Z

packages/firestore/src/util/assert.ts

-  if (!assertion) {
-    fail(message);
-  }
+export function softAssert(


While "soft" vs "hard" is a kind of natural pairing, softAssert doesn't really suggest that it's going to be stripped out. What do you think of debugAssert (and leave hardAssert alone)? FWIW The Firebase C++ SDK calls these "dev assertions".

I wanted to rename these back to "assert" before merging, but debugAssert is more meaningful. I will rename in a separate commit on this PR.

wilhuff · 2020-04-06T20:46:12Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -53,7 +53,7 @@ import {
  WatchTargetChange,
  WatchTargetChangeState
 } from '../../../src/remote/watch_change';
-import { assert } from '../../../src/util/assert';
+import { softAssert } from '../../../src/util/assert';


It seems to me like tests should always use hardAssert so that the condition is always evaluated, even when running against a stripped build.

This is obviously unimportant in unit tests, but it seems like it could have bearing on integration tests, and it seems like being able to make a strong blanket statement like "tests should use hardAssert seems like it could help keep things simple.

WDYT?

The tests themselves are actually never minified. We run the full tests against the minified build, so even in this mode the "softAsserts" are kept. I think the new name "debugAsserts" conveys that they are run in tests better than "softAsserts" and almost as well as "hardAsserts", so I kept it as for now. I can go either way here...

FWIW, rather than changing these to hardAsserts, we might even want to use failed test expectations.

Makes sense.

Changing to failed test expectations would be fine too, but seems orthogonal to the issue at hand and seems like it could be deferred.

wilhuff · 2020-04-07T22:05:03Z

packages/firestore/test/unit/local/local_store.test.ts

@@ -53,7 +53,7 @@ import {
  WatchTargetChange,
  WatchTargetChangeState
 } from '../../../src/remote/watch_change';
-import { assert } from '../../../src/util/assert';
+import { softAssert } from '../../../src/util/assert';


Makes sense.

Changing to failed test expectations would be fine too, but seems orthogonal to the issue at hand and seems like it could be deferred.

wilhuff · 2020-04-07T22:27:33Z

packages/firestore/src/util/assert.ts

 /**
 * Unconditionally fails, throwing an Error with the given message.
 *
+ * The code of callsites invoking this function are stripped out in production


Apologies--I totally missed that you're stripping out calls to fail() as well. There are a few cases where data outside our control results in a failure that would be unhandled. File and line numbers taken from 761d0d3.

in database.ts:676 (Transattion.get): if (!docs || docs.length !== 1) -- this seems like it could straightforwardly be rewritten as a hardAssert

in user_data_reader:356 (parseMergeData): it seems like this may be validating user input? Possibly this should be converted to a validation error (or possibly this was validated elsewhere)

in query.ts:494 (Operator.fromString): the default case seems like it might be validating user input. Oh wait, this is validated at database.ts:1459. This one's OK, I think.

in encoded_resource_path.ts:142, 170 (decodeResourcePath): this is decoding a resource path from IndexedDB

in indexeddb_mutation_queue.ts:348 (getAllMutationBatchesAffectingDocumentKey): dangling document-references could reflect corruption in IndexedDB

Same on line 480 (lookupMutationBatches)

local_serializer.ts:67: (fromDbRemoteDocument): borderline case, but your fromFoo in serializer.ts seemed mostly to go toward hardAssert so I thought I'd flag

values.ts:380: (canonifyValue): This is how we process input from the network, so it's possible to get unexpected values here. The other fail calls in this file are all default cases for TypeOrder so I think they're fine.

webchannel_connection.ts:170: RPC error code has a remote source

Actually I'm going to stop--there seem like more and we should at least discuss this. A hardFail/debugFail split would definitely address this (or just leaving all fail calls in too, though that cuts into size savings).

Many of these seem like they could be reformulated in terms of the error handling that's in place, but that seems out of scope for this change.

I reverted back the documentation change for fail. Let's tackle this one API at a time. If we want to keep fail() but reduce the size impact we can remove the error message, but first I would like to look at the actual size impact of the messages.

So, for now, this PR differentiates between the different assertions, and then I will update the Transformer PR to not stip fail().

wilhuff

LGTM

Please update description (softAsserts => debugAsserts).

schmidt-sebastian added 2 commits April 3, 2020 14:45

[AUTOMATED]: Prettier Code Styling

79ad572

schmidt-sebastian force-pushed the mrschmidt/hardasserts branch from cff106e to accd742 Compare April 3, 2020 21:46

schmidt-sebastian requested a review from wilhuff April 3, 2020 21:47

schmidt-sebastian assigned wilhuff Apr 3, 2020

schmidt-sebastian added the api: firestore label Apr 3, 2020

wilhuff reviewed Apr 6, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 6, 2020

schmidt-sebastian added 2 commits April 6, 2020 22:29

Harden some asserts

b61626e

Rename

157fa3b

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 7, 2020

wilhuff reviewed Apr 7, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 7, 2020

schmidt-sebastian added 4 commits April 7, 2020 16:06

First Merge

8b8d631

Don't strip fail()

1406717

Merge

079509d

Cleanup

84b1354

schmidt-sebastian assigned wilhuff and unassigned schmidt-sebastian Apr 7, 2020

wilhuff approved these changes Apr 8, 2020

View reviewed changes

wilhuff assigned schmidt-sebastian and unassigned wilhuff Apr 8, 2020

schmidt-sebastian changed the title ~~softAsserts/hardAsserts~~ Differentiate between hardAsserts and debugAsserts Apr 8, 2020

schmidt-sebastian merged commit a789d74 into master Apr 8, 2020

schmidt-sebastian deleted the mrschmidt/hardasserts branch April 8, 2020 00:28

Feiyang1 mentioned this pull request Apr 9, 2020

Add core types & error map to auth-exp #2890

Merged

firebase locked and limited conversation to collaborators May 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Differentiate between hardAsserts and debugAsserts #2859

Differentiate between hardAsserts and debugAsserts #2859

schmidt-sebastian commented Apr 2, 2020 •

edited

Loading

google-oss-bot commented Apr 3, 2020 •

edited

Loading

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020 •

edited

Loading

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 6, 2020

schmidt-sebastian Apr 7, 2020

wilhuff Apr 7, 2020

wilhuff Apr 7, 2020

wilhuff Apr 7, 2020

schmidt-sebastian Apr 7, 2020

wilhuff left a comment •

edited

Loading

Differentiate between hardAsserts and debugAsserts #2859

Differentiate between hardAsserts and debugAsserts #2859

Conversation

schmidt-sebastian commented Apr 2, 2020 • edited Loading

google-oss-bot commented Apr 3, 2020 • edited Loading

Binary Size Report

Affected SDKs

Test Logs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmidt-sebastian Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff left a comment • edited Loading

Choose a reason for hiding this comment

schmidt-sebastian commented Apr 2, 2020 •

edited

Loading

google-oss-bot commented Apr 3, 2020 •

edited

Loading

schmidt-sebastian Apr 7, 2020 •

edited

Loading

wilhuff left a comment •

edited

Loading