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

fix(firestore): Allow queries with combined in and array-contains-any #7142

Merged
merged 3 commits into from
Jun 5, 2023

Conversation

ibobo
Copy link
Contributor

@ibobo ibobo commented May 30, 2023

Description

Since late april queries containing in and array-contains-any together are supported as queries with multiple in statements, given that the number of generated or queries in normal form is below 30. Since @react-native-firebase/firestore is currently blocking any of these queries, this PR aims at removing that block, leaving the check for the normal form limit to the underlying firestore engine.

This is the relevant firebase docs on these limitations: https://firebase.google.com/docs/firestore/query-data/queries#limitations_3

Please note: I do not have a history of the changes available, I know this change has been added with the or operator support.

Related issues

None

Release Summary

Allow the use of in and array-contains-any in the same query.

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan

I ran the e2e tests, these are the results:

  1679 passing (3m)
  116 pending

Apart from running e2e tests I did use my fork in my own project using the aforementioned in and array-contains-any filters in some queries, and they work fine.

🔥

@vercel
Copy link

vercel bot commented May 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2023 8:26am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
react-native-firebase-next ⬜️ Ignored (Inspect) May 30, 2023 8:26am

@CLAassistant
Copy link

CLAassistant commented May 30, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Fascinating - you are exactly right! This was changed recently and makes the query language much more powerful, assuming we let them through our filtering :-). Thanks for taking the time to fix this not just for yourself but for all users, much appreciated!

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Jun 5, 2023
@mikehardy
Copy link
Collaborator

The iOS CI failure is a timeout I'm fixing in #7163 and does not affect this PR - will merge this after that one passes CI so it doesn't fail on main as well post-merge

@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

Merging #7142 (01974cd) into main (a58ea4b) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #7142      +/-   ##
============================================
- Coverage     54.00%   53.98%   -0.02%     
  Complexity      734      734              
============================================
  Files           230      230              
  Lines         11503    11497       -6     
  Branches       1854     1851       -3     
============================================
- Hits           6211     6205       -6     
  Misses         4949     4949              
  Partials        343      343              

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

Successfully merging this pull request may close these issues.

3 participants