feat(EN-208): set a different polling interval per plugin where appropriate #572
Conversation
WalkthroughAdds a shared PollingPeriod type with defaults/minimum and parsing; updates many connector Configs to accept/validate pollingPeriod strings and include the field; changes combineConfigs to let plugin values take precedence; updates generated client model tags and schema generation; adds unit and integration test updates and docs example edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Raw
participant Shared as sharedconfig.NewPollingPeriod
participant Builder
participant Validator
Client->>Parser: submit JSON payload (pollingPeriod: string)
Parser->>Raw: unmarshal into raw struct (pollingPeriod string)
Raw->>Shared: NewPollingPeriod(raw, DefaultPollingPeriod, MinimumPollingPeriod)
Shared-->>Parser: PollingPeriod or error
Parser->>Builder: assemble final Config (fields + PollingPeriod)
Builder->>Validator: validate Config
Validator-->>Client: return validated Config or models.ErrInvalidConfig
sequenceDiagram
participant Caller
participant combineConfigs
participant baseMap
participant pluginMap
Caller->>combineConfigs: pass baseConfig, pluginConfig
combineConfigs->>pluginMap: start with plugin values
combineConfigs->>baseMap: iterate base keys
alt key missing in pluginMap
combineConfigs->>pluginMap: copy base value (fill missing)
else key present in pluginMap
note right of combineConfigs: keep plugin value (plugin precedence)
end
combineConfigs-->>Caller: return merged config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
| "name": "string", | ||
| "pageSize": 25, | ||
| "pollingPeriod": "2m", | ||
| "pollingPeriod": "string", |
There was a problem hiding this comment.
this change is annoying, but fixing it seems quite challenging. LMK if I should look more into it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (27)
internal/connectors/plugins/public/increase/config.go (2)
9-9: Prefer stdlib error wrapping (or fix pkg/errors usage) for consistent error chains.If possible, switch to
fmt.Errorf("%w: %v", models.ErrInvalidConfig, err)and the standarderrors.Is/As. If you must keep pkg/errors, ensure we wrap the original error, not the sentinel.
46-47: Wrap validation errors as invalid config for consistency.Currently validator errors are returned raw. Consider tagging them with ErrInvalidConfig like other paths.
- validate := validator.New(validator.WithRequiredStructEnabled()) - return config, validate.Struct(config) + validate := validator.New(validator.WithRequiredStructEnabled()) + if err := validate.Struct(config); err != nil { + return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) + } + return config, nilinternal/connectors/plugins/public/increase/config_test.go (1)
14-16: Assert on constructor errors for clarity.Small nit: guard
NewPollingPeriodwithrequire.NoErrorto fail fast if inputs change.- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) - longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err) + longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err)internal/connectors/plugins/sharedconfig/polling_period_test.go (1)
16-20: Add a case for def < min when raw is empty.Covers an important branch in NewPollingPeriod.
cases := []struct { @@ - {"", 30 * time.Minute, 20 * time.Minute, 30 * time.Minute, false}, + {"", 30 * time.Minute, 20 * time.Minute, 30 * time.Minute, false}, + {"", 10 * time.Minute, 20 * time.Minute, 20 * time.Minute, false},Optionally wrap each with t.Run for easier debugging.
docs/api/README.md (1)
7307-7308: Docs consistency: standardize pollingPeriod across connector examples.Stripe's example now shows
"pollingPeriod": "string"(line 7307), but 22 other connectors still use"2m". Update either Stripe back to"2m"for consistency, or update all remaining examples to match the new"string"format.Lines to update if standardizing to
"string": 753, 929, 975, 4194, 4334, 4363, 6721, 6847, 6882, 6915, 6950, 6981, 7012, 7044, 7076, 7109, 7141, 7173, 7205, 7240, 7275, 7337, 7367.internal/connectors/config.go (1)
34-41: LGTM: Merge logic correctly implements plugin precedence.The updated logic correctly preserves pluginConfig values and only adds missing keys from baseConfig, which aligns with the PR's objective to support plugin-specific polling periods.
Minor note: The
continuestatement on line 40 is redundant since it's the last statement in the loop body. You can safely remove it for slightly cleaner code:for key, value := range baseMap { _, exists := pluginMap[key] if !exists { pluginMap[key] = value - continue } }internal/connectors/plugins/public/moneycorp/config_test.go (2)
14-16: Don’t ignore constructor errors; assert them.Validate the results of sharedconfig.NewPollingPeriod to catch future changes to defaults/minimums.
- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) - longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err) + longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err)
41-56: Add a test for clamping values below the minimum.Ensure “5m” clamps to MinimumPollingPeriod (20m).
{ name: "Invalid polling period", payload: []byte(`{"clientID":"client_123","apiKey":"sk_test","endpoint":"https://api.moneycorp.com","pollingPeriod":"not-a-duration"}`), expected: Config{}, expectError: true, }, + { + name: "Below minimum polling period clamps to minimum", + payload: []byte(`{"clientID":"client_123","apiKey":"sk_test","endpoint":"https://api.moneycorp.com","pollingPeriod":"5m"}`), + expected: Config{ + ClientID: "client_123", + APIKey: "sk_test", + Endpoint: "https://api.moneycorp.com", + PollingPeriod: sharedconfig.PollingPeriod(sharedconfig.MinimumPollingPeriod), + }, + expectError: false, + },internal/connectors/plugins/public/moneycorp/config.go (2)
15-16: Validate endpoint as a URL.Align with Column’s config by adding the url validator.
- Endpoint string `json:"endpoint" validate:"required"` + Endpoint string `json:"endpoint" validate:"required,url"`
26-27: Add error context while preserving the sentinel.Provide actionable context for callers (“json unmarshal” vs “pollingPeriod”).
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) + return Config{}, errors.Wrapf(models.ErrInvalidConfig, "json unmarshal: %v", err) @@ - if err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) + if err != nil { + return Config{}, errors.Wrapf(models.ErrInvalidConfig, "pollingPeriod: %v", err)Also applies to: 35-36
internal/connectors/plugins/public/modulr/config_test.go (2)
14-16: Assert constructor errors.- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) - longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err) + longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err)
41-56: Add clamp-to-minimum test.{ name: "Invalid polling period", payload: []byte(`{"apiKey":"ak_test","apiSecret":"as_test","endpoint":"https://api.modulr.com","pollingPeriod":"not-a-duration"}`), expected: Config{}, expectError: true, }, + { + name: "Below minimum polling period clamps to minimum", + payload: []byte(`{"apiKey":"ak_test","apiSecret":"as_test","endpoint":"https://api.modulr.com","pollingPeriod":"5m"}`), + expected: Config{ + APIKey: "ak_test", + APISecret: "as_test", + Endpoint: "https://api.modulr.com", + PollingPeriod: sharedconfig.PollingPeriod(sharedconfig.MinimumPollingPeriod), + }, + expectError: false, + },internal/connectors/plugins/public/qonto/config_test.go (1)
127-148: Add a “below minimum” clamp scenario.Covers the min-bound behavior.
Context("with invalid polling period", func() { @@ }) + + Context("with polling period below minimum", func() { + BeforeEach(func() { + payload = json.RawMessage(`{"clientID":"validClient","apiKey":"validApiKey","endpoint":"https://example.com","pollingPeriod":"5m"}`) + }) + It("should clamp to the minimum", func() { + Expect(err).To(BeNil()) + Expect(config.PollingPeriod).To(Equal(sharedconfig.PollingPeriod(sharedconfig.MinimumPollingPeriod))) + }) + })internal/connectors/plugins/public/generic/config_test.go (2)
14-16: Assert constructor errors.- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) - longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err) + longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err)
49-54: Add clamp-to-minimum test.{ name: "Invalid polling period", payload: []byte(`{"apiKey":"sk_test","endpoint":"https://api.generic.example","pollingPeriod":"not-a-duration"}`), expected: Config{}, expectError: true, }, + { + name: "Below minimum polling period clamps to minimum", + payload: []byte(`{"apiKey":"sk_test","endpoint":"https://api.generic.example","pollingPeriod":"5m"}`), + expected: Config{ + APIKey: "sk_test", + Endpoint: "https://api.generic.example", + PollingPeriod: sharedconfig.PollingPeriod(sharedconfig.MinimumPollingPeriod), + }, + expectError: false, + },internal/connectors/plugins/public/mangopay/config_test.go (2)
14-16: Assert constructor errors.- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) - longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err) + longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) + require.NoError(t, err)
49-56: Add clamp-to-minimum test.{ name: "Invalid polling period", payload: []byte(`{"clientID":"client_123","apiKey":"sk_test","endpoint":"https://api.mangopay.com","pollingPeriod":"not-a-duration"}`), expected: Config{}, expectError: true, }, + { + name: "Below minimum polling period clamps to minimum", + payload: []byte(`{"clientID":"client_123","apiKey":"sk_test","endpoint":"https://api.mangopay.com","pollingPeriod":"5m"}`), + expected: Config{ + ClientID: "client_123", + APIKey: "sk_test", + Endpoint: "https://api.mangopay.com", + PollingPeriod: sharedconfig.PollingPeriod(sharedconfig.MinimumPollingPeriod), + }, + expectError: false, + },internal/connectors/plugins/public/column/config.go (2)
24-26: Add contextual messages to wrapped errors.Differentiate unmarshal vs pollingPeriod errors while keeping ErrInvalidConfig.
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) + return Config{}, errors.Wrapf(models.ErrInvalidConfig, "json unmarshal: %v", err) @@ - if err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) + if err != nil { + return Config{}, errors.Wrapf(models.ErrInvalidConfig, "pollingPeriod: %v", err)Also applies to: 33-35
12-16: Remove the "url" validator from Endpoint to align with other plugins.Column's Endpoint field is the only plugin using
validate:"required,url"; all other plugins (bankingcircle, currencycloud, generic, increase, mangopay, modulr, moneycorp, powens, qonto, tink) use onlyvalidate:"required". Removeurlfrom the validator tag on line 14 to maintain consistency across plugins. Per established patterns in the codebase, core-level validations should not be duplicated at the plugin level.Note: The reference to lines 37-44 is inaccurate—those lines contain only value assignments in the unmarshal function, not validation definitions.
internal/connectors/plugins/public/qonto/config.go (1)
3-10: Prefer stdlib wrapping; current Wrap loses original causeUsing errors.Wrap(models.ErrInvalidConfig, err.Error()) makes the sentinel the cause and drops the original error as a cause (only its text remains). Use fmt.Errorf to both classify and preserve context.
Suggested change:
- Replace pkg/errors Wrap calls with fmt.Errorf("%w: %v", models.ErrInvalidConfig, err)
- Drop the github.com/pkg/errors import and add fmt
Apply this diff:
import ( "encoding/json" - "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" + "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" "github.com/formancehq/payments/internal/models" "github.com/go-playground/validator/v10" - "github.com/pkg/errors" + "fmt" ) @@ - if err := json.Unmarshal(payload, &raw); err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err := json.Unmarshal(payload, &raw); err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + } @@ - if err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + }Also applies to: 28-30, 37-39
internal/connectors/plugins/public/generic/config.go (1)
6-10: Use fmt.Errorf for sentinel classification + original error contextReplace pkg/errors.Wrap(models.ErrInvalidConfig, err.Error()) with fmt.Errorf("%w: %v", models.ErrInvalidConfig, err). Remove the pkg/errors import and add fmt.
import ( "encoding/json" - "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" + "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" "github.com/formancehq/payments/internal/models" "github.com/go-playground/validator/v10" - "github.com/pkg/errors" + "fmt" ) @@ - if err := json.Unmarshal(payload, &raw); err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err := json.Unmarshal(payload, &raw); err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + } @@ - if err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + }Also applies to: 24-26, 33-35
internal/connectors/plugins/sharedconfig/polling_period.go (1)
8-19: Add GoDoc for exported APIAdd short comments for MinimumPollingPeriod, DefaultPollingPeriod, type PollingPeriod, and NewPollingPeriod to satisfy linters and improve discoverability.
Example:
-const ( +// MinimumPollingPeriod is the lowest allowed polling period across plugins. +const ( MinimumPollingPeriod = 20 * time.Minute - DefaultPollingPeriod = 30 * time.Minute + // DefaultPollingPeriod is used when no value is provided. + DefaultPollingPeriod = 30 * time.Minute ) -type PollingPeriod time.Duration +// PollingPeriod wraps time.Duration to control JSON marshaling and defaults. +type PollingPeriod time.Duration @@ -// Helper to construct the value while applying min/default. +// NewPollingPeriod parses raw, applies defaults and enforces bounds.internal/connectors/config_test.go (1)
13-43: Add tests for min clamp and invalid negativesStrengthen coverage around pollingPeriod handling during merge.
@@ func TestCombineConfigs_PluginPrecedence(t *testing.T) { @@ } + +func TestCombineConfigs_PollingPeriod_ClampedToMin(t *testing.T) { + t.Parallel() + base := models.Config{PollingPeriod: 60 * time.Minute} + pluginCfg := map[string]any{"pollingPeriod": "5m"} + b, err := combineConfigs(base, pluginCfg) + require.NoError(t, err) + var out map[string]any + require.NoError(t, json.Unmarshal(b, &out)) + assert.Equal(t, "20m0s", out["pollingPeriod"]) +} + +func TestCombineConfigs_PollingPeriod_NegativeRejected(t *testing.T) { + t.Parallel() + base := models.Config{} + pluginCfg := map[string]any{"pollingPeriod": "-5m"} + _, err := combineConfigs(base, pluginCfg) + require.Error(t, err) +}internal/connectors/plugins/public/atlar/config.go (2)
6-10: Adopt fmt.Errorf for wrapping invalid configsMirror the error-wrapping change here to keep classification and context.
import ( "encoding/json" - "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" + "github.com/formancehq/payments/internal/connectors/plugins/sharedconfig" "github.com/formancehq/payments/internal/models" "github.com/go-playground/validator/v10" - "github.com/pkg/errors" + "fmt" ) @@ - if err := json.Unmarshal(payload, &raw); err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err := json.Unmarshal(payload, &raw); err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + } @@ - if err != nil { - return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) - } + if err != nil { + return Config{}, fmt.Errorf("%w: %v", models.ErrInvalidConfig, err) + }Also applies to: 26-28, 35-37
19-25: Minor: consider json.RawMessage for payload type consistencyOther plugins use json.RawMessage; aligning signatures helps reuse and testing. Safe to defer.
internal/connectors/plugins/public/wise/config_test.go (2)
25-25: Minor formatting: Remove extra space.There's an extra space before
t.Parallel().Apply this diff:
- t.Parallel() + t.Parallel()
82-84: Consider checking error types more robustly.The string-based test name matching for error validation is brittle. If test names change, the validation is skipped. Consider using
errors.Is(err, models.ErrInvalidConfig)where appropriate, or adding an expected error field to the test struct.Example refactor:
type testCase struct { name string payload []byte expected Config expectError bool checkErrType bool // whether to verify ErrInvalidConfig wrapping }Then in the test execution:
if tt.expectError { require.Error(t, err) if tt.checkErrType { assert.ErrorIs(t, err, models.ErrInvalidConfig) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v3/v3-connectors-config.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (29)
docs/api/README.md(1 hunks)internal/connectors/config.go(1 hunks)internal/connectors/config_test.go(1 hunks)internal/connectors/plugins/public/atlar/config.go(1 hunks)internal/connectors/plugins/public/atlar/config_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config_test.go(1 hunks)internal/connectors/plugins/public/column/config.go(1 hunks)internal/connectors/plugins/public/column/config_test.go(1 hunks)internal/connectors/plugins/public/generic/config.go(1 hunks)internal/connectors/plugins/public/generic/config_test.go(1 hunks)internal/connectors/plugins/public/increase/config.go(1 hunks)internal/connectors/plugins/public/increase/config_test.go(1 hunks)internal/connectors/plugins/public/mangopay/config.go(1 hunks)internal/connectors/plugins/public/mangopay/config_test.go(1 hunks)internal/connectors/plugins/public/modulr/config.go(1 hunks)internal/connectors/plugins/public/modulr/config_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/config.go(1 hunks)internal/connectors/plugins/public/moneycorp/config_test.go(1 hunks)internal/connectors/plugins/public/qonto/config.go(1 hunks)internal/connectors/plugins/public/qonto/config_test.go(5 hunks)internal/connectors/plugins/public/stripe/config.go(1 hunks)internal/connectors/plugins/public/stripe/config_test.go(1 hunks)internal/connectors/plugins/public/wise/config.go(2 hunks)internal/connectors/plugins/public/wise/config_test.go(1 hunks)internal/connectors/plugins/sharedconfig/polling_period.go(1 hunks)internal/connectors/plugins/sharedconfig/polling_period_test.go(1 hunks)pkg/client/models/components/v3stripeconfig.go(1 hunks)tools/compile-configs/main.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (26)
tools/compile-configs/main.go (1)
tools/compile-configs/definitions.go (1)
V3Config(30-34)
internal/connectors/plugins/public/modulr/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/modulr/config.go (1)
Config(12-17)
pkg/client/models/components/v3stripeconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/qonto/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/stripe/config_test.go (3)
internal/connectors/plugins/public/stripe/config.go (1)
Config(12-15)internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/increase/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/increase/config.go (1)
Config(12-17)
internal/connectors/plugins/public/moneycorp/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/increase/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/stripe/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/wise/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/moneycorp/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/moneycorp/config.go (1)
Config(12-17)
internal/connectors/plugins/sharedconfig/polling_period_test.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
NewPollingPeriod(22-37)
internal/connectors/plugins/public/generic/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/bankingcircle/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/bankingcircle/config.go (1)
Config(12-20)
internal/connectors/plugins/public/column/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/column/config.go (1)
Config(12-16)
internal/connectors/plugins/public/modulr/config.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)
internal/connectors/config_test.go (2)
internal/connectors/plugins/public/stripe/config.go (1)
Config(12-15)internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/wise/config_test.go (3)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/wise/config.go (1)
Config(17-23)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/mangopay/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/mangopay/config.go (1)
Config(12-17)
internal/connectors/plugins/public/bankingcircle/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/generic/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/generic/config.go (1)
Config(12-16)
internal/connectors/plugins/public/atlar/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/atlar/config.go (1)
Config(12-17)
internal/connectors/plugins/public/column/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/mangopay/config.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)
internal/connectors/plugins/public/atlar/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/qonto/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
🔇 Additional comments (22)
tools/compile-configs/main.go (3)
162-164: LGTM: Critical nil-check prevents panic.This nil-check is essential because line 165 accesses
field.Tag.Value, which would panic iffield.Tagis nil. Skipping untagged fields is correct since the JSON tag is required to determine the property name (line 175).
176-196: LGTM: Robust type resolution for AST nodes.The enhanced type resolution correctly handles identifiers, selector expressions (e.g.,
time.Duration), and pointer types. The nested switch for*ast.StarExprappropriately unwraps pointer types to extract the underlying type name.Note: Nested pointers (e.g.,
**Type) are not supported and will return an error, which is acceptable for this use case.
199-213: LGTM: Appropriate handling of duration types.The additions of
DurationandPollingPeriodcases correctly map these types to"string"in the JSON schema, which aligns with their serialization format (e.g., "2m", "30m"). The updated error message usingtypNameprovides better context for unsupported types.pkg/client/models/components/v3stripeconfig.go (1)
13-13: Making pollingPeriod optional removes the SDK-side “2m” default — verify spec/SDK parity.Looks good, but this changes behavior for clients that omitted the field. Please confirm the OpenAPI/spec and other SDKs reflect “optional, server‑default to 30m (min 20m)” and that tests cover omitted pollingPeriod end‑to‑end.
internal/connectors/plugins/public/modulr/config.go (1)
19-46: LGTM: Well-structured polling period integration.The refactored unmarshal flow correctly:
- Parses the raw JSON into an intermediate struct
- Constructs a validated PollingPeriod with appropriate defaults (30m) and minimums (20m)
- Assembles the final Config before validation
This pattern is consistent across all plugins in the PR and properly handles edge cases like missing or invalid polling periods.
internal/connectors/plugins/public/column/config_test.go (1)
11-68: LGTM: Comprehensive test coverage for Column config.The test suite effectively validates:
- Default polling period behavior (30 minutes)
- Custom polling period parsing ("45m")
- Required field validation
- Invalid duration string handling
The parallel execution and use of table-driven tests follows Go best practices.
internal/connectors/plugins/public/bankingcircle/config_test.go (1)
11-76: LGTM: Thorough test coverage for BankingCircle config.The test suite mirrors the pattern used across other plugins and comprehensively validates polling period handling, including defaults, custom values, validation errors, and invalid inputs.
internal/connectors/plugins/public/bankingcircle/config.go (1)
22-56: LGTM: Consistent polling period implementation.The BankingCircle config follows the same proven pattern as other plugins in this PR, correctly handling polling period parsing, validation, and defaulting.
internal/connectors/plugins/public/stripe/config_test.go (1)
13-99: LGTM: Excellent test coverage with valuable edge cases.The Stripe test suite goes beyond the baseline coverage seen in other plugins by additionally testing:
- Minimum polling period coercion (5m → 20m) on lines 51-62
- Marshalling behavior to ensure duration format preservation on lines 89-99
- Specific error type validation (validator.ValidationErrors)
This comprehensive approach provides strong confidence in the implementation's correctness.
internal/connectors/plugins/public/stripe/config.go (1)
17-43: LGTM: Clean polling period integration for Stripe.The implementation follows the established pattern and correctly handles polling period parsing and validation. The use of
json.RawMessageas the parameter type is appropriate for this context.internal/connectors/plugins/public/atlar/config_test.go (1)
11-70: LGTM: Solid test coverage for Atlar config.The test suite provides comprehensive validation of polling period handling, consistent with the patterns established across other plugins in this PR.
internal/connectors/plugins/public/moneycorp/config.go (1)
12-16: The review comment is incorrect. The PollingPeriod type already implements MarshalJSON with proper string encoding (json.Marshal(time.Duration(p).String())), preventing the number-encoding concern. Additionally, the codebase safely handles deserialization through an intermediate struct pattern—Config is never directly unmarshaled; instead, incoming JSON is unmarshaled into a temporary struct with string fields, then converted via NewPollingPeriod(). No changes are needed.Likely an incorrect or invalid review comment.
internal/connectors/plugins/public/qonto/config.go (1)
41-50: LGTM on the parsing/validation flowTwo-step unmarshal with explicit PollingPeriod construction and validator usage is clear and robust. Default/min logic is applied before validation as intended.
internal/connectors/plugins/public/generic/config.go (1)
37-44: Config build + validation looks goodClean construction of Config with normalized PollingPeriod and then validate.
internal/connectors/plugins/public/wise/config.go (3)
10-10: LGTM! Clean addition of polling period support.The import and struct field addition are properly integrated. The PollingPeriod field is correctly typed and tagged.
Also applies to: 18-20
56-63: LGTM! Proper two-stage unmarshalling pattern.Using an intermediate struct with PollingPeriod as string allows proper parsing and validation through
sharedconfig.NewPollingPeriod. Error wrapping is consistent.
65-78: LGTM! Correct polling period parsing and Config construction.The implementation properly:
- Delegates parsing to
sharedconfig.NewPollingPeriodwith appropriate defaults and minimums- Handles errors by wrapping with
models.ErrInvalidConfig- Constructs the Config with all fields before validation
internal/connectors/plugins/public/wise/config_test.go (2)
1-22: LGTM! Well-structured test setup.The helper function
makePayloadand thevalidRSAPublicKeyPEMconstant provide clean test infrastructure. The imports are appropriate for the test requirements.
36-74: LGTM! Comprehensive test case coverage.The test cases thoroughly cover:
- Default polling period behavior
- Custom polling period values
- Required field validation
- Invalid duration string handling
- Invalid public key handling
internal/connectors/plugins/public/mangopay/config.go (3)
6-6: LGTM! Consistent implementation across connectors.The import and struct changes follow the same pattern as the Wise connector, ensuring consistency across the codebase.
Also applies to: 12-17
20-37: LGTM! Proper implementation of polling period parsing.The unmarshalAndValidateConfig function correctly:
- Uses intermediate struct for parsing
- Delegates to
sharedconfig.NewPollingPeriodwith consistent defaults and minimums- Wraps errors appropriately
39-46: LGTM! Config construction, validation, and test coverage are all correct.The Config is properly constructed with all fields including the parsed PollingPeriod, validation is performed appropriately, and comprehensive test coverage exists in
config_test.gowith test cases for default polling period, custom polling period, invalid polling period, and missing required fields.
| if err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } |
There was a problem hiding this comment.
Same wrapping issue here — preserve the parsing error.
Mirror the fix so the invalid polling period retains its cause/stack.
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| if err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/increase/config.go around lines 35-37, the
code currently discards the original parsing error by wrapping
models.ErrInvalidConfig around err.Error(); change it to wrap the original
parsing error with the sentinel message so the cause/stack is preserved — e.g.,
return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) (or
errors.WithMessage(err, models.ErrInvalidConfig.Error())) so the returned error
retains the original error as its cause while still indicating an invalid
config.
| // Helper to construct the value while applying min/default. | ||
| func NewPollingPeriod(raw string, def, min time.Duration) (PollingPeriod, error) { | ||
| if raw == "" { | ||
| if def < min { | ||
| return PollingPeriod(min), nil | ||
| } | ||
| return PollingPeriod(def), nil | ||
| } | ||
| v, err := time.ParseDuration(raw) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| if v < min { | ||
| v = min | ||
| } | ||
| return PollingPeriod(v), nil | ||
| } |
There was a problem hiding this comment.
Reject negative durations instead of silently clamping
Currently, "-5m" becomes the minimum (20m), which hides misconfiguration. Return an error for negatives; keep clamping only for 0 < v < min.
Apply this diff:
package sharedconfig
import (
"encoding/json"
+ "fmt"
"time"
)
@@
// Helper to construct the value while applying min/default.
func NewPollingPeriod(raw string, def, min time.Duration) (PollingPeriod, error) {
if raw == "" {
if def < min {
return PollingPeriod(min), nil
}
return PollingPeriod(def), nil
}
v, err := time.ParseDuration(raw)
if err != nil {
return 0, err
}
+ if v < 0 {
+ return 0, fmt.Errorf("pollingPeriod must be non-negative, got %q", raw)
+ }
if v < min {
v = min
}
return PollingPeriod(v), nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Helper to construct the value while applying min/default. | |
| func NewPollingPeriod(raw string, def, min time.Duration) (PollingPeriod, error) { | |
| if raw == "" { | |
| if def < min { | |
| return PollingPeriod(min), nil | |
| } | |
| return PollingPeriod(def), nil | |
| } | |
| v, err := time.ParseDuration(raw) | |
| if err != nil { | |
| return 0, err | |
| } | |
| if v < min { | |
| v = min | |
| } | |
| return PollingPeriod(v), nil | |
| } | |
| package sharedconfig | |
| import ( | |
| "encoding/json" | |
| "fmt" | |
| "time" | |
| ) | |
| // Helper to construct the value while applying min/default. | |
| func NewPollingPeriod(raw string, def, min time.Duration) (PollingPeriod, error) { | |
| if raw == "" { | |
| if def < min { | |
| return PollingPeriod(min), nil | |
| } | |
| return PollingPeriod(def), nil | |
| } | |
| v, err := time.ParseDuration(raw) | |
| if err != nil { | |
| return 0, err | |
| } | |
| if v < 0 { | |
| return 0, fmt.Errorf("pollingPeriod must be non-negative, got %q", raw) | |
| } | |
| if v < min { | |
| v = min | |
| } | |
| return PollingPeriod(v), nil | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/sharedconfig/polling_period.go around lines
21-37, change the logic so negative durations are rejected rather than silently
clamped: when parsing raw, if v < 0 return a descriptive error (e.g. "negative
duration not allowed") instead of clamping to min; keep the existing clamp for
values where 0 < v < min; also validate the default branch (raw == "") to return
an error if def < 0, otherwise keep the current behavior of returning min when 0
<= def < min and def when def >= min.
fguery
force-pushed
the
feat/EN-208_change_polling_interval
branch
from
6ee844c to
8f34c00
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/models/config_test.go (1)
225-233: Fix inconsistent default polling period assertion.The test asserts
2*time.Minutefor the default polling period, but the actual default was changed to30*time.Minuteininternal/models/config.goline 11. This test will fail.Apply this diff:
- assert.Equal(t, 2*time.Minute, config.PollingPeriod) + assert.Equal(t, 30*time.Minute, config.PollingPeriod)
♻️ Duplicate comments (2)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
21-37: Address negative duration handling as previously flagged.As noted in a previous review, the current implementation silently clamps negative durations (e.g., "-5m") to the minimum (20m), which masks configuration errors. This issue remains unaddressed.
Per the earlier review, negative durations should be explicitly rejected with a descriptive error rather than silently corrected. This prevents users from being unaware that their configuration contains invalid values.
Apply the previously suggested diff to reject negative durations:
func NewPollingPeriod(raw string, def, min time.Duration) (PollingPeriod, error) { + if def < 0 { + return 0, fmt.Errorf("default pollingPeriod must be non-negative, got %v", def) + } if raw == "" { if def < min { return PollingPeriod(min), nil } return PollingPeriod(def), nil } v, err := time.ParseDuration(raw) if err != nil { return 0, err } + if v < 0 { + return 0, fmt.Errorf("pollingPeriod must be non-negative, got %q", raw) + } if v < min { v = min } return PollingPeriod(v), nil }Don't forget to add the
"fmt"import.internal/connectors/plugins/public/increase/config.go (1)
26-37: Same wrapping bug as already flagged.Agree with the existing feedback: wrap the concrete
err, notmodels.ErrInvalidConfig, so the real cause stays in the chain while reusing the sentinel text.
🧹 Nitpick comments (1)
internal/connectors/plugins/public/wise/config_test.go (1)
24-93: Good test coverage; consider adding minimum clamping test.The test suite covers the main scenarios well: valid config, missing fields, custom polling period, invalid period, and invalid key. The use of parallel execution and field-by-field comparison (avoiding issues with the private
webhookPublicKeyfield) is appropriate.Consider adding a test case for a polling period below the minimum (e.g.,
"10m") to verify thatsharedconfig.NewPollingPeriodcorrectly clamps it toMinimumPollingPeriod(20 minutes). This would explicitly document and validate the clamping behavior in the context of the Wise connector.Example test case to add:
{ name: "Polling period below minimum gets clamped", payload: makePayload(t, map[string]string{"apiKey": "sk_test", "webhookPublicKey": validRSAPublicKeyPEM, "pollingPeriod": "10m"}), expected: Config{ APIKey: "sk_test", WebhookPublicKey: validRSAPublicKeyPEM, PollingPeriod: defaultPollingPeriod, // Should be clamped to MinimumPollingPeriod (20m), not 10m }, expectError: false, },Note: Update the expected
PollingPeriodvalue to match the minimum (20m) rather than the default (30m).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
openapi.yamlis excluded by!**/*.yamlopenapi/v3/v3-connectors-config.yamlis excluded by!**/*.yamlpkg/client/.speakeasy/gen.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (41)
docs/api/README.md(11 hunks)internal/connectors/config.go(1 hunks)internal/connectors/config_test.go(1 hunks)internal/connectors/plugins/public/atlar/config.go(1 hunks)internal/connectors/plugins/public/atlar/config_test.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config.go(1 hunks)internal/connectors/plugins/public/bankingcircle/config_test.go(1 hunks)internal/connectors/plugins/public/column/config.go(1 hunks)internal/connectors/plugins/public/column/config_test.go(1 hunks)internal/connectors/plugins/public/generic/config.go(1 hunks)internal/connectors/plugins/public/generic/config_test.go(1 hunks)internal/connectors/plugins/public/increase/config.go(1 hunks)internal/connectors/plugins/public/increase/config_test.go(1 hunks)internal/connectors/plugins/public/mangopay/config.go(1 hunks)internal/connectors/plugins/public/mangopay/config_test.go(1 hunks)internal/connectors/plugins/public/modulr/config.go(1 hunks)internal/connectors/plugins/public/modulr/config_test.go(1 hunks)internal/connectors/plugins/public/moneycorp/config.go(1 hunks)internal/connectors/plugins/public/moneycorp/config_test.go(1 hunks)internal/connectors/plugins/public/qonto/config.go(1 hunks)internal/connectors/plugins/public/qonto/config_test.go(5 hunks)internal/connectors/plugins/public/stripe/config.go(1 hunks)internal/connectors/plugins/public/stripe/config_test.go(1 hunks)internal/connectors/plugins/public/wise/config.go(2 hunks)internal/connectors/plugins/public/wise/config_test.go(1 hunks)internal/connectors/plugins/sharedconfig/polling_period.go(1 hunks)internal/connectors/plugins/sharedconfig/polling_period_test.go(1 hunks)internal/models/config.go(2 hunks)internal/models/config_test.go(4 hunks)pkg/client/models/components/v3atlarconfig.go(1 hunks)pkg/client/models/components/v3bankingcircleconfig.go(1 hunks)pkg/client/models/components/v3columnconfig.go(1 hunks)pkg/client/models/components/v3genericconfig.go(1 hunks)pkg/client/models/components/v3increaseconfig.go(1 hunks)pkg/client/models/components/v3mangopayconfig.go(1 hunks)pkg/client/models/components/v3modulrconfig.go(1 hunks)pkg/client/models/components/v3moneycorpconfig.go(1 hunks)pkg/client/models/components/v3qontoconfig.go(1 hunks)pkg/client/models/components/v3stripeconfig.go(1 hunks)pkg/client/models/components/v3wiseconfig.go(1 hunks)tools/compile-configs/main.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- tools/compile-configs/main.go
- pkg/client/models/components/v3mangopayconfig.go
- pkg/client/models/components/v3bankingcircleconfig.go
- internal/connectors/config_test.go
- internal/connectors/plugins/sharedconfig/polling_period_test.go
- internal/connectors/plugins/public/moneycorp/config_test.go
- internal/connectors/plugins/public/bankingcircle/config_test.go
- internal/connectors/plugins/public/column/config_test.go
- internal/connectors/plugins/public/stripe/config_test.go
- internal/connectors/plugins/public/modulr/config_test.go
- internal/connectors/plugins/public/atlar/config_test.go
- internal/connectors/plugins/public/qonto/config.go
- internal/connectors/plugins/public/mangopay/config_test.go
- internal/connectors/plugins/public/stripe/config.go
- docs/api/README.md
- pkg/client/models/components/v3moneycorpconfig.go
- pkg/client/models/components/v3stripeconfig.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-08T13:46:21.578Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 509
File: internal/connectors/plugins/public/powens/client/transactions.go:77-91
Timestamp: 2025-08-08T13:46:21.578Z
Learning: In formancehq/payments Powens client (file: internal/connectors/plugins/public/powens/client/transactions.go), keep single-layout time parsing for Transaction fields (Date: time.DateOnly, DateTime: time.RFC3339, LastUpdate: time.DateTime); no need for multi-layout fallbacks per maintainer preference (paul-nicolas) in PR #509.
Applied to files:
internal/connectors/plugins/public/modulr/config.gointernal/connectors/plugins/public/bankingcircle/config.gointernal/connectors/plugins/public/atlar/config.gointernal/connectors/plugins/public/qonto/config_test.gointernal/connectors/plugins/public/mangopay/config.gointernal/connectors/plugins/public/moneycorp/config.gointernal/connectors/plugins/public/wise/config.go
📚 Learning: 2025-01-17T09:48:09.578Z
Learnt from: laouji
Repo: formancehq/payments PR: 262
File: internal/connectors/plugins/registry/plugins.go:31-42
Timestamp: 2025-01-17T09:48:09.578Z
Learning: In Go packages that handle plugin registration, when RegisterPlugin is called from init() blocks, it should panic on configuration errors rather than return them, to prevent the program from starting with misconfigured plugins.
Applied to files:
internal/connectors/plugins/public/increase/config.go
📚 Learning: 2025-05-15T16:35:29.660Z
Learnt from: fguery
Repo: formancehq/payments PR: 427
File: internal/connectors/plugins/public/qonto/balances.go:20-55
Timestamp: 2025-05-15T16:35:29.660Z
Learning: The Qonto connector plugin has unit tests for fetchNextBalances in internal/connectors/plugins/public/qonto/balances_test.go, covering success cases, missing FromPayload, invalid JSON handling, and other error conditions.
Applied to files:
internal/connectors/plugins/public/wise/config_test.gointernal/connectors/plugins/public/qonto/config_test.gointernal/connectors/plugins/public/generic/config_test.go
📚 Learning: 2025-08-08T13:48:23.427Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 509
File: internal/connectors/plugins/public/powens/create_user_link.go:31-36
Timestamp: 2025-08-08T13:48:23.427Z
Learning: In formancehq/payments Powens plugin validation functions (e.g., validateCreateUserLinkRequest in internal/connectors/plugins/public/powens/create_user_link.go), avoid duplicating core validations like redirect URL format; the core layer already validates these per maintainer preference (paul-nicolas) in PR #509.
Applied to files:
internal/connectors/plugins/public/wise/config_test.gointernal/connectors/plugins/public/qonto/config_test.go
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
Repo: formancehq/payments PR: 193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
Applied to files:
internal/connectors/plugins/public/qonto/config_test.go
🧬 Code graph analysis (22)
internal/connectors/plugins/public/modulr/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/generic/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
pkg/client/models/components/v3wiseconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/increase/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
pkg/client/models/components/v3qontoconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
pkg/client/models/components/v3genericconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
pkg/client/models/components/v3increaseconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/models/config_test.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/models/config.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
pkg/client/models/components/v3modulrconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/wise/config_test.go (3)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/wise/config.go (1)
Config(17-23)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/increase/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/increase/config.go (1)
Config(12-17)
internal/connectors/plugins/public/bankingcircle/config.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)
internal/connectors/plugins/public/atlar/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/qonto/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/generic/config_test.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)PollingPeriod(13-13)internal/connectors/plugins/public/generic/config.go (1)
Config(12-16)
pkg/client/models/components/v3atlarconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/mangopay/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/column/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
pkg/client/models/components/v3columnconfig.go (1)
internal/connectors/plugins/sharedconfig/polling_period.go (1)
PollingPeriod(13-13)
internal/connectors/plugins/public/moneycorp/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
internal/connectors/plugins/public/wise/config.go (2)
internal/connectors/plugins/sharedconfig/polling_period.go (4)
PollingPeriod(13-13)NewPollingPeriod(22-37)DefaultPollingPeriod(10-10)MinimumPollingPeriod(9-9)internal/models/errors.go (1)
ErrInvalidConfig(9-9)
🔇 Additional comments (22)
pkg/client/models/components/v3modulrconfig.go (1)
15-15: Change is consistent with other v3 connector configs and appropriately removes hardcoded defaults.The removal of
default:"2m"and addition ofomitemptyin v3modulrconfig.go aligns with the pattern across all other v3 connector config files (v3moneycorpconfig, v3stripeconfig, v3wiseconfig, v3qontoconfig, v3mangopayconfig, v3increaseconfig, v3columnconfig). This consistency indicates the file was properly regenerated from the updated Speakeasy specification to centralize polling period management in sharedconfig.internal/connectors/config.go (1)
34-41: LGTM! Plugin config precedence correctly implemented.The merge logic now properly preserves plugin config values by only assigning base config values for keys that don't already exist in the plugin map. This ensures plugin-specific overrides (like
pollingPeriod) take precedence while still benefiting from base config defaults.internal/models/config.go (1)
11-11: LGTM! Default polling period appropriately increased.The default polling period has been increased from 2 minutes to 30 minutes, aligning with the PR's goal to set more reasonable defaults for expensive workflows.
internal/connectors/plugins/public/qonto/config_test.go (2)
24-30: LGTM! Test setup properly initializes polling period fixtures.The BeforeEach block correctly creates default and custom polling period fixtures using
sharedconfig.NewPollingPeriod, and properly validates that no errors occur during initialization. This provides clean test fixtures for the assertions throughout the test suite.
127-148: LGTM! Comprehensive test coverage for polling period scenarios.The new test contexts thoroughly cover:
- Custom polling period parsing (45m)
- Invalid polling period handling (non-duration string)
Both test cases properly assert expected behavior and error conditions.
internal/connectors/plugins/public/generic/config.go (1)
18-44: LGTM! Two-step unmarshal pattern properly implements polling period handling.The refactored
unmarshalAndValidateConfigcorrectly:
- Unmarshals into an intermediate raw struct with string-typed
pollingPeriod- Constructs a typed
PollingPeriodviasharedconfig.NewPollingPeriodwith appropriate defaults/minimums- Assembles the final
Configwith the computedPollingPeriod- Validates the complete struct
Error handling properly wraps both JSON unmarshal errors and polling period construction errors with
models.ErrInvalidConfig.internal/connectors/plugins/public/generic/config_test.go (1)
11-68: LGTM! Comprehensive table-driven tests for config unmarshalling.The test suite properly covers:
- Valid configuration with default polling period
- Missing required fields (validation error expected)
- Custom polling period (45m)
- Invalid polling period (non-duration string)
Test structure follows best practices:
- Parallel execution
- Proper use of
requirefor errors andassertfor equality- Clear test case naming
internal/connectors/plugins/public/atlar/config.go (1)
19-47: LGTM! Consistent polling period implementation.The refactored
unmarshalAndValidateConfigfollows the same proven two-step pattern as other plugins (generic, qonto, etc.):
- Unmarshal to raw struct
- Construct typed
PollingPeriodviasharedconfig.NewPollingPeriod- Assemble and validate final
ConfigError handling and validation flow are correct.
internal/models/config_test.go (1)
19-19: LGTM! Test values updated to reflect new polling period defaults.The test assertions have been correctly updated to use the new polling period values (30 minutes default, 20 minutes minimum) throughout the validation and unmarshalling tests.
Also applies to: 79-79, 96-96, 167-167
internal/connectors/plugins/sharedconfig/polling_period.go (2)
8-11: LGTM! Well-defined constants for polling period bounds.The constants clearly define the minimum (20 minutes) and default (30 minutes) polling periods, aligning with the PR's objective to set reasonable defaults for expensive workflows.
13-19: LGTM! Clean type definition and JSON marshalling.The
PollingPeriodtype wrapper provides:
- A clear semantic type distinct from raw
time.Duration- Convenient
Duration()accessor- Proper JSON marshalling to duration strings
internal/connectors/plugins/public/bankingcircle/config.go (1)
22-56: LGTM!The two-step unmarshal pattern for handling
PollingPeriodis correctly implemented. The intermediate raw struct parses the string value,sharedconfig.NewPollingPeriodapplies defaults and minimums, and the finalConfigstruct receives the typed value before validation.pkg/client/models/components/v3genericconfig.go (1)
14-14: LGTM!The JSON tag change removes the default value and adds
omitempty, aligning with the centralized polling period handling viasharedconfig. This is appropriate for a generated file that reflects API schema changes.pkg/client/models/components/v3wiseconfig.go (1)
13-13: LGTM!Consistent with other generated config models, the
PollingPeriodfield tag now usesomitemptyand relies on centralized default handling.pkg/client/models/components/v3qontoconfig.go (1)
15-15: LGTM!The tag change is consistent with the project-wide shift to omitempty-driven JSON output for optional polling period fields.
pkg/client/models/components/v3increaseconfig.go (1)
14-14: LGTM!The
PollingPeriodfield tag change aligns with the broader refactoring to centralize polling period defaults and useomitemptyfor JSON marshaling.pkg/client/models/components/v3atlarconfig.go (1)
14-14: LGTM!The field tag update is consistent with the migration to shared polling period handling across all connector configs.
pkg/client/models/components/v3columnconfig.go (1)
14-14: LGTM!Final generated config model updated consistently with the
omitemptypattern for optional polling period fields.internal/connectors/plugins/public/wise/config.go (3)
10-10: LGTM: Import addition for shared polling configuration.The import of
sharedconfigis necessary for the newPollingPeriodtype and enables consistent polling period handling across connectors.
18-20: LGTM: PollingPeriod field added to Config.The new field is properly typed using
sharedconfig.PollingPeriodwith appropriate JSON tag. The formatting is consistent with existing fields.
56-78: LGTM: Clean refactor with proper validation flow.The refactored
unmarshalAndValidateConfigfollows a sound pattern:
- Unmarshals into a raw struct with
PollingPeriodas string- Constructs a typed
PollingPeriodusing shared defaults (30m) and minimums (20m) with error handling- Builds the final
Configwith the validatedPollingPeriodError handling is appropriate, wrapping errors with
models.ErrInvalidConfig.internal/connectors/plugins/public/wise/config_test.go (1)
18-22: LGTM: Clean helper function.The
makePayloadhelper simplifies test setup by marshaling test data and handling errors withrequire.NoError.
| if err := json.Unmarshal(payload, &raw); err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| pp, err := sharedconfig.NewPollingPeriod( | ||
| raw.PollingPeriod, | ||
| sharedconfig.DefaultPollingPeriod, | ||
| sharedconfig.MinimumPollingPeriod, | ||
| ) | ||
| if err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| config := Config{ | ||
| APIKey: raw.APIKey, | ||
| Endpoint: raw.Endpoint, | ||
| PollingPeriod: pp, | ||
| } |
There was a problem hiding this comment.
Keep the underlying error in the chain.
Wrapping models.ErrInvalidConfig drops the JSON/NewPollingPeriod failure details. Wrap err and reuse the sentinel string instead:
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())
…
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| config := Config{ | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } | |
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| config := Config{ | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/column/config.go around lines 24 to 41,
the code wraps the sentinel models.ErrInvalidConfig around the underlying
json.Unmarshal/NewPollingPeriod errors which loses the original error chain;
change the wrap order so the original error is the cause and the sentinel
provides the message — i.e. replace errors.Wrap(models.ErrInvalidConfig,
err.Error()) with errors.Wrap(err, models.ErrInvalidConfig.Error()) (do this for
both the json.Unmarshal and NewPollingPeriod error returns) so the underlying
error is preserved while reusing the sentinel text.
| defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) | ||
| longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) |
There was a problem hiding this comment.
Handle errors in test setup.
The errors returned by sharedconfig.NewPollingPeriod are being silently ignored. If these calls fail, the test will use zero-value PollingPeriod instances, leading to incorrect test behavior or false positives.
Apply this diff to properly handle errors:
- defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod)
- longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod)
+ defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod)
+ require.NoError(t, err)
+ longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod)
+ require.NoError(t, err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defaultPollingPeriod, _ := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) | |
| longPollingPeriod, _ := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) | |
| defaultPollingPeriod, err := sharedconfig.NewPollingPeriod("", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) | |
| require.NoError(t, err) | |
| longPollingPeriod, err := sharedconfig.NewPollingPeriod("45m", sharedconfig.DefaultPollingPeriod, sharedconfig.MinimumPollingPeriod) | |
| require.NoError(t, err) |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/increase/config_test.go around lines 14 to
15, the errors returned by sharedconfig.NewPollingPeriod are ignored; change the
test setup to capture the error return values and fail the test if an error
occurs (e.g., defaultPollingPeriod, err := sharedconfig.NewPollingPeriod(...);
require.NoError(t, err) or if err != nil { t.Fatalf("failed to create
defaultPollingPeriod: %v", err) }), and do the same for longPollingPeriod so the
test never continues with zero-value PollingPeriod on failure.
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| pp, err := sharedconfig.NewPollingPeriod( | ||
| raw.PollingPeriod, | ||
| sharedconfig.DefaultPollingPeriod, | ||
| sharedconfig.MinimumPollingPeriod, | ||
| ) | ||
| if err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| config := Config{ | ||
| ClientID: raw.ClientID, | ||
| APIKey: raw.APIKey, | ||
| Endpoint: raw.Endpoint, | ||
| PollingPeriod: pp, | ||
| } |
There was a problem hiding this comment.
Don’t lose the real error while tagging invalid config.
Currently the sentinel becomes the cause and the actual parse/polling-period error is lost. Wrap err and keep the sentinel message:
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())
…
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| config := Config{ | |
| ClientID: raw.ClientID, | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| config := Config{ | |
| ClientID: raw.ClientID, | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/mangopay/config.go around lines 27 to 44,
the code currently calls errors.Wrap(models.ErrInvalidConfig, err.Error()) which
makes the sentinel the cause and loses the original parse/polling-period error;
change both return sites to wrap the original err with the sentinel message
(e.g., errors.Wrap(err, models.ErrInvalidConfig.Error()) or errors.Wrapf(err,
"%s", models.ErrInvalidConfig)) so the original error remains the cause while
the sentinel is included in the message.
| if err := json.Unmarshal(payload, &raw); err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| pp, err := sharedconfig.NewPollingPeriod( | ||
| raw.PollingPeriod, | ||
| sharedconfig.DefaultPollingPeriod, | ||
| sharedconfig.MinimumPollingPeriod, | ||
| ) | ||
| if err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| config := Config{ | ||
| APIKey: raw.APIKey, | ||
| APISecret: raw.APISecret, | ||
| Endpoint: raw.Endpoint, | ||
| PollingPeriod: pp, | ||
| } |
There was a problem hiding this comment.
Wrap the actual error, not the sentinel.
errors.Wrap(models.ErrInvalidConfig, err.Error()) masks the root cause and stack. Swap the arguments so the concrete error remains the cause while keeping the sentinel text:
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())
…
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| config := Config{ | |
| APIKey: raw.APIKey, | |
| APISecret: raw.APISecret, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } | |
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| config := Config{ | |
| APIKey: raw.APIKey, | |
| APISecret: raw.APISecret, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/modulr/config.go around lines 26 to 44,
the current calls use errors.Wrap(models.ErrInvalidConfig, err.Error()) which
makes the sentinel the cause and hides the original error; change both wraps to
errors.Wrap(err, models.ErrInvalidConfig.Error()) (i.e., pass the concrete err
as the first arg and the sentinel message as the wrap message) so the original
error remains the cause while preserving the sentinel text.
| if err := json.Unmarshal(payload, &raw); err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| pp, err := sharedconfig.NewPollingPeriod( | ||
| raw.PollingPeriod, | ||
| sharedconfig.DefaultPollingPeriod, | ||
| sharedconfig.MinimumPollingPeriod, | ||
| ) | ||
| if err != nil { | ||
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | ||
| } | ||
|
|
||
| config := Config{ | ||
| ClientID: raw.ClientID, | ||
| APIKey: raw.APIKey, | ||
| Endpoint: raw.Endpoint, | ||
| PollingPeriod: pp, | ||
| } |
There was a problem hiding this comment.
Preserve the original parsing error when wrapping.
Passing models.ErrInvalidConfig as the first argument makes it the cause and drops the real unmarshalling/polling-period error, so callers lose the stack trace and actual failure detail. Wrap the concrete err instead and reuse the sentinel’s message:
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())
…
- return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error())
+ return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(models.ErrInvalidConfig, err.Error()) | |
| } | |
| config := Config{ | |
| ClientID: raw.ClientID, | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } | |
| if err := json.Unmarshal(payload, &raw); err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| pp, err := sharedconfig.NewPollingPeriod( | |
| raw.PollingPeriod, | |
| sharedconfig.DefaultPollingPeriod, | |
| sharedconfig.MinimumPollingPeriod, | |
| ) | |
| if err != nil { | |
| return Config{}, errors.Wrap(err, models.ErrInvalidConfig.Error()) | |
| } | |
| config := Config{ | |
| ClientID: raw.ClientID, | |
| APIKey: raw.APIKey, | |
| Endpoint: raw.Endpoint, | |
| PollingPeriod: pp, | |
| } |
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/moneycorp/config.go around lines 26 to 44,
the code wraps the sentinel models.ErrInvalidConfig as the cause which discards
the original unmarshalling/polling-period error; change the error wrapping so
the concrete err is the cause (e.g., errors.Wrap(err,
models.ErrInvalidConfig.Error()) or equivalent) when returning Config{},
preserving the original error and stack while reusing the sentinel's message for
context.
8f34c00 to
66ce32f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/e2e/api_connectors_test.go (2)
80-93: Consider verifying all config fields for completeness.The refactoring from struct equality to field-by-field comparison improves test clarity. However,
LinkFlowErrorandUpdateLinkFlowErrorare set in the test config (lines 581-582, 586) but not verified in the assertions. Consider adding assertions for these fields to ensure complete validation.Apply this diff to add assertions for the remaining fields:
Expect(getRes.V3GetConnectorConfigResponse.Data.V3DummypayConfig.PageSize).To(Equal(connectorConf.PageSize)) + Expect(getRes.V3GetConnectorConfigResponse.Data.V3DummypayConfig.LinkFlowError).To(Equal(connectorConf.LinkFlowError)) + Expect(getRes.V3GetConnectorConfigResponse.Data.V3DummypayConfig.UpdateLinkFlowError).To(Equal(connectorConf.UpdateLinkFlowError)) Expect(getRes.V3GetConnectorConfigResponse.Data.Type).To(Equal(components.V3ConnectorConfigTypeDummypay))
201-201: Consider using consistent duration format.The duration "30m0s" is functionally correct but inconsistent with other test values that use "30m" (lines 106, 178, 584, 601). For consistency, consider using "30m".
Apply this diff for consistency:
- config.PollingPeriod = pointer.For("30m0s") + config.PollingPeriod = pointer.For("30m")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/connectors/engine/engine_test.go(3 hunks)test/e2e/api_connectors_test.go(6 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-08T13:46:21.578Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 509
File: internal/connectors/plugins/public/powens/client/transactions.go:77-91
Timestamp: 2025-08-08T13:46:21.578Z
Learning: In formancehq/payments Powens client (file: internal/connectors/plugins/public/powens/client/transactions.go), keep single-layout time parsing for Transaction fields (Date: time.DateOnly, DateTime: time.RFC3339, LastUpdate: time.DateTime); no need for multi-layout fallbacks per maintainer preference (paul-nicolas) in PR #509.
Applied to files:
test/e2e/api_connectors_test.go
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
Repo: formancehq/payments PR: 193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
Applied to files:
test/e2e/api_connectors_test.go
📚 Learning: 2025-02-07T15:47:17.240Z
Learnt from: laouji
Repo: formancehq/payments PR: 317
File: internal/api/v2/handler_bank_accounts_forward_to_connector.go:17-17
Timestamp: 2025-02-07T15:47:17.240Z
Learning: In the bank accounts forward to connector handler, the connectorID validator tag is deliberately omitted to maintain error code consistency (ErrMissingOrInvalidBody for missing connectorID). The validation is instead performed manually later in the handler using models.ConnectorIDFromString.
Applied to files:
internal/connectors/engine/engine_test.go
🧬 Code graph analysis (1)
test/e2e/api_connectors_test.go (3)
pkg/client/models/components/v3getconnectorconfigresponse.go (1)
V3GetConnectorConfigResponse(5-7)pkg/client/models/components/v3dummypayconfig.go (1)
V3DummypayConfig(9-17)pkg/client/models/components/v3connectorconfig.go (1)
V3ConnectorConfigTypeDummypay(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Dirty
🔇 Additional comments (3)
internal/connectors/engine/engine_test.go (1)
137-137: LGTM! Test fixtures updated to reflect new polling period constraints.The polling period values in test fixtures have been correctly updated to align with the new defaults introduced in this PR:
- Lines 137 and 402:
"30m"matches the new default polling period- Line 467:
"20m"matches the new minimum polling period and appropriately tests edge casesThese are straightforward test data updates with no logic changes.
Also applies to: 402-402, 467-467
test/e2e/api_connectors_test.go (2)
106-106: LGTM: Polling period values align with PR objectives.The FilePollingPeriod values have been updated to "30m", which aligns with the PR objective to set longer default polling intervals for expensive plugin workflows.
Also applies to: 178-178
584-584: LGTM: Factory functions updated with new polling period defaults.The connector config factory functions now use "30m" as the default polling period, ensuring all tests that rely on these factories consistently use the new longer intervals. This centralized change aligns well with the PR objectives.
Also applies to: 601-601
fguery
force-pushed
the
feat/EN-208_change_polling_interval
branch
from
3fda18a to
88ba09a
Compare
All plugins we've changed have multiple layers of workflows, making it expensive to compute.
This sets a more reasonable default of 30mn/20mn minimal polling period.