-
Notifications
You must be signed in to change notification settings - Fork 366
feat: each iterator can report its types on either end now #2849
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
Conversation
b2f95da to
956504b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2849 +/- ##
==========================================
+ Coverage 74.43% 74.43% +0.01%
==========================================
Files 484 484
Lines 57444 57629 +185
==========================================
+ Hits 42750 42889 +139
- Misses 11701 11736 +35
- Partials 2993 3004 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
See comments, otherwise LGTM
pkg/query/intersection.go
Outdated
| // All subiterators should have the same resource type | ||
| return i.subIts[0].ResourceType() |
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.
Is this enforced somewhere, implicitly or explicitly?
pkg/query/types.go
Outdated
| return []ObjectType{} | ||
| } | ||
|
|
||
| seen := make(map[string]bool) |
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.
Could also use mapz.Set here as well
6acb9e2 to
8394a1f
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.
See comments
| // Validate that all subiterators have the same resource type | ||
| for idx, subIt := range i.subIts[1:] { | ||
| subType, err := subIt.ResourceType() | ||
| if err != nil { | ||
| return ObjectType{}, err | ||
| } | ||
| if firstType != subType { | ||
| return ObjectType{}, spiceerrors.MustBugf("intersection resource type mismatch: subiterator 0 has type %s, subiterator %d has type %s", | ||
| firstType.String(), idx+1, subType.String()) | ||
| } | ||
| } |
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.
Is this potentially expensive?
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.
Not terribly, no.
8394a1f to
6a11b47
Compare
| } | ||
|
|
||
| func (a *Arrow) ResourceType() (ObjectType, error) { | ||
| // Arrow's resources come from the left side |
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.
i don't yet understand the reasoning behind this (why left and not right?) i suppose it will become apparent in a follow up PR where we use these functions
but in any case i'd move this to be a proper godoc of the function
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.
definition user {}
definition group {
relation member: user
}
definition document {
relation group: group
permission view = group->member
}
for view, then, the document is clearly the type of the resource -- it's also the type of the resource for the group relation (so this is the left side). Then the right side is the right side of member, which is user
pkg/query/self.go
Outdated
| var _ Iterator = &Self{} | ||
|
|
||
| func NewSelf(relation string) *Self { | ||
| func NewSelf(relation string, resourceType ObjectType) *Self { |
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.
i can't find a caller of this function where relation != resourceType.subrelation. Do you need that first parameter at all?
c734e0b to
5894c5e
Compare
5894c5e to
401e9a2
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.
![]()
Description
Testing
References