Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[🐛] 🔥 Query validation may be overly restrictive for inequality filters #8161

Open
1 of 10 tasks
moorea opened this issue Nov 25, 2024 · 2 comments
Open
1 of 10 tasks
Labels

Comments

@moorea
Copy link

moorea commented Nov 25, 2024

Issue

I believe there may be a discrepancy between the firestore query validation logic in FirestoreQueryModifiers.js and Firebase's official documentation regarding inequality filters and orderBy requirements.

The current validation logic in _validateOrderByCheck() enforces that when using an inequality operator (>, <, >=, <=), the first orderBy() must be on the same field as the inequality filter. If not, the query is rejected with the error:

Invalid query. Initial Query.orderBy() parameter: ${orderFieldPath} has to be the same as the Query.where() fieldPath parameter(s): ${filterFieldPath} when an inequality operator is invoked

However, I do not see this specified anywhere in the official Firestore documentation about query limitations.

Using the firestore query builder, I am able to construct and run a query with an inequality filter that does not match the orderBy.

Screenshot 2024-11-25 at 2 34 39 PM

If I fork this repo and remove this block from _validateOrderByCheck():

if (INEQUALITY[filter.operator]) {
  // Initial orderBy() parameter has to match every where() fieldPath parameter when inequality operator is invoked
  if (filterFieldPath !== this._orders[0].fieldPath._toPath()) {
    throw new Error(
      `Invalid query. Initial Query.orderBy() parameter: ${orderFieldPath} has to be the same as the Query.where() fieldPath parameter(s): ${filterFieldPath} when an inequality operator is invoked `,
    );
  }
}

Then from my app, I'm also able to successfully execute the same query pictured above.

There are discussions in the docs around best practices and performance considerations for optimizing indexes, but this appears to be a performance optimization recommendation rather than a hard requirement.

Proposed Discussion Points

I'm happy to provide additional testing or help investigate further if needed. Let me know if any clarification would be helpful!


Project Files

Javascript

Click To Expand

package.json:

# N/A

firebase.json for react-native-firebase v6:

# N/A

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


@azizj1
Copy link

azizj1 commented Nov 26, 2024

_validateOrderByCheck() also gets stuck in a catch-22 and consistently crashes with same error as above if you have multiple inequalities and any orderBy. E.g., this example from firebase docs wouldn't work in this react-native-firebase library:

const q = query(
    collection(db, "cities"),
    where('population', '<', 1000000),
    where('density', '<', 10000),
    orderBy('population') // FAILS. Wants `density` first
  )

fails because it complains that density needs to be the first orderBy. But if you do

const q = query(
    collection(db, "cities"),
    where('population', '<', 1000000),
    where('density', '<', 10000),
    orderBy('density') // FAILS. Wants `population` first
  )

it fails again because now it wants population to be the first.

The following examples also fail:

const q = query(
    collection(db, "cities"),
    where('population', '<', 1000000),
    where('density', '<', 10000),
    orderBy('population'), // FAILS. Wants `density` first
    orderBy('density')
  )

const q = query(
    collection(db, "cities"),
    where('population', '<', 1000000),
    where('density', '<', 10000),
    orderBy('density'), // FAILS. Wants `population` first
    orderBy('population')
  )

@mikehardy
Copy link
Collaborator

This is the most likely reason:

Could this validation be a legacy requirement that's no longer needed?

If you propose a PR that works for you we could probably get it merged - some of the e2e tests will likely need a fixup as well to match newly-possible query structure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants