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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Jan 25, 2024
Merged

fix(connector): Use ConnectorError::InvalidConnectorConfig for an invalid CoinbaseConnectorMeta #3168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
dbfa22a
chore: removed unnecessary clone
bkontur Dec 19, 2023
276b1d4
fix(connector): use `ConnectorError::InvalidConnectorConfig` for an i…
bkontur Dec 19, 2023
40c78ea
chore: unrelated clippy
bkontur Dec 19, 2023
109e330
Merge branch 'main' into bko-fix-2899
bkontur Dec 19, 2023
12f0664
Merge remote-tracking branch 'origin/main' into bko-fix-2899
bkontur Dec 19, 2023
cd400bd
chore: fix clippy for coinbase transformer tests
bkontur Dec 19, 2023
9352adb
Merge branch 'main' into bko-fix-2899
bkontur Dec 20, 2023
9c11db9
Merge remote-tracking branch 'origin/main' into bko-fix-2899
bkontur Jan 6, 2024
1f14cb3
chore: undo requested changes
bkontur Jan 6, 2024
54cf926
refactor: move test `coinbase_payments_request_try_from_works` from `…
bkontur Jan 6, 2024
3e83c44
Merge remote-tracking branch 'origin/main' into bko-fix-2899
bkontur Jan 10, 2024
0bc0674
chore: address pr review - change error message to `metadata`
bkontur Jan 10, 2024
7154951
chore: fmt
bkontur Jan 10, 2024
f1f1ca2
Merge branch 'juspay:main' into bko-fix-2899
bkontur Jan 11, 2024
0fa74d2
Merge branch 'juspay:main' into bko-fix-2899
bkontur Jan 18, 2024
f4e1512
Update crates/router/src/core/admin.rs
bkontur Jan 18, 2024
5df2ef4
chore: address pr review comments
bkontur Jan 18, 2024
56d7303
chore: fmt
bkontur Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions crates/router/src/connector/coinbase/transformers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use std::collections::HashMap;

use common_utils::pii;
use error_stack::ResultExt;
use serde::{Deserialize, Serialize};

use crate::{
Expand Down Expand Up @@ -250,6 +252,14 @@ pub struct CoinbaseConnectorMeta {
pub pricing_type: String,
}

impl TryFrom<&Option<pii::SecretSerdeValue>> for CoinbaseConnectorMeta {
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

type Error = error_stack::Report<errors::ConnectorError>;
fn try_from(meta_data: &Option<pii::SecretSerdeValue>) -> Result<Self, Self::Error> {
utils::to_connector_meta_from_secret(meta_data.clone())
.change_context(errors::ConnectorError::InvalidConnectorConfig { config: "metadata" })
}
}

fn get_crypto_specific_payment_data(
item: &types::PaymentsAuthorizeRouterData,
) -> Result<CoinbasePaymentsRequest, error_stack::Report<errors::ConnectorError>> {
Expand All @@ -260,11 +270,10 @@ fn get_crypto_specific_payment_data(
let name =
billing_address.and_then(|add| add.get_first_name().ok().map(|name| name.to_owned()));
let description = item.get_description().ok();
let connector_meta: CoinbaseConnectorMeta =
utils::to_connector_meta_from_secret_with_required_field(
item.connector_meta_data.clone(),
"Pricing Type Not present in connector meta data",
)?;
let connector_meta = CoinbaseConnectorMeta::try_from(&item.connector_meta_data)
.change_context(errors::ConnectorError::InvalidConnectorConfig {
config: "Merchant connector account metadata",
})?;
let pricing_type = connector_meta.pricing_type;
let local_price = get_local_price(item);
let redirect_url = item.request.get_return_url()?;
Expand Down
17 changes: 0 additions & 17 deletions crates/router/src/connector/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1198,23 +1198,6 @@ where
json.parse_value(std::any::type_name::<T>()).switch()
}

pub fn to_connector_meta_from_secret_with_required_field<T>(
connector_meta: Option<Secret<serde_json::Value>>,
error_message: &'static str,
) -> Result<T, Error>
where
T: serde::de::DeserializeOwned,
{
let connector_error = errors::ConnectorError::MissingRequiredField {
field_name: error_message,
};
let parsed_meta = to_connector_meta_from_secret(connector_meta).ok();
match parsed_meta {
Some(meta) => Ok(meta),
_ => Err(connector_error.into()),
}
}

pub fn to_connector_meta_from_secret<T>(
connector_meta: Option<Secret<serde_json::Value>>,
) -> Result<T, Error>
Expand Down
2 changes: 1 addition & 1 deletion crates/router/src/core/admin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,9 +1713,9 @@ pub(crate) fn validate_auth_and_metadata_type(
checkout::transformers::CheckoutAuthType::try_from(val)?;
Ok(())
}

api_enums::Connector::Coinbase => {
coinbase::transformers::CoinbaseAuthType::try_from(val)?;
coinbase::transformers::CoinbaseConnectorMeta::try_from(connector_meta_data)?;
Ok(())
}
api_enums::Connector::Cryptopay => {
Expand Down
Loading