Skip to content

[cloud_firestore] Add FieldPath#1296

Merged
collinjackson merged 30 commits into
firebase:masterfrom
creativecreatorormaybenot:firestore-field-path
Nov 7, 2019
Merged

[cloud_firestore] Add FieldPath#1296
collinjackson merged 30 commits into
firebase:masterfrom
creativecreatorormaybenot:firestore-field-path

Conversation

@creativecreatorormaybenot

@creativecreatorormaybenot creativecreatorormaybenot commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

Description

This brought my attention to FieldPath again. While my solution on StackOverflow works, it is not particularly nice to use.

Related Issues

Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@creativecreatorormaybenot creativecreatorormaybenot changed the title Add FieldPath [cloud_firestore] Add FieldPath Oct 22, 2019

@collinjackson collinjackson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding tests! It seems like we're relying on an implementation detail (the sentinel value is "__name__") and instead I think we should implement it in the same way as FieldValue and call the native version of FieldPath.documentId.

https://github.com/FirebaseExtended/flutterfire/blob/a24db9c4dac979989b86838069095b95ab2202a9/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java#L917

Comment thread packages/cloud_firestore/lib/src/field_path.dart Outdated
Comment thread packages/cloud_firestore/example/android/gradle.properties Outdated
@creativecreatorormaybenot creativecreatorormaybenot changed the title [cloud_firestore] Add FieldPath [cloud_firestore] Add FieldPath [WIP] Oct 23, 2019
@creativecreatorormaybenot creativecreatorormaybenot changed the title [cloud_firestore] Add FieldPath [WIP] [cloud_firestore] Add FieldPath Oct 23, 2019
@creativecreatorormaybenot

Copy link
Copy Markdown
Contributor Author

@collinjackson What kind of formatter are you using in flutterfire?

When I use the formatter (which the IDE does automatically 😞), the whole file is changed and the Cirrus CI test also fails.. It is very cumbersome for me to manually adjust the format.

@collinjackson

collinjackson commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

@collinjackson What kind of formatter are you using in flutterfire?

When I use the formatter (which the IDE does automatically 😞), the whole file is changed and the Cirrus CI test also fails.. It is very cumbersome for me to manually adjust the format.

We're using pub global run flutter_plugin_tools format. That's all I know...

Pro tip: The failure message gives you a patch command that will fix the formatting for you:

patch -p1 <<DONE
diff --git a/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java b/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
index f231617..524589d 100644
--- a/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
+++ b/packages/cloud_firestore/android/src/main/java/io/flutter/plugins/firebase/cloudfirestore/CloudFirestorePlugin.java
@@ -133,14 +133,15 @@ public class CloudFirestorePlugin implements MethodCallHandler {
         if (field instanceof FieldPath) {
           if (field == FieldPath.documentId()) {
-            throw new IllegalArgumentException("You cannot use {start/end}{At/After}Document " +
-                    "when ordering by the document id. When using any of the mentioned methods, " +
-                    "the library will order by the document id implicitly as " +
-                    "it needs to add other fields to the order clause.");
+            throw new IllegalArgumentException(
+                "You cannot use {start/end}{At/After}Document "
+                    + "when ordering by the document id. When using any of the mentioned methods, "
+                    + "the library will order by the document id implicitly as "
+                    + "it needs to add other fields to the order clause.");
           } else {
             // Unsupported type.
           }
-        } else if (field instanceof  String) {
+        } else if (field instanceof String) {
           String orderByFieldName = (String) field;
           if (orderByFieldName.contains(".")) {
             String[] fieldNameParts = orderByFieldName.split("\\.");
@@ -313,7 +314,7 @@ public class CloudFirestorePlugin implements MethodCallHandler {
       boolean descending = (boolean) order.get(1);
       Query.Direction direction =
-              descending ? Query.Direction.DESCENDING : Query.Direction.ASCENDING;
+          descending ? Query.Direction.DESCENDING : Query.Direction.ASCENDING;
       if (fieldName != null) {
         query = query.orderBy(fieldName, direction);
diff --git a/packages/cloud_firestore/lib/src/field_path.dart b/packages/cloud_firestore/lib/src/field_path.dart
index e144cd0..64cdde8 100644
--- a/packages/cloud_firestore/lib/src/field_path.dart
+++ b/packages/cloud_firestore/lib/src/field_path.dart
@@ -16,5 +16,6 @@ class FieldPath {
   final _FieldPathType type;
   /// The path to the document id, which can be used in queries.
-  static FieldPath get documentId => const FieldPath._(_FieldPathType.documentId);
+  static FieldPath get documentId =>
+      const FieldPath._(_FieldPathType.documentId);
 }
diff --git a/packages/cloud_firestore/lib/src/firestore_message_codec.dart b/packages/cloud_firestore/lib/src/firestore_message_codec.dart
index 1a555fc..9f658cd 100644
--- a/packages/cloud_firestore/lib/src/firestore_message_codec.dart
+++ b/packages/cloud_firestore/lib/src/firestore_message_codec.dart
@@ -34,7 +34,7 @@ class FirestoreMessageCodec extends StandardMessageCodec {
   static const Map<_FieldPathType, int> _kFieldPathCodes =
       <_FieldPathType, int>{
     _FieldPathType.documentId: _kDocumentId,
-      };
+  };
   @override
   void writeValue(WriteBuffer buffer, dynamic value) {
diff --git a/packages/cloud_firestore/test/cloud_firestore_test.dart b/packages/cloud_firestore/test/cloud_firestore_test.dart
index d437202..63a3f06 100755
--- a/packages/cloud_firestore/test/cloud_firestore_test.dart
+++ b/packages/cloud_firestore/test/cloud_firestore_test.dart
@@ -1264,7 +1264,8 @@ bool _deepEquals(dynamic valueA, dynamic valueB) {
   if (valueA is FieldValue) {
     return valueB is FieldValue && _deepEqualsFieldValue(valueA, valueB);
   }
-  if (valueA is FieldPath) return valueB is FieldPath && valueA.type == valueB.type;
+  if (valueA is FieldPath)
+    return valueB is FieldPath && valueA.type == valueB.type;
   return valueA == valueB;
 }
DONE

@creativecreatorormaybenot

Copy link
Copy Markdown
Contributor Author

@collinjackson Thanks a bunch!

@creativecreatorormaybenot

creativecreatorormaybenot commented Oct 23, 2019

Copy link
Copy Markdown
Contributor Author

I think that I am done with the new implementation. I added some exceptions that will help users understand what about their queries was incorrect and most importantly added assertions when possible.

@creativecreatorormaybenot

Copy link
Copy Markdown
Contributor Author

@collinjackson Can you review again?

Comment thread packages/cloud_firestore/ios/Classes/CloudFirestorePlugin.m Outdated

@collinjackson collinjackson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this lgtm

@collinjackson collinjackson merged commit 33e0586 into firebase:master Nov 7, 2019
@creativecreatorormaybenot creativecreatorormaybenot deleted the firestore-field-path branch November 7, 2019 23:02
kroikie pushed a commit to collinjackson/flutterfire that referenced this pull request Nov 15, 2019
@firebase firebase locked and limited conversation to collaborators Aug 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants