internal/storetest/storedata.go (1)

32-38: 🛠️ Refactor suggestion

Ambiguity when both User and Users are populated

All call-sites treat Users as authoritative when non-empty, silently ignoring User. Accidentally filling in both fields will therefore drop the single user without warning.
Consider validating the struct at read time to guarantee mutual exclusivity, e.g.:

func (c ModelTestCheck) EffectiveUsers() ([]string, error) {
    if len(c.Users) > 0 && c.User != "" {
-       // silently prefer Users today
+       return nil, fmt.Errorf("both 'user' and 'users' are set – choose one")
    }
    if len(c.Users) > 0 {
        return c.Users, nil
    }
    return []string{c.User}, nil
}

Centralising the “effective users” logic also removes the duplication noted in other files.

cmd/store/import.go (1)

235-238: Same ‘effective users’ logic duplicated across 3 files
This was already flagged in previous reviews – extracting a helper (e.g. checkTest.EffectiveUsers()) avoids drift.

internal/storetest/remotetest.go (1)

37-41: Shared logic – please extract helper
Same comment as before: users := … / fallback to single user appears in localtest.go, remotetest.go, and import.go. One helper on ModelTestCheck will keep behaviour consistent.

internal/storetest/localtest.go (1)

28-33: Extract duplicated “effective users” logic into a helper

The same users := ... ; if len(checkTest.Users) > 0 { … } pattern now appears in both local and remote test runners (see previous review). Centralising this in a small helper (e.g. getEffectiveUsers(single string, multi []string) []string) removes duplication, keeps precedence rules in one place and prevents drift.

cmd/store/import.go (1)

240-249: Minor optimisation – pre-allocate slice

If a check has many relations and many users the inner append may trigger multiple reallocations. Pre-allocating once per check is cheap and removes that cost:

-    for _, user := range users {
-        for relation, expectation := range checkTest.Assertions {
-            assertions = append(assertions, client.ClientAssertion{
+    for _, user := range users {
+        for relation, expectation := range checkTest.Assertions {
+            assertions = append(assertions, client.ClientAssertion{
                 User:        user,
                 Relation:    relation,
                 Object:      checkTest.Object,
                 Expectation: expectation,
             })
-        }
+        }
     }

Not critical but easy to include.

cmd/store/import_test.go (1)

148-155: Hard-coding expected assertions based on test case name couples data & control

Instead of branching on the string literal, include the expected assertions inside each table entry:

-        mockWriteAssertions: true,
-        ...
-        testStore: …,
+        mockWriteAssertions: true,
+        expected: multiUserAssertions,
+        testStore: …,

and use that directly in the setup helper. Reduces indirection and makes adding new cases trivial.

internal/storetest/localtest.go (1)

47-55: Avoid repeated context marshaling

structpb.NewStruct(*checkTest.Context) is invoked once per (user, relation) pair even though the context is identical for all of them. Moving the conversion outside both loops (or caching ctx after the first conversion) would save work and simplify error handling.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

internal/storetest/utils.go (2)

4-5: Return a defensive copy of the slice
checkTest.Users is returned directly, so any caller that appends/modifies the slice mutates the same backing array captured in the copied struct. This can create non-obvious test coupling. A trivial, allocation-cheap fix:

-        return checkTest.Users
+        return append([]string(nil), checkTest.Users...)

---

3-3: Minor: accept pointer to avoid unnecessary struct copy
ModelTestCheck carries several fields (maps, slices). Passing it by value copies the slice/map headers each call; passing *ModelTestCheck avoids this and signals immutability intent. Low impact but worth considering.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

internal/storetest/read-from-input.go (1)

34-40: Close the opened file to avoid descriptor leaks

os.Open leaves the descriptor open for the lifetime of the process if Close is never called.
Add a defer testFile.Close() immediately after the error check to ensure resources are released even on early returns.

 testFile, err := os.Open(fileName)
 if err != nil {
     return format, nil, fmt.Errorf("failed to read file %s due to %w", fileName, err)
 }
+defer testFile.Close() // always close the file handle

internal/storetest/read-from-input.go (1)

57-60: Validation already happens again in RunTests – minor duplication

storeData.Validate() is also invoked at the top of RunTests.
Running it here is harmless but redundant; if the method ever becomes expensive, this double-call could be avoided by trusting the caller or documenting the expectation.

No action required now, just flagging for awareness.

internal/storetest/tests.go (1)

39-42: Second call to Validate() – consider consolidating responsibility

RunTests performs its own validation, but ReadFromFile already does so.
If every consumer comes through ReadFromFile, you can drop this call.
Conversely, if callers may construct StoreData programmatically, leave it here and document that RunTests must receive validated data.

Your call – just ensure the contract is explicit.

internal/storetest/storedata_test.go (1)

10-32: Split into named sub-tests for clearer failure output

All four scenarios run in the same function; when one fails, it’s harder to see which.
Using t.Run gives granular reporting and keeps the table extensible.

-func TestStoreDataValidate(t *testing.T) {
-    t.Parallel()
-
-    validSingle := ...
-    assert.NoError(t, validSingle.Validate())
-
-    validUsers := ...
-    assert.NoError(t, validUsers.Validate())
-
-    invalidBoth := ...
-    require.Error(t, invalidBoth.Validate())
-
-    invalidNone := ...
-    require.Error(t, invalidNone.Validate())
-}
+func TestStoreDataValidate(t *testing.T) {
+    t.Parallel()
+
+    cases := []struct {
+        name    string
+        data    StoreData
+        wantErr bool
+    }{
+        {"single user", validSingle, false},
+        {"multiple users", validUsers, false},
+        {"both fields", invalidBoth, true},
+        {"no user fields", invalidNone, true},
+    }
+
+    for _, tc := range cases {
+        tc := tc
+        t.Run(tc.name, func(t *testing.T) {
+            if tc.wantErr {
+                require.Error(t, tc.data.Validate())
+            } else {
+                assert.NoError(t, tc.data.Validate())
+            }
+        })
+    }
+}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

internal/storetest/remotetest.go (1)

40-56: Minor perf tweak – pre-allocate results slice
With nested loops over users, objects, and assertions, the slice can grow rapidly. Pre-computing its capacity prevents multiple re-allocations and marginally speeds large test suites.

-	results := []ModelTestCheckSingleResult{}
+	capacity := len(users) * len(objects) * len(checkTest.Assertions)
+	results := make([]ModelTestCheckSingleResult, 0, capacity)

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

example/model.fga.yaml (2)

51-59: LGTM: Excellent demonstration of multi-entity testing

The new test case effectively demonstrates the users and objects functionality, testing multiple entities with a single assertion block. However, please fix the trailing spaces.

Remove trailing spaces:

         assertions:
           can_view: true
-          can_write: true          
+          can_write: true

---

83-83: Remove trailing whitespace

-          
+

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

internal/storetest/storedata.go (1)

144-173: Fix dynamic error creation to use static errors.

This issue was already flagged in previous reviews. The validation logic correctly enforces mutual exclusivity, but the error creation should use static errors instead of dynamic ones to follow Go best practices.

Please refer to the previous review comment which provided a comprehensive solution using static error variables.

internal/storetest/storedata.go (1)

34-37: Fix struct tag alignment for consistency.

The struct tags are not aligned consistently, affecting code readability.

Apply this diff to align the tags:

-	User       string                  `json:"user,omitempty"       yaml:"user,omitempty"`
-	Users      []string                `json:"users,omitempty"      yaml:"users,omitempty"`
-	Object     string                  `json:"object,omitempty"     yaml:"object,omitempty"`
-	Objects    []string                `json:"objects,omitempty"    yaml:"objects,omitempty"`
+	User       string                  `json:"user,omitempty"    yaml:"user,omitempty"`
+	Users      []string                `json:"users,omitempty"   yaml:"users,omitempty"`
+	Object     string                  `json:"object,omitempty"  yaml:"object,omitempty"`
+	Objects    []string                `json:"objects,omitempty" yaml:"objects,omitempty"`

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

example/model.fga.yaml (2)

57-57: Remove trailing spaces.

Static analysis detected trailing spaces on this line.

-          can_write: true          
+          can_write: true

---

81-81: Remove trailing spaces.

Static analysis detected trailing spaces on this line.

-          
+

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

example/model.fga.yaml (2)

55-58: Clarify assertion coverage for generated combinations.
Add a brief comment here to note that the assertions block applies to all user‐object pairs produced by the cross product of the two lists. This will improve readability for someone unfamiliar with the new syntax.

---

81-81: Remove trailing whitespace.
yamllint flags a trailing space on this blank line—please delete it to keep the file clean.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

example/model.fga.yaml (2)

50-57: Remove trailing whitespace in comment
Static analysis flagged a trailing space at the end of the comment line.

--- a/example/model.fga.yaml
+++ b/example/model.fga.yaml
@@ line 55
-          # These assertions are run for each user 
+          # These assertions are run for each user

---

58-65: Remove trailing whitespace in list and assertion value
Static analysis flagged trailing spaces at the end of the user mapping and the can_write value.

--- a/example/model.fga.yaml
+++ b/example/model.fga.yaml
@@ line 58
-      - user: user:anne 
+      - user: user:anne
@@ line 65
-          can_write: true           
+          can_write: true

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro