feat(payments_v2): add logic to render required_fields in response of payment_method list #8317
Conversation
Sakilmostak
added
A-core
|
Review changes with Changed Files
|
WalkthroughThe changes update the handling of required fields for payment methods by making the required fields always present as vectors, even if empty, instead of being optional. Supporting structures, traits, and methods are introduced or updated to consistently retrieve and manage required fields, with naming and type adjustments throughout the relevant modules. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant PaymentMethodsCore
participant Settings
Client->>API: Request list of payment methods
API->>PaymentMethodsCore: list_payment_methods()
PaymentMethodsCore->>Settings: Get RequiredFields config
PaymentMethodsCore->>PaymentMethodsCore: get_required_fields(config)
PaymentMethodsCore->>PaymentMethodsCore: Combine common and mandate fields
PaymentMethodsCore->>API: Return payment methods with required_fields (Vec)
API->>Client: Respond with payment methods and required_fields
Poem
🪧 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 (
|
![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)
crates/router/src/core/payments/payment_methods.rs (4)
63-68: Avoid cloning the entirerequired_fieldsconfig on every request
state.conf.required_fields.clone()copies the whole configuration for eachlist_payment_methodscall.
Unless the struct is trivially small, this is avoidable churn. Passing an immutable reference (or anArc) keeps the API identical for downstream code while eliminating the per-call allocation.- .get_required_fields(RequiredFieldsInput::new(state.conf.required_fields.clone())) + .get_required_fields(RequiredFieldsInput::new(&state.conf.required_fields))…and update
RequiredFieldsInputaccordingly.
75-86:RequiredFieldsInputis a single-field wrapper – consider inliningA newtype wrapper makes sense when you need custom traits or type safety. Here it just forwards a cloned config. Removing it (or converting it into a simple
&settings::RequiredFieldsparameter) reduces mental overhead and an extra struct hop.
88-111: Trait may be over-generalised and name collides with impl method
GetRequiredFieldsis only implemented forsettings::RequiredFields, and you already have another method namedget_required_fieldsonFilteredPaymentMethodsEnabled. The duplicate naming is easy to mix up in call-sites. If no additional implementors are expected, a plain inherent method onsettings::RequiredFieldskeeps the API flatter and avoids the cognitive clash.
133-153: Minor allocation optimisations in required-field aggregation
iter().flatten().map(ToOwned::to_owned)clones each element one by one.
IfRequiredFieldInfoisClone,iter().flatten().cloned()is clearer and avoids importingToOwned.common_required_fields.chain(mandate_required_fields).collect::<Vec<_>>()builds an intermediate iterator just to collect; consider reserving capacity when both vectors areSometo reduce reallocations.Not a blocker, but easy wins.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/api_models/src/payments.rs(1 hunks)crates/router/src/core/payments/payment_methods.rs(8 hunks)
🔇 Additional comments (1)
crates/router/src/core/payments/payment_methods.rs (1)
236-241:surchargeis alwaysNone– verify intended behaviour
perform_surcharge_calculationcopies therequired_fieldsbut leavessurcharge: None.
If surcharge calculation will be plugged in later, add aTODOso the placeholder isn’t forgotten; otherwise, remove the dead path to avoid misleading callers.
hyperswitch-bot
bot
removed
the
M-api-contract-changes
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api-reference-v2/openapi_spec.json(2 hunks)
🔇 Additional comments (1)
api-reference-v2/openapi_spec.json (1)
22037-22041: Confirm mandatory inclusion ofrequired_fields.Adding
"required_fields"to therequiredarray correctly enforces its presence in every response object, matching the updated Rust model (Vec<RequiredFieldInfo>) and the PR goal to always return this field (even if empty).
api-reference-v2/openapi_spec.json
Outdated
| "required_fields": { | ||
| "allOf": [ | ||
| { | ||
| "$ref": "#/components/schemas/RequiredFieldInfo" | ||
| } | ||
| ], | ||
| "nullable": true | ||
| "$ref": "#/components/schemas/RequiredFieldInfo" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix schema type: wrap RequiredFieldInfo in an array.
The required_fields property should be defined as an array of RequiredFieldInfo objects—not a single object reference. Without the "type": "array" and "items" wrapper, code generators and validators will misinterpret this field.
Apply this diff:
-"required_fields": {
- "$ref": "#/components/schemas/RequiredFieldInfo"
-},
+"required_fields": {
+ "type": "array",
+ "items": {
+ "$ref": "#/components/schemas/RequiredFieldInfo"
+ }
+},📝 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.
| "required_fields": { | |
| "allOf": [ | |
| { | |
| "$ref": "#/components/schemas/RequiredFieldInfo" | |
| } | |
| ], | |
| "nullable": true | |
| "$ref": "#/components/schemas/RequiredFieldInfo" | |
| }, | |
| "required_fields": { | |
| "type": "array", | |
| "items": { | |
| "$ref": "#/components/schemas/RequiredFieldInfo" | |
| } | |
| }, |
🤖 Prompt for AI Agents
In api-reference-v2/openapi_spec.json around lines 22057 to 22059, the
required_fields property is incorrectly defined as a single object reference. To
fix this, change required_fields to be an array by adding "type": "array" and
wrapping the "$ref" inside an "items" object. This ensures required_fields is
correctly interpreted as an array of RequiredFieldInfo objects.
… payment_method list (#8317) Co-authored-by: hyperswitch-bot[bot] <148525504+hyperswitch-bot[bot]@users.noreply.github.com>
Type of Change
Description
This add the filtering and logic for required fields in payment_mehtod list of payment's v2.
Additional Changes
Motivation and Context
How did you test it?
Tested through Postman:
Create an MCA in V2 (adyen):
Create an payment intent:
List payment method:
Following should be the response:
Checklist
cargo +nightly fmt --allcargo clippySummary by CodeRabbit