fix(api): PMNT-121 Return 400 when connector cannot create account #473
Conversation
WalkthroughA new typed error Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Engine
Client->>API: Create Account or Payment Request
API->>Engine: CreateFormanceAccount() / CreateFormancePayment()
Engine-->>API: ErrConnectorCapabilityNotSupported
API-->>Client: HTTP 400 with CONNECTOR_CAPABILITY_NOT_SUPPORTED error
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
internal/connectors/engine/engine.go
Outdated
| @@ -366,7 +366,7 @@ func (e *engine) CreateFormanceAccount(ctx context.Context, account models.Accou | |||
| } | |||
|
|
|||
| if !found { | |||
| err := errors.New("connector does not support account creation") | |||
| err := ErrConnectorCapacityNotSupported | |||
There was a problem hiding this comment.
Maybe that would be interesting to have a more precise error containing which capacity was not supported
There was a problem hiding this comment.
You could create a typed error that would let you do that.
Alternatively you could just provide a link to documentation here in the user facing error message: https://docs.formance.com/payments/connectors/#supported-processors
The above documentation is generated from the capabilities of each connector.
There was a problem hiding this comment.
I added the typed error
internal/api/v3/errors.go
Outdated
| @@ -31,6 +33,8 @@ func handleServiceErrors(w http.ResponseWriter, r *http.Request, err error) { | |||
| api.BadRequest(w, ErrValidation, err) | |||
| case errors.Is(err, services.ErrNotFound): | |||
| api.NotFound(w, err) | |||
| case errors.Is(err, engine.ErrConnectorCapacityNotSupported): | |||
There was a problem hiding this comment.
I did not create an error in "services", should I?
There was a problem hiding this comment.
Assuming you can unwrap the engine error correctly here, it is not needed in services.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/connectors/engine/engine.go (1)
368-372: Return path aligned, but wording still inconsistent with capability enumGood switch to a typed sentinel error; however the message still talks about “capacity” while the enum we checked above is
CAPABILITY_ALLOW_FORMANCE_ACCOUNT_CREATION. If you keep the current identifier, at least harmonise the error text to stay in sync with the public enum – it is often bubbled up to logs and API callers.-err := ErrConnectorCapacityNotSupported +err := ErrConnectorCapacityNotSupported // TODO: rename to …Capability… ?Also,
CreateFormancePaymentbelow still returns the old anonymous error string (“connector does not support payment creation”). For consistency and easier HTTP mapping you may want to introduce the same typed error there.internal/api/services/accounts_create_test.go (1)
48-53: Test improves coverage – tiny nit on assertion styleThe new case correctly exercises the new typed error.
Very minor: since you already flagtypedError: true, you can spare the duplicate literal inexpectedErrorand reuse theerrfield to reduce repetition, but that is stylistic – feel free to ignore.internal/api/v3/errors.go (2)
4-5: Avoid importing large package set when only one constant neededAdding
enginesolely forErrConnectorCapacityNotSupportedis fine, but if you rename the constant as suggested you could re-export it fromservices(existing precedent forErrValidation, etc.) and keep HTTP layer decoupled from engine internals.
15-20: Constant name follows variable – revisit after renamingShould be updated if the sentinel error is renamed to keep parity between API code and engine error.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/api/services/accounts_create_test.go(1 hunks)internal/api/v3/errors.go(3 hunks)internal/connectors/engine/engine.go(1 hunks)internal/connectors/engine/errors.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Dirty
🔇 Additional comments (1)
internal/api/v3/errors.go (1)
36-37: Mapping added – ensure exhaustive coverageNice addition. Remember to update
CreateFormancePaymentonce a typed error is introduced there, otherwise the HTTP layer will still fall through to 500 for the analogous payment scenario.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #473 +/- ##
==========================================
- Coverage 69.66% 69.65% -0.01%
==========================================
Files 627 627
Lines 32196 32208 +12
==========================================
+ Hits 22428 22436 +8
- Misses 8554 8558 +4
Partials 1214 1214 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
internal/connectors/engine/engine.go
Outdated
| @@ -366,7 +366,7 @@ func (e *engine) CreateFormanceAccount(ctx context.Context, account models.Accou | |||
| } | |||
|
|
|||
| if !found { | |||
| err := errors.New("connector does not support account creation") | |||
| err := ErrConnectorCapabilityNotSupported | |||
There was a problem hiding this comment.
Can you check if there are other places that return untyped error when a capability is not supported?
There was a problem hiding this comment.
CreatePayment and CreateAccount are the only places I have found where the registry.GetCapabilities function was called.
There was a problem hiding this comment.
But then there is all the stuff that's executed from the worker, where you get "not implemented" error if the connector plugin has not implemented the function. Maybe we could then explicitly create function in the plugins that would return the not supported error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/api/v3/handler_accounts_create_test.go (2)
3-16: Fix import grouping – external package slipped into the std-lib block
github.com/formancehq/payments/internal/connectors/engineis an external package but is currently placed between standard-library imports.
This will be automatically re-written bygoimports/golangci-lint import-orderand may break CI. Put it below the blank line that separates std-lib and third-party blocks.-import ( - "errors" - "github.com/formancehq/payments/internal/connectors/engine" - "net/http" - "net/http/httptest" - "time" +import ( + // Std-lib + "errors" + "net/http" + "net/http/httptest" + "time" + + // External / internal + "github.com/formancehq/payments/internal/connectors/engine"
44-57: Minor naming & cleanup in the new test case
- Acronyms should stay uppercase to follow Go naming conventions →
notSupportedConnectorIDinstead ofnotSupportedConnectorId.expectedErris only used once as the return value; consider in-lining to reduce noise.- expectedErr := engine.ErrConnectorCapabilityNotSupported - m.EXPECT().AccountsCreate(gomock.Any(), gomock.Any()).Return(expectedErr) - notSupportedConnectorId := models.ConnectorID{Reference: uuid.New(), Provider: "stripe"} + m.EXPECT().AccountsCreate(gomock.Any(), gomock.Any()). + Return(engine.ErrConnectorCapabilityNotSupported) + + notSupportedConnectorID := models.ConnectorID{Reference: uuid.New(), Provider: "stripe"} ... - ConnectorID: notSupportedConnectorId.String(), + ConnectorID: notSupportedConnectorID.String(),These tweaks keep the test concise and consistent with existing naming patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/api/services/accounts_create_test.go(1 hunks)internal/api/v3/handler_accounts_create_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/api/services/accounts_create_test.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/api/v3/handler_payments_create_test.go (1)
60-78: Nit: keepIDacronym capitalised and drop redundant variable
- Go style:
IDis an acronym and should stay uppercase (notSupportedConnectorID).expectedErris only used as the return value of the mock; the local variable itself is not needed.- notSupportedConnectorId := models.ConnectorID{Reference: uuid.New(), Provider: "stripe"} - expectedErr := &engine.ErrConnectorCapabilityNotSupported{Capability: "CreateFormancePayment", Provider: notSupportedConnectorId.Provider} - m.EXPECT().PaymentsCreate(gomock.Any(), gomock.Any()).Return(expectedErr) + notSupportedConnectorID := models.ConnectorID{Reference: uuid.New(), Provider: "stripe"} + m.EXPECT(). + PaymentsCreate(gomock.Any(), gomock.Any()). + Return(&engine.ErrConnectorCapabilityNotSupported{ + Capability: "CreateFormancePayment", + Provider: notSupportedConnectorID.Provider, + })internal/api/v2/handler_payments_create_test.go (1)
48-66: Same acronym / redundancy issue as v3 testApply the same clean-up for
notSupportedConnectorIDand inline the returned error.Additionally, double-check the constant
PAYMENT_STATUS_AMOUNT_ADJUSTEMENT– the usual spelling isADJUSTMENT. If it’s a typo in the model, worth fixing at the source before it leaks further.internal/api/v2/errors.go (1)
15-22: Potential name collision with engine errorThe new constant re-uses the exact name of the engine error (
ErrConnectorCapabilityNotSupported).
Although they live in different packages, using the same identifier for two different kinds (string vs struct) can be confusing in call sites:api.BadRequest(w, v2.ErrConnectorCapabilityNotSupported, err)Developers might assume the second argument is the same “error object”. Consider a clearer suffix, e.g.
ErrCodeConnectorCapabilityNotSupported.internal/api/v2/handler_accounts_create_test.go (1)
47-62: Apply the same acronym & inlining nitRename
notSupportedConnectorId→notSupportedConnectorIDand inline the error value in the mock expectation to stay consistent with other tests.internal/connectors/engine/errors.go (3)
17-20: Exported type should carry a doc-comment and could drop theErrprefixThe project usually reserves the
Err*naming convention for sentinel variables (ErrValidation,ErrNotFound).
Here, you introduce a type namedErrConnectorCapabilityNotSupported, which slightly blurs that distinction. A more idiomatic pattern is:// ConnectorCapabilityNotSupportedError is returned when … type ConnectorCapabilityNotSupportedError struct { … } var ErrConnectorCapabilityNotSupported = &ConnectorCapabilityNotSupportedError{}– or simply keep the struct but rename it to
ConnectorCapabilityNotSupportedError.Either way, add a godoc comment so public packages get proper documentation.
22-24: Error message wording – put dynamic values firstMinor readability tweak: place the variable parts in quotes and start with the provider for easier grepping in logs.
-return fmt.Sprintf("%s capability is not supported by the provider %s. Check here the supported features: https://docs.formance.com/payments/connectors/#supported-processors", e.Capability, e.Provider) +return fmt.Sprintf("provider %q does not support capability %q. See supported features: https://docs.formance.com/payments/connectors/#supported-processors", e.Provider, e.Capability)
26-29: CustomIsimplementation could compare against the concrete type onlyCurrent code works, but an ultra-small simplification avoids the extra allocation check:
-func (e *ErrConnectorCapabilityNotSupported) Is(err error) bool { - _, ok := err.(*ErrConnectorCapabilityNotSupported) - return ok +func (e *ErrConnectorCapabilityNotSupported) Is(target error) bool { + _, ok := target.(*ErrConnectorCapabilityNotSupported) + return ok }Renaming the parameter also clarifies that
targetis the value passed toerrors.Is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
internal/api/services/accounts_create_test.go(1 hunks)internal/api/services/payments_create_test.go(1 hunks)internal/api/v2/errors.go(3 hunks)internal/api/v2/handler_accounts_create_test.go(2 hunks)internal/api/v2/handler_payments_create_test.go(2 hunks)internal/api/v3/errors.go(3 hunks)internal/api/v3/handler_accounts_create_test.go(2 hunks)internal/api/v3/handler_payments_create_test.go(2 hunks)internal/connectors/engine/engine.go(4 hunks)internal/connectors/engine/errors.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/api/v3/handler_accounts_create_test.go
- internal/api/services/accounts_create_test.go
- internal/api/v3/errors.go
- internal/connectors/engine/engine.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (2)
internal/api/v2/errors.go (2)
4-6: Import only what you use
errorsis already in the import list; after this change it’s still used, good.
No action required.
37-38: Fine-grained matching is goodMatching with
errors.Is(err, &engine.ErrConnectorCapabilityNotSupported{})relies on the customIsmethod – well done, this keeps the switch concise.
| { | ||
| name: "capability not supported", | ||
| err: &engine.ErrConnectorCapabilityNotSupported{}, | ||
| expectedError: &engine.ErrConnectorCapabilityNotSupported{}, | ||
| typedError: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider asserting the error content, not only the type
The new test checks ErrorIs, which guarantees the type matches, but it doesn’t validate the fields (Capability, Provider) embedded in the error.
That leaves room for a regression where the engine returns the right type but the wrong capability/provider and the test would still pass.
- require.ErrorIs(t, err, test.expectedError)
+ var capErr *engine.ErrConnectorCapabilityNotSupported
+ require.ErrorAs(t, err, &capErr)
+ require.Equal(t, "CreateFormancePayment", capErr.Capability)
+ // provider field is empty in the current test data, adapt if neededCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In internal/api/services/payments_create_test.go around lines 48 to 53, the test
currently asserts only the error type using ErrorIs, which misses verifying the
actual content of the error fields like Capability and Provider. Update the test
to explicitly check that these fields in the returned error match the expected
values, ensuring the error's internal data is correct and preventing regressions
where the type is right but the content is wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…rted-400' into fix/PMNT-121-connector-not-supported-400 # Conflicts: # internal/api/services/accounts_create_test.go
There was a problem hiding this comment.
Bug: Capability Name Mismatch in Test Case
The test case for ErrConnectorCapabilityNotSupported expects the capability 'CreateAccounts', but the mocked engine method CreateFormanceAccount implies the capability should be 'CreateFormanceAccount'. This mismatch causes the test to fail or incorrectly validate the error.
internal/api/services/accounts_create_test.go#L49-L52
payments/internal/api/services/accounts_create_test.go
Lines 49 to 52 in a6d65b0
Bug: Payment Capability Name Mismatch
The capability not supported test case for payment creation expects the ErrConnectorCapabilityNotSupported error to contain 'CreatePayments' as the capability. This conflicts with the actual CreateFormancePayment engine function's capability name, which is 'CreateFormancePayment'. This mismatch will cause the test to fail or validate an incorrect error.
internal/api/services/payments_create_test.go#L49-L52
payments/internal/api/services/payments_create_test.go
Lines 49 to 52 in a6d65b0
Was this report helpful? Give feedback by reacting with 👍 or 👎
Now the http resbody looks like this: