Fix array-contains queries #1913

var-const · 2019-06-24T23:11:36Z

#1894 introduced a regression that made persisted queries deserialize incorrectly. Fix the regression and add tests.

The root cause is that the refactoring introduced a type hierarchy of classes derived from FieldFilter, but serializer always deserializes persisted filters to the base class FieldFilter, never to the derived types. This would have affected array-contains queries, in queries, and key field queries.

Fixes #1904.

var-const · 2019-06-24T23:12:55Z

packages/firestore/test/unit/remote/node/serializer.test.ts

@@ -25,6 +25,9 @@ import { GeoPoint } from '../../../../src/api/geo_point';
 import { Timestamp } from '../../../../src/api/timestamp';


I couldn't figure out how to test KeyFieldFilter due to problems with creating a query with this filter.

Not sure if this is what you mean, but it might look something like:

const input = filter(DOCUMENT_KEY_NAME, '==', ref('project/database', 'coll/doc'));

Thanks! That worked.

mikelehen

Looks good with a bit of suggested improvement to make the FieldFilter constructor protected to prevent this mistake going forward. But we could do that later, in a separate PR if you prefer.

mikelehen · 2019-06-24T23:39:23Z

packages/firestore/src/remote/serializer.ts

@@ -1282,7 +1282,7 @@ export class JsonProtoSerializer {
  }

  fromFieldFilter(filter: api.Filter): Filter {
-    return new FieldFilter(
+    return Filter.create(


There are two other instances of new FieldFilter() in this file that I think should optimally be FieldFilter.create() as well (though in practice they're not harmful).

To make sure we don't mess this up going forward, I think it might make sense to move Filter.create() to be FieldFilter.create() so we can make the FieldFilter constructor protected. This is probably more correct anyway, since the only kind of filter that Filter.create() could conceivably create with its current arguments is FieldFilter.

I agree, and I like this suggestion. Done, PTAL.

mikelehen · 2019-06-24T23:41:25Z

packages/firestore/test/unit/remote/node/serializer.test.ts

-      expect(s.fromFieldFilter(actual)).to.deep.equal(input);
+      const roundtripped = s.fromFieldFilter(actual);
+      expect(roundtripped).to.deep.equal(input);
+      expect(roundtripped).to.be.instanceof(FieldFilter);


FWIW we could instead do:

expect(Object.getPrototypeOf(roundtripped)).to.equal(Object.getPrototypeOf(input));

to avoid the redundancy of specifying the type of input... but in practice it doesn't matter and it's easier to read as-is... so probably not a good improvement (unless we wanted to refactor these tests to share code or something).

I don't know how getPrototypeOf works -- if it checks for the exact class, then it could be an improvement, because given, say, ArrayContainsFilter, it will nevertheless satisfy a check for instanceof FieldFilter (due to inheritance).

Ah, yes. I believe the prototype check would be more strict than that... that said it's not super normal to deal with an object's prototype directly or do equal checks on it. I am not 100% confident it wouldn't bite us in the future some day (due to browser differences or something). So I don't feel strongly that it's a better approach or anything (i.e. feel free to leave as-is).

mikelehen · 2019-06-25T00:16:12Z

commits.txt

+fd5a31a6bba01b79f9fd269c96926c8aa6248701 Generate a new token after getId calls if there is no valid token (#1869)
+99e6cc8267d4eea4b2893a45d097c6cb20eddd15 Add support for running Firestore integration tests against the emulator (#1851)
+3f2c7bf8a08978bea065ac1706af016e8646f234 Check that window is complete in BrowserConnectivityMonitor (#1870)
+73f10767a8473468aa6fb6300abe93f998b59ecf Minor style changes for a RTDB comment block (#1857)


🤔

Think you accidentally added some extra files...

I think it's the prettier script striking again. Removed now.

mikelehen

Looks good. Thanks!

mikelehen · 2019-06-25T00:26:39Z

packages/firestore/test/unit/remote/node/serializer.test.ts

+      expect(roundtripped).to.be.instanceof(FieldFilter);
+    });
+
+    it('converts key field', () => {


Thanks for adding!

* Rename relation_filter.{h,cc} to field_filter.{h,cc} * Rename RelationFilter to FieldFilter * Move Filter::Create to FieldFilter * Port api::Query::ValidateNewFilter * Add ArrayContainsFilter * Add KeyFieldFilter * Make FieldFilter constructor protected * Port FieldFilter refactor from the JS SDK firebase/firebase-js-sdk#1894 * Add spec test from firebase/firebase-js-sdk#1913 * Accept array-contains as a synonym for array_contains

var-const added 2 commits June 24, 2019 19:07

Fix array-contains

81f3b29

[AUTOMATED]: Prettier Code Styling

e2ac222

var-const requested a review from wilhuff June 24, 2019 23:11

var-const assigned wilhuff and mikelehen and unassigned wilhuff Jun 24, 2019

var-const requested review from mikelehen and removed request for wilhuff June 24, 2019 23:11

var-const added the api: firestore label Jun 24, 2019

var-const commented Jun 24, 2019

View reviewed changes

mikelehen approved these changes Jun 24, 2019

View reviewed changes

mikelehen assigned var-const and unassigned mikelehen Jun 24, 2019

var-const added 2 commits June 24, 2019 20:13

Review feedback

f415822

[AUTOMATED]: Prettier Code Styling

a84e4de

var-const requested review from Feiyang1, hiranya911 and hsubox76 as code owners June 25, 2019 00:14

mikelehen reviewed Jun 25, 2019

View reviewed changes

Remove accidental

ec32b9d

var-const assigned mikelehen and unassigned var-const Jun 25, 2019

mikelehen approved these changes Jun 25, 2019

View reviewed changes

Appease linter

af30584

mikelehen assigned var-const and unassigned mikelehen Jun 25, 2019

Appease linter

4d3f1c0

var-const merged commit 9bb7e70 into master Jun 25, 2019

var-const deleted the varconst/fix-array-contains branch June 25, 2019 03:03

var-const mentioned this pull request Jun 25, 2019

Firestore 1.4.2 in Firebase 6.2.2: array-contains query returns empty array with offline persistence #1904

Closed

thebrianchen mentioned this pull request Jun 26, 2019

Refactor Filters firebase/firebase-android-sdk#570

Merged

wilhuff mentioned this pull request Jul 15, 2019

Refactor Filters firebase/firebase-ios-sdk#3359

Merged

wilhuff added a commit to firebase/firebase-ios-sdk that referenced this pull request Jul 20, 2019

Add spec test from firebase/firebase-js-sdk#1913

477513e

firebase locked and limited conversation to collaborators Oct 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix array-contains queries #1913

Fix array-contains queries #1913

Uh oh!

var-const commented Jun 24, 2019

Uh oh!

var-const Jun 24, 2019

Uh oh!

mikelehen Jun 24, 2019 •

edited

Loading

Uh oh!

var-const Jun 25, 2019

Uh oh!

mikelehen left a comment

Uh oh!

mikelehen Jun 24, 2019

Uh oh!

var-const Jun 25, 2019

Uh oh!

mikelehen Jun 24, 2019

Uh oh!

var-const Jun 25, 2019

Uh oh!

mikelehen Jun 25, 2019

Uh oh!

mikelehen Jun 25, 2019

Uh oh!

var-const Jun 25, 2019

Uh oh!

mikelehen left a comment

Uh oh!

mikelehen Jun 25, 2019

Uh oh!

Uh oh!

		@@ -25,6 +25,9 @@ import { GeoPoint } from '../../../../src/api/geo_point';
		import { Timestamp } from '../../../../src/api/timestamp';

Fix array-contains queries #1913

Fix array-contains queries #1913

Uh oh!

Conversation

var-const commented Jun 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen Jun 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikelehen 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!

mikelehen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikelehen Jun 24, 2019 •

edited

Loading