deduplicate edges when conditions exist and add conditions to the edges #422
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
elbuo8
reviewed
Feb 20, 2025
elbuo8
reviewed
Feb 20, 2025
elbuo8
reviewed
Feb 20, 2025
elbuo8
reviewed
Feb 20, 2025
elbuo8
reviewed
Feb 20, 2025
elbuo8
reviewed
Feb 20, 2025
Co-authored-by: Yamil Asusta <[email protected]>
Co-authored-by: Yamil Asusta <[email protected]>
Co-authored-by: Yamil Asusta <[email protected]>
elbuo8
approved these changes
Feb 20, 2025
adriantam
reviewed
Feb 21, 2025
| @@ -83,7 +83,8 @@ func (g *AuthorizationModelGraph) Reversed() (*AuthorizationModelGraph, error) { | |||
| if !ok { | |||
| return nil, fmt.Errorf("%w: could not cast to AuthorizationModelEdge", ErrBuildingGraph) | |||
| } | |||
| graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.conditionedOn) | |||
| newEdge := graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.tuplesetRelation, "") | |||
| newEdge.conditions = casted.conditions | |||
There was a problem hiding this comment.
why not graphBuilder.AddEdge(nextLine.To(), nextLine.From(), casted.edgeType, casted.tuplesetRelation, casted.conditions) ?
| if graphBuilder.hasEdge(nodeSource, parentNode, TTUEdge, typeTuplesetRelation) { | ||
| // de-dup types that are conditioned, e.g. if define viewer: [user, user with condX] | ||
| // we only draw one edge from user to x#viewer | ||
| continue |
There was a problem hiding this comment.
Do we need to call upsert? Otherwise, will be lose the condition?
| type permission | ||
| relations | ||
| define assignee: [role#assignee, role#assignee with condX] | ||
| define member: [user, permission#member, permission#member with condX] |
There was a problem hiding this comment.
will be easier to read if they line up :)
| require.Len(t, edges, 1) | ||
| conditions := edges[0].conditions | ||
| require.Empty(t, edges[0].tuplesetRelation) | ||
| require.Len(t, conditions, 2) |
There was a problem hiding this comment.
should we assert on the conditions itself? (i.e., check the individual array item)
yissellokta
added a commit
that referenced
this pull request
Feb 21, 2025
4 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 24, 2025
* feedback from github.com//pull/422 * Update pkg/go/graph/graph_builder.go Co-authored-by: Adrian Tam <[email protected]> --------- Co-authored-by: Adrian Tam <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Inconsistent behavior in the parser was deduplicating the edges if directly related to the terminal type but not in the case of a userset, causing the algorithms in the type system to not identify models that should have fast path
Description
The parser was deduplicating cases like the one below, where it only constructs one edge from group#member to user:
type group
define member: [user, user with condX]
In the case of a userset with conditions it was not behaving in the same way, and it was creating two edges from group#member to folder#viewer
type group
define member: [folder#viewer, folder#viewer with condX]
type folder
define viewer: [user]
This PR covers the following changes:
1- Add the deduplication for the userset case
2- rename conditionOn on the edge to have a name that represents its value, tupleUserset, in a ttu (X from Y), it is the Y
3- add conditions to the edge, to add more info required to substitute in the future the type system and use the graph
4- Add tests that cover all these changes
References
Review Checklist
main