feat: integrate scenario-specific relationships and attributes in schema files#2834
Open
blessuselessk wants to merge 2 commits intoPermify:masterfrom
Open
Conversation
…ema files Add support for defining relationships and attributes within individual scenarios, in addition to the existing global definitions. This enables more focused and contextual testing while maintaining backward compatibility. Changes: - Add Relationships and Attributes fields to Scenario struct - Process scenario-specific relationships/attributes in development engine (RunWithShape) and CLI validate command before running checks - Update coverage calculations to aggregate global and scenario-specific relationships/attributes for accurate coverage reporting - Add test cases verifying scenario-specific coverage calculations - Update example YAML schemas (custom-roles, banking-system, user-groups, organizations-hierarchies) to demonstrate the new capability Fixes Permify#838
📝 WalkthroughWalkthroughAdded support for scenario-specific relationships and attributes: scenarios can declare their own relationships/attributes which are written to the datastore during validation/development runs, included in coverage calculations, and cleaned up after scenario execution. Changes
Sequence DiagramsequenceDiagram
participant Scenario as Scenario Definition
participant Processor as Validator/Development Processor
participant Schema as Schema Registry (SR)
participant Datastore as Datastore (DW)
participant Coverage as Coverage Calculator
Scenario->>Processor: Provide checks + scenario relationships/attributes
Processor->>Schema: Read entity definitions for each item
Processor->>Processor: Validate tuple/attribute against schema
Processor->>Datastore: Write validated tuples/attributes
Processor->>Processor: Execute scenario checks/assertions
Processor->>Datastore: Delete scenario-written tuples/attributes (cleanup)
Processor->>Coverage: Trigger coverage calculation
Coverage->>Coverage: Aggregate global + scenario relationships/attributes
Coverage->>Coverage: Compute uncovered sets and percentages
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 (2)
pkg/development/development.go (1)
286-326: Consider extracting helper functions to reduce code duplication.The relationship and attribute writing logic (parse → read definition → validate → write) is repeated identically for global data (lines 190-234, 237-281) and scenario-specific data (lines 286-326, 329-369). This duplication could be reduced.
♻️ Suggested refactor to extract helper functions
// writeRelationship validates and writes a single relationship tuple func (c *Development) writeRelationship(ctx context.Context, tenantID, version, rel string) error { tup, err := tuple.Tuple(rel) if err != nil { return fmt.Errorf("parse: %w", err) } definition, _, err := c.Container.SR.ReadEntityDefinition(ctx, tenantID, tup.GetEntity().GetType(), version) if err != nil { return fmt.Errorf("read definition: %w", err) } if err = validation.ValidateTuple(definition, tup); err != nil { return fmt.Errorf("validate: %w", err) } _, err = c.Container.DW.Write(ctx, tenantID, database.NewTupleCollection(tup), database.NewAttributeCollection()) return err } // writeAttribute validates and writes a single attribute func (c *Development) writeAttribute(ctx context.Context, tenantID, version, attr string) error { a, err := attribute.Attribute(attr) if err != nil { return fmt.Errorf("parse: %w", err) } definition, _, err := c.Container.SR.ReadEntityDefinition(ctx, tenantID, a.GetEntity().GetType(), version) if err != nil { return fmt.Errorf("read definition: %w", err) } if err = validation.ValidateAttribute(definition, a); err != nil { return fmt.Errorf("validate: %w", err) } _, err = c.Container.DW.Write(ctx, tenantID, database.NewTupleCollection(), database.NewAttributeCollection(a)) return err }Then both global and scenario-specific loops can use these helpers with appropriate error handling.
🤖 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, The duplicated parse→read definition→validate→write logic for relationships and attributes should be extracted into two helpers (e.g., writeRelationship(ctx, tenantID, version, rel string) and writeAttribute(ctx, tenantID, version, attr string)) that perform tuple.Tuple/attribute.Attribute parsing, call c.Container.SR.ReadEntityDefinition, validation.ValidateTuple/validation.ValidateAttribute, and c.Container.DW.Write, returning an error; then replace the repeated loops in Development with calls to these helpers and convert returned errors into the existing Error struct entries (same Type/Key/Message formatting) so the global and scenario-specific blocks reuse the helpers and remove duplication while keeping existing error handling.pkg/cmd/validate.go (1)
273-308: Same duplication and persistence concerns apply here.This block mirrors the scenario relationships handling and has the same issues noted above. A similar
writeAttributeshelper could be extracted to consolidate the four attribute-writing code paths (global and scenario-specific in bothvalidate.goand the development engine).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/validate.go` around lines 273 - 308, This code duplicates attribute-writing logic for scenario attributes (same as global attributes) and should be consolidated: extract a helper function (e.g., writeAttributes) and replace the loop over scenario.Attributes with a single call to it. The helper should accept context, the attribute list, the version, and necessary collaborators (attribute.Attribute, dev.Container.SR.ReadEntityDefinition, serverValidation.ValidateAttribute, dev.Container.DW.Write, database.NewTupleCollection, database.NewAttributeCollection) and perform the parse, definition lookup, server validation, DW.Write and logging/collection of errors exactly as the current block does; then call that helper from this location to remove duplication and reuse the same helper from the other validate.go/global paths.
🤖 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/cmd/validate.go`:
- Around line 236-271: Scenario relationships written in the
scenario.Relationships loop are persisted and duplicate the global-relationships
logic; extract the shared logic into a helper (e.g., writeRelationships) that
accepts ctx, relationships, sr (dev.Container.SR), dw (dev.Container.DW), tenant
("t1"), version, list (*ErrList) and indent to consolidate tuple parsing
(tuple.Tuple), schema lookup (ReadEntityDefinition), validation
(serverValidation.ValidateTuple) and write (DW.Write), then replace both the
global loop and the scenario loop to call that helper; additionally, ensure
scenario-specific data is cleaned up after the scenario (delete written tuples
via DW or provide clear documentation that scenario data is cumulative) so
subsequent scenarios are not polluted.
- Around line 248-260: Error handling for dev.Container.SR.ReadEntityDefinition
and serverValidation.ValidateTuple is inconsistent between global and scenario
relationship processing: scenario code collects errors (list.Add and continue)
while global code currently returns on those errors; update the global path that
calls ReadEntityDefinition and ValidateTuple so it mirrors the scenario behavior
by adding err.Error() to list (via list.Add) and continuing instead of
returning, ensuring tuple conversion and Write errors keep the same
collect-and-continue semantics; alternatively, if you prefer fail-fast, change
the scenario block to return on those calls—make one consistent choice and add a
brief comment in the functions referencing ReadEntityDefinition and
ValidateTuple to document the chosen strategy.
---
Nitpick comments:
In `@pkg/cmd/validate.go`:
- Around line 273-308: This code duplicates attribute-writing logic for scenario
attributes (same as global attributes) and should be consolidated: extract a
helper function (e.g., writeAttributes) and replace the loop over
scenario.Attributes with a single call to it. The helper should accept context,
the attribute list, the version, and necessary collaborators
(attribute.Attribute, dev.Container.SR.ReadEntityDefinition,
serverValidation.ValidateAttribute, dev.Container.DW.Write,
database.NewTupleCollection, database.NewAttributeCollection) and perform the
parse, definition lookup, server validation, DW.Write and logging/collection of
errors exactly as the current block does; then call that helper from this
location to remove duplication and reuse the same helper from the other
validate.go/global paths.
In `@pkg/development/development.go`:
- Around line 286-326: The duplicated parse→read definition→validate→write logic
for relationships and attributes should be extracted into two helpers (e.g.,
writeRelationship(ctx, tenantID, version, rel string) and writeAttribute(ctx,
tenantID, version, attr string)) that perform tuple.Tuple/attribute.Attribute
parsing, call c.Container.SR.ReadEntityDefinition,
validation.ValidateTuple/validation.ValidateAttribute, and c.Container.DW.Write,
returning an error; then replace the repeated loops in Development with calls to
these helpers and convert returned errors into the existing Error struct entries
(same Type/Key/Message formatting) so the global and scenario-specific blocks
reuse the helpers and remove duplication while keeping existing error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13152f0a-1686-49c2-be72-b27b21631595
📒 Files selected for processing (9)
assets/example-shapes/banking-system.yamlassets/example-shapes/custom-roles.yamlassets/example-shapes/organizations-hierarchies.yamlassets/example-shapes/user-groups.yamlpkg/cmd/validate.gopkg/development/coverage/coverage.gopkg/development/coverage/coverage_test.gopkg/development/development.gopkg/development/file/shape.go
…ach scenario Address CodeRabbit review feedback: 1. Fix scenario data persistence: Relationships and attributes written for a scenario are now deleted after that scenario's checks complete, preventing pollution of subsequent scenarios. 2. Reduce code duplication: Extract writeRelationships, writeAttributes, deleteRelationships, and deleteAttributes helper functions in both development.go and validate.go. The repeated parse-validate-write logic for global and scenario-specific data now shares the same code path.
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/development/development.go (2)
176-196: Same early-return concern as invalidate.go.Consider applying the "continue on error" approach to ensure all items are attempted for cleanup, collecting errors for reporting at the end.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/development/development.go` around lines 176 - 196, The deleteRelationships function currently returns immediately on the first Delete error which prevents trying to remove remaining tuples; update deleteRelationships to continue attempting Delete for each tuple (use the existing function name deleteRelationships and the call to c.Container.DW.Delete) and collect any errors (e.g., in a slice or an errors.Join/strings builder) instead of early-returning, then after the loop return a combined/aggregated error if any deletes failed (or nil if all succeeded) so all cleanup attempts are performed and failures are reported together.
198-213: Same early-return pattern asdeleteRelationships.Consider applying consistent error aggregation here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/development/development.go` around lines 198 - 213, The deleteAttributes function currently returns immediately on the first Delete error (same pattern as deleteRelationships); change it to aggregate errors instead: iterate all attrs, call c.Container.DW.Delete(ctx, "t1", &v1.TupleFilter{}, &v1.AttributeFilter{...}) for each, collect any returned errors (e.g., append to a slice or use a multi-error helper) and continue processing remaining attrs, then if any errors were collected return an aggregated error at the end; reference the deleteAttributes function and the c.Container.DW.Delete invocation when making the change to match the error-aggregation strategy used for deleteRelationships.pkg/cmd/validate.go (2)
175-190: Same early-return pattern asdeleteRelationships.Consider applying the same "continue on error" approach here for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/validate.go` around lines 175 - 190, The deleteAttributes function currently returns immediately on the first Delete error; change it to match deleteRelationships by continuing the loop on per-attribute errors: inside the loop call dev.Container.DW.Delete for each attr, if Delete returns an error record the first non-nil error (e.g., firstErr) but do not return immediately—continue processing remaining attributes—and after the loop return the recorded error (or nil if none). This preserves consistency with deleteRelationships while still surfacing an error if any deletion failed.
153-173: Early return on delete error may leave partial cleanup.If deletion fails for one tuple, remaining tuples are not cleaned up. This could leave stale scenario data in the datastore. Consider continuing on error and aggregating failures, similar to the write helpers.
♻️ Proposed fix to continue cleanup on errors
func deleteRelationships(ctx context.Context, dev *development.Development, tuples []*base.Tuple) error { + var errs []string for _, tup := range tuples { _, err := dev.Container.DW.Delete(ctx, "t1", &base.TupleFilter{ Entity: &base.EntityFilter{ Type: tup.GetEntity().GetType(), Ids: []string{tup.GetEntity().GetId()}, }, Relation: tup.GetRelation(), Subject: &base.SubjectFilter{ Type: tup.GetSubject().GetType(), Ids: []string{tup.GetSubject().GetId()}, Relation: tup.GetSubject().GetRelation(), }, }, &base.AttributeFilter{}) if err != nil { - return err + errs = append(errs, err.Error()) } } + if len(errs) > 0 { + return errors.New(strings.Join(errs, "; ")) + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/validate.go` around lines 153 - 173, The deleteRelationships function currently returns immediately on the first Delete error which can leave remaining tuples undeleted; update deleteRelationships to continue iterating over all tuples even if dev.Container.DW.Delete returns an error, collect/aggregate any errors (e.g., append error messages or use a multi-error) and after the loop return a combined error if any failures occurred (or nil if all succeeded). Locate the loop over tuples in deleteRelationships and replace the early return on err with logic that records the failure (referencing dev.Container.DW.Delete and the tuple variable tup) and proceeds to the next tuple, then at the end return an aggregated error consistent with other write helper patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/cmd/validate.go`:
- Around line 175-190: The deleteAttributes function currently returns
immediately on the first Delete error; change it to match deleteRelationships by
continuing the loop on per-attribute errors: inside the loop call
dev.Container.DW.Delete for each attr, if Delete returns an error record the
first non-nil error (e.g., firstErr) but do not return immediately—continue
processing remaining attributes—and after the loop return the recorded error (or
nil if none). This preserves consistency with deleteRelationships while still
surfacing an error if any deletion failed.
- Around line 153-173: The deleteRelationships function currently returns
immediately on the first Delete error which can leave remaining tuples
undeleted; update deleteRelationships to continue iterating over all tuples even
if dev.Container.DW.Delete returns an error, collect/aggregate any errors (e.g.,
append error messages or use a multi-error) and after the loop return a combined
error if any failures occurred (or nil if all succeeded). Locate the loop over
tuples in deleteRelationships and replace the early return on err with logic
that records the failure (referencing dev.Container.DW.Delete and the tuple
variable tup) and proceeds to the next tuple, then at the end return an
aggregated error consistent with other write helper patterns.
In `@pkg/development/development.go`:
- Around line 176-196: The deleteRelationships function currently returns
immediately on the first Delete error which prevents trying to remove remaining
tuples; update deleteRelationships to continue attempting Delete for each tuple
(use the existing function name deleteRelationships and the call to
c.Container.DW.Delete) and collect any errors (e.g., in a slice or an
errors.Join/strings builder) instead of early-returning, then after the loop
return a combined/aggregated error if any deletes failed (or nil if all
succeeded) so all cleanup attempts are performed and failures are reported
together.
- Around line 198-213: The deleteAttributes function currently returns
immediately on the first Delete error (same pattern as deleteRelationships);
change it to aggregate errors instead: iterate all attrs, call
c.Container.DW.Delete(ctx, "t1", &v1.TupleFilter{}, &v1.AttributeFilter{...})
for each, collect any returned errors (e.g., append to a slice or use a
multi-error helper) and continue processing remaining attrs, then if any errors
were collected return an aggregated error at the end; reference the
deleteAttributes function and the c.Container.DW.Delete invocation when making
the change to match the error-aggregation strategy used for deleteRelationships.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ea6e52c-1701-4b1c-8ed4-cc65875f414c
📒 Files selected for processing (2)
pkg/cmd/validate.gopkg/development/development.go
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.
Fixes #838
/claim #838
Summary
This PR adds support for defining relationships and attributes within individual scenarios in schema YAML files, as proposed in #838. Scenarios can now carry their own relationship and attribute definitions that are written to the database before checks run, in addition to the global definitions.
Key changes:
RelationshipsandAttributesfields to theScenariostruct inpkg/development/file/shape.goRunWithShape(development engine) and thevalidateCLI command, writing them before running scenario checkscustom-roles,banking-system,user-groups,organizations-hierarchies) to demonstrate the new capabilityBackward compatible: existing YAML files without scenario-specific fields parse and work exactly as before.
Example
Test plan
go build ./...succeeds with no compilation errorspermify validatesucceeds against all four updated example YAML filesSummary by CodeRabbit
New Features
Behavior Changes
Tests
Examples