🔥startAfter not working correctly when passing a document, when there are multiple documents matching the filtered fields

[puf]

Issue

If you have a collection where multiple documents have the same value for a field you order on, the startAfter method always returns the first document with the value. It is supposed to use the ID of the document as a disambiguation key, but that key is being dropped from the query in _handleQueryCursor.

For an example of how to reproduce this (and how it should work), see my answer to this question on Stack Overflow and this jsbin. The code from there:

var db = firebase.firestore();
var ref = db.collection("58783480").orderBy('number', 'desc');
ref.get().then(function(snapshot) {
  console.log("Got all results");
  snapshot.forEach(function(doc) {
    console.log(doc.id+' => '+JSON.stringify(doc.data()));
  });  
  var last;
  ref.limit(2).get().then(function(snapshot) {
    console.log("Got first page");
    snapshot.forEach(function(doc) {
      console.log(doc.id+' => '+JSON.stringify(doc.data()));
      last = doc;
    });
    ref.startAfter(last).limit(2).get().then(function(snapshot) {
      console.log("Got second page");
      snapshot.forEach(function(doc) {
        console.log(doc.id+' => '+JSON.stringify(doc.data()));
        last = doc;
      });
    });
  })
});

Output

Got all results
wbXwyLJheRfYXXWlY46j => {"index":2,"number":2}
kGC5cYPN1nKnZCcAb9oQ => {"index":6,"number":2}
8Ek8iWCDQPPJ5s2n8PiQ => {"index":4,"number":2}
mr7MdAygvuheF6AUtWma => {"index":1,"number":1}
RCO5SvNn4fdoE49OKrIV => {"index":3,"number":1}
CvVG7VP1hXTtcfdUaeNl => {"index":5,"number":1}

Got first page
wbXwyLJheRfYXXWlY46j => {"index":2,"number":2}
kGC5cYPN1nKnZCcAb9oQ => {"index":6,"number":2}

Got second page
8Ek8iWCDQPPJ5s2n8PiQ => {"index":4,"number":2}
mr7MdAygvuheF6AUtWma => {"index":1,"number":1}

This is the output from the regular JavaScript SDK, and is correct: the second page starts with the document after the last document on the first page.

If you run this same code (feel free to use my database, by copying the config from the jsbin) in React Native, it starts the second page with document mr7MdAygvuheF6AUtWma , thus skipping 8Ek8iWCDQPPJ5s2n8PiQ. I didn't try, but expect that startAt would instead start the second page with wbXwyLJheRfYXXWlY46j again.

---

Project Files

iOS

Click To Expand

ios/Podfile:

• I'm not using Pods
• I'm using Pods and my Podfile looks like:

# N/A

AppDelegate.m:

// N/A

---

Android

Click To Expand

Have you converted to AndroidX?

• my application is an AndroidX application?
• I am using android/gradle.settings jetifier=true for Android compatibility?
• I am using the NPM package jetifier for react-native compatibility?

android/build.gradle:

// N/A

android/app/build.gradle:

// N/A

android/settings.gradle:

// N/A

MainApplication.java:

// N/A

AndroidManifest.xml:

<!-- N/A -->

---

Environment

Click To Expand

react-native info output:

 OUTPUT GOES HERE

• Platform that you're experiencing the issue on:
  • iOS
  • Android
  • iOS but have not tested behavior on Android
  • Android but have not tested behavior on iOS
  • Both
• react-native-firebase version you're using that has this issue:
  • e.g. 5.4.3
• Firebase module(s) you're using that has the issue:
  • e.g. Instance ID
• Are you using TypeScript?
  • Y/N & VERSION

---

Think react-native-firebase is great? Please consider supporting all of the project maintainers and contributors by donating via our Open Collective where all contributors can submit expenses. [Learn More]

• 👉 Check out React Native Firebase and Invertase on Twitter for updates on the library.

[puf]

@shripadashtekar Please create a new issue for... your new issue. Adding more information here, does not help either this issue, or the new one you found.

[mikehardy]

