-
Notifications
You must be signed in to change notification settings - Fork 290
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
introduce caveat support in WriteRelationships/ReadRelationships #838
Merged
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
github-actions
bot
added
area/api v1
Affects the v1 API
area/dependencies
Affects dependencies
area/tooling
Affects the dev or user toolchain (e.g. tests, ci, build tools)
labels
Sep 22, 2022
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
4 times, most recently
from
September 27, 2022 16:19
008963e
to
a18066f
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
2 times, most recently
from
September 27, 2022 16:42
5084df5
to
40f76bd
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
5 times, most recently
from
September 27, 2022 17:58
cd0a252
to
d8f32f0
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 27, 2022 18:11
d8f32f0
to
5a5f1d3
Compare
vroldanbet
changed the title
introduce caveat support in WriteRelationships
introduce caveat support in WriteRelationships/ReadRelationships
Sep 28, 2022
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 28, 2022 19:34
5a5f1d3
to
b35cd00
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 28, 2022 19:35
b35cd00
to
7cfd73c
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
2 times, most recently
from
September 29, 2022 16:16
9b06a37
to
c3ce2f3
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 29, 2022 16:29
c3ce2f3
to
679697f
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 29, 2022 16:33
679697f
to
4bea32c
Compare
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 29, 2022 16:47
4bea32c
to
fdf5d93
Compare
this commit changes parts of the datastore implementation. The duality caveatID/CaveatName was problematic to handle between the controller layer and the datastore layer: The usage of ID introduced a situation where a core.RelationTuple cannot be converted to a v1.Relationship without a database lookup, because database tuples reference caveats via ID, but we return named caveats to the client via v1.Relationship. It would be a departure from what happens today in service controllers: v1-to-core would introduce I/O operations. The question here is that it's not clear the usage of ID buys us anything at this point. The rationale behind them was being able to support "anonymous caveats". An anonymous caveat signature defined in place in a relation in the schema. Because they don't have a name, we need a way to identify them. However, it's not clear anonymous caveats is needed at this point, and that we should work on it when the need arises.
vroldanbet
force-pushed
the
add-caveat-to-write-relationships
branch
from
September 29, 2022 16:55
fdf5d93
to
3356329
Compare
josephschorr
approved these changes
Sep 29, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Labels
area/api v1
Affects the v1 API
area/caveats
Affects caveated relationships
area/datastore
Affects the storage system
area/dependencies
Affects dependencies
area/tooling
Affects the dev or user toolchain (e.g. tests, ci, build tools)
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.
Tracking: #386
Closes #836
Depends on authzed/api#38
Depends on authzed/authzed-go#71
What
Introduces support for writing caveated relationships via
WriteRelationships
APIHow
The duality
caveat_id
/caveat_name
was problematic to handle between the controller layer and the datastore layer, so it was adjusted.The usage of ID introduced a situation where a
core.RelationTuple
cannot be converted to av1.Relationship
without a database lookup, because database tuples reference caveats via ID, but we return named caveats to the client viav1.Relationship
. It would be a departure from what happens today in service controllers: v1-to-core would introduce I/O operations.☝🏻 is certainly not a deal breaker we couldn't addres, but the question here is that it's not clear the usage of ID buys us anything at this point. The rationale behind them was being able to support "anonymous caveats". An anonymous caveat signature defined in place in a relation in the schema. Because they don't have a name, we need a way to identify them.
However, it's not clear anonymous caveats is needed at this point, and that we should work on it when the need arises.
Note: because caveat is not yet exposed via
Schema
API, I resourced to writing it directly via the datastore, using type assertions.Note on failing buf build
I intentionally broke
core
proto definition. These haven't been yet released and are not being used so we can ignore that error: