feat: add permission condition coverage to coverage command#2792
feat: add permission condition coverage to coverage command#2792285729101 wants to merge 1 commit intoPermify:masterfrom
Conversation
Add detailed condition-level coverage analysis that walks each permission's condition tree (union, intersection, exclusion) and reports which leaf components (relations, attributes, computed usersets) are covered by test data in each scenario. New types: ConditionComponent, ConditionCoverageInfo New field: EntityCoverageInfo.PermissionConditionCoverage New display: displayConditionCoverage() in CLI output Closes Permify#837
📝 WalkthroughWalkthroughThis change introduces comprehensive permission condition coverage analysis by adding condition-tracking types and computation logic to the coverage system, along with display functionality that renders detailed per-permission condition metrics with coverage percentages and uncovered components. Changes
Sequence DiagramsequenceDiagram
participant Run as Run()
participant Calc as calculateEntityCoverage()
participant ExtPerm as extractAssertedPermissions()
participant ExtComp as extractConditionComponents()
participant IsCov as isComponentCovered()
participant CalcCond as calculateConditionCoverage()
participant Display as displayConditionCoverage()
Run->>Calc: Process scenario with entity definition
Calc->>ExtPerm: Extract permissions from scenario checks/filters
ExtPerm-->>Calc: Return asserted permissions []string
loop For each permission
Calc->>ExtComp: Traverse condition tree into leaf components
ExtComp-->>Calc: Return ConditionComponent[]
loop For each component
Calc->>IsCov: Check if covered by test relations/attributes
IsCov-->>Calc: Return coverage status
end
end
Calc->>CalcCond: Compute coverage per permission
CalcCond-->>Calc: Return ConditionCoverageInfo map
Calc-->>Run: Update EntityCoverageInfo with PermissionConditionCoverage
Run->>Display: Invoke display with coverage data
Display-->>Run: Render condition coverage with color-coded metrics
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/development/coverage/coverage.go (1)
661-688: Usestrings.Cutinstead of a manual rune loop for splitting on'.'.The manual iteration on lines 674–679 to find the dot separator is verbose and harder to read. Go 1.18+ provides
strings.Cutwhich is idiomatic for this exact use case.♻️ Proposed refactor
+import "strings" + // ... case "tuple_to_userset": // For tuple_to_userset like "parent.admin", check that the tuple set relation ("parent") is covered - tupleSetRelation := comp.Name - // If it contains a dot, split to get the tuple set relation - for i, c := range comp.Name { - if c == '.' { - tupleSetRelation = comp.Name[:i] - break - } - } + tupleSetRelation, _, _ := strings.Cut(comp.Name, ".") return coveredRelations[tupleSetRelation]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/development/coverage/coverage.go` around lines 661 - 688, The manual rune loop in isComponentCovered to extract the tuple set relation from comp.Name is verbose; replace it with strings.Cut to split comp.Name on '.' and use the left part if the cut succeeds. Update the tuple_to_userset branch in isComponentCovered to call strings.Cut(comp.Name, ".") (importing "strings" if not present), assign the left result to tupleSetRelation when ok is true, and then return coveredRelations[tupleSetRelation].pkg/cmd/coverage.go (1)
166-202: Non-deterministic output order for scenarios and permissions due to map iteration.Both
PermissionConditionCoverage(outer map) andpermCoverages(inner map) are iterated directly, so the order of scenarios and permissions in the CLI output will vary between runs. The existing assertion coverage display (line 152) has the same pattern, so this is consistent — but if stable output is desired (e.g., for snapshot testing or diffing), consider sorting the keys before iterating.♻️ Example: sorted iteration for deterministic output
+import "sort" + func displayConditionCoverage(entityCoverageInfo cov.EntityCoverageInfo) { if len(entityCoverageInfo.PermissionConditionCoverage) == 0 { return } fmt.Printf(" permission condition coverage:\n") - for scenarioName, permCoverages := range entityCoverageInfo.PermissionConditionCoverage { + scenarioNames := make([]string, 0, len(entityCoverageInfo.PermissionConditionCoverage)) + for k := range entityCoverageInfo.PermissionConditionCoverage { + scenarioNames = append(scenarioNames, k) + } + sort.Strings(scenarioNames) + + for _, scenarioName := range scenarioNames { + permCoverages := entityCoverageInfo.PermissionConditionCoverage[scenarioName] fmt.Printf(" %s:\n", scenarioName) - for permName, condCov := range permCoverages { + permNames := make([]string, 0, len(permCoverages)) + for k := range permCoverages { + permNames = append(permNames, k) + } + sort.Strings(permNames) + + for _, permName := range permNames { + condCov := permCoverages[permName]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/coverage.go` around lines 166 - 202, The output order from displayConditionCoverage is non-deterministic because it iterates maps PermissionConditionCoverage and permCoverages directly; fix this by collecting and sorting the scenario keys from entityCoverageInfo.PermissionConditionCoverage and then, for each scenario, collecting and sorting the permission keys from permCoverages before iterating so scenarios and permissions print in a stable, deterministic order (use sort.Strings on the slices of keys and then index into the maps when printing).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/development/coverage/coverage.go`:
- Around line 580-623: leafToComponent currently maps any ComputedUserSet to
Type: "relation", causing permission references to be misclassified as
uncovered; update leafToComponent to inspect
entityDef.GetReferences()[cus.GetRelation()] when cus :=
leaf.GetComputedUserSet() is non-nil and set Type to "permission" when the
reference kind is REFERENCE_PERMISSION (otherwise keep "relation"), and then
update isComponentCovered to treat Type "permission" as covered (similar to
"call"). Locate the leafToComponent function and the ConditionComponent type
usage, change the ComputedUserSet branch to consult entityDef.GetReferences()
for cus.GetRelation(), and add handling for the new "permission" type inside
isComponentCovered so permission references are considered covered.
---
Nitpick comments:
In `@pkg/cmd/coverage.go`:
- Around line 166-202: The output order from displayConditionCoverage is
non-deterministic because it iterates maps PermissionConditionCoverage and
permCoverages directly; fix this by collecting and sorting the scenario keys
from entityCoverageInfo.PermissionConditionCoverage and then, for each scenario,
collecting and sorting the permission keys from permCoverages before iterating
so scenarios and permissions print in a stable, deterministic order (use
sort.Strings on the slices of keys and then index into the maps when printing).
In `@pkg/development/coverage/coverage.go`:
- Around line 661-688: The manual rune loop in isComponentCovered to extract the
tuple set relation from comp.Name is verbose; replace it with strings.Cut to
split comp.Name on '.' and use the left part if the cut succeeds. Update the
tuple_to_userset branch in isComponentCovered to call strings.Cut(comp.Name,
".") (importing "strings" if not present), assign the left result to
tupleSetRelation when ok is true, and then return
coveredRelations[tupleSetRelation].
| // leafToComponent converts a Leaf node to a ConditionComponent | ||
| func leafToComponent(leaf *base.Leaf) *ConditionComponent { | ||
| if cus := leaf.GetComputedUserSet(); cus != nil { | ||
| return &ConditionComponent{ | ||
| Name: cus.GetRelation(), | ||
| Type: "relation", | ||
| } | ||
| } | ||
|
|
||
| if ttus := leaf.GetTupleToUserSet(); ttus != nil { | ||
| tupleSetRelation := "" | ||
| if ttus.GetTupleSet() != nil { | ||
| tupleSetRelation = ttus.GetTupleSet().GetRelation() | ||
| } | ||
| computedRelation := "" | ||
| if ttus.GetComputed() != nil { | ||
| computedRelation = ttus.GetComputed().GetRelation() | ||
| } | ||
| name := tupleSetRelation | ||
| if computedRelation != "" { | ||
| name = tupleSetRelation + "." + computedRelation | ||
| } | ||
| return &ConditionComponent{ | ||
| Name: name, | ||
| Type: "tuple_to_userset", | ||
| } | ||
| } | ||
|
|
||
| if ca := leaf.GetComputedAttribute(); ca != nil { | ||
| return &ConditionComponent{ | ||
| Name: ca.GetName(), | ||
| Type: "attribute", | ||
| } | ||
| } | ||
|
|
||
| if call := leaf.GetCall(); call != nil { | ||
| return &ConditionComponent{ | ||
| Name: call.GetRuleName(), | ||
| Type: "call", | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that ComputedUserSet leaves can reference permissions in Permify's permission trees.
# Look at how the compiler builds permission children from the DSL.
echo "=== Check how ComputedUserSet is created in the compiler ==="
rg -n "ComputedUserSet" --type=go -C5 -g '!*_test.go' -g '!*.pb.go' | head -100
echo ""
echo "=== Check EntityDefinition References field usage ==="
rg -n "GetReferences\(\)" --type=go -C3 -g '!*.pb.go'Repository: Permify/permify
Length of output: 23362
🏁 Script executed:
# Also check the isComponentCovered function to see how it handles components
rg -n "isComponentCovered" --type=go -C10 -g '!*_test.go' -g '!*.pb.go'Repository: Permify/permify
Length of output: 3595
🏁 Script executed:
# Check the structure of leafToComponent and what calls it
rg -n "leafToComponent" --type=go -B3 -A3 -g '!*_test.go' -g '!*.pb.go'Repository: Permify/permify
Length of output: 1094
🏁 Script executed:
# Check how ComputedUserSet is built and what values GetRelation() can have
rg -n "ComputedUserSet" pkg/development/coverage/coverage.go -B5 -A5
# Look for examples of permissions being referenced in permission definitions
rg -n "permission" pkg/development/coverage/coverage.go -i | head -20
# Check the base.ComputedUserSet proto definition or usage
rg -n "type ComputedUserSet" --type=go -A10 -g '!*.pb.go'Repository: Permify/permify
Length of output: 3257
🏁 Script executed:
# Look for test cases that show ComputedUserSet referencing a permission
fd coverage_test.go | xargs grep -l "ComputedUserSet" 2>/dev/null | head -5
rg -n "ComputedUserSet" internal/schema/ -A3 -B3 | grep -i "permission" -A3 -B3Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Check the actual schema builder to understand what ComputedUserSet can reference
cat pkg/schema/builder.go | grep -A20 "func ComputedUserSet"
# Look for DSL examples with ComputedUserSet referencing permissions
fd '*.go' -x grep -l "permission.*ComputedUserSet\|ComputedUserSet.*permission" {} \; 2>/dev/nullRepository: Permify/permify
Length of output: 647
🏁 Script executed:
# Check the walker more carefully - specifically how it resolves what ComputedUserSet references
rg -n "WalkComputedUserSet\|GetRelation" internal/schema/walker.go -B3 -A3Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Look for DSL files or examples showing permission definitions
fd -e perm -o -e dsl | head -10
find . -name "*.perm" -o -name "*.dsl" | head -10
# Look for test fixtures with permission definitions
rg "permission.*=" pkg/development/coverage/ -A2 -B2 | head -40Repository: Permify/permify
Length of output: 2551
🏁 Script executed:
# Check the DSL compiler to see if it allows permissions in ComputedUserSet contexts
rg "ComputedUserSet\|permission" pkg/dsl/compiler/compiler.go -B3 -A3 | head -80Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Search for real examples in tests that might show permission references
rg "edit.*view\|permission.*permission" --type=go -C3 | head -50Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Look at the schema structure more carefully - specifically what can be referenced
rg "REFERENCE_PERMISSION\|REFERENCE_RELATION" --type=go -B2 -A2 | head -60Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Check if there are test cases in the coverage tests
fd coverage_test.go
wc -l pkg/development/coverage/coverage_test.go 2>/dev/null || echo "File not found"Repository: Permify/permify
Length of output: 146
🏁 Script executed:
# Look for permission definition examples in comments or documentation
rg "permission.*=.*permission\|permission.*=.*permission" pkg/development/coverage/ -C5Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Check for examples that show the relationship between permissions and relations
rg "GetPermissions\|GetRelations" pkg/development/coverage/coverage.go -B2 -A2Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Look at the test cases to understand permission references
cat pkg/development/coverage/coverage_test.go | head -200Repository: Permify/permify
Length of output: 6272
🏁 Script executed:
# Look specifically at examples with permission-to-permission references
rg "permission.*=.*permission\|permission.*=.*\." pkg/development/coverage/coverage_test.go -B2 -A2 | head -80Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Check how the compiler handles permission references in the condition tree
rg -n "Walk\|Permission\|ComputedUserSet" internal/schema/walker.go -C3 | head -100Repository: Permify/permify
Length of output: 41
🏁 Script executed:
# Verify if entityDef.GetReferences() can tell us if something is a permission or relation
rg "REFERENCE_PERMISSION" --type=go -B5 -A5 | head -100Repository: Permify/permify
Length of output: 6151
🏁 Script executed:
# Look for a clear example in the test where a permission references another permission in ComputedUserSet
rg "permission.*=" pkg/development/coverage/coverage_test.go | grep -v "action"Repository: Permify/permify
Length of output: 293
🏁 Script executed:
# Verify that the example "permission member = ... group.member" is actually parsed as ComputedUserSet
# Check the compiled schema structure to see what type is assigned
cat pkg/development/coverage/coverage_test.go | sed -n '150,200p'Repository: Permify/permify
Length of output: 1793
🏁 Script executed:
# Finally, verify that isComponentCovered really doesn't handle permissions
sed -n '661,690p' pkg/development/coverage/coverage.goRepository: Permify/permify
Length of output: 1288
ComputedUserSet can reference a permission, not just a relation — unconditionally labeling it as "relation" causes false "uncovered" reports.
In Permify's DSL, a ComputedUserSet leaf can refer to either a relation or another permission (e.g., permission group_member = group.member where member is a permission). The EntityDefinition.References map distinguishes these via REFERENCE_PERMISSION and REFERENCE_RELATION types. Currently, leafToComponent unconditionally assigns Type: "relation" to all ComputedUserSet leaves. Since isComponentCovered only handles "relation", "tuple_to_userset", "attribute", and "call" types, any permission reference is reported as uncovered—which is incorrect.
Consult entityDef.GetReferences()[cus.GetRelation()] to distinguish the two cases. For permission-referencing leaves, assign a distinct type (e.g., "permission") and handle it in isComponentCovered as always covered (analogous to "call"), since the referenced permission gets its own separate coverage entry.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/development/coverage/coverage.go` around lines 580 - 623, leafToComponent
currently maps any ComputedUserSet to Type: "relation", causing permission
references to be misclassified as uncovered; update leafToComponent to inspect
entityDef.GetReferences()[cus.GetRelation()] when cus :=
leaf.GetComputedUserSet() is non-nil and set Type to "permission" when the
reference kind is REFERENCE_PERMISSION (otherwise keep "relation"), and then
update isComponentCovered to treat Type "permission" as covered (similar to
"call"). Locate the leafToComponent function and the ConditionComponent type
usage, change the ComputedUserSet branch to consult entityDef.GetReferences()
for cus.GetRelation(), and add handling for the new "permission" type inside
isComponentCovered so permission references are considered covered.
285729101
force-pushed
the
feat/coverage-command-enhancement
branch
from
6f2cdd0 to
a6d74ee
Compare
|
@tolgaozen @EgeAytin adds permission condition coverage to the coverage command for more thorough testing. |
1 participant
Summary
coveragecommandConditionComponent,ConditionCoverageInfofor tracking individual condition componentsPermissionConditionCoveragefield toEntityCoverageInfomapping scenario → permission → condition coverageChanges
pkg/development/coverage/coverage.go: New condition coverage types andcalculateConditionCoverage()function that extracts leaf components from permission condition trees and checks coverage against test datapkg/cmd/coverage.go: NewdisplayConditionCoverage()function for CLI output with color-coded percentagesTest plan
go build ./...to verify compilation/claim #837
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes