Implement a typechecking function in schema#2374
Conversation
github-actions
Bot
added
the
area/tooling
barakmich
force-pushed
the
barakmich/typecheck
branch
from
620c6e6 to
8a35625
Compare
There was a problem hiding this comment.
So this raises an interesting question: the target subject types of view here are technically user and organization#member, since you can issue a check for organization#member here. Question is: do we include that in this set?
There was a problem hiding this comment.
I think we do include it and make it required to be specified
There was a problem hiding this comment.
organization#member is also a user though (or, really, the set of things that are on the other end of that relation -- and here we have computed subtypes again). I'm okay with it being specified, though
There was a problem hiding this comment.
Yeah, but its valid, so I think we should add it
barakmich
force-pushed
the
barakmich/typecheck
branch
from
8a35625 to
3c06f92
Compare
| } | ||
| if rel.TypeInformation != nil { | ||
| return ts.getTypesForInfo(ctx, defName, rel.TypeInformation, seen) | ||
| } else if rel.UsersetRewrite != nil { |
There was a problem hiding this comment.
drop the else here, since all branches return
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rel, ok := def.GetRelation(relationName) |
There was a problem hiding this comment.
newlines after if statements
There was a problem hiding this comment.
The wsl linter does this for you 😄 https://github.com/bombsimon/wsl.
We should send a separate PR adding it
| if dr.GetRelation() == ellipsesRelation { | ||
| out.Add(dr.GetNamespace()) | ||
| } else if dr.GetRelation() != "" { | ||
| rest, err := ts.getTypesForRelationInternal(ctx, dr.GetNamespace(), dr.GetRelation(), seen) |
There was a problem hiding this comment.
you could pass out to this call and then not need to create a new set every time
| `, | ||
| expected: map[string][]string{ | ||
| "organization#member": {"user"}, | ||
| "resource#viewer": {"user"}, |
There was a problem hiding this comment.
this should include organization#member; please make sure the test captures if it is missing (not sure why it is succeeding right now)
There was a problem hiding this comment.
I'm not sure I completely understand why it should.
The type of an organization#member is a user, it's just the set of them; so if I combine direct users and then organization members, it's all users in the end, and the expected type of the viewer permission is always user.
That's what I'd expect, anyway; if there were types annotated after the permission name like:
permission view: user = org->member + viewer
as opposed to
permission view: organization#member + user = org->member + viewer
There was a problem hiding this comment.
Sorry, I deleted my previous comment because it was wrong 😄
One use case for this work could be to warn people that their permission doesn't make sense because it will never ever return true. For this use case I believe that you should not include organization#member because it's not a terminal type.
E.g. if your permission is
relation aaa: user
relation bbb: organization#member
permission view = aaa & bbb
view can return true for some users (if the user is member of some organization)
However, if it was
relation aaa: user
relation bbb: apiKey
permission view = aaa & bbb
view will never return true no matter what relationships you write.
The code of CheckPermission API could have something like
if relation is intersection AND len(terminalTypes) != 1 {
return error("intersection will never lead to true, please rewrite it")
}There was a problem hiding this comment.
It necessary because it is a valid subject type for the check, and for LS. The type annotation is indicating the types of allowed subjects, so it should be present.
There was a problem hiding this comment.
But we're not gonna use this code to validate the subject types for Check and LS, right?
| require.Len(t, types, len(expected), rel) | ||
|
|
||
| for _, typ := range types { | ||
| require.Contains(t, expected, typ, fmt.Sprintf("expected %v to be in %v", typ, expected)) |
There was a problem hiding this comment.
Better to build a set of expected vs a set of actual and compare, to ensure none are missing either
barakmich
force-pushed
the
barakmich/typecheck
branch
from
3c06f92 to
b2a3dc1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2374 +/- ##
==========================================
- Coverage 76.39% 76.39% -0.00%
==========================================
Files 479 480 +1
Lines 52235 52341 +106
==========================================
+ Hits 39902 39979 +77
- Misses 9710 9733 +23
- Partials 2623 2629 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
barakmich
force-pushed
the
barakmich/typecheck
branch
2 times, most recently
from
52d478e to
11c0119
Compare
…equire graph and there's an unrelated bug discovered
barakmich
force-pushed
the
barakmich/typecheck
branch
from
4f89cd1 to
635d065
Compare
3 participants
Addressing the implementation part of #2371
Also opened a new issue stemming from the new
schema.Graphobject not covering all cases.