feat: Add ACP to pubsub KMS #3206
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3206 +/- ##
===========================================
+ Coverage 77.44% 77.46% +0.01%
===========================================
Files 376 378 +2
Lines 34791 34941 +150
===========================================
+ Hits 26943 27064 +121
- Misses 6238 6257 +19
- Partials 1610 1620 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 12 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
@islamaliev should I wait for you to fix the failing tests, or is it okay to review? |
| FieldID: core.COMPOSITE_NAMESPACE, | ||
| } | ||
| headset := clock.NewHeadSet(headstore, headStoreKey) | ||
| cids, _, err := headset.List(ctx) |
There was a problem hiding this comment.
todo: There appears to be no reason to fully scan all the head cids in the constructor, only to return an iterator from it that then iterates through the results. It seems to defeat the point a little bit and makes the code look odd, and more scary than it needs to be.
This is worse given that in the normal/happy use-case (RetrieveCollectionFromDocID, not retry), *DocHeadBlocksIterator.Next is only called once, and so only one of the potentially many fetched cids are actually used.
Please rework this (maybe make HeadSet an iterator) to avoid the extra fetching.
Note: Please see my note RE a bug in the model before spending time on this.
There was a problem hiding this comment.
Next is only called once
It uses all produced value in db::db.publishDocUpdateEvent and db::db.retryDoc.
Only in db::CollectionRetriever.RetrieveCollectionFromDocID it uses just the first produces value.
There was a problem hiding this comment.
Ah, I missed publishDocUpdateEvent thanks - that is still half of the happy-path paths though. And doesn't change the unusualness of doing extra/discarded work in the constructor of a type that is already an iterator.
I'm not sure we'll convince each other either way here though, and it's only immediate impact on users is (probably minor) performance, so if you don't want to do the suggestion please leave this thread unresolved in case anyone else feels like joining the conversation :)
islamaliev
force-pushed
the
feat/ecn-key-exchange-with-acp
branch
from
e21590c
to
21269c1
Compare
islamaliev
force-pushed
the
feat/ecn-key-exchange-with-acp
branch
from
057377f
to
5c03868
Compare
| @@ -227,6 +227,32 @@ func (db *db) AddPolicy( | |||
| return client.AddPolicyResult{PolicyID: policyID}, nil | |||
| } | |||
|
|
|||
| func (db *db) publishDocUpdateEvent(ctx context.Context, docID string, collection client.Collection) error { | |||
There was a problem hiding this comment.
todo: This function is scoped to a large and important type, does very similar things to a couple of existing places, and ignores the context txn. It looks very easy to erroneously call this, and in an area where testing is relatively weak (transactions).
Please document this function to highlight that it ignores the transaction, and does a whole load of work to find the heads (something the two existing locations do not need to do).
Please do not refactor the existing locations as I have changed them in another open PR, with this 3rd addition we can look at refactoring the 3 after this (and maybe the other PR, depending on timing) has merged.
There was a problem hiding this comment.
LGTM Thanks very much for this change and explaining why that line was being hit.
Just one suggestion remaining incase you missed.
suggestion: Add a test where after sharing a document, it is revoked and then shared again. i.e. using DeleteDocActorRelationship
I don't think it makes much sense, as this would mostly test acp functionality which we already have plenty tests for. Revoking of encryption keys is planned for later anyway. This is where we will add a bunch of tests. |
Relevant issue(s)
Resolves #2893
This change adds ACP to pubsub KMS.
A new event
MergeCompletewas introduced in order to make it correspond toMergeCompleteName. This is necessary to notify listeners that the merge was executed on decrypted blocks.Upon granting access to a document via
AddDocActorRelationshipwe publish notUpdateevent so that the actors that being given access to can now request a document anew.In testing framework
WaitForSyncis extended to allow specifying documents that should be received in decrypted state.