fix: remove/resolve fixmes for services (#187)

crates/router/src/compatibility/stripe/errors.rs

@@ -384,7 +384,6 @@ impl From<errors::ApiErrorResponse> for StripeErrorCode {
             errors::ApiErrorResponse::PreconditionFailed { message } => {
                 Self::PreconditionFailed { message }
             }
-            errors::ApiErrorResponse::BadCredentials => Self::Unauthorized,
             errors::ApiErrorResponse::InvalidDataValue { field_name } => Self::ParameterMissing {
                 field_name: field_name.to_owned(),
                 param: field_name.to_owned(),

crates/router/src/core/errors.rs

@@ -13,7 +13,7 @@ use router_env::opentelemetry::metrics::MetricsError;
 use storage_models::errors as storage_errors;
 
 pub use self::api_error_response::ApiErrorResponse;
-pub(crate) use self::utils::{ApiClientErrorExt, ConnectorErrorExt, StorageErrorExt};
+pub(crate) use self::utils::{ConnectorErrorExt, StorageErrorExt};
 use crate::services;
 pub type RouterResult<T> = CustomResult<T, ApiErrorResponse>;
 pub type RouterResponse<T> = CustomResult<services::BachResponse<T>, ApiErrorResponse>;
@@ -246,22 +246,8 @@ pub enum ApiClientError {
     #[error("Failed to decode response")]
     ResponseDecodingFailed,
 
-    #[error("Server responded with Bad Request")]
-    BadRequestReceived(bytes::Bytes),
-    #[error("Server responded with Unauthorized")]
-    UnauthorizedReceived(bytes::Bytes),
-    #[error("Server responded with Forbidden")]
-    ForbiddenReceived,
-    #[error("Server responded with Not Found")]
-    NotFoundReceived(bytes::Bytes),
-    #[error("Server responded with Method Not Allowed")]
-    MethodNotAllowedReceived,
     #[error("Server responded with Request Timeout")]
     RequestTimeoutReceived,
-    #[error("Server responded with Unprocessable Entity")]
-    UnprocessableEntityReceived(bytes::Bytes),
-    #[error("Server responded with Too Many Requests")]
-    TooManyRequestsReceived,
 
     #[error("Server responded with Internal Server Error")]
     InternalServerErrorReceived,

crates/router/src/core/errors/api_error_response.rs

@@ -19,15 +19,9 @@ pub enum ErrorType {
 pub enum ApiErrorResponse {
     #[error(
         error_type = ErrorType::InvalidRequestError, code = "IR_01",
-        message = "API key not provided. Provide API key in the Authorization header, using api-key (e.g api-key: API_KEY)."
+        message = "API key not provided or invalid API key used. Provide API key in the Authorization header or create new API key, using api-key (e.g api-key: API_KEY)."
     )]
     Unauthorized,
-    #[error(
-        error_type = ErrorType::InvalidRequestError, code = "IR_02",
-        message = "Access forbidden, invalid API key was used. Please create your new API key from \
-                    the Dashboard Settings section."
-    )]
-    BadCredentials,
     #[error(error_type = ErrorType::InvalidRequestError, code = "IR_03", message = "Unrecognized request URL.")]
     InvalidRequestUrl,
     #[error(error_type = ErrorType::InvalidRequestError, code = "IR_04", message = "The HTTP method is not applicable for this API.")]
@@ -148,9 +142,7 @@ impl actix_web::ResponseError for ApiErrorResponse {
         use reqwest::StatusCode;
 
         match self {
-            Self::Unauthorized | Self::BadCredentials | Self::InvalidEphermeralKey => {
+            Self::Unauthorized | Self::InvalidEphermeralKey => StatusCode::UNAUTHORIZED, // 401
-                StatusCode::UNAUTHORIZED
-            } // 401
             Self::InvalidRequestUrl => StatusCode::NOT_FOUND,                            // 404
             Self::InvalidHttpMethod => StatusCode::METHOD_NOT_ALLOWED,                   // 405
             Self::MissingRequiredField { .. } | Self::InvalidDataValue { .. } => {

crates/router/src/core/errors/utils.rs

@@ -36,27 +36,6 @@ impl StorageErrorExt for error_stack::Report<errors::StorageError> {
     }
 }
 
-pub(crate) trait ApiClientErrorExt {
-    fn to_unsuccessful_processing_step_response(
-        self,
-    ) -> error_stack::Report<errors::ConnectorError>;
-}
-
-impl ApiClientErrorExt for error_stack::Report<errors::ApiClientError> {
-    fn to_unsuccessful_processing_step_response(
-        self,
-    ) -> error_stack::Report<errors::ConnectorError> {
-        let data = match self.current_context() {
-            errors::ApiClientError::BadRequestReceived(bytes)
-            | errors::ApiClientError::UnauthorizedReceived(bytes)
-            | errors::ApiClientError::NotFoundReceived(bytes)
-            | errors::ApiClientError::UnprocessableEntityReceived(bytes) => Some(bytes.clone()),
-            _ => None,
-        };
-        self.change_context(errors::ConnectorError::ProcessingStepFailed(data))
-    }
-}
-
 pub(crate) trait ConnectorErrorExt {
     fn to_refund_failed_response(self) -> error_stack::Report<errors::ApiErrorResponse>;
     fn to_payment_failed_response(self) -> error_stack::Report<errors::ApiErrorResponse>;

crates/router/src/services/api.rs

@@ -18,9 +18,7 @@ pub use self::request::{Method, Request, RequestBuilder};
 use crate::{
     configs::settings::Connectors,
     core::{
-        errors::{
+        errors::{self, CustomResult, RouterResponse, RouterResult, StorageErrorExt},
-            self, ApiClientErrorExt, CustomResult, RouterResponse, RouterResult, StorageErrorExt,
-        },
         payments,
     },
     db::StorageInterface,
@@ -173,7 +171,8 @@ where
                             logger::debug!(?response);
                             Ok(response)
                         }
-                        Err(error) => Err(error.to_unsuccessful_processing_step_response()),
+                        Err(error) => Err(error
+                            .change_context(errors::ConnectorError::ProcessingStepFailed(None))),
                     }
                 }
                 None => Ok(router_data),
@@ -205,7 +204,6 @@ async fn send_request(
     logger::debug!(method=?request.method, headers=?request.headers, payload=?request.payload, ?request);
     let url = &request.url;
     let should_bypass_proxy = client::proxy_bypass_urls(&state.conf.locker).contains(url);
-    // TODO propogate error for request timeout
     let client = client::create_client(
         &state.conf.proxy,
         should_bypass_proxy,
@@ -247,8 +245,11 @@ async fn send_request(
         Method::Put => client.put(url).add_headers(headers).send().await,
         Method::Delete => client.delete(url).add_headers(headers).send().await,
     }
+    .map_err(|error| match error {
+        error if error.is_timeout() => errors::ApiClientError::RequestTimeoutReceived,
+        _ => errors::ApiClientError::RequestNotSent,
+    })
     .into_report()
-    .change_context(errors::ApiClientError::RequestNotSent)
     .attach_printable("Unable to send request to connector")
 }
 
@@ -313,11 +314,8 @@ async fn handle_response(
                     }))
                 }
 
-                _ => {
+                _ => Err(report!(errors::ApiClientError::UnexpectedServerResponse)
-                    // FIXME: may need to understand redirects
+                    .attach_printable("Unexpected response from server")),
-                    Err(report!(errors::ApiClientError::UnexpectedServerResponse)
-                        .attach_printable("Unexpected response from server"))
-                }
             }
         })?
         .await