Hey @puf - I don't see some of the information in the template, but you link directly to a source file that makes me 99.9% sure you mean react-native-firebase v6, and indeed though it was a commit-hash in the link that appears to be the latest commit and the line appears the same in master as of today. Is that correct? Just want to make sure we're talking about exactly the same code. Normally the template is vital but I think this report may be complete regardless...

[shripadashtekar]

@puf sure. I'll create a new issue for the same.
Hey @mikehardy , The issue was observed in V5.5.6. I haven't checked it on V6.

[puf]

Hey Mike,
I didn't reproduce the issue myself, so can't speak for the iOS/Android details. But I showed the correct behavior in the jsbin, and got OP from Stack Overflow to then show their output based on the same code and data.

The _handleQueryCursor source I linked to seems to only pass the values of the DocumentSnapshot along to Firestore (as far as I can see), which would explain the behavior OP is seeing.

[Ehesp]

Hey @puf - thanks for the detailed report. I'll investigate this and see where the issue is.

[puf]

Sure thing Elliot. Let me know if there's anything I can help with, as it seems quite fundamental. Either here or on mail (puf@g.c) is fine. I also filed an internal bug to get the docs clarified, as it's now easy to misunderstand them in the case where you're passing a list of field values.

…

On Mon, Nov 11, 2019 at 3:23 AM Elliot Hesp ***@***.***> wrote: Hey @puf <https://github.com/puf> - thanks for the detailed report. I'll investigate this and see where the issue is. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2854?email_source=notifications&email_token=AAG7BX42K24XWM2YXY2XJP3QTE6CNA5CNFSM4JLMW2T2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDWQJ5I#issuecomment-552404213>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG7BXYUUXCUPII3XHLSOV3QTE6CNANCNFSM4JLMW2TQ> .

[puf]

Hey Elliott,
I chatted with one of our DB engineers about this today, and he reminded me that Firestore auto-adds ID to all sort orders. So you can use something like query.orderBy('field1', 'field2', FieldPath.documentId()).startAt('value1', 'value2', 'document-id'), even when the developer didn't sort on document ID.
So while it'd be best to pass the actual DocumentSnapshot along to prevent mismatches, adding the ID to the sort order might be a quick fix.

[Ehesp]

Hey @puf

We've had a good look into what's going on here. Currently we have some limitations on how we can do this for React Native:

Parsing a DocumentSnapshot to a filter

DocumentSnapshot on Android & iOS cannot be serialized & recreated. In React Native, communication is done over a JSON bridge between JS & Native (Java/Obj-c). The problem here is that new instances of DocumentSnapshot can't be created natively (private constructors, including private constructors for each of the data types). To work around this, we instead used the internal __name__ property to build a field value so we can apply query cursors.

This leads onto the second issue...

Using __name__ field value

When using the __name__ field value with the document ID on its own with no other field value filters this works fine. However, as soon as we combine it with any other filters (e.g. orderBy) the native SDKs assume an index needs to be created (errors with failed precondition). When you try to create the index through the console link provided, it tries to create the index with __name__ included as a compound field which doesn't work as it's an internal field and errors.

