feat(configs): Simplify connector config definition to reduce boilerplate code #262
Conversation
WalkthroughThis pull request introduces significant updates to configuration validation across multiple payment connector plugins. The changes primarily involve integrating the Changes
Assessment against linked issues
Possibly related PRs
Suggested Reviewers
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 (
|
9a008e2 to
91618cc
Compare
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
91618cc to
3361830
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (19)
internal/connectors/plugins/public/moneycorp/config.go (1)
22-23: Consider caching the validator instance.The validator instance is created for each validation call. Since validator creation involves reflection, it's more efficient to create it once and reuse it.
+var validate = validator.New(validator.WithRequiredStructEnabled()) + func unmarshalAndValidateConfig(payload json.RawMessage) (Config, error) { var config Config if err := json.Unmarshal(payload, &config); err != nil { return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) } - validate := validator.New(validator.WithRequiredStructEnabled()) return config, validate.Struct(config) }internal/connectors/plugins/public/dummypay/config.go (1)
19-20: Consider wrapping validation errors consistently.Other connector configs wrap errors with
models.ErrInvalidConfig. Consider maintaining consistency:validate := validator.New(validator.WithRequiredStructEnabled()) - return config, validate.Struct(config) + if err := validate.Struct(config); err != nil { + return Config{}, fmt.Errorf("failed to validate config: %w", err) + } + return config, nilinternal/connectors/plugins/public/atlar/config.go (1)
17-17: Consider using consistent parameter types across connectors.Other connectors use
json.RawMessagefor the payload parameter, while this one uses[]byte.-func unmarshalAndValidateConfig(payload []byte) (Config, error) { +func unmarshalAndValidateConfig(payload json.RawMessage) (Config, error) {internal/connectors/plugins/public/modulr/config.go (2)
22-23: Consider reusing validator instance for better performance.The validator instance is created for each validation call. Consider creating a package-level validator instance to avoid the overhead of repeated instantiation.
+var validate = validator.New(validator.WithRequiredStructEnabled()) + func unmarshalAndValidateConfig(payload []byte) (Config, error) { var config Config if err := json.Unmarshal(payload, &config); err != nil { return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) } - validate := validator.New(validator.WithRequiredStructEnabled()) return config, validate.Struct(config) }
23-23: Consider wrapping validation errors consistently.For consistency with JSON unmarshal errors, consider wrapping validation errors with models.ErrInvalidConfig.
- return config, validate.Struct(config) + if err := validate.Struct(config); err != nil { + return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) + } + return config, nilinternal/connectors/plugins/public/mangopay/config.go (1)
22-23: Apply same performance and error handling improvements as modulr.For consistency across connectors and better performance:
- Reuse validator instance
- Wrap validation errors with models.ErrInvalidConfig
+var validate = validator.New(validator.WithRequiredStructEnabled()) + func unmarshalAndValidateConfig(payload json.RawMessage) (Config, error) { var config Config if err := json.Unmarshal(payload, &config); err != nil { return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) } - validate := validator.New(validator.WithRequiredStructEnabled()) - return config, validate.Struct(config) + if err := validate.Struct(config); err != nil { + return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) + } + return config, nilinternal/connectors/plugins/public/currencycloud/config.go (1)
22-23: Apply same performance and error handling improvements as other connectors.For consistency across connectors and better performance:
- Reuse validator instance
- Wrap validation errors with models.ErrInvalidConfig
+var validate = validator.New(validator.WithRequiredStructEnabled()) + func unmarshalAndValidateConfig(payload json.RawMessage) (Config, error) { var config Config if err := json.Unmarshal(payload, &config); err != nil { return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) } - validate := validator.New(validator.WithRequiredStructEnabled()) - return config, validate.Struct(config) + if err := validate.Struct(config); err != nil { + return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) + } + return config, nilinternal/connectors/plugins/public/adyen/config.go (1)
24-25: Apply same performance and error handling improvements as other connectors.For consistency across connectors and better performance:
- Reuse validator instance
- Wrap validation errors with models.ErrInvalidConfig
+var validate = validator.New(validator.WithRequiredStructEnabled()) + func unmarshalAndValidateConfig(payload []byte) (Config, error) { var config Config if err := json.Unmarshal(payload, &config); err != nil { return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) } - validate := validator.New(validator.WithRequiredStructEnabled()) - return config, validate.Struct(config) + if err := validate.Struct(config); err != nil { + return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) + } + return config, niltools/connector-template/template/config.gotpl (1)
14-17: Enhance example comments to demonstrate more validation patterns.While the current examples show basic required field validation, consider adding examples of other common validation patterns that connectors might need, such as:
- URL validation for endpoints
- Length constraints for API keys
- Enum values for modes/environments
Example enhancement:
// ClientID string `json:"clientID" validate:"required"` // APIKey string `json:"apiKey" validate:"required,min=32,max=64"` -// Endpoint string `json:"endpoint" validate:"required"` +// Endpoint string `json:"endpoint" validate:"required,url"` +// Mode string `json:"mode" validate:"required, production"`internal/connectors/plugins/public/bankingcircle/config.go (1)
12-17: Enhance validation rules for URLs and certificates.While the required validation is good, consider adding more specific validation rules:
- URL validation for endpoints
- PEM format validation for certificates
- Endpoint string `json:"endpoint" yaml:"endpoint" validate:"required"` - AuthorizationEndpoint string `json:"authorizationEndpoint" yaml:"authorizationEndpoint" validate:"required"` - UserCertificate string `json:"userCertificate" yaml:"userCertificate" validate:"required"` - UserCertificateKey string `json:"userCertificateKey" yaml:"userCertificateKey" validate:"required"` + Endpoint string `json:"endpoint" yaml:"endpoint" validate:"required,url"` + AuthorizationEndpoint string `json:"authorizationEndpoint" yaml:"authorizationEndpoint" validate:"required,url"` + UserCertificate string `json:"userCertificate" yaml:"userCertificate" validate:"required,startswith=-----BEGIN CERTIFICATE-----"` + UserCertificateKey string `json:"userCertificateKey" yaml:"userCertificateKey" validate:"required,startswith=-----BEGIN PRIVATE KEY-----"`internal/connectors/plugins/public/wise/config.go (1)
48-51: Consider improving error handling for validation errors.The current implementation might lose the context of validation errors.
validate := validator.New(validator.WithRequiredStructEnabled()) if err := validate.Struct(config); err != nil { - return config, err + return config, fmt.Errorf("config validation failed: %w", err) }internal/connectors/plugins/public/stripe/plugin_test.go (1)
33-33: Consider adding more specific validation test cases.While using
ContainSubstring("APIKey")is more resilient, consider adding test cases for:
- Invalid API key format
- Multiple validation errors
- Edge cases (empty string vs missing field)
Example additional test cases:
It("should report multiple validation errors", func(ctx SpecContext) { config := json.RawMessage(`{"apiKey": "", "webhookSecret": ""}`) _, err := New("stripe", logger, config) Expect(err.Error()).To(And( ContainSubstring("APIKey"), ContainSubstring("required"), )) })internal/connectors/plugins/public/generic/plugin_test.go (1)
33-33: LGTM! The error assertion changes improve maintainability.The switch from exact error message matching to substring checks makes the tests less brittle and easier to maintain.
Consider using a test helper function to standardize these config validation checks across all plugin tests:
func expectConfigValidationError(err error, field string) { Expect(err.Error()).To(ContainSubstring(field)) }Also applies to: 39-39
Earthfile (1)
37-37: LGTM! The simplified config generation improves maintainability.Using
jqto generate empty JSON objects is a cleaner approach that reduces boilerplate.Consider adding a comment explaining the purpose of the empty configs and why they're needed:
+ # Generate empty config templates for each connector RUN jq -cn "{(\"$c\"): {}}" >> raw_configs.jsoninternal/connectors/plugins/public/wise/plugin_test.go (1)
78-78: LGTM! Consider adding more validation test cases.The error message change correctly reflects the switch to using the validator package for config validation.
Consider adding test cases for:
- Individual required field validation
- Field format validation (e.g., malformed API keys)
- Edge cases like whitespace-only values
test/e2e/api_connectors_test.go (1)
95-98: LGTM! Consider adding more edge cases.The test cases effectively cover empty and invalid directory paths for both versions.
Consider adding test cases for:
- Directory paths with special characters but valid format
- Maximum length directory paths
- Directory paths with trailing slashes
internal/connectors/engine/engine.go (1)
339-342: Consider extracting common validation error handling.The validation error handling is identical to InstallConnector. Consider extracting this into a helper function to reduce duplication.
+func wrapValidationError(err error) error { + if _, ok := err.(validator.ValidationErrors); ok || errors.Is(err, models.ErrInvalidConfig) { + return errors.Wrap(ErrValidation, err.Error()) + } + return err +} func (e *engine) UpdateConnector(ctx context.Context, connectorID models.ConnectorID, rawConfig json.RawMessage) error { // ... err := e.plugins.RegisterPlugin(connector.ID, connector.Name, config, connector.Config, true) if err != nil { otel.RecordError(span, err) - if _, ok := err.(validator.ValidationErrors); ok || errors.Is(err, models.ErrInvalidConfig) { - return errors.Wrap(ErrValidation, err.Error()) - } - return err + return wrapValidationError(err) } // ... }internal/connectors/plugins/public/currencycloud/plugin_test.go (1)
33-33: LGTM! Consider standardizing test case naming.The switch to substring-based error validation is a good improvement that makes tests less brittle. For consistency across connector plugins, consider renaming the test cases to follow the pattern:
-It("should report errors in config - loginID" +It("reports validation errors in the config - loginID"Also applies to: 39-39, 45-45
internal/connectors/plugins/public/mangopay/plugin_test.go (1)
33-34: Consider standardizing error message format across connectors.While the explicit nil checks improve test robustness, consider standardizing the error message format across all connectors. This would make error handling more consistent and maintainable.
Also applies to: 40-41, 47-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/connectors/plugins/configs.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/adyen/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/atlar/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/bankingcircle/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/currencycloud/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/dummypay/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/generic/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/mangopay/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/modulr/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/moneycorp/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/stripe/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/wise/config.jsonis excluded by!**/*.json
📒 Files selected for processing (25)
Earthfile(1 hunks)internal/connectors/engine/engine.go(3 hunks)internal/connectors/plugins/public/adyen/config.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(1 hunks)internal/connectors/plugins/public/atlar/config.go(1 hunks)internal/connectors/plugins/public/atlar/plugin_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config.go(1 hunks)internal/connectors/plugins/public/bankingcircle/plugin_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/config.go(1 hunks)internal/connectors/plugins/public/currencycloud/plugin_test.go(1 hunks)internal/connectors/plugins/public/dummypay/config.go(1 hunks)internal/connectors/plugins/public/generic/config.go(1 hunks)internal/connectors/plugins/public/generic/plugin_test.go(1 hunks)internal/connectors/plugins/public/mangopay/config.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin_test.go(1 hunks)internal/connectors/plugins/public/modulr/config.go(1 hunks)internal/connectors/plugins/public/modulr/plugin_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/config.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin_test.go(1 hunks)internal/connectors/plugins/public/stripe/config.go(1 hunks)internal/connectors/plugins/public/stripe/plugin_test.go(1 hunks)internal/connectors/plugins/public/wise/config.go(2 hunks)internal/connectors/plugins/public/wise/plugin_test.go(1 hunks)test/e2e/api_connectors_test.go(2 hunks)tools/connector-template/template/config.gotpl(2 hunks)
🔇 Additional comments (17)
internal/connectors/plugins/public/moneycorp/config.go (2)
7-7: LGTM! Good choice of validation library.Using
go-playground/validator/v10is a solid choice for standardizing validation across connectors.
12-14: LGTM! Clean struct definition with validation tags.The validation tags effectively replace custom validation logic, reducing boilerplate while maintaining clear requirements.
internal/connectors/plugins/public/moneycorp/plugin_test.go (1)
37-37: LGTM! Test updated to match new validation error messages.The test correctly verifies the validation error from the validator library.
internal/connectors/plugins/public/stripe/config.go (1)
12-12: LGTM! Clean validation implementation.The changes effectively simplify the config validation while maintaining proper error handling.
Also applies to: 20-21
internal/connectors/plugins/public/generic/config.go (1)
12-13: LGTM! Consistent validation implementation.The changes follow the established pattern for config validation across connectors.
Also applies to: 21-22
internal/connectors/plugins/public/atlar/config.go (1)
12-14: LGTM! Validation implementation is clean.The validation changes align well with other connectors.
Also applies to: 22-23
internal/connectors/plugins/public/adyen/config.go (1)
12-16: Verify secure handling of sensitive configuration fields.The config contains several sensitive fields (APIKey, WebhookUsername, WebhookPassword). Let's verify they're handled securely throughout the codebase.
✅ Verification successful
Sensitive configuration fields are properly secured.
The configuration fields (APIKey, WebhookUsername, WebhookPassword) are handled securely:
- Encrypted at rest using PGP symmetric encryption with AES256
- No plain-text storage in the database
- No logging of sensitive values
- Proper encryption/decryption flow in the storage layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential security issues with sensitive config fields # Check for logging of sensitive fields echo "Checking for potential logging of sensitive fields..." rg -i "log|print|fmt" -A 5 -B 5 "APIKey|WebhookUsername|WebhookPassword" # Check for storage/transmission of sensitive fields echo "Checking how sensitive fields are stored/transmitted..." rg "APIKey|WebhookUsername|WebhookPassword" -A 5 -B 5 "database|store|send|http|marshal"Length of output: 555
Script:
#!/bin/bash # Search for usage of sensitive fields in logging contexts echo "=== Checking logging patterns ===" rg -i "log|print|fmt" -C 5 --type go # Search for sensitive field usage patterns echo -e "\n=== Checking field usage patterns ===" rg "(APIKey|WebhookUsername|WebhookPassword)" -C 5 --type go # Search for struct initialization and field access echo -e "\n=== Checking initialization and access patterns ===" ast-grep --pattern 'Config{$$$}' ast-grep --pattern '$_.APIKey' as 8000 t-grep --pattern '$_.WebhookUsername' ast-grep --pattern '$_.WebhookPassword'Length of output: 66704
Script:
#!/bin/bash # Search for Adyen config struct usage and encryption patterns echo "=== Checking Adyen config implementation ===" rg -C 5 "type.+Config.+struct" internal/connectors/plugins/public/adyen/ # Check for encryption/secure storage patterns echo -e "\n=== Checking encryption patterns ===" rg -C 5 "encrypt|mask|redact|sensitive" internal/connectors/plugins/public/adyen/ # Check for config field usage echo -e "\n=== Checking config field usage ===" rg -C 5 "Config.*{" internal/connectors/plugins/public/adyen/ rg -C 5 "\.APIKey|\.WebhookUsername|\.WebhookPassword" internal/connectors/plugins/public/adyen/Length of output: 3531
Script:
#!/bin/bash # Check storage layer implementation echo "=== Checking storage layer ===" rg -C 5 "configEncryptionKey|encrypt" internal/storage/ # Check general security patterns echo -e "\n=== Checking general security patterns ===" rg -C 5 "sensitive|encrypt" internal/connectors/ # Check client implementation echo -e "\n=== Checking client implementation ===" rg -C 5 "New\(" internal/connectors/plugins/public/adyen/client/Length of output: 23677
tools/connector-template/template/config.gotpl (1)
26-27: LGTM! Good use of WithRequiredStructEnabled.The validation setup is correct and consistent with best practices. Using
WithRequiredStructEnabled()ensures proper validation of required fields.internal/connectors/plugins/public/bankingcircle/config.go (1)
25-26: LGTM! Consistent validation setup.The validation setup matches the template pattern and correctly uses
WithRequiredStructEnabled().internal/connectors/plugins/public/wise/config.go (1)
16-17: LGTM! Good separation of basic and custom validation.The validation tags are correctly applied while maintaining custom RSA key validation.
internal/connectors/plugins/public/atlar/plugin_test.go (1)
33-33: LGTM! Error assertion changes align with the project's direction.The changes to use substring checks for config validation errors are consistent with other plugin tests and improve maintainability.
Also applies to: 39-39, 45-45
internal/connectors/plugins/public/modulr/plugin_test.go (1)
33-33: LGTM! Error assertion changes maintain consistency.The changes to use substring checks for config validation errors maintain consistency with other plugin tests and support the goal of reducing boilerplate.
Also applies to: 39-39, 45-45
test/e2e/api_connectors_test.go (1)
87-93: LGTM! Well-structured test function enhancement.The updated test function signature with
dirValandexpectedErrparameters improves test clarity and allows for more precise validation testing.internal/connectors/engine/engine.go (2)
18-18: LGTM! Validator package import added.The validator package import is correctly placed and version is appropriately pinned.
136-139: LGTM! Comprehensive validation error handling.The error handling properly handles both validator errors and custom invalid config errors, maintaining error context through wrapping.
internal/connectors/plugins/public/adyen/plugin_test.go (1)
36-37: LGTM! Comprehensive config validation.The test cases properly validate all required fields with clear, comprehensive test data. The error validation approach using substrings is consistent with other connector implementations.
Also applies to: 40-41, 44-44
internal/connectors/plugins/public/bankingcircle/plugin_test.go (1)
33-33: Verify the intentional difference in error validation approaches.The test uses substring validation for basic fields but maintains exact error matching for certificate validation. While this might be intentional for security reasons, please verify if this mixed approach is desired.
Run this script to check error handling consistency across connectors:
Also applies to: 39-39, 45-45, 51-51, 57-57, 63-63, 67-69
✅ Verification successful
The mixed error validation approach is intentionally designed and appropriate
The codebase consistently uses:
- Substring matching for basic config field validation, allowing for flexible error messages
- Exact error matching for security-critical certificate validation, ensuring precise error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check error handling patterns across connector plugins # Look for certificate-related error handling patterns # Search for certificate validation patterns rg -A 2 "certificate.*error|error.*certificate" internal/connectors/plugins/public/ # Search for error validation patterns in tests ast-grep --pattern 'Expect($_, $_).To(MatchError($_))'Length of output: 1579
Script:
#!/bin/bash # Search for all error validation patterns in the test file rg "Expect\(.*error.*\).To\(" internal/connectors/plugins/public/bankingcircle/plugin_test.go -B 2 -A 1 # Search for ContainSubstring usage in tests across all connectors for comparison rg "ContainSubstring.*error|error.*ContainSubstring" internal/connectors/plugins/public/ -A 1Length of output: 201
Script:
#!/bin/bash # Search for Expect patterns in test file rg "Expect\(" internal/connectors/plugins/public/bankingcircle/plugin_test.go -B 1 -A 1 # Use ast-grep to find test assertions ast-grep --pattern 'Expect($_).To($_)' # Simple search for ContainSubstring rg "ContainSubstring" internal/connectors/plugins/public/bankingcircle/plugin_test.goLength of output: 65980
| return fmt.Errorf("missing directory in config: %w", models.ErrInvalidConfig) | ||
| } | ||
| return nil | ||
| Directory string `json:"directory" validate:"required,dirpath"` |
There was a problem hiding this comment.
💡 Codebase verification
Missing implementation for "dirpath" custom validator
The dirpath validation tag is used in the Directory field, but there is no implementation of this custom validator in the codebase. This will cause validation to fail at runtime.
- Add the custom validator implementation and register it with the validator instance
- Example implementation:
v.RegisterValidation("dirpath", func(fl validator.FieldLevel) bool { // Add directory path validation logic here return true })
🔗 Analysis chain
Verify the implementation of the "dirpath" custom validator.
The dirpath validation tag is used but its implementation is not visible in the provided code. This custom validator needs to be registered with the validator instance.
Let's check if the custom validator is defined elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dirpath validator implementation
rg -l "RegisterValidation.*dirpath"
Length of output: 37
Script:
#!/bin/bash
# Search for dirpath validator function definition
rg -l "func.*dirpath"
# Search for validator initialization patterns
rg -l "validator\.New\(\)"
# Search for similar directory validation patterns
rg -i "directory.*validate"
Length of output: 201
|
|
||
| return nil | ||
| APIKey string `json:"apiKey" validate:"required"` | ||
| WebhookUsername string `json:"webhookUsername" validate:"required"` |
There was a problem hiding this comment.
Ok perhaps I misunderstood your explanation yesterday about the webhooks being necessary. All three fields were marked as "required" in the config.json, but it hadn't been enforced by the original config.Validate() function, so I thought they had been accidentally left out from the implementation. I was trying to align documentation and implementation.
If they are optional, then we can remove the required tag.
There was a problem hiding this comment.
oh my bad, it was a miscommunication then ! Webhooks are necessary in Adyen, but the username/password is not, depending on if the users wants to have an basic auth or not
There was a problem hiding this comment.
and for the LiveEndpointPrefix field, it is not necessary for test endpoits/sandbox for adyen
3361830 to
8cb597a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (12)
internal/connectors/plugins/registry/plugins.go (1)
66-67: Remove debug statements usingfmt.PrintfThe calls to
fmt.Printfat lines 66-67 and 86-87 appear to be leftover debug statements. It's recommended to remove these prints or replace them with appropriate logging at the correct log level.Also applies to: 86-87
internal/connectors/plugins/registry/configs.go (2)
13-19: Add documentation for configuration types.Consider adding package-level documentation to explain:
- The purpose and usage of each type
- The relationship between
Configs,Config, andParameter- Examples of how to define custom configurations
21-38: Enhance default parameter definitions.Consider the following improvements:
- Add validation for the duration format in
pollingPeriod- Document the expected format and valid ranges for each parameter
- Consider using constants for default values to improve maintainability
Example enhancement:
+const ( + DefaultPollingPeriod = "2m" + DefaultPageSize = "100" +) var ( defaultParameters = map[string]Parameter{ "pollingPeriod": { DataType: TypeDurationNs, Required: false, - DefaultValue: "2m", + DefaultValue: DefaultPollingPeriod, },internal/connectors/plugins/registry/plugins_test.go (2)
22-33: Consider adding validation for more edge cases.The
Configstruct provides good coverage for basic types, but consider adding tests for:
- Nested structs
- Array/slice types
- Map types
- Custom validation rules
31-32: Document the purpose of special field types.Add comments explaining why
NilJsonTagandunexportedFieldare included in the test struct, as their purpose isn't immediately clear.internal/api/backend/backend.go (1)
34-34: Document the breaking change in Backend interface.Consider adding a comment in the code or updating the changelog to document this breaking change in the Backend interface.
+// ConnectorsConfigs returns the registry configurations for all supported connectors. +// Note: Changed from plugins.Configs to registry.Configs in v0.x.x ConnectorsConfigs() registry.Configstools/connector-template/template/plugin.gotpl (1)
Line range hint
35-35: Address TODO comment in template.The template contains a TODO comment about using the config to create the client. This should be updated to demonstrate the proper usage of the config struct.
- _ = config // TODO: use config to create client and remove this line + client := client.New( + // TODO: Add example of using config fields to create client + // config.Field1, + // config.Field2, + )internal/connectors/engine/workflow/main_test.go (1)
87-87: Consider adding test cases for config validation.While the change adapts the test to the new RegisterPlugin signature, consider adding test cases that:
- Verify config validation behavior
- Test various config scenarios (valid, invalid, empty)
internal/connectors/plugins/public/generic/config.go (1)
21-22: Consider improving validation error handling.The current implementation might not provide user-friendly error messages when validation fails. Consider wrapping validation errors with more context.
validate := validator.New(validator.WithRequiredStructEnabled()) -return config, validate.Struct(config) +if err := validate.Struct(config); err != nil { + return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) +} +return config, nilinternal/connectors/plugins/public/currencycloud/config.go (1)
Line range hint
1-23: Consider creating a common validation helper.Given that all connectors now share identical validation logic, consider creating a common helper function to reduce code duplication.
Example implementation:
// internal/connectors/plugins/common/config/validator.go package config import ( "encoding/json" "github.com/go-playground/validator/v10" "github.com/pkg/errors" "github.com/formancehq/payments/internal/models" ) func UnmarshalAndValidate[T any](payload json.RawMessage) (T, error) { var config T if err := json.Unmarshal(payload, &config); err != nil { return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) } validate := validator.New(validator.WithRequiredStructEnabled()) if err := validate.Struct(config); err != nil { return config, errors.Wrap(models.ErrInvalidConfig, err.Error()) } return config, nil }This would allow each connector to simplify its validation to:
func unmarshalAndValidateConfig(payload json.RawMessage) (Config, error) { return config.UnmarshalAndValidate[Config](payload) }test/e2e/api_connectors_test.go (1)
134-145: Consider adding v2 test cases for config updates.While the test cases effectively validate directory paths for v3, consider adding similar test cases for v2 to maintain consistent test coverage across versions.
Add these test cases:
DescribeTable("should respond with a validation error when plugin-side config invalid", func(ver int, dirValue string, expectedErr string) { config := newConnectorConfigurationFn()(id) config.Directory = dirValue err := ConnectorConfigUpdate(ctx, app.GetValue(), ver, connectorID, &config) Expect(err).NotTo(BeNil()) Expect(err.Error()).To(ContainSubstring("400")) Expect(err.Error()).To(ContainSubstring(expectedErr)) }, Entry("empty directory", 3, "", "validation for 'Directory' failed on the 'required' tag"), Entry("invalid directory", 3, "$#2djskajdj", "validation for 'Directory' failed on the 'dirpath' tag"), + Entry("empty directory with v2", 2, "", "validation for 'Directory' failed on the 'required' tag"), + Entry("invalid directory with v2", 2, "$#2djskajdj", "validation for 'Directory' failed on the 'dirpath' tag"), )internal/connectors/engine/engine.go (1)
339-341: Consider extracting duplicated error handling logic.The error handling logic is identical to the one in
InstallConnector. Consider extracting it into a helper function to reduce code duplication.Example refactor:
+func isValidationError(err error) bool { + _, ok := err.(validator.ValidationErrors) + return ok || errors.Is(err, models.ErrInvalidConfig) +} + +func wrapValidationError(err error) error { + if isValidationError(err) { + return errors.Wrap(ErrValidation, err.Error()) + } + return err +} + func (e *engine) InstallConnector(ctx context.Context, provider string, rawConfig json.RawMessage) (models.ConnectorID, error) { // ... err := e.plugins.RegisterPlugin(connector.ID, connector.Name, config, connector.Config, false) if err != nil { otel.RecordError(span, err) - if _, ok := err.(validator.ValidationErrors); ok || errors.Is(err, models.ErrInvalidConfig) { - return models.ConnectorID{}, errors.Wrap(ErrValidation, err.Error()) - } - return models.ConnectorID{}, err + return models.ConnectorID{}, wrapValidationError(err) } // ... } func (e *engine) UpdateConnector(ctx context.Context, connectorID models.ConnectorID, rawConfig json.RawMessage) error { // ... err := e.plugins.RegisterPlugin(connector.ID, connector.Name, config, connector.Config, true) if err != nil { otel.RecordError(span, err) - if _, ok := err.(validator.ValidationErrors); ok || errors.Is(err, models.ErrInvalidConfig) { - return errors.Wrap(ErrValidation, err.Error()) - } - return err + return wrapValidationError(err) } // ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/connectors/plugins/configs.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/adyen/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/atlar/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/bankingcircle/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/currencycloud/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/dummypay/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/generic/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/mangopay/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/modulr/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/moneycorp/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/stripe/config.jsonis excluded by!**/*.jsoninternal/connectors/plugins/public/wise/config.jsonis excluded by!**/*.json
📒 Files selected for processing (49)
Earthfile(0 hunks)internal/api/backend/backend.go(2 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connector_configs.go(1 hunks)internal/api/v2/handler_connectors_configs.go(2 hunks)internal/api/v2/handler_connectors_configs_test.go(2 hunks)internal/api/v3/handler_connectors_configs.go(2 hunks)internal/api/v3/handler_connectors_configs_test.go(2 hunks)internal/connectors/engine/engine.go(3 hunks)internal/connectors/engine/workflow/main_test.go(1 hunks)internal/connectors/plugins/configs.go(0 hunks)internal/connectors/plugins/public/adyen/config.go(1 hunks)internal/connectors/plugins/public/adyen/plugin.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(1 hunks)internal/connectors/plugins/public/atlar/config.go(1 hunks)internal/connectors/plugins/public/atlar/plugin.go(1 hunks)internal/connectors/plugins/public/atlar/plugin_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config.go(1 hunks)internal/connectors/plugins/public/bankingcircle/plugin.go(1 hunks)internal/connectors/plugins/public/bankingcircle/plugin_test.go(1 hunks)internal/connectors/plugins/public/currencycloud/config.go(1 hunks)internal/connectors/plugins/public/currencycloud/plugin.go(1 hunks)internal/connectors/plugins/public/currencycloud/plugin_test.go(1 hunks)internal/connectors/plugins/public/dummypay/config.go(1 hunks)internal/connectors/plugins/public/dummypay/plugin.go(1 hunks)internal/connectors/plugins/public/generic/config.go(1 hunks)internal/connectors/plugins/public/generic/plugin.go(1 hunks)internal/connectors/plugins/public/generic/plugin_test.go(1 hunks)internal/connectors/plugins/public/mangopay/config.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin_test.go(1 hunks)internal/connectors/plugins/public/modulr/config.go(1 hunks)internal/connectors/plugins/public/modulr/plugin.go(1 hunks)internal/connectors/plugins/public/modulr/plugin_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/config.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin_test.go(1 hunks)internal/connectors/plugins/public/stripe/config.go(1 hunks)internal/connectors/plugins/public/stripe/plugin.go(1 hunks)internal/connectors/plugins/public/stripe/plugin_test.go(1 hunks)internal/connectors/plugins/public/wise/config.go(2 hunks)internal/connectors/plugins/public/wise/plugin.go(1 hunks)internal/connectors/plugins/public/wise/plugin_test.go(1 hunks)internal/connectors/plugins/registry/configs.go(1 hunks)internal/connectors/plugins/registry/plugins.go(3 hunks)internal/connectors/plugins/registry/plugins_test.go(1 hunks)test/e2e/api_connectors_test.go(2 hunks)tools/connector-template/template/config.gotpl(2 hunks)tools/connector-template/template/plugin.gotpl(2 hunks)
💤 Files with no reviewable changes (2)
- Earthfile
- internal/connectors/plugins/configs.go
🚧 Files skipped from review as they are similar to previous changes (17)
- internal/connectors/plugins/public/wise/plugin_test.go
- internal/connectors/plugins/public/moneycorp/plugin_test.go
- internal/connectors/plugins/public/stripe/plugin_test.go
- tools/connector-template/template/config.gotpl
- internal/connectors/plugins/public/dummypay/config.go
- internal/connectors/plugins/public/mangopay/plugin_test.go
- internal/connectors/plugins/public/adyen/plugin_test.go
- internal/connectors/plugins/public/atlar/plugin_test.go
- internal/connectors/plugins/public/bankingcircle/plugin_test.go
- internal/connectors/plugins/public/generic/plugin_test.go
- internal/connectors/plugins/public/stripe/config.go
- internal/connectors/plugins/public/moneycorp/config.go
- internal/connectors/plugins/public/adyen/config.go
- internal/connectors/plugins/public/modulr/plugin_test.go
- internal/connectors/plugins/public/bankingcircle/config.go
- internal/connectors/plugins/public/currencycloud/plugin_test.go
- internal/connectors/plugins/public/wise/config.go
🔇 Additional comments (32)
internal/api/services/connector_configs.go (1)
7-7: LGTM!The changes correctly update the import to use
registry, and the functionConnectorsConfigsnow returnsregistry.Configs, reflecting the updated configuration management.Also applies to: 11-12
internal/api/v2/handler_connectors_configs.go (1)
9-9: LGTM!The import path is properly updated to use the
registrypackage. The variableconfsis appropriately used to represent the configurations, and the response encoding correctly utilizesapi.BaseResponse[registry.Configs].Also applies to: 18-18, 20-21
internal/connectors/plugins/registry/configs.go (1)
3-11: LGTM! Well-structured type system for configuration parameters.The type system is clear and provides good type safety for configuration parameters.
internal/api/v3/handler_connectors_configs.go (1)
9-9: LGTM! Clean transition to registry package.The changes correctly update the import path and type references while maintaining the same functionality.
Also applies to: 18-18, 20-22
internal/api/v3/handler_connectors_configs_test.go (1)
8-8: LGTM! Test updates align with implementation changes.The test correctly uses the new registry package and maintains test coverage.
Also applies to: 32-32
internal/api/v2/handler_connectors_configs_test.go (1)
8-8: LGTM! Consistent test updates across API versions.The v2 API test changes mirror the v3 updates, maintaining consistency across API versions.
Also applies to: 32-32
internal/connectors/plugins/public/generic/plugin.go (1)
19-19: LGTM! Config parameter addition aligns with standardization effort.The addition of
Config{}parameter toRegisterPlugincall follows the new pattern for simplified configuration handling.internal/connectors/plugins/registry/plugins_test.go (1)
49-120: LGTM! Comprehensive test coverage for configuration parsing.The test suite thoroughly validates:
- Required vs optional fields
- Different data types
- JSON metadata handling
- Default values
internal/connectors/plugins/public/atlar/plugin.go (1)
20-20: LGTM! Consistent with configuration standardization.The addition of
Config{}parameter aligns with the project-wide standardization of plugin configurations.internal/connectors/plugins/public/moneycorp/plugin.go (2)
19-19: LGTM! Maintains consistency with configuration changes.The addition of
Config{}parameter follows the standardized approach for plugin configuration handling.
Line range hint
1-1: Verify consistent implementation across all plugins.Let's ensure all plugins have been updated with the new configuration pattern.
internal/connectors/plugins/public/currencycloud/plugin.go (1)
Line range hint
15-19: LGTM! Consistent with the config simplification initiative.The addition of
Config{}as a parameter toRegisterPluginaligns with the PR's objective of simplifying connector configuration definitions.Let's verify this change is consistent across all payment connector plugins:
✅ Verification successful
Verified: Config simplification is consistently implemented across all payment connector plugins
All payment connector plugins in the codebase follow the same pattern, using
Config{}as the last parameter inRegisterPlugincalls, which confirms the alignment with the config simplification initiative.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all plugin registration patterns to ensure consistent config usage ast-grep --pattern 'registry.RegisterPlugin($_, $_, $_, Config{})'Length of output: 3959
internal/connectors/plugins/public/modulr/plugin.go (1)
Line range hint
15-19: LGTM! Consistent with other plugins.The change follows the same pattern as other plugins in this PR.
internal/connectors/plugins/public/bankingcircle/plugin.go (1)
Line range hint
15-19: LGTM! Consistent with other plugins.The change follows the same pattern as other plugins in this PR.
internal/connectors/plugins/public/stripe/plugin.go (2)
Line range hint
15-19: LGTM! Consistent with other plugins.The change follows the same pattern as other plugins in this PR.
Line range hint
34-34: Verify thenilparameter usage in client initialization.The
client.Newfunction is called withnilas its first parameter. This seems unusual and might indicate a potential issue.Let's verify the intended usage:
✅ Verification successful
The
nilparameter usage inclient.Newis correct and intentionalThe
nilbackend parameter triggers the creation of a default backend with a metrics-wrapped HTTP client, which is the expected behavior for normal operation. This allows for custom backend injection when needed (e.g., for testing) while providing sensible defaults.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the client.New function signature and usage ast-grep --pattern 'func New($_, $_, $_) $_ { $$$ }' # Search for other instances of client initialization rg -A 2 'client.New\('Length of output: 21743
internal/connectors/plugins/public/adyen/plugin.go (1)
20-20: LGTM! Standardized plugin registration with Config struct.The addition of
Config{}parameter aligns with the PR's objective of simplifying connector configuration handling.internal/api/backend/backend.go (1)
9-9: Verify the impact of config package migration.The migration from
plugins.Configstoregistry.Configsis a breaking change that affects the Backend interface.Let's verify all usages of the Configs type:
Also applies to: 34-34
✅ Verification successful
✓ Config package migration is complete
The migration from
plugins.Configstoregistry.Configshas been properly implemented across the codebase. All references to the Configs type consistently use the registry package, and no legacy code using the old type was found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to plugins.Configs rg "plugins\.Configs" # Search for all implementations of the Backend interface ast-grep --pattern 'type $_ struct { $$$ } var _ Backend = $_'Length of output: 91
Script:
#!/bin/bash # Find Backend interface definition ast-grep --pattern 'type Backend interface { $$$ }' # Search for any references to Configs type rg -A 2 "Configs" # Find implementations using composition pattern ast-grep --pattern 'type $_ struct { Backend $$$ }'Length of output: 55271
tools/connector-template/template/plugin.gotpl (1)
19-19: LGTM! Template updated for standardized config handling.The template now includes
Config{}parameter, ensuring all new connectors follow the standardized configuration pattern.internal/connectors/plugins/public/dummypay/plugin.go (1)
18-18: LGTM! Well-implemented config handling.The addition of
Config{}parameter is consistent with other connectors. The implementation inNewfunction shows proper usage of the config struct for client initialization.internal/connectors/plugins/public/mangopay/plugin.go (1)
21-21: Verify Config struct definition and validation rules.The addition of
Config{}aligns with the PR's goal of simplifying connector configurations. However, we should ensure that:
- The Config struct is properly defined with appropriate validation tags
- The validation rules are comprehensive for the mangopay connector requirements
✅ Verification successful
Config struct is properly defined with comprehensive validation rules
The mangopay Config struct has all necessary fields with appropriate
requiredvalidation tags, following the project's standard pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Config struct definition and validation rules ast-grep --pattern 'type Config struct { $$$ }' # Check for validation tag usage rg -A 5 'validate:"[^"]*"'Length of output: 18079
internal/connectors/plugins/public/wise/plugin.go (1)
20-20: LGTM! Consistent with the standardized configuration approach.The addition of
Config{}follows the same pattern as other plugins, maintaining consistency in the codebase.internal/api/backend/backend_generated.go (1)
19-19: Verify source file for mock generation.This is an auto-generated file. Ensure that:
- The source file (backend.go) has been updated with the new registry.Configs type
- The mockgen command has been run with the latest source
Also applies to: 212-215
✅ Verification successful
Source and generated files are properly aligned
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check source file for registry.Configs usage rg -l "registry\.Configs" | grep -v "generated" # Verify mockgen command in source rg "^//go:generate.*mockgen.*backend\.go"Length of output: 494
Script:
#!/bin/bash # Check the exact usage of registry.Configs in the source file rg "registry\.Configs" internal/api/backend/backend.go -A 2 -B 2Length of output: 326
internal/connectors/plugins/public/generic/config.go (1)
12-13: LGTM! Validation tags are correctly defined.The required validation tags on APIKey and Endpoint fields align with the PR's goal of standardizing config validation.
internal/connectors/plugins/public/atlar/config.go (2)
12-14: LGTM! Validation tags are consistent with other connectors.The required validation tags on BaseURL, AccessKey, and Secret fields maintain consistency across connectors.
22-23: Consider consistent error handling across connectors.Similar to the generic connector, consider wrapping validation errors with more context.
internal/connectors/plugins/public/modulr/config.go (1)
12-14: LGTM! Changes follow the established pattern.The implementation maintains consistency with other connectors in both validation tags and error handling.
Also applies to: 22-23
internal/connectors/plugins/public/mangopay/config.go (1)
12-14: LGTM! Changes follow the established pattern.The implementation maintains consistency with other connectors in both validation tags and error handling.
Also applies to: 22-23
internal/connectors/plugins/public/currencycloud/config.go (1)
12-14: LGTM! Changes follow the established pattern.The implementation maintains consistency with other connectors in both validation tags and error handling.
Also applies to: 22-23
test/e2e/api_connectors_test.go (1)
89-100: LGTM! Test cases effectively validate directory path requirements.The test cases comprehensively cover directory validation scenarios for both connector versions:
- Empty directory validation
- Invalid directory path validation
- Consistent error message validation
internal/connectors/engine/engine.go (2)
18-18: LGTM! Import aligns with validation integration.The addition of the
validatorpackage import supports the PR's objective of standardizing configuration validation.
136-138: LGTM! Error handling enhancement improves validation.The error handling now properly handles both validator errors and invalid config errors, providing consistent error wrapping.
8cb597a to
8057d8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
internal/connectors/plugins/registry/plugins.go (1)
72-85:⚠️ Potential issueImprove type handling in switch statement
The current implementation has limited type support and uses panic for error handling. This is a duplicate of previous review comments but still needs to be addressed.
Previous suggestions included:
- Adding support for all integer types
- Replacing panic with error return
- Adding proper error handling
🧹 Nitpick comments (4)
internal/connectors/plugins/public/adyen/config.go (1)
16-18: Enhance webhook authentication documentation and validation.While the validation is functionally correct, consider these improvements:
- Expand the RFC 7617 comment to clarify why ":" is excluded (it's a delimiter in Basic Auth).
- Add
omitemptyto WebhookPassword for consistency with WebhookUsername.// https://datatracker.ietf.org/doc/html/rfc7617 +// The ":" character is excluded from username as it's used as a delimiter in Basic Auth WebhookUsername string `json:"webhookUsername" validate:"omitempty,excludes=:"` -WebhookPassword string `json:"webhookPassword" validate:""` +WebhookPassword string `json:"webhookPassword" validate:"omitempty"`internal/connectors/plugins/registry/plugins.go (3)
66-67: Remove debug print statementDebug print statements should not be in production code.
- fmt.Printf("CONTINUE: %s\n", fieldName)
86-86: Remove debug print statementDebug print statements should not be in production code.
- fmt.Printf("FIELD: %s\n", fieldName)
45-93: Consider caching reflection resultsThe current implementation uses reflection for every config setup. While this provides flexibility, it can impact performance in high-throughput scenarios where plugins are frequently registered.
Consider caching the reflection results for each config type to improve performance. This could be implemented using a concurrent map to store the processed field information.
Example approach:
var configCache sync.Map // map[reflect.Type]Config func setupConfig(provider string, conf any) (Config, error) { typ := reflect.TypeOf(conf) if cached, ok := configCache.Load(typ); ok { return cached.(Config).clone(), nil } // existing implementation configCache.Store(typ, config.clone()) return config, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/connectors/plugins/configs.jsonis excluded by!**/*.json
📒 Files selected for processing (27)
Earthfile(0 hunks)internal/api/backend/backend.go(2 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connector_configs.go(1 hunks)internal/api/v2/handler_connectors_configs.go(2 hunks)internal/api/v2/handler_connectors_configs_test.go(2 hunks)internal/api/v3/handler_connectors_configs.go(2 hunks)internal/api/v3/handler_connectors_configs_test.go(2 hunks)internal/connectors/engine/workflow/main_test.go(1 hunks)internal/connectors/plugins/configs.go(0 hunks)internal/connectors/plugins/public/adyen/config.go(1 hunks)internal/connectors/plugins/public/adyen/plugin.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(1 hunks)internal/connectors/plugins/public/atlar/plugin.go(1 hunks)internal/connectors/plugins/public/bankingcircle/plugin.go(1 hunks)internal/connectors/plugins/public/currencycloud/plugin.go(1 hunks)internal/connectors/plugins/public/dummypay/plugin.go(1 hunks)internal/connectors/plugins/public/generic/plugin.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin.go(1 hunks)internal/connectors/plugins/public/modulr/plugin.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin.go(1 hunks)internal/connectors/plugins/public/stripe/plugin.go(1 hunks)internal/connectors/plugins/public/wise/plugin.go(1 hunks)internal/connectors/plugins/registry/configs.go(1 hunks)internal/connectors/plugins/registry/plugins.go(3 hunks)internal/connectors/plugins/registry/plugins_test.go(1 hunks)tools/connector-template/template/plugin.gotpl(2 hunks)
💤 Files with no reviewable changes (2)
- Earthfile
- internal/connectors/plugins/configs.go
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/connectors/plugins/public/bankingcircle/plugin.go
- internal/connectors/plugins/public/wise/plugin.go
- internal/connectors/plugins/public/currencycloud/plugin.go
- internal/connectors/engine/workflow/main_test.go
- internal/api/services/connector_configs.go
- internal/api/v2/handler_connectors_configs_test.go
- tools/connector-template/template/plugin.gotpl
- internal/connectors/plugins/public/moneycorp/plugin.go
- internal/api/v3/handler_connectors_configs_test.go
- internal/connectors/plugins/public/adyen/plugin_test.go
- internal/connectors/plugins/public/adyen/plugin.go
- internal/api/backend/backend.go
- internal/connectors/plugins/registry/plugins_test.go
- internal/api/v3/handler_connectors_configs.go
- internal/connectors/plugins/public/dummypay/plugin.go
- internal/connectors/plugins/public/generic/plugin.go
- internal/api/backend/backend_generated.go
- internal/api/v2/handler_connectors_configs.go
- internal/connectors/plugins/public/stripe/plugin.go
- internal/connectors/plugins/registry/configs.go
- internal/connectors/plugins/public/atlar/plugin.go
- internal/connectors/plugins/public/modulr/plugin.go
- internal/connectors/plugins/public/mangopay/plugin.go
🔇 Additional comments (3)
internal/connectors/plugins/public/adyen/config.go (2)
7-7: LGTM! Good choice of validation library.The go-playground/validator is a robust and well-maintained choice for struct validation.
12-14: LGTM! Validation tags correctly reflect field requirements.The validation tags accurately capture the requirements:
- Required fields: APIKey, CompanyID
- Optional LiveEndpointPrefix with alphanumeric validation
internal/connectors/plugins/registry/plugins.go (1)
119-133: LGTM! Well-implemented config retrieval functionsThe implementation is clean, consistent with other functions, and has proper error handling.
8057d8b to
ea53eaa
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
internal/connectors/plugins/registry/plugins.go (4)
7-7: 🛠️ Refactor suggestionReplace log package with error handling
The
logpackage is imported primarily forlog.Panicf, which should be replaced with proper error handling.
32-43:⚠️ Potential issuePropagate errors from setupConfig
The function should handle potential errors from
setupConfiginstead of allowing panics to propagate.Additionally, consider validating the input parameters:
func RegisterPlugin( provider string, createFunc PluginCreateFunction, capabilities []models.Capability, conf any, -) { +) error { + if provider == "" { + return fmt.Errorf("provider cannot be empty") + } + if createFunc == nil { + return fmt.Errorf("createFunc cannot be nil") + } + config, err := setupConfig(provider, conf) + if err != nil { + return fmt.Errorf("failed to setup config: %w", err) + } pluginsRegistry[provider] = PluginInformation{ capabilities: capabilities, createFunc: createFunc, - config: setupConfig(provider, conf), + config: config, } + return nil }
45-52:⚠️ Potential issueAdd validation for conf parameter
The function should validate the conf parameter before processing.
72-85:⚠️ Potential issueImprove type handling and error management
The type handling is incomplete and uses panic for error handling.
Additionally, consider using a type registry pattern for better maintainability:
type typeHandler func(reflect.Type) (Type, error) var typeHandlers = map[reflect.Kind]typeHandler{ reflect.String: func(t reflect.Type) (Type, error) { return TypeString, nil }, reflect.Int64: func(t reflect.Type) (Type, error) { if t.Name() == "Duration" { return TypeDurationNs, nil } return TypeInteger, nil }, // ... other handlers }
🧹 Nitpick comments (1)
internal/connectors/plugins/public/adyen/config.go (1)
16-18: Consider making the WebhookPassword validation tag more explicit.While an empty validation tag (
validate:"") effectively makes the field optional, it might be clearer to explicitly state this usingvalidate:"omitempty"for consistency with other optional fields.- WebhookPassword string `json:"webhookPassword" validate:""` + WebhookPassword string `json:"webhookPassword" validate:"omitempty"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
internal/connectors/plugins/configs.jsonis excluded by!**/*.json
📒 Files selected for processing (27)
Earthfile(0 hunks)internal/api/backend/backend.go(2 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connector_configs.go(1 hunks)internal/api/v2/handler_connectors_configs.go(2 hunks)internal/api/v2/handler_connectors_configs_test.go(2 hunks)internal/api/v3/handler_connectors_configs.go(2 hunks)internal/api/v3/handler_connectors_configs_test.go(2 hunks)internal/connectors/engine/workflow/main_test.go(1 hunks)internal/connectors/plugins/configs.go(0 hunks)internal/connectors/plugins/public/adyen/config.go(1 hunks)internal/connectors/plugins/public/adyen/plugin.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(1 hunks)internal/connectors/plugins/public/atlar/plugin.go(1 hunks)internal/connectors/plugins/public/bankingcircle/plugin.go(1 hunks)internal/connectors/plugins/public/currencycloud/plugin.go(1 hunks)internal/connectors/plugins/public/dummypay/plugin.go(1 hunks)internal/connectors/plugins/public/generic/plugin.go(1 hunks)internal/connectors/plugins/public/mangopay/plugin.go(1 hunks)internal/connectors/plugins/public/modulr/plugin.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin.go(1 hunks)internal/connectors/plugins/public/stripe/plugin.go(1 hunks)internal/connectors/plugins/public/wise/plugin.go(1 hunks)internal/connectors/plugins/registry/configs.go(1 hunks)internal/connectors/plugins/registry/plugins.go(3 hunks)internal/connectors/plugins/registry/plugins_test.go(1 hunks)tools/connector-template/template/plugin.gotpl(2 hunks)
💤 Files with no reviewable changes (2)
- Earthfile
- internal/connectors/plugins/configs.go
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/connectors/plugins/public/bankingcircle/plugin.go
- internal/connectors/plugins/public/adyen/plugin.go
- internal/connectors/plugins/public/moneycorp/plugin.go
- internal/connectors/plugins/public/atlar/plugin.go
- internal/api/backend/backend_generated.go
- internal/connectors/plugins/public/generic/plugin.go
- internal/connectors/plugins/public/adyen/plugin_test.go
- internal/api/v2/handler_connectors_configs_test.go
- internal/connectors/plugins/public/modulr/plugin.go
- internal/api/v3/handler_connectors_configs_test.go
- internal/connectors/engine/workflow/main_test.go
- internal/api/services/connector_configs.go
- internal/connectors/plugins/public/stripe/plugin.go
- internal/connectors/plugins/public/mangopay/plugin.go
- tools/connector-template/template/plugin.gotpl
- internal/api/backend/backend.go
- internal/connectors/plugins/public/dummypay/plugin.go
- internal/api/v3/handler_connectors_configs.go
- internal/connectors/plugins/registry/plugins_test.go
- internal/connectors/plugins/public/currencycloud/plugin.go
- internal/connectors/plugins/registry/configs.go
- internal/connectors/plugins/public/wise/plugin.go
- internal/api/v2/handler_connectors_configs.go
🔇 Additional comments (5)
internal/connectors/plugins/public/adyen/config.go (3)
7-7: LGTM! Good choice of validation library.The go-playground/validator is a robust and widely-used validation library that will help standardize validation across connectors.
12-14: LGTM! Well-structured validation rules.The validation tags for APIKey, CompanyID, and LiveEndpointPrefix are appropriately configured based on their requirements.
26-27: Improve validation error handling for better user experience.While the validation setup is good, consider wrapping validation errors with models.ErrInvalidConfig for consistency with unmarshal errors.
validate := validator.New(validator.WithRequiredStructEnabled()) -return config, validate.Struct(config) +if err := validate.Struct(config); err != nil { + return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) +} +return config, nilinternal/connectors/plugins/registry/plugins.go (2)
21-21: LGTM: Config field additionThe addition of the
configfield toPluginInformationis well-structured and maintains proper encapsulation.
119-133: LGTM: Well-structured config retrieval functionsThe implementation of
GetConfigsandGetConfigis clean, consistent with existing patterns, and handles error cases appropriately.
ea53eaa to
f09f9f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/connectors/plugins/public/adyen/config.go (1)
26-27: 🛠️ Refactor suggestionImprove validation error handling for better user experience.
Consider wrapping validation errors with models.ErrInvalidConfig for consistency with unmarshal errors.
validate := validator.New(validator.WithRequiredStructEnabled()) -return config, validate.Struct(config) +if err := validate.Struct(config); err != nil { + return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) +} +return config, nilinternal/connectors/plugins/registry/plugins.go (1)
31-42:⚠️ Potential issueHandle potential errors from setupConfig
The function should handle potential errors from setupConfig to prevent panics from propagating.
This issue was previously raised in past reviews and remains unaddressed.
🧹 Nitpick comments (2)
internal/connectors/plugins/public/adyen/config.go (1)
18-18: Consider adding password validation rules.While the WebhookPassword is optional, when provided, consider adding basic password validation rules (e.g., minimum length, complexity) to ensure secure webhook authentication.
- WebhookPassword string `json:"webhookPassword" validate:""` + WebhookPassword string `json:"webhookPassword" validate:"omitempty,min=8"`internal/connectors/plugins/registry/plugins.go (1)
65-94: Enhance validation tag handlingThe validation tag parsing is simplistic and only checks for "required". Consider parsing additional validation rules.
Suggestions:
- Parse min/max values for numeric fields
- Support regex patterns for strings
- Add enum validation support
- Handle nested struct validation
Example implementation:
type ValidationRules struct { Required bool Min *float64 Max *float64 Pattern *regexp.Regexp Enum []string } func parseValidationTag(tag string) ValidationRules { // Parse validation rules from tag // Example: "required,min=0,max=100,pattern=^[A-Z]+$" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/connectors/plugins/public/adyen/config.go(1 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(1 hunks)internal/connectors/plugins/registry/plugins.go(3 hunks)internal/connectors/plugins/registry/plugins_test.go(1 hunks)tools/connector-template/template/plugin.gotpl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/connectors/plugins/public/adyen/plugin_test.go
- tools/connector-template/template/plugin.gotpl
- internal/connectors/plugins/registry/plugins_test.go
🔇 Additional comments (3)
internal/connectors/plugins/public/adyen/config.go (2)
7-7: LGTM! Good choice of validation library.The go-playground/validator is a well-maintained and widely used validation library in the Go ecosystem.
12-18: LGTM! Well-structured validation rules and documentation.The validation tags align with the discussed requirements in the PR comments. Good addition of the RFC reference for webhook authentication.
internal/connectors/plugins/registry/plugins.go (1)
122-136: LGTM! Well-implemented configuration retrievalThe GetConfigs and GetConfig functions are well implemented with:
- Proper error handling
- Case-insensitive provider name handling
- Consistent return types
| "log" | ||
| "reflect" | ||
| "regexp" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using errors instead of log.Panic
The log package is imported primarily for log.Panic calls. This is not recommended for production code as it can cause the application to crash unexpectedly.
Consider using error returns instead of panics for better error handling. This was previously suggested in past reviews but is still relevant for the new code.
| switch vt.Kind() { | ||
| case reflect.String: | ||
| dataType = TypeString | ||
| case reflect.Uint, reflect.Uint16, reflect.Uint32, reflect.Uint64: | ||
| dataType = TypeUnsignedInteger | ||
| case reflect.Int64: | ||
| if field.Type.Name() == "Duration" { | ||
| dataType = TypeDurationNs | ||
| break | ||
| } | ||
| fallthrough | ||
| default: | ||
| log.Panicf("unhandled type for field %q: %q", val.Type().Field(i).Name, field.Type.Name()) | ||
| } |
There was a problem hiding this comment.
Expand type support in setupConfig
The type switch is incomplete and missing support for common types:
- float32/float64
- bool
- maps
- slices
Apply this diff to add support for more types:
switch vt.Kind() {
case reflect.String:
dataType = TypeString
+case reflect.Bool:
+ dataType = TypeBool
+case reflect.Float32, reflect.Float64:
+ dataType = TypeFloat
+case reflect.Map:
+ dataType = TypeMap
+case reflect.Slice:
+ dataType = TypeArray
case reflect.Uint, reflect.Uint16, reflect.Uint32, reflect.Uint64:
dataType = TypeUnsignedInteger
case reflect.Int64:Committable suggestion skipped: line range outside the PR's diff.
Fixes: ENG-1549
This eliminates the need for a user to define a config.json or a custom validation function as now everything is done within the
Configstruct. Errors are standardized as well. Can be customized later if we decide it's not human friendly enough.Validation errors at install time now look like this: