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: Change timestamp field in transactions #269

Merged
merged 7 commits into from
Jul 19, 2022

Conversation

gfyrag
Copy link
Contributor

@gfyrag gfyrag commented Jul 7, 2022

Change timestamp field in transactions from string to time.Time

This change the type of the field 'Timestamp' inside transactions.

Both sqlite and Postgres can handle encoding of the field without additional efforts.
But, at least sqlite, if the db column is not defined as a date type, is not able to decode the value directly to a time.Time struct.
So the decoding is still manual for reading transactions (ok on logs since the column has a date type).

Both SQLite and Postgres encode time.Time as rfc3339nano.

So, to keep backward compatibility, we :

  • Round the dates to the second internally (on transactions, and logs)
  • Add a custom marshal function on transactions to format dates as rfc3339

We could change the precision in a later release.

Also the PR set the same date for the log, and the generated transactions.
It was not the case previously. I don't think it is a breaking change since the logs is not exposed anywhere actually.

The PR also remove verbose logging on tests because the tests give so many line and the CI refuse to display all the lines.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring / Technical debt

What parts of the code are impacted ?

  • pkg/core
  • pkg/storage

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@gfyrag gfyrag requested a review from a team as a code owner July 7, 2022 12:44
@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch from 275fe33 to c97f440 Compare July 7, 2022 12:48
@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #269 (0bb66f8) into main (3e0f91b) will decrease coverage by 0.36%.
The diff coverage is 100.00%.

❗ Current head 0bb66f8 differs from pull request most recent head bc4438c. Consider uploading reports for the commit bc4438c to get more accurate results

@@            Coverage Diff             @@
##             main     #269      +/-   ##
==========================================
- Coverage   71.85%   71.48%   -0.37%     
==========================================
  Files          82       79       -3     
  Lines        4302     4230      -72     
==========================================
- Hits         3091     3024      -67     
- Misses        936      939       +3     
+ Partials      275      267       -8     
Impacted Files Coverage Δ
pkg/core/log.go 83.13% <100.00%> (+0.63%) ⬆️
pkg/core/transaction.go 75.51% <100.00%> (+5.51%) ⬆️
pkg/ledger/process.go 84.21% <100.00%> (ø)
pkg/storage/sqlstorage/log.go 78.70% <100.00%> (+1.78%) ⬆️
pkg/storage/sqlstorage/transactions.go 85.79% <100.00%> (ø)
pkg/storage/cache.go 53.84% <0.00%> (-23.08%) ⬇️
pkg/storage/sqlstorage/store.go 78.87% <0.00%> (-11.61%) ⬇️
pkg/storage/sqlstorage/flavor.go 53.19% <0.00%> (-4.26%) ⬇️
pkg/api/internal/testing.go 90.16% <0.00%> (-1.10%) ⬇️
pkg/storage/sqlstorage/migrate.go
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e0f91b...bc4438c. Read the comment docs.

@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch 2 times, most recently from 280a017 to 5a82b7c Compare July 7, 2022 12:56
@gfyrag gfyrag changed the title Refacto/num 624 change timestamp field in transactions feat: Change timestamp field in transactions Jul 7, 2022
@gfyrag gfyrag requested a review from flemzord as a code owner July 7, 2022 15:13
@@ -91,7 +91,6 @@ func (s *Store) AppendLog(ctx context.Context, logs ...core.Log) error {
func (s *Store) lastLog(ctx context.Context, exec executor) (*core.Log, error) {
var (
l core.Log
ts sql.NullString
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this removed ? shouldnt it have been replaced by a NullTime ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Scan() method is called directly with a pointer on the date :
err := rows.Scan(&l.ID, &l.Type, &l.Hash, &l.Date, &data)
No need to handle NULL values as the date is required.

jdupas22
jdupas22 previously approved these changes Jul 8, 2022
@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch from c0fa3c2 to b37c281 Compare July 8, 2022 13:29
pkg/ledger/process.go Outdated Show resolved Hide resolved
pkg/core/log.go Outdated Show resolved Hide resolved
@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch from 730a458 to c301dfa Compare July 15, 2022 14:36
jdupas22
jdupas22 previously approved these changes Jul 15, 2022
@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch from 0e29950 to 54172b9 Compare July 15, 2022 15:23
jdupas22
jdupas22 previously approved these changes Jul 15, 2022
Add MarshalJSON() on transaction object to encode time.Time as rfc3339 (for backward compatibility).
Also, since the dates was previously stored as rfc3339 into the database, we round to the second the generated dates when creating new logs.
@gfyrag gfyrag force-pushed the refacto/num-624-change-timestamp-field-in-transactions branch from 54172b9 to bc4438c Compare July 18, 2022 10:56
@gfyrag gfyrag merged commit e8d6169 into main Jul 19, 2022
@gfyrag gfyrag deleted the refacto/num-624-change-timestamp-field-in-transactions branch July 19, 2022 10:31
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