@@ -541,24 +539,16 @@ pub async fn authenticate_merchant<'a>(
             authenticate_by_api_key(store, api_key).await
         }
 
-        MerchantAuthentication::MerchantId(merchant_id) => {
+        MerchantAuthentication::MerchantId(merchant_id) => store
-            store
             .find_merchant_account_by_merchant_id(&merchant_id)
             .await
-                .map_err(|error| {
+            .map_err(|error| error.to_not_found_response(errors::ApiErrorResponse::Unauthorized)),
-                    // TODO: The BadCredentials error is too specific for api keys, and inappropriate for AdminApiKey/MerchantID
-                    // https://juspay.atlassian.net/browse/ORCA-366
-                    error.to_not_found_response(errors::ApiErrorResponse::BadCredentials)
-                })
-        }
 
         MerchantAuthentication::AdminApiKey => {
             let admin_api_key =
                 get_api_key(request).change_context(errors::ApiErrorResponse::Unauthorized)?;
             if admin_api_key != "test_admin" {
-                // TODO: The BadCredentials error is too specific for api keys, and inappropriate
+                Err(report!(errors::ApiErrorResponse::Unauthorized)
-                // for AdminApiKey/MerchantID
-                Err(report!(errors::ApiErrorResponse::BadCredentials)
                     .attach_printable("Admin Authentication Failure"))?;
             }
 
@@ -599,7 +589,7 @@ pub async fn authenticate_connector<'a>(
         ConnectorAuthentication::MerchantId(merchant_id) => store
             .find_merchant_account_by_merchant_id(merchant_id)
             .await
-            .map_err(|error| error.to_not_found_response(errors::ApiErrorResponse::BadCredentials)),
+            .map_err(|error| error.to_not_found_response(errors::ApiErrorResponse::Unauthorized)),
     }
 }
 
@@ -627,7 +617,7 @@ pub(crate) async fn authenticate_eph_key<'a>(
         let ek = store
             .get_ephemeral_key(api_key)
             .await
-            .change_context(errors::ApiErrorResponse::BadCredentials)?;
+            .change_context(errors::ApiErrorResponse::Unauthorized)?;
         utils::when(
             ek.customer_id.ne(&customer_id),
             Err(report!(errors::ApiErrorResponse::InvalidEphermeralKey)),
@@ -657,7 +647,7 @@ pub async fn authenticate_by_api_key(
     store
         .find_merchant_account_by_api_key(api_key)
         .await
-        .change_context(errors::ApiErrorResponse::BadCredentials)
+        .change_context(errors::ApiErrorResponse::Unauthorized)
         .attach_printable("Merchant not authenticated")
 }
 
@@ -668,7 +658,7 @@ async fn authenticate_by_publishable_key(
     store
         .find_merchant_account_by_publishable_key(publishable_key)
         .await
-        .change_context(errors::ApiErrorResponse::BadCredentials)
+        .change_context(errors::ApiErrorResponse::Unauthorized)
         .attach_printable("Merchant not authenticated")
 }
 

crates/router/src/services/api/client.rs

@@ -100,7 +100,6 @@ pub(super) fn create_client(
         .attach_printable_lazy(|| "Error with client library")
 }
 
-// TODO: Move to env variable
 pub(super) fn proxy_bypass_urls(locker: &Locker) -> Vec<String> {
     let locker_host = locker.host.to_owned();
     let basilisk_host = locker.basilisk_host.to_owned();

crates/router/src/services/api/request.rs

@@ -1,4 +1,4 @@
-use std::str::FromStr;
+use std::{collections, str::FromStr};
 
 use error_stack::{IntoReport, ResultExt};
 use masking::Secret;
@@ -10,7 +10,7 @@ use crate::{
     logger,
 };
 
-pub(crate) type Headers = Vec<(String, String)>;
+pub(crate) type Headers = collections::HashSet<(String, String)>;
 
 #[derive(
     Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize, strum::Display, strum::EnumString,
@@ -46,7 +46,7 @@ impl Request {
         Self {
             method,
             url: String::from(url),
-            headers: Vec::new(),
+            headers: collections::HashSet::new(),
             payload: None,
             content_type: None,
             certificate: None,
@@ -60,7 +60,7 @@ impl Request {
 
     pub fn add_header(&mut self, header: &str, value: &str) {
         self.headers
-            .push((String::from(header), String::from(value)));
+            .insert((String::from(header), String::from(value)));
     }
 
     pub fn add_content_type(&mut self, content_type: ContentType) {
@@ -91,7 +91,7 @@ impl RequestBuilder {
         Self {
             method: Method::Get,
             url: String::with_capacity(1024),
-            headers: Vec::new(),
+            headers: std::collections::HashSet::new(),
             payload: None,
             content_type: None,
             certificate: None,
@@ -110,14 +110,13 @@ impl RequestBuilder {
     }
 
     pub fn header(mut self, header: &str, value: &str) -> Self {
-        self.headers.push((header.into(), value.into()));
+        self.headers.insert((header.into(), value.into()));
         self
     }
 
     pub fn headers(mut self, headers: Vec<(String, String)>) -> Self {
-        // Fixme add union property
+        let mut h = headers.into_iter().map(|(h, v)| (h, v));
-        let mut h = headers.into_iter().map(|(h, v)| (h, v)).collect();
+        self.headers.extend(&mut h);
-        self.headers.append(&mut h);
         self
     }
 

crates/router/src/services/encryption.rs

@@ -234,7 +234,7 @@ pub fn decrypt(mut data: Vec<u8>, key: &[u8]) -> CustomResult<String, errors::En
 }
 
 pub fn get_key_id(keys: &Jwekey) -> &str {
-    let key_identifier = "1"; //TODO https://github.com/juspay/orca/issues/46
+    let key_identifier = "1"; // [#46]: Fetch this value from redis or external sources
     if key_identifier == "1" {
         &keys.locker_key_identifier1
     } else {