feat: (temporal) add initial delay when being rate-limited by PSP #236
Conversation
WalkthroughThis pull request introduces a comprehensive enhancement to the connector configuration update functionality across multiple components of the system. The changes span from the API layer to the storage and backend implementations, enabling a new endpoint for updating connector configurations. The implementation includes robust error handling, validation, and integration with existing system components, providing a mechanism for users to modify connector settings dynamically. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
laouji
force-pushed
the
feat/temporal-rate-limiting
branch
from
68e50f2 to
4935ea7
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
internal/api/v3/handler_connectors_config_update.go (3)
20-26: Potential improvements for error context.
When parsing the connector ID, consider including contextual information about the received value. This can help during debugging if the connector ID is malformed.
28-33: Consider adding a request body size limit.
Reading the entire request body without a limit could expose the service to large payload attacks. You may want to configure a maximum size usinghttp.MaxBytesReader.
50-55: Connector construction is clear.
Storing the raw config can be beneficial for auditing, though consider whether secure or sensitive fields need redaction before logging.internal/connectors/engine/activities/plugin_fetch_next_others.go (1)
24-24: Properly propagating polling errors.
By switching totemporalPluginPollingError(err), the code clarifies that failures are related to the polling mechanism rather than general plugin issues. It is good practice to check if all relevant details of the original error are preserved intemporalPluginPollingErrorfor debugging and monitoring.internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go (1)
24-24: Refined polling error handling.
Updating the error totemporalPluginPollingErroraligns with the revised strategy for handling rate-limited or other polling-related failures. Ensure that all consumers of this function are prepared for this specific error type, especially around concurrency or retry logic, if applicable.internal/connectors/engine/plugins/plugin_generated.go (1)
24-24: Newisgomockfield is harmless but consider documenting its usage.
If it’s serving as a sentinel marker for mocking, a brief comment clarifying its role would aid maintainability.internal/connectors/plugins/public/atlar/client/client.go (1)
105-107: Consider aligning error wrapping format with other PSP clients.Here, the order of arguments in
fmt.Errorf("atlar error: %w: %w", err, httpwrapper.ErrStatusCodeTooManyRequests)differs from the pattern used elsewhere. Aligning the error wrap style can help maintain cross-service consistency.internal/connectors/plugins/public/stripe/client/client_generated.go (1)
24-24: Remove unused field if not needed.This field
isgomock struct{}appears to be unused. If it’s intended for tracking or for future usage, add a comment clarifying its purpose. Otherwise, consider removing it to reduce clutter.internal/connectors/engine/workers.go (1)
192-201: Consider rollback on plugin re-registration error.Here, if re-registration fails, we log the error and return. However, if the plugin was previously registered, the system might remain in a partially updated state. Consider removing or reverting the previous plugin version or leaving a fallback to avoid orphaned workers or misconfigured plugins.
internal/storage/connectors_test.go (1)
129-173: Robust test coverage forConnectorsConfigUpdate.This test thoroughly checks configuration updates, including the scenario where a connector doesn't exist yet. It's a solid addition to ensure correctness. You may consider adding concurrency or partial update checks in the future to further expand coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (29)
internal/api/backend/backend.go(1 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update_test.go(1 hunks)internal/api/v3/handler_test.go(1 hunks)internal/api/v3/router.go(1 hunks)internal/connectors/engine/activities/errors.go(3 hunks)internal/connectors/engine/activities/plugin_fetch_next_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_balances.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_others.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_payments.go(1 hunks)internal/connectors/engine/plugins/plugin_generated.go(1 hunks)internal/connectors/engine/workers.go(1 hunks)internal/connectors/httpwrapper/client.go(1 hunks)internal/connectors/plugins/errors.go(1 hunks)internal/connectors/plugins/public/adyen/client/client.go(1 hunks)internal/connectors/plugins/public/atlar/client/client.go(1 hunks)internal/connectors/plugins/public/atlar/client/client_generated.go(1 hunks)internal/connectors/plugins/public/moneycorp/client/client.go(1 hunks)internal/connectors/plugins/public/stripe/client/client.go(1 hunks)internal/connectors/plugins/public/stripe/client/client_generated.go(1 hunks)internal/connectors/plugins/registry/errors.go(1 hunks)internal/models/plugin_generated.go(1 hunks)internal/storage/connectors.go(1 hunks)internal/storage/connectors_test.go(2 hunks)internal/storage/storage.go(1 hunks)internal/storage/storage_generated.go(2 hunks)
🔇 Additional comments (46)
internal/api/v3/handler_connectors_config_update.go (6)
1-2: File-level overview looks good.
Package declaration is straightforward, and the chosen package name v3 suggests a versioned API approach, which is a helpful convention for backwards compatibility.
3-13: Imports are well organized.
Imports cover JSON handling, IO for reading request bodies, and references to internal packages for backend functionality, models, and observability. This set of imports seems aligned with the handler’s declared responsibilities.
15-19: Overall function structure is clear.
The connectorsConfigUpdate function returns an http.HandlerFunc and sets up observability with a tracing span, which is good for diagnosing issues.
35-42: Validation approach looks solid.
Using a default config, unmarshaling from JSON, and capturing errors with api.BadRequest ensures robust error handling.
44-48: Validation errors are handled appropriately.
Returning a 400 status code with ErrValidation indicates a user-side misconfiguration and is consistent with HTTP best practices.
56-62: Backend update call is correct.
Using ConnectorsConfigUpdate in the backend centralizes the logic for updates. The no-content response is an appropriate choice for a successful update.
internal/connectors/engine/activities/errors.go (5)
5-5: Importing “time” is necessary for your retry delay.
No issues here; usage is consistent with the new backoff strategy.
16-16: New constant for rate-limiting.
Defining ErrTypeRateLimited is a good architectural choice for distinct error classification.
21-22: Refactored error handling with a helper function.
Delegating to temporalPluginErrorCheck clarifies the logic and avoids duplication.
24-26: Dedicated polling error handler is a good design.
Separating polling scenarios from normal plugin errors helps your code remain more readable and maintainable.
38-50: Rate-limit handling strategies look appropriate.
You return non-retryable errors if it’s a polled scenario, preventing infinite loops of rate-limited retries. Otherwise, a 5-second delay is introduced before the next retry, which aligns with the new rate-limiting strategy in the PR summary.
internal/connectors/plugins/errors.go (1)
11-11: New ErrUpstreamRatelimit variable.
Adding a specific error type for upstream rate-limits is clear and consistent with the introduced rate-limiting logic in other parts of the code.
internal/api/services/connectors_config_update.go (3)
1-2: Service package looks well-named.
It implies higher-level application logic.
3-8: Imports are minimal and targeted.
They match exactly what is needed for the update functionality, avoiding bloat.
9-15: ConnectorsConfigUpdate method is straightforward.
The layering (Service -> storage) and error wrapping with newStorageError is consistent with domain-driven design.
internal/connectors/plugins/registry/errors.go (1)
23-24: Dedicated handling for HTTP 429 Too Many Requests.
Wrapping both the original error and a new ErrUpstreamRatelimit clarifies the cause while preserving the root error details. This is a resilient approach to error translation.
internal/connectors/engine/activities/plugin_fetch_next_payments.go (1)
24-24: Ensure correct categorization of polling errors.
Changing from temporalPluginError(err) to temporalPluginPollingError(err) properly distinguishes polling-specific failures from general plugin issues. This helps with more precise error handling and logging for rate-limited or polling-related problems. Keep an eye on whether this impacts upstream retries or backoff logic and confirm that you still capture relevant context in logs or monitoring systems.
internal/connectors/engine/activities/plugin_fetch_next_balances.go (1)
24-24: Consistent error categorization for polling.
This mirrors the approach in plugin_fetch_next_payments.go, ensuring that polling issues are handled distinctly. Verify whether additional handling is needed in places where temporalPluginError might still be used for other types of errors, to maintain consistency throughout.
internal/connectors/plugins/public/moneycorp/client/client.go (1)
39-42: Explicit rate-limiting error handling looks good.
Providing a dedicated error for 429 (Too Many Requests) makes the code more expressive and facilitates handling rate-limiting logic.
internal/api/v3/handler_connectors_config_update_test.go (5)
16-21: Great use of Ginkgo context blocks.
This structure fosters clear, context-driven tests and improves readability.
52-57: Validation test for missing name is solid.
Validating connector names ensures consistent configurations. Consider also verifying length or character constraints if relevant.
59-63: Validation test for invalid connector ID is thorough.
This guards against malformed or incorrect IDs, preventing unexpected runtime behavior.
65-71: Internal server error handling ensures robust back-end interactions.
Properly mocking backend errors and asserting on the 500 response bolsters reliability.
73-77: Successful update test covers happy path effectively.
Verifying the 204 No Content response crisply indicates successful configuration updates.
internal/connectors/plugins/public/adyen/client/client.go (1)
70-73: Adyen-specific rate-limiting error is correctly signaled.
Returning a meaningful error message for 429 improves visibility and consistency with the broader rate-limit handling strategy.
internal/connectors/plugins/public/stripe/client/client.go (1)
90-92: Good introduction of rate-limiting error wrapping.
Properly wrapping stripe.ErrorCodeRateLimit with httpwrapper.ErrStatusCodeTooManyRequests improves clarity. Ensure upstream handlers catch and handle this wrapped error appropriately, particularly around retry logic for rate limits.
internal/api/v3/router.go (1)
91-91: Add or verify test coverage for the new PATCH /config endpoint.
The new endpoint is crucial for updating connector configurations. Verify it’s covered by tests to ensure correct validation, security, and error-handling.
internal/connectors/httpwrapper/client.go (2)
23-26: New error constant ensures better classification of rate-limiting scenarios.
Declaring ErrStatusCodeTooManyRequests clarifies subsequent handling logic for HTTP 429 responses.
29-31: Consistent classification of HTTP 429 status code.
This check aligns with the new error constant, helping unify error handling across various connectors.
internal/api/backend/backend.go (1)
36-36: Ensure consistent naming & usage for new config update method.
This new method clearly exposes connector config updates in the Backend interface. However, ensure clients calling this method consistently name it and properly handle the returned error to avoid silent failures.
internal/connectors/engine/workers.go (1)
184-188: Verify teardown path for scheduled deletions.
Removing the worker upon scheduling a connector for deletion is valid to avoid leftover processes. Ensure that scheduled tasks are safely canceled, if any.
internal/storage/connectors.go (1)
128-150: Evaluate concurrency control for config updates.
While the transaction ensures atomic encryption and commit, there is a small potential race window between the ConnectorsGet call and the NewUpdate(). If multiple updates to the same connector occur concurrently, the last caller silently wins. For production environments, consider optimistic locking or a version field to detect conflicting updates and handle them gracefully.
internal/connectors/plugins/public/atlar/client/client_generated.go (1)
31-31: Looks good – new field added for mock identification.
This addition of isgomock struct{} is consistent with the GoMock pattern seen elsewhere. No immediate concerns.
internal/storage/storage.go (1)
44-44: Watch for breaking changes with new interface method.
Adding ConnectorsConfigUpdate(ctx context.Context, c models.Connector) error is a useful enhancement. However, be mindful that this addition will break compilation for any existing custom implementations of Storage that do not implement this new method.
internal/storage/connectors_test.go (3)
5-5: Import for JSON usage is valid.
The new import for "encoding/json" is appropriately used for handling connector configuration data.
13-13: Import for canonical JSON is properly introduced.
The "github.com/gibson042/canonicaljson-go" library helps ensure canonical serialization.
15-15: Assertion library is well-chosen.
"github.com/stretchr/testify/assert" is a good fit for clearer test assertions.
internal/models/plugin_generated.go (1)
23-23: Consistent mock pattern.
The isgomock struct{} field aligns with other mock enhancements. No issues.
internal/api/backend/backend_generated.go (3)
30-30: Question the necessity of empty struct isgomock
The added field isgomock struct{} in the MockBackend struct appears to serve as a marker or placeholder. Verify if it is truly needed for the mock generation process or if it can be safely removed without losing functionality.
197-203: Validate error handling in ConnectorsConfigUpdate
The mock method correctly calls the underlying mock controller. Ensure that, in production usage, we handle potential errors properly (e.g., logging, wrapping). Confirm that any calling code checks for errors appropriately.
205-209: Confirm consistency of recorder method
The recorder method ConnectorsConfigUpdate matches the signature of the mock method. This is consistent with GoMock’s conventions and looks good.
internal/storage/storage_generated.go (3)
27-27: Question the usage of empty struct isgomock
Similar to the MockBackend struct, the isgomock struct{} field in MockStorage might be superfluous. Confirm if it’s required by the mock generator configuration. Otherwise, consider removing it to avoid confusion.
306-312: Ensure correct error flow in ConnectorsConfigUpdate
The mock function’s logic is straightforward: it captures parameters and returns an error from the mock controller. Make sure that real implementations properly handle database or external errors (e.g., rollback if partial updates occur).
314-318: Confirm alignment with the interface signature
The recorder method is consistent with the mock function. This consistency is crucial in tests to accurately mock calls and verify usage. No issues detected here.
internal/connectors/engine/activities/plugin_fetch_next_accounts.go (1)
24-24: Use precise error categorization for retry logic
Replacing temporalPluginError(err) with temporalPluginPollingError(err) suggests more specific error categorization. Ensure upstream logic differentiates between standard plugin errors and polling-related rate-limit errors when deciding on retries or plugin suspension.
internal/api/v3/handler_test.go (1)
30-30: Promote clarity by converting response bytes to string
Switching to string(data) before asserting the substring is a solid move for readability and correctness. This ensures the substring comparison is always performed on a string type.
laouji
force-pushed
the
feat/temporal-rate-limiting
branch
from
3b3e794 to
5e09cf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/testserver/api.go (1)
34-40: Add support or clearly document for version 2.
The current implementation explicitly returns an error for version 2. Though this might be intentional (per the PR description that version 2 does not support updates), consider adding a comment in the code to clarify why it is unsupported. This documentation helps maintainers and future developers quickly understand the rationale.internal/api/services/connectors_config_update.go (1)
1-15: Validate error handling strategy
TheConnectorsConfigUpdatefunction delegates tos.storage.ConnectorsConfigUpdate. If an error arises, it is wrapped as a “storage error.” Consider whether certain error codes or types require different handling (for example, conflict or not found). If so, you might want to customize the response or wrap more specifically.internal/api/v3/handler_connectors_config_update.go (1)
28-33: Validate non-empty request body
Reading the request body and returning aBadRequestif it’s missing or invalid is good. Consider logging the length of the body or restricting excessively large payloads to be extra safe against potential resource misuse.internal/storage/connectors_test.go (1)
129-173: Consider adding more test cases for edge scenarios.The current test implementation is solid and follows the established patterns. Consider adding these additional test cases:
- Invalid JSON config
- Empty config
- Null config
Here's a suggested implementation:
func TestConnectorsConfigUpdate(t *testing.T) { // ... existing test code ... + t.Run("invalid json config", func(t *testing.T) { + c := models.Connector{ + ID: defaultConnector.ID, + Config: json.RawMessage(`{invalid`), + } + require.Error(t, store.ConnectorsConfigUpdate(ctx, c)) + }) + + t.Run("empty config", func(t *testing.T) { + c := models.Connector{ + ID: defaultConnector.ID, + Config: json.RawMessage(``), + } + require.Error(t, store.ConnectorsConfigUpdate(ctx, c)) + }) + + t.Run("null config", func(t *testing.T) { + c := models.Connector{ + ID: defaultConnector.ID, + Config: json.RawMessage(`null`), + } + require.Error(t, store.ConnectorsConfigUpdate(ctx, c)) + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
internal/api/backend/backend.go(1 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update_test.go(1 hunks)internal/api/v3/handler_test.go(1 hunks)internal/api/v3/router.go(1 hunks)internal/connectors/engine/activities/errors.go(3 hunks)internal/connectors/engine/activities/plugin_fetch_next_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_balances.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_others.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_payments.go(1 hunks)internal/connectors/engine/plugins/plugin_generated.go(1 hunks)internal/connectors/engine/workers.go(1 hunks)internal/connectors/httpwrapper/client.go(1 hunks)internal/connectors/plugins/errors.go(1 hunks)internal/connectors/plugins/public/adyen/client/client.go(1 hunks)internal/connectors/plugins/public/atlar/client/client.go(1 hunks)internal/connectors/plugins/public/atlar/client/client_generated.go(1 hunks)internal/connectors/plugins/public/moneycorp/client/client.go(1 hunks)internal/connectors/plugins/public/stripe/client/client.go(1 hunks)internal/connectors/plugins/public/stripe/client/client_generated.go(1 hunks)internal/connectors/plugins/registry/errors.go(1 hunks)internal/models/plugin_generated.go(1 hunks)internal/storage/connectors.go(1 hunks)internal/storage/connectors_test.go(2 hunks)internal/storage/storage.go(1 hunks)internal/storage/storage_generated.go(2 hunks)pkg/testserver/api.go(1 hunks)test/e2e/api_connectors_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- internal/models/plugin_generated.go
- internal/connectors/plugins/public/atlar/client/client.go
- internal/connectors/plugins/errors.go
- internal/connectors/engine/activities/plugin_fetch_next_accounts.go
- internal/connectors/plugins/public/stripe/client/client_generated.go
- internal/connectors/plugins/public/atlar/client/client_generated.go
- internal/connectors/engine/activities/plugin_fetch_next_payments.go
- internal/api/v3/router.go
- internal/connectors/engine/activities/plugin_fetch_next_balances.go
- internal/connectors/plugins/registry/errors.go
- internal/storage/connectors.go
- internal/connectors/plugins/public/stripe/client/client.go
- internal/connectors/httpwrapper/client.go
- internal/connectors/engine/activities/plugin_fetch_next_others.go
- internal/storage/storage.go
- internal/connectors/plugins/public/moneycorp/client/client.go
- internal/api/v3/handler_test.go
- internal/connectors/engine/activities/errors.go
- internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go
- internal/api/v3/handler_connectors_config_update_test.go
- internal/storage/storage_generated.go
- internal/connectors/plugins/public/adyen/client/client.go
- internal/connectors/engine/workers.go
🔇 Additional comments (13)
test/e2e/api_connectors_test.go (3)
87-94: Good structure for test variables
Defining these variables up front in a When block is organized and clear. Ensure that any test-specific variable reuse or scoping is consistent across your tests to avoid confusion in parallel or concurrent test scenarios.
95-103: Consider verifying existing connectors before re-installation
In the JustBeforeEach, you immediately install a connector on a fresh UUID. This is fine for an isolated test scenario, but if there's any possibility that your environment or test data might leak between tests, you may want to assert that no connector with the same UUID exists beforehand.
105-115: Thorough test for connector config update
The test properly changes the PollingPeriod field, updates the connector, then retrieves it to confirm the new configuration. This helps ensure coverage of edit flows.
internal/api/v3/handler_connectors_config_update.go (3)
20-26: Add context on version constraints
We retrieve the connector ID and immediately interpret it as if we’re in a version 3 scenario. If other versions may invoke this endpoint in the future, consider adding runtime checks or ensuring the router routes them differently.
35-42: Ensure consistent default config handling
You initialize a models.DefaultConfig() and then unmarshal the user-supplied JSON. Make sure the default config values do not override user-provided configuration if the JSON doesn’t explicitly set them (unless that is desired behavior).
44-48: Comprehensive validation
config.Validate() is an excellent step to safeguard the data. If the validation is extensive or prone to frequent updates, consider detailing error types or codes to differentiate validation errors in logs or telemetry.
internal/connectors/engine/plugins/plugin_generated.go (1)
24-24: LGTM on the mock marker field.
The addition of isgomock struct{} is a standard gomock implementation detail.
internal/api/backend/backend.go (1)
36-36: LGTM on the interface addition.
The ConnectorsConfigUpdate method is well-placed and its signature aligns with the interface's style. This addition enables dynamic updates to connector configurations, which is essential for implementing the rate-limiting strategy.
Let's verify the error handling implementation:
✅ Verification successful
Error handling is properly implemented in the service layer
Based on the verification results, I found that:
- The service implementation in
internal/api/services/connectors_config_update.goproperly handles errors by:- Checking for errors from the storage layer (
if err != nil) - Wrapping storage errors with context using
newStorageError(err, "update connector") - Properly propagating errors up the call stack
- Checking for errors from the storage layer (
While we couldn't directly verify the storage layer implementation, the service layer demonstrates appropriate error handling patterns, which is consistent with the interface design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling in implementations
ast-grep --pattern 'func $_(ctx context.Context, connector models.Connector) error {
$$$
return err
}'
Length of output: 108
Script:
#!/bin/bash
# Let's try a broader search for implementations of ConnectorsConfigUpdate
rg "func.*ConnectorsConfigUpdate.*\(ctx context\.Context, connector models\.Connector\) error" -A 10
Length of output: 1740
Script:
#!/bin/bash
# Let's check the storage implementation to complete our error handling verification
rg "func.*ConnectorsConfigUpdate.*\(ctx context\.Context, connector models\.Connector\) error" -g "internal/storage/*" -A 10
Length of output: 126
internal/storage/connectors_test.go (1)
5-5: LGTM on the new imports.
The added imports support JSON handling and testing assertions.
Also applies to: 13-13, 15-15
internal/api/backend/backend_generated.go (4)
30-30: LGTM: Auto-generated marker field
The addition of the isgomock field is an auto-generated change by mockgen.
197-203: LGTM: Correctly implemented mock method
The mock implementation follows the standard gomock patterns and correctly handles the method signature and return types.
205-209: LGTM: Correctly implemented mock recorder method
The mock recorder implementation follows the standard gomock patterns and correctly sets up the method recording.
197-209: Verify test coverage for rate-limiting scenarios
Since these mocks are part of implementing rate-limiting strategies, ensure they are utilized in tests covering rate-limit handling scenarios.
internal/connectors/engine/activities/plugin_fetch_next_accounts.go
Outdated
Show resolved
Hide resolved
… onUpdate function
…e before disabling retry
laouji
force-pushed
the
feat/temporal-rate-limiting
branch
from
5e09cf7 to
0aad8f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
internal/connectors/plugins/public/stripe/plugin.go (1)
Rate limiting implementation is insufficient
The current implementation only detects rate limit errors from Stripe API and wraps them with a
TooManyRequestserror, but does not implement any proactive rate limiting or retry mechanism. This could lead to:
- No prevention of hitting rate limits in the first place
- No automatic retries with backoff when rate limits are hit
- Potential service disruption during high load
Locations needing attention:
internal/connectors/plugins/public/stripe/client/client.go: Only contains basic rate limit error detection without proper handling🔗 Analysis chain
Line range hint
1-150: Verify rate-limiting implementationThe PR objectives mention implementing rate-limiting strategy for PSP calls, but the current changes only add logging capabilities. Ensure that rate-limiting logic is implemented in the client package or will be added in subsequent changes.
Let's check if rate-limiting logic exists in the client package:
Let's check the actual implementation around the rate limit handling:
Let's check if there's any retry mechanism implemented in the client package:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for rate-limiting implementation in the stripe client package # Look for rate-limiting related code in the client package echo "Searching for rate-limiting implementation..." rg -g '*.go' -i "rate.?limit|retry|backoff|delay" "internal/connectors/plugins/public/stripe/client/"Length of output: 431
Script:
#!/bin/bash # Get context around rate limit handling in the client rg -g '*.go' -B 5 -A 5 "ErrorCodeRateLimit" "internal/connectors/plugins/public/stripe/client/"Length of output: 1030
Script:
#!/bin/bash # Check for retry mechanism implementation ast-grep --pattern 'func $_($$) { $$$ retry $$$ $$$ }' # Also check for any retry-related imports rg -g '*.go' "import.*retry" "internal/connectors/plugins/public/stripe/client/"Length of output: 143
🧹 Nitpick comments (21)
cmd/worker.go (1)
29-29: Default delay value consideration.A 5-second default delay is a reasonable starting point. However, consider whether this might still be too frequent under heavy load or might slow operations unnecessarily in lighter load scenarios. You may eventually make it configurable or reevaluate the default based on real usage metrics.
internal/connectors/plugins/public/mangopay/plugin.go (1)
42-42: Propagate logger usage throughout plugin code once created.A logger has been successfully assigned in the
Pluginconstructor. As a next step, consider invoking it in relevant places (e.g., error conditions, request handling, or data transformations) to maximize your logging benefits.internal/api/services/connectors_config_update.go (1)
9-14: Consider adding logs or context for easier debugging.
While this error handling is solid, adding at least a debug-level log before returning could help pinpoint issues faster if the storage operation fails.func (s *Service) ConnectorsConfigUpdate(ctx context.Context, connector models.Connector) error { + // log.Debug("Attempting to update connector", "connector", connector) err := s.storage.ConnectorsConfigUpdate(ctx, connector) if err != nil { return newStorageError(err, "update connector") } return nil }internal/connectors/plugins/registry/plugins.go (1)
12-12: Add usage example in documentation
The updated signature now requires alogging.Logger, which is a beneficial addition for debugging. Consider augmenting documentation or comments to clarify usage of this parameter for third-party developers implementing custom plugins.internal/connectors/engine/plugins/plugin.go (2)
22-22: Enrich interface-level documentation
TheRegisterPlugininterface now includes an additional boolean flag to control whether existing plugins are updated. Consider refining the interface-level docstring to explain how this parameter behaves and any consequences of setting it to false vs. true.
66-66: Consider logging plugin re-registration attempts
When a plugin already exists andupdateExistingis set to false, the function silently returns. For improved visibility, consider logging that a plugin re-registration request was skipped, particularly for troubleshooting.internal/connectors/plugins/public/generic/plugin.go (1)
29-29: Provide context for returned error
InsideNew(...), any parse or validation error is returned as-is. To differentiate plugin config errors from other runtime issues, consider wrapping with context (e.g.,fmt.Errorf("generic plugin config error: %w", err)).internal/connectors/plugins/public/wise/payments.go (2)
9-9: Use structured logging fields if available.
The import ofgithub.aaakk.us.kg/formancehq/go-libs/v2/loggingis a solid replacement forhashicorp/go-hclog. Ensure that logging statements leverage structured fields (key-value pairs) where possible, as this can significantly improve observability and log query capabilities.
108-108: Leverage structured logs for clarity.
logger.WithField("transfer_id", transfer.ID).Info("...")is excellent for structured messaging. Consider adding more fields if beneficial to debugging (e.g., currency details, user context, etc.).internal/connectors/plugins/public/modulr/plugin.go (1)
42-42: Logger field initialization is appropriate.
You could consider adding debug logs during plugin creation to confirm successful plugin instantiation.internal/connectors/plugins/public/bankingcircle/plugin.go (2)
29-29: New function now supports logging.
Ensure that any error conditions or key plugin events are logged for observability.
42-42: Logger is being correctly set.
Adding debug or info logs during plugin creation might assist in diagnosing installation conflicts.internal/connectors/plugins/public/adyen/plugin.go (2)
33-33: Constructor now incorporating a logger param.
Consider adding error logs for the outcome ofunmarshalAndValidateConfigto make debugging easier.
50-50: Logger field is assigned.
Consider leveraging this logger for potential warnings or errors before/after webhook configs are initialized.tools/connector-template/template/plugin.gotpl (2)
24-24: Newloggerfield for the plugin struct.
Remember to remove or replace the_ = configplaceholder once the configuration is used.
40-40: Logger field is now assigned.
Consider adding minimal debug or info logs to track plugin creation, especially for diagnosing installation or config issues.internal/connectors/plugins/public/dummypay/plugin.go (1)
16-17: Ensure logger is not nil
Currently, there's no explicit nil check on theloggerargument. Although not strictly required, consider validating to avoid runtime panics in edge cases.registry.RegisterPlugin("dummypay", func(name string, logger logging.Logger, rm json.RawMessage) (models.Plugin, error) { + if logger == nil { + return nil, fmt.Errorf("logger cannot be nil") + } return New(name, logger, rm) }, capabilities)internal/connectors/plugins/public/wise/plugin.go (1)
18-19: Registry callback with logger
This is consistent with the approach in other plugins. Consider verifying or gracefully handling situations where logger may be nil.registry.RegisterPlugin(ProviderName, func(name string, logger logging.Logger, rm json.RawMessage) (models.Plugin, error) { + if logger == nil { + return nil, errors.New("logger is nil") + } return New(name, logger, rm) }, capabilities)internal/connectors/plugins/public/stripe/payments.go (1)
488-488: Error logging for unsupported transaction type
Appending the transaction type in the log fields is informative, but note that using%wwithErrorfmight be confusing. Consider%vor more direct string concatenation if you do not intend to wrap that error.- p.logger.WithField("type", balanceTransaction.Type).Errorf("unsupported balance transaction type: %w", ErrUnsupportedTransactionType) + p.logger.WithFields(map[string]any{"type": balanceTransaction.Type}). + Errorf("unsupported balance transaction type: %v", ErrUnsupportedTransactionType)internal/connectors/plugins/public/stripe/plugin.go (1)
Line range hint
28-40: Consider utilizing the logger for rate-limit eventsThe logger is properly injected, but it's not being utilized yet. Given the PR's objective of implementing rate-limiting strategy, consider adding logging statements for rate-limit events, retries, and polling operations.
Example usage:
func (p *Plugin) FetchNextAccounts(ctx context.Context, req models.FetchNextAccountsRequest) (models.FetchNextAccountsResponse, error) { if p.client == nil { + p.logger.Debug("Skipping FetchNextAccounts: client not initialized") return models.FetchNextAccountsResponse{}, plugins.ErrNotYetInstalled } + p.logger.Info("Fetching next accounts", "request", req) return p.fetchNextAccounts(ctx, req) }pkg/testserver/api.go (1)
43-49: Enhance error message for version 2The error message for version 2 could be more descriptive to help users understand why it's not supported and what alternatives are available.
Consider this improvement:
- return fmt.Errorf("connector update not supported by version %d", ver) + return fmt.Errorf("connector config update is not supported in API version %d. Please use API version 3 or later", ver)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (62)
cmd/root.go(1 hunks)cmd/worker.go(3 hunks)internal/api/backend/backend.go(1 hunks)internal/api/backend/backend_generated.go(2 hunks)internal/api/services/connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update.go(1 hunks)internal/api/v3/handler_connectors_config_update_test.go(1 hunks)internal/api/v3/handler_test.go(1 hunks)internal/api/v3/router.go(1 hunks)internal/connectors/engine/activities/activity.go(3 hunks)internal/connectors/engine/activities/errors.go(2 hunks)internal/connectors/engine/activities/plugin_create_bank_account.go(1 hunks)internal/connectors/engine/activities/plugin_create_payout.go(1 hunks)internal/connectors/engine/activities/plugin_create_transfer.go(1 hunks)internal/connectors/engine/activities/plugin_create_webhooks.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_balances.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_others.go(1 hunks)internal/connectors/engine/activities/plugin_fetch_next_payments.go(1 hunks)internal/connectors/engine/activities/plugin_install_connector.go(1 hunks)internal/connectors/engine/activities/plugin_poll_payout_status.go(1 hunks)internal/connectors/engine/activities/plugin_poll_transfer_status.go(1 hunks)internal/connectors/engine/activities/plugin_reverse_payout.go(1 hunks)internal/connectors/engine/activities/plugin_reverse_transfer.go(1 hunks)internal/connectors/engine/activities/plugin_translate_webhook.go(1 hunks)internal/connectors/engine/activities/plugin_uninstall_connector.go(1 hunks)internal/connectors/engine/plugins/plugin.go(2 hunks)internal/connectors/engine/plugins/plugin_generated.go(1 hunks)internal/connectors/engine/workers.go(3 hunks)internal/connectors/httpwrapper/client.go(2 hunks)internal/connectors/plugins/errors.go(1 hunks)internal/connectors/plugins/public/adyen/client/client.go(1 hunks)internal/connectors/plugins/public/adyen/plugin.go(3 hunks)internal/connectors/plugins/public/atlar/client/client.go(1 hunks)internal/connectors/plugins/public/atlar/client/client_generated.go(1 hunks)internal/connectors/plugins/public/atlar/plugin.go(3 hunks)internal/connectors/plugins/public/bankingcircle/plugin.go(3 hunks)internal/connectors/plugins/public/currencycloud/plugin.go(3 hunks)internal/connectors/plugins/public/dummypay/plugin.go(1 hunks)internal/connectors/plugins/public/generic/plugin.go(3 hunks)internal/connectors/plugins/public/mangopay/plugin.go(3 hunks)internal/connectors/plugins/public/modulr/plugin.go(3 hunks)internal/connectors/plugins/public/moneycorp/client/client.go(1 hunks)internal/connectors/plugins/public/moneycorp/plugin.go(3 hunks)internal/connectors/plugins/public/stripe/client/client.go(1 hunks)internal/connectors/plugins/public/stripe/client/client_generated.go(1 hunks)internal/connectors/plugins/public/stripe/payments.go(3 hunks)internal/connectors/plugins/public/stripe/plugin.go(3 hunks)internal/connectors/plugins/public/wise/payments.go(4 hunks)internal/connectors/plugins/public/wise/plugin.go(4 hunks)internal/connectors/plugins/registry/errors.go(1 hunks)internal/connectors/plugins/registry/plugins.go(2 hunks)internal/models/plugin_generated.go(1 hunks)internal/storage/connectors.go(1 hunks)internal/storage/connectors_test.go(2 hunks)internal/storage/storage.go(1 hunks)internal/storage/storage_generated.go(2 hunks)internal/worker/module.go(3 hunks)pkg/testserver/api.go(1 hunks)test/e2e/api_connectors_test.go(1 hunks)tools/connector-template/template/plugin.gotpl(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (27)
- internal/connectors/plugins/registry/errors.go
- internal/connectors/plugins/errors.go
- internal/connectors/plugins/public/stripe/client/client_generated.go
- internal/models/plugin_generated.go
- internal/storage/connectors_test.go
- internal/api/v3/router.go
- internal/connectors/plugins/public/stripe/client/client.go
- internal/api/backend/backend.go
- internal/connectors/plugins/public/atlar/client/client_generated.go
- internal/api/v3/handler_test.go
- internal/connectors/httpwrapper/client.go
- internal/connectors/plugins/public/moneycorp/client/client.go
- internal/connectors/plugins/public/atlar/client/client.go
- internal/connectors/plugins/public/adyen/client/client.go
- internal/storage/storage.go
- internal/storage/connectors.go
- internal/connectors/engine/activities/plugin_fetch_next_payments.go
- internal/connectors/engine/activities/plugin_fetch_next_others.go
- internal/connectors/engine/activities/plugin_fetch_next_balances.go
- internal/api/v3/handler_connectors_config_update.go
- internal/connectors/engine/activities/plugin_fetch_next_accounts.go
- internal/connectors/engine/activities/plugin_fetch_next_external_accounts.go
- internal/connectors/engine/plugins/plugin_generated.go
- internal/api/v3/handler_connectors_config_update_test.go
- test/e2e/api_connectors_test.go
- internal/storage/storage_generated.go
- internal/connectors/engine/workers.go
🔇 Additional comments (80)
internal/connectors/engine/activities/plugin_translate_webhook.go (2)
18-18: Good use of context-aware error reportingIncluding
ctxin the error handler helps enrich logs and traces, making debugging easier. Ensure all potential errors in plugins also follow this pattern for consistency.
23-23: Maintain consistent error handling across plugin callsThis is a good approach, similar to the handling at line 18. Consider verifying if additional context (e.g., connector metadata) can be embedded in error logs for better observability.
cmd/root.go (1)
23-23: Good addition for external configuration.Defining the new flag name as a constant is consistent with the existing pattern and helps keep flag names centralized. No issues found.
cmd/worker.go (3)
5-5: Import usage is correct.Nice job importing
time, which is needed for handling the rate-limiting delay logic.
59-59: Graceful fallback on missing or invalid flag values.Currently, the code retrieves the duration flag value without error checking for negative values or zero. Consider validating it and providing a fallback if the user mistakenly configures an invalid value.
67-67: Seamless injection into the worker module.Passing the rate-limiting retry delay into the worker module is well-structured. This ensures that rate-limiting concerns remain configurable and properly scoped to the worker.
internal/worker/module.go (3)
5-5: Import is correct.Bringing in
timeis necessary for dealing with rate-limiting intervals. No issues here.
36-36: Parameter addition for clearer rate-limiting behavior.Defining the delay parameter at the function signature level is a good approach, promoting explicit configuration for rate-limiting across the module.
53-60: Improved activity setup for rate-limited scenarios.Injecting the logger and
temporalRateLimitingRetryDelayintoactivities.Newwill enable fine-grained control over rate-limiting behavior and better observability. Great design for future extensibility.internal/connectors/plugins/public/mangopay/plugin.go (3)
9-9: Good addition for consistent logging capability.Importing a logger library is a positive step, as it allows for better tracing and debugging of plugin behavior. Additionally, this aligns with the approach taken across other plugins in the codebase.
19-20: Ensure error handling for registry registration.While registering the plugin, ensure any potential errors during plugin initialization are properly logged or handled upstream. Returning an error here is sufficient programmatically, but additional logs might help diagnose failures quickly.
32-32: Parametrize logger usage.The updated
New(...)signature correctly includes alogging.Loggerparameter. Make sure any callers outside this file also pass a valid logger instance to maintain consistent logging across the codebase.internal/connectors/engine/activities/plugin_install_connector.go (1)
18-23: Proper context-aware error handling.
The use ofa.temporalPluginError(ctx, err)ensures errors are wrapped with contextual information, improving debuggability in Temporal workflows. No issues found.internal/connectors/engine/activities/plugin_create_payout.go (1)
18-23: Well-handled context propagation.
Ensuring the context is passed totemporalPluginErrorprovides consistent error tracing and logging across Temporal activities. Implementation looks good.internal/connectors/engine/activities/plugin_create_webhooks.go (1)
18-23: Context-aware error handling is consistent.
Passing the context througha.temporalPluginError(ctx, err)aligns with best practices for granular logging. No concerns.internal/connectors/engine/activities/plugin_reverse_transfer.go (2)
18-18: Context-aware error handling is a welcome improvement.Passing the context to the error function helps ensure that tracing and cancellation signals are handled properly in Temporal.
23-23: Consistency with context-based error handling.Ensuring the same pattern for all plugin calls aligns well with a standardized approach to capturing contextual information during errors.
internal/connectors/engine/activities/plugin_poll_payout_status.go (2)
18-18: Context usage in error reporting looks good.Including
ctxin the error function ensures appropriate logging and error tracking within Temporal.
23-23: Consistent error function call.Maintaining consistency in error handling across plugin calls makes the code more predictable and easier to maintain.
internal/connectors/engine/activities/plugin_create_bank_account.go (2)
18-18: Enhanced error handling is beneficial.This addition of
ctxis important, ensuring improved visibility into the context of raised errors.
23-23: Context-based error wrapping.It’s advisable to maintain this approach consistently for all plugin calls to facilitate debugging and observability.
internal/connectors/engine/activities/plugin_poll_transfer_status.go (2)
18-18: Properly adding context to error processing.This change makes debugging and tracing easier, especially when multiple activities operate concurrently.
23-23: Consistent context usage in error handling.Leveraging this standardized pattern allows for consistency in diagnosing errors throughout the codebase.
internal/connectors/engine/activities/plugin_uninstall_connector.go (1)
28-28: Pass context for improved error instrumentation
Switching fromtemporalPluginError(err)toa.temporalPluginError(ctx, err)is a solid improvement, ensuring that contextual information is available for enhanced error handling. Just verify that this additional context doesn't inadvertently cause any race conditions or cancellations to be ignored.internal/connectors/plugins/registry/plugins.go (1)
38-38: Confirm consistent argument ordering
When callinginfo.createFunc(...), ensure that the parameter ordering (connectorName,logger,rawConfig) remains consistent throughout the codebase. This helps avoid confusion if plugin creation is performed in multiple places.internal/connectors/engine/plugins/plugin.go (1)
59-59: Great addition for reusability
AddingupdateExisting boolis a straightforward way to distinguish between first-time registration and updates. This is a neat approach that promotes configurability without duplicating registration logic.internal/connectors/plugins/public/generic/plugin.go (4)
7-7: No concerns with adding the logging import
Importingloggingis aligned with the new design. All good here.
17-18: Validate minimal logging usage
The inline plugin registration wrapper now includes the logger. Ensure that the plugin’s internal flow uses this logger to record relevant events (e.g., request/response details) when callingNew(...).
23-24: Keep field ordering consistent
Thenameandloggerfields are grouped logically, which improves readability. Just be sure you consistently follow this layout across other plugins for uniformity.
39-39: Commendable use of structured logging
Storing and passingloggerin the plugin helps with diagnostic logging, especially in large-scale production environments. Good improvement!internal/connectors/plugins/public/wise/payments.go (2)
53-53: Ensure the logger parameter remains consistent across the codebase.
Passingp.loggertofillPaymentsis a good approach. Double-check references in other plugin methods to ensure all logging paths remain consistent in usage.
93-93: Good injection of theloggerparameter.
This change clarifies dependency injection and avoids using a globally scoped logger.internal/connectors/plugins/public/atlar/plugin.go (5)
8-8: Clean use of go-libs logging.
Importinggithub.aaakk.us.kg/formancehq/go-libs/v2/loggingaligns with the project’s logging standard.
18-19: Explicit logger parameter for plugin registration.
Great job exposing theloggerparameter here, ensuring centralized logging capability from the point of plugin registration.
24-25: Logger field in thePluginstruct.
Storing the logger in the struct is beneficial for subsequent methods to maintain consistent logging context.
30-30: UpdatedNewconstructor to includelogger.
This reinforces strong dependency injection. Keep it consistent in other plugin constructors as well.
43-43: Proper logger assignment.
Assigningloggerto the plugin is straightforward and maintains clarity in traceability.internal/connectors/plugins/public/moneycorp/plugin.go (5)
7-7: Added structured logging library import.
Ensures logs can be standardized and correlated across the system.
17-18: Plugin registration with logger dependency.
This pattern allows each plugin to receive a dedicatedlogging.Logger. Good approach for consistent logging.
23-24: IntroducingloggerinsidePluginstruct.
This design centralizes the logger, making it available for any method within the plugin.
29-29: Constructor injection oflogger.
IncludingloggerinNewfosters better testability and dependency management.
39-39: Logger assignment in the returned plugin.
Straightforward approach: good for maintaining consistent logging.internal/connectors/plugins/public/currencycloud/plugin.go (5)
7-7: Use oflogging.Loggerfrom go-libs.
Consistent with the logging approach across the connectors.
17-18: Registry registration with logger parameter.
This ensures the plugin receives context-specific logging at initialization.
23-24: Logger field maintains clarity.
Having theloggerfield in thePluginstruct is beneficial for uniform log usage in plugin methods.
29-29: Injected logger in the constructor.
Clearly indicates that thePlugindepends on logging, improving code readability and testability.
39-39: Assigningloggerproperly.
Straightforward property assignment ensures consistent logs for all the plugin’s operations.internal/connectors/plugins/public/modulr/plugin.go (4)
7-7: No issues with the new import.
The logging import aligns with the pattern used throughout other plugins.
17-18: Good addition of theloggerparameter toRegisterPlugin.
Passing the logger into the plugin constructor allows better observability and debugging.
23-24: Struct extension looks good.
The addition oflogger logging.Loggeris consistent with the new constructor signature.
29-29: Constructor signature updated correctly.
Receiving a logger here is a good pattern to ensure each plugin can manage its own logs.internal/connectors/plugins/public/bankingcircle/plugin.go (3)
7-7: Import statement is correct.
The logging package is required for the new logger field.
17-18: Logger parameter addition is consistent with design.
Passing a logger into the constructor ensures consistent logging across the plugin.
23-24: Struct updated withlogger logging.Logger.
Well-aligned with the new constructor signature.internal/connectors/plugins/public/adyen/plugin.go (3)
8-8: New logging import is appropriate.
Matches the established pattern in adjacent plugins.
18-19: EnhancedRegisterPluginsignature.
Includinglogger logging.Loggerpaves the way for improved diagnostics.
24-25: Addedlogger logging.Loggerto the plugin struct.
This properly introduces logging capabilities at the plugin level.tools/connector-template/template/plugin.gotpl (3)
7-7: Correct addition of logging import.
Keeps it consistent with the other plugin templates.
17-18: Logger integration inregistry.RegisterPlugin.
Ensures every new plugin instance has logging capabilities from the outset.
29-29: Constructor updated to receivelogger.
Allows thorough logging of initialization flows or config-parsing steps.internal/connectors/plugins/public/dummypay/plugin.go (4)
8-8: Logging import looks good
It's great to see a unified logging approach. No issues here.
23-23: Logger field in struct
Addingloggeras a field will help maintain structured logs throughout the plugin. Looks solid.
35-35: Structured logging assignment
Assigningloggerin the plugin initialization is correct.
27-27: Updated constructor signature
Includinglogger logging.Loggerallows consistent logging usage. Be sure to update all plugin usage references accordingly.internal/connectors/plugins/public/wise/plugin.go (4)
8-8: Consistent logging strategy
Importing the same logging package across plugins helps maintain consistent log formatting.
35-36: Adding logger field
Looks correct and aligns with the new constructor approach for logging.
43-43: Updated constructor
Including the logger inNew()ensures all plugin methods can leverage structured logging.
53-53: Storing logger
Assigning logger in the plugin struct is consistent with your code pattern.internal/connectors/engine/activities/activity.go (3)
5-7: Added time & logging imports
These new imports support your rate-limiting logic and structured logging.
18-24: New fields in Activities struct
Nice addition ofloggerandrateLimitingRetryDelay. This helps with diagnosing and controlling rate-limit strategies.
337-342: Assigning new fields
Properly assigning the struct fields ensures your new rate-limit and logging features are functional.internal/connectors/plugins/public/stripe/payments.go (2)
84-87: Structured logging for nil source
Good use ofp.logger.WithFieldsto provide context in logs. This helps debug unexpected nil sources.
100-103: Consistent nil check for charge
Again, excellent structured logging. This ensures clarity when diagnosing missing charge data.internal/connectors/engine/activities/errors.go (3)
18-22: LGTM! Clear error type definition and regex pattern.The new
ErrTypeRateLimitedconstant and regex pattern for identifying scheduled workflows are well-defined. The regex pattern effectively matches ISO8601 timestamps that are appended to scheduled workflow IDs.
24-30: LGTM! Clean separation of concerns in error handling.The refactoring to make error handling context-aware and separate polling vs non-polling scenarios improves code organization and maintainability.
43-66: Consider making the rate-limiting retry delay configurable via service flags.The implementation of rate-limiting retry strategy is solid, with a good separation between scheduled polling activities (which skip retries) and other activities (which retry with delay). However, the
rateLimitingRetryDelayshould be configurable to allow fine-tuning based on different PSP requirements.This suggestion aligns with the previous discussion between paul-nicolas and laouji about making the duration configurable via service flags.
internal/connectors/engine/activities/plugin_reverse_payout.go (1)
18-18: LGTM! Consistent error handling with context.The error handling has been properly updated to use the new context-aware
temporalPluginErrormethod, maintaining consistency with the enhanced error handling strategy.Also applies to: 23-23
internal/connectors/engine/activities/plugin_create_transfer.go (1)
18-18: LGTM! Consistent error handling with context.The error handling has been properly updated to use the new context-aware
temporalPluginErrormethod, maintaining consistency with the enhanced error handling strategy.Also applies to: 23-23
internal/connectors/plugins/public/stripe/plugin.go (1)
23-24: LGTM: Logger field additionThe logger field is properly added to the Plugin struct, following the standard pattern for logger integration.
internal/api/backend/backend_generated.go (1)
30-30: LGTM: Generated mock codeThe generated mock code follows the expected patterns and correctly implements the new ConnectorsConfigUpdate method.
Also applies to: 197-209
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (18)
internal/connectors/engine/activities/plugin_reverse_payout_test.go (1)
38-39: Sensible approach for a test logger and delay.
The new logger and delay setup for testing is clear. However, consider documenting how the delay interacts with time-sensitive logic in tests to avoid flakiness due to environment constraints.internal/connectors/engine/activities/plugin_fetch_next_balances_test.go (1)
38-39: Delay introduction in testsDefining a fixed delay (50 ms) in tests can help simulate time-based logic. However, consider whether this delay can cause slow tests if scaled up or used often. If time-based ordering is crucial, you might explore mocking time or using a context-based approach to reduce test runtime.
internal/connectors/engine/activities/plugin_fetch_next_payments_test.go (1)
27-28: Consider potential test flakiness with a short delay
Introducing a 50ms delay is valid, but very short intervals can sometimes cause intermittent test failures due to timing issues or system load. You might consider using a slightly longer delay to improve test stability.internal/connectors/engine/activities/plugin_uninstall_connector_test.go (1)
38-39: Consider extracting the delay value into a named constant
Although 50 ms is an acceptable choice for a test-related delay, moving it to a named constant (e.g.,const testDelay = 50 * time.Millisecond) can improve clarity and reduce the chance of typos.internal/connectors/engine/activities/plugin_install_connector_test.go (1)
27-28: Parameterizing the logger and delayDefining the logger and
delayin test scope increases clarity and reusability. You'll be able to easily adjust the delay if needed without scattering changes around the code. Consider referencing these values through a shared test setup file if multiple test suites require similar setups.internal/connectors/engine/activities/plugin_create_webhooks_test.go (1)
38-39: Consider extracting the delay into a shared test constant.
If this exact delay value (50 ms) is reused across test files, centralizing it in a common utility or constant can help maintain consistency and facilitate updates.- delay = 50 * time.Millisecond + const testDelay = 50 * time.Millisecond + delay = testDelayinternal/connectors/engine/activities/plugin_create_payout_test.go (1)
40-41: Extract delay configuration.
As with the other tests, consider pulling the delay value into a reusable constant or config to keep test parameters consistent across the codebase.internal/connectors/engine/activities/plugin_fetch_next_accounts_test.go (1)
38-39: Reconsider the hardcoded delay.
Consolidating this test delay across multiple files can reduce repetition and ensure uniform behavior if updates are needed later.internal/connectors/engine/activities/plugin_create_transfer_test.go (1)
40-41: Use a shared constant for the delay if repeated frequently.
Adopting a single source of truth for the delay ensures test consistency and simplifies adjustments.internal/connectors/engine/activities/plugin_create_bank_account_test.go (1)
46-47: Validate delay duration for potential test flakiness.A delay of 50ms might be suitable here. However, be mindful of environmental differences and potential flakiness if tests occasionally trigger longer latencies. If stability issues arise, consider using a mock or adjustable delay in these tests.
internal/connectors/engine/activities/plugin_poll_payout_status_test.go (1)
27-28: Keep an eye on test performance under delay.Defining a 50ms delay is generally harmless, but be mindful of how multiple tests each introducing a delay might add up in larger test suites.
internal/connectors/engine/activities/plugin_fetch_next_others_test.go (1)
27-28: Consider makingdelayconfigurable for testing and production.
While the hard-coded 50ms delay is fine for now, you might consider retrieving its value from configuration, allowing more flexibility in various environments.internal/connectors/engine/activities/plugin_fetch_next_external_accounts_test.go (2)
5-5: Ensure time-based utilities fit testing needs.
Adding"time"is necessary for implementing delays or time-bound logic. If further time- or date-dependent logic expands, consider using a time mocking framework to avoid flakey tests.
27-28: Check for test flakiness with delay.
Defining a logger and a 50 ms delay is fine for demonstration. However, be aware that any artificially introduced delay can lengthen test execution, and if this logic is extended or made more complex, the test suite might become slower or prone to timing issues.internal/connectors/engine/activities/plugin_poll_transfer_status_test.go (3)
5-5: Consider defining a constant or configurable option for time usage.
Importing thetimepackage is fine here, but consider representing this duration as a top-level constant or making it configurable to avoid magic numbers in tests.
40-40: Improve readability of logger instantiation.
Initializing the logger inline is helpful, but consider adding a descriptive name or comment about the parameters being used (e.g., why structured is on, color is off, etc.) to improve clarity for future maintainers.
41-41: Revisit the chosen duration for clarity and maintainability.
While 50ms might be sufficient for test scenarios, consider a descriptive variable name or constant (e.g.,defaultRateLimitDelay) to clarify why this value is chosen and allow simpler adjustments.internal/connectors/engine/activities/plugin_reverse_transfer_test.go (1)
38-39: Consider making the default delay and logging parameters configurable.
While 50ms is fine in most test cases, consider whether externalizing these parameters might improve test flexibility and reduce potential flakiness in more complex environments or integration tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
internal/connectors/engine/activities/plugin_create_bank_account_test.go(2 hunks)internal/connectors/engine/activities/plugin_create_payout_test.go(2 hunks)internal/connectors/engine/activities/plugin_create_transfer_test.go(2 hunks)internal/connectors/engine/activities/plugin_create_webhooks_test.go(2 hunks)internal/connectors/engine/activities/plugin_fetch_next_accounts_test.go(2 hunks)internal/connectors/engine/activities/plugin_fetch_next_balances_test.go(2 hunks)internal/connectors/engine/activities/plugin_fetch_next_external_accounts_test.go(3 hunks)internal/connectors/engine/activities/plugin_fetch_next_others_test.go(3 hunks)internal/connectors/engine/activities/plugin_fetch_next_payments_test.go(3 hunks)internal/connectors/engine/activities/plugin_install_connector_test.go(3 hunks)internal/connectors/engine/activities/plugin_poll_payout_status_test.go(3 hunks)internal/connectors/engine/activities/plugin_poll_transfer_status_test.go(2 hunks)internal/connectors/engine/activities/plugin_reverse_payout_test.go(2 hunks)internal/connectors/engine/activities/plugin_reverse_transfer_test.go(2 hunks)internal/connectors/engine/activities/plugin_uninstall_connector_test.go(2 hunks)internal/connectors/engine/plugins/plugin_generated.go(2 hunks)internal/connectors/plugins/public/adyen/plugin_test.go(3 hunks)internal/connectors/plugins/public/atlar/plugin_test.go(3 hunks)internal/connectors/plugins/public/bankingcircle/plugin_test.go(3 hunks)internal/connectors/plugins/public/currencycloud/plugin_test.go(3 hunks)internal/connectors/plugins/public/generic/plugin_test.go(3 hunks)internal/connectors/plugins/public/mangopay/plugin_test.go(3 hunks)internal/connectors/plugins/public/modulr/plugin_test.go(3 hunks)internal/connectors/plugins/public/moneycorp/plugin_test.go(3 hunks)internal/connectors/plugins/public/stripe/payments_test.go(2 hunks)internal/connectors/plugins/public/stripe/plugin_test.go(3 hunks)internal/connectors/plugins/public/wise/plugin_test.go(5 hunks)
🔇 Additional comments (103)
internal/connectors/engine/activities/plugin_reverse_payout_test.go (3)
5-5: Import usage is aligned with changes.
No concerns with adding thetimepackage, as it's necessary to handle the new delay logic.
7-7: Adoption of the logging package is good.
Includinggithub.aaakk.us.kg/formancehq/go-libs/v2/loggingin tests helps provide standard logging output, which can simplify debugging. Ensure the logger usage remains lightweight for test environments.
46-46: Updated constructor usage is consistent with new signature.
This call toactivities.Newaligns with the introduced parameters for logging and rate-limiting delay. Validate any additional test coverage to confirm that the new delay is tested or verified in scenarios involving rate-limiting.internal/connectors/engine/activities/plugin_fetch_next_balances_test.go (3)
5-5: Import looks goodThis import of the
timepackage is straightforward and appropriate, required for the delay functionality.
7-7: Ensure consistent logging configurationUsing
logging.NewDefaultLogger(GinkgoWriter, true, false, false)is a valid approach to integrate with Ginkgo for test logs. Make sure other tests also follow a similar or uniform configuration if you want consistent test output.
47-47: Check consistency across the codebase for Activities constructor changesThe new signature adds
loggeranddelay. Review all references toactivities.Newto ensure that they follow this updated signature consistently. Also verify thatdelayis handled within theActivitiesstruct as intended, especially for scenarios beyond test usage.✅ Verification successful
Constructor signature changes are consistently applied across the codebase
The verification shows that all usages of
activities.New()constructor follow the same signature pattern withloggeranddelayparameters. There are two distinct usage patterns found:
- Production usage in
internal/worker/module.go:activities.New(logger, temporalClient, storage, events, plugins, temporalRateLimitingRetryDelay)
- Test usage across multiple test files:
activities.New(logger, nil, s, evts, p, delay)Both patterns consistently include all required parameters in the correct order, with test files using
nilfor the temporal client as expected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: # Verify that the updated `activities.New(logger, nil, s, evts, p, delay)` constructor is # used consistently across the codebase. rg -A 5 'activities\.New\('Length of output: 9753
internal/connectors/engine/activities/plugin_fetch_next_payments_test.go (2)
5-7: Importing 'time' and logging library is a good addition
These additions set up the foundation for introducing configurable delays and improved debugging capabilities. Great job ensuring the necessary libraries are in place.
47-47: Initialization of 'Activities' with logger and delay
Including the logger and delay in the constructor promotes better observability and enables tailored retry or wait mechanisms. This aligns with best practices for rate-limiting strategies.internal/connectors/engine/activities/plugin_uninstall_connector_test.go (3)
5-5: Importing “time” is appropriate for introducing test delays
It looks good to import “time” here to support your new delay-based test logic.
7-7: Nice addition of the logging library
Bringing ingithub.aaakk.us.kg/formancehq/go-libs/v2/loggingenables standardized logging across tests, which helps with transparent debugging.
47-47: Verify safe handling of the nil argument in theActivitiesconstructor
Passingnilwhere a dependency might be expected could lead to unexpected runtime behavior if usage withinactivities.Newis not carefully guarded. Consider adding a non-nil object or verifying that the constructor is robust to nil inputs.internal/connectors/engine/plugins/plugin_generated.go (2)
75-86: Verify mock regeneration after interface changes.The RegisterPlugin method signature has been updated to include a new boolean parameter. Please ensure that:
- The source interface in
plugin.gohas been updated first- All mocks have been regenerated using the command:
mockgen -source plugin.go -destination plugin_generated.go -package plugins . Plugins✅ Verification successful
Mock implementation matches the interface definition
The verification shows that the mock implementation correctly matches the interface definition found in
internal/models/plugin.go:RegisterPlugin(models.ConnectorID, string, models.Config, json.RawMessage, bool) errorThe generated mock in
plugin_generated.gohas the same signature with the new boolean parameter:RegisterPlugin(arg0 models.ConnectorID, arg1 string, arg2 models.Config, arg3 json.RawMessage, arg4 bool) errorThis confirms that the mocks have been properly regenerated after the interface changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the mock file is up-to-date with its source # Find the source file fd -t f "plugin.go" --exclude "*_generated.go" --exclude "*_test.go" # Compare the RegisterPlugin method signature in both files echo "Source file method signature:" rg "RegisterPlugin.*?\)" --no-filename $(fd -t f "plugin.go" --exclude "*_generated.go" --exclude "*_test.go") echo "Generated mock method signature:" rg "RegisterPlugin.*?\)" --no-filename internal/connectors/engine/plugins/plugin_generated.goLength of output: 3882
Line range hint
1-1: Verify removal of GetCapabilities method.The GetCapabilities method has been removed from the interface. Please ensure that:
- This removal is intentional and aligns with the PR objectives
- All code that previously called this method has been updated
internal/connectors/engine/activities/plugin_install_connector_test.go (3)
5-5: Nice introduction of thetimepackageIt’s good practice to rely on
time.Durationfor specific delays in tests. Helps make the code more explicit and easier to read.
7-7: Great choice in logging libraryUsing the existing logger from
go-libs/v2/loggingis consistent with other areas of the codebase. Ensures uniformity and centralized log management.
47-47: Confirm usage of the updated constructor throughout the codebaseYou have introduced a new signature for
activities.New(logger, nil, s, evts, p, delay). Verify that no other references to the old constructor remain, and all instantiations properly pass the required parameters (particularly, theloggeranddelay).✅ Verification successful
Let me also search for the actual constructor definition to understand its evolution:
And let's also check for any test files that might use this constructor:
Let me also check the file history to see the constructor changes:
Based on the verification results, I can now provide the final response:
All constructor usages are consistent with the updated signature
The verification shows that all usages of
activities.New()across the codebase are consistent with the updated constructor signature. There are two main patterns of usage:
- Production usage in
internal/worker/module.go:activities.New(logger, temporalClient, storage, events, plugins, temporalRateLimitingRetryDelay)
- Test usages across multiple test files:
activities.New(logger, nil, s, evts, p, delay)Both patterns properly include all required parameters including
loggeranddelay, matching the constructor definition found ininternal/connectors/engine/activities/activity.go.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching the entire repository for references to the older constructor usage without `logger` and `delay` rg -A 2 $'activities.New('Length of output: 108
Script:
#!/bin/bash # Searching for activities.New constructor usage with proper escaping rg -A 2 "activities\.New\("Length of output: 5641
Script:
#!/bin/bash # Find the constructor definition ast-grep --pattern 'func New($$$) *Activities { $$$ }'Length of output: 58
Script:
#!/bin/bash # Find test files that might use the constructor fd -e test.go --exec grep -l "activities.New"Length of output: 45
Script:
#!/bin/bash # Let's try to find the constructor definition with a different pattern ast-grep --pattern 'func New(logger $_,$$$) *Activities {'Length of output: 60
Script:
#!/bin/bash # Check the specific file for the constructor rg -A 5 "func New" internal/connectors/engine/activities/activities.goLength of output: 164
Script:
#!/bin/bash # Let's find the activities.go file first fd activities.goLength of output: 18
Script:
#!/bin/bash # Let's also check for any interface or struct definition related to Activities rg -A 5 "type Activities"Length of output: 477
Script:
#!/bin/bash # Now that we found the file, let's check the constructor in activity.go rg -A 10 "func New" internal/connectors/engine/activities/activity.goLength of output: 362
internal/connectors/engine/activities/plugin_create_webhooks_test.go (3)
5-5: Good use of the time package.
This import is appropriate for implementing delay-based testing scenarios.
7-7: Logger import looks correct.
Introducing a logger in tests can greatly aid in debugging and verifying code paths.
47-47: Validate the second parameter’s nil usage.
It's fine to passnilif it’s intentional, but ensure there are no hidden side effects inactivities.NewwhentemporalClient(or similar argument) is nil.internal/connectors/engine/activities/plugin_create_payout_test.go (3)
5-5: Appropriate time import.
This aligns with the need for delay-based logic in the test suite.
7-7: Logger import integration.
Including detailed logs in the test flow can help diagnose issues with payout creation.
49-49: Confirm nil argument acceptance.
Double-check that the activity constructor handles the second parameter asnilwithout unexpected side effects.internal/connectors/engine/activities/plugin_fetch_next_accounts_test.go (3)
5-5: Time import introduced for delay logic.
This is well-aligned with the new rate limiting and retry delay features.
7-7: Logging package usage is appropriate.
Having a logger during tests boosts clarity when debugging failures or anomalies.
47-47: Nil temporal client usage.
Additional verification might be necessary to ensure that a nil client won’t cause errors if the code path unexpectedly attempts to use it.internal/connectors/engine/activities/plugin_create_transfer_test.go (3)
5-5: Time import aligns with new retry/delay strategy.
This is consistent with the approach seen in the other test files.
7-7: Logger import for improved test visibility.
This can help identify issues during plugin transfer operations.
49-49: Confirm that passing nil is intentional.
Verify whether the second parameter (e.g.,temporalClient) can safely remain nil across all tested execution paths.internal/connectors/engine/activities/plugin_create_bank_account_test.go (3)
6-6: Importingtimelooks appropriate.This import is needed to manage the delay mechanism introduced in the test.
8-8: Introduce a dedicated test logger consistently.Importing the logger package in test code helps capture relevant logs. Ensure consistent usage across all tests for clarity and debugging purposes.
55-55: Confirm non-nil arguments for Activities constructor.You’re passing
logger, nil, s, evts, p, delay. Ensure thatnilfor the second argument is intended and safely handled insideactivities.New. If it’s optional or planned to be replaced with future components, consider a clear inline comment.internal/connectors/engine/activities/plugin_poll_payout_status_test.go (3)
5-5: Importingtimeis consistent with the new delay logic.Similar to the other file, importing
timeis necessary for managing the delay.
7-7: Leverage structured logging for test clarity.Using
github.com/formancehq/go-libs/v2/loggingfor a test-specific logger can improve diagnostic output without mixing logs from other tests. Good practice.
49-49: Double-check overall constructor usage.Calls to
activities.New(logger, nil, s, evts, p, delay)should consistently handle all parameters. Confirm thatnilfor the second argument is expected and that the constructor can handle it gracefully.internal/connectors/plugins/public/moneycorp/plugin_test.go (4)
8-8: Importing the logging package is aligned with the new logging strategy.
The addition of"github.com/formancehq/go-libs/v2/logging"is consistent with the other plugin tests. No issues here.
25-25: Logger initialization with default parameters is appropriate.
This logger will help capture test execution details. If you need specific configurations (like JSON formatting or different log levels), consider adjusting constructor parameters.
36-36: Logger passed to themoneycorp.Newcall.
Passing the logger to the plugin constructor adds more detailed logs and aligns it with other plugins. Check if the plugin code handles the logger correctly.
40-40: Repeated instantiation with logger is consistent.
Using the same logger instance is good for test consistency. No further concerns.internal/connectors/plugins/public/stripe/plugin_test.go (4)
7-7: Logging package import is consistent with the overall plugin logging approach.
No issues noted. Continue using the library’s recommended patterns as needed.
21-22: Logger declaration for Stripe plugin tests.
Defining a package-level logger instance is fine. Ensure it’s reused consistently throughout the test suite.
32-32: Ensuring logger is passed toNewfor better traceability.
This should improve visibility into missing config parameters. Implementation looks correct.
38-38: Passing logger again for a valid configuration.
Remains consistent across different test scenarios. No concerns here.internal/connectors/plugins/public/generic/plugin_test.go (5)
7-7: Importing the logging package to enhance the testing workflow.
Aligns with the logging approach used in other plugins. No issues.
21-22: Declaring a logger at test scope.
Consistent with other tests. Using it across multiple tests ensures uniform logging.
32-32: Logger is passed toNewwhen config is missing the API key.
This setup helps produce clearer logs regarding invalid configurations.
38-38: Logger is passed toNewwhen config is missing the endpoint.
Same pattern as the previous test. Ensures consistent logging for each error path.
44-44: Logger is passed toNewfor a valid configuration.
Ensures consistent logging coverage for successful installation scenarios as well.internal/connectors/plugins/public/adyen/plugin_test.go (5)
7-7: Importing logging aligns with the broader logging improvement.
Highly consistent with other plugin test updates.
23-25: Adding a logger with the mock client integration.
This ensures test logs capture any interactions, especially when using mock expectations.
36-36: Passing logger toNewfor Adyen config validation (missingapiKey).
Allows better troubleshooting in case of invalid JSON configurations.
40-40: Passing logger for missingcompanyID.
Similarly enhances clarity in log output when config is incomplete.
44-44: Using the logger for a fully valid Adyen config.
Maintains consistency across failure and success scenarios.internal/connectors/plugins/public/atlar/plugin_test.go (6)
7-7: Importing logging for test instrumentation
Adding the logging package import is aligned with the new approach of capturing logs within tests.
21-22: Defining a logger instance for improved test observability
Instantiating a dedicated logger improves traceability during test runs, especially when diagnosing failures.
32-32: Passing logger toNewfor richer logging
Including the logger as a parameter ensures all plugin operations are logged, aiding in debugging.
38-38: Ensuring logger is included for config validation
The new signature systematically provides logs for errors like invalid configuration.
44-44: Maintaining consistency in logger usage for all test cases
Applying the updated function signature with a logger is crucial for uniform test coverage.
50-50: Valid scenario test enhanced with logging
Even in success paths, logging can help capture key transitions within the plugin’s lifecycle.internal/connectors/plugins/public/modulr/plugin_test.go (6)
7-7: Importing logging for test instrumentation
Including the logging package extends observability and aligns with the updated plugin constructor.
21-22: Adding logger instance to facilitate detailed test logs
This approach standardizes logging across plugin tests for easier troubleshooting.
32-32: Passing logger into the plugin constructor
Ensures all test-related actions have comprehensive log coverage.
38-38: Logger now included in config validation errors
Helps record precisely when and why config validation fails.
44-44: Completing the logger integration for all error checks
Maintains consistency in capturing diagnostic details for missing fields.
50-50: Including logger for success path
A consistent logging pattern aids continuous improvements in test diagnostics.internal/connectors/plugins/public/currencycloud/plugin_test.go (6)
7-7: Importing logger for consistent test logging
Keeps the approach uniform across different plugin test suites.
21-22: Introducing a logger variable for better test observability
Creates a standard logging flow for installation and configuration tests.
32-32: Providing the logger in theNewfunction call
Allows capturing logs around missing client credentials and other config fields.
38-38: Refining error logging for improved feedback
Enabling detailed logs for config errors helps with faster issue triaging.
44-44: Consistent logger usage for mandatory field checks
Ensures all plugin constructors produce uniform log entries.
50-50: Sustaining logging even when installation succeeds
Maintaining a full log trail of both successes and failures is beneficial.internal/connectors/plugins/public/mangopay/plugin_test.go (6)
7-7: Logging import for consolidated test instrumentation
Brings this file in line with the updated logging pattern across plugins.
21-22: Defining custom logger for plugin tests
Helps capture consistent logs when diagnosing installation or config issues.
32-32: Incorporating logger in plugin creation
Enables thorough logging of configuration checks (e.g., missing client ID).
38-38: Enhancing error logging for API key validation
Logger availability ensures error details are captured in the test output.
44-44: Expanding logging to cover endpoint validation
All mandatory config fields now produce standardized logs when missing.
50-50: Logger integrated into success path
Collecting logs on successful installs can help confirm correct configurations.internal/connectors/plugins/public/bankingcircle/plugin_test.go (9)
7-7: Thank you for adding the logging dependency.
Including theloggingpackage here is a step forward for better diagnostic output in tests.
21-22: Logger instance initialization is a good move.
Creating a logger withGinkgoWritercaptures test-related logging nicely.
32-32: New logger parameter forNewfunction call noticed.
Ensuring the plugin gets a logger is beneficial for debugging when config validation fails.
38-38: Repeated logger passing is consistent.
Continuing to pass the logger toNewacross tests ensures consistent observability.
44-44: Sustained usage of logger is aligned with better debugging.
Likewise, verifying the missing endpoint scenario with proper logging is a welcome approach.
50-50: Good practice to rely on logger in test validations.
Helps surface error details when diagnosing config issues, such as a missing authorization endpoint.
56-56: Ensuring logging for missing certificate.
Enhances traceability if a user’s certificate is not configured.
62-62: Continued alignment with the new logging pattern.
All missing config checks, including the certificate key, will now be well-logged.
68-68: Error logs for invalid certificate loading.
Clearly highlightstls: failed to find any PEM data..., making debugging simpler.internal/connectors/plugins/public/wise/plugin_test.go (6)
18-18: Streaming logs within tests.
Importinglogginghere sets the stage for consistent logs horizontally across all plugins.
38-38: Instantiating the default logger.
Centralized approach to capture runtime details for the Wise plugin’s tests.
77-77: Logger passed as a parameter toNew.
Ensures that config validation issues, like missing fields, are clearly visible in logs.
82-82: Helpful for capturing erroneous PEM key usage.
Better logs should expedite debugging if malformed keys appear in the future.
92-92: Logger integration upon valid install scenario.
Verifying the plugin’s normal flow with logs fosters confidence in the robust setup.
116-116: Logger integrated with further plugin calls.
Passing the logger intoNewagain ensures subsequent steps, like translating webhooks, are logged.internal/connectors/plugins/public/stripe/payments_test.go (3)
7-7: Importing the shared logging library for Stripe tests.
This maintains consistency with the other plugins in the system.
18-19: Logger initialization pattern is consistent.
Aligns with the approach in other connectors, ensuring uniform debugging capabilities.
23-23: Plugin logging reference set.
By injecting the logger intoplg, any plugin-level activity now logs out more transparently.internal/connectors/engine/activities/plugin_fetch_next_others_test.go (3)
5-5: Good use of thetimepackage for delay handling.
This import will enable finer control over retry and rate-limiting logic.
7-7: Consistent logging strategy usinggo-libs/v2/logging.
Having a structured logger is beneficial for debugging and observability, and the chosen library integrates well with the existing codebase.
47-47: Validate thenilparameter passed toNew(...).
Ensure that passingnilfor the second parameter is intentional and won't lead to unexpected behavior, especially if future functionality requires a non-nil argument.internal/connectors/engine/activities/plugin_fetch_next_external_accounts_test.go (2)
7-7: Validate logging approach consistency.
Usinggithub.aaakk.us.kg/formancehq/go-libs/v2/loggingis consistent with other parts of the codebase. Just ensure that all test logs rely on the same logger for uniformity and potentially simplified log parsing.
47-47: Reviewnilparameter usage in constructor.
When callingactivities.New(logger, nil, s, evts, p, delay), ensure that the second parameter intentionally remainsniland that theActivitiesobject properly handles it. If its usage is expected, add a comment or clarify in the constructor documentation to avoid confusion.internal/connectors/engine/activities/plugin_poll_transfer_status_test.go (2)
7-7: Good approach to incorporate structured logging.
Including thego-libs/v2/loggingpackage ensures structured logs and consistent formatting across tests and production. This is a solid improvement.
49-49: Ensure passing nil for Temporal client is intentional.
If the activities need a real or mocked Temporal client, consider injecting it rather than usingnil. Relying onnilmight cause confusion or hidden panics.Run the following script to find all references to the
activities.Newconstructor and confirm whethernilusage is valid:✅ Verification successful
Let me also check the implementation of the New constructor to understand its parameters:
Let me also search for any test files that might use this constructor:
Passing
nilfor Temporal client in tests is valid and intentionalBased on the codebase analysis:
- The production code in
internal/worker/module.gocorrectly initializes the activities with a real Temporal client- All test files consistently pass
nilfor the Temporal client parameter, indicating this is an intentional testing pattern- The
Newconstructor accepts the Temporal client as an optional dependency, suggesting it's not required for all activity operations🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify how the Activities constructor is used throughout the codebase. rg -A 3 $'activities.New('Length of output: 108
Script:
#!/bin/bash # Search for activities.New constructor usage with proper regex escaping rg -A 3 "activities\.New\("Length of output: 7266
Script:
#!/bin/bash # Find the activities.New constructor implementation ast-grep --pattern 'func New($_) { $$$ }'Length of output: 45
Script:
#!/bin/bash # Find test files that might contain activities.New fd -e test.go --exec grep -l "activities.New"Length of output: 45
Script:
#!/bin/bash # Let's find the activities.New constructor implementation with a more specific pattern ast-grep --pattern 'func New(logger $_,$_,storage $_,events $_,plugins $_,delay $_) $_'Length of output: 89
Script:
#!/bin/bash # Let's also check the file directly where New should be defined fd -e go -E '*test.go' --exec grep -l "func New"Length of output: 4377
Script:
#!/bin/bash # Let's check the activities.New implementation in the activity.go file rg -A 10 "func New" internal/connectors/engine/activities/activity.goLength of output: 362
internal/connectors/engine/activities/plugin_reverse_transfer_test.go (2)
5-5: Imports look good.
Bringing intimeandloggingseems appropriate for handling delays and logging functionality in tests.Also applies to: 7-7
46-46: Constructor arguments have increased.
Ensure that other tests or code paths constructingactivities.Activitiesare updated accordingly. Passingnilfor the second parameter is acceptable for now, but watch out for unintended side effects if this parameter is later expected to be non-nil.
| ErrTypeUnimplemented = "UNIMPLEMENTED" | ||
| ) | ||
|
|
||
| func temporalPluginError(err error) error { | ||
| var scheduleSuffix = regexp.MustCompile(`\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$`) |
There was a problem hiding this comment.
Instead of doing all the match regexp, why not passing a boolean in the struct request of the workflow ?
It will prevent in the future some errors if the schedule suffix is changed no ?
fixes: ENG-1554
fixes: ENG-1521
Suggested rate-limiting strategy: