Add NOT_IN and != queries (not public) #3557

thebrianchen · 2020-08-04T16:48:49Z

No description provided.

changeset-bot · 2020-08-04T16:48:51Z

💥 No Changeset

Latest commit: 8fef964

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

google-oss-bot · 2020-08-04T17:01:22Z

Binary Size Report

Affected SDKs

@firebase/analytics

Type Base (2fa0353) Head (19793f9) Diff

esm2017 9.66 kB 9.89 kB +229 B (+2.4%)

main 10.9 kB 11.1 kB +225 B (+2.1%)

module 10.6 kB 10.8 kB +234 B (+2.2%)

`@firebase/firestore`

Type	Base (`2fa0353`)	Head (`19793f9`)	Diff
browser	247 kB	249 kB	+1.78 kB (+0.7%)
esm2017	193 kB	195 kB	+1.52 kB (+0.8%)
main	472 kB	474 kB	+2.38 kB (+0.5%)
module	244 kB	246 kB	+1.73 kB (+0.7%)
react-native	193 kB	195 kB	+1.51 kB (+0.8%)

`@firebase/firestore/exp`

Type	Base (`2fa0353`)	Head (`19793f9`)	Diff
browser	187 kB	188 kB	+1.48 kB (+0.8%)
main	464 kB	467 kB	+2.32 kB (+0.5%)
module	187 kB	188 kB	+1.48 kB (+0.8%)
react-native	187 kB	188 kB	+1.46 kB (+0.8%)

`@firebase/firestore/lite`

Type	Base (`2fa0353`)	Head (`19793f9`)	Diff
browser	67.9 kB	64.3 kB	-3.56 kB (-5.2%)
main	144 kB	142 kB	-1.37 kB (-1.0%)
module	67.9 kB	64.3 kB	-3.56 kB (-5.2%)
react-native	67.9 kB	64.4 kB	-3.51 kB (-5.2%)

`@firebase/firestore/memory`

Type	Base (`2fa0353`)	Head (`19793f9`)	Diff
browser	185 kB	187 kB	+1.51 kB (+0.8%)
esm2017	145 kB	146 kB	+1.27 kB (+0.9%)
main	347 kB	349 kB	+2.02 kB (+0.6%)
module	183 kB	185 kB	+1.50 kB (+0.8%)
react-native	145 kB	146 kB	+1.26 kB (+0.9%)

`firebase`

Type	Base (`2fa0353`)	Head (`19793f9`)	Diff
firebase-analytics.js	28.0 kB	28.3 kB	+318 B (+1.1%)
firebase-firestore.js	286 kB	287 kB	+1.66 kB (+0.6%)
firebase-firestore.memory.js	226 kB	227 kB	+1.42 kB (+0.6%)
firebase.js	819 kB	821 kB	+1.86 kB (+0.2%)

Test Logs

Base (2fa0353d): https://github.com/firebase/firebase-js-sdk/actions/runs/195338703
Head (19793f9d): https://github.com/firebase/firebase-js-sdk/actions/runs/196650082

wilhuff · 2020-08-04T21:46:45Z

packages/firestore/src/api/database.ts

@@ -1684,14 +1688,17 @@ function validateDisjunctiveFilterElements(
        'maximum of 10 elements in the value array.'
    );
  }
-  if (value.indexOf(null) >= 0) {
+  if (value.indexOf(null) >= 0 && operator !== Operator.NOT_IN) {


Avoid double negatives if you can. In this case, the only valid operators are IN and NOT_IN, so you could make this check === Operator.IN and be clearer without loss of functionality.

Also, if you put the operator check first then we won't have to search the array for null in cases where that doesn't apply.

Same feedback below, though also consider factoring out the common if check:

if (operator === Operator.IN) { if (contains null) { ... } if (contains NaN) { ... } }

Refactored. However, it's possible for the operator to be ARRAY_CONTAINS_ANY inside newQueryFilter(), so I added that as well.

wilhuff · 2020-08-04T21:50:09Z

packages/firestore/src/api/database.ts

@@ -1700,14 +1707,41 @@ function validateDisjunctiveFilterElements(
  }
 }

+function conflictingOps(op: Operator): Operator[] {


Comments please. In particular please document the context in which these operations could be conflicting.

Within the body of this function it would be useful to spell out the rules by which you arrived at these results.

These comments are mostly just repeating what the code is doing. What I was hoping for was a higher-level comment (outside the switch statement) that gave the collective rules together. For example, as you've implemented this now, you've repeated "Only one ARRAY operator can be used" in multiple places and I'm prompting for it to be in one place.

Basically: when you constructed these lists, you had to evaluate a list of rules. Then for each operator pair you evaluated those rules to determine if they were incompatible. What I want you to document is the list of rules you used to build this structure.

Makes sense. Documented at the top of the function.

wilhuff · 2020-08-04T21:55:32Z

packages/firestore/src/api/database.ts

+
+  const conflictingOp = query.findFilterOperator(conflictingOps(filter.op));
+  if (conflictingOp !== null) {
+    // We special case when it's a duplicate op to give a slightly clearer error message.


When commenting in code like this, you can drop the "we" and write as if you're telling the computer what to do (i.e. in the imperative mood with an implicit subject).

Special case when ...

(Though I see you're just moving the comment from below.)

wilhuff · 2020-08-04T22:06:04Z

packages/firestore/src/core/query.ts

+    op: Operator,
+    value: api.Value
+  ): FieldFilter {
+    debugAssert(


You can avoid this assertion by making this a part of the type of the parameter. I believe you can declare op like this:

op: Operator.IN | Operator.NOT_IN

and then the caller will have to be in a context where this is true.

Much cleaner, thanks!

wilhuff · 2020-08-04T22:16:01Z

packages/firestore/src/core/query.ts

+  }
+
+  matches(doc: Document): boolean {
+    return !this.keys.some(key => key.isEqual(doc.key));


The difference between these types is the operator name and this exclamation point. It seems like you could either:

extract a common class for this that takes the operator as a parameter;

extract a helper function for creating the keys array;

make KeyFiieldInFilter the base class and have it take the operator as a parameter;

move the construction of the keys array into the caller (passing it as an extra argument) since you now have a dedicated createKeyFieldInFilter.

(listed in order of my decreasing preference, but my preference isn't strong.)

Just duplicating the code seems size unfriendly.

Extracted into helper.

wilhuff · 2020-08-04T22:21:50Z

packages/firestore/test/integration/api/validation.test.ts

+          collection.where('y', '>', 32).where('x', notEqualOp, 33)
+        ).to.throw(
+          'Invalid query. All where filters with an ' +
+            'inequality (<, <=, >, or >=) must be on the same field.' +


Should != be in this list of inequalities?

I included a TODO in the validation check to update the message once != queries are public. That way developers won't see it until the feature is released.

wilhuff · 2020-08-04T22:22:54Z

packages/firestore/test/integration/api/validation.test.ts

+      );
+    });
+
+    validationIt(persistence, 'with != and NOT_IN filters fail', db => {


Since the user-visible spelling for NOT_IN is not-in, maybe describe this in terms of "not-in" rather than the proto name for this operator?

wilhuff · 2020-08-04T22:23:23Z

packages/firestore/test/integration/api/validation.test.ts

+          .where('foo', notInOp, [2, 3])
+          .where('foo', notEqualOp, 4)
+      ).to.throw(
+        "Invalid query. You cannot use '!=' filters with " + "'not-in' filters."


What's with the extra plus sign in here? There's a couple in this file. Search for " + ".

Removed. I think it may have been due to line wrapping, but it's no longer an issue now.

wilhuff · 2020-08-04T22:25:08Z

packages/firestore/test/integration/util/helpers.ts

+
+// TODO(ne-queries): This exists just so we don't have to do the cast
+// repeatedly. Once we expose '!=' publicly we can remove it and
+// just use 'array-contains-any' in all the tests.


Typo? Surely you don't mean "array-contains-any" here (or "in" in the next one).

Fixed. sloppy copy/paste

wilhuff · 2020-08-04T22:26:10Z

packages/firestore/test/unit/core/query.test.ts

@@ -218,6 +218,64 @@ describe('Query', () => {
    expect(queryMatches(query1, document)).to.be.true;
  });

+  it('matches NOT_IN filters', () => {


Same comment re NOT_IN vs not-in. NOT_IN is a proto representation detail while not-in is the public API value.

wilhuff

LGTM

thebrianchen added the api: firestore label Aug 4, 2020

thebrianchen self-assigned this Aug 4, 2020

add != and NOT_IN queries

df56eb2

thebrianchen force-pushed the bc/ne branch from 0fc8ac9 to df56eb2 Compare August 4, 2020 19:12

small cleanup

b2346f3

thebrianchen requested a review from wilhuff August 4, 2020 21:23

thebrianchen assigned wilhuff and unassigned thebrianchen Aug 4, 2020

wilhuff suggested changes Aug 4, 2020

View reviewed changes

resolve comments

d914433

thebrianchen requested a review from wilhuff August 5, 2020 14:25

wilhuff assigned thebrianchen and unassigned wilhuff Aug 5, 2020

clean conflictingOps comments

8fef964

thebrianchen assigned wilhuff and unassigned thebrianchen Aug 5, 2020

wilhuff approved these changes Aug 5, 2020

View reviewed changes

wilhuff assigned thebrianchen and unassigned wilhuff Aug 5, 2020

thebrianchen merged commit cf3401d into master Aug 5, 2020

thebrianchen mentioned this pull request Aug 10, 2020

Add != and NOT_IN queries (not public) firebase/firebase-android-sdk#1872

Merged

firebase locked and limited conversation to collaborators Sep 5, 2020

Add NOT_IN and != queries (not public) #3557

Add NOT_IN and != queries (not public) #3557

Uh oh!

Conversation

thebrianchen commented Aug 4, 2020

Uh oh!

changeset-bot bot commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💥 No Changeset

Uh oh!

google-oss-bot commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

@firebase/analytics

@firebase/firestore

@firebase/firestore/exp

@firebase/firestore/lite

@firebase/firestore/memory

firebase

Test Logs

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!

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!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

changeset-bot bot commented Aug 4, 2020 •

edited

Loading

google-oss-bot commented Aug 4, 2020 •

edited

Loading

`@firebase/analytics`

`@firebase/firestore`

`@firebase/firestore/exp`

`@firebase/firestore/lite`

`@firebase/firestore/memory`

`firebase`