From 2eda7e9418d3ccda2ed5df4c5740053388a4c59e Mon Sep 17 00:00:00 2001 From: Edmo Vamerlatti Costa <11836452+edmocosta@users.noreply.github.com> Date: Tue, 22 Oct 2024 20:02:15 +0200 Subject: [PATCH] [pkg/ottl] Refactor grammar validation to use the AST visitor and combining errors (#35728) Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> --- .../ottl-grammar-custom-errors-visitor.yaml | 27 +++ pkg/ottl/functions.go | 28 ++- pkg/ottl/grammar.go | 215 ++++++------------ pkg/ottl/parser_test.go | 211 +++++++++-------- 4 files changed, 235 insertions(+), 246 deletions(-) create mode 100644 .chloggen/ottl-grammar-custom-errors-visitor.yaml diff --git a/.chloggen/ottl-grammar-custom-errors-visitor.yaml b/.chloggen/ottl-grammar-custom-errors-visitor.yaml new file mode 100644 index 000000000000..252cb7d3f14b --- /dev/null +++ b/.chloggen/ottl-grammar-custom-errors-visitor.yaml @@ -0,0 +1,27 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) +component: pkg/ottl + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: "Parsing invalid statements and conditions now prints all errors instead of just the first one found." + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [35728] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# If your change doesn't affect end users or the exported elements of any package, +# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [user] diff --git a/pkg/ottl/functions.go b/pkg/ottl/functions.go index 251e79587b75..4ff92123c7e6 100644 --- a/pkg/ottl/functions.go +++ b/pkg/ottl/functions.go @@ -33,16 +33,7 @@ func buildOriginalText(path *path) string { for i, f := range path.Fields { builder.WriteString(f.Name) if len(f.Keys) > 0 { - for _, k := range f.Keys { - builder.WriteString("[") - if k.Int != nil { - builder.WriteString(strconv.FormatInt(*k.Int, 10)) - } - if k.String != nil { - builder.WriteString(*k.String) - } - builder.WriteString("]") - } + builder.WriteString(buildOriginalKeysText(f.Keys)) } if i != len(path.Fields)-1 { builder.WriteString(".") @@ -51,6 +42,23 @@ func buildOriginalText(path *path) string { return builder.String() } +func buildOriginalKeysText(keys []key) string { + var builder strings.Builder + if len(keys) > 0 { + for _, k := range keys { + builder.WriteString("[") + if k.Int != nil { + builder.WriteString(strconv.FormatInt(*k.Int, 10)) + } + if k.String != nil { + builder.WriteString(*k.String) + } + builder.WriteString("]") + } + } + return builder.String() +} + func (p *Parser[K]) newPath(path *path) (*basePath[K], error) { if len(path.Fields) == 0 { return nil, fmt.Errorf("cannot make a path from zero fields") diff --git a/pkg/ottl/grammar.go b/pkg/ottl/grammar.go index 2407a138c1c2..a1e5eb53a81d 100644 --- a/pkg/ottl/grammar.go +++ b/pkg/ottl/grammar.go @@ -6,6 +6,7 @@ package ottl // import "github.com/open-telemetry/opentelemetry-collector-contri import ( "encoding/hex" "fmt" + "strings" "github.com/alecthomas/participle/v2/lexer" ) @@ -19,17 +20,17 @@ type parsedStatement struct { } func (p *parsedStatement) checkForCustomError() error { + validator := &grammarCustomErrorsVisitor{} if p.Converter != nil { - return fmt.Errorf("editor names must start with a lowercase letter but got '%v'", p.Converter.Function) - } - err := p.Editor.checkForCustomError() - if err != nil { - return err + validator.add(fmt.Errorf("editor names must start with a lowercase letter but got '%v'", p.Converter.Function)) } + + p.Editor.accept(validator) if p.WhereClause != nil { - return p.WhereClause.checkForCustomError() + p.WhereClause.accept(validator) } - return nil + + return validator.join() } type constExpr struct { @@ -47,16 +48,6 @@ type booleanValue struct { SubExpr *booleanExpression `parser:"| '(' @@ ')' )"` } -func (b *booleanValue) checkForCustomError() error { - if b.Comparison != nil { - return b.Comparison.checkForCustomError() - } - if b.SubExpr != nil { - return b.SubExpr.checkForCustomError() - } - return nil -} - func (b *booleanValue) accept(v grammarVisitor) { if b.Comparison != nil { b.Comparison.accept(v) @@ -75,10 +66,6 @@ type opAndBooleanValue struct { Value *booleanValue `parser:"@@"` } -func (b *opAndBooleanValue) checkForCustomError() error { - return b.Value.checkForCustomError() -} - func (b *opAndBooleanValue) accept(v grammarVisitor) { if b.Value != nil { b.Value.accept(v) @@ -91,20 +78,6 @@ type term struct { Right []*opAndBooleanValue `parser:"@@*"` } -func (b *term) checkForCustomError() error { - err := b.Left.checkForCustomError() - if err != nil { - return err - } - for _, r := range b.Right { - err = r.checkForCustomError() - if err != nil { - return err - } - } - return nil -} - func (b *term) accept(v grammarVisitor) { if b.Left != nil { b.Left.accept(v) @@ -122,10 +95,6 @@ type opOrTerm struct { Term *term `parser:"@@"` } -func (b *opOrTerm) checkForCustomError() error { - return b.Term.checkForCustomError() -} - func (b *opOrTerm) accept(v grammarVisitor) { if b.Term != nil { b.Term.accept(v) @@ -140,17 +109,9 @@ type booleanExpression struct { } func (b *booleanExpression) checkForCustomError() error { - err := b.Left.checkForCustomError() - if err != nil { - return err - } - for _, r := range b.Right { - err = r.checkForCustomError() - if err != nil { - return err - } - } - return nil + validator := &grammarCustomErrorsVisitor{} + b.accept(validator) + return validator.join() } func (b *booleanExpression) accept(v grammarVisitor) { @@ -224,15 +185,6 @@ type comparison struct { Right value `parser:"@@"` } -func (c *comparison) checkForCustomError() error { - err := c.Left.checkForCustomError() - if err != nil { - return err - } - err = c.Right.checkForCustomError() - return err -} - func (c *comparison) accept(v grammarVisitor) { c.Left.accept(v) c.Right.accept(v) @@ -246,21 +198,6 @@ type editor struct { Keys []key `parser:"( @@ )*"` } -func (i *editor) checkForCustomError() error { - var err error - - for _, arg := range i.Arguments { - err = arg.checkForCustomError() - if err != nil { - return err - } - } - if i.Keys != nil { - return fmt.Errorf("only paths and converters may be indexed, not editors, but got %v %v", i.Function, i.Keys) - } - return nil -} - func (i *editor) accept(v grammarVisitor) { v.visitEditor(i) for _, arg := range i.Arguments { @@ -289,10 +226,6 @@ type argument struct { FunctionName *string `parser:"| @(Uppercase(Uppercase | Lowercase)*) )"` } -func (a *argument) checkForCustomError() error { - return a.Value.checkForCustomError() -} - func (a *argument) accept(v grammarVisitor) { a.Value.accept(v) } @@ -311,16 +244,6 @@ type value struct { List *list `parser:"| @@)"` } -func (v *value) checkForCustomError() error { - if v.Literal != nil { - return v.Literal.checkForCustomError() - } - if v.MathExpression != nil { - return v.MathExpression.checkForCustomError() - } - return nil -} - func (v *value) accept(vis grammarVisitor) { vis.visitValue(v) if v.Literal != nil { @@ -416,13 +339,6 @@ type mathExprLiteral struct { Path *path `parser:"| @@ )"` } -func (m *mathExprLiteral) checkForCustomError() error { - if m.Editor != nil { - return fmt.Errorf("converter names must start with an uppercase letter but got '%v'", m.Editor.Function) - } - return nil -} - func (m *mathExprLiteral) accept(v grammarVisitor) { v.visitMathExprLiteral(m) if m.Path != nil { @@ -441,13 +357,6 @@ type mathValue struct { SubExpression *mathExpression `parser:"| '(' @@ ')' )"` } -func (m *mathValue) checkForCustomError() error { - if m.Literal != nil { - return m.Literal.checkForCustomError() - } - return m.SubExpression.checkForCustomError() -} - func (m *mathValue) accept(v grammarVisitor) { if m.Literal != nil { m.Literal.accept(v) @@ -462,10 +371,6 @@ type opMultDivValue struct { Value *mathValue `parser:"@@"` } -func (m *opMultDivValue) checkForCustomError() error { - return m.Value.checkForCustomError() -} - func (m *opMultDivValue) accept(v grammarVisitor) { if m.Value != nil { m.Value.accept(v) @@ -477,20 +382,6 @@ type addSubTerm struct { Right []*opMultDivValue `parser:"@@*"` } -func (m *addSubTerm) checkForCustomError() error { - err := m.Left.checkForCustomError() - if err != nil { - return err - } - for _, r := range m.Right { - err = r.checkForCustomError() - if err != nil { - return err - } - } - return nil -} - func (m *addSubTerm) accept(v grammarVisitor) { if m.Left != nil { m.Left.accept(v) @@ -507,13 +398,9 @@ type opAddSubTerm struct { Term *addSubTerm `parser:"@@"` } -func (m *opAddSubTerm) checkForCustomError() error { - return m.Term.checkForCustomError() -} - -func (m *opAddSubTerm) accept(v grammarVisitor) { - if m.Term != nil { - m.Term.accept(v) +func (r *opAddSubTerm) accept(v grammarVisitor) { + if r.Term != nil { + r.Term.accept(v) } } @@ -522,20 +409,6 @@ type mathExpression struct { Right []*opAddSubTerm `parser:"@@*"` } -func (m *mathExpression) checkForCustomError() error { - err := m.Left.checkForCustomError() - if err != nil { - return err - } - for _, r := range m.Right { - err = r.checkForCustomError() - if err != nil { - return err - } - } - return nil -} - func (m *mathExpression) accept(v grammarVisitor) { if m.Left != nil { m.Left.accept(v) @@ -620,6 +493,34 @@ func buildLexer() *lexer.StatefulDefinition { }) } +// grammarCustomError represents a grammar error in which the statement has a valid syntax +// according to the grammar's definition, but is still logically invalid. +type grammarCustomError struct { + errs []error +} + +// Error returns all errors messages separate by semicolons. +func (e *grammarCustomError) Error() string { + switch len(e.errs) { + case 0: + return "" + case 1: + return e.errs[0].Error() + default: + var b strings.Builder + b.WriteString(e.errs[0].Error()) + for _, err := range e.errs[1:] { + b.WriteString("; ") + b.WriteString(err.Error()) + } + return b.String() + } +} + +func (e *grammarCustomError) Unwrap() []error { + return e.errs +} + // grammarVisitor allows accessing the grammar AST nodes using the visitor pattern. type grammarVisitor interface { visitPath(v *path) @@ -627,3 +528,35 @@ type grammarVisitor interface { visitValue(v *value) visitMathExprLiteral(v *mathExprLiteral) } + +// grammarCustomErrorsVisitor is used to execute custom validations on the grammar AST. +type grammarCustomErrorsVisitor struct { + errs []error +} + +func (g *grammarCustomErrorsVisitor) add(err error) { + g.errs = append(g.errs, err) +} + +func (g *grammarCustomErrorsVisitor) join() error { + if len(g.errs) == 0 { + return nil + } + return &grammarCustomError{errs: g.errs} +} + +func (g *grammarCustomErrorsVisitor) visitPath(_ *path) {} + +func (g *grammarCustomErrorsVisitor) visitValue(_ *value) {} + +func (g *grammarCustomErrorsVisitor) visitEditor(v *editor) { + if v.Keys != nil { + g.add(fmt.Errorf("only paths and converters may be indexed, not editors, but got %s%s", v.Function, buildOriginalKeysText(v.Keys))) + } +} + +func (g *grammarCustomErrorsVisitor) visitMathExprLiteral(v *mathExprLiteral) { + if v.Editor != nil { + g.add(fmt.Errorf("converter names must start with an uppercase letter but got '%v'", v.Editor.Function)) + } +} diff --git a/pkg/ottl/parser_test.go b/pkg/ottl/parser_test.go index 8a6040741f63..e8bb93af6f9b 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -14,6 +14,7 @@ import ( "github.com/alecthomas/participle/v2/lexer" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component/componenttest" "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" @@ -2193,71 +2194,81 @@ func Test_ParseConditions_Error(t *testing.T) { // This test doesn't validate parser results, simply checks whether the parse succeeds or not. // It's a fast way to check a large range of possible syntaxes. func Test_parseStatement(t *testing.T) { + converterNameErrorPrefix := "converter names must start with an uppercase letter" + editorWithIndexErrorPrefix := "only paths and converters may be indexed" + tests := []struct { - statement string - wantErr bool + statement string + wantErr bool + wantErrContaining string }{ - {`set(`, true}, - {`set("foo)`, true}, - {`set(name.)`, true}, - {`("foo")`, true}, - {`set("foo") where name =||= "fido"`, true}, - {`set(span_id, SpanIDWrapper{not a hex string})`, true}, - {`set(span_id, SpanIDWrapper{01})`, true}, - {`set(span_id, SpanIDWrapper{010203040506070809})`, true}, - {`set(trace_id, TraceIDWrapper{not a hex string})`, true}, - {`set(trace_id, TraceIDWrapper{0102030405060708090a0b0c0d0e0f})`, true}, - {`set(trace_id, TraceIDWrapper{0102030405060708090a0b0c0d0e0f1011})`, true}, - {`set("foo") where name = "fido"`, true}, - {`set("foo") where name or "fido"`, true}, - {`set("foo") where name and "fido"`, true}, - {`set("foo") where name and`, true}, - {`set("foo") where name or`, true}, - {`set("foo") where (`, true}, - {`set("foo") where )`, true}, - {`set("foo") where (name == "fido"))`, true}, - {`set("foo") where ((name == "fido")`, true}, - {`Set()`, true}, - {`set(int())`, true}, - {`set(1 + int())`, true}, - {`set(int() + 1)`, true}, - {`set(1 * int())`, true}, - {`set(1 * 1 + (2 * int()))`, true}, - {`set() where int() == 1`, true}, - {`set() where 1 == int()`, true}, - {`set() where true and 1 == int() `, true}, - {`set() where false or 1 == int() `, true}, - {`set(foo.attributes["bar"].cat, "dog")`, false}, - {`set(set = foo.attributes["animal"], val = "dog") where animal == "cat"`, false}, - {`test() where service == "pinger" or foo.attributes["endpoint"] == "/x/alive"`, false}, - {`test() where service == "pinger" or foo.attributes["verb"] == "GET" and foo.attributes["endpoint"] == "/x/alive"`, false}, - {`test() where animal > "cat"`, false}, - {`test() where animal >= "cat"`, false}, - {`test() where animal <= "cat"`, false}, - {`test() where animal < "cat"`, false}, - {`test() where animal =< "dog"`, true}, - {`test() where animal => "dog"`, true}, - {`test() where animal <> "dog"`, true}, - {`test() where animal = "dog"`, true}, - {`test() where animal`, true}, - {`test() where animal ==`, true}, - {`test() where ==`, true}, - {`test() where == animal`, true}, - {`test() where attributes["path"] == "/healthcheck"`, false}, - {`test() where one() == 1`, true}, - {`test(fail())`, true}, - {`Test()`, true}, + {statement: `set(`, wantErr: true}, + {statement: `set("foo)`, wantErr: true}, + {statement: `set(name.)`, wantErr: true}, + {statement: `("foo")`, wantErr: true}, + {statement: `set("foo") where name =||= "fido"`, wantErr: true}, + {statement: `set(span_id, SpanIDWrapper{not a hex string})`, wantErr: true}, + {statement: `set(span_id, SpanIDWrapper{01})`, wantErr: true}, + {statement: `set(span_id, SpanIDWrapper{010203040506070809})`, wantErr: true}, + {statement: `set(trace_id, TraceIDWrapper{not a hex string})`, wantErr: true}, + {statement: `set(trace_id, TraceIDWrapper{0102030405060708090a0b0c0d0e0f})`, wantErr: true}, + {statement: `set(trace_id, TraceIDWrapper{0102030405060708090a0b0c0d0e0f1011})`, wantErr: true}, + {statement: `set("foo") where name = "fido"`, wantErr: true}, + {statement: `set("foo") where name or "fido"`, wantErr: true}, + {statement: `set("foo") where name and "fido"`, wantErr: true}, + {statement: `set("foo") where name and`, wantErr: true}, + {statement: `set("foo") where name or`, wantErr: true}, + {statement: `set("foo") where (`, wantErr: true}, + {statement: `set("foo") where )`, wantErr: true}, + {statement: `set("foo") where (name == "fido"))`, wantErr: true}, + {statement: `set("foo") where ((name == "fido")`, wantErr: true}, + {statement: `Set()`, wantErr: true}, + {statement: `set(int())`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set(1 + int())`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set(int() + 1)`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set(1 * int())`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set(1 * 1 + (2 * int()))`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set() where int() == 1`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set() where 1 == int()`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set() where true and 1 == int() `, wantErrContaining: converterNameErrorPrefix}, + {statement: `set() where false or 1 == int() `, wantErrContaining: converterNameErrorPrefix}, + {statement: `set(foo.attributes["bar"].cat)["key"]`, wantErrContaining: editorWithIndexErrorPrefix}, + {statement: `set(foo.attributes["bar"].cat, "dog")`}, + {statement: `set(set = foo.attributes["animal"], val = "dog") where animal == "cat"`}, + {statement: `test() where service == "pinger" or foo.attributes["endpoint"] == "/x/alive"`}, + {statement: `test() where service == "pinger" or foo.attributes["verb"] == "GET" and foo.attributes["endpoint"] == "/x/alive"`}, + {statement: `test() where animal > "cat"`}, + {statement: `test() where animal >= "cat"`}, + {statement: `test() where animal <= "cat"`}, + {statement: `test() where animal < "cat"`}, + {statement: `test() where animal =< "dog"`, wantErr: true}, + {statement: `test() where animal => "dog"`, wantErr: true}, + {statement: `test() where animal <> "dog"`, wantErr: true}, + {statement: `test() where animal = "dog"`, wantErr: true}, + {statement: `test() where animal`, wantErr: true}, + {statement: `test() where animal ==`, wantErr: true}, + {statement: `test() where ==`, wantErr: true}, + {statement: `test() where == animal`, wantErr: true}, + {statement: `test() where attributes["path"] == "/healthcheck"`}, + {statement: `test() where one() == 1`, wantErr: true}, + {statement: `test(fail())`, wantErrContaining: converterNameErrorPrefix}, + {statement: `Test()`, wantErr: true}, + {statement: `set() where test(foo)["key"] == "bar"`, wantErrContaining: converterNameErrorPrefix}, + {statement: `set() where test(foo)["key"] == "bar"`, wantErrContaining: editorWithIndexErrorPrefix}, } pat := regexp.MustCompile("[^a-zA-Z0-9]+") for _, tt := range tests { name := pat.ReplaceAllString(tt.statement, "_") t.Run(name, func(t *testing.T) { ast, err := parseStatement(tt.statement) - if (err != nil) != tt.wantErr { - t.Errorf("parseStatement(%s) error = %v, wantErr %v", tt.statement, err, tt.wantErr) + if (err != nil) != (tt.wantErr || tt.wantErrContaining != "") { + t.Errorf("parseStatement(%s) error = %v, wantErr %v, wantErrContaining %v", tt.statement, err, tt.wantErr, tt.wantErrContaining) t.Errorf("AST: %+v", ast) return } + if tt.wantErrContaining != "" { + require.ErrorContains(t, err, tt.wantErrContaining) + } }) } } @@ -2265,59 +2276,69 @@ func Test_parseStatement(t *testing.T) { // This test doesn't validate parser results, simply checks whether the parse succeeds or not. // It's a fast way to check a large range of possible syntaxes. func Test_parseCondition(t *testing.T) { + converterNameErrorPrefix := "converter names must start with an uppercase letter" + editorWithIndexErrorPrefix := "only paths and converters may be indexed" + tests := []struct { - condition string - wantErr bool + condition string + wantErr bool + wantErrContaining string }{ - {`set(`, true}, - {`set("foo)`, true}, - {`set(name.)`, true}, - {`("foo")`, true}, - {`name =||= "fido"`, true}, - {`name = "fido"`, true}, - {`name or "fido"`, true}, - {`name and "fido"`, true}, - {`name and`, true}, - {`name or`, true}, - {`(`, true}, - {`)`, true}, - {`(name == "fido"))`, true}, - {`((name == "fido")`, true}, - {`set()`, true}, - {`Int() == 1`, false}, - {`1 == Int()`, false}, - {`true and 1 == Int() `, false}, - {`false or 1 == Int() `, false}, - {`service == "pinger" or foo.attributes["endpoint"] == "/x/alive"`, false}, - {`service == "pinger" or foo.attributes["verb"] == "GET" and foo.attributes["endpoint"] == "/x/alive"`, false}, - {`animal > "cat"`, false}, - {`animal >= "cat"`, false}, - {`animal <= "cat"`, false}, - {`animal < "cat"`, false}, - {`animal =< "dog"`, true}, - {`animal => "dog"`, true}, - {`animal <> "dog"`, true}, - {`animal = "dog"`, true}, - {`animal`, true}, - {`animal ==`, true}, - {`==`, true}, - {`== animal`, true}, - {`attributes["path"] == "/healthcheck"`, false}, - {`One() == 1`, false}, - {`test(fail())`, true}, - {`Test()`, false}, - {`"test" == Foo`, true}, + {condition: `set(`, wantErr: true}, + {condition: `set("foo)`, wantErr: true}, + {condition: `set(name.)`, wantErr: true}, + {condition: `("foo")`, wantErr: true}, + {condition: `name =||= "fido"`, wantErr: true}, + {condition: `name = "fido"`, wantErr: true}, + {condition: `name or "fido"`, wantErr: true}, + {condition: `name and "fido"`, wantErr: true}, + {condition: `name and`, wantErr: true}, + {condition: `name or`, wantErr: true}, + {condition: `(`, wantErr: true}, + {condition: `)`, wantErr: true}, + {condition: `(name == "fido"))`, wantErr: true}, + {condition: `((name == "fido")`, wantErr: true}, + {condition: `set()`, wantErr: true}, + {condition: `Int() == 1`}, + {condition: `1 == Int()`}, + {condition: `true and 1 == Int() `}, + {condition: `false or 1 == Int() `}, + {condition: `service == "pinger" or foo.attributes["endpoint"] == "/x/alive"`}, + {condition: `service == "pinger" or foo.attributes["verb"] == "GET" and foo.attributes["endpoint"] == "/x/alive"`}, + {condition: `animal > "cat"`}, + {condition: `animal >= "cat"`}, + {condition: `animal <= "cat"`}, + {condition: `animal < "cat"`}, + {condition: `animal =< "dog"`, wantErr: true}, + {condition: `animal => "dog"`, wantErr: true}, + {condition: `animal <> "dog"`, wantErr: true}, + {condition: `animal = "dog"`, wantErr: true}, + {condition: `animal`, wantErr: true}, + {condition: `animal ==`, wantErr: true}, + {condition: `==`, wantErr: true}, + {condition: `== animal`, wantErr: true}, + {condition: `attributes["path"] == "/healthcheck"`}, + {condition: `One() == 1`}, + {condition: `test(fail())`, wantErr: true}, + {condition: `Test()`}, + {condition: `"test" == Foo`, wantErr: true}, + {condition: `test(animal) == "dog"`, wantErrContaining: converterNameErrorPrefix}, + {condition: `test(animal)["kind"] == "birds"`, wantErrContaining: converterNameErrorPrefix}, + {condition: `test(animal)["kind"] == "birds"`, wantErrContaining: editorWithIndexErrorPrefix}, } pat := regexp.MustCompile("[^a-zA-Z0-9]+") for _, tt := range tests { name := pat.ReplaceAllString(tt.condition, "_") t.Run(name, func(t *testing.T) { ast, err := parseCondition(tt.condition) - if (err != nil) != tt.wantErr { + if (err != nil) != (tt.wantErr || tt.wantErrContaining != "") { t.Errorf("parseCondition(%s) error = %v, wantErr %v", tt.condition, err, tt.wantErr) t.Errorf("AST: %+v", ast) return } + if tt.wantErrContaining != "" { + require.ErrorContains(t, err, tt.wantErrContaining) + } }) } }