perf: in Check, if source and target types aren't connected, return allowed=false
#1582
Conversation
miparnisari
changed the title
miparnisari
force-pushed
the
check-pruning
branch
from
4defa75
to
d6c934a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1582 +/- ##
==========================================
+ Coverage 86.20% 86.46% +0.26%
==========================================
Files 87 88 +1
Lines 8183 8406 +223
==========================================
+ Hits 7053 7267 +214
- Misses 797 801 +4
- Partials 333 338 +5 ☔ View full report in Codecov by Sentry. |
miparnisari
force-pushed
the
check-pruning
branch
from
d6c934a
to
13e972b
Compare
internal/graph/check.go
Outdated
| @@ -615,6 +629,28 @@ func (c *LocalChecker) ResolveCheck( | |||
| return resp, nil | |||
| } | |||
|
|
|||
| // areTypesConnected returns true if it is possible to reach objectType#relation starting from userType[#userRelation]. | |||
| func (c *LocalChecker) areTypesConnected(typesys *typesystem.TypeSystem, user, userType, userRelation, objectType, relation string) (bool, error) { | |||
| g := New(typesys) | |||
internal/graph/check.go
Outdated
| sourceRel = typesystem.DirectRelationReference(userType, userRelation) | ||
| } | ||
|
|
||
| edges, err := g.GetRelationshipEdges(typesystem.DirectRelationReference(objectType, relation), sourceRel) |
There was a problem hiding this comment.
Perhaps the fast follow is improving the typesystem package, particularly as this object lives in memory for a long period of time, calculating this multiple times would be waste of compute that could be incrementally cached lazily.
There was a problem hiding this comment.
Yes, we should definitely calculate the edges up front once and not in every sub-problem resolution... however, that wasn't a trivial change so I didn't do it
There was a problem hiding this comment.
This needs to happen before we merge this PR. Graph construction from the typesystem in critical Check resolution code is a "no go" right now IMO.
Let's fix up the typesystem, do better lazy compute of the type graph, and then let's merge this work in.
There was a problem hiding this comment.
@elbuo8 @jon-whit I took a stab at computing the graph once (mostly copied over from https://github.com/jon-whit/openfga-graphviz-gen/) and then using BFS to get the connectivity. I didn't reuse the getRelationshipEdges code because that was harder for me to adapt than writing this code. I think this code is more intuitive because you can visualize it easily. Let me know what you think! We can also extend it to compute cycles way faster than today (using https://github.com/jon-whit/openfga-graphviz-gen/blob/main/writer.go#L140)
Note that this PR adds more CPU and memory consumption during model validation. But again, this can be mitigated by changing the cycle detection code which is currently very inefficient as a fast follow-up.
miparnisari
changed the title
allowed=false
Description
Adding a performance optimization for queries of the form
Check(user, relation, object). Ifusertype andobjecttypes are disconnected/unreachable, we return early withallowed=false.I achieve this by first building the graph of the model using most of the code from https://github.com/jon-whit/openfga-graphviz-gen.
Testing
With this model
And this tuple:
doc:budget # parent @ folder:parentBefore
After