Add support for creating namespaced databases #294
Conversation
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DBManager
participant DB
Client->>DBManager: NewNamespacedDB(dsn, namespace)
DBManager->>DBManager: parseConfig(dsn)
DBManager->>DB: createNamespace(namespace)
DB->>DBManager: Namespace created or exists
DBManager->>DB: newPGXDB(config)
DB->>DBManager: Database connection established
DBManager->>Client: Return DB connection
Warning Rate limit exceeded@neekolas has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
pkg/db/pgx_test.go (1)
16-22: Consider increasing timeouts for CI environments.The 1-second timeouts might be too aggressive for CI environments where database operations could be slower.
Consider:
- Making timeouts configurable via environment variables
- Using longer default timeouts (e.g., 5-10 seconds)
- Adding retry logic for transient failures
pkg/testutils/store.go (2)
16-17: Consider making database configuration more flexible and secureWhile making these constants public supports the namespaced databases feature, exposing hardcoded credentials and connection details through exported constants raises some concerns:
- Hardcoded credentials in source code
- Fixed port number that might not match all test environments
- Disabled SSL mode in the connection string
Consider:
- Using environment variables for credentials and connection details
- Adding documentation about the expected test database configuration
- Making SSL mode configurable for different test environments
Line range hint
1-94: Consider enhancing test database managementThe current implementation provides a solid foundation for test database management, but as we add support for namespaced databases, consider these architectural improvements:
- Add a configuration struct to centralize test database settings
- Implement connection pooling for test databases to improve performance
- Add parallel test support with database name prefixes to prevent conflicts
- Consider adding cleanup timeout handling for long-running tests
Would you like help implementing any of these improvements?
pkg/db/pgx.go (1)
15-25: Consider using context deadlines instead of custom timing logicCurrently,
waitUntilDBReadymanually calculates the timeout usingtime.Now().Add(waitTime). Using context deadlines can simplify the code and ensure better propagation of cancellation signals.Apply this diff to refactor the function:
func waitUntilDBReady(ctx context.Context, db *pgxpool.Pool, waitTime time.Duration) error { - waitUntil := time.Now().Add(waitTime) + ctx, cancel := context.WithTimeout(ctx, waitTime) + defer cancel() - err := db.Ping(ctx) + for { + err := db.Ping(ctx) + if err == nil { + return nil + } + + select { + case <-time.After(3 * time.Second): + continue + case <-ctx.Done(): + return ctx.Err() + } + } - - for err != nil && time.Now().Before(waitUntil) { - time.Sleep(3 * time.Second) - err = db.Ping(ctx) - } - return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/db/pgx.go(2 hunks)pkg/db/pgx_test.go(1 hunks)pkg/testutils/store.go(2 hunks)
🔇 Additional comments (4)
pkg/db/pgx_test.go (1)
1-10: LGTM!
The package declaration and imports are appropriate for a database test file.
pkg/testutils/store.go (2)
31-31: LGTM!
The usage of the exported constants is correct and consistent.
39-39: Verify database name sanitization
While using random strings for database names helps prevent conflicts, we should ensure proper validation of the generated name to prevent potential SQL injection vulnerabilities.
Let's check the implementation of RandomStringLower:
Additionally, consider adding a validation regex for the database name before using it in the SQL query.
✅ Verification successful
Let me check the implementation of RandomString and analyze how the database name is used.
Let me check the definition of letterRunes and look for any database creation code.
Let me check one final thing to see the complete random string generation implementation.
Database name generation is secure against SQL injection
The implementation is safe because:
- The random database name is generated using only alphanumeric characters (a-z, A-Z) from a predefined set of
letterRunes - The database creation is properly quoted in both implementations:
CREATE DATABASE " + dbNamein testutils/store.goCREATE DATABASE "%s"with proper formatting in pgx.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find and analyze the implementation of RandomStringLower
ast-grep --pattern 'func RandomStringLower($$$) $_'
Length of output: 324
Script:
#!/bin/bash
# Find the implementation of RandomString
ast-grep --pattern 'func RandomString($$$) $_'
# Check how the database name is used in openDB
ast-grep --pattern 'func openDB($$$) $_'
Length of output: 1114
Script:
#!/bin/bash
# Find the definition of letterRunes
ast-grep --pattern 'letterRunes := $_'
ast-grep --pattern 'letterRunes = $_'
ast-grep --pattern 'var letterRunes $_'
# Look for database creation code
rg -A 5 "CREATE DATABASE"
Length of output: 2313
Script:
#!/bin/bash
# Find the complete file content to see letterRunes declaration
cat pkg/testutils/random.go
Length of output: 1239
pkg/db/pgx.go (1)
7-7: Unused import: "strings" package
The "strings" package is imported but may no longer be necessary after refactoring error handling.
Run the following script to check if "strings" is still used in the codebase:
If no usages are found, you can safely remove the import:
import (
"context"
"database/sql"
"fmt"
- "strings"
"time"
"github.com/jackc/pgx/v5/pgxpool"
"github.com/jackc/pgx/v5/stdlib"
"github.com/xmtp/xmtpd/pkg/migrations"
)
neekolas
force-pushed
the
11-26-add_support_for_creating_namespaced_databases
branch
3 times, most recently
from
21e80d5
to
6e15894
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
pkg/db/pgx_test.go (1)
12-35: Consider test improvements for robustnessA few suggestions to enhance the test:
- The hardcoded 1-second timeouts might be too aggressive for slower systems or high-load environments
- The test could benefit from parallel execution support
Consider these improvements:
func TestNamespacedDB(t *testing.T) { + t.Parallel() startingDsn := testutils.LocalTestDBDSNPrefix + "/foo" + testutils.LocalTestDBDSNSuffix newDBName := testutils.RandomString(32) // Create namespaced DB namespacedDB, err := NewNamespacedDB( context.Background(), startingDsn, newDBName, - time.Second, - time.Second, + 5*time.Second, // Increased timeout for slower systems + 5*time.Second, )pkg/db/pgx.go (1)
15-25: Consider enhancing the retry mechanismThe current implementation could be improved in several ways:
- Use exponential backoff instead of a fixed 3-second interval
- Add logging for retry attempts
- Provide more context in the returned error
func waitUntilDBReady(ctx context.Context, db *pgxpool.Pool, waitTime time.Duration) error { waitUntil := time.Now().Add(waitTime) + retryInterval := time.Second err := db.Ping(ctx) for err != nil && time.Now().Before(waitUntil) { - time.Sleep(3 * time.Second) + log.Printf("Database not ready, retrying in %v: %v", retryInterval, err) + time.Sleep(retryInterval) + retryInterval = min(retryInterval*2, 10*time.Second) err = db.Ping(ctx) } - return err + if err != nil { + return fmt.Errorf("database not ready after %v: %w", waitTime, err) + } + return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/db/pgx.go(2 hunks)pkg/db/pgx_test.go(1 hunks)pkg/testutils/store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/testutils/store.go
🔇 Additional comments (5)
pkg/db/pgx_test.go (1)
1-11: LGTM!
The imports are appropriate and well-organized for database testing purposes.
pkg/db/pgx.go (4)
Line range hint 27-37: LGTM!
The function is well-structured with clear single responsibility.
39-56: LGTM!
The refactoring improves modularity by separating configuration parsing from connection establishment.
58-91: Skip duplicate comments
The SQL injection and error handling issues identified in the past reviews are still valid and should be addressed.
130-135: LGTM!
The changes are consistent with the overall refactoring pattern.
neekolas
force-pushed
the
11-26-add_support_for_creating_namespaced_databases
branch
from
6e15894
to
4e6c14c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/db/pgx.go (2)
18-28: Enhance retry mechanism with exponential backoffThe current implementation uses a fixed 3-second sleep interval which might not be optimal for all scenarios. Consider implementing exponential backoff for more efficient retries.
+var ( + initialRetryInterval = 1 * time.Second + maxRetryInterval = 10 * time.Second +) func waitUntilDBReady(ctx context.Context, db *pgxpool.Pool, waitTime time.Duration) error { waitUntil := time.Now().Add(waitTime) + retryInterval := initialRetryInterval err := db.Ping(ctx) for err != nil && time.Now().Before(waitUntil) { - time.Sleep(3 * time.Second) + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(retryInterval): + } err = db.Ping(ctx) + // Exponential backoff with maximum interval + retryInterval = time.Duration( + math.Min(float64(retryInterval*2), float64(maxRetryInterval)), + ) } return err }
116-145: Add documentation about migration behaviorThe function runs migrations but this behavior isn't documented in the function comment. Users should be aware that creating a new namespaced database will automatically run migrations.
-// Creates a new database with the given namespace if it doesn't exist and returns the full DSN for the new database. +// NewNamespacedDB creates a new database with the given namespace if it doesn't exist, +// runs all pending migrations, and returns a database connection. +// +// The function will: +// 1. Create a new database if it doesn't exist +// 2. Establish a connection to the new database +// 3. Run all pending migrations +// 4. Return the database connection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/db/pgx.go(2 hunks)pkg/db/pgx_test.go(1 hunks)pkg/testutils/store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/db/pgx_test.go
- pkg/testutils/store.go
🔇 Additional comments (4)
pkg/db/pgx.go (4)
Line range hint 30-40: LGTM!
The function is well-structured, handles errors appropriately, and has a single responsibility.
42-59: LGTM!
The refactored function is well-structured with improved modularity by accepting a config object and using the new waitUntilDBReady function.
77-114: Previous security and error handling concerns remain unaddressed
The previously identified issues regarding SQL injection vulnerability and error handling are still present in this function.
Please refer to the previous review comments for detailed fixes regarding:
- SQL injection vulnerability when creating database
- Error handling using error strings instead of PostgreSQL error codes
153-157: LGTM!
The changes appropriately utilize the new parseConfig function, maintaining consistency with the modular approach.
neekolas
force-pushed
the
11-26-add_support_for_creating_namespaced_databases
branch
3 times, most recently
from
de7bd67
to
b97c1ee
Compare
neekolas
force-pushed
the
11-26-add_support_for_creating_namespaced_databases
branch
from
b97c1ee
to
f1897ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
pkg/db/pgx_test.go (2)
12-35: Enhance test coverage with database state verificationWhile the test verifies the database name, it should also validate:
- Database state after creation
- Migration status (if applicable)
- Basic CRUD operations
func TestNamespacedDB(t *testing.T) { startingDsn := testutils.LocalTestDBDSNPrefix + "/foo" + testutils.LocalTestDBDSNSuffix newDBName := "xmtp_" + testutils.RandomString(24) // Create namespaced DB namespacedDB, err := NewNamespacedDB( context.Background(), startingDsn, newDBName, time.Second, time.Second, ) t.Cleanup(func() { namespacedDB.Close() }) require.NoError(t, err) result, err := namespacedDB.Query("SELECT current_database();") require.NoError(t, err) defer result.Close() require.True(t, result.Next()) var dbName string err = result.Scan(&dbName) require.NoError(t, err) require.Equal(t, newDBName, dbName) + + // Verify database state + _, err = namespacedDB.Exec(` + CREATE TABLE test_table ( + id SERIAL PRIMARY KEY, + name TEXT + ) + `) + require.NoError(t, err) + + // Test basic CRUD operations + _, err = namespacedDB.Exec("INSERT INTO test_table (name) VALUES ($1)", "test") + require.NoError(t, err) + + var count int + err = namespacedDB.QueryRow("SELECT COUNT(*) FROM test_table").Scan(&count) + require.NoError(t, err) + require.Equal(t, 1, count) }
1-85: Consider organizing tests into groups using t.RunThe test file would benefit from organizing related tests into groups using
t.Run. This improves readability and allows for better test filtering and parallel execution.+func TestNamespacedDB_Creation(t *testing.T) { + t.Run("successful creation", TestNamespacedDB) + t.Run("repeated creation", TestNamespaceRepeat) + t.Run("invalid name", TestNamespacedDBInvalidName) + t.Run("invalid dsn", TestNamespacedDBInvalidDSN) + // Add new error scenario tests here +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
pkg/db/pgx.go(2 hunks)pkg/db/pgx_test.go(1 hunks)pkg/testutils/store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/db/pgx.go
- pkg/testutils/store.go
🔇 Additional comments (1)
pkg/db/pgx_test.go (1)
37-63: 🛠️ Refactor suggestion
Verify database state consistency between connections
The test should verify that both connections point to the same database and maintain consistent state.
require.NoError(t, err)
require.NotNil(t, db2)
t.Cleanup(func() { db2.Close() })
+
+ // Verify both connections point to the same database
+ var dbName1, dbName2 string
+ err = db1.QueryRow("SELECT current_database()").Scan(&dbName1)
+ require.NoError(t, err)
+ err = db2.QueryRow("SELECT current_database()").Scan(&dbName2)
+ require.NoError(t, err)
+ require.Equal(t, dbName1, dbName2)
+
+ // Verify database state consistency
+ _, err = db1.Exec("CREATE TABLE test_table (id SERIAL PRIMARY KEY)")
+ require.NoError(t, err)
+ _, err = db2.Exec("INSERT INTO test_table DEFAULT VALUES")
+ require.NoError(t, err)
+ var count int
+ err = db1.QueryRow("SELECT COUNT(*) FROM test_table").Scan(&count)
+ require.NoError(t, err)
+ require.Equal(t, 1, count)
}| func TestNamespacedDBInvalidName(t *testing.T) { | ||
| _, err := NewNamespacedDB( | ||
| context.Background(), | ||
| testutils.LocalTestDBDSNPrefix+"/foo"+testutils.LocalTestDBDSNSuffix, | ||
| "invalid/name", | ||
| time.Second, | ||
| time.Second, | ||
| ) | ||
| require.Error(t, err) | ||
| } | ||
|
|
||
| func TestNamespacedDBInvalidDSN(t *testing.T) { | ||
| _, err := NewNamespacedDB( | ||
| context.Background(), | ||
| "invalid-dsn", | ||
| "dbname", | ||
| time.Second, | ||
| time.Second, | ||
| ) | ||
| require.Error(t, err) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add more error scenario tests
While the current error tests are good, consider adding these scenarios:
- Connection timeout
- Permission issues
- Concurrent database creation attempts
+func TestNamespacedDBTimeout(t *testing.T) {
+ _, err := NewNamespacedDB(
+ context.Background(),
+ testutils.LocalTestDBDSNPrefix+"/foo"+testutils.LocalTestDBDSNSuffix,
+ "timeout_db",
+ time.Nanosecond, // Very short timeout
+ time.Nanosecond,
+ )
+ require.Error(t, err)
+}
+
+func TestNamespacedDBConcurrent(t *testing.T) {
+ dbName := "xmtp_" + testutils.RandomString(24)
+ dsn := testutils.LocalTestDBDSNPrefix + "/foo" + testutils.LocalTestDBDSNSuffix
+
+ // Create multiple connections concurrently
+ const numConnections = 5
+ errChan := make(chan error, numConnections)
+ for i := 0; i < numConnections; i++ {
+ go func() {
+ db, err := NewNamespacedDB(
+ context.Background(),
+ dsn,
+ dbName,
+ time.Second,
+ time.Second,
+ )
+ if err == nil {
+ defer db.Close()
+ }
+ errChan <- err
+ }()
+ }
+
+ // Collect results
+ var successCount int
+ for i := 0; i < numConnections; i++ {
+ if err := <-errChan; err == nil {
+ successCount++
+ }
+ }
+
+ // At least one should succeed, others might fail due to concurrent creation
+ require.GreaterOrEqual(t, successCount, 1)
+}Committable suggestion skipped: line range outside the PR's diff.
TL;DR
Added support for namespaced databases with automatic creation and migration capabilities.
What changed?
NewNamespacedDBfunction to create and manage isolated database instancescreateNamespacefunction to handle database creationwaitUntilDBReadyfor connection readiness checksHow to test?
TestNamespacedDBWhy make this change?
This change enables better isolation between different database instances, allowing for separate namespaces per instance. This is particularly useful for testing scenarios and multi-tenant applications where data separation is crucial.
Ticket
#225
Summary by CodeRabbit