Add DELETE method for data API #1132
Conversation
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
nguyer
requested review from
peterbroadhurst,
awrichar,
shorsher and
nickgaski
as code owners
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Codecov Report
@@ Coverage Diff @@
## main #1132 +/- ##
===========================================
+ Coverage 99.62% 100.00% +0.37%
===========================================
Files 303 305 +2
Lines 19795 19963 +168
===========================================
+ Hits 19721 19963 +242
+ Misses 74 0 -74
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
| DROP TABLE blobs; | ||
| ALTER TABLE temp_blobs RENAME TO blobs; | ||
| CREATE INDEX blobs_hash_data_id ON blobs(hash, data_id); | ||
| CREATE INDEX blobs_payload_ref ON blobs(payload_ref); |
There was a problem hiding this comment.
Note for self: Confirm why we need this reverse lookup index
There was a problem hiding this comment.
ok - this is for a compatibility check during deletion, that we do not delete a payload ref in DX if it's referred to by another data ID.
| DROP INDEX blobs_hash; | ||
| DROP TABLE blobs; | ||
| ALTER TABLE temp_blobs RENAME TO blobs; | ||
| CREATE INDEX blobs_hash_data_id ON blobs(hash, data_id); |
There was a problem hiding this comment.
Should this just be namespace, data_id? Not sure the hash adds value, and particularly not first as data_id is the thing we expect to be unique in most practical cases.
There was a problem hiding this comment.
I thought we had discussed this and decided that we should always be looking up blobs by both their hash and data ID to prevent spoofing of different data?
There was a problem hiding this comment.
Yes, but I don't think that changes my suggestion on the index that's most efficient.
I believe the lookup is:
{ ns: "mynamespace", data_id: "id12345", hash: "abce1234" }
I assert the most efficient index to support this lookup is:
namespace, data_id
- Leaving off
namespacefeels wrong - Adding in a 3rd dimension of
hashseems unnecessary (as the scan length should be trivially small as we're covering a very minor edge case there)
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
internal/data/data_manager.go
Outdated
| } | ||
| if data.Blob != nil && data.Blob.Hash != nil { | ||
|
|
||
| blob, err := dm.database.GetBlob(ctx, dm.namespace.Name, data.ID, data.Blob.Hash) |
There was a problem hiding this comment.
I believe we determined there could be multiple blobs at this point @nguyer
e.g. I was expecting to see:
blobs, err := dm.database.GetBlobs(ctx, db.namespace.Name, fb.And(fb.Eq("data_id",data.ID), fb.Eq(data.Blob.Hash))I'm not sure it even makes sense to have a GetBlob database-level action, because the only unique key for a record is the Sequence key (which we correctly use in delete).
There was a problem hiding this comment.
The specific case I'm thinking of, is when you receive the same data, with the same ID/Hash from multiple peers in your business network, on different privacy groups, in multiple different messages.
There was a problem hiding this comment.
Thanks for working that all through 👍
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Depends on: hyperledger/firefly-dataexchange-https#73