-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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): Use ConnectorError::InvalidConnectorConfig
for an invalid CoinbaseConnectorMeta
#3168
Conversation
…nvalid `CoinbaseConnectorMeta`
ConnectorError::InvalidConnectorConfig
for an invalid CoinbaseConnectorMeta
ConnectorError::InvalidConnectorConfig
for an invalid CoinbaseConnectorMeta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please attach screenshots of testing if possible. refer #2746 to know how to test this issue.
@@ -250,6 +252,17 @@ pub struct CoinbaseConnectorMeta { | |||
pub pricing_type: String, | |||
} | |||
|
|||
impl TryFrom<&Option<pii::SecretSerdeValue>> for CoinbaseConnectorMeta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes will impact previous merchants who are using coinbase, This PR needs data migrations. Could you please refer #2746 description for more info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue#2899 does not mention any data migrations.
I've checked that #2746, and in that case, the migration involved replacing terminalId
with terminal_id
due to the removal of #[serde(rename_all = "camelCase")]
here.
However, my issue is different. If I understand correctly, this issue is about empty JSON for CoinbaseConnectorMeta
or the absence of the pricing_type
attribute in the JSON, and the goal is to add validations to prevent storing invalid CoinbaseConnectorMeta
.
So, I am not sure about the migration here. If empty JSON or JSON without pricing_type
is stored, how should the migration to the correct CoinbaseConnectorMeta
be handled? What value should be set for pricing_type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, We will take this up as these are data migrations. FYI, I was talking about existing merchants, we need to add pricing_type
in metadata and set it to empty string. In that case, when merchant tries to make payment connector throws error and they are expected to update the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srujanchikke aha, ok, thank you, make senses now, so I prepared migration script and simple test demonstration that it works, please see PR description.
…transformers.rs` to dedicated test file
Ok, I will try the next week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to make changes where ever CoinbaseConnectorMeta
is used
let connector_meta: CoinbaseConnectorMeta = |
let metadata = item.router_data.get_connector_meta()?; let connector_meta: CoinbaseConnectorMeta = metadata.parse_value(“CoinbaseConnectorMeta”) .change_context(errors::ConnectorError::InvalidConnectorConfig { config: “Merchant connector account metadata”, })?;
@@ -250,6 +252,17 @@ pub struct CoinbaseConnectorMeta { | |||
pub pricing_type: String, | |||
} | |||
|
|||
impl TryFrom<&Option<pii::SecretSerdeValue>> for CoinbaseConnectorMeta { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, We will take this up as these are data migrations. FYI, I was talking about existing merchants, we need to add pricing_type
in metadata and set it to empty string. In that case, when merchant tries to make payment connector throws error and they are expected to update the metadata.
@srujanchikke testing done with local |
@srujanchikke I think ,I've addressed all your review comments, added migration script (with simple test for sql) and added screenshots from postman testing, anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bkontur ,
Apologies for the delay, Given some minor comments, Please resolve it.
Type of Change
Description
I've removed
to_connector_meta_from_secret_with_required_field
, which usesMissingRequiredField
as an error (I also didn't like the unnecessary initialization of theMissingRequiredField
before we knew if it would be needed). Instead, I added aTryFrom
implementation forCoinbaseConnectorMeta
, which uses the requestedConnectorError::InvalidConnectorConfig
. Additionally, I includedCoinbaseConnectorMeta::try_from
in thevalidate_auth_and_metadata_type
as suggested.Migration
Additional Changes
Motivation and Context
This PR addresses the bug from the issue: #2899.
How did you test it?
Run unit test:
Migration script test
https://onecompiler.com/postgresql/3zz2tpkad
Test script with test data and run migration:
Output:
Run locally:
Invalid metadata:
Empty metadata:
Correct metadata:
Checklist
cargo +nightly fmt --all
cargo clippy