-
Notifications
You must be signed in to change notification settings - Fork 348
feat: support relation and object deprecation #2465
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
feat: support relation and object deprecation #2465
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2465 +/- ##
==========================================
- Coverage 77.45% 77.39% -0.06%
==========================================
Files 417 417
Lines 51091 51394 +303
==========================================
+ Hits 39569 39769 +200
- Misses 9032 9110 +78
- Partials 2490 2515 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
842e884 to
b7e49c0
Compare
77f5d67 to
cba3045
Compare
tstirrat15
left a comment
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.
Left a couple of comments.
I'm also wondering about the behavior of these annotations more generally - what happens if you put this annotation on a permission instead of a relation? Should it have an effect?
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.
A general note on the approach: to the degree that we can, we want to avoid breaking folks' existing schemas. See what we did with expiration for an example of how we've worked around it with use directives - if we add this feature, it'll be with one of those use directives.
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.
We also need the composableschemadsl package to be updated accordingly.
pkg/schemadsl/lexer/lex_def.go
Outdated
| if l.acceptString("deprecated") { | ||
| l.emit(TokenTypeKeyword) |
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.
This isn't something that should live in the lexer - it should live in the parser.
Or at least my sense is that an @ should be a marker, and then the token after it should be a keyword, rather than the entire thing being marked as a keyword.
It's also going to relate to what I said above about the use directive.
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.
it might be worth looking at #202, which implemented generic decorators (but we didn't have a use case so never merged it)
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.
reworked the approach a little bit including the use directive just like how it was done for expiration feature, adding more comments for better review.
c9af820 to
4ad2bdc
Compare
|
I wonder if we want to allow the deprecated annotation on each subject type vs the relation as a whole; I could see benefits to either. @ecordell thoughts? |
|
we may also support something like which can include both subjects and relations, this way deprecation can be more flexible and granular also including the comments about the deprecation. Any thoughts @josephschorr @ecordell @tstirrat15 ? |
|
This is also missing changes to the schema generator package and associated tests |
bb9ec3a to
5c08a5d
Compare
Updated the generator to accommodate deprecation and added some tests. I know you all are a bit busy, so feel free to take a look whenever you get the chance, thanks! |
01cf430 to
4599651
Compare
0f6aa4c to
063ecb4
Compare
Is the |
Signed-off-by: Kartikay <[email protected]>
Signed-off-by: Kartikay <[email protected]> Please enter the commit message for your changes. Lines starting
Signed-off-by: Kartikay <[email protected]>
234989c to
5c2c076
Compare
Signed-off-by: Kartikay <[email protected]>
0455584 to
bb9f5b7
Compare
ead2bda to
0bd6021
Compare
Signed-off-by: Kartikay <[email protected]>
0bd6021 to
f436b79
Compare
|
requesting for initial review, the deprecation support now looks like This adds support for deprecating relation inside a definition namespace for marking the immediate next relation as deprecated, for outside a namespace a relation can be marked deprecated as (also would migrate this pr to a new branch if this branch looks messy after too many rebases) |
Sorry @kartikaysaxena, this is my bad at not being explicit: I liked the idea of being able to mark definitions as deprecated, but they should be done so by decorating the definition, not as a distinct line item: |
understood, this is implemented and superseded by #2504, mind taking a look? |
Related: #2396
This PR adds support for relation-level deprecation in schemas.
@deprecated(...)directive to indicate they are deprecated.--enable-experimental-relationship-deprecationalong with use directive.Example:
use deprecation definition user {} definition resource { @deprecated(warn) relation viewer: user @deprecated(error) relation admin: user }Note
Each relation has an enum
DeprecationTypeassigned to them, if unspecified it defaults toDEPRECATED_TYPE_UNSPECIFIED