From e48a35cf3d45a89657fd89a1eb2cbc3c29e1342f Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 13 Sep 2024 14:35:51 +0200 Subject: [PATCH 1/3] Add grammar utility to extract paths from statements and expressions --- .chloggen/ottl_add_grammar_paths_visitor.yaml | 29 ++ pkg/ottl/grammar.go | 230 +++++---- pkg/ottl/parser_test.go | 171 +++++++ pkg/ottl/paths.go | 32 ++ pkg/ottl/paths_test.go | 450 ++++++++++++++++++ 5 files changed, 831 insertions(+), 81 deletions(-) create mode 100644 .chloggen/ottl_add_grammar_paths_visitor.yaml create mode 100644 pkg/ottl/paths.go create mode 100644 pkg/ottl/paths_test.go diff --git a/.chloggen/ottl_add_grammar_paths_visitor.yaml b/.chloggen/ottl_add_grammar_paths_visitor.yaml new file mode 100644 index 000000000000..11e5b35ae4f6 --- /dev/null +++ b/.chloggen/ottl_add_grammar_paths_visitor.yaml @@ -0,0 +1,29 @@ +# 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: Add grammar utility to extract paths from statements and expressions + +# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. +issues: [29017] + +# (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: | + - The grammar `ottl.path` struct has a new field `Pos lexer.Position`, which value represents the path's token position. + - Grammar's custom errors are now displayed all at once instead of one by one. + +# 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, api] diff --git a/pkg/ottl/grammar.go b/pkg/ottl/grammar.go index 369eff00ecbd..4d47c616a666 100644 --- a/pkg/ottl/grammar.go +++ b/pkg/ottl/grammar.go @@ -5,6 +5,7 @@ package ottl // import "github.com/open-telemetry/opentelemetry-collector-contri import ( "encoding/hex" + "errors" "fmt" "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,14 +48,16 @@ type booleanValue struct { SubExpr *booleanExpression `parser:"| '(' @@ ')' )"` } -func (b *booleanValue) checkForCustomError() error { +func (b *booleanValue) accept(v grammarVisitor) { if b.Comparison != nil { - return b.Comparison.checkForCustomError() + b.Comparison.accept(v) + } + if b.ConstExpr != nil && b.ConstExpr.Converter != nil { + b.ConstExpr.Converter.accept(v) } if b.SubExpr != nil { - return b.SubExpr.checkForCustomError() + b.SubExpr.accept(v) } - return nil } // opAndBooleanValue represents the right side of an AND boolean expression. @@ -63,8 +66,10 @@ 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) + } } // term represents an arbitrary number of boolean values joined by AND. @@ -73,18 +78,15 @@ type term struct { Right []*opAndBooleanValue `parser:"@@*"` } -func (b *term) checkForCustomError() error { - err := b.Left.checkForCustomError() - if err != nil { - return err +func (b *term) accept(v grammarVisitor) { + if b.Left != nil { + b.Left.accept(v) } for _, r := range b.Right { - err = r.checkForCustomError() - if err != nil { - return err + if r != nil { + r.accept(v) } } - return nil } // opOrTerm represents the right side of an OR boolean expression. @@ -93,8 +95,10 @@ 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) + } } // booleanExpression represents a true/false decision expressed @@ -105,17 +109,20 @@ type booleanExpression struct { } func (b *booleanExpression) checkForCustomError() error { - err := b.Left.checkForCustomError() - if err != nil { - return err + validator := &grammarCustomErrorsVisitor{} + b.accept(validator) + return validator.join() +} + +func (b *booleanExpression) accept(v grammarVisitor) { + if b.Left != nil { + b.Left.accept(v) } for _, r := range b.Right { - err = r.checkForCustomError() - if err != nil { - return err + if r != nil { + r.accept(v) } } - return nil } // compareOp is the type of a comparison operator. @@ -178,13 +185,9 @@ 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) } // editor represents the function call of a statement. @@ -195,19 +198,11 @@ type editor struct { Keys []key `parser:"( @@ )*"` } -func (i *editor) checkForCustomError() error { - var err error - +func (i *editor) accept(v grammarVisitor) { + v.visitEditor(i) 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) + arg.accept(v) } - return nil } // converter represents a converter function call. @@ -217,14 +212,22 @@ type converter struct { Keys []key `parser:"( @@ )*"` } +func (c *converter) accept(v grammarVisitor) { + if c.Arguments != nil { + for _, a := range c.Arguments { + a.accept(v) + } + } +} + type argument struct { Name string `parser:"(@(Lowercase(Uppercase | Lowercase)*) Equal)?"` Value value `parser:"( @@"` 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) } // value represents a part of a parsed statement which is resolved to a value of some sort. This can be a telemetry path @@ -241,18 +244,27 @@ type value struct { List *list `parser:"| @@)"` } -func (v *value) checkForCustomError() error { +func (v *value) accept(vis grammarVisitor) { + vis.visitValue(v) if v.Literal != nil { - return v.Literal.checkForCustomError() + v.Literal.accept(vis) } if v.MathExpression != nil { - return v.MathExpression.checkForCustomError() + v.MathExpression.accept(vis) + } + if v.Map != nil { + v.Map.accept(vis) + } + if v.List != nil { + for _, i := range v.List.Values { + i.accept(vis) + } } - return nil } // path represents a telemetry path mathExpression. type path struct { + Pos lexer.Position Context string `parser:"(@Lowercase '.')?"` Fields []field `parser:"@@ ( '.' @@ )*"` } @@ -276,6 +288,14 @@ type mapValue struct { Values []mapItem `parser:"'{' (@@ ','?)* '}'"` } +func (m *mapValue) accept(v grammarVisitor) { + for _, i := range m.Values { + if i.Value != nil { + i.Value.accept(v) + } + } +} + type mapItem struct { Key *string `parser:"@String ':'"` Value *value `parser:"@@"` @@ -319,11 +339,17 @@ type mathExprLiteral struct { Path *path `parser:"| @@ )"` } -func (m *mathExprLiteral) checkForCustomError() error { +func (m *mathExprLiteral) accept(v grammarVisitor) { + v.visitMathExprLiteral(m) + if m.Path != nil { + v.visitPath(m.Path) + } if m.Editor != nil { - return fmt.Errorf("converter names must start with an uppercase letter but got '%v'", m.Editor.Function) + m.Editor.accept(v) + } + if m.Converter != nil { + m.Converter.accept(v) } - return nil } type mathValue struct { @@ -331,11 +357,13 @@ type mathValue struct { SubExpression *mathExpression `parser:"| '(' @@ ')' )"` } -func (m *mathValue) checkForCustomError() error { +func (m *mathValue) accept(v grammarVisitor) { if m.Literal != nil { - return m.Literal.checkForCustomError() + m.Literal.accept(v) + } + if m.SubExpression != nil { + m.SubExpression.accept(v) } - return m.SubExpression.checkForCustomError() } type opMultDivValue struct { @@ -343,8 +371,10 @@ 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) + } } type addSubTerm struct { @@ -352,18 +382,15 @@ type addSubTerm struct { Right []*opMultDivValue `parser:"@@*"` } -func (m *addSubTerm) checkForCustomError() error { - err := m.Left.checkForCustomError() - if err != nil { - return err +func (m *addSubTerm) accept(v grammarVisitor) { + if m.Left != nil { + m.Left.accept(v) } for _, r := range m.Right { - err = r.checkForCustomError() - if err != nil { - return err + if r != nil { + r.accept(v) } } - return nil } type opAddSubTerm struct { @@ -371,8 +398,10 @@ type opAddSubTerm struct { Term *addSubTerm `parser:"@@"` } -func (m *opAddSubTerm) checkForCustomError() error { - return m.Term.checkForCustomError() +func (r *opAddSubTerm) accept(v grammarVisitor) { + if r.Term != nil { + r.Term.accept(v) + } } type mathExpression struct { @@ -380,18 +409,17 @@ type mathExpression struct { Right []*opAddSubTerm `parser:"@@*"` } -func (m *mathExpression) checkForCustomError() error { - err := m.Left.checkForCustomError() - if err != nil { - return err +func (m *mathExpression) accept(v grammarVisitor) { + if m.Left != nil { + m.Left.accept(v) } - for _, r := range m.Right { - err = r.checkForCustomError() - if err != nil { - return err + if m.Right != nil { + for _, r := range m.Right { + if r != nil { + r.accept(v) + } } } - return nil } type mathOp int @@ -464,3 +492,43 @@ func buildLexer() *lexer.StatefulDefinition { {Name: "whitespace", Pattern: `\s+`}, }) } + +// grammarVisitor allows accessing the grammar AST nodes using the visitor pattern. +type grammarVisitor interface { + visitPath(v *path) + visitEditor(v *editor) + visitValue(v *value) + visitMathExprLiteral(v *mathExprLiteral) +} + +// grammarCustomErrorsVisitor is used to execute custom validations on the grammar AST. +type grammarCustomErrorsVisitor struct { + errors []error +} + +func (g *grammarCustomErrorsVisitor) add(err error) { + g.errors = append(g.errors, err) +} + +func (g *grammarCustomErrorsVisitor) join() error { + if len(g.errors) == 0 { + return nil + } + return errors.Join(g.errors...) +} + +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 %v %v", v.Function, 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 d0a5e4b47add..8a6040741f63 100644 --- a/pkg/ottl/parser_test.go +++ b/pkg/ottl/parser_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + "github.com/alecthomas/participle/v2/lexer" "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/component/componenttest" @@ -207,6 +208,11 @@ func Test_parse(t *testing.T) { Value: &value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 38, + Line: 1, + Column: 39, + }, Context: "bear", Fields: []field{ { @@ -267,6 +273,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 24, + Line: 1, + Column: 25, + }, Context: "bear", Fields: []field{ { @@ -298,6 +309,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Context: "foo", Fields: []field{ { @@ -337,6 +353,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Context: "", Fields: []field{ { @@ -373,6 +394,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 16, + Line: 1, + Column: 17, + }, Fields: []field{ { Name: "attributes", @@ -396,6 +422,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 51, + Line: 1, + Column: 52, + }, Fields: []field{ { Name: "attributes", @@ -431,6 +462,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 16, + Line: 1, + Column: 17, + }, Fields: []field{ { Name: "attributes", @@ -464,6 +500,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 16, + Line: 1, + Column: 17, + }, Fields: []field{ { Name: "attributes", @@ -499,6 +540,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Context: "foo", Fields: []field{ { @@ -553,6 +599,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Context: "foo", Fields: []field{ { @@ -585,6 +636,11 @@ func Test_parse(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 44, + Line: 1, + Column: 45, + }, Fields: []field{ { Name: "name", @@ -614,6 +670,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Context: "foo", Fields: []field{ { @@ -646,6 +707,11 @@ func Test_parse(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 44, + Line: 1, + Column: 45, + }, Fields: []field{ { Name: "name", @@ -675,6 +741,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 7, + Line: 1, + Column: 8, + }, Context: "foo", Fields: []field{ { @@ -707,6 +778,11 @@ func Test_parse(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 52, + Line: 1, + Column: 53, + }, Fields: []field{ { Name: "name", @@ -797,6 +873,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -832,6 +913,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -867,6 +953,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -902,6 +993,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -939,6 +1035,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -980,6 +1081,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -1024,6 +1130,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -1095,6 +1206,11 @@ func Test_parse(t *testing.T) { { Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 70, + Line: 1, + Column: 71, + }, Fields: []field{ { Name: "attributes", @@ -1128,6 +1244,11 @@ func Test_parse(t *testing.T) { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "attributes", @@ -1213,6 +1334,11 @@ func Test_parse(t *testing.T) { Left: &mathValue{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 55, + Line: 1, + Column: 56, + }, Fields: []field{ { Name: "three", @@ -1287,6 +1413,11 @@ func Test_parseCondition_full(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 0, + Line: 1, + Column: 1, + }, Fields: []field{ { Name: "name", @@ -1314,6 +1445,11 @@ func Test_parseCondition_full(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 0, + Line: 1, + Column: 1, + }, Fields: []field{ { Name: "name", @@ -1378,6 +1514,11 @@ func Test_parseCondition_full(t *testing.T) { Left: &mathValue{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 13, + Line: 1, + Column: 14, + }, Fields: []field{ { Name: "three", @@ -1477,6 +1618,11 @@ func setNameTest(b *booleanExpression) *parsedStatement { Value: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, Fields: []field{ { Name: "name", @@ -1713,6 +1859,11 @@ func Test_parseWhere(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 24, + Line: 1, + Column: 25, + }, Fields: []field{ { Name: "name", @@ -1735,6 +1886,11 @@ func Test_parseWhere(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 42, + Line: 1, + Column: 43, + }, Fields: []field{ { Name: "name", @@ -1763,6 +1919,11 @@ func Test_parseWhere(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 24, + Line: 1, + Column: 25, + }, Fields: []field{ { Name: "name", @@ -1787,6 +1948,11 @@ func Test_parseWhere(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 41, + Line: 1, + Column: 42, + }, Fields: []field{ { Name: "name", @@ -1839,6 +2005,11 @@ func Test_parseWhere(t *testing.T) { Left: value{ Literal: &mathExprLiteral{ Path: &path{ + Pos: lexer.Position{ + Offset: 28, + Line: 1, + Column: 29, + }, Fields: []field{ { Name: "name", diff --git a/pkg/ottl/paths.go b/pkg/ottl/paths.go new file mode 100644 index 000000000000..dbb66ee7c994 --- /dev/null +++ b/pkg/ottl/paths.go @@ -0,0 +1,32 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package ottl // import "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl" + +// grammarPathVisitor is used to extract all path from a parsedStatement or booleanExpression +type grammarPathVisitor struct { + paths []path +} + +func (v *grammarPathVisitor) visitEditor(_ *editor) {} +func (v *grammarPathVisitor) visitValue(_ *value) {} +func (v *grammarPathVisitor) visitMathExprLiteral(_ *mathExprLiteral) {} + +func (v *grammarPathVisitor) visitPath(value *path) { + v.paths = append(v.paths, *value) +} + +func getParsedStatementPaths(ps *parsedStatement) []path { + visitor := &grammarPathVisitor{} + ps.Editor.accept(visitor) + if ps.WhereClause != nil { + ps.WhereClause.accept(visitor) + } + return visitor.paths +} + +func getBooleanExpressionPaths(be *booleanExpression) []path { + visitor := &grammarPathVisitor{} + be.accept(visitor) + return visitor.paths +} diff --git a/pkg/ottl/paths_test.go b/pkg/ottl/paths_test.go new file mode 100644 index 000000000000..9f31dda15718 --- /dev/null +++ b/pkg/ottl/paths_test.go @@ -0,0 +1,450 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package ottl + +import ( + "testing" + + "github.com/alecthomas/participle/v2/lexer" + "github.com/stretchr/testify/require" + + "github.com/open-telemetry/opentelemetry-collector-contrib/pkg/ottl/ottltest" +) + +func Test_getParsedStatementPaths(t *testing.T) { + tests := []struct { + name string + statement string + expected []path + }{ + { + name: "editor with nested map with path", + statement: `fff({"mapAttr": {"foo": "bar", "get": bear.honey, "arrayAttr":["foo", "bar"]}})`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 38, + Line: 1, + Column: 39, + }, + Context: "bear", + Fields: []field{ + { + Name: "honey", + }, + }, + }, + }, + }, + { + name: "editor with function path parameter", + statement: `set("foo", GetSomething(bear.honey))`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 24, + Line: 1, + Column: 25, + }, + Context: "bear", + Fields: []field{ + { + Name: "honey", + }, + }, + }, + }, + }, + { + name: "path with key", + statement: `set(foo.attributes["bar"].cat, "dog")`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "foo", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + { + Name: "cat", + }, + }, + }, + }, + }, + { + name: "single path field segment", + statement: `set(attributes["bar"], "dog")`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + }, + }, + }, + }, + { + name: "converter parameters", + statement: `replace_pattern(attributes["message"], "device=*", attributes["device_name"], SHA256)`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 16, + Line: 1, + Column: 17, + }, + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("message"), + }, + }, + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 51, + Line: 1, + Column: 52, + }, + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("device_name"), + }, + }, + }, + }, + }, + }, + }, + { + name: "complex path with multiple keys", + statement: `set(foo.bar["x"]["y"].z, Test()[0]["pass"])`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "foo", + Fields: []field{ + { + Name: "bar", + Keys: []key{ + { + String: ottltest.Strp("x"), + }, + { + String: ottltest.Strp("y"), + }, + }, + }, + { + Name: "z", + }, + }, + }, + }, + }, + { + name: "where clause", + statement: `set(foo.attributes["bar"].cat, "dog") where name == "fido"`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "foo", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + { + Name: "cat", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 44, + Line: 1, + Column: 45, + }, + Fields: []field{ + { + Name: "name", + }, + }, + }, + }, + }, + { + name: "where clause multiple conditions", + statement: `set(foo.attributes["bar"].cat, "dog") where name == "fido" and surname == "dido" or surname == "DIDO"`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "foo", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + { + Name: "cat", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 44, + Line: 1, + Column: 45, + }, + Fields: []field{ + { + Name: "name", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 63, + Line: 1, + Column: 64, + }, + Fields: []field{ + { + Name: "surname", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 84, + Line: 1, + Column: 85, + }, + Fields: []field{ + { + Name: "surname", + }, + }, + }, + }, + }, + { + name: "where clause sub expression", + statement: `set(foo.attributes["bar"].cat, "value") where three / (1 + 1) == foo.value`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Context: "foo", + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("bar"), + }, + }, + }, + { + Name: "cat", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 46, + Line: 1, + Column: 47, + }, + Fields: []field{ + { + Name: "three", + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 65, + Line: 1, + Column: 66, + }, + Context: "foo", + Fields: []field{ + { + Name: "value", + }, + }, + }, + }, + }, + { + name: "converter with path list", + statement: `set(attributes["test"], [bear.bear, bear.honey])`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("test"), + }, + }, + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 25, + Line: 1, + Column: 26, + }, + Context: "bear", + Fields: []field{{Name: "bear"}}, + }, + { + Pos: lexer.Position{ + Offset: 36, + Line: 1, + Column: 37, + }, + Context: "bear", + Fields: []field{{Name: "honey"}}, + }, + }, + }, + { + name: "converter math math expression", + statement: `set(attributes["test"], 1000 - 600) where 1 + 1 * 2 == three / One()`, + expected: []path{ + { + Pos: lexer.Position{ + Offset: 4, + Line: 1, + Column: 5, + }, + Fields: []field{ + { + Name: "attributes", + Keys: []key{ + { + String: ottltest.Strp("test"), + }, + }, + }, + }, + }, + { + Pos: lexer.Position{ + Offset: 55, + Line: 1, + Column: 56, + }, + Fields: []field{ + { + Name: "three", + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ps, err := parseStatement(tt.statement) + require.NoError(t, err) + + paths := getParsedStatementPaths(ps) + require.Equal(t, tt.expected, paths) + }) + } +} + +func Test_getBooleanExpressionPaths(t *testing.T) { + expected := []path{ + { + Pos: lexer.Position{ + Offset: 0, + Line: 1, + Column: 1, + }, + Context: "honey", + Fields: []field{{Name: "bear"}}, + }, + { + Pos: lexer.Position{ + Offset: 21, + Line: 1, + Column: 22, + }, + Context: "foo", + Fields: []field{{Name: "bar"}}, + }, + } + + c, err := parseCondition("honey.bear == 1 and (foo.bar == true or 1 == 1)") + require.NoError(t, err) + + paths := getBooleanExpressionPaths(c) + require.Equal(t, expected, paths) +} From 055d362592dc8e27f0b91bd0568f1de43d25f518 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 27 Sep 2024 14:10:17 +0200 Subject: [PATCH 2/3] Revert checkForCustomError changes --- .chloggen/ottl_add_grammar_paths_visitor.yaml | 4 +- pkg/ottl/grammar.go | 187 +++++++++++++----- 2 files changed, 142 insertions(+), 49 deletions(-) diff --git a/.chloggen/ottl_add_grammar_paths_visitor.yaml b/.chloggen/ottl_add_grammar_paths_visitor.yaml index 11e5b35ae4f6..254d1e1ede6e 100644 --- a/.chloggen/ottl_add_grammar_paths_visitor.yaml +++ b/.chloggen/ottl_add_grammar_paths_visitor.yaml @@ -15,9 +15,7 @@ issues: [29017] # (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: | - - The grammar `ottl.path` struct has a new field `Pos lexer.Position`, which value represents the path's token position. - - Grammar's custom errors are now displayed all at once instead of one by one. +subtext: "The grammar `ottl.path` struct has a new field `Pos lexer.Position`, which value represents the path's token position." # 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. diff --git a/pkg/ottl/grammar.go b/pkg/ottl/grammar.go index 4d47c616a666..2407a138c1c2 100644 --- a/pkg/ottl/grammar.go +++ b/pkg/ottl/grammar.go @@ -5,7 +5,6 @@ package ottl // import "github.com/open-telemetry/opentelemetry-collector-contri import ( "encoding/hex" - "errors" "fmt" "github.com/alecthomas/participle/v2/lexer" @@ -20,17 +19,17 @@ type parsedStatement struct { } func (p *parsedStatement) checkForCustomError() error { - validator := &grammarCustomErrorsVisitor{} if p.Converter != nil { - validator.add(fmt.Errorf("editor names must start with a lowercase letter but got '%v'", p.Converter.Function)) + 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 } - - p.Editor.accept(validator) if p.WhereClause != nil { - p.WhereClause.accept(validator) + return p.WhereClause.checkForCustomError() } - - return validator.join() + return nil } type constExpr struct { @@ -48,6 +47,16 @@ 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) @@ -66,6 +75,10 @@ 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) @@ -78,6 +91,20 @@ 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) @@ -95,6 +122,10 @@ 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) @@ -109,9 +140,17 @@ type booleanExpression struct { } func (b *booleanExpression) checkForCustomError() error { - validator := &grammarCustomErrorsVisitor{} - b.accept(validator) - return validator.join() + 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 *booleanExpression) accept(v grammarVisitor) { @@ -185,6 +224,15 @@ 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) @@ -198,6 +246,21 @@ 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 { @@ -226,6 +289,10 @@ 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) } @@ -244,6 +311,16 @@ 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 { @@ -339,6 +416,13 @@ 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 { @@ -357,6 +441,13 @@ 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) @@ -371,6 +462,10 @@ 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) @@ -382,6 +477,20 @@ 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) @@ -398,9 +507,13 @@ type opAddSubTerm struct { Term *addSubTerm `parser:"@@"` } -func (r *opAddSubTerm) accept(v grammarVisitor) { - if r.Term != nil { - r.Term.accept(v) +func (m *opAddSubTerm) checkForCustomError() error { + return m.Term.checkForCustomError() +} + +func (m *opAddSubTerm) accept(v grammarVisitor) { + if m.Term != nil { + m.Term.accept(v) } } @@ -409,6 +522,20 @@ 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) @@ -500,35 +627,3 @@ type grammarVisitor interface { visitValue(v *value) visitMathExprLiteral(v *mathExprLiteral) } - -// grammarCustomErrorsVisitor is used to execute custom validations on the grammar AST. -type grammarCustomErrorsVisitor struct { - errors []error -} - -func (g *grammarCustomErrorsVisitor) add(err error) { - g.errors = append(g.errors, err) -} - -func (g *grammarCustomErrorsVisitor) join() error { - if len(g.errors) == 0 { - return nil - } - return errors.Join(g.errors...) -} - -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 %v %v", v.Function, 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)) - } -} From c3c5fa327c4ba9d0e743a916d0add12c3baeed28 Mon Sep 17 00:00:00 2001 From: edmocosta <11836452+edmocosta@users.noreply.github.com> Date: Fri, 27 Sep 2024 19:03:40 +0200 Subject: [PATCH 3/3] Remove unnecessary changelog --- .chloggen/ottl_add_grammar_paths_visitor.yaml | 27 ------------------- 1 file changed, 27 deletions(-) delete mode 100644 .chloggen/ottl_add_grammar_paths_visitor.yaml diff --git a/.chloggen/ottl_add_grammar_paths_visitor.yaml b/.chloggen/ottl_add_grammar_paths_visitor.yaml deleted file mode 100644 index 254d1e1ede6e..000000000000 --- a/.chloggen/ottl_add_grammar_paths_visitor.yaml +++ /dev/null @@ -1,27 +0,0 @@ -# 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: Add grammar utility to extract paths from statements and expressions - -# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. -issues: [29017] - -# (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: "The grammar `ottl.path` struct has a new field `Pos lexer.Position`, which value represents the path's token position." - -# 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, api]