🔥 Feature: Add Firestore Storage Driver #1931
Conversation
yusirdemir
requested review from
ReneWerner87,
efectn,
gaby and
sixcolors
and removed request for
a team
Summary of ChangesHello @yusirdemir, 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 a new Firestore storage driver, significantly expanding the data persistence options available within the Fiber framework. By integrating with Google Cloud's NoSQL document database, this driver provides a scalable and flexible solution for managing application data, complete with features like data expiration and context-aware operations. This enhancement allows Fiber users to leverage Firestore's capabilities directly through a familiar storage interface. Highlights
Ignored Files
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
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new Firestore storage driver: configuration and defaults, credential loading, context-aware CRUD with expiration semantics, bulk Reset, connection exposure, README docs, and emulator-based integration tests and benchmarks. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Storage as Firestore Storage
participant Client as firestore.Client
participant DB as Firestore
rect rgba(200,230,255,0.3)
Note over App,Storage: Initialization
App->>Storage: New(config)
Storage->>Storage: validate ProjectID & apply defaults
Storage->>Client: create client (credentials)
Client->>DB: connect
Storage-->>App: *Storage
end
rect rgba(220,255,220,0.25)
Note over App,Storage: Set flow
App->>Storage: Set(key, value, exp)
Storage->>Storage: ensure ctx with timeout
Storage->>Client: Collection(col).Doc(key).Set(ctx, doc)
Client->>DB: write
DB-->>Client: OK
Storage-->>App: nil
end
rect rgba(255,240,200,0.2)
Note over App,Storage: Get flow
App->>Storage: Get(key)
Storage->>Storage: ensure ctx with timeout
Storage->>Client: Collection(col).Doc(key).Get(ctx)
Client->>DB: read
alt found
DB-->>Client: doc
Client-->>Storage: snapshot
Storage->>Storage: check ExpiresAt
alt expired
Storage->>Client: Doc(key).Delete(ctx) (async/inline)
Storage-->>App: nil
else not expired
Storage-->>App: value
end
else not found
DB-->>Client: NotFound
Storage-->>App: nil
end
end
rect rgba(240,230,255,0.2)
Note over App,Storage: Reset flow
App->>Storage: Reset()
Storage->>Storage: ensure ctx with timeout
Storage->>Client: BulkWriter -> delete docs in collection
Client->>DB: batch deletes
DB-->>Client: OK
Storage-->>App: nil
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 introduces a new Firestore storage driver. The implementation is comprehensive, including configuration, tests with testcontainers, and documentation. I've found a critical issue in the Reset functionality and some areas for improvement in configuration handling, documentation, and code structure. My comments below provide details and suggestions for these points. Overall, this is a great contribution.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
firestore/firestore_test.go (1)
51-57: RestoreFIRESTORE_EMULATOR_HOSTafter each test.We call
os.Setenvbut never revert it, so the emulator host leaks into later tests running in the same process. Capture the previous value and restore it viat.Cleanupto keep the environment isolated.- os.Setenv("FIRESTORE_EMULATOR_HOST", host+":"+port.Port()) + prev, had := os.LookupEnv("FIRESTORE_EMULATOR_HOST") + require.NoError(t, os.Setenv("FIRESTORE_EMULATOR_HOST", host+":"+port.Port())) + t.Cleanup(func() { + var err error + if had { + err = os.Setenv("FIRESTORE_EMULATOR_HOST", prev) + } else { + err = os.Unsetenv("FIRESTORE_EMULATOR_HOST") + } + require.NoError(t, err) + })firestore/firestore.go (4)
82-84: Prefer idiomatic empty string check.Use
key == ""instead oflen(key) <= 0for clarity. The<= 0comparison is unnecessary for strings (length can never be negative) and less idiomatic than direct empty string comparison.Apply this diff:
- if len(key) <= 0 { + if key == "" { return nil, nil }
128-130: Prefer idiomatic empty checks.Same as in
GetWithContext, usekey == ""instead oflen(key) <= 0. For the value check,len(val) == 0is fine since you're checking actual byte slice length.Apply this diff:
- if len(key) <= 0 || len(val) <= 0 { + if key == "" || len(val) == 0 { return nil }
160-162: Prefer idiomatic empty string check.Same as in previous methods, use
key == ""instead oflen(key) <= 0.Apply this diff:
- if len(key) <= 0 { + if key == "" { return nil }
86-90: Consider extracting repeated context timeout pattern.The pattern of checking
ctx.Deadline()and adding a timeout if missing is repeated in four methods (GetWithContext, SetWithContext, DeleteWithContext, ResetWithContext). Consider extracting to a helper method to reduce duplication.Example helper:
// ensureTimeout returns a context with a timeout if the provided context doesn't have a deadline func (s *Storage) ensureTimeout(ctx context.Context) (context.Context, context.CancelFunc) { if _, ok := ctx.Deadline(); !ok { return context.WithTimeout(ctx, s.timeout) } return ctx, func() {} }Then use it in each method:
func (s *Storage) GetWithContext(ctx context.Context, key string) ([]byte, error) { if key == "" { return nil, nil } - if _, ok := ctx.Deadline(); !ok { - var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, s.timeout) - defer cancel() - } + ctx, cancel := s.ensureTimeout(ctx) + defer cancel() doc, err := s.client.Collection(s.collection).Doc(key).Get(ctx) // ... }Also applies to: 132-136, 164-168, 186-190
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/test-firestore.ymlis excluded by!**/*.ymlfirestore/go.modis excluded by!**/*.modfirestore/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
README.md(3 hunks)firestore/README.md(1 hunks)firestore/config.go(1 hunks)firestore/config_test.go(1 hunks)firestore/firestore.go(1 hunks)firestore/firestore_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-12T11:24:31.153Z
Learnt from: ReneWerner87
PR: gofiber/storage#0
File: :0-0
Timestamp: 2025-02-12T11:24:31.153Z
Learning: The storage package in gofiber/storage repository can be used independently of the Fiber web framework.
Applied to files:
README.md
🧬 Code graph analysis (4)
firestore/config.go (1)
minio/config.go (1)
Credentials(54-59)
firestore/firestore_test.go (2)
firestore/firestore.go (2)
Storage(25-29)New(39-71)firestore/config.go (1)
Config(8-42)
firestore/config_test.go (1)
firestore/config.go (2)
Config(8-42)ConfigDefault(45-49)
firestore/firestore.go (1)
firestore/config.go (2)
Config(8-42)ConfigDefault(45-49)
🪛 LanguageTool
firestore/README.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...n) ### Table of Contents - Signatures - Installation - [Examples...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...Signatures](#signatures) - Installation - Examples - Config...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...Installation](#installation) - Examples - Config - [Default Config](#def...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...ation) - Examples - Config - Default Config ### Si...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
firestore/README.md
13-13: Images should have alternate text (alt text)
(MD045, no-alt-text)
16-16: Images should have alternate text (alt text)
(MD045, no-alt-text)
19-19: Images should have alternate text (alt text)
(MD045, no-alt-text)
31-31: Hard tabs
Column: 1
(MD010, no-hard-tabs)
32-32: Hard tabs
Column: 1
(MD010, no-hard-tabs)
33-33: Hard tabs
Column: 1
(MD010, no-hard-tabs)
35-35: Hard tabs
Column: 1
(MD010, no-hard-tabs)
36-36: Hard tabs
Column: 1
(MD010, no-hard-tabs)
37-37: Hard tabs
Column: 1
(MD010, no-hard-tabs)
39-39: Hard tabs
Column: 1
(MD010, no-hard-tabs)
40-40: Hard tabs
Column: 1
(MD010, no-hard-tabs)
41-41: Hard tabs
Column: 1
(MD010, no-hard-tabs)
43-43: Hard tabs
Column: 1
(MD010, no-hard-tabs)
44-44: Hard tabs
Column: 1
(MD010, no-hard-tabs)
45-45: Hard tabs
Column: 1
(MD010, no-hard-tabs)
46-46: Hard tabs
Column: 1
(MD010, no-hard-tabs)
48-48: Hard tabs
Column: 1
(MD010, no-hard-tabs)
49-49: Hard tabs
Column: 1
(MD010, no-hard-tabs)
50-50: Hard tabs
Column: 1
(MD010, no-hard-tabs)
52-52: Hard tabs
Column: 1
(MD010, no-hard-tabs)
53-53: Hard tabs
Column: 1
(MD010, no-hard-tabs)
54-54: Hard tabs
Column: 1
(MD010, no-hard-tabs)
56-56: Hard tabs
Column: 1
(MD010, no-hard-tabs)
57-57: Hard tabs
Column: 1
(MD010, no-hard-tabs)
59-59: Hard tabs
Column: 1
(MD010, no-hard-tabs)
60-60: Hard tabs
Column: 1
(MD010, no-hard-tabs)
62-62: Hard tabs
Column: 1
(MD010, no-hard-tabs)
63-63: Hard tabs
Column: 1
(MD010, no-hard-tabs)
64-64: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Images should have alternate text (alt text)
(MD045, no-alt-text)
72-72: Images should have alternate text (alt text)
(MD045, no-alt-text)
73-73: Images should have alternate text (alt text)
(MD045, no-alt-text)
74-74: Images should have alternate text (alt text)
(MD045, no-alt-text)
75-75: Images should have alternate text (alt text)
(MD045, no-alt-text)
76-76: Images should have alternate text (alt text)
(MD045, no-alt-text)
77-77: Images should have alternate text (alt text)
(MD045, no-alt-text)
78-78: Images should have alternate text (alt text)
(MD045, no-alt-text)
79-79: Images should have alternate text (alt text)
(MD045, no-alt-text)
80-80: Images should have alternate text (alt text)
(MD045, no-alt-text)
81-81: Images should have alternate text (alt text)
(MD045, no-alt-text)
82-82: Images should have alternate text (alt text)
(MD045, no-alt-text)
84-84: Images should have alternate text (alt text)
(MD045, no-alt-text)
85-85: Images should have alternate text (alt text)
(MD045, no-alt-text)
86-86: Images should have alternate text (alt text)
(MD045, no-alt-text)
87-87: Images should have alternate text (alt text)
(MD045, no-alt-text)
88-88: Images should have alternate text (alt text)
(MD045, no-alt-text)
89-89: Images should have alternate text (alt text)
(MD045, no-alt-text)
90-90: Images should have alternate text (alt text)
(MD045, no-alt-text)
91-91: Images should have alternate text (alt text)
(MD045, no-alt-text)
92-92: Images should have alternate text (alt text)
(MD045, no-alt-text)
93-93: Images should have alternate text (alt text)
(MD045, no-alt-text)
94-94: Images should have alternate text (alt text)
(MD045, no-alt-text)
95-95: Images should have alternate text (alt text)
(MD045, no-alt-text)
96-96: Images should have alternate text (alt text)
(MD045, no-alt-text)
97-97: Images should have alternate text (alt text)
(MD045, no-alt-text)
98-98: Images should have alternate text (alt text)
(MD045, no-alt-text)
99-99: Images should have alternate text (alt text)
(MD045, no-alt-text)
100-100: Images should have alternate text (alt text)
(MD045, no-alt-text)
101-101: Images should have alternate text (alt text)
(MD045, no-alt-text)
102-102: Images should have alternate text (alt text)
(MD045, no-alt-text)
README.md
82-82: Images should have alternate text (alt text)
(MD045, no-alt-text)
102-102: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (2)
firestore/firestore.go (2)
54-56: Verify panic usage aligns with library design expectations.The
Newfunction panics on client creation failure and reset errors. While this is acceptable for critical initialization failures, it prevents callers from gracefully handling errors (e.g., retrying with different credentials, logging and continuing). Consider whether returning(*Storage, error)would provide better flexibility for library users.If panic is intentional for fail-fast initialization, ensure it's clearly documented in the API documentation and examples so users can wrap calls appropriately.
Also applies to: 65-67
106-114: Async deletion silently ignores errors.Expired documents are deleted asynchronously in a fire-and-forget goroutine. While this is acceptable for performance (avoiding blocking reads), deletion failures are silently ignored. Consider whether observability (e.g., logging, metrics) should be added for tracking failed cleanup operations, especially if expired documents accumulate due to persistent errors.
If silent failure is acceptable, ensure this behavior is documented so users understand that expired documents might persist temporarily if deletion fails.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
firestore/README.md (1)
22-37: Signatures block now matches the actual public APIConstructor return type corrected to
*Storage; looks good and consistent.firestore/firestore.go (1)
212-214: Check and propagate BulkWriter.Flush errorA failed flush means documents weren’t deleted, but the method returns nil. Propagate the error.
Apply this diff:
- bulkWriter.Flush() - return nil + if err := bulkWriter.Flush(); err != nil { + return fmt.Errorf("failed to flush bulk deletions: %w", err) + } + return nil
🧹 Nitpick comments (9)
firestore/README.md (2)
57-89: Use an import alias to avoid confusion with the Cloud Firestore package nameMany projects also import
cloud.google.com/go/firestoreasfirestore. Using an alias for the storage driver avoids collisions and improves clarity across examples.Update examples to use an alias:
-```go -import "github.com/gofiber/storage/firestore/v2" +```go +import firestorage "github.com/gofiber/storage/firestore/v2"@@
-// Initialize with Application Default Credentials
-store := firestore.New(firestore.Config{
+// Initialize with Application Default Credentials
+store := firestorage.New(firestorage.Config{
ProjectID: "my-gcp-project",
})
@@
-// Initialize with service account JSON file
-store := firestore.New(firestore.Config{
+// Initialize with service account JSON file
+store := firestorage.New(firestorage.Config{
ProjectID: "my-gcp-project",
CredentialsPath: "/path/to/service-account-key.json",
Collection: "sessions",
})
@@
-// Initialize with embedded credentials JSON
-store := firestore.New(firestore.Config{
+// Initialize with embedded credentials JSON
+store := firestorage.New(firestorage.Config{
ProjectID: "my-gcp-project",
Credentials:{"type": "service_account", ...},
})
@@
-// Initialize with custom timeout
-store := firestore.New(firestore.Config{
+// Initialize with custom timeout
+store := firestorage.New(firestorage.Config{
ProjectID: "my-gcp-project",
Collection: "fiber_storage",
RequestTimeout: 10 * time.Second,
Reset: false,
})--- `66-87`: **Replace hard tabs with spaces in code blocks to satisfy markdownlint (MD010)** Tabs are flagged across code blocks by markdownlint-cli2. Replace with spaces for consistent rendering. As per static analysis hints Also applies to: 97-105, 115-145, 151-155 </blockquote></details> <details> <summary>firestore/firestore_test.go (2)</summary><blockquote> `29-36`: **Add startup timeout and wait for listening port to reduce flakiness** Use a bounded context and an explicit port wait strategy. Apply this diff: ```diff - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() @@ - WaitingFor: wait.ForLog("Dev App Server is now running"), + WaitingFor: wait.ForListeningPort(firestorePort).WithStartupTimeout(60 * time.Second),As per retrieved learnings on testcontainers-go
55-55: Restore FIRESTORE_EMULATOR_HOST after test to avoid cross-test leakagePreserve the original env var and restore it in cleanup.
Apply this diff:
- os.Setenv("FIRESTORE_EMULATOR_HOST", host+":"+port.Port()) + prevVal, hadPrev := os.LookupEnv("FIRESTORE_EMULATOR_HOST") + os.Setenv("FIRESTORE_EMULATOR_HOST", host+":"+port.Port()) + t.Cleanup(func() { + if hadPrev { + _ = os.Setenv("FIRESTORE_EMULATOR_HOST", prevVal) + } else { + _ = os.Unsetenv("FIRESTORE_EMULATOR_HOST") + } + })firestore/firestore.go (5)
108-113: Async delete on expired keys: consider handling errors or bounding goroutinesCurrent goroutine ignores delete errors and may spawn many goroutines under load. Consider a best-effort synchronous delete on the same request context, or add lightweight logging/metrics and a bounded worker.
Example minimal change (sync on same timeout-bound context):
- // Delete expired document asynchronously - go func() { - delCtx, cancel := context.WithTimeout(context.Background(), s.timeout) - defer cancel() - _, _ = s.client.Collection(s.collection).Doc(key).Delete(delCtx) - }() + // Best-effort delete expired document + _, _ = s.client.Collection(s.collection).Doc(key).Delete(ctx)
31-36: Avoid storing the key inside the document (it’s already the document ID)The
Keyfield duplicates the Firestore document ID and costs extra storage. You can drop it for leaner documents.Apply this diff:
type document struct { - Key string `firestore:"k"` Value []byte `firestore:"v"` ExpiresAt time.Time `firestore:"exp_at,omitempty"` } @@ - doc := document{ - Key: key, - Value: val, - } + doc := document{Value: val}Also applies to: 138-145
2-2: Don’t pin client version in commentsThe import isn’t versioned in code; this will drift. Remove the specific version to avoid stale docs.
24-25: Fix struct comment wordingThis declares a struct, not an interface.
Apply this edit:
-// Storage interface that is implemented by storage providers +// Storage is a Firestore-backed implementation of the Fiber storage interface.
229-240: Allow customizing timeout when using NewFromConnectionCurrently hardcodes
ConfigDefault.RequestTimeout. Consider accepting options or a config to settimeout.Example:
// NewFromConnectionWithConfig(client *firestore.Client, cfg Config) *StorageOr add a setter:
func (s *Storage) WithTimeout(d time.Duration) *Storage { s.timeout = d; return s }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
firestore/README.md(1 hunks)firestore/config.go(1 hunks)firestore/firestore.go(1 hunks)firestore/firestore_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- firestore/config.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
firestore/firestore_test.go
🧬 Code graph analysis (2)
firestore/firestore_test.go (2)
firestore/firestore.go (2)
Storage(25-29)New(39-71)firestore/config.go (1)
Config(8-42)
firestore/firestore.go (1)
firestore/config.go (2)
Config(8-42)ConfigDefault(45-49)
🪛 LanguageTool
firestore/README.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...n) ### Table of Contents - Signatures - Installation - [Examples...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...Signatures](#signatures) - Installation - Examples - Config...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...Installation](#installation) - Examples - Config - [Default Config](#def...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...ation) - Examples - Config - Default Config ### Si...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
firestore/README.md
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 1
(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1
(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1
(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
142-142: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
144-144: Hard tabs
Column: 1
(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1
(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
firestore/README.md (1)
63-109: Replace hard tabs with spaces in code blocks.The code blocks contain hard tabs for indentation (lines 66, 71–73, 78–79, 84–87, 97–99, 105, 115–117, 119–122, 124–128, 130–134, 136–139, 141–144, 152–154), which violates the markdownlint MD010 rule. Convert all hard tabs to spaces for consistency with markdown formatting standards.
#!/bin/bash # Identify and count hard tabs in the file grep -P '\t' firestore/README.md | wc -lAs a suggested approach, you may use a sed or similar tool to replace tabs with spaces:
sed -i 's/\t/ /g' firestore/README.mdOr manually review and replace tabs in the code blocks with 2 or 4 spaces per indent level.
firestore/firestore_test.go (3)
3-12: Make emulator startup + endpoint wiring more robust
- Avoid brittle log matching; wait on the listening port instead.
- Build FIRESTORE_EMULATOR_HOST safely (IPv6‑safe) and handle Setenv error.
Apply:
import ( "context" + "net" "os" "testing" "time" + "github.com/docker/go-connections/nat" "github.com/stretchr/testify/require" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/wait" ) @@ req := testcontainers.ContainerRequest{ Image: img, ExposedPorts: []string{firestorePort}, Cmd: []string{"gcloud", "beta", "emulators", "firestore", "start", "--host-port=0.0.0.0:8080"}, - WaitingFor: wait.ForLog("Dev App Server is now running"), + WaitingFor: wait.ForListeningPort(nat.Port(firestorePort)).WithStartupTimeout(60 * time.Second), } @@ - os.Setenv("FIRESTORE_EMULATOR_HOST", host+":"+port.Port()) + endpoint := net.JoinHostPort(host, port.Port()) + if err := os.Setenv("FIRESTORE_EMULATOR_HOST", endpoint); err != nil { + t.Fatalf("failed to set FIRESTORE_EMULATOR_HOST: %v", err) + }Optional: If your Go version exposes TB.Setenv, prefer:
t.Setenv("FIRESTORE_EMULATOR_HOST", endpoint)Can you confirm the project’s minimum Go version supports testing.TB.Setenv?
Also applies to: 31-36, 57-58
29-30: Add timeouts to container create/terminate to prevent hangsLong‑running Docker operations can stall CI. Bound them with deadlines.
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() @@ t.Cleanup(func() { if c != nil { - if err := c.Terminate(ctx); err != nil { + ctxTerm, cancelTerm := context.WithTimeout(context.Background(), 15*time.Second) + defer cancelTerm() + if err := c.Terminate(ctxTerm); err != nil { t.Fatalf("failed to terminate container: %s", err) } } })Also applies to: 42-48
161-166: Deflake expiration test: replace fixed sleep with EventuallyClock/skew and emulator timing make fixed sleeps flaky.
- time.Sleep(1100 * time.Millisecond) - - result, err := testStore.Get(key) - require.NoError(t, err) - require.Zero(t, len(result), "Key should have expired") + require.Eventually(t, func() bool { + b, err := testStore.Get(key) + return err == nil && len(b) == 0 + }, 5*time.Second, 50*time.Millisecond, "Key should have expired")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
firestore/README.md(1 hunks)firestore/firestore_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
firestore/firestore_test.go
🧬 Code graph analysis (1)
firestore/firestore_test.go (2)
firestore/firestore.go (2)
Storage(25-29)New(39-71)firestore/config.go (1)
Config(8-42)
🪛 LanguageTool
firestore/README.md
[grammar] ~16-~16: There might be a mistake here.
Context: ...n) ### Table of Contents - Signatures - Installation - [Examples...
(QB_NEW_EN)
[grammar] ~17-~17: There might be a mistake here.
Context: ...Signatures](#signatures) - Installation - Examples - Config...
(QB_NEW_EN)
[grammar] ~18-~18: There might be a mistake here.
Context: ...Installation](#installation) - Examples - Config - [Default Config](#def...
(QB_NEW_EN)
[grammar] ~19-~19: There might be a mistake here.
Context: ...ation) - Examples - Config - Default Config ### Si...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
firestore/README.md
66-66: Hard tabs
Column: 1
(MD010, no-hard-tabs)
71-71: Hard tabs
Column: 1
(MD010, no-hard-tabs)
72-72: Hard tabs
Column: 1
(MD010, no-hard-tabs)
73-73: Hard tabs
Column: 1
(MD010, no-hard-tabs)
78-78: Hard tabs
Column: 1
(MD010, no-hard-tabs)
79-79: Hard tabs
Column: 1
(MD010, no-hard-tabs)
84-84: Hard tabs
Column: 1
(MD010, no-hard-tabs)
85-85: Hard tabs
Column: 1
(MD010, no-hard-tabs)
86-86: Hard tabs
Column: 1
(MD010, no-hard-tabs)
87-87: Hard tabs
Column: 1
(MD010, no-hard-tabs)
97-97: Hard tabs
Column: 1
(MD010, no-hard-tabs)
98-98: Hard tabs
Column: 1
(MD010, no-hard-tabs)
99-99: Hard tabs
Column: 1
(MD010, no-hard-tabs)
105-105: Hard tabs
Column: 1
(MD010, no-hard-tabs)
115-115: Hard tabs
Column: 1
(MD010, no-hard-tabs)
116-116: Hard tabs
Column: 1
(MD010, no-hard-tabs)
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
119-119: Hard tabs
Column: 1
(MD010, no-hard-tabs)
120-120: Hard tabs
Column: 1
(MD010, no-hard-tabs)
121-121: Hard tabs
Column: 1
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
134-134: Hard tabs
Column: 1
(MD010, no-hard-tabs)
136-136: Hard tabs
Column: 1
(MD010, no-hard-tabs)
137-137: Hard tabs
Column: 1
(MD010, no-hard-tabs)
138-138: Hard tabs
Column: 1
(MD010, no-hard-tabs)
139-139: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
142-142: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
144-144: Hard tabs
Column: 1
(MD010, no-hard-tabs)
152-152: Hard tabs
Column: 1
(MD010, no-hard-tabs)
153-153: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
🔇 Additional comments (2)
firestore/README.md (1)
1-163: Documentation is comprehensive and addresses prior feedback.The README provides clear API signatures, well-structured examples covering multiple initialization patterns (ADC, credentials file, embedded credentials, custom timeout), and complete Config documentation. The credentials precedence note (line 12) is clear. The example using an existing Firestore client (lines 95–109) now includes all necessary imports and context initialization.
All identified issues from prior review cycles appear resolved:
- Constructor signature (line 25) correctly shows
*Storagereturn type.- Context example (line 102) now defines
ctx := context.Background().- Credentials note (line 12) clarifies precedence and defaults.
firestore/firestore_test.go (1)
66-156: Solid, comprehensive integration coverageCRUD, context‑cancel, reset, close, and conn paths look good and read cleanly.
Also applies to: 168-280
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
firestore/firestore_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
firestore/firestore_test.go
🧬 Code graph analysis (1)
firestore/firestore_test.go (2)
firestore/firestore.go (2)
Storage(25-29)New(39-71)firestore/config.go (1)
Config(8-42)
🔇 Additional comments (4)
firestore/firestore_test.go (4)
1-19: LGTM!The package structure, imports, and constants are well-organized. The environment variable override for the Firestore image provides good flexibility for testing.
42-48: LGTM! Nil check prevents panic.The cleanup function now properly guards against nil container references, which resolves the critical issue flagged in the previous review.
66-280: LGTM! Comprehensive test coverage.The test suite thoroughly covers:
- Basic CRUD operations (Set, Get, Delete)
- Context-aware variants with cancellation scenarios
- Expiration semantics
- Reset functionality
- Connection management (Close, Conn)
All tests follow consistent patterns and use appropriate assertions.
321-327: LGTM! Error handling improved.The benchmark now captures and validates both
errSetanderrDel, which addresses the concern from the previous review about hiding failures.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
firestore/firestore_test.go (2)
75-289: Comprehensive test coverage with proper isolation.The test suite thoroughly covers CRUD operations, context cancellation, expiration behavior, and connection management. Each test properly creates an isolated store and cleans up resources.
Optional: Consider reducing expiration test sleep time.
Line 170 sleeps for 1100ms to test a 1-second expiration. While this is safe, using a shorter TTL (e.g., 100ms expiration with 150ms sleep) would speed up test execution without sacrificing reliability.
var ( key = "john" val = []byte("doe") - exp = 1 * time.Second + exp = 100 * time.Millisecond ) testStore := newTestStore(t) defer testStore.Close() err := testStore.Set(key, val, exp) require.NoError(t, err) - time.Sleep(1100 * time.Millisecond) + time.Sleep(150 * time.Millisecond)
291-321: Consider consistent error handling across benchmarks.The
Benchmark_Firestore_SetAndDeletecorrectly captures errors per iteration, butBenchmark_Firestore_SetandBenchmark_Firestore_Getonly check the final error. While checking only the last error is a common pattern, the inconsistency with the already-fixed SetAndDelete benchmark suggests these could follow the same approach for uniformity.Optional: Capture errors explicitly in Set and Get benchmarks for consistency.
Apply similar error handling as
Benchmark_Firestore_SetAndDelete:For
Benchmark_Firestore_Set:b.ReportAllocs() b.ResetTimer() var err error for i := 0; i < b.N; i++ { err = testStore.Set("john", []byte("doe"), 0) } - require.NoError(b, err)For
Benchmark_Firestore_Get:b.ReportAllocs() b.ResetTimer() + var err error for i := 0; i < b.N; i++ { - _, err = testStore.Get("john") + _, err = testStore.Get("john") } - require.NoError(b, err)Note: The current pattern is acceptable; this suggestion is purely for consistency with the corrected SetAndDelete benchmark.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
firestore/firestore_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-20T23:52:03.362Z
Learnt from: mdelapenya
PR: gofiber/storage#1665
File: cassandra/cassandra_test.go:35-38
Timestamp: 2025-04-20T23:52:03.362Z
Learning: In testcontainers-go, calling `testcontainers.CleanupContainer(t, c)` before checking the error from container creation is safe due to the implementation details of the library. The CleanupContainer function handles nil or partially initialized containers gracefully.
Applied to files:
firestore/firestore_test.go
🧬 Code graph analysis (1)
firestore/firestore_test.go (2)
firestore/firestore.go (2)
Storage(25-29)New(39-71)firestore/config.go (1)
Config(8-42)
🔇 Additional comments (2)
firestore/firestore_test.go (2)
1-19: LGTM! Well-organized test setup.The imports, constants, and test configuration are appropriate for integration testing with testcontainers. The environment variable override pattern provides good flexibility for CI/CD environments.
21-73: Excellent test helper with proper resource management.The container lifecycle, environment variable handling, and cleanup logic correctly address all previous review comments. The nil-check before
Terminateprevents panics, and the environment variable restoration prevents test pollution.
1 participant
Hello Fiber Team,
This pull request introduces a new Firestore storage driver for the Fiber storage package.
Firestore is Google Cloud’s NoSQL document database. This driver allows using Firestore as a storage backend through the standard Fiber Storage interface.
Thanks for the reviews 🙏
Summary by CodeRabbit
New Features
Documentation
Tests