Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[cloud_firestore] Support for map fields in document pagination #1781

Merged
merged 9 commits into from
Jul 3, 2019
Merged

[cloud_firestore] Support for map fields in document pagination #1781

merged 9 commits into from
Jul 3, 2019

Conversation

creativecreatorormaybenot
Copy link
Contributor

@creativecreatorormaybenot creativecreatorormaybenot commented Jun 30, 2019

Currently, a query with orderBy('cake.flavor') will return no documents when using any document pagination method (startAtDocument etc.).
This happens because the internal method, which gets the data from the documents to fill in the startAt methods, will try to do something like map['cake.flavor'], which will return nothing.
It should really be retrieving the field's value using map['cake']['flavor'], which this pull request adds.

I am not sure about the iOS implementation, however, CI should tell me as I added an integration test (after the index is added).

@collinjackson The query in the integration test requires an index. It would be great if that could be added to test the iOS implementation as well.

@creativecreatorormaybenot creativecreatorormaybenot changed the title [cloud_firestore] Support for map fields for document pagination [cloud_firestore] Support for map fields in document pagination Jun 30, 2019
Copy link
Contributor

@collinjackson collinjackson left a comment

Choose a reason for hiding this comment

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

Thanks! I added the index.

I think it is reasonable to consider this a minor update because developers might want to require this as a minimum version for code that relies on map fields in document pagination.

Am I understanding correctly that Firebase automatically recognizes '.' as a magic field separator and that 'cake.flavor.type' is a thing that you can do today with existing native Firestore SDKs and it will be handled automatically. We're just reimplementing that functionality to try to make pagination work.

I feel like there are some maintainability challenges to our pagination implementation, but I don't see an obvious solution, and I'm ok with merging this in the meantime.

@creativecreatorormaybenot
Copy link
Contributor Author

creativecreatorormaybenot commented Jul 3, 2019

Section 1

@collinjackson I tried to create fields that use a . separator in the Firebase Console and it works as well, however, I do not think that it is problem.
In fact, the . separator is also how you query map fields in the native Firestore SDKs. I could not find any example in the documentation, however, in this video with timestamp Todd Kerpelman states that "Firestore will create an address.zip index". This also explains why this "magic field separator" works, i.e. Firestore simply treats all maps as if their fields were simply using the . separator when creating the indexes for querying. You can also see that my .where('cake.flavor.test_run', isEqualTo: testRun) in the integration test works even though I did not add any functionality to make that work.

Section 2

However, as I mentioned, you can store fields with a . in the field name as well, so I am going to adjust the implementation to check if the document data contains any data at e.g. cake.flavor.type and if not, treating cake as a map first and then checking if that map contains a field called flavor.type and if that does not exist, treat flavor as a map as well.
I will add an integration test for this as well to make sure that the distinction works fine.

Section 3

I feel like there are some maintainability challenges to our pagination implementation

Do you mean that it seems hard to adjust or improve on in the future?

Oh, as a side note: An easy empirical proof for the . separator being native to Cloud Firestore maps is going to the Firebaes Console and navigating to Database -> Data. There, you can select the messages collection (assuming that you are signed in to the integration test Firebase project) and press the filter icon at the top of the collection and then, where it says Enter field, you can type cake.flavor.type, select Ascending or Descending and Apply. I believe that there should still be a few documents that did not get cleaned up due to failing tests. Otherwise, you could just create a few test documents with maps to test out this functionality.

Edit

I was about to implement what I described in Section 2, however, playing around in the Firebase Console, I found out that calling a field e.g. a.b.c and then ordering by a.b.c will not yield any results, i.e. that document (or field) will not show up in the result. If I add a map a containing a map b with field c to the document, the query will yield a result and that result will always show the value of the field in the map.
This makes me think that querying regular fields with dots in their names is not supported by Cloud Firestore natively (only querying maps using the dot syntax). Should I still add this functionality or do you just want to merge this as is?

Illustration: https://i.imgur.com/MLgV8By.mp4

@collinjackson
Copy link
Contributor

Section 3

I feel like there are some maintainability challenges to our pagination implementation

Do you mean that it seems hard to adjust or improve on in the future?

Yes, it just feels like there may be unhandled edge cases since we're reimplementing functionality that exists in the native SDKs.

Oh, as a side note: An easy empirical proof for the . separator being native to Cloud Firestore maps is going to the Firebaes Console and navigating to Database -> Data. There, you can select the messages collection (assuming that you are signed in to the integration test Firebase project) and press the filter icon at the top of the collection and then, where it says Enter field, you can type cake.flavor.type, select Ascending or Descending and Apply. I believe that there should still be a few documents that did not get cleaned up due to failing tests. Otherwise, you could just create a few test documents with maps to test out this functionality.

Edit

I was about to implement what I described in Section 2, however, playing around in the Firebase Console, I found out that calling a field e.g. a.b.c and then ordering by a.b.c will not yield any results, i.e. that document (or field) will not show up in the result. If I add a map a containing a map b with field c to the document, the query will yield a result and that result will always show the value of the field in the map.
This makes me think that querying regular fields with dots in their names is not supported by Cloud Firestore natively (only querying maps using the dot syntax). Should I still add this functionality or do you just want to merge this as is?

Illustration: https://i.imgur.com/MLgV8By.mp4

OK, lf it's not supported natively then let's go ahead and merge this as is. Thank you!

@collinjackson collinjackson merged commit 6b87cc2 into flutter:master Jul 3, 2019
@creativecreatorormaybenot creativecreatorormaybenot deleted the document-pagination-with-map-fields branch July 3, 2019 19:30
tango5614 added a commit to tango5614/plugins that referenced this pull request Jul 4, 2019
* commit '6b87cc29ef724c5e45f5a47a40263a3f3386bb71':
  [cloud_firestore] Support for map fields in document pagination (flutter#1781)
mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 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.

3 participants