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

feat(core): diesel models, domain models and db interface changes for callback_mapper table #6571

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

Conversation

prasunna09
Copy link
Contributor

@prasunna09 prasunna09 commented Nov 14, 2024

Type of Change

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

Description

Created callback_mapper table, added diesel, domain models and implemented interface for DB operations.
this is added for network tokenization webhooks feature. Since this is generic, any kind of data (excluding sensitive info) can be stored.

db changes -
Screenshot 2024-11-14 at 4 51 02 PM

Network tokenization webhook feature use case - no merchant id is passed from token requestor, so network_token_requestor_ref_id is stored in this table along with merchant_id

Additional Changes

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

Motivation and Context

How did you test it?

This PR is only meant for creating new call_back_mapper table and doesn't interfere with any existing flows.

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

@prasunna09 prasunna09 added the C-feature Category: Feature request or enhancement label Nov 14, 2024
@prasunna09 prasunna09 added this to the November 2024 Release milestone Nov 14, 2024
@prasunna09 prasunna09 self-assigned this Nov 14, 2024
@prasunna09 prasunna09 requested review from a team as code owners November 14, 2024 11:26
@hyperswitch-bot hyperswitch-bot bot added the M-database-changes Metadata: This PR involves database schema changes label Nov 14, 2024
jarnura
jarnura previously approved these changes Nov 14, 2024
crates/diesel_models/src/call_back_mapper.rs Outdated Show resolved Hide resolved
crates/router/src/db/call_back_mapper.rs Outdated Show resolved Hide resolved
crates/router/src/db/call_back_mapper.rs Outdated Show resolved Hide resolved
@prasunna09 prasunna09 changed the title feat(core): diesel models and db interface changes for call_back_mapper table feat(core): diesel models, domain models and db interface changes for callback_mapper table Nov 20, 2024
Comment on lines 3 to 4
id VARCHAR(128) NOT NULL PRIMARY KEY,
type VARCHAR(64) NOT NULL,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What values are stored in the id column: are we generating them or is it being provided from third-party sources?

    1. If it's being provided by third party sources, how do we handle possible collision happening due to the id field?
  2. What sort of values does the type column hold, can they be an enum on the Rust side while being a VARCHAR on the database side?

Comment on lines 6 to 7
created_at TIMESTAMP NOT NULL DEFAULT now()::TIMESTAMP,
last_modified_at TIMESTAMP NOT NULL DEFAULT now()::TIMESTAMP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the defaults on the database side, have the application provide the values for these columns always. This would help prevent issues with the timestamps being generated based on the database server's time zone configuration.

use crate::schema::callback_mapper;

#[derive(
Clone, Debug, Eq, PartialEq, Identifiable, Queryable, Selectable, Serialize, Deserialize,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the Serialize and Deserialize derives on CallBackMapper and CallBackMapperNew?

crates/diesel_models/src/callback_mapper.rs Outdated Show resolved Hide resolved
Comment on lines 14 to 20
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
pub struct CallBackMapperNew {
pub id: String,
#[serde(rename = "type")]
pub type_: String,
pub data: pii::SecretSerdeValue,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you'd need the CallBackMapperNew domain model (most probably). The idea is that we should be able to insert and retrieve the domain model itself, and it would be converted to the corresponding types from the diesel_models crate based on need.

crates/router/src/db/callback_mapper.rs Outdated Show resolved Hide resolved
Comment on lines 15 to 18
async fn insert_call_back_mapper(
&self,
call_back_mapper: DomainCallBackMapper::CallBackMapperNew,
) -> CustomResult<DomainCallBackMapper::CallBackMapper, errors::StorageError>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have this method accept the CallbackMapper domain type itself?

crates/router/src/db/callback_mapper.rs Outdated Show resolved Hide resolved
crates/router/src/db/callback_mapper.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature Category: Feature request or enhancement M-database-changes Metadata: This PR involves database schema changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants