Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 56 additions & 58 deletions internal/services/integrationtesting/query_plan_consistency_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
package integrationtesting_test

import (
"fmt"
"path/filepath"
"strings"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -108,50 +106,68 @@ func runQueryPlanAssertions(t *testing.T, handle *queryPlanConsistencyHandle) {
for _, assertion := range entry.assertions {
assertion := assertion
t.Run(assertion.RelationshipWithContextString, func(t *testing.T) {
require := require.New(t)

rel := assertion.Relationship
it, err := query.BuildIteratorFromSchema(handle.schema, rel.Resource.ObjectType, rel.Resource.Relation)
require.NoError(err)

qctx := handle.buildContext(t)
// Run both unoptimized and optimized versions
for _, optimizationMode := range []struct {
name string
optimize bool
}{
{"unoptimized", false},
{"optimized", true},
} {
optimizationMode := optimizationMode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, having to copy the variable hasn't been necessary since go 1.22 https://go.dev/wiki/LoopvarExperiment

t.Run(optimizationMode.name, func(t *testing.T) {
require := require.New(t)

rel := assertion.Relationship
it, err := query.BuildIteratorFromSchema(handle.schema, rel.Resource.ObjectType, rel.Resource.Relation)
require.NoError(err)

// Apply static optimizations if requested
if optimizationMode.optimize {
it, _, err = query.ApplyOptimizations(it, query.StaticOptimizations)
require.NoError(err)
}

// Add caveat context from assertion if available
if len(assertion.CaveatContext) > 0 {
qctx.CaveatContext = assertion.CaveatContext
}
qctx := handle.buildContext(t)

seq, err := qctx.Check(it, []query.Object{query.GetObject(rel.Resource)}, rel.Subject)
require.NoError(err)
// Add caveat context from assertion if available
if len(assertion.CaveatContext) > 0 {
qctx.CaveatContext = assertion.CaveatContext
}

rels, err := query.CollectAll(seq)
require.NoError(err)
seq, err := qctx.Check(it, []query.Object{query.GetObject(rel.Resource)}, rel.Subject)
require.NoError(err)

rels, err := query.CollectAll(seq)
require.NoError(err)

// Print trace if test fails
if qctx.TraceLogger != nil {
defer func() {
if t.Failed() {
t.Logf("Trace for %s:\n%s", entry.name, qctx.TraceLogger.DumpTrace())
// Also print the tree structure for debugging
if it != nil {
t.Logf("Tree structure:\n%s", it.Explain().String())
}
}
}()
}

// Print trace if test fails
if qctx.TraceLogger != nil {
defer func() {
if t.Failed() {
t.Logf("Trace for %s:\n%s", entry.name, qctx.TraceLogger.DumpTrace())
// Also print the tree structure for debugging
if it != nil {
t.Logf("Tree structure:\n%s", explainTree(it, 0))
switch entry.expectedPermissionship {
case v1.CheckPermissionResponse_PERMISSIONSHIP_CONDITIONAL_PERMISSION:
require.Len(rels, 1)
require.NotNil(rels[0].Caveat)
case v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION:
require.Len(rels, 1)
require.Nil(rels[0].Caveat)
case v1.CheckPermissionResponse_PERMISSIONSHIP_NO_PERMISSION:
if len(rels) != 0 && qctx.TraceLogger != nil {
t.Logf("Expected 0 relations but got %d. Trace:\n%s", len(rels), qctx.TraceLogger.DumpTrace())
}
require.Len(rels, 0)
}
}()
}

switch entry.expectedPermissionship {
case v1.CheckPermissionResponse_PERMISSIONSHIP_CONDITIONAL_PERMISSION:
require.Len(rels, 1)
require.NotNil(rels[0].Caveat)
case v1.CheckPermissionResponse_PERMISSIONSHIP_HAS_PERMISSION:
require.Len(rels, 1)
require.Nil(rels[0].Caveat)
case v1.CheckPermissionResponse_PERMISSIONSHIP_NO_PERMISSION:
if len(rels) != 0 && qctx.TraceLogger != nil {
t.Logf("Expected 0 relations but got %d. Trace:\n%s", len(rels), qctx.TraceLogger.DumpTrace())
}
require.Len(rels, 0)
})
}
})
}
Expand All @@ -160,21 +176,3 @@ func runQueryPlanAssertions(t *testing.T, handle *queryPlanConsistencyHandle) {
}
})
}

// explainTree recursively explains the tree structure for debugging
func explainTree(iter query.Iterator, depth int) string {
indent := strings.Repeat(" ", depth)
explain := iter.Explain()
result := fmt.Sprintf("%s%s: %s\n", indent, explain.Name, explain.Info)

for _, subExplain := range explain.SubExplain {
// For SubExplain, we need to create a dummy iterator to get the tree structure
// This is a simplified approach - in practice we'd need access to the actual sub-iterators
subResult := fmt.Sprintf("%s %s: %s\n", indent, subExplain.Name, subExplain.Info)
result += subResult
// Note: We can't recursively call explainTree on SubExplain because it's not an Iterator
// This gives us one level of detail which should be sufficient for debugging
}

return result
}
15 changes: 11 additions & 4 deletions pkg/query/alias.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRel
return nil, err
}

return func(yield func(Path, error) bool) {
// Create combined sequence with self-edge and rewritten paths
combined := func(yield func(Path, error) bool) {
// Yield the self-edge first
if !yield(selfPath, nil) {
return
Expand All @@ -57,7 +58,10 @@ func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRel
return
}
}
}, nil
}

// Wrap with deduplication to handle duplicate paths after rewriting
return DeduplicatePathSeq(combined), nil
}
}

Expand All @@ -67,7 +71,7 @@ func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRel
return nil, err
}

return func(yield func(Path, error) bool) {
rewritten := func(yield func(Path, error) bool) {
for path, err := range subSeq {
if err != nil {
yield(Path{}, err)
Expand All @@ -79,7 +83,10 @@ func (a *Alias) CheckImpl(ctx *Context, resources []Object, subject ObjectAndRel
return
}
}
}, nil
}

// Wrap with deduplication to handle duplicate paths after rewriting
return DeduplicatePathSeq(rewritten), nil
}

func (a *Alias) IterSubjectsImpl(ctx *Context, resource Object) (PathSeq, error) {
Expand Down
97 changes: 97 additions & 0 deletions pkg/query/optimize.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package query

// TypedOptimizerFunc is a function that transforms an iterator of a specific type T
// into a potentially optimized iterator. It returns the optimized iterator, a boolean
// indicating whether any optimization was performed, and an error if the optimization failed.
//
// The type parameter T constrains the function to operate only on specific iterator types,
// providing compile-time type safety when creating typed optimizers.
type TypedOptimizerFunc[T Iterator] func(it T) (Iterator, bool, error)

// OptimizerFunc is a type-erased wrapper around TypedOptimizerFunc[T] that can be
// stored in a homogeneous list while maintaining type safety at runtime.
type OptimizerFunc func(it Iterator) (Iterator, bool, error)

// WrapOptimizer wraps a typed TypedOptimizerFunc[T] into a type-erased OptimizerFunc.
// This allows optimizer functions for different concrete iterator types to be stored
// together in a heterogeneous list.
func WrapOptimizer[T Iterator](fn TypedOptimizerFunc[T]) OptimizerFunc {
return func(it Iterator) (Iterator, bool, error) {
if v, ok := it.(T); ok {
return fn(v)
}
return it, false, nil
}
}

// StaticOptimizations is a list of optimization functions that can be safely applied
// to any iterator tree without needing runtime information or context.
var StaticOptimizations = []OptimizerFunc{
RemoveNullIterators,
ElideSingletonUnionAndIntersection,
Copy link
Contributor

@miparnisari miparnisari Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to google what "elide" meant 😅 Now that I looked at the code I would have called this "collapse". But i'm not going to die on this hill

WrapOptimizer(PushdownCaveatEvaluation),
}

// ApplyOptimizations recursively applies a list of optimizer functions to an iterator
// tree, transforming it into an optimized form.
//
// The function operates bottom-up, optimizing leafs and subiterators first, and replacing the
// subtrees up to the top, which it then returns.
//
// Parameters:
// - it: The iterator tree to optimize
// - fns: A list of optimizer functions to apply
//
// Returns:
// - The optimized iterator (which may be the same as the input if no optimizations applied)
// - A boolean indicating whether any changes were made
// - An error if any optimization failed
func ApplyOptimizations(it Iterator, fns []OptimizerFunc) (Iterator, bool, error) {
var err error
origSubs := it.Subiterators()
changed := false
if len(origSubs) != 0 {
// Make a copy of the subiterators slice to avoid mutating the original iterator
subs := make([]Iterator, len(origSubs))
copy(subs, origSubs)

subChanged := false
for i, subit := range subs {
newit, ok, err := ApplyOptimizations(subit, fns)
if err != nil {
return nil, false, err

Check warning on line 62 in pkg/query/optimize.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize.go#L62

Added line #L62 was not covered by tests
}
if ok {
subs[i] = newit
subChanged = true
}
}
if subChanged {
changed = true
it, err = it.ReplaceSubiterators(subs)
if err != nil {
return nil, false, err

Check warning on line 73 in pkg/query/optimize.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize.go#L73

Added line #L73 was not covered by tests
}
}
}

// Apply each optimizer to the current iterator
// If any optimizer transforms the iterator, recursively optimize the new tree
for _, fn := range fns {
newit, fnChanged, err := fn(it)
if err != nil {
return nil, false, err

Check warning on line 83 in pkg/query/optimize.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize.go#L83

Added line #L83 was not covered by tests
}
if fnChanged {
// The iterator was transformed - recursively optimize the new tree
// to ensure all optimizations are fully applied
optimizedIt, _, err := ApplyOptimizations(newit, fns)
if err != nil {
return nil, false, err

Check warning on line 90 in pkg/query/optimize.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize.go#L90

Added line #L90 was not covered by tests
}
// Return true for changed since we did transform the iterator
return optimizedIt, true, nil
}
}
return it, changed, nil
}
93 changes: 93 additions & 0 deletions pkg/query/optimize_caveat.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package query

import (
core "github.com/authzed/spicedb/pkg/proto/core/v1"
"github.com/authzed/spicedb/pkg/spiceerrors"
)

// PushdownCaveatEvaluation pushes caveat evaluation down through certain composite iterators
// to allow earlier filtering and better performance.
//
// This optimization transforms:
//
// Caveat(Union[A, B]) -> Union[Caveat(A), B] (if only A contains the caveat)
// Caveat(Union[A, B]) -> Union[Caveat(A), Caveat(B)] (if both contain the caveat)
//
// The pushdown does NOT occur through IntersectionArrow iterators, as they have special
// semantics that require caveat evaluation to happen after the intersection.
func PushdownCaveatEvaluation(c *CaveatIterator) (Iterator, bool, error) {
// Don't push through IntersectionArrow
if _, ok := c.subiterator.(*IntersectionArrow); ok {
return c, false, nil
}

// Don't push down if the subiterator is already a CaveatIterator
// This prevents infinite recursion
if _, ok := c.subiterator.(*CaveatIterator); ok {
return c, false, nil
}

// Get the subiterators of the child
subs := c.subiterator.Subiterators()
if len(subs) == 0 {
// No subiterators to push down into (e.g., leaf iterator)
return c, false, nil
}

// Find which subiterators contain relations with this caveat
newSubs := make([]Iterator, len(subs))
changed := false
for i, sub := range subs {
if containsCaveat(sub, c.caveat) {
// Wrap this subiterator with the caveat
newSubs[i] = NewCaveatIterator(sub, c.caveat)
changed = true
} else {
// Leave unchanged
newSubs[i] = sub
}
}

if !changed {
return c, false, nil

Check warning on line 52 in pkg/query/optimize_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize_caveat.go#L52

Added line #L52 was not covered by tests
}

// Replace the subiterators in the child iterator
newChild, err := c.subiterator.ReplaceSubiterators(newSubs)
if err != nil {
return nil, false, err

Check warning on line 58 in pkg/query/optimize_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize_caveat.go#L58

Added line #L58 was not covered by tests
}

// Return the child without the caveat wrapper
return newChild, true, nil
}

// containsCaveat checks if an iterator tree contains a RelationIterator
// that references the given caveat.
func containsCaveat(it Iterator, caveat *core.ContextualizedCaveat) bool {
found := false
_, err := Walk(it, func(node Iterator) (Iterator, error) {
if rel, ok := node.(*RelationIterator); ok {
if relationContainsCaveat(rel, caveat) {
found = true
}
}
return node, nil
})
if err != nil {
spiceerrors.MustPanicf("should never error -- callback contains no errors, but linters must always check")

Check warning on line 78 in pkg/query/optimize_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize_caveat.go#L78

Added line #L78 was not covered by tests
}

return found
}

// relationContainsCaveat checks if a RelationIterator's base relation
// has a caveat that matches the given caveat name.
func relationContainsCaveat(rel *RelationIterator, caveat *core.ContextualizedCaveat) bool {
if rel.base == nil || caveat == nil {
return false

Check warning on line 88 in pkg/query/optimize_caveat.go

View check run for this annotation

Codecov / codecov/patch

pkg/query/optimize_caveat.go#L88

Added line #L88 was not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code path isn't covered with unit tests

}

// Check if the relation has this caveat
return rel.base.Caveat() == caveat.CaveatName
}
Loading
Loading