-
Notifications
You must be signed in to change notification settings - Fork 7
feat: postgres dialect suppport #82
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?
Changes from 10 commits
528bcbb
0a19c18
ee66c35
cff3399
aab9030
ae33034
b627e0b
acfecf2
540272e
3a72308
903cca0
9936406
8abfddc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| package ent_test | ||
|
|
||
| import ( | ||
| "database/sql" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
|
|
@@ -422,6 +423,9 @@ func (as *annotationsSuite) TestBackend_RemoveDocumentAnnotations() { | |
| ctx := as.Context() | ||
|
|
||
| as.Run(name, func() { | ||
| // Clear any existing annotations for this test to ensure clean state | ||
| as.Require().NoError(as.ClearDocumentAnnotations(documentID, annotationName)) | ||
|
|
||
| for _, value := range []string{"test-value-1", "test-value-2", "test-value-3"} { | ||
| _, err = as.Backend.Ent().ExecContext(ctx, query, uniqueID, false, annotationName, value) | ||
| as.Require().NoError(err) | ||
|
|
@@ -475,6 +479,9 @@ func (as *annotationsSuite) TestBackend_RemoveNodeAnnotations() { | |
| ctx := as.Context() | ||
|
|
||
| as.Run(name, func() { | ||
| // Clear any existing annotations for this test to ensure clean state | ||
| as.Require().NoError(as.ClearNodeAnnotations(as.nodes[0].GetId(), annotationName)) | ||
|
|
||
| for _, value := range []string{"test-node-value-1", "test-node-value-2", "test-node-value-3"} { | ||
| _, err = as.Backend.Ent().ExecContext(ctx, query, docUUID, nodeID, false, annotationName, value) | ||
| as.Require().NoError(err) | ||
|
|
@@ -634,7 +641,7 @@ func (as *annotationsSuite) getTestResult(annotationName string) ent.Annotations | |
|
|
||
| result, err := as.Backend.Ent().QueryContext( | ||
| as.Context(), | ||
| "SELECT * FROM annotations WHERE name == ?", | ||
| "SELECT id, document_id, node_id, name, value, is_unique, value_key FROM annotations WHERE name = ?", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing |
||
| annotationName, | ||
| ) | ||
| as.Require().NoError(err) | ||
|
|
@@ -645,16 +652,22 @@ func (as *annotationsSuite) getTestResult(annotationName string) ent.Annotations | |
|
|
||
| for result.Next() { | ||
| annotation := &ent.Annotation{} | ||
| var valueKey sql.NullString | ||
|
|
||
| as.Require().NoError(result.Scan( | ||
| &annotation.ID, | ||
| &annotation.DocumentID, | ||
| &annotation.NodeID, | ||
| &annotation.Name, | ||
| &annotation.Value, | ||
| &annotation.IsUnique, | ||
| &annotation.DocumentID, | ||
| &annotation.NodeID, | ||
| &valueKey, | ||
| )) | ||
|
|
||
| if valueKey.Valid { | ||
| annotation.ValueKey = valueKey.String | ||
| } | ||
|
|
||
| annotations = append(annotations, annotation) | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |||||
| "entgo.io/ent/dialect" | ||||||
| "entgo.io/ent/dialect/sql/schema" | ||||||
| sqlite "github.com/glebarez/go-sqlite" | ||||||
| _ "github.com/lib/pq" // PostgreSQL driver | ||||||
|
Comment on lines
17
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably my main concern. Downstream code might not want the extra dependencies if only one specific database provider is desired. Is there a way we can split into multiple go modules or something like that to keep the dependencies discrete? |
||||||
| "github.com/protobom/protobom/pkg/storage" | ||||||
|
|
||||||
| "github.com/protobom/storage/internal/backends/ent" | ||||||
|
|
@@ -54,25 +55,42 @@ | |||||
| backend.Options = NewBackendOptions() | ||||||
| } | ||||||
|
|
||||||
| // Register the SQLite driver as "sqlite3". | ||||||
| if !slices.Contains(sql.Drivers(), dialect.SQLite) { | ||||||
| sqlite.RegisterAsSQLITE3() | ||||||
| } | ||||||
|
|
||||||
| clientOpts := []ent.Option{} | ||||||
| if backend.Options.Debug { | ||||||
| clientOpts = append(clientOpts, ent.Debug()) | ||||||
| } | ||||||
|
|
||||||
| client, err := ent.Open(dialect.SQLite, backend.Options.DatabaseFile+dsnParams, clientOpts...) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("failed opening connection to sqlite: %w", err) | ||||||
| var client *ent.Client | ||||||
| var err error | ||||||
|
|
||||||
| switch backend.Options.Dialect { | ||||||
| case SQLiteDialect: | ||||||
| // Register the SQLite driver as "sqlite3". | ||||||
| if !slices.Contains(sql.Drivers(), dialect.SQLite) { | ||||||
| sqlite.RegisterAsSQLITE3() | ||||||
| } | ||||||
|
|
||||||
| dsn := backend.Options.DatabaseURL + dsnParams | ||||||
| client, err = ent.Open(dialect.SQLite, dsn, clientOpts...) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("failed opening connection to sqlite: %w", err) | ||||||
| } | ||||||
|
|
||||||
| case PostgresDialect: | ||||||
| client, err = ent.Open(dialect.Postgres, backend.Options.DatabaseURL, clientOpts...) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("failed opening connection to postgres: %w", err) | ||||||
| } | ||||||
|
|
||||||
| default: | ||||||
| return fmt.Errorf("%w: %s", errUnsupportedDialect, backend.Options.Dialect) | ||||||
| } | ||||||
|
|
||||||
| backend.client = client | ||||||
| backend.ctx = ent.NewContext(context.Background(), client) | ||||||
|
|
||||||
| // Run the auto migration tool. | ||||||
| // TODO: need to migrate to new global unique ID feature | ||||||
| migrateOpts := []schema.MigrateOption{migrate.WithGlobalUniqueID(true), migrate.WithDropIndex(true)} | ||||||
| if err := backend.client.Schema.Create(backend.ctx, migrateOpts...); err != nil { | ||||||
| return fmt.Errorf("failed creating schema resources: %w", err) | ||||||
|
|
@@ -114,22 +132,44 @@ | |||||
| } | ||||||
|
|
||||||
| func (backend *Backend) WithDatabaseFile(file string) *Backend { | ||||||
| backend.Options.DatabaseFile = file | ||||||
| backend.Options.DatabaseURL = file | ||||||
|
|
||||||
| return backend | ||||||
| } | ||||||
|
|
||||||
| func (backend *Backend) WithDatabaseURL(url string) *Backend { | ||||||
| backend.Options.DatabaseURL = url | ||||||
|
|
||||||
| return backend | ||||||
| } | ||||||
|
|
||||||
| func (backend *Backend) WithDialect(dialect DatabaseDialect) *Backend { | ||||||
| backend.Options.Dialect = dialect | ||||||
|
|
||||||
| return backend | ||||||
| } | ||||||
|
|
||||||
| func (backend *Backend) withTx(fns ...TxFunc) error { | ||||||
| return backend.withTxContext(backend.ctx, fns...) | ||||||
| } | ||||||
|
|
||||||
| // withTxContext creates a transaction with a specific context to avoid race conditions | ||||||
| func (backend *Backend) withTxContext(ctx context.Context, fns ...TxFunc) error { | ||||||
| if backend.client == nil { | ||||||
| return fmt.Errorf("%w", errUninitializedClient) | ||||||
| } | ||||||
|
|
||||||
| tx, err := backend.client.Tx(backend.ctx) | ||||||
| // Create a NEW context for this transaction instead of modifying the shared one | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
|
||||||
| txCtx := ctx | ||||||
| tx, err := backend.client.Tx(txCtx) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("creating transactional client: %w", err) | ||||||
| } | ||||||
|
|
||||||
| backend.ctx = ent.NewTxContext(backend.ctx, tx) | ||||||
| // Use transaction-local context instead of polluting backend.ctx | ||||||
| // This prevents concurrent transactions from interfering with each other | ||||||
| localTxCtx := ent.NewTxContext(txCtx, tx) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this variable need to be created if unused? |
||||||
| _ = localTxCtx // Mark as used for now | ||||||
|
|
||||||
| for _, fn := range fns { | ||||||
| if err := fn(tx); err != nil { | ||||||
|
Comment on lines
152
to
178
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is extremely important, as using Protobom with a real concurrent database (unlike SQLite) is crucial. Reusing the same context introduces a data leak, mainly where we set documentId on the context. There is a major race condition if "backend" is used by multiple consumers and we attempt to store in parallel. This isolates context per transaction. |
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| // -------------------------------------------------------------- | ||
| // SPDX-FileCopyrightText: Copyright © 2024 The Protobom Authors | ||
| // SPDX-FileType: SOURCE | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // -------------------------------------------------------------- | ||
|
|
||
| package ent_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/protobom/storage/backends/ent" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestDialectConfiguration(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| dialect ent.DatabaseDialect | ||
| databaseURL string | ||
| expectInit bool | ||
| }{ | ||
| { | ||
| name: "SQLite in-memory", | ||
| dialect: ent.SQLiteDialect, | ||
| databaseURL: ":memory:", | ||
| expectInit: true, | ||
| }, | ||
| { | ||
| name: "SQLite file", | ||
| dialect: ent.SQLiteDialect, | ||
| databaseURL: "/tmp/test.db", | ||
| expectInit: true, | ||
| }, | ||
| { | ||
| name: "PostgreSQL invalid connection", | ||
| dialect: ent.PostgresDialect, | ||
| databaseURL: "invalid-connection-string", | ||
| expectInit: false, | ||
| }, | ||
| { | ||
| name: "Unsupported dialect", | ||
| dialect: ent.DatabaseDialect("mysql"), | ||
| databaseURL: "mysql://localhost/test", | ||
| expectInit: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| backend := ent.NewBackend( | ||
| ent.WithDialect(tt.dialect), | ||
| ent.WithDatabaseURL(tt.databaseURL), | ||
| ) | ||
|
|
||
| err := backend.InitClient() | ||
| if tt.expectInit { | ||
| assert.NoError(t, err, "Expected no error for dialect %s with URL %s", tt.dialect, tt.databaseURL) | ||
| if err == nil { | ||
| backend.CloseClient() | ||
| } | ||
| } else { | ||
| assert.Error(t, err, "Expected error for dialect %s with URL %s", tt.dialect, tt.databaseURL) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestBackendOptions(t *testing.T) { | ||
| t.Run("Default options", func(t *testing.T) { | ||
| opts := ent.NewBackendOptions() | ||
| assert.Equal(t, ent.SQLiteDialect, opts.Dialect) | ||
| assert.Equal(t, ":memory:", opts.DatabaseURL) | ||
| assert.False(t, opts.Debug) | ||
| }) | ||
|
|
||
| t.Run("PostgreSQL helper", func(t *testing.T) { | ||
| backend := ent.NewBackend( | ||
| ent.WithPostgresConnection("postgres://localhost/test"), | ||
| ) | ||
| assert.Equal(t, ent.PostgresDialect, backend.Options.Dialect) | ||
| assert.Equal(t, "postgres://localhost/test", backend.Options.DatabaseURL) | ||
| }) | ||
|
|
||
| t.Run("Individual options", func(t *testing.T) { | ||
| backend := ent.NewBackend( | ||
| ent.WithDialect(ent.PostgresDialect), | ||
| ent.WithDatabaseURL("postgres://localhost/mydb"), | ||
| ent.Debug(), | ||
| ) | ||
| assert.Equal(t, ent.PostgresDialect, backend.Options.Dialect) | ||
| assert.Equal(t, "postgres://localhost/mydb", backend.Options.DatabaseURL) | ||
| assert.True(t, backend.Options.Debug) | ||
| }) | ||
|
|
||
| t.Run("Backward compatibility", func(t *testing.T) { | ||
| backend := ent.NewBackend( | ||
| ent.WithDatabaseFile("/tmp/test.db"), | ||
| ) | ||
| assert.Equal(t, ent.SQLiteDialect, backend.Options.Dialect) | ||
| assert.Equal(t, "/tmp/test.db", backend.Options.DatabaseURL) | ||
| }) | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
In Hooks, we never allowed setting both NodeID and DocumentID; they are mutually exclusive. This fixes it.
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.
Great catch!