feat: add update of configs #64
Conversation
WalkthroughThe pull request updates the import paths for various packages from version 1 to version 2 of the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant Handler
participant Storage
participant Database
Client->>Server: PUT /configs/{id}
Server->>Handler: Route update request
Handler->>Handler: Decode JSON payload
Handler->>Handler: Validate configuration
Handler->>Storage: Call UpdateOneConfig
Storage->>Database: Execute update query
Database-->>Storage: Return update result
Storage-->>Handler: Return error or nil
Handler-->>Server: Send HTTP response
Server-->>Client: Respond with status
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/storage/store.go (1)
28-28: Consider returning the updated config for consistency.Other update methods in this interface (e.g.,
UpdateOneConfigActivation) return awebhooks.Config, whereas here we return only an error. To maintain consistency, consider returning the updated config object:- UpdateOneConfig(ctx context.Context, id string, cfg webhooks.ConfigUser) error + UpdateOneConfig(ctx context.Context, id string, cfg webhooks.ConfigUser) (webhooks.Config, error)pkg/server/update.go (2)
16-20: Ensure content type validation or size limits for request body.Currently, we decode the JSON body but we do not fully enforce the content type or handle large payloads. Consider verifying the request's content type and imposing a maximum body size to avoid potential resource exhaustion or malformed data input.
🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
31-40: Return the updated resource in successful responses.When performing a PUT request, it's often useful to return the updated resource in the response body. This helps API clients confirm exactly what has changed, particularly if the server performs any transformations or validations.
🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
pkg/server/handler.go (1)
69-69: Document the new PUT endpoint.The newly added route for updating configurations is consistent with other endpoints. Please ensure it's documented in any API documentation or OpenAPI spec so clients can discover and properly use it.
pkg/storage/postgres/postgres_test.go (1)
62-112: Split the insertion and update operations into separate tests.This test function validates both insertion and updating in a single method. Consider splitting it into two tests (e.g.,
TestConfigsInsertandTestConfigsUpdate) or renaming it to more accurately reflect its purpose. Also consider adding negative cases (e.g., updating a non-existent configuration) to strengthen coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/server/handler.go(1 hunks)pkg/server/update.go(1 hunks)pkg/storage/postgres/postgres.go(2 hunks)pkg/storage/postgres/postgres_test.go(1 hunks)pkg/storage/store.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Default
pkg/server/update.go
[error] Uncommitted changes detected in file
pkg/storage/postgres/postgres.go
[error] Uncommitted changes detected in file
🔇 Additional comments (2)
pkg/server/update.go (1)
22-27: Provide more detailed or aggregated validation errors.If multiple validation errors exist in
cfg.Validate(), it may be helpful to collect and return all of them, rather than stopping at the first error. This gives clients more visibility into all invalid fields in a single response.🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
pkg/storage/postgres/postgres.go (1)
6-6: No concerns regarding new import.Importing
pgdialectis appropriate for usingpgdialect.Array().🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
| func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error { | ||
| if _, err := s.db.NewUpdate(). | ||
| Model(&webhooks.Config{}). | ||
| Where("id = ?", id). | ||
| Set("endpoint = ?", cfgUser.Endpoint). | ||
| Set("secret = ?", cfgUser.Secret). | ||
| Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)). | ||
| Exec(ctx); err != nil { | ||
| return errors.Wrap(err, "updating config") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistency by updating 'updated_at' timestamp.
To stay consistent with other update methods (e.g., UpdateOneConfigActivation), consider updating the updated_at field here as well.
Proposed snippet:
Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)).
+Set("updated_at = ?", time.Now().UTC()).
Exec(ctx); err != nil {
return errors.Wrap(err, "updating config")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error { | |
| if _, err := s.db.NewUpdate(). | |
| Model(&webhooks.Config{}). | |
| Where("id = ?", id). | |
| Set("endpoint = ?", cfgUser.Endpoint). | |
| Set("secret = ?", cfgUser.Secret). | |
| Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)). | |
| Exec(ctx); err != nil { | |
| return errors.Wrap(err, "updating config") | |
| } | |
| return nil | |
| } | |
| func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error { | |
| if _, err := s.db.NewUpdate(). | |
| Model(&webhooks.Config{}). | |
| Where("id = ?", id). | |
| Set("endpoint = ?", cfgUser.Endpoint). | |
| Set("secret = ?", cfgUser.Secret). | |
| Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)). | |
| Set("updated_at = ?", time.Now().UTC()). | |
| Exec(ctx); err != nil { | |
| return errors.Wrap(err, "updating config") | |
| } | |
| return nil | |
| } |
🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
Handle non-existing config ID in update.
This method does not handle cases where the provided ID does not exist (i.e., 0 rows affected). It might incorrectly report success. For consistency with other methods (e.g., DeleteOneConfig), consider returning a storage.ErrConfigNotFound when no matching row is updated.
Example fix:
func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error {
- if _, err := s.db.NewUpdate().
+ res, err := s.db.NewUpdate().
Model(&webhooks.Config{}).
Where("id = ?", id).
Set("endpoint = ?", cfgUser.Endpoint).
Set("secret = ?", cfgUser.Secret).
Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)).
Exec(ctx); err != nil {
return errors.Wrap(err, "updating config")
}
+ if rows, _ := res.RowsAffected(); rows == 0 {
+ return storage.ErrConfigNotFound
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error { | |
| if _, err := s.db.NewUpdate(). | |
| Model(&webhooks.Config{}). | |
| Where("id = ?", id). | |
| Set("endpoint = ?", cfgUser.Endpoint). | |
| Set("secret = ?", cfgUser.Secret). | |
| Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)). | |
| Exec(ctx); err != nil { | |
| return errors.Wrap(err, "updating config") | |
| } | |
| return nil | |
| } | |
| func (s Store) UpdateOneConfig(ctx context.Context, id string, cfgUser webhooks.ConfigUser) error { | |
| res, err := s.db.NewUpdate(). | |
| Model(&webhooks.Config{}). | |
| Where("id = ?", id). | |
| Set("endpoint = ?", cfgUser.Endpoint). | |
| Set("secret = ?", cfgUser.Secret). | |
| Set("event_types = ?", pgdialect.Array(cfgUser.EventTypes)). | |
| Exec(ctx); err != nil { | |
| return errors.Wrap(err, "updating config") | |
| } | |
| if rows, _ := res.RowsAffected(); rows == 0 { | |
| return storage.ErrConfigNotFound | |
| } | |
| return nil | |
| } |
🧰 Tools
🪛 GitHub Actions: Default
[error] Uncommitted changes detected in file
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/storage/migrations.go (1)
51-51: Consider adding additional constraints to the new 'name' column.
If this field is mandatory or frequently queried, addingNOT NULLor indexing might be helpful.Table("configs"). - ColumnExpr("name varchar(255)"). + ColumnExpr("name varchar(255) NOT NULL").pkg/worker/module.go (1)
112-112: Potentially unused cancel function.
ctx, _ = context.WithCancel(ctx)ignores the cancel function. If cancellation is not needed, remove it; otherwise store and invoke it when appropriate.- ctx, _ = context.WithCancel(ctx) + // If you truly need a cancellable context, store the cancel function: + ctx, cancel := context.WithCancel(ctx) + defer cancel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (23)
cmd/migrate.go(1 hunks)cmd/root.go(1 hunks)cmd/serve.go(3 hunks)cmd/worker.go(3 hunks)pkg/attempt.go(1 hunks)pkg/server/activation.go(1 hunks)pkg/server/apierrors/errors.go(1 hunks)pkg/server/delete.go(1 hunks)pkg/server/get.go(1 hunks)pkg/server/handler.go(2 hunks)pkg/server/info.go(1 hunks)pkg/server/insert.go(1 hunks)pkg/server/module.go(1 hunks)pkg/server/secret.go(1 hunks)pkg/server/test.go(1 hunks)pkg/server/update.go(1 hunks)pkg/storage/migrations.go(3 hunks)pkg/storage/postgres/main_test.go(1 hunks)pkg/storage/postgres/module.go(1 hunks)pkg/storage/postgres/postgres_test.go(2 hunks)pkg/worker/handler.go(1 hunks)pkg/worker/module.go(2 hunks)pkg/worker/worker.go(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- cmd/root.go
- pkg/worker/worker.go
- pkg/attempt.go
- pkg/server/delete.go
- pkg/server/insert.go
- pkg/server/test.go
- pkg/server/secret.go
- pkg/server/apierrors/errors.go
- pkg/server/activation.go
- pkg/server/get.go
- cmd/migrate.go
- pkg/storage/postgres/main_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/server/update.go
- pkg/storage/postgres/postgres_test.go
- pkg/server/handler.go
🔇 Additional comments (19)
pkg/server/info.go (2)
Line range hint
9-17: Implementation looks good!The ServiceInfo struct and handler implementation are clean and focused.
6-6: Verify the go-libs v2 upgrade impact.The upgrade to v2 of go-libs could introduce breaking changes. Let's verify the API compatibility and ensure consistent version usage across the codebase.
✅ Verification successful
✅ go-libs v2 upgrade is consistent and compatible
All imports have been consistently updated to v2, and the API usage remains compatible. No further verification needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify go-libs version consistency and check for potential breaking changes # Check for mixed versions of go-libs echo "Checking for mixed versions of go-libs..." rg -g '*.go' "github.com/formancehq/go-libs(/v[0-9]+)?/" -A 1 # Check if the api.RawOk function signature has changed in v2 echo "Checking api.RawOk usage..." ast-grep --pattern 'api.RawOk($_, $_)' # Look for go.mod to verify the dependency version echo "Checking go.mod for go-libs version..." cat go.mod | grep -A 1 "github.com/formancehq/go-libs"Length of output: 6091
pkg/storage/postgres/module.go (2)
Line range hint
11-19: Implementation looks good!The module setup and dependency injection remain correct after the version upgrade. The function properly initializes the PostgreSQL store with the updated bunconnect package.
6-6: Verify compatibility with go-libs v2.The import path has been updated to use v2 of go-libs. Since this is a major version upgrade, we should verify API compatibility.
Let's check for any breaking changes in the v2 API:
✅ Verification successful
Import update to go-libs v2 is consistent and compatible
The codebase shows consistent usage of go-libs v2 across all files, with compatible API usage patterns for both ConnectionOptions and Module functions. No breaking changes or compatibility issues detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential breaking changes in the bunconnect package # Check if ConnectionOptions type has changed ast-grep --pattern 'type ConnectionOptions struct { $$$ }' # Check if Module function signature has changed ast-grep --pattern 'func Module($$$) fx.Option' # Look for any migration guides or breaking changes in README fd -t f "README" -X cat {} | rg -i "breaking|migration|upgrade|v2"Length of output: 181
Script:
#!/bin/bash # Search for bunconnect usage patterns echo "=== Searching for bunconnect imports ===" rg "\"github.com/formancehq/go-libs/(v[0-9]+/)?bun/bunconnect\"" -A 3 echo -e "\n=== Searching for ConnectionOptions usage ===" rg "bunconnect\.ConnectionOptions" -A 3 echo -e "\n=== Looking for Module function implementation ===" rg "func.*Module.*\(" -A 10 pkg/storage/postgres/module.goLength of output: 3015
pkg/storage/migrations.go (4)
9-9: Upgrade to v2 migrations import looks good!
No immediate issues noted; proceed.
18-18: Context-based migration is beneficial.
Passingctx context.Contextinto the migration function is a great practice.
62-62: Validate error handling upon migration completion.
Currently, errors frommigrator.Up(ctx)bubble up, so ensure upstream handles them gracefully (e.g., logs or fails fast).
14-14: Confirm new migrator instantiation.
Ensure thatmigrations.NewMigrator(db)correctly utilizes the database connection in the new v2 package.✅ Verification successful
Migration setup correctly uses v2 package
Themigrations.NewMigrator(db)correctly utilizes the database connection with the new v2 package (github.com/formancehq/go-libs/v2/migrations).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify references to migrations.NewMigrator rg "migrations.NewMigrator\(" -A5Length of output: 428
Script:
#!/bin/bash # Check imports and NewMigrator implementation echo "=== Checking imports in migrations.go ===" rg "^import.*migrations" pkg/storage/migrations.go -A5 echo -e "\n=== Looking for NewMigrator implementation ===" ast-grep --pattern 'func NewMigrator($_) $_'Length of output: 305
Script:
#!/bin/bash # Broader search for migrations-related code echo "=== Full content of migrations.go ===" cat pkg/storage/migrations.go echo -e "\n=== Looking for migrations packages in go.mod ===" cat go.mod | grep migrationsLength of output: 1875
pkg/server/module.go (2)
7-8: Good transition to v2 imports.
All newly introduced and updated v2 imports appear correct.Also applies to: 11-11, 14-16
23-26: Order of OTLP modules.
Appendingotlp.FXModuleFromFlags(cmd)beforeotlptraces.FXModuleFromFlags(cmd)looks intentional and consistent.pkg/worker/handler.go (2)
8-10: Verify API compatibility with go-libs v2.The upgrade to v2 of go-libs packages might introduce breaking changes. Let's verify the API compatibility and consistent usage across the codebase.
✅ Verification successful
All imports consistently updated to go-libs v2
The codebase has been fully migrated to go-libs v2 with consistent API usage patterns and no remaining v1 imports. The go.mod file confirms a single, unified dependency on v2.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining v1 imports and verify consistent upgrade echo "Checking for any remaining v1 imports..." rg "github.com/formancehq/go-libs/(?!v2)[^/]*/" --type go echo "Checking usage of potentially changed APIs..." rg "OTLPMiddleware|FromContext" --type go -A 2Length of output: 12515
Script:
#!/bin/bash # Description: Check for any remaining v1 imports with corrected regex echo "Checking for any remaining v1 imports..." rg --pcre2 "github\.com/formancehq/go-libs(?!/v2)[^/]*/" --type go echo "Checking go.mod for dependency versions..." cat go.modLength of output: 9259
8-10: Clarify PR scope and title.The PR title suggests "feat: add update of configs" but these changes appear to be primarily about upgrading go-libs dependencies to v2. Consider updating the PR title to better reflect the nature of these changes, or clarify if this is part of a larger feature implementation.
cmd/worker.go (4)
6-20: LGTM! Consistent v2 import updates.The migration of all go-libs imports to v2 is systematic and well-organized. The import paths are correctly updated while maintaining the package structure.
23-23: Good use of import aliasing to avoid naming conflicts.The alias
innerotlpclearly distinguishes between the local and external OTLP packages.
64-71: Verify OTLP module initialization order.The order of module initialization looks correct, but we should verify there are no dependency conflicts between
innerotlp.HttpClientModule()andotlp.FXModuleFromFlags(cmd).✅ Verification successful
OTLP module initialization order is correct
The current order is optimal with
innerotlp.HttpClientModule()initialized beforeotlp.FXModuleFromFlags(cmd). This ensures the instrumented HTTP client is available for any OTLP modules that might need it, following proper dependency injection patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any dependency injection order requirements or initialization patterns rg -t go 'fx\.New|fx\.Module' -A 5 -B 5Length of output: 348
Script:
#!/bin/bash # Search for OTLP module implementations and any ordering requirements echo "=== Searching for innerotlp HttpClientModule ===" rg -t go "HttpClientModule" -A 10 -B 10 echo -e "\n=== Searching for otlp FXModuleFromFlags ===" rg -t go "FXModuleFromFlags.*otlp" -A 10 -B 10 echo -e "\n=== Searching for OTLP-related documentation ===" fd -e md -e txt . | xargs rg -i "otlp.*initialization|otlp.*order" || trueLength of output: 3629
Script:
#!/bin/bash # Search for OTLP module implementations echo "=== Searching for otlp package implementations ===" rg -t go "func FXModuleFromFlags" -A 15 -B 5 pkg/otlp/ echo -e "\n=== Searching for otlp package imports ===" rg -t go "^import \(" -A 10 pkg/otlp/Length of output: 732
Line range hint
36-44: Verify OTLP flags compatibility.The addition of OTLP flags needs verification to ensure backward compatibility with existing configurations.
✅ Verification successful
OTLP flags implementation verified as safe and compatible
The OTLP flags implementation is verified to be safe as it:
- Uses standard flag registration from a stable v2 library
- Follows the same pattern as other core components
- Is consistently implemented across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any existing config files or environment variables that might be affected rg -g '!{.git,vendor}/*' -l '(?i)otlp.*flag|otlp.*config' # Look for any flag usage in test files rg -t go -g '*_test.go' 'cmd\.Flags\(\)\.Get.*otlp'Length of output: 161
Script:
#!/bin/bash # Check OTLP flag definitions and usage echo "=== OTLP Flag Definitions ===" rg -A 5 "AddFlags.*otlp" echo -e "\n=== OTLP Environment Variables ===" rg -i "otlp.*env|env.*otlp" echo -e "\n=== Configuration Examples ===" fd -e yaml -e yml -e json | xargs rg -l -i "otlp" echo -e "\n=== Documentation References ===" fd "README.md" | xargs rg -l -i "otlp"Length of output: 465
Script:
#!/bin/bash # Check the content of files with OTLP references echo "=== cmd/serve.go ===" cat cmd/serve.go echo -e "\n=== cmd/worker.go ===" cat cmd/worker.go echo -e "\n=== pkg/server/module.go ===" cat pkg/server/module.go # Also check the otlp package echo -e "\n=== OTLP Package ====" fd -t f . | rg -l "/otlp/"Length of output: 6288
cmd/serve.go (3)
4-12: LGTM! Consistent import updates with worker.go.The v2 import updates maintain consistency with the changes in worker.go.
Line range hint
31-39: Verify flag initialization order.The flag initialization sequence matches worker.go. However, we should verify that the order doesn't affect the serve command's specific requirements.
✅ Verification successful
Flag initialization order is correctly implemented
The identical flag initialization sequence in both serve.go and worker.go is intentional and correct. All flag retrievals occur after complete initialization, and there are no order-dependent flag operations that would affect the serve command's functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any serve-specific flag dependencies rg -t go 'cmd\.Flags\(\)\.Get' cmd/serve.go -A 1 -B 1 # Look for any custom flag initialization in tests rg -t go -g '*_test.go' 'serve.*flag|flag.*serve'Length of output: 575
Script:
#!/bin/bash # Check flag package for dependencies and defaults rg -t go 'func.*AddFlags' -A 5 # Compare with worker.go initialization cat cmd/worker.go # Check AddFlags implementations for any ordering requirements ast-grep --pattern 'func AddFlags($$$) { $$$ }'Length of output: 2449
59-62: Verify module compatibility in combined worker mode.When running in combined worker mode (isWorker=true), we need to ensure the OTLP modules from both serve and worker components work correctly together.
✅ Verification successful
OTLP modules are compatible in combined worker mode
The codebase shows that OTLP modules are properly shared between serve and worker components, using identical configuration and initialization in both standalone and combined modes. The worker functionality is cleanly modularized and the OTLP setup is consistent across all execution modes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any tests or configurations handling combined mode rg -t go 'isWorker.*true|worker.*mode' # Look for any potential module conflicts rg -t go 'fx\.Module.*worker.*serve|serve.*worker'Length of output: 93
Script:
#!/bin/bash # Search for worker-related flags and configuration rg -t go "worker" -A 3 -B 3 # Search specifically for OTLP configuration and modules rg -t go "otlp" -A 3 -B 3 # Look for command flags and initialization rg -t go "cmd.*worker|worker.*cmd" -A 3 -B 3Length of output: 9983
No description provided.