From 0b6b0afc09966203cb3e7612d4ac40e49e6dfd50 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Thu, 27 Feb 2020 17:44:35 -0800 Subject: [PATCH 1/6] Reproduce the issue described in #2044 in a test. ``` I/flutter ( 4164): 00:07 +9: Firestore pagination with DocumentReference (#2044) [E] I/flutter ( 4164): Invalid argument: Instance of 'DocumentReference' I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 392:7 StandardMessageCodec.writeValue I/flutter ( 4164): package:cloud_firestore_platform_interface/src/method_channel/utils/firestore_message_codec.dart 83:13 FirestoreMessageCodec.writeValue I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 389:9 StandardMessageCodec.writeValue. I/flutter ( 4164): dart:collection-patch/compact_hash.dart 379:8 _LinkedHashMapMixin.forEach I/flutter ( 4164): package:flutter/src/services/message_codecs.dart 387:13 StandardMessageCodec.writeValue I/flutter ( 4164): package:cloud_firestore_platform_interface/src/method_channel/utils/firestore_message_codec.dart 83:13 FirestoreMessageCodec.writeValue ``` --- .../example/test_driver/cloud_firestore.dart | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart index da8629a382e2..5b397961f380 100644 --- a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart +++ b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart @@ -294,6 +294,50 @@ void main() { await doc2.delete(); }); + test('pagination with DocumentReference (#2044)', () async { + // Populate the database with two test documents + final CollectionReference messages = firestore.collection('messages'); + final DocumentReference doc1 = messages.document(); + // Use document ID as a unique identifier to ensure that we don't + // collide with other tests running against this database. + final String testRun = doc1.documentID; + await doc1.setData({ + 'message': 'pagination testing1', + 'test_run': testRun, + 'created_at': FieldValue.serverTimestamp(), + }); + final DocumentSnapshot snapshot1 = await doc1.get(); + final DocumentReference doc2 = messages.document(); + await doc2.setData({ + 'message': 'pagination testing2', + 'test_run': testRun, + 'created_at': FieldValue.serverTimestamp(), + 'reference': doc1, + }); + final DocumentSnapshot snapshot2 = await doc2.get(); + + QuerySnapshot snapshot; + List results; + + // startAtDocument with the Snapshot of a doc that contains a DocumentReference. + snapshot = await messages + .orderBy('created_at', descending: true) + .where('test_run', isEqualTo: testRun) + .startAtDocument(snapshot2) + .getDocuments(); + + // TODO(ditman): The above should crash, ensure the expects below work after fixing the issue. + results = snapshot.documents; + expect(results.length, 2); + expect(results[0].data['message'], 'pagination testing1'); + expect(results[1].data['message'], 'pagination testing2'); + + + // Clean up + await doc2.delete(); + await doc1.delete(); + }); + test('FieldPath.documentId', () async { // Populate the database with two test documents. final CollectionReference messages = firestore.collection('messages'); From 536182916503ce5f8b54229aa62bbabcb2f94edd Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Fri, 28 Feb 2020 18:04:08 -0800 Subject: [PATCH 2/6] [cloud_firestore_web] Ensure FieldValueFactoryWeb encodes params. This is required for the arrayRemove/arrayUnion FieldValues. --- packages/cloud_firestore/cloud_firestore_web/CHANGELOG.md | 4 ++++ .../cloud_firestore_web/lib/src/field_value_factory_web.dart | 5 +++-- packages/cloud_firestore/cloud_firestore_web/pubspec.yaml | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/cloud_firestore/cloud_firestore_web/CHANGELOG.md b/packages/cloud_firestore/cloud_firestore_web/CHANGELOG.md index 6411d418545c..19069e805356 100644 --- a/packages/cloud_firestore/cloud_firestore_web/CHANGELOG.md +++ b/packages/cloud_firestore/cloud_firestore_web/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.1.1+1 + +* Ensure FieldValueFactoryWeb correctly encodes parameters for arrayRemove/arrayUnion FieldValues. + ## 0.1.1 * Support equality comparison of field values. diff --git a/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart b/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart index c8b54de446f2..8e117e8215d7 100644 --- a/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart +++ b/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart @@ -7,17 +7,18 @@ import 'package:firebase/firestore.dart' as web; import 'package:js/js_util.dart'; import 'package:cloud_firestore_web/src/field_value_web.dart'; +import 'package:cloud_firestore_web/src/utils/codec_utility.dart'; /// An implementation of [FieldValueFactoryPlatform] which builds [FieldValuePlatform] /// instance that is [jsify] friendly class FieldValueFactoryWeb extends FieldValueFactoryPlatform { @override FieldValueWeb arrayRemove(List elements) => - FieldValueWeb(web.FieldValue.arrayRemove(elements)); + FieldValueWeb(web.FieldValue.arrayRemove(CodecUtility.valueEncode(elements))); @override FieldValueWeb arrayUnion(List elements) => - FieldValueWeb(web.FieldValue.arrayUnion(elements)); + FieldValueWeb(web.FieldValue.arrayUnion(CodecUtility.valueEncode(elements))); @override FieldValueWeb delete() => FieldValueWeb(web.FieldValue.delete()); diff --git a/packages/cloud_firestore/cloud_firestore_web/pubspec.yaml b/packages/cloud_firestore/cloud_firestore_web/pubspec.yaml index 1fff0b661b50..36fbc984a73e 100644 --- a/packages/cloud_firestore/cloud_firestore_web/pubspec.yaml +++ b/packages/cloud_firestore/cloud_firestore_web/pubspec.yaml @@ -1,7 +1,7 @@ name: cloud_firestore_web description: The web implementation of cloud_firestore homepage: https://github.com/FirebaseExtended/flutterfire/tree/master/packages/cloud_firestore/cloud_firestore_web -version: 0.1.1 +version: 0.1.1+1 flutter: plugin: From e3aeeb90e52b862c746017fb58113303f2683e52 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Mon, 2 Mar 2020 11:33:01 -0800 Subject: [PATCH 3/6] [cloud_firestore] Ensure `DocumentSnapshotPlatform` is well encoded. Also tweak some integ tests so everything passes now, in Android. --- .../cloud_firestore/example/run-tests-web | 9 ++++++ .../example/test_driver/cloud_firestore.dart | 29 +++++++++---------- .../lib/src/utils/platform_utils.dart | 5 +++- 3 files changed, 27 insertions(+), 16 deletions(-) create mode 100755 packages/cloud_firestore/cloud_firestore/example/run-tests-web diff --git a/packages/cloud_firestore/cloud_firestore/example/run-tests-web b/packages/cloud_firestore/cloud_firestore/example/run-tests-web new file mode 100755 index 000000000000..0334baa6e633 --- /dev/null +++ b/packages/cloud_firestore/cloud_firestore/example/run-tests-web @@ -0,0 +1,9 @@ +#!/usr/bin/sh +echo "Moving tests to lib/main.dart..." +cp test_driver/cloud_firestore.dart lib/main.dart + +flutter run -d chrome + +echo "Restoring lib/main.dart..." +git restore lib/main.dart + diff --git a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart index 5b397961f380..fa41141fbc3a 100644 --- a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart +++ b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart @@ -131,6 +131,7 @@ void main() { snapshot.metadata.hasPendingWrites || snapshot.metadata.isFromCache) { snapshot = await snapshotsWithMetadataChanges.take(1).first; } + // At this point, the test is a flake... expect(snapshot.data['hello'], 'world'); await ref.delete(); @@ -304,14 +305,14 @@ void main() { await doc1.setData({ 'message': 'pagination testing1', 'test_run': testRun, - 'created_at': FieldValue.serverTimestamp(), + 'some_order': 1, }); final DocumentSnapshot snapshot1 = await doc1.get(); final DocumentReference doc2 = messages.document(); await doc2.setData({ 'message': 'pagination testing2', 'test_run': testRun, - 'created_at': FieldValue.serverTimestamp(), + 'some_order': 2, 'reference': doc1, }); final DocumentSnapshot snapshot2 = await doc2.get(); @@ -321,17 +322,17 @@ void main() { // startAtDocument with the Snapshot of a doc that contains a DocumentReference. snapshot = await messages - .orderBy('created_at', descending: true) + .orderBy('some_order', descending: true) .where('test_run', isEqualTo: testRun) .startAtDocument(snapshot2) .getDocuments(); - // TODO(ditman): The above should crash, ensure the expects below work after fixing the issue. results = snapshot.documents; expect(results.length, 2); - expect(results[0].data['message'], 'pagination testing1'); - expect(results[1].data['message'], 'pagination testing2'); + // Results are expected in descending order. + expect(results[0].data['message'], 'pagination testing2'); + expect(results[1].data['message'], 'pagination testing1'); // Clean up await doc2.delete(); @@ -383,16 +384,14 @@ void main() { await ref.document('maputo').setData({ 'country': 'Mozambique', }); - final QuerySnapshot snapshot = await ref - .where('country', whereIn: ['USA', 'Mozambique']) - .orderBy('country') - .getDocuments(); + final QuerySnapshot snapshot = await ref.where('country', + whereIn: ['USA', 'Mozambique']).getDocuments(); final List results = snapshot.documents; expect(results.length, 2); final DocumentSnapshot snapshot1 = results[0]; final DocumentSnapshot snapshot2 = results[1]; - expect(snapshot1.documentID, 'maputo'); - expect(snapshot2.documentID, 'la'); + expect(snapshot1.documentID, 'la'); + expect(snapshot2.documentID, 'maputo'); }); test('Query.whereArrayContainsAny', () async { @@ -413,8 +412,8 @@ void main() { expect(results.length, 2); final DocumentSnapshot snapshot1 = results[0]; final DocumentSnapshot snapshot2 = results[1]; - expect(snapshot1.documentID, 'la'); - expect(snapshot2.documentID, 'tokyo'); + expect(snapshot1.documentID, 'tokyo'); + expect(snapshot2.documentID, 'la'); }); test('Query.whereArrayContainsAny using DocumentReference', () async { @@ -498,7 +497,7 @@ void main() { expect(snapshot.data['children'].length, equals(2)); }); - test('FieldValue.arrayRemove with DocumentRefrence', () async { + test('FieldValue.arrayRemove with DocumentReference', () async { final CollectionReference ref = firestore.collection('messages'); await ref.document('test-docRef-1').setData({"message": "1"}); await ref.document('test-docRef-2').setData({"message": "2"}); diff --git a/packages/cloud_firestore/cloud_firestore/lib/src/utils/platform_utils.dart b/packages/cloud_firestore/cloud_firestore/lib/src/utils/platform_utils.dart index c9880257c67a..12ce5f1a7b92 100644 --- a/packages/cloud_firestore/cloud_firestore/lib/src/utils/platform_utils.dart +++ b/packages/cloud_firestore/cloud_firestore/lib/src/utils/platform_utils.dart @@ -9,7 +9,10 @@ class _PlatformUtils { DocumentSnapshot documentSnapshot) => platform.DocumentSnapshotPlatform( documentSnapshot.reference.path, - documentSnapshot.data, + // We could access `documentSnapshot._delegate.data` directly instead + // of going through the getter that `replaceDelegatesWithValuesInMap` + // on the data, but this way, the code is not tied to a part-ed lib implementation. + _CodecUtility.replaceValueWithDelegatesInMap(documentSnapshot.data), platform.SnapshotMetadataPlatform( documentSnapshot.metadata.hasPendingWrites, documentSnapshot.metadata.isFromCache, From e18fd9fd98e8d8fc99bd8ae15025819d58bf1232 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Mon, 2 Mar 2020 11:47:20 -0800 Subject: [PATCH 4/6] Bump version. --- packages/cloud_firestore/cloud_firestore/CHANGELOG.md | 5 +++++ packages/cloud_firestore/cloud_firestore/pubspec.yaml | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/cloud_firestore/cloud_firestore/CHANGELOG.md b/packages/cloud_firestore/cloud_firestore/CHANGELOG.md index 6f166d499e32..7131826a5b9d 100644 --- a/packages/cloud_firestore/cloud_firestore/CHANGELOG.md +++ b/packages/cloud_firestore/cloud_firestore/CHANGELOG.md @@ -1,3 +1,8 @@ +## 0.13.4+1 + +* Fix crash with pagination with `DocumentReference` (#2044) +* Minor tweaks to integ tests. + ## 0.13.4 * Support equality comparison on `FieldValue` instances. diff --git a/packages/cloud_firestore/cloud_firestore/pubspec.yaml b/packages/cloud_firestore/cloud_firestore/pubspec.yaml index 875715fd8500..6560518ee6c0 100755 --- a/packages/cloud_firestore/cloud_firestore/pubspec.yaml +++ b/packages/cloud_firestore/cloud_firestore/pubspec.yaml @@ -3,7 +3,7 @@ description: Flutter plugin for Cloud Firestore, a cloud-hosted, noSQL database with live synchronization and offline support on Android and iOS. homepage: https://github.com/FirebaseExtended/flutterfire/tree/master/packages/cloud_firestore/cloud_firestore -version: 0.13.4 +version: 0.13.4+1 flutter: plugin: From 39c305394955a79abd7a55a85c6a6eec1784d1b8 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Mon, 2 Mar 2020 11:55:09 -0800 Subject: [PATCH 5/6] Remove unused_local_variable --- .../cloud_firestore/example/test_driver/cloud_firestore.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart index fa41141fbc3a..f07af1cbb327 100644 --- a/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart +++ b/packages/cloud_firestore/cloud_firestore/example/test_driver/cloud_firestore.dart @@ -307,7 +307,6 @@ void main() { 'test_run': testRun, 'some_order': 1, }); - final DocumentSnapshot snapshot1 = await doc1.get(); final DocumentReference doc2 = messages.document(); await doc2.setData({ 'message': 'pagination testing2', From 52eb53661d989de261eb0201b13d9fa2ed635e38 Mon Sep 17 00:00:00 2001 From: David Iglesias Teixeira Date: Mon, 2 Mar 2020 12:02:14 -0800 Subject: [PATCH 6/6] dartfmt -w . --- .../lib/src/field_value_factory_web.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart b/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart index 8e117e8215d7..2d93b5794ebf 100644 --- a/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart +++ b/packages/cloud_firestore/cloud_firestore_web/lib/src/field_value_factory_web.dart @@ -13,12 +13,12 @@ import 'package:cloud_firestore_web/src/utils/codec_utility.dart'; /// instance that is [jsify] friendly class FieldValueFactoryWeb extends FieldValueFactoryPlatform { @override - FieldValueWeb arrayRemove(List elements) => - FieldValueWeb(web.FieldValue.arrayRemove(CodecUtility.valueEncode(elements))); + FieldValueWeb arrayRemove(List elements) => FieldValueWeb( + web.FieldValue.arrayRemove(CodecUtility.valueEncode(elements))); @override - FieldValueWeb arrayUnion(List elements) => - FieldValueWeb(web.FieldValue.arrayUnion(CodecUtility.valueEncode(elements))); + FieldValueWeb arrayUnion(List elements) => FieldValueWeb( + web.FieldValue.arrayUnion(CodecUtility.valueEncode(elements))); @override FieldValueWeb delete() => FieldValueWeb(web.FieldValue.delete());