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

Add versioning, created updated times for notification templates schema #6171

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

darshanasbg
Copy link
Contributor

@darshanasbg darshanasbg commented Dec 3, 2024

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 45.26%. Comparing base (12f8728) to head (84027a3).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #6171   +/-   ##
=========================================
  Coverage     45.26%   45.26%           
+ Complexity    13819    13818    -1     
=========================================
  Files          1615     1615           
  Lines        100526   100528    +2     
  Branches      17307    17308    +1     
=========================================
+ Hits          45502    45509    +7     
+ Misses        48329    48323    -6     
- Partials       6695     6696    +1     
Flag Coverage Δ
unit 27.81% <ø> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1579,6 +1579,9 @@ CREATE TABLE IDN_NOTIFICATION_TYPE (
NAME VARCHAR(255) NOT NULL,
CHANNEL VARCHAR(255) NOT NULL,
TENANT_ID INTEGER NOT NULL,
VERSION VARCHAR(15) NOT NULL,
CREATED_AT TIMESTAMP NOT NULL,
Copy link
Contributor

@Thumimku Thumimku Dec 3, 2024

Choose a reason for hiding this comment

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

Suggested change
CREATED_AT TIMESTAMP NOT NULL,
CREATED_AT TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP,
UPDATED_AT TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,

Some DB support have DEFAULT and ON_UPDATE support, we can use that if applicable?

Copy link
Contributor Author

@darshanasbg darshanasbg Dec 3, 2024

Choose a reason for hiding this comment

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

Whether these values needs to be managed from the db side or the application side is something we have to decide here..

I also thought to handle this from the db level as per ee52c3c

But bit further check on things, I choose maintaining these values at the application level due to following reasons:

  1. There as data format difference between databases.. But this is a minor reason, since the difference was only at the millisecond levels
  2. Different db impls have their own way of doing this, there is no constant one way to do across this in different db types and versions.. and when consider an example on how these going to be effected with the data replications with the things like DR or multi-regions is something we don't have in-depth expertise with the various database types we need to support...

So going down with the path of depending on db to maintain these records increase the complexity of the solution, reduce the maintainability product. So selected the handling at the application level data handling to make things simple.

If we are concerned about, each developer to having to handle this at each of their DAO impls, we can avoid such duplicated efforts by centralizing maintaining this logic in a place like jdbcTemplates.

Copy link
Member

@omindu omindu Dec 3, 2024

Choose a reason for hiding this comment

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

+1 for the approach suggested initially in this PR based on @darshanasbg's reasoning

How were we historically handling the created times? The above concerns should be there for those as well

@darshanasbg darshanasbg marked this pull request as ready for review December 13, 2024 11:28
@darshanasbg darshanasbg merged commit 3dd227c into wso2:master Dec 13, 2024
2 checks passed
Copy link

sonarcloud bot commented Dec 13, 2024

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.

4 participants