feat(connectors): Add more validation constraints to connector config base fields #444
Conversation
|
""" WalkthroughThe changes introduce validation tags to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Engine
participant Validator
participant Storage
User->>Engine: UpdateConnector(config)
Engine->>Validator: Validate(config)
Validator-->>Engine: ValidationResult
alt Validation passes
Engine->>Storage: Update config and name
Storage-->>Engine: Update result
Engine-->>User: Success/Updated connector
else Validation fails
Engine-->>User: Validation error
end
Assessment against linked issues
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/models/config.go (1)
16-18: Consider using named constants for validation boundsThe validation constraints are well thought out, but could be improved by defining the numeric bounds as named constants for better maintainability and readability.
+const ( + minNameLength = 3 + maxNameLength = 500 + minPollingPeriodNs = 30000000000 // 30 seconds in ns + maxPollingPeriodNs = 86400000000000 // 1 day in ns + maxPageSize = 150 +) type Config struct { - Name string `json:"name" validate:"required,gte=3,lte=500"` - PollingPeriod time.Duration `json:"pollingPeriod" validate:"required,gte=30000000000,lte=86400000000000"` // gte=30s lte=1d in ns - PageSize int `json:"pageSize" validate:"lte=150"` + Name string `json:"name" validate:"required,gte=minNameLength,lte=maxNameLength"` + PollingPeriod time.Duration `json:"pollingPeriod" validate:"required,gte=minPollingPeriodNs,lte=maxPollingPeriodNs"` // gte=30s lte=1d in ns + PageSize int `json:"pageSize" validate:"lte=maxPageSize"` }internal/storage/connectors.go (1)
154-154: Error message could be more descriptiveThe error message doesn't reflect that both name and config are being updated.
- return e("failed to encrypt config", err) + return e("failed to update connector name and config", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/connectors/engine/engine_test.go(4 hunks)internal/connectors/plugins/public/adyen/plugin.go(1 hunks)internal/connectors/plugins/registry/plugins.go(1 hunks)internal/connectors/plugins/registry/wrapper.go(2 hunks)internal/models/config.go(3 hunks)internal/models/config_test.go(2 hunks)internal/storage/connectors.go(1 hunks)internal/storage/connectors_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
internal/storage/connectors.go (1)
Learnt from: laouji
PR: formancehq/payments#251
File: internal/connectors/engine/engine.go:327-333
Timestamp: 2025-01-09T09:56:16.746Z
Learning: In the `ConnectorsConfigUpdate` storage method, only the config field is updated for connectors. Other fields like CreatedAt are preserved and not affected by the update operation.
🧬 Code Graph Analysis (2)
internal/models/config_test.go (1)
internal/models/config.go (1)
Config(15-19)
internal/connectors/plugins/registry/plugins.go (1)
internal/connectors/plugins/registry/wrapper.go (1)
New(20-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (21)
internal/models/config.go (2)
6-7: Good addition of the validator packageThe change to use a structured validator is a good improvement, making validation more declarative and maintainable.
67-68: Good use of the validator packageThe simplified validation method leverages the validator package effectively, reducing custom code and improving maintainability.
internal/connectors/plugins/public/adyen/plugin.go (1)
26-28: Code formatting improvementThe realignment of the struct fields improves readability without changing functionality.
internal/storage/connectors.go (1)
149-149: Good addition of name update to match config updatesUpdating the connector name alongside the config ensures consistency in the database, aligning with the ConnectorsConfigUpdate method's purpose.
internal/connectors/plugins/registry/plugins.go (1)
119-119: Updated parameter passing to match new signatureCorrectly updated the call to
Newto pass all four parameters (connectorID, provider, logger, p) as required by the updated function signature in wrapper.go.internal/storage/connectors_test.go (1)
145-146: LGTM! The test now validates that connector name updates are properly persisted.The test case correctly sets the connector name to "new name" when updating the configuration and properly validates that the stored connector's name is updated accordingly. This change aligns with the enhanced validation constraints where connector metadata like Name is now preserved during config updates.
Also applies to: 154-154
internal/connectors/plugins/registry/wrapper.go (4)
12-15: Improved context tracking with new connector fields.Adding the connectorID and provider fields to the impl struct enhances the plugin wrapper with better context tracking.
20-26: Constructor correctly initializes the new fields.The constructor function has been properly updated to accept and initialize the new connector fields.
33-47: Enhanced logging for improved observability.The logging and OpenTelemetry span creation has been improved to include the provider information, which enhances the observability of the system.
51-67: Consistent logging pattern applied throughout all methods.The changes to include provider information in logs and spans have been consistently applied to all methods in the plugin wrapper. This improves traceability and debugging.
Also applies to: 69-85, 87-103, 105-121, 123-139, 141-157, 159-175, 177-193, 195-211, 213-229, 231-247, 249-265, 267-283, 285-301, 303-319, 321-337
internal/models/config_test.go (4)
9-9: Added validator package import for structured validation testing.The import of validator/v10 supports the enhanced validation approach using structured validation errors.
17-29: Improved "missing name" test with structured validation checks.The test now properly initializes other fields with valid values, ensuring that only the name validation fails. It also properly asserts the validation error using the validator's ValidationErrors type rather than string comparison, which is more robust.
31-62: Enhanced polling period validation with table-driven tests.The polling period validation tests have been improved using a table-driven approach that tests both the lower and upper bounds. This provides better coverage of the validation constraints and makes it easier to add additional test cases in the future.
64-91: Added page size validation tests with proper error checking.New tests for page size validation have been added to ensure the upper bound constraint is properly enforced. The structured validation error checking approach is consistently applied.
internal/connectors/engine/engine_test.go (7)
8-9: Added necessary imports for new tests.The additions of the time and errors packages support the enhanced testing of connector configuration validation and updates.
Also applies to: 21-22
145-155: Added test for default value prefilling during installation.This test ensures that when a connector is installed with empty configuration values, the system correctly prefills them with default values while preserving explicitly set values like the name.
387-402: Added tests for connector update validation.New tests verify that connector updates properly validate configuration and reject invalid configurations.
404-420: Test for config prefilling on update matches installation behavior.This test ensures consistent behavior between installation and updates when it comes to prefilling default configuration values, which is important for maintaining backward compatibility.
422-434: Error propagation test for plugin registry failures.The test verifies that errors from the plugin registry during connector updates are correctly propagated to the caller, maintaining proper error handling.
436-447: Storage error propagation test.This test confirms that database errors during connector updates are properly propagated, ensuring the system doesn't silently fail when storage operations encounter issues.
449-471: Happy path test for successful connector update.This comprehensive test verifies that when updating a connector:
- The system preserves important metadata (ID, CreatedAt, Provider)
- It correctly updates the name and configuration
- The storage layer receives the properly constructed connector object
This ensures that connector updates maintain data integrity while applying the requested changes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #444 +/- ##
==========================================
+ Coverage 68.42% 69.03% +0.61%
==========================================
Files 586 586
Lines 29815 29811 -4
==========================================
+ Hits 20400 20580 +180
+ Misses 8332 8127 -205
- Partials 1083 1104 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/connectors/plugins/registry/wrapper_test.go (4)
30-39: Consider verifying the response in addition to the error.While the test correctly verifies that no error is returned, it would be more comprehensive to also verify that the response from the plugin is correctly passed through by the wrapper.
- _, err := wrapper.Install(ctx, req) + response, err := wrapper.Install(ctx, req) Expect(err).To(BeNil()) + Expect(response).To(Equal(models.InstallResponse{}))
33-38: Consider documenting the purpose of the Name() expectation.The test expects the
Name()method to be called up to twice, but it's not immediately clear why. Adding a comment explaining the rationale would improve code maintainability.+ // Name() may be called during tracing setup and method execution plg.EXPECT().Name().Return("dummy").MaxTimes(2)
30-205: Complete test coverage for wrapper functionality.The test suite thoroughly covers all wrapper methods, ensuring that calls are correctly delegated to the underlying plugin. This comprehensive test pattern confirms the wrapper acts as a transparent proxy as intended.
Consider adding tests for error scenarios to ensure errors from the plugin are properly propagated through the wrapper.
186-191: Consider testing with realistic webhook config.The test currently uses an empty webhook config. Consider enhancing this test with a more realistic config to better validate the behavior.
- req := models.VerifyWebhookRequest{Config: &models.WebhookConfig{}} + req := models.VerifyWebhookRequest{Config: &models.WebhookConfig{ + URL: "https://example.com/webhook", + EventTypes: []string{"payment.created"}, + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/connectors/plugins/registry/wrapper_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (2)
internal/connectors/plugins/registry/wrapper_test.go (2)
3-10: Good use of testing dependencies.The imports show a well-structured test setup using Ginkgo/Gomega for BDD-style testing and gomock for mocking dependencies. This approach provides a clean way to test the wrapper component in isolation.
12-28: Well-organized test setup.The test setup is clean and organized. Using
BeforeEachto initialize the controller, mock plugin, connector ID, and logger for each test ensures consistent test conditions and follows BDD best practices.
Fixes: PMNT-47