-
Notifications
You must be signed in to change notification settings - Fork 1.1k
First draft of firestore-basics skill #9719
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: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @joehan, 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 initial draft of a new 'firestore-basics' skill. Its primary goal is to serve as a comprehensive resource for users looking to understand and implement Cloud Firestore, covering essential aspects from initial setup and configuration to securing data with rules and integrating the SDK into applications. 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. 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
|
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 introduces a new 'firestore-basics' skill, providing a foundational guide to Cloud Firestore. The skill is well-structured into provisioning, security rules, and SDK usage. My review includes suggestions to enhance the examples and documentation to make them more practical for users. Specifically, I've recommended improving how data is logged in the SDK examples, adding more advanced security rule patterns like data validation and separated read/write access, and including a section on deploying Firestore configurations. These suggestions align with the TODOs mentioned in the pull request description and aim to make the skill more complete.
|
/gemini review |
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 introduces a new 'firestore-basics' skill, providing comprehensive documentation and code examples for web, Android, and iOS. The content is well-structured and covers key aspects of Firestore, including provisioning, security rules, SDK usage, and indexes. My review focuses on improving the correctness and clarity of the code snippets and documentation. I've identified several issues, such as incorrect API usage in the Android SDK examples, unsafe coding practices like force-unwrapping in Swift, and formatting problems in the markdown files. I've provided specific suggestions to fix these issues, which will enhance the quality and reliability of the skill for developers.
|
|
||
| ```kotlin | ||
| db.collection("cities") | ||
| .orderBy("name", Query.Direction.KEY_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.
| ``` | ||
| match /databases/{database}/documents { | ||
| // Allow access based on email domain | ||
| match /some_collection/{document} { | ||
| allow read: if request.auth != null | ||
| && request.auth.email_verified | ||
| && request.auth.email.endsWith('@example.com') | ||
| } | ||
| } | ||
| ``` |
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.
The example for allowing access to users with verified emails contains a redundant and incorrect nested match /databases/{database}/documents block. Security rules files should only have one top-level service block, and match blocks for paths are nested within that. This extra nesting will cause a syntax error.
| ``` | |
| match /databases/{database}/documents { | |
| // Allow access based on email domain | |
| match /some_collection/{document} { | |
| allow read: if request.auth != null | |
| && request.auth.email_verified | |
| && request.auth.email.endsWith('@example.com') | |
| } | |
| } | |
| ``` | |
| ### Allow only verified emails | |
| Requires users to verify ownership of the email address before using it to read or write data. This snippet should be placed inside your `match /databases/{database}/documents` block. | |
| ```firestore | |
| // Allow access based on email domain | |
| match /some_collection/{document} { | |
| allow read: if request.auth != null | |
| && request.auth.email_verified | |
| && request.auth.email.endsWith('@example.com'); | |
| } |
| import { initializeApp } from "firebase/app"; | ||
| import { getFirestore } from "firebase/firestore"; | ||
|
|
||
| const firebaseConfig = { | ||
| // Your config options. Get the values by running 'firebase apps:sdkconfig <platform> <app-id>' | ||
| }; | ||
|
|
||
| const app = initializeApp(firebaseConfig); | ||
| const db = getFirestore(app); | ||
|
|
||
| ``` |
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.
The markdown formatting in this section is broken. The import statements are outside the code block, and there is a stray ``` at the end. This makes the example incorrect and hard to read. All code, including imports, should be inside a single javascript code block. Additionally, this guide is missing an example for connecting to the Firestore emulator, which is present in the Android and iOS guides. This formatting issue is repeated for all code examples in this file.
| import { initializeApp } from "firebase/app"; | |
| import { getFirestore } from "firebase/firestore"; | |
| const firebaseConfig = { | |
| // Your config options. Get the values by running 'firebase apps:sdkconfig <platform> <app-id>' | |
| }; | |
| const app = initializeApp(firebaseConfig); | |
| const db = getFirestore(app); | |
| ``` | |
| import { initializeApp } from "firebase/app"; | |
| import { getFirestore, connectFirestoreEmulator } from "firebase/firestore"; | |
| const firebaseConfig = { | |
| // Your config options. Get the values by running 'firebase apps:sdkconfig <platform> <app-id>' | |
| }; | |
| const app = initializeApp(firebaseConfig); | |
| const db = getFirestore(app); | |
| // Connect to Emulator. This should be conditional (e.g. in a development environment). | |
| connectFirestoreEmulator(db, 'localhost', 8080); |
|
|
||
| // Merge | ||
| db.collection("cities").document("LA") | ||
| .set(mapOf("population" to 3900000), SetOptions.merge()) |
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.
| val snapshot = transaction.get(sfDocRef) | ||
|
|
||
| // Note: You can also use FieldValue.increment() for simple counters | ||
| val newPopulation = snapshot.getDouble("population")!! + 1 |
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.
Using the not-null assertion operator (!!) is risky as it can cause a NullPointerException if the "population" field is missing or not a Double, which would crash the app. It's safer to handle this potential null case gracefully, for example by using the elvis operator ?: with a default value.
| val newPopulation = snapshot.getDouble("population")!! + 1 | |
| val newPopulation = (snapshot.getDouble("population") ?: 0.0) + 1 |
| if let err = err { | ||
| print("Error adding document: \(err)") | ||
| } else { | ||
| print("Document added with ID: \(ref!.documentID)") |
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.
| let dataDescription = document.data().map(String.init(describing:)) ?? "nil" | ||
| print("Document data: \(dataDescription)") |
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.
Using String(describing:) on the document.data() dictionary is not very illustrative for developers. It would be more helpful to show an example of how to access individual fields from the data dictionary, for instance, by casting a value to its expected type.
Example:
if let data = document.data() {
let name = data["name"] as? String ?? "N/A"
print("City name: \(name)")
}| if let err = err { | ||
| print("Error getting documents: \(err)") | ||
| } else { | ||
| for document in querySnapshot!.documents { |
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.
Force-unwrapping querySnapshot! is unsafe. If an error occurs that is not caught by if let err = err, but querySnapshot is still nil, this will cause a crash. It's safer to use optional binding or the nil-coalescing operator.
| for document in querySnapshot!.documents { | |
| for document in querySnapshot?.documents ?? [] { |
| ## Deploy rules and indexes | ||
| To deploy all rules and indexes | ||
| ``` | ||
| firebase deploy --only firestore | ||
| ``` | ||
| To deploy just rules | ||
| ``` | ||
| firebase deploy --only firestore:rules | ||
| ``` | ||
| To deploy just indexes | ||
| ``` | ||
| firebase deploy --only firestore:indexes | ||
| ``` |
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.
The deployment commands are currently formatted as separate lines of inline code, which is hard to read and copy. It would be better to group them into a single bash code block for clarity and ease of use.
| ## Deploy rules and indexes | |
| To deploy all rules and indexes | |
| ``` | |
| firebase deploy --only firestore | |
| ``` | |
| To deploy just rules | |
| ``` | |
| firebase deploy --only firestore:rules | |
| ``` | |
| To deploy just indexes | |
| ``` | |
| firebase deploy --only firestore:indexes | |
| ``` | |
| ## Deploy rules and indexes | |
| To deploy all rules and indexes: | |
| ```bash | |
| firebase deploy --only firestore |
To deploy just rules:
firebase deploy --only firestore:rulesTo deploy just indexes:
firebase deploy --only firestore:indexes| await runTransaction(db, async (transaction) => { | ||
| const sfDoc = await transaction.get(sfDocRef); | ||
| if (!sfDoc.exists()) { | ||
| throw "Document does not exist!"; |
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.
Slightly more polished draft of the firestore-basics skill. It covers:
TODOs