Add support for setting DataAccessMode in firebase.json#10065
Conversation
Summary of ChangesHello, 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 introduces the ability to specify the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for specifying dataAccessMode for Firestore databases in firebase.json, which is a great addition for enterprise edition databases. My review focuses on ensuring this change doesn't introduce unintended breaking changes and improves code clarity.
I've identified a potential breaking change where the default dataAccessMode for enterprise databases is altered. I've suggested a modification to preserve the existing behavior, along with an additional test case to cover this scenario. I also have a minor suggestion to simplify some conditional logic.
Overall, the changes are well-structured and the addition of tests is much appreciated.
src/deploy/firestore/prepare.ts
Outdated
| firestoreDataAccessMode: firestoreDataAccessMode ?? types.DataAccessMode.ENABLED, | ||
| mongodbCompatibleDataAccessMode: | ||
| mongodbCompatibleDataAccessMode ?? types.DataAccessMode.DISABLED, |
There was a problem hiding this comment.
Based on the PR description, changing the default dataAccessMode for enterprise databases is a breaking change that should be deferred to a future major version. This implementation changes the default to FIRESTORE_NATIVE for all databases if dataAccessMode is not specified. To avoid this breaking change for enterprise databases, the default should be conditional on the database edition, preserving MONGODB_COMPATIBLE as the default for enterprise.
firestoreDataAccessMode:
firestoreDataAccessMode ??
(edition === types.DatabaseEdition.ENTERPRISE
? types.DataAccessMode.DISABLED
: types.DataAccessMode.ENABLED),
mongodbCompatibleDataAccessMode:
mongodbCompatibleDataAccessMode ??
(edition === types.DatabaseEdition.ENTERPRISE
? types.DataAccessMode.ENABLED
: types.DataAccessMode.DISABLED),
|
|
||
| let firestoreDataAccessMode: types.DataAccessMode | undefined; | ||
| let mongodbCompatibleDataAccessMode: types.DataAccessMode | undefined; | ||
| if (firestoreCfg.dataAccessMode) { |
There was a problem hiding this comment.
We will need two dataAccessMode (firestoreDataAccessMode and mongoDataAccessMode - consistent with API design). Firestore will add support for both modes in the future (interoperability).
There was a problem hiding this comment.
I was thinking that we could simplify this into a single field dataAccessMode, and turn it into an array once we add interoperability - ie:
// Just Mongo
dataAccessMode: MONGODB_COMPATIBLE
// Just Native
dataAccessMode: FIRESTORE_NATIVE
// Interop mode
dataAccessMode: [ MONGODB_COMPATIBLE, FIRESTORE_NATIVE]
This seemed more ergonomic to me (especially before interop is available), but I don't feel too strongly about this tho, so happy to match the backend API design if you prefer
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for setting dataAccessMode when creating a Firestore database, which is a valuable addition. The implementation is correct and includes a comprehensive set of new tests. My feedback focuses on improving code conciseness and adhering to the repository's TypeScript style guide, especially in the new test file to establish good patterns.
src/deploy/firestore/prepare.spec.ts
Outdated
| @@ -0,0 +1,160 @@ | |||
| import { expect } from "chai"; | |||
| import * as sinon from "sinon"; | |||
| import * as prepare from "./prepare"; | |||
There was a problem hiding this comment.
To better align with the style guide which advises against using any as an escape hatch (line 38), consider changing this to a default import. This will allow you to call the function directly as prepare(...) in your tests, avoiding the need for (prepare.default as any) casting at multiple call sites (e.g., line 58, 84, 110).
| import * as prepare from "./prepare"; | |
| import prepare from "./prepare"; |
References
- The style guide states: "Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards." (link)
There was a problem hiding this comment.
Are we able to do this? Or does the tsconfig.json setup break things?
| const options = { | ||
| projectId, | ||
| config: { | ||
| path: (p: string) => p, | ||
| data: { | ||
| firestore: { | ||
| database: "test-db", | ||
| }, | ||
| }, | ||
| }, | ||
| } as unknown as Options; |
There was a problem hiding this comment.
The style guide (line 38) advises against using unknown as an escape hatch. Casting with as unknown as Options can make tests brittle and hide type errors. For new test files, it's a good practice to establish a pattern of creating more robust mocks. Consider creating a test utility to generate mock Options objects, or at a minimum, define the mock object with Partial<Options> and then cast to Options to make the partial nature of the mock more explicit.
References
- The style guide states: "Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards." (link)
There was a problem hiding this comment.
I'm a little worried about this since this kind of removes type safety
There was a problem hiding this comment.
Gonna leave this for now since it is a test - however, we do have a mockOptions function definite in src/emulator/controller.spec.ts that could be turned into a general utility. Making a note to do so in a follow up PR (since we'll use it in other places too)
| if (firestoreCfg.dataAccessMode === "FIRESTORE_NATIVE") { | ||
| firestoreDataAccessMode = types.DataAccessMode.ENABLED; | ||
| mongodbCompatibleDataAccessMode = types.DataAccessMode.DISABLED; | ||
| } else if (firestoreCfg.dataAccessMode === "MONGODB_COMPATIBLE") { | ||
| firestoreDataAccessMode = types.DataAccessMode.DISABLED; | ||
| mongodbCompatibleDataAccessMode = types.DataAccessMode.ENABLED; | ||
| } |
There was a problem hiding this comment.
This if/else if block can be made more concise and less repetitive by using a boolean variable to determine the mode and then using ternary operators for the assignments. This improves readability and aligns with the style guide's preference for simpler branching logic.
const isNative = firestoreCfg.dataAccessMode === "FIRESTORE_NATIVE";
firestoreDataAccessMode = isNative ? types.DataAccessMode.ENABLED : types.DataAccessMode.DISABLED;
mongodbCompatibleDataAccessMode = isNative ? types.DataAccessMode.DISABLED : types.DataAccessMode.ENABLED;References
- The style guide encourages reducing nesting and considering helper functions to encapsulate branching. While not reducing nesting, this change simplifies the branching logic, which is in the spirit of the rule. (link)
maneesht
left a comment
maneesht
left a comment
There was a problem hiding this comment.
LGTM, though it might be worth looking at the gemini review's comments in a few places
* Add support for setting DataAccessMode in firebase.json * Dont change defaults * Regen schema; * pr fixes
4 participants
Description
Seems like we overlooked this in go/firestore-data-access-mode - without this, all Enterprise edition DBs created via deploy are in MongoDB mode.
In the future, we probably want to flip the default here to be native mode - however, that is a breaking change so we will need to wait for a major version.