feat: integrate scenario-specific relationships and attributes in schema files#2791
Open
285729101 wants to merge 4 commits intoPermify:masterfrom
Open
feat: integrate scenario-specific relationships and attributes in schema files#2791285729101 wants to merge 4 commits intoPermify:masterfrom
285729101 wants to merge 4 commits intoPermify:masterfrom
Conversation
Allow scenarios to define their own relationships and attributes in addition to global definitions, enabling more focused and contextual testing. This addresses the need for dual flexibility: maintaining the efficiency of global definitions while providing scenario-specific configurations for complex test cases. Changes: - Add Relationships and Attributes fields to Scenario struct - Process scenario-specific data before running checks in development.go - Support scenario-specific data in CLI validate command - Include scenario-specific data in coverage calculations - Update example YAML schemas to demonstrate the new capability - Add test cases for scenario-specific relationships and attributes Closes Permify#838
…pecific data Move task relationships to scenario-specific in custom-roles.yaml and add a scenario-specific attribute example in banking-system.yaml.
Update calculateEntityCoverage to aggregate global and scenario-specific relationships and attributes for coverage analysis. Also pass merged data to condition-level coverage calculations.
📝 WalkthroughWalkthroughThis pull request introduces per-scenario relationships and attributes support across the validation and development frameworks. Changes include adding new fields to the Scenario struct, processing scenario-specific relationships and attributes during validation and development workflows, extending coverage tracking with condition-based metrics, and updating example YAML files to demonstrate the new functionality. Changes
Sequence DiagramsequenceDiagram
participant Shape as Shape File
participant Validator as Scenario Validator
participant Processor as Relationship/Attribute Processor
participant DB as Database
participant Coverage as Coverage Tracker
Shape->>Validator: Load scenario with relationships & attributes
Validator->>Processor: For each scenario, process relationships
Processor->>Processor: Convert to Tuple
Processor->>Processor: Validate against schema
Processor->>DB: Write tuple
Processor->>Validator: Log success/failure
Validator->>Processor: For each scenario, process attributes
Processor->>Processor: Convert to Attribute
Processor->>Processor: Validate against schema
Processor->>DB: Write attribute
Processor->>Validator: Log success/failure
Validator->>Validator: Execute checks on scenario data
Validator->>Coverage: Report covered relationships & attributes
Coverage->>Coverage: Calculate condition coverage percentage
Coverage->>Coverage: Track per-scenario coverage metrics
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (4)
assets/example-shapes/organizations-hierarchies.yaml (1)
49-58: Scenario logic is correct; minor style inconsistency with relationship quoting.The permissions logic checks out —
user:9999asownerofrepository:1234correctly grants botheditanddelete.Nit: the scenario-specific relationship on line 52 uses a quoted string (
"repository:1234#owner@user:9999"), while the global relationships on lines 26–27 are unquoted. Consider keeping them consistent for readability.relationships: - - "repository:1234#owner@user:9999" + - repository:1234#owner@user:9999🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/example-shapes/organizations-hierarchies.yaml` around lines 49 - 58, The relationship in the scenario "owner_access_test" uses a quoted string "repository:1234#owner@user:9999" which is inconsistent with the unquoted global relationships; update the relationships entry for owner_access_test to use the unquoted form repository:1234#owner@user:9999 (or alternatively quote all relationship entries) so the formatting matches the global relationships while preserving the same relation and subject.assets/example-shapes/custom-roles.yaml (1)
29-29: Emptyattributeskey is acceptable but consider omitting or adding a comment.The bare
attributes:resolves tonullin YAML. This works fine if the deserializer treatsnullas an empty list, but for an example file it may confuse users. Consider either removing it (if optional) or adding a short comment like# no global attributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/example-shapes/custom-roles.yaml` at line 29, The bare attributes: key in the YAML example produces a null value and can confuse readers — either remove the empty attributes: entry entirely if global attributes are optional, or replace it with a short inline comment (e.g., "# no global attributes") so the intent is clear; update the assets/example-shapes/custom-roles.yaml by locating the attributes: key and applying one of these two fixes.pkg/cmd/validate.go (1)
239-270: Same code duplication concern asdevelopment.go— extract shared helpers.The parse → validate → write pattern for relationships and attributes is repeated here as well. A shared helper would eliminate duplication across both
validate.goanddevelopment.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/validate.go` around lines 239 - 270, Extract the repeated parse→validate→write logic into a shared helper (e.g., processTuple or processEntityTuple) and call it from both validate.go and development.go: the helper should accept context, the tuple string (t), version, references to dev.Container.SR and dev.Container.DW, serverValidation, and the aggregated error list/color logger; inside it perform tuple.Tuple(t), call ReadEntityDefinition (dev.Container.SR.ReadEntityDefinition) to get definition, run serverValidation.ValidateTuple(definition, tup), attempt dev.Container.DW.Write(...) and return any formatted error or success message so the caller can add to list and print — replace the inlined blocks in the for-loops with a call to this helper to remove duplication while preserving identical error formatting and logging behavior.pkg/development/development.go (1)
286-326: Consider extracting shared relationship/attribute processing logic to reduce duplication.The parse → read entity definition → validate → write → collect error pattern is repeated four times: global relationships (lines 190–234), global attributes (lines 237–281), scenario relationships (lines 286–326), and scenario attributes (lines 329–369). Extracting into helpers like
processRelationships(ctx, rels, version, errType, errKey)andprocessAttributes(...)would reduce ~160 lines of near-identical code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/development/development.go` around lines 286 - 326, Duplicate parse→readDefinition→validate→write error-handling logic for relationships/attributes should be extracted into reusable helpers; create helper functions (e.g., processRelationships(ctx, rels []string, version string, errType string, errKey int) and processAttributes(...)) that perform tuple.Tuple(), c.Container.SR.ReadEntityDefinition(ctx, "t1", tup.GetEntity().GetType(), version), validation.ValidateTuple/validation.ValidateAttribute, and c.Container.DW.Write(ctx, "t1", database.NewTupleCollection(...) / database.NewAttributeCollection()), and on error append the same Error struct to the shared errors slice; replace the four duplicated blocks (global relationships/attributes and scenario relationships/attributes that iterate over scenario.Relationships/scenario.Attributes) with calls to these helpers passing the proper rel/attr slice, version, "scenarios"/"globals" errType and the index i or nil key as appropriate.
🤖 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 168-169: The append call using append(shape.Relationships,
scenario.Relationships...) can reuse shape.Relationships' backing array and
corrupt shared data; replace it with the copy-then-append pattern used
elsewhere: allocate a new slice sized to
len(shape.Relationships)+len(scenario.Relationships), copy(shape.Relationships,
...) into it, then copy or append scenario.Relationships into the new slice and
assign that to scenarioRelationships (do the same for scenarioAttributes with
shape.Attributes and scenario.Attributes). This ensures no reuse of the original
backing arrays when computing scenarioRelationships and scenarioAttributes in
calculateConditionCoverage.
In `@pkg/development/development.go`:
- Around line 285-369: The scenario loop is writing relationships/attributes
into the shared data writer (c.Container.DW) so scenario state accumulates
across iterations; to make scenarios isolated, reset or replace the data store
before each scenario: inside RunWithShape (the loop handling
scenario.Relationships and scenario.Attributes) either call the data-store
reset/truncate API on c.Container.DW (e.g., DW.Clear/Truncate/Reset if
available) or construct a fresh in-memory writer instance for each scenario and
use that for c.Container.DW.Write; update any tests or callers to use the
per-scenario DW and ensure validation
(validation.ValidateTuple/ValidateAttribute) still reads the correct entity
definitions, or if cumulative behavior is intended, update the function/method
documentation and naming to explicitly state that scenarios incrementally build
on prior data.
---
Duplicate comments:
In `@pkg/cmd/validate.go`:
- Around line 236-308: Scenario-specific relationships/attributes are being
written to the persistent DB under the hard-coded tenant "t1" via
dev.Container.DW.Write (using database.NewTupleCollection/
NewAttributeCollection) which causes them to persist across subsequent
scenarios; change the code to isolate scenario writes by using a
temporary/unique tenant or scoped transaction per scenario (e.g., derive a
scenario-specific tenant id from scenario name or open a DW transaction and
rollback/cleanup after the scenario) instead of writing to "t1", and ensure
cleanup of any temporary writes; also standardize error behavior or add a brief
comment in the loops handling scenario.Relationships and scenario.Attributes
explaining why those errors are soft-fail (list.Add/continue) if you intend to
keep that behavior.
---
Nitpick comments:
In `@assets/example-shapes/custom-roles.yaml`:
- Line 29: The bare attributes: key in the YAML example produces a null value
and can confuse readers — either remove the empty attributes: entry entirely if
global attributes are optional, or replace it with a short inline comment (e.g.,
"# no global attributes") so the intent is clear; update the
assets/example-shapes/custom-roles.yaml by locating the attributes: key and
applying one of these two fixes.
In `@assets/example-shapes/organizations-hierarchies.yaml`:
- Around line 49-58: The relationship in the scenario "owner_access_test" uses a
quoted string "repository:1234#owner@user:9999" which is inconsistent with the
unquoted global relationships; update the relationships entry for
owner_access_test to use the unquoted form repository:1234#owner@user:9999 (or
alternatively quote all relationship entries) so the formatting matches the
global relationships while preserving the same relation and subject.
In `@pkg/cmd/validate.go`:
- Around line 239-270: Extract the repeated parse→validate→write logic into a
shared helper (e.g., processTuple or processEntityTuple) and call it from both
validate.go and development.go: the helper should accept context, the tuple
string (t), version, references to dev.Container.SR and dev.Container.DW,
serverValidation, and the aggregated error list/color logger; inside it perform
tuple.Tuple(t), call ReadEntityDefinition
(dev.Container.SR.ReadEntityDefinition) to get definition, run
serverValidation.ValidateTuple(definition, tup), attempt
dev.Container.DW.Write(...) and return any formatted error or success message so
the caller can add to list and print — replace the inlined blocks in the
for-loops with a call to this helper to remove duplication while preserving
identical error formatting and logging behavior.
In `@pkg/development/development.go`:
- Around line 286-326: Duplicate parse→readDefinition→validate→write
error-handling logic for relationships/attributes should be extracted into
reusable helpers; create helper functions (e.g., processRelationships(ctx, rels
[]string, version string, errType string, errKey int) and
processAttributes(...)) that perform tuple.Tuple(),
c.Container.SR.ReadEntityDefinition(ctx, "t1", tup.GetEntity().GetType(),
version), validation.ValidateTuple/validation.ValidateAttribute, and
c.Container.DW.Write(ctx, "t1", database.NewTupleCollection(...) /
database.NewAttributeCollection()), and on error append the same Error struct to
the shared errors slice; replace the four duplicated blocks (global
relationships/attributes and scenario relationships/attributes that iterate over
scenario.Relationships/scenario.Attributes) with calls to these helpers passing
the proper rel/attr slice, version, "scenarios"/"globals" errType and the index
i or nil key as appropriate.
Comment on lines
+168
to
+169
| scenarioRelationships := append(shape.Relationships, scenario.Relationships...) | ||
| scenarioAttributes := append(shape.Attributes, scenario.Attributes...) |
There was a problem hiding this comment.
Unsafe use of append on shared slice — may silently corrupt data across loop iterations.
append(shape.Relationships, scenario.Relationships...) reuses the backing array of shape.Relationships if it has spare capacity, meaning a later iteration can overwrite data written by an earlier one. While the result is currently consumed before the next iteration, this is a well-known Go footgun that will silently break if calculateConditionCoverage ever stores or returns the slice.
Use the same copy-then-append pattern already used on lines 142–146.
Proposed fix
- scenarioRelationships := append(shape.Relationships, scenario.Relationships...)
- scenarioAttributes := append(shape.Attributes, scenario.Attributes...)
+ scenarioRelationships := make([]string, len(shape.Relationships), len(shape.Relationships)+len(scenario.Relationships))
+ copy(scenarioRelationships, shape.Relationships)
+ scenarioRelationships = append(scenarioRelationships, scenario.Relationships...)
+
+ scenarioAttributes := make([]string, len(shape.Attributes), len(shape.Attributes)+len(scenario.Attributes))
+ copy(scenarioAttributes, shape.Attributes)
+ scenarioAttributes = append(scenarioAttributes, scenario.Attributes...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scenarioRelationships := append(shape.Relationships, scenario.Relationships...) | |
| scenarioAttributes := append(shape.Attributes, scenario.Attributes...) | |
| scenarioRelationships := make([]string, len(shape.Relationships), len(shape.Relationships)+len(scenario.Relationships)) | |
| copy(scenarioRelationships, shape.Relationships) | |
| scenarioRelationships = append(scenarioRelationships, scenario.Relationships...) | |
| scenarioAttributes := make([]string, len(shape.Attributes), len(shape.Attributes)+len(scenario.Attributes)) | |
| copy(scenarioAttributes, shape.Attributes) | |
| scenarioAttributes = append(scenarioAttributes, scenario.Attributes...) |
🤖 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 168 - 169, The append call
using append(shape.Relationships, scenario.Relationships...) can reuse
shape.Relationships' backing array and corrupt shared data; replace it with the
copy-then-append pattern used elsewhere: allocate a new slice sized to
len(shape.Relationships)+len(scenario.Relationships), copy(shape.Relationships,
...) into it, then copy or append scenario.Relationships into the new slice and
assign that to scenarioRelationships (do the same for scenarioAttributes with
shape.Attributes and scenario.Attributes). This ensures no reuse of the original
backing arrays when computing scenarioRelationships and scenarioAttributes in
calculateConditionCoverage.
Comment on lines
+285
to
+369
| // Write scenario-specific relationships if any are defined | ||
| for _, t := range scenario.Relationships { | ||
| tup, err := tuple.Tuple(t) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("relationship: %s: %s", t, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| definition, _, err := c.Container.SR.ReadEntityDefinition(ctx, "t1", tup.GetEntity().GetType(), version) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("relationship: %s: %s", t, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| err = validation.ValidateTuple(definition, tup) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("relationship: %s: %s", t, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| _, err = c.Container.DW.Write(ctx, "t1", database.NewTupleCollection(tup), database.NewAttributeCollection()) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("relationship: %s: %s", t, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| // Write scenario-specific attributes if any are defined | ||
| for _, a := range scenario.Attributes { | ||
| attr, err := attribute.Attribute(a) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("attribute: %s: %s", a, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| definition, _, err := c.Container.SR.ReadEntityDefinition(ctx, "t1", attr.GetEntity().GetType(), version) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("attribute: %s: %s", a, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| err = validation.ValidateAttribute(definition, attr) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("attribute: %s: %s", a, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
|
|
||
| _, err = c.Container.DW.Write(ctx, "t1", database.NewTupleCollection(), database.NewAttributeCollection(attr)) | ||
| if err != nil { | ||
| errors = append(errors, Error{ | ||
| Type: "scenarios", | ||
| Key: i, | ||
| Message: fmt.Sprintf("attribute: %s: %s", a, err.Error()), | ||
| }) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'reset\|rollback\|snapshot\|truncate\|clear\|delete.*tuple\|delete.*attribute' --type=go -g '!vendor/**'Repository: Permify/permify
Length of output: 41
🏁 Script executed:
head -n 400 pkg/development/development.go | tail -n 150Repository: Permify/permify
Length of output: 3919
🏁 Script executed:
fd -e _test.go | xargs grep -l "scenario" | head -5Repository: Permify/permify
Length of output: 41
🏁 Script executed:
head -n 450 pkg/development/development.go | tail -n 200Repository: Permify/permify
Length of output: 5371
🏁 Script executed:
head -n 300 pkg/development/development.goRepository: Permify/permify
Length of output: 8152
🏁 Script executed:
rg -B5 -A5 'scenario.*reset\|reset.*scenario\|scenario.*isolat\|isolat.*scenario' --type=go pkg/development/Repository: Permify/permify
Length of output: 41
Scenario-specific relationships and attributes accumulate across all scenarios without isolation.
The database writes scenario-specific relationships and attributes (lines 286–326 and 329–369) to the same in-memory store as global relationships/attributes. Since there is no reset or rollback mechanism between scenario iterations, data persists cumulatively. For example, if scenario 1 writes team:1#member@user:2, scenario 2 will see that tuple even if it doesn't declare it.
The comment "Each item in the Scenarios slice is processed individually" suggests independence, but the implementation contradicts this—all scenarios share a single database instance throughout the RunWithShape() call.
Clarify whether this cumulative behavior is intentional. If scenarios should be isolated, implement a reset mechanism (truncate tuples/attributes or use snapshots) between iterations. If cumulative behavior is intended, update the naming and documentation to reflect that scenarios build upon prior data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/development/development.go` around lines 285 - 369, The scenario loop is
writing relationships/attributes into the shared data writer (c.Container.DW) so
scenario state accumulates across iterations; to make scenarios isolated, reset
or replace the data store before each scenario: inside RunWithShape (the loop
handling scenario.Relationships and scenario.Attributes) either call the
data-store reset/truncate API on c.Container.DW (e.g., DW.Clear/Truncate/Reset
if available) or construct a fresh in-memory writer instance for each scenario
and use that for c.Container.DW.Write; update any tests or callers to use the
per-scenario DW and ensure validation
(validation.ValidateTuple/ValidateAttribute) still reads the correct entity
definitions, or if cumulative behavior is intended, update the function/method
documentation and naming to explicitly state that scenarios incrementally build
on prior data.
285729101
force-pushed
the
feat/schema-scenario-refinement
branch
from
4f103a6 to
486e85e
Compare
Author
|
@tolgaozen integrates scenario-specific relationships and attributes into schema files for better test coverage. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
RelationshipsandAttributesfields to theScenariostruct, enabling scenarios to define their own relationships and attributes in addition to global definitionsRunWithShape) and the CLIvalidatecommandcustom-roles,banking-system,user-groups,organizations-hierarchies) to demonstrate the new capabilityTest plan
go build ./...succeeds with no compilation errorspermify validatecommand against updated YAML files/claim #838
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Tests