Conversation
Co-authored-by: salaun.clement <salaun.clement@gmail.com>
|
Cursor Agent can help with this pull request. Just |
WalkthroughMigrates the public Stripe connector to the new plugin SDK. Updates imports and usages to pkg/pluginsdk for metrics, HTTP, errors, plugins, and registry. Adds new SDK modules (errors, http, metrics, plugins base/errors, registry). Adjusts Stripe client code to use pluginsdk OperationContext and HTTP error constants. Minor formatting-only edits. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Registry as pluginsdk/registry
participant StripePlugin as Stripe Public Plugin
participant Metrics as pluginsdk/metrics
participant HTTP as pluginsdk/http
participant StripeAPI as Stripe API
Note over App,Registry: Initialization
App->>Registry: RegisterPlugin(provider, type, factory, ...)
Registry-->>App: Registration complete
Note over App,StripePlugin: Operation (e.g., List Accounts)
App->>StripePlugin: FetchNextAccounts(ctx, req)
StripePlugin->>Metrics: OperationContext(ctx, "list_accounts")
Metrics-->>StripePlugin: ctx'
StripePlugin->>HTTP: Do(ctx', http.Request, expBody, errBody)
HTTP->>StripeAPI: HTTP request (OTEL-instrumented)
StripeAPI-->>HTTP: Response (status, body)
HTTP-->>StripePlugin: status, maybe error (ErrStatusCode*)
alt Not yet installed
StripePlugin-->>App: error = pluginsdk.ErrNotYetInstalled
else Success
StripePlugin-->>App: Response data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
- Coverage 66.50% 66.30% -0.21%
==========================================
Files 783 788 +5
Lines 40232 40382 +150
==========================================
+ Hits 26757 26775 +18
- Misses 12038 12170 +132
Partials 1437 1437 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: salaun.clement <salaun.clement@gmail.com>
altitude
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
pkg/pluginsdk/plugins/errors.go (1)
5-11: Exported error vars lack GoDoc; consider adding brief comments (and optional naming nit).Add short comments for exported identifiers to satisfy linters and improve discoverability. Optional: “ErrUpstreamRatelimit” → “ErrUpstreamRateLimit” for consistent camel-casing.
Apply comments-only diff:
var ( - ErrNotImplemented = errors.New("not implemented") - ErrNotYetInstalled = errors.New("not yet installed") - ErrInvalidClientRequest = errors.New("invalid client request") - ErrUpstreamRatelimit = errors.New("rate limited by upstream server") - ErrCurrencyNotSupported = errors.New("currency not supported") + // ErrNotImplemented indicates the called operation is not implemented by the plugin. + ErrNotImplemented = errors.New("not implemented") + // ErrNotYetInstalled indicates the plugin has not been installed/configured yet. + ErrNotYetInstalled = errors.New("not yet installed") + // ErrInvalidClientRequest indicates the request from the caller is invalid or unprocessable. + ErrInvalidClientRequest = errors.New("invalid client request") + // ErrUpstreamRatelimit indicates the upstream provider rate limited the request (e.g., HTTP 429). + ErrUpstreamRatelimit = errors.New("rate limited by upstream server") + // ErrCurrencyNotSupported indicates the requested currency is not supported by the plugin/provider. + ErrCurrencyNotSupported = errors.New("currency not supported") )If you want the naming nit as well, rename (will require updating call sites):
- ErrUpstreamRatelimit = errors.New("rate limited by upstream server") + ErrUpstreamRateLimit = errors.New("rate limited by upstream server")pkg/pluginsdk/metrics/metrics.go (2)
11-17: Use an unexported, struct-based context key to avoid collisions (staticcheck SA1029).Defining the key as an unexported empty struct prevents accidental collisions across packages and appeases linters.
-type MetricOpContextKey string - -const MetricOperationContextKey MetricOpContextKey = "_metric_operation_context_key" +type metricOpContextKey struct{} +var metricOperationContextKey metricOpContextKey @@ -func OperationContext(ctx context.Context, operation string) context.Context { - return context.WithValue(ctx, MetricOperationContextKey, operation) -} +func OperationContext(ctx context.Context, operation string) context.Context { + return context.WithValue(ctx, metricOperationContextKey, operation) +}
27-31: Optional: clone default transport to avoid mutating the shared singleton.Cloning the default transport is safer when wrapping or customizing.
func NewTransport(_ string, opts TransportOpts) http.RoundTripper { if opts.Transport == nil { - opts.Transport = http.DefaultTransport + if dt, ok := http.DefaultTransport.(*http.Transport); ok { + opts.Transport = dt.Clone() + } else { + opts.Transport = http.DefaultTransport + } } return &Transport{parent: opts.Transport} }pkg/pluginsdk/http/http.go (1)
37-61: Always wrap transport with otelhttp and accept nil config.
- If Transport is nil, it currently skips otelhttp; instrumentation is lost.
- Accept nil config for ergonomic defaults.
-func NewClient(config *Config) Client { - if config.Timeout == 0 { +func NewClient(config *Config) Client { + if config == nil { + config = &Config{} + } + if config.Timeout == 0 { config.Timeout = 10 * time.Second } - if config.Transport != nil { - config.Transport = otelhttp.NewTransport(config.Transport) - } else { - config.Transport = http.DefaultTransport.(*http.Transport).Clone() - } + var baseTransport http.RoundTripper + if config.Transport != nil { + baseTransport = config.Transport + } else { + baseTransport = http.DefaultTransport.(*http.Transport).Clone() + } + config.Transport = otelhttp.NewTransport(baseTransport) if config.HttpErrorCheckerFn == nil { config.HttpErrorCheckerFn = func(code int) error { if code == http.StatusTooManyRequests { return ErrStatusCodeTooManyRequests } if code >= http.StatusBadRequest && code < http.StatusInternalServerError { return ErrStatusCodeClientError } else if code >= http.StatusInternalServerError { return ErrStatusCodeServerError } return nil } } httpClient := &http.Client{Timeout: config.Timeout, Transport: config.Transport} return &client{httpClient: httpClient, httpErrorCheckerFn: config.HttpErrorCheckerFn} }internal/connectors/plugins/public/stripe/client/client.go (1)
49-50: Instrumentation detail: NewHTTPClient ignores name; consider plumb-through.pluginsdkmetrics.NewHTTPClient(name, ...) discards name and calls NewTransport("") per the SDK snippet. You may lose per‑connector labeling in metrics. Suggest passing name into the transport.
Proposed SDK tweak:
--- a/pkg/pluginsdk/metrics/metrics.go +++ b/pkg/pluginsdk/metrics/metrics.go @@ -func NewHTTPClient(_ string, timeout time.Duration) *http.Client { +func NewHTTPClient(name string, timeout time.Duration) *http.Client { return &http.Client{ Timeout: timeout, - Transport: NewTransport("", TransportOpts{}), + Transport: NewTransport(name, TransportOpts{}), } }pkg/pluginsdk/plugins/base.go (1)
3-13: Avoid leaking internalmodels in SDK API; add alias and compile-time assertTo keep the SDK surface decoupled (and prepare for external repos), avoid returning/mentioning
internalmodels.*in exported signatures. Alias the interface locally and return that fromNewBasePlugin. Also add a compile-time assert.Apply this diff:
import ( "context" internalmodels "github.com/formancehq/payments/internal/models" ) +// SDK-facing alias to avoid exposing internal path in exported signatures +type Plugin = internalmodels.Plugin + type basePlugin struct{} -func NewBasePlugin() internalmodels.Plugin { +func NewBasePlugin() Plugin { return &basePlugin{} } + +// Ensure interface conformance at compile time +var _ Plugin = (*basePlugin)(nil)pkg/pluginsdk/errors/errors.go (2)
11-22: Harden Unwrap to avoid panics; handle nils; minor Cause guardCurrent
Unwrapasserts[]errorand can panic. Also handle nil inputs inNewWrappedErrorand short-circuitCause(nil).Apply this diff:
func NewWrappedError(cause error, newError error) error { - return &wrappedError{err: fmt.Errorf("%w: %w", cause, newError)} + // Gracefully handle nils while preserving errors.Is/As behavior + if cause == nil { + return newError + } + if newError == nil { + return cause + } + return &wrappedError{err: fmt.Errorf("%w: %w", cause, newError)} } func (e *wrappedError) Error() string { return e.err.Error() } // Unwrap chain support so errors.Is works properly func (e *wrappedError) Unwrap() []error { - return e.err.(interface{ Unwrap() []error }).Unwrap() + switch u := e.err.(type) { + case interface{ Unwrap() []error }: + return u.Unwrap() + case interface{ Unwrap() error }: + if v := u.Unwrap(); v != nil { + return []error{v} + } + return nil + default: + return nil + } } // Cause unwraps the error to the root cause func Cause(err error) error { + if err == nil { + return nil + } for { switch v := err.(type) { case interface{ Unwrap() error }: next := v.Unwrap() if next == nil { return err } err = next case interface{ Unwrap() []error }: nexts := v.Unwrap() if len(nexts) == 0 { return err } err = nexts[0] default: return err } } }Also applies to: 24-44
1-3: Package name “errors” can cause import alias frictionConsider renaming to
sdkerrorsorerrutilto avoid clashing with the standarderrorspackage in imports.internal/connectors/plugins/public/stripe/plugin.go (1)
68-92: Deduplicate “not installed” guardThe repeated
p.client == nilchecks can be centralized for readability.You could add:
func (p *Plugin) ensureInstalled() error { if p.client == nil { return pluginsdk.ErrNotYetInstalled } return nil }Then early-return in each method:
if err := p.ensureInstalled(); err != nil { return <ResponseType>{}, err }Also applies to: 96-109, 111-124, 126-139
pkg/pluginsdk/registry/registry.go (1)
11-45: SDK API leaks internal types; alias them to avoid “internal/” in exported signaturesExternal connectors won’t be able to refer to
internal/...types. Provide SDK-level aliases and update signatures to use them. Keeps binary compatibility while hiding internal paths.Apply this diff:
import ( "encoding/json" "github.com/formancehq/go-libs/v3/logging" internalregistry "github.com/formancehq/payments/internal/connectors/plugins/registry" internalmodels "github.com/formancehq/payments/internal/models" ) +// SDK-facing aliases to avoid exposing internal paths in public signatures. +type ( + PluginType = internalmodels.PluginType + ConnectorID = internalmodels.ConnectorID + Plugin = internalmodels.Plugin + Capability = internalmodels.Capability + Configs = internalregistry.Configs + Config = internalregistry.Config +) + // RegisterPlugin exposes the internal registry for external connectors via the SDK path. // It keeps the same signature so existing plugins can register seamlessly. func RegisterPlugin( - provider string, - pluginType internalmodels.PluginType, - createFunc func(internalmodels.ConnectorID, string, logging.Logger, json.RawMessage) (internalmodels.Plugin, error), - capabilities []internalmodels.Capability, - conf any, + provider string, + pluginType PluginType, + createFunc func(ConnectorID, string, logging.Logger, json.RawMessage) (Plugin, error), + capabilities []Capability, + conf any, ) { internalregistry.RegisterPlugin(provider, pluginType, createFunc, capabilities, conf) } // GetPluginType forwards to the internal registry. -func GetPluginType(provider string) (internalmodels.PluginType, error) { +func GetPluginType(provider string) (PluginType, error) { return internalregistry.GetPluginType(provider) } // GetCapabilities forwards to the internal registry. -func GetCapabilities(provider string) ([]internalmodels.Capability, error) { +func GetCapabilities(provider string) ([]Capability, error) { return internalregistry.GetCapabilities(provider) } // GetConfigs forwards to the internal registry. -func GetConfigs(debug bool) internalregistry.Configs { +func GetConfigs(debug bool) Configs { return internalregistry.GetConfigs(debug) } // GetConfig forwards to the internal registry. -func GetConfig(provider string) (internalregistry.Config, error) { +func GetConfig(provider string) (Config, error) { return internalregistry.GetConfig(provider) } // DummyPSPName re-exports the internal constant for convenience. const DummyPSPName = internalregistry.DummyPSPName
📜 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 (20)
internal/connectors/plugins/public/stripe/client/accounts.go(3 hunks)internal/connectors/plugins/public/stripe/client/balances.go(1 hunks)internal/connectors/plugins/public/stripe/client/client.go(3 hunks)internal/connectors/plugins/public/stripe/client/external_accounts.go(3 hunks)internal/connectors/plugins/public/stripe/client/payments.go(3 hunks)internal/connectors/plugins/public/stripe/client/payouts.go(2 hunks)internal/connectors/plugins/public/stripe/client/reversals.go(2 hunks)internal/connectors/plugins/public/stripe/client/transfers.go(2 hunks)internal/connectors/plugins/public/stripe/create_payouts.go(1 hunks)internal/connectors/plugins/public/stripe/create_transfers.go(1 hunks)internal/connectors/plugins/public/stripe/plugin.go(5 hunks)internal/connectors/plugins/public/stripe/reverse_transfers.go(1 hunks)internal/connectors/plugins/public/stripe/utils.go(1 hunks)internal/connectors/plugins/public/stripe/workflow.go(1 hunks)pkg/pluginsdk/errors/errors.go(1 hunks)pkg/pluginsdk/http/http.go(1 hunks)pkg/pluginsdk/metrics/metrics.go(1 hunks)pkg/pluginsdk/plugins/base.go(1 hunks)pkg/pluginsdk/plugins/errors.go(1 hunks)pkg/pluginsdk/registry/registry.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
PR: formancehq/payments#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/stripe/client/balances.go
📚 Learning: 2025-08-08T13:48:23.427Z
Learnt from: paul-nicolas
PR: formancehq/payments#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/stripe/utils.go
📚 Learning: 2025-08-08T13:46:21.578Z
Learnt from: paul-nicolas
PR: formancehq/payments#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/stripe/client/payments.go
🧬 Code graph analysis (15)
internal/connectors/plugins/public/stripe/client/balances.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
pkg/pluginsdk/plugins/errors.go (1)
internal/connectors/plugins/public/stripe/plugin.go (1)
New(32-48)
pkg/pluginsdk/metrics/metrics.go (1)
pkg/pluginsdk/http/http.go (1)
Client(28-30)
internal/connectors/plugins/public/stripe/client/payouts.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
internal/connectors/plugins/public/stripe/client/client.go (3)
pkg/pluginsdk/metrics/metrics.go (1)
NewHTTPClient(38-43)pkg/pluginsdk/http/http.go (2)
ErrStatusCodeTooManyRequests(19-19)ErrStatusCodeClientError(17-17)pkg/pluginsdk/errors/errors.go (1)
NewWrappedError(11-13)
pkg/pluginsdk/http/http.go (2)
internal/connectors/plugins/public/stripe/client/client.go (2)
New(47-62)Client(25-33)pkg/pluginsdk/metrics/metrics.go (2)
Transport(23-25)NewTransport(27-32)
internal/connectors/plugins/public/stripe/client/payments.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
internal/connectors/plugins/public/stripe/workflow.go (1)
internal/models/connector_tasks_tree.go (6)
ConnectorTaskTree(21-33)TaskType(3-3)TASK_FETCH_ACCOUNTS(7-7)TASK_FETCH_BALANCES(8-8)TASK_FETCH_PAYMENTS(10-10)TASK_FETCH_EXTERNAL_ACCOUNTS(9-9)
internal/connectors/plugins/public/stripe/client/reversals.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
pkg/pluginsdk/registry/registry.go (4)
internal/models/plugin.go (1)
PluginType(13-13)internal/connectors/plugins/public/stripe/plugin.go (1)
Plugin(23-30)internal/models/capabilities.go (1)
Capability(9-9)internal/connectors/plugins/registry/configs.go (1)
Configs(13-13)
internal/connectors/plugins/public/stripe/client/external_accounts.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
internal/connectors/plugins/public/stripe/plugin.go (6)
pkg/pluginsdk/registry/registry.go (1)
RegisterPlugin(13-21)internal/connectors/plugins/registry/plugins.go (1)
RegisterPlugin(40-53)pkg/pluginsdk/plugins/base.go (1)
NewBasePlugin(11-13)internal/connectors/plugins/base_plugin.go (1)
NewBasePlugin(11-13)pkg/pluginsdk/plugins/errors.go (1)
ErrNotYetInstalled(7-7)internal/connectors/plugins/errors.go (1)
ErrNotYetInstalled(9-9)
pkg/pluginsdk/plugins/base.go (5)
internal/models/config.go (1)
PluginInternalConfig(17-17)internal/models/plugin.go (9)
InstallRequest(39-41)InstallResponse(43-45)UninstallRequest(47-50)UninstallResponse(52-52)CreateWebhooksResponse(60-63)TrimWebhookRequest(65-68)TrimWebhookResponse(70-72)VerifyWebhookResponse(79-81)TranslateWebhookResponse(89-91)pkg/pluginsdk/plugins/errors.go (1)
ErrNotImplemented(6-6)internal/models/webhooks.go (1)
PSPWebhook(30-36)internal/models/plugin_open_banking.go (6)
CreateUserLinkResponse(63-71)CompleteUserLinkResponse(109-112)UpdateUserLinkResponse(85-92)CompleteUpdateUserLinkResponse(99-102)DeleteUserConnectionResponse(127-127)DeleteUserResponse(133-133)
internal/connectors/plugins/public/stripe/client/transfers.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
internal/connectors/plugins/public/stripe/client/accounts.go (2)
pkg/pluginsdk/metrics/metrics.go (1)
OperationContext(15-17)internal/connectors/metrics/transport.go (1)
OperationContext(16-18)
⏰ 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 (16)
internal/connectors/plugins/public/stripe/workflow.go (1)
6-32: LGTM – formatting-only change.No functional changes detected; task tree remains identical.
internal/connectors/plugins/public/stripe/create_transfers.go (1)
14-14: Same import migration as create_payouts.go – verify behavior remains identical.Paraphrasing the previous comment: confirm NewWrappedError parity with the prior package.
internal/connectors/plugins/public/stripe/client/balances.go (1)
7-12: LGTM – metrics context migrated to pluginsdk.Context propagation via pluginsdkmetrics.OperationContext is correct and matches the new SDK surface.
internal/connectors/plugins/public/stripe/utils.go (1)
4-8: LGTM – errors import migrated to pluginsdk.Validation logic unchanged; import swap aligns with the new SDK.
internal/connectors/plugins/public/stripe/create_payouts.go (1)
14-14: NewWrappedError parity confirmed — no behavioral change. pkg/pluginsdk/errors.NewWrappedError implements the same fmt.Errorf("%w: %w") wrapping, Unwrap() and Cause() logic as internal/utils/errors.NewWrappedError, so error.Is and cause extraction behavior is preserved.internal/connectors/plugins/public/stripe/client/reversals.go (1)
6-6: LGTM: migrated to pluginsdk metrics context.Import and OperationContext swap to pluginsdkmetrics is correct and consistent with the SDK migration.
Also applies to: 22-22
internal/connectors/plugins/public/stripe/reverse_transfers.go (1)
11-16: LGTM: errors import switched to pluginsdk.Import move to pkg/pluginsdk/errors aligns with the SDK decoupling; no functional changes in this file.
internal/connectors/plugins/public/stripe/client/transfers.go (1)
6-7: LGTM: context source replaced with pluginsdkmetrics.OperationContext migration is correct; behavior preserved.
Also applies to: 22-24
internal/connectors/plugins/public/stripe/client/external_accounts.go (1)
6-6: LGTM: metrics context migration.Both scan and list paths now use pluginsdkmetrics.OperationContext as expected.
Also applies to: 26-26, 46-46
internal/connectors/plugins/public/stripe/client/payouts.go (1)
6-6: LGTM: OperationContext migrated to pluginsdkmetrics.No behavior change; aligns with the new SDK.
Also applies to: 23-24
internal/connectors/plugins/public/stripe/client/payments.go (1)
6-6: LGTM: metrics context migration on both scan and list paths.Operation labels preserved; no logic changes.
Also applies to: 35-35, 52-52
internal/connectors/plugins/public/stripe/client/accounts.go (1)
6-6: LGTM: migrated to pluginsdkmetrics for OperationContext.Scan and list paths updated consistently.
Also applies to: 20-20, 35-35
pkg/pluginsdk/plugins/base.go (1)
83-87: TrimWebhook passthrough baseline is sensibleReturning the inbound webhook as-is provides a safe default. Downstream connectors can override for provider-specific filtering.
internal/connectors/plugins/public/stripe/plugin.go (3)
17-21: Good migration to SDK registryRegistration now goes through the SDK façade, aligning with the decoupling plan.
41-41: Good switch to SDK base pluginEmbedding
pluginsdk.NewBasePlugin()standardizes default behavior and error baselines.
68-71: Verify all legacy ErrNotYetInstalled references were migratedThe previous rg invocation skipped files; re-run this search and replace any matches with pluginsdk.ErrNotYetInstalled:
rg -n --hidden -S --unrestricted -P '\b(?:plugins.ErrNotYetInstalled|ErrNotYetInstalled)\b'
| if stripeErr.Code == stripe.ErrorCodeRateLimit { | ||
| return errorsutils.NewWrappedError( | ||
| err, | ||
| httpwrapper.ErrStatusCodeTooManyRequests, | ||
| pluginsdkhttp.ErrStatusCodeTooManyRequests, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Critical: pluginsdk errors.NewWrappedError likely breaks classification (uses two %w).
pluginsdk/errors.NewWrappedError (see pkg/pluginsdk/errors/errors.go) uses fmt.Errorf("%w: %w", ...), which is invalid (only one %w is honored). Result: only the first error is wrapped; the classification error (e.g., ErrStatusCodeTooManyRequests / ErrStatusCodeClientError) is not discoverable via errors.Is, breaking non‑retryable vs retryable decisioning.
Fix in pluginsdk/errors to wrap the classification error and keep the cause in the message:
--- a/pkg/pluginsdk/errors/errors.go
+++ b/pkg/pluginsdk/errors/errors.go
@@
-func NewWrappedError(cause error, newError error) error {
- return &wrappedError{err: fmt.Errorf("%w: %w", cause, newError)}
-}
+func NewWrappedError(cause error, classify error) error {
+ // Wrap classification for errors.Is(err, classify) while retaining the cause in the message.
+ return &wrappedError{err: fmt.Errorf("%w: %v", classify, cause)}
+}Alternatively, if you want both errors to be discoverable via errors.Is, consider using errors.Join(classify, cause) inside wrappedError.
Also applies to: 88-94
🤖 Prompt for AI Agents
In internal/connectors/plugins/public/stripe/client/client.go around lines 81-86
(also apply same fix to 88-94), the use of pluginsdk/errors.NewWrappedError to
combine a classification sentinel and the stripe error results in only the first
%w being wrapped (the classification sentinel), breaking errors.Is checks for
the classification; update the call so the classification error is wrapped as
the primary error and the stripe cause is preserved in a way discoverable by
errors.Is (either by changing NewWrappedError to wrap the classification
sentinel and include the cause in the message, or replace this call-site with
errors.Join(classifyErr, causeErr) so both are discoverable), ensuring
non-retryable vs retryable classification remains detectable.
| func (c *client) Do(ctx context.Context, req *http.Request, expectedBody, errorBody any) (int, error) { | ||
| resp, err := c.httpClient.Do(req) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("failed to make request: %w", err) | ||
| } | ||
| reqErr := c.httpErrorCheckerFn(resp.StatusCode) | ||
| if resp.Body == nil || (reqErr == nil && expectedBody == nil) || (reqErr != nil && errorBody == nil) { | ||
| return resp.StatusCode, reqErr | ||
| } |
There was a problem hiding this comment.
Request context is ignored and response body can leak on early returns.
- ctx isn’t propagated to the request (cancellation/timeouts/attrs lost).
- Early returns happen before closing resp.Body, risking resource leaks.
-func (c *client) Do(ctx context.Context, req *http.Request, expectedBody, errorBody any) (int, error) {
- resp, err := c.httpClient.Do(req)
+func (c *client) Do(ctx context.Context, req *http.Request, expectedBody, errorBody any) (int, error) {
+ req = req.WithContext(ctx)
+ resp, err := c.httpClient.Do(req)
if err != nil {
return 0, fmt.Errorf("failed to make request: %w", err)
}
+ if resp != nil && resp.Body != nil {
+ defer resp.Body.Close()
+ }
reqErr := c.httpErrorCheckerFn(resp.StatusCode)
if resp.Body == nil || (reqErr == nil && expectedBody == nil) || (reqErr != nil && errorBody == nil) {
return resp.StatusCode, reqErr
}
- defer resp.Body.Close()Also applies to: 72-76
🤖 Prompt for AI Agents
In pkg/pluginsdk/http/http.go around lines 63–71 (and similarly 72–76), the
request context is not propagated and resp.Body may leak on early returns; fix
by calling req = req.WithContext(ctx) (or pass contextualized req into
httpClient.Do), then after getting a non-nil resp immediately check resp.Body
and defer resp.Body.Close() so every return path closes the body, and only
perform the status/error/body-nil checks after the defer; ensure you still
return resp.StatusCode and the appropriate error or decoded body but never
return before closing resp.Body.
2 participants
Introduce a
pluginsdkmodule and migrate the Stripe plugin to use it, enabling future externalization of connectors.This PR implements the first phase of "Option B" from the investigation, creating a dedicated SDK (
pkg/pluginsdk) to provide a stable, decoupled API for connectors. This avoids exposing internal models directly and sets the foundation for moving connectors to external repositories. The Stripe plugin is updated to use this new SDK.