Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(connector): add metadata transformation with encode_to_value for MerchantAccountUpdate and merchant_account_update #3434

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Jan 23, 2024

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This pull request aligns the validation of metadata for the create and update merchant account functionality. In the struct MerchantAccountUpdate, the request type now contains metadata: Option<pii::SecretSerdeValue>, which has been updated to metadata: Option<MerchantAccountMetadata>. As a result, validation and transformation for metadata have been added to the fn merchant_account_update.

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

When creating a merchant account using fn create_merchant_account, the MerchantAccountCreate request type contains metadata: Option<MerchantAccountMetadata>. These metadata are then transformed into JSON here and subsequently stored in the database as JSON.

Another struct, MerchantAccountResponse, contains metadata: Option<pii::SecretSerdeValue>. This MerchantAccountResponse is utilized for read operations as a result. So far, so good.

When updating a merchant account with fn merchant_account_update, there is a MerchantAccountUpdate request type that contains metadata: Option<pii::SecretSerdeValue> without any validation/transformation. I think this is a bug. When updating a merchant account using the fn merchant_account_update, someone can store whatever JSON in the metadata, and this JSON is not even checked for compatibility with MerchantAccountMetadata as is done in the fn create_merchant_account.

I discovered this potential issue while testing another PR.

How did you test it?

TODO: not yet, but if this PR/issue is relevant to fix, I can do postman test.

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

…r `MerchantAccountUpdate` and `merchant_account_update`
@bkontur bkontur requested review from a team as code owners January 23, 2024 22:54
@srujanchikke
Copy link
Contributor

Hi @bkontur, Thank you for bringing this to our attention. However, I believe the issue you mentioned has already been addressed in PR #3305. I think what you referred to in the description has been resolved in this pull request. Please review it to see if anything is still missing. Please share your findings with us!

@bkontur
Copy link
Contributor Author

bkontur commented Jan 31, 2024

Hi @bkontur, Thank you for bringing this to our attention. However, I believe the issue you mentioned has already been addressed in PR #3305. I think what you referred to in the description has been resolved in this pull request. Please review it to see if anything is still missing. Please share your findings with us!

Hi @srujanchikke iiuc, the #3305 is about fixing fn update_payment_connector, but this PR is about fixing fn merchant_account_update.

For example, you can create a merchant account with valid metadata:

{
   "compatible_connector":"coinbase",
   "city":"NY",
   "unit":"245"
}

but nothing stops you to update invalid metadata with fn merchant_account_update:

{
   "compatible_connector":"coinbase_or_invalid_value",
   "city":"NY",
   "unit":"245"
}

so my fix prevents to store an invalid api_enums::Connector value coinbase_or_invalid_value with fn merchant_account_update.

However, if you think it is not necessary, feel free to close this PR.

@srujanchikke
Copy link
Contributor

Hi @bkontur ,
My bad, This is a valid issue. Could you please create an issue and attach it to this PR? Also, please include screenshots of the Postman testing.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants