fix(query): fix arrow reversal tagging the subrelation incorrectly

Author: barakmich

pkg/query/arrow.go

@@ -142,12 +142,14 @@ func (a *ArrowIterator) checkRightToLeft(ctx *Context, resources []Object, subje
 			}
 			rightPathCount++
 
-			// rightPath.Resource is an intermediate object from the right side
-			// Now check if any of our input resources connect to this intermediate via left
+			// rightPath.Resource is an intermediate object from the right side.
+			// Now check if any of our input resources connect to this intermediate via left.
+			// Use tuple.Ellipsis as the relation since the left side stores subjects with "..."
+			// (the canonical relation for direct membership with no subrelation).
 			intermediateAsSubject := ObjectAndRelation{
 				ObjectType: rightPath.Resource.ObjectType,
 				ObjectID:   rightPath.Resource.ObjectID,
-				Relation:   "",
+				Relation:   tuple.Ellipsis,
 			}
 
 			leftSeq, err := ctx.Check(a.left, resources, intermediateAsSubject)

pkg/query/arrow_reversal_test.go

@@ -0,0 +1,159 @@
+package query
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/authzed/spicedb/internal/datastore/common"
+	"github.com/authzed/spicedb/internal/datastore/memdb"
+	"github.com/authzed/spicedb/pkg/datalayer"
+	"github.com/authzed/spicedb/pkg/datastore"
+	"github.com/authzed/spicedb/pkg/schema/v2"
+	"github.com/authzed/spicedb/pkg/schemadsl/compiler"
+	"github.com/authzed/spicedb/pkg/schemadsl/input"
+	"github.com/authzed/spicedb/pkg/tuple"
+)
+
+// TestDoubleWideArrowAdvisedMatchesPlain builds the file→org→group→user
+// double-arrow hierarchy, checks that the plain (LTR) plan finds paths from
+// file0 to user42, then applies a CountAdvisor (which may flip one or both
+// arrow directions to RTL) and asserts the advised plan returns the same set
+// of paths.
+func TestDoubleWideArrowAdvisedMatchesPlain(t *testing.T) {
+	t.Parallel()
+
+	const (
+		numFiles      = 5
+		numOrgs       = 29  // prime
+		numGroups     = 97  // prime
+		numUsers      = 997 // prime
+		orgsPerFile   = 4
+		groupsPerOrg  = 10
+		usersPerGroup = 15
+	)
+
+	rawDS, err := memdb.NewMemdbDatastore(0, 0, memdb.DisableGC)
+	require.NoError(t, err)
+
+	ctx := context.Background()
+
+	schemaText := `
+		definition user {}
+
+		definition group {
+			relation member: user
+		}
+
+		definition org {
+			relation group: group
+			permission member = group->member
+		}
+
+		definition file {
+			relation org: org
+			relation view: user
+			permission viewer = view + org->member
+		}
+	`
+
+	compiled, err := compiler.Compile(compiler.InputSchema{
+		Source:       input.Source("test"),
+		SchemaString: schemaText,
+	}, compiler.AllowUnprefixedObjectType())
+	require.NoError(t, err)
+
+	_, err = rawDS.ReadWriteTx(ctx, func(ctx context.Context, rwt datastore.ReadWriteTransaction) error {
+		return rwt.LegacyWriteNamespaces(ctx, compiled.ObjectDefinitions...)
+	})
+	require.NoError(t, err)
+
+	relationships := make([]tuple.Relationship, 0,
+		numFiles*orgsPerFile+numOrgs*groupsPerOrg+numGroups*usersPerGroup)
+
+	for fileID := 0; fileID < numFiles; fileID++ {
+		step := fileID + 1
+		for i := 0; i < orgsPerFile; i++ {
+			relationships = append(relationships, tuple.MustParse(
+				fmt.Sprintf("file:file%d#org@org:org%d", fileID, (i*step)%numOrgs),
+			))
+		}
+	}
+
+	for orgID := 0; orgID < numOrgs; orgID++ {
+		step := orgID + 1
+		for i := 0; i < groupsPerOrg; i++ {
+			relationships = append(relationships, tuple.MustParse(
+				fmt.Sprintf("org:org%d#group@group:group%d", orgID, (i*step)%numGroups),
+			))
+		}
+	}
+
+	for groupID := 0; groupID < numGroups; groupID++ {
+		step := groupID + 1
+		for i := 0; i < usersPerGroup; i++ {
+			relationships = append(relationships, tuple.MustParse(
+				fmt.Sprintf("group:group%d#member@user:user%d", groupID, (i*step)%numUsers),
+			))
+		}
+	}
+
+	revision, err := common.WriteRelationships(ctx, rawDS, tuple.UpdateOperationCreate, relationships...)
+	require.NoError(t, err)
+
+	dsSchema, err := schema.BuildSchemaFromDefinitions(compiled.ObjectDefinitions, nil)
+	require.NoError(t, err)
+
+	canonicalOutline, err := BuildOutlineFromSchema(dsSchema, "file", "viewer")
+	require.NoError(t, err)
+
+	resources := NewObjects("file", "file0")
+	subject := NewObject("user", "user42").WithEllipses()
+
+	reader := NewQueryDatastoreReader(datalayer.NewDataLayer(rawDS).SnapshotReader(revision))
+
+	// ---- plain (LTR) ----
+
+	plainTrace := NewTraceLogger()
+	plainIt, err := canonicalOutline.Compile()
+	require.NoError(t, err)
+
+	plainSeq, err := NewLocalContext(ctx, WithReader(reader), WithTraceLogger(plainTrace)).
+		Check(plainIt, resources, subject)
+	require.NoError(t, err)
+	plainPaths, err := CollectAll(plainSeq)
+	require.NoError(t, err)
+	require.NotEmpty(t, plainPaths, "plain plan must find paths from file0 to user42")
+
+	// ---- advised (CountAdvisor applied after a warm-up run) ----
+
+	obs := NewCountObserver()
+	warmIt, err := canonicalOutline.Compile()
+	require.NoError(t, err)
+	warmSeq, err := NewLocalContext(ctx, WithReader(reader), WithObserver(obs)).
+		Check(warmIt, resources, subject)
+	require.NoError(t, err)
+	_, err = CollectAll(warmSeq)
+	require.NoError(t, err)
+
+	advisedCO, err := ApplyAdvisor(canonicalOutline, NewCountAdvisor(obs.GetStats()))
+	require.NoError(t, err)
+	advisedIt, err := advisedCO.Compile()
+	require.NoError(t, err)
+
+	advisedTrace := NewTraceLogger()
+	advisedSeq, err := NewLocalContext(ctx, WithReader(reader), WithTraceLogger(advisedTrace)).
+		Check(advisedIt, resources, subject)
+	require.NoError(t, err)
+	advisedPaths, err := CollectAll(advisedSeq)
+	require.NoError(t, err)
+
+	t.Logf("plain explain:\n%s\nplain trace:\n%s", plainIt.Explain(), plainTrace.DumpTrace())
+	t.Logf("advised explain:\n%s\nadvised trace:\n%s", advisedIt.Explain(), advisedTrace.DumpTrace())
+
+	require.Equal(t, plainPaths, advisedPaths,
+		"advised plan must return the same paths as plain",
+	)
+}

pkg/query/datastore.go

@@ -450,9 +450,15 @@ func (r *DatastoreIterator) IterResourcesImpl(ctx *Context, subject ObjectAndRel
 	}
 
 	// Check if subject relation matches what this iterator expects.
-	// Both the schema's expected subrelation and the query's subject relation must match exactly.
-	// Ellipsis is a specific relation value, not a wildcard.
-	if r.base.Subrelation() != subject.Relation {
+	// When the schema uses ellipsis ("..."), accept both "" and "..." from the caller.
+	// Ideally callers always pass tuple.Ellipsis, but defensive tolerance here avoids
+	// silent misses if a future caller constructs an ObjectAndRelation with Relation: "".
+	expectedSubrelation := r.base.Subrelation()
+	if expectedSubrelation == tuple.Ellipsis {
+		if subject.Relation != "" && subject.Relation != tuple.Ellipsis {
+			return EmptyPathSeq(), nil
+		}
+	} else if expectedSubrelation != subject.Relation {
 		return EmptyPathSeq(), nil
 	}
 
@@ -712,12 +718,12 @@ func (r *DatastoreIterator) SubjectTypes() ([]ObjectType, error) {
 		}}, nil
 	}
 
-	// For ellipsis, return the base type with empty subrelation
-	// Ellipsis means "any relation on this type"
+	// For ellipsis, preserve the ellipsis subrelation so callers that construct
+	// ObjectAndRelation values from SubjectTypes get the correct relation to query with.
 	if r.base.Subrelation() == tuple.Ellipsis {
 		return []ObjectType{{
 			Type:        r.base.Type(),
-			Subrelation: "",
+			Subrelation: tuple.Ellipsis,
 		}}, nil
 	}
 

pkg/query/datastore_test.go

@@ -414,6 +414,6 @@ func TestDatastoreIterator_Types(t *testing.T) {
 		require.NoError(err)
 		require.Len(subjectTypes, 1)
 		require.Equal("user", subjectTypes[0].Type)
-		require.Empty(subjectTypes[0].Subrelation) // Ellipsis returns empty subrelation
+		require.Equal(tuple.Ellipsis, subjectTypes[0].Subrelation) // Ellipsis is preserved as-is
 	})
 }