This commit was the result of this finding (fixing #2719). However as we can see, this change causes this issue.

---

I'm not sure how we can approach a fix for this, given the native SDK doesn't allow us to create our own DocumentSnapshot instances that we can use to apply in the query. There may be a workaround on iOS & Android may be possible through reflection to make the constructors public, however this would touch a lot of internal classes and be easily breakable.

[mikelehen]

@Ehesp Hi Elliot. Firestore SDK engineer here. I am curious about the "Using name field value" issue you're running into where you get prompted to construct an index. That shouldn't happen as far as I know and I'd love more details about what you're doing.

For a bit of context, any time you construct a query and don't include 'name' in the orderBys, Firestore will implicitly assume an additional .orderBy('__name__', DIRECTION) where DIRECTION will match the last orderBy direction of your query (or 'asc' if you have no orderBys).

This means:

• query.where(...) => query.where(...).orderBy('__name__', 'asc')
• query.where(...).orderBy('foo', 'asc') => query.where(...).orderBy('foo', 'asc').orderBy('__name__', 'asc')
• query.where(...).orderBy('foo', 'desc') => query.where(...).orderBy('foo', 'desc').orderBy('__name__', 'desc')
• query.where(...).orderBy('__name__', 'desc') => query.where(...).orderBy('__name__', 'desc')

In all of these cases, the before and after versions of the query use the same index, and so no additional index needs to be constructed. Perhaps when you are adding the name orderBy you aren't matching the correct direction, leading to the prompt to create an index and resulting difficulties.

[Salakar]

@mikelehen Firestore will implicitly assume an additional .orderBy('__name__', DIRECTION) where DIRECTION will match the last orderBy direction of your query (or 'asc' if you have no orderBys).

Ah, thank you for this - looks like this is what we've missed then - I've pushed up #2866 and added tests based on the example @puf provided and all seems to be working now 🎉 its no longer failing and prompting for an index.

Thanks for your helping debugging this everyone, providing CI passes I'll get a patch release out later today

[shripadashtekar]

HI Team,
How can I get the fix as I am on react-native-firebase v5.5.6? I can't upgrade to v6 of this library, as it's minimum react native version requirement is v6 and I am on react-native v0.59.8. I can't afford to upgrade everything. Is the backport forthcoming for 5.x?

[mikehardy]

@shripadashtekar I think you'd have to backport it - it should go somewhere around here https://github.com/invertase/react-native-firebase/blob/v5.x.x/src/modules/firestore/Query.js#L462 but the code there has diverged quite a bit from v5 to v6 so you'll have to take care that the query you emit is actually what you intend

Forward-porting to react-native 0.60+ / react-native-firebase v6 may be a similar amount of effort, it's hard to say - but it is possible to say that the forward-port effort may only be deferred, not eliminated, so that effort may be the better time investment over time if it can be justified in your project

[shripadashtekar]

Thanks, but if you can help me with the actual changes(if not many) for 5.5.6, I'll be grateful, as I am not aware of the code and will take time for me to figure out the change.

[mikehardy]

Sorry, I can't commit to any time on it, and I'm not familiar with that area either

[shripadashtekar]

No Problem @mikehardy . @Salakar Could you please help me with the same? My entire application is dependent on pagination, so it's a blocker for me.
Thanks.

[Salakar]

Hey all, the fix is now live in v6.0.4. Thanks

[puf]

Thanks for the quick turnaround Mike! 🙏

[LeonidVeremchuk]

Sorry for comment. I am looking for solution...

But where().where().startAfter().limit().get(). works correctly,
orderBy().where().where().startAfter().limit().get() work ones and return empty collection when try to fetch next "page"

"@react-native-firebase/firestore": "10.4.0",
"react-native": "0.63.3",

[LeonidVeremchuk]

Interesting situation with my last comment:
type and status is String
When I changed a type to Number value its works correctly

[mikehardy]

Don't waste any time with old versions, glad you updated that.
Can you reproduce with https://github.com/firebase/quickstart-android/tree/master/firestore ?

[LeonidVeremchuk]

@mikehardy
I write small test with you sample
It work on android correctly

D/TEST: size = 10
D/TEST: document1 id = qHB6NLBtXicPigljU6Cb
D/TEST: document2 id = v5Kk0OegdRE4yt6LclZs

[mikehardy]

🤔 so it appears it is working for the native sdk but obviously we're not doing something correctly

the README in the tests directory should get you set up with an inspection rig for this module within just a few minutes, then if you add '.only' to any of the tests in packages/firestore/e2e and run it on android you can execute just that one test (an existing one, or a new one, suggest making a new one in the issues area). If you can get it reproducing in the e2e tests then you've got a really fast path towards iterating / instrumenting the module code and seeing exactly what/where things are going wrong, and probably can get a fix up. You know your use case better than anyone and I sincerely believe it won't take long to do - that's where I'd suggest going next

A couple deep-links https://github.com/invertase/react-native-firebase/blob/master/tests/README.md#running-specific-tests
https://github.com/invertase/react-native-firebase/blob/master/packages/firestore/e2e/issues.e2e.js

[LeonidVeremchuk]

@mikehardy
I apologize. After several tests, I found that I was continuing to use the old version of the SDK.

In 10.4.1 this problem is not reproduced. Thanks for answers.