Event and Records Subscriptions #607
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 98.83% 98.54% -0.29%
==========================================
Files 71 78 +7
Lines 9255 10253 +998
Branches 1391 1506 +115
==========================================
+ Hits 9147 10104 +957
- Misses 100 140 +40
- Partials 8 9 +1 ☔ View full report in Codecov by Sentry. |
LiranCohen
force-pushed
the
lirancohen/subscriptions
branch
from
61dac9b to
5d3671c
Compare
LiranCohen
force-pushed
the
lirancohen/subscriptions
branch
3 times, most recently
from
7df607b to
eab1545
Compare
LiranCohen
force-pushed
the
lirancohen/subscriptions
branch
from
abb43b3 to
4da1a3c
Compare
LiranCohen
requested review from
mistermoe,
csuwildcat and
thehenrytsai
as code owners
Co-authored-by: Liran Cohen <[email protected]> Co-authored-by: Andor Kesselman <[email protected]> Date: Thu Dec 21 12:34:36 2023 -0500
…et becomes a thing
LiranCohen
force-pushed
the
lirancohen/subscriptions
branch
from
2b23061 to
f49e516
Compare
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": [ | ||
| "descriptor" |
There was a problem hiding this comment.
Sanity: authorization is required also no?
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": [ | ||
| "descriptor" |
There was a problem hiding this comment.
Sanity: authorization is required also no?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Working off of the work @andorsk put in for Subscriptions, #508.
I re-worked it to match our current scopes and the way we handle permissions.
This PR adds 2 new interfaces
EventsSubscribeandRecordsSubscribe.EventsSubscribewill follow the current authorization forEventsGetand the one that is coming forEventsQuery, that is only tenant is currently authorized for it, without any grants/delegation. This will come in a separate PR.RecordsSubscribefollows the authorization/delegation rules fromRecordsQueryat the moment, and builds up match filters for theEventStreamin the same way that it builds up filters for theMessageStorewhen retrieving messages.There are still many missing tests, I am considering moving some of the tests from
RecordsQueryinto scenario testing where we can test both Query and Subscribe together.Some things to note:
Current approach for re-authorization are TTL based to re-authorize the original subscription message, in a future PR we could purge the subscription upon receipt of a
PermissionsRevokeor aRecordsDeleteassociated with a protocol role. I am open to ripping out re-authorization completely from this PR in favor of doing that approach in a subsequent PR.Also I overloaded the the
subscribemethod on the interface in order to be able to have more defined behaviors and objects around the individual types of subscriptions. I'm personally 50/50 on keeping this abstraction as they are very similar. The only difference currently is that theRecordsSubscribehandler will type the messages it produces as eitherRecordsWriteMessageorRecordsDeleteMessagewhere as theEventsSubscribewill emit aGenericMessage.