-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(firestore): smart filtering of __name__ fields in index listings #8860
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
base: master
Are you sure you want to change the base?
fix(firestore): smart filtering of __name__ fields in index listings #8860
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @TorbenWetter, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request delivers a crucial fix for Firestore index deployments by refining how __name__
fields are handled during index listing. It introduces a 'smart filtering' mechanism that differentiates between explicit and implicit __name__
fields, thereby resolving deployment conflicts and duplicate index errors. The change is backward compatible and isolated to the index listing logic, ensuring more robust and reliable Firestore deployments.
Highlights
- Firestore Index Listing Fix: Addresses a critical regression that caused
firebase deploy --only firestore
to fail with "409 index already exists" errors, stemming from an overly aggressive previous fix for duplicate index issues. - Intelligent
__name__
Field Filtering: ThelistIndexes
method insrc/firestore/api.ts
has been enhanced to intelligently filter__name__
fields. It now preserves__name__
fields withDESCENDING
order (which are explicitly defined and meaningful) while removing those withASCENDING
order (which are implicit and default). - Comprehensive Test Coverage: Five new, comprehensive test cases have been added to
src/firestore/indexes.spec.ts
under a newIndexListingWithNameFields
suite. These tests validate the intelligent filtering logic across various scenarios, including handling of bothASCENDING
andDESCENDING
__name__
fields, and ensuring correct distinction between indexes. - Problem Resolution: This change successfully resolves both the duplicate index issue (#7629) and the deployment conflicts (#8859) by ensuring that the comparison logic for Firestore indexes accurately reflects user specifications and API responses.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a regression by intelligently filtering __name__
fields from Firestore index listings. The core logic change in src/firestore/api.ts
is correct and aligns with the PR description.
I've provided two main pieces of feedback:
- A suggestion to refactor the implementation in
src/firestore/api.ts
for better type safety and conciseness. - A more critical point regarding the new tests in
src/firestore/indexes.spec.ts
. The tests currently duplicate the production logic, which means they aren't actually testing the implementation. I've recommended a refactoring approach to ensure the tests are correctly verifying the production code.
Overall, the fix is good, but the testing approach needs to be improved to ensure long-term maintainability.
Preserve __name__ fields with DESCENDING order while filtering out implicit ASCENDING ones to fix deployment conflicts. - Fixes duplicate index issues (firebase#7629) - Fixes deployment conflicts (firebase#8859) - Add comprehensive test coverage Fixes firebase#7629, firebase#8859
9120ded
to
f127fba
Compare
Refactored __name__ field filtering from listIndexes into static method FirestoreApi.processIndexes() to ensure tests verify production code rather than duplicating the implementation.
Simplified the filter expression in processIndexes() to a single boolean expression as suggested in PR feedback for better readability and conciseness.
Thanks for opening up this PR @TorbenWetter! I'm testing the changes and it seems like the error is still occurring. Tried with indexes
{
"indexes": [
{
"collectionGroup": "widgets",
"queryScope": "COLLECTION",
"fields": [
{ "fieldPath": "foo", "arrayConfig": "CONTAINS" },
{ "fieldPath": "bar", "mode": "DESCENDING" }
]
},
{
"collectionGroup": "widgets",
"queryScope": "COLLECTION",
"fields": [
{ "fieldPath": "foo", "arrayConfig": "CONTAINS" },
{ "fieldPath": "bar", "mode": "ASCENDING" }
]
}
]
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taking a closer look at the issue @aalej found.
Hmmm, it seems like the behavior here is tricker than we initially realized - in the example @aalej shared, the 'implied' direction for name is decending when the other fields are descending |
Ahhh, turns out we documented it - https://firebase.google.com/docs/firestore/query-data/index-overview#default_ordering_and_the_name_field the default ordering follows the ordering of the last field in the index. |
I think in issue #7629, there's an instance where the default ordering for There's also a case where the indexes deployed changed, here's a repro - #8758 (comment) |
Problem
The recent fix for #7629 introduced a regression where
firebase deploy --only firestore
fails with "409 index already exists" errors. The original fix was too aggressive, preserving ALL__name__
fields instead of filtering intelligently.Root Cause
The previous implementation completely removed the filtering logic for
__name__
fields. While this fixed the duplicate index issue (#7629), it introduced deployment conflicts because:__name__
fields__name__
fieldsSolution
This PR implements smart filtering that:
__name__
fields with DESCENDING order (explicit/meaningful)__name__
fields with ASCENDING order (implicit/default)firebase deploy --only firestore
fails with 409 error on existing indexes in firebase-tools v14.11.0 #8859)Implementation
The fix modifies the
listIndexes
method insrc/firestore/api.ts
to filter__name__
fields intelligently:Changes
listIndexes
method with intelligent__name__
field filteringTesting
IndexListingWithNameFields
validates the fixImpact
This change is backward compatible and isolated to index listing logic. It resolves the critical deployment failures in v14.11.0 while maintaining the semantic meaning of indexes.
Fixes #7629, #8859