implement meta id and timestamp logic #64

henry-jackson · 2021-11-06T17:56:58Z

closes #63

Can be tested by posting metadata to a transaction (using quickstart transaction data from README):

curl -X POST -H "Content-Type: application/json" http://localhost:3068/quickstart/transactions/0/metadata -d \
'{
   "transaction-type": "driver-payout",
   "ride-type": "standard"
 }'

Checking db:

sqlite> select * from metadata;
meta_id  meta_target_type  meta_target_id  meta_key                  meta_value       timestamp
-------  ----------------  --------------  ------------------------  ---------------  -------------------------
0        transaction       0               transaction-type          "driver-payout"  2021-11-06T13:53:08-04:00
1        transaction       0               ride-type                 "standard"       2021-11-06T13:53:08-04:00

codecov · 2021-11-06T17:58:39Z

Codecov Report

Merging #64 (5128f82) into main (27c7379) will decrease coverage by 1.58%.
The diff coverage is 25.54%.

❗ Current head 5128f82 differs from pull request most recent head b9294b3. Consider uploading reports for the commit b9294b3 to get more accurate results

@@            Coverage Diff             @@
##             main      #64      +/-   ##
==========================================
- Coverage   35.78%   34.20%   -1.59%     
==========================================
  Files          28       28              
  Lines        1537     1725     +188     
==========================================
+ Hits          550      590      +40     
- Misses        930     1072     +142     
- Partials       57       63       +6

Impacted Files	Coverage Δ
storage/postgres/aggregations.go	`0.00% <0.00%> (ø)`
storage/postgres/metadata.go	`0.00% <0.00%> (ø)`
storage/storage.go	`38.46% <ø> (ø)`
storage/sqlite/metadata.go	`40.80% <25.58%> (-35.99%)`	⬇️
storage/sqlite/transactions.go	`82.77% <54.54%> (-1.57%)`	⬇️
ledger/ledger.go	`71.42% <70.00%> (-1.44%)`	⬇️
storage/sqlite/aggregations.go	`91.66% <100.00%> (+1.34%)`	⬆️

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 cb88643...b9294b3. Read the comment docs.

ledger/ledger.go

Signed-off-by: Henry Jackson <34102861+henry-jackson@users.noreply.github.com>

pullrequest

✅ This pull request was sent to the PullRequest network.

@henry-jackson you can click here to see the review status or cancel the code review job - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

PullRequest Breakdown

Reviewable lines of change

+ 167
- 63

100% Go

Type of change

Feature - These changes are adding a new feature or improvement to existing code.

pullrequest

Welcome to PullRequest! I added some comments regarding error handling and ways to possibly simplify your code.

Alex K

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

ledger/ledger.go

storage/postgres/metadata.go

storage/sqlite/metadata.go

pullrequest

There's a lot of great stuff here in this PR but I do suggest you make some changes.

Specifically, I called out where you need to use defer rows.Close() to make sure you clean up the allocations that happen when you create a result.

I also suggest you look at the types you're using and either use int64 everywhere or use int.

I also think you can use context.Background() everywhere you're using context.TODO() because you're never making calls to expire the context, you're not setting timeouts etc.

Additionally, I suggest the use of a logging package so that there's only one place in your entire application that cares about whether debugging is enabled. The package I suggested is used in thousands of applications and work very well.

Hopefully these suggestions are helpful. My goal is to try and save you from some very painful and weird debugging later!

Andy W

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

ledger/ledger.go

pullrequest · 2021-11-10T23:50:50Z

ledger/ledger.go

+
+	timestamp := time.Now().Format(time.RFC3339)
+
+	for key, value := range m {


if m is a map, you shouldn't count on the values in it being returned in order. As part of the Go spec it's always been stated that items in the map may not be returned in the same order each time. It's not random, there is a method to the madness, but it's not the same as the insertion order.

Andy W

The order that the metadata are inserted shouldn't be important - correct me if I'm wrong @altitude @flemzord

That's correct, we don't care about the order here!

I only brought up the ordering thing because it seemed like you were iterating through it to determine the current highest value of something and if things weren't always processed in the same order it seemed like that could cause problems.

Andy W

pullrequest · 2021-11-10T23:50:50Z

storage/postgres/aggregations.go

+	sb := sqlbuilder.NewSelectBuilder()
+
+	sb.
+		Select("count(*)").


There's no need to do count(*) to get a row count on the table as this will have the database pull a lot of data. In fact, postgres does a full table scan to calculate this result. It would be better to just query for the highest value of the table since that can use an index or you should use a sequence to actually generate the IDs instead.

https://www.postgresql.org/docs/9.5/sql-createsequence.html

Andy W

implemented as selecting meta_id desc limit 1. Using a sequence to generate IDs seems like a great idea for a ticket! @altitude

storage/postgres/metadata.go

pullrequest · 2021-11-10T23:50:50Z

storage/postgres/metadata.go


 	if err != nil {
 		return err
 	}

 	return nil
 }
