Implement NSPredicate-based query #531

zxu123 · 2017-12-05T16:07:25Z

Discussion

API change proposal, http://go/firestore-nspredicate-based-query

Testing

New test is added to cover the new function and all tests pass.

API Changes

API change proposal, http://go/firestore-nspredicate-based-query

… vars. * add queryByRemovingFilters() in FSTQuery and test case; * add property usePredicate in FIRQuery and update old APIs queryWhereField/Path; * add a no-op queryFilteredUsingPredicate() skeleton in FIRQuery. All tests pass.

* implement queryFilteredUsingPredicate in FIRQuery; * add new unit test skeleton FIRQueryTests.m; * reverts last change on adding usePredicate as we are now allowing mix usage of two types of query filtering APIs.

* add unit test and integration test; * revert a few irrelevant line-change in last commit in project.pbxproj.

rename queryFilteredUsingPredicate to queryWherePredicate

raise assertion in the particular case of NSBlockPredicate.

wilhuff

This change is definitely on the right track.

wilhuff · 2017-12-05T21:14:49Z

Firestore/Example/Tests/FIRQueryTests.m

@@ -0,0 +1,72 @@
+/*


Please put this file in the location that parallels the implementation: under API as a peer of FIRGeoPointTests.m.

wilhuff · 2017-12-05T21:22:04Z

Firestore/Example/Tests/FIRQueryTests.m

+ * limitations under the License.
+ */
+
+#import "FirebaseFirestore/FIRFirestore.h"


For the public surface area of Firestore, please just @import the module, like so:

@import FirebaseFirestore;

That gets both the FIRFirestore.h and FIRQuery.h headers.

wilhuff · 2017-12-05T21:25:40Z

Firestore/Source/API/FIRQuery.m

+    id value = nil;
+    if (comparison.leftExpression.keyPath == nil &&
+        comparison.rightExpression.keyPath == nil) {
+      FSTThrowInvalidArgument(@"Invalid query. Predicate comparison must have "


This error message is more specific than it needs to be and could be simplified. Maybe:

Predicate comparisons must include a key path.

wilhuff · 2017-12-05T21:27:22Z

Firestore/Source/API/FIRQuery.m

+    }
+    NSString *path;
+    id value = nil;
+    if (comparison.leftExpression.keyPath == nil &&


This logic doesn't handle the case where both sides are keyPaths. Is that possible?

Refactored the logic, now only keypath = constant and constant = keypath are allowed comparison through type check.

wilhuff · 2017-12-05T21:29:08Z

Firestore/Source/API/FIRQuery.m

+        comparison.rightExpression.keyPath == nil) {
+      FSTThrowInvalidArgument(@"Invalid query. Predicate comparison must have "
+                              "a key path on either side of its expression.");
+    } else if (comparison.leftExpression.keyPath == nil) {


nit: This could be clearer if you tested that the key path was non-nil then used it in the immediately following block.

As it stands you're comparing against leftExpression.keyPath then using the rightExpression.keyPath right below.

done, refactored

wilhuff · 2017-12-05T21:36:47Z

Firestore/Source/API/FIRQuery.m

+                ^BOOL(id obj, NSDictionary *bindings) { return true; }] class]]) {
+    FSTThrowInvalidArgument(@"Invalid query. Block-based predicates are not "
+                            "supported. Please use predicateWithFormat to "
+                            "construct predicate instead.");


"Construct" is a C++ and Java-ism that's not native to Objective-C (where objects are created or initialized). Also "predicate" should be pluralized.

Please use predicateWithFormat to create predicates instead.

Here and below.

wilhuff · 2017-12-05T21:38:37Z

Firestore/Source/Public/FIRQuery.h

+ * Creates and returns a new `FIRQuery` with the additional filter that documents must
+ * satisfy the specified predicate.
+ *
+ * @param predicate The predicate the document must satisfy. Can be either comparison


This should specifically mention that block-based predicates are not supported

wilhuff · 2017-12-05T21:41:26Z

Firestore/Source/API/FIRQuery.m

                                 value:value];
 }

+- (FIRQuery *)queryWherePredicate:(NSPredicate *)predicate {


This method is very large. Consider breaking it down into a few smaller pieces.

For example, you could make queryWherePredicate: do the instanceof checks and then delegate to strongly-typed single-purpose methods like queryWhereComparisonPredicate: and queryWhereCompoundPredicate.

wilhuff · 2017-12-05T21:45:39Z

Firestore/Source/API/FIRQuery.m

+                                "not supported.");
+        break;
+      default:
+        FSTThrowInvalidArgument(@"Invalid query. Predicate does not support "


This "Predicate does not support" phrasing isn't quite right. NSPredicate does support these things and this phrasing seems to suggest that the predicate itself doesn't support these.

The other phrasing just above is much better: "Custom predicate filters are not supported" is a better example.

Maybe this one could be

Operator type %lu is not supported.

wilhuff · 2017-12-05T21:47:01Z

Firestore/Source/API/FIRQuery.m

+    NSCompoundPredicate *compound = (NSCompoundPredicate *)predicate;
+    if (compound.compoundPredicateType != NSAndPredicateType ||
+        compound.subpredicates.count == 0) {
+      FSTThrowInvalidArgument(@"Invalid query. Predicate only supports "


Similar to above, this would be better phrased as:

Only compound queries using AND are supported.

wilhuff · 2017-12-05T21:50:32Z

Also, please add an entry to Firestore/CHANGELOG.md describing what was done. Something like:

[feature] Queries can now be created from an NSPredicate.

revert name queryWherePredicate() back to queryFilteredUsingPredicate().

* improve docs / assertion message; * fix a bug where transposed comparison is not treated correctly; also add unit test to cover that case.

project change of adding the FIRQueryTests.m file

paulb777 · 2017-12-06T14:55:13Z

Firestore/Example/Tests/Integration/API/FIRQueryTests.m

  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"b", @"a" ]));
 }

+- (void)testQueryWithPredicate {


Please fix the trailing space travis failure -

$ ! git grep -I ' $'
Firestore/Example/Tests/Integration/API/FIRQueryTests.m:
The command "! git grep -I ' $'" exited with 1.

* refactoring to split logic into two helpers; * update change log.

zxu123

Updated. PTAL.

zxu123 · 2017-12-05T22:37:49Z

Firestore/Example/Tests/FIRQueryTests.m

@@ -0,0 +1,72 @@
+/*


zxu123 · 2017-12-05T22:38:35Z

Firestore/Source/Public/FIRQuery.h

+ * Creates and returns a new `FIRQuery` with the additional filter that documents must
+ * satisfy the specified predicate.
+ *
+ * @param predicate The predicate the document must satisfy. Can be either comparison


added in @param part

zxu123 · 2017-12-05T22:41:54Z

Firestore/Example/Tests/FIRQueryTests.m

+ * limitations under the License.
+ */
+
+#import "FirebaseFirestore/FIRFirestore.h"


zxu123 · 2017-12-05T22:43:21Z

Firestore/Source/API/FIRQuery.m

+    }
+    NSString *path;
+    id value = nil;
+    if (comparison.leftExpression.keyPath == nil &&


Refactored the logic, now only keypath = constant and constant = keypath are allowed comparison through type check.

zxu123 · 2017-12-05T22:43:29Z

Firestore/Source/API/FIRQuery.m

+    id value = nil;
+    if (comparison.leftExpression.keyPath == nil &&
+        comparison.rightExpression.keyPath == nil) {
+      FSTThrowInvalidArgument(@"Invalid query. Predicate comparison must have "


zxu123 · 2017-12-05T22:44:38Z

Firestore/Source/API/FIRQuery.m

+                                "not supported.");
+        break;
+      default:
+        FSTThrowInvalidArgument(@"Invalid query. Predicate does not support "


zxu123 · 2017-12-05T22:44:43Z

Firestore/Source/API/FIRQuery.m

+    NSCompoundPredicate *compound = (NSCompoundPredicate *)predicate;
+    if (compound.compoundPredicateType != NSAndPredicateType ||
+        compound.subpredicates.count == 0) {
+      FSTThrowInvalidArgument(@"Invalid query. Predicate only supports "


zxu123 · 2017-12-05T22:44:50Z

Firestore/Source/API/FIRQuery.m

+                ^BOOL(id obj, NSDictionary *bindings) { return true; }] class]]) {
+    FSTThrowInvalidArgument(@"Invalid query. Block-based predicates are not "
+                            "supported. Please use predicateWithFormat to "
+                            "construct predicate instead.");


zxu123 · 2017-12-06T22:12:40Z

Firestore/Example/Tests/Integration/API/FIRQueryTests.m

  XCTAssertEqualObjects(FIRQuerySnapshotGetIDs(snapshot), (@[ @"b", @"a" ]));
 }

+- (void)testQueryWithPredicate {


zxu123 · 2017-12-06T22:12:52Z

Firestore/Source/API/FIRQuery.m

                                 value:value];
 }

+- (FIRQuery *)queryWherePredicate:(NSPredicate *)predicate {


remove trailing space

wilhuff

Pretty much LGTM except one (minor) question.

Note that I've changed this to target the firestore-api-changes branch so that we can release all of these API changes at once.

wilhuff · 2017-12-07T00:02:24Z

Firestore/Source/Public/FIRQuery.h

+ * @return The created `FIRQuery`.
+ */
+// clang-format off
+- (FIRQuery *)queryFilteredUsingPredicate:(NSPredicate *)predicate NS_SWIFT_NAME(filter(using:));


Shouldn't this NS_SWIFT_NAME be filtered(using:)? Mentioned this in the API proposal.

Yes, changed.

I'm really sorry to have pointed this out because I was totally wrong :-(.

@mikelehen points out that the other swift names are things like orderBy rather than orderedBy so this really should be filter(using:).

Again I'm super sorry to have caused you extra work like this and I've reverted that change here.

no problem, fellow. I like discussions, which is the only way great idea comes from.

zxu123 · 2017-12-07T02:46:20Z

please advise when it is OK to merge.

swift name filter -> filtered

This reverts commit 5585202.

wilhuff

LGTM to squash and merge into firestore-api-changes.

* implement queryFilteredUsingPredicate in FIRQuery; * add unit test and integration test for queryFilteredUsingPredicate; * project change of adding the FIRQueryTests.m file; * refactoring queryFilteredUsingPredicate to split logic into two helpers;

zxu123 added 3 commits December 4, 2017 11:21

implement queryFilteredUsingPredicate

bd5c0fb

* implement queryFilteredUsingPredicate in FIRQuery; * add new unit test skeleton FIRQueryTests.m; * reverts last change on adding usePredicate as we are now allowing mix usage of two types of query filtering APIs.

add unit test and integration test for queryFilteredUsingPredicate

a69c362

* add unit test and integration test; * revert a few irrelevant line-change in last commit in project.pbxproj.

zxu123 requested a review from wilhuff December 5, 2017 16:07

googlebot added the cla: yes label Dec 5, 2017

zxu123 added 2 commits December 5, 2017 15:07

rename queryFilteredUsingPredicate to queryWherePredicate

456426a

rename queryFilteredUsingPredicate to queryWherePredicate

improve error logging

d416ed2

raise assertion in the particular case of NSBlockPredicate.

wilhuff suggested changes Dec 5, 2017

View reviewed changes

zxu123 added 3 commits December 5, 2017 16:54

time-travelling!

9fb7f21

revert name queryWherePredicate() back to queryFilteredUsingPredicate().

improve doc and fix a bug

a4ef136

* improve docs / assertion message; * fix a bug where transposed comparison is not treated correctly; also add unit test to cover that case.

project change

c741d01

project change of adding the FIRQueryTests.m file

paulb777 requested changes Dec 6, 2017

View reviewed changes

zxu123 added 3 commits December 6, 2017 14:24

refactoring to split logic into two helpers

3d08f0e

* refactoring to split logic into two helpers; * update change log.

fix typo

c19e28b

move FIRQueryTests.m into Tests/API/

edcae33

zxu123 commented Dec 6, 2017

View reviewed changes

zxu123 assigned wilhuff Dec 6, 2017

zxu123 added the api: firestore label Dec 6, 2017

remote trailing space

cfe8b4b

remove trailing space

wilhuff changed the base branch from master to firestore-api-changes December 6, 2017 23:45

wilhuff reviewed Dec 7, 2017

View reviewed changes

zxu123 and others added 2 commits December 6, 2017 21:46

fix typo in name

5585202

swift name filter -> filtered

Revert "fix typo in name"

0ad7e06

This reverts commit 5585202.

wilhuff approved these changes Dec 7, 2017

View reviewed changes

zxu123 merged commit 5da88e4 into firestore-api-changes Dec 7, 2017

zxu123 deleted the zxu/predquery branch December 7, 2017 20:36

firebase locked and limited conversation to collaborators Nov 10, 2019

Implement NSPredicate-based query #531

Implement NSPredicate-based query #531

Uh oh!

Conversation

zxu123 commented Dec 5, 2017

Discussion

Testing

API Changes

Uh oh!

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

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 commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

wilhuff left a comment