-
Notifications
You must be signed in to change notification settings - Fork 2
Description
Summary
The main Convert() function doesn't accept a context.Context parameter, preventing cancellation, timeouts, and observability integration.
Location
cel2sql.go:20
Issue
func Convert(ast *cel.Ast, opts ...ConvertOption) (string, error)This prevents:
- Cancellation of long-running conversions
- Timeout protection for complex expressions
- Distributed tracing integration
- Resource cleanup on context cancellation
Inconsistent with pg.TypeProvider.LoadTableSchema() which does use context.
Impact
High - Large or complex CEL expressions could run indefinitely with no way to cancel. Cannot integrate with timeout/cancellation patterns common in web services.
Updated Solution (Using Functional Options)
With the new functional options API (from #62), we can add context support as a non-breaking change using WithContext() option!
Implementation Plan
1. Add Context Option
type convertOptions struct {
schemas map[string]pg.Schema
ctx context.Context // NEW
}
func WithContext(ctx context.Context) ConvertOption {
return func(o *convertOptions) {
o.ctx = ctx
}
}2. Update Converter
type converter struct {
str strings.Builder
typeMap map[int64]*exprpb.Type
schemas map[string]pg.Schema
ctx context.Context // NEW
}3. Add Context Checks
Add helper method and checks at key recursion points:
visit()- main traversal entryvisitCall()- every function callvisitComprehension()- before processing comprehensions- Individual comprehension handlers
func (con *converter) checkContext() error {
if con.ctx == nil {
return nil
}
return con.ctx.Err()
}
func (con *converter) visit(expr *exprpb.Expr) error {
if err := con.checkContext(); err != nil {
return fmt.Errorf("conversion cancelled: %w", err)
}
// ... rest of function
}Usage
Backward compatible - existing code works without changes:
sql, err := cel2sql.Convert(ast)With context - opt-in cancellation support:
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
sql, err := cel2sql.Convert(ast,
cel2sql.WithContext(ctx),
cel2sql.WithSchemas(schemas))Benefits
- ✅ Non-breaking change (unlike original proposal)
- ✅ Opt-in (only pay cost if context provided)
- ✅ Consistent with
WithSchemas()pattern - ✅ No need for separate
ConvertWithContext()function - ✅ No need for v3 major version bump
Files to Change
cel2sql.go- Add option, update converter, add checkscontext_test.go- NEW: Tests for cancellation/timeoutexamples/context/- NEW: Example demonstrating usage
Original Proposal (Superseded)
Add ConvertWithContext() and deprecate Convert(), or increment to v3
The functional options API provides a much cleaner solution!