+
+func (s *PGStore) FindMeta(q query.Query) (query.Cursor, error) {
+	q.Limit = int(math.Max(-1, math.Min(float64(q.Limit), 100))) + 1


The way this is written, it's really hard to figure out what you're doing here. I copied/pasted it into my IDE and reformatted it and ... I'm still not sure what you're trying to do here.

Andy W

This ensures the limit value is between 0 and 101. The reason why, I'm not completely sure - it would be worth at least adding a comment to make this readable at a glance. This logic is borrowed from other FindX functions such as FindTransaction. Checking git blame it looks like @altitude may have been the original author - Clem do you have any more info on the reason for this limit so that we could document?

pullrequest · 2021-11-10T23:50:50Z

storage/postgres/metadata.go

+		return c, err
+	}
+
+	foundMetadata := map[int64]core.Metadata{}


you should call make() on this with a length of 0 to initialize it

Andy W

This is functionally identical. I'm going to leave this unchanged if this is just a style preference because this style matches all but one of the other empty map initializations in the repo

storage/postgres/metadata.go

pullrequest · 2021-11-10T23:50:52Z

storage/postgres/metadata.go

+	}
+
+	results := make([]core.Metadata, len(foundMetadata))
+	for id, md := range foundMetadata {


Isn't this just the same slice as foundMetadata ? Seems like you're iterating through the list an extra time for no reason.

Andy W

pullrequest · 2021-11-10T23:50:52Z

storage/postgres/metadata.go

+	}
+	c.Data = results
+
+	total, _ := s.CountMeta()


I'm a little unsure why this method in the postgres package is calling a method in the sqllite package... or is it? the naming is confusing.

Andy W

s refers to the *PGStore in this function signature. Both the sqlite and postgres storage implementations have a CountMeta function, and they each call their own implementation of CountMeta.

What's confusing about the name to you? It is a requirement of implementations of interfaces that the methods have the same name as the interface, so CountMeta is pretty sensical to me as it describes what each database object will be doing

In the PR I couldn't see that they both had a CountMeta function

Andy W

storage/postgres/metadata.go

pullrequest · 2021-11-10T23:50:52Z

storage/sqlite/aggregations.go

@@ -35,6 +35,23 @@ func (s *SQLiteStore) CountAccounts() (int64, error) {
 	return count, err
 }

+func (s *SQLiteStore) CountMeta() (int64, error) {


see note above about return values

Andy W

pullrequest

I added more details and a re-written code segment to show a more efficient way to handle the list of data.

Andy W

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

pullrequest · 2021-11-11T03:29:04Z

storage/postgres/metadata.go

+
+		md := core.Metadata{}
+
+		var (


I re-wrote this a bit to show a different way of doing it. This way you don't have to loop through all the data a second time.

foundMetadata := map[int64]core.Metadata{} results := make([]core.Metadata, len(foundMetadata)) for rows.Next() { md := core.Metadata{} rows.Scan( &md.metaID, &md.targetType, &md.targetID, &md.metaValue, ) var value json.RawMessage err = json.Unmarshal([]byte(md), &value) if err != nil { return c, err } md.metaKey = value foundMetadata[md.metaID] = md results[md.metaID] = md }

Andy W

There were some issues with this implementation:

You initialize results with against the length of an empty map, so it is an empty array. This will always cause a panic when you do results[md.metaID] = md later.

Metadata does not have any of these fields you reference when scanning the row. The type is a wrapper around a map[string]json.RawMessage, that's why they are unmarshalled into standalone vars

Although, I do agree I can avoid iterating through my map and just initialize a slice that is of length q.Limit. I initially implemented this and then decided to just remove this function from both storage implementations and the interface since I'm no longer using it to determine the last metadata id (originally I had been in order to follow the pattern set for finding the last transaction), and it seems out of scope to include in this PR without calling it in the business logic of the ledger

My apologies, my only intention was to try and illustrate a more concise method of iterating through the rows. I know it wasn't complete and I wasn't completely familiar with your Metadata type.

Andy W

henry-jackson · 2021-11-14T02:47:18Z

Removed FindMeta from this because it is no longer used it to determine the last metadata id (originally I had used it in order to follow the pattern set for finding the last transaction with FindTransaction). It seems out of scope to include in this PR without calling it in the business logic of the ledger. Some review comments and replies to those comments may now be irrelevant

pullrequest

PullRequest reviewed the updates made to #64 up until the latest commit (b9294b3). No further issues were found.

Reviewed by:

Andy W

pullrequest

I reviewed your feedback on my feedback. Much appreciated! My rewrite of the function wasn't meant to be production-grade. I had hoped only to illustrate some ways I've condensed those type of functions before.

Andy W

Was this helpful? Yes | No

Reviewers will be notified any time you reply to their comments or commit new changes.
You can also request a full follow-up review from your PullRequest dashboard.

Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com> Co-authored-by: Maxence Maireaux <maxence@maireaux.fr>

henry-jackson added 2 commits November 6, 2021 13:21

implement meta id and timestamp logic

fe80295

bugfix for multiple metadata in one post

36e52f5

github-actions bot added @domain/ledger @domain/storage labels Nov 6, 2021

matiasinsaurralde reviewed Nov 6, 2021

View reviewed changes

ledger/ledger.go Outdated Show resolved Hide resolved

simplify logic per PR suggestion

5128f82

Signed-off-by: Henry Jackson <34102861+henry-jackson@users.noreply.github.com>

pullrequest bot reviewed Nov 10, 2021

View reviewed changes

pullrequest bot reviewed Nov 11, 2021

View reviewed changes

henry-jackson added 2 commits November 13, 2021 21:25

implement PR feedback

7b9ad32

fix count err

b9294b3

pullrequest bot reviewed Nov 14, 2021

View reviewed changes

pullrequest bot reviewed Nov 15, 2021

View reviewed changes

altitude approved these changes Nov 16, 2021

View reviewed changes

henry-jackson mentioned this pull request Nov 20, 2021

Feature request: Add a logger #66

Closed

altitude changed the base branch from main to dev/meta-id-and-ts November 23, 2021 10:49

altitude merged commit 75ade71 into formancehq:dev/meta-id-and-ts Nov 23, 2021

altitude mentioned this pull request Nov 23, 2021

SaveMeta should include an ID and timestamp #63

Closed

gfyrag added a commit that referenced this pull request Feb 23, 2023

feat: Integration testing (#64)

0f0a8db

Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com> Co-authored-by: Maxence Maireaux <maxence@maireaux.fr>

flemzord added a commit that referenced this pull request Mar 27, 2023

feat: Integration testing (#64)

de228a9

Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com> Co-authored-by: Maxence Maireaux <maxence@maireaux.fr>

flemzord added a commit that referenced this pull request Apr 18, 2023

feat: Integration testing (#64)

4466ec6

Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com> Co-authored-by: Maxence Maireaux <maxence@maireaux.fr>

flemzord added a commit that referenced this pull request Sep 20, 2023

feat: Integration testing (#64)

8f9587e

Co-authored-by: Geoffrey Ragot <geoffrey.ragot@gmail.com> Co-authored-by: Maxence Maireaux <maxence@maireaux.fr>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement meta id and timestamp logic #64

implement meta id and timestamp logic #64

henry-jackson commented Nov 6, 2021

codecov bot commented Nov 6, 2021 •

edited

pullrequest bot left a comment

pullrequest bot left a comment •

edited

pullrequest bot left a comment •

edited

pullrequest bot left a comment •

edited

pullrequest bot Nov 10, 2021

henry-jackson Nov 13, 2021

altitude Nov 16, 2021

pullrequest bot Nov 16, 2021

pullrequest bot Nov 10, 2021

henry-jackson Nov 13, 2021

pullrequest bot Nov 10, 2021

henry-jackson Nov 13, 2021

pullrequest bot Nov 10, 2021

henry-jackson Nov 14, 2021

pullrequest bot Nov 10, 2021

pullrequest bot Nov 10, 2021

henry-jackson Nov 14, 2021

pullrequest bot Nov 16, 2021

pullrequest bot Nov 10, 2021

pullrequest bot left a comment

pullrequest bot Nov 11, 2021

henry-jackson Nov 14, 2021

pullrequest bot Nov 15, 2021

henry-jackson commented Nov 14, 2021

pullrequest bot left a comment

pullrequest bot left a comment


		timestamp := time.Now().Format(time.RFC3339)

		for key, value := range m {

implement meta id and timestamp logic #64

implement meta id and timestamp logic #64

Conversation

henry-jackson commented Nov 6, 2021

codecov bot commented Nov 6, 2021 • edited

Codecov Report

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment • edited

Choose a reason for hiding this comment

PullRequest Breakdown

pullrequest bot left a comment • edited

Choose a reason for hiding this comment

pullrequest bot left a comment • edited

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

henry-jackson Nov 13, 2021

Choose a reason for hiding this comment

altitude Nov 16, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 16, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

henry-jackson Nov 13, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

henry-jackson Nov 13, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

henry-jackson Nov 14, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

henry-jackson Nov 14, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 16, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 10, 2021

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Nov 11, 2021

Choose a reason for hiding this comment

henry-jackson Nov 14, 2021

Choose a reason for hiding this comment

pullrequest bot Nov 15, 2021

Choose a reason for hiding this comment

henry-jackson commented Nov 14, 2021

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 6, 2021 •

edited

pullrequest bot left a comment •

edited

pullrequest bot left a comment •

edited

pullrequest bot left a comment •

edited