diff --git a/pkg/qlng/ast_nodes.go b/pkg/qlng/ast_nodes.go index 065a6a9f6..ef30197ca 100644 --- a/pkg/qlng/ast_nodes.go +++ b/pkg/qlng/ast_nodes.go @@ -265,10 +265,14 @@ func (nn parserNodes) ToAST() (out *ASTNode) { auxArgs = aux } - return &ASTNode{ - Ref: "group", - Args: auxArgs, + if len(auxArgs) > 1 { + return &ASTNode{ + Ref: "group", + Args: auxArgs, + } } + + return auxArgs[0] } // A simplified type registry for the types that QL needs to understand diff --git a/pkg/report/frame.go b/pkg/report/frame.go index 0ab128dbc..8355cf5d7 100644 --- a/pkg/report/frame.go +++ b/pkg/report/frame.go @@ -1,6 +1,7 @@ package report import ( + "bytes" "encoding/json" "fmt" "strings" @@ -108,28 +109,6 @@ func MakeColumnOfKind(k string) *FrameColumn { } } -func KindOf(v expr.TypedValue) string { - // @todo ... - if v == nil { - return "String" - } - - switch v.Type() { - case "Integer", - "UnsignedInteger", - "Float": - return "Number" - case "DateTime": - return "DateTime" - case "ID": - return "Ref" - case "Boolean": - return "Checkbox" - default: - return "String" - } -} - func (f *Filter) UnmarshalJSON(data []byte) (err error) { var aux interface{} if err = json.Unmarshal(data, &aux); err != nil { @@ -152,6 +131,11 @@ func (f *Filter) UnmarshalJSON(data []byte) (err error) { return } + // special case for empty JSON + if bytes.Equal([]byte{'{', '}'}, data) { + return + } + // non-string is considered an AST and we parse that if err = json.Unmarshal(data, &f.ASTNode); err != nil { return @@ -169,6 +153,7 @@ func (f *Filter) UnmarshalJSON(data []byte) (err error) { } aux, err := p.Parse(n.Raw) + aux.Raw = n.Raw if err != nil { return false, n, err } diff --git a/pkg/report/step_group.go b/pkg/report/step_group.go index c80113bb4..b48025afb 100644 --- a/pkg/report/step_group.go +++ b/pkg/report/step_group.go @@ -1,6 +1,7 @@ package report import ( + "bytes" "context" "encoding/json" "errors" @@ -120,6 +121,11 @@ func (def *colDef) UnmarshalJSON(data []byte) (err error) { return } + // special case for empty JSON + if bytes.Equal([]byte{'{', '}'}, data) { + return + } + // non-string is considered an AST and we parse that if err = json.Unmarshal(data, &def.ASTNode); err != nil { return @@ -137,6 +143,7 @@ func (def *colDef) UnmarshalJSON(data []byte) (err error) { } aux, err := p.Parse(n.Raw) + aux.Raw = n.Raw if err != nil { return false, n, err } diff --git a/store/rdbms/ast_transformer.go b/store/rdbms/ast_transformer.go index c905331e7..6a2387d2c 100644 --- a/store/rdbms/ast_transformer.go +++ b/store/rdbms/ast_transformer.go @@ -20,8 +20,8 @@ type ( ResultType string } - ASTFormatterFn func(n *qlng.ASTNode, aa ...FormattedASTArgs) (bool, string, []interface{}, error) - HandlerSig func(aa ...FormattedASTArgs) (string, []interface{}, error) + ASTFormatterFn func(n *qlng.ASTNode) HandlerSig + HandlerSig func(aa ...FormattedASTArgs) (string, []interface{}, bool, error) exprHandler struct { Args argSet @@ -185,8 +185,8 @@ var ( // generic stuff "null": { Result: wrapRes("Null"), - Handler: func(aa ...FormattedASTArgs) (string, []interface{}, error) { - return "NULL", nil, nil + Handler: func(aa ...FormattedASTArgs) (string, []interface{}, bool, error) { + return "NULL", nil, true, nil }, }, @@ -218,10 +218,10 @@ func (t *astTransformer) SetPlaceholder(use bool) { // ToSql conforms the struct to squirrel allowing trivial RDBMS use func (t *astTransformer) ToSql() (string, []interface{}, error) { - return t.toSql(t.root) + return t.toSql(t.root, true) } -func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) { +func (t *astTransformer) toSql(n *qlng.ASTNode, isRoot bool) (string, []interface{}, error) { // Leaf edge-cases switch { case n.Symbol != "": @@ -233,7 +233,7 @@ func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) { // Process arguments for the op. args := make([]FormattedASTArgs, len(n.Args)) for i, a := range n.Args { - s, pp, err := t.toSql(a) + s, pp, err := t.toSql(a, false) if err != nil { return "", nil, err } @@ -250,21 +250,29 @@ func (t *astTransformer) toSql(n *qlng.ASTNode) (string, []interface{}, error) { continue } - if ok, s, args, err := c(n, args...); ok { + if h := c(n); h != nil { + s, args, encl, err := h(args...) + if !isRoot && !encl && len(args) > 1 { + s = "(" + s + ")" + } return s, args, err } } if n.Ref == "group" { - return bracketHandler(args...) + s, args, _, err := bracketHandler(args...) + return s, args, err } // Default handlers if e, ok := sqlExprRegistry[n.Ref]; !ok { return "", nil, fmt.Errorf("unknown expression: handler not defined: %s", n.Ref) } else { - s, args, err := e.Handler(args...) - return s, args, err + s, _args, encl, err := e.Handler(args...) + if !isRoot && !encl && len(args) > 1 { + s = "(" + s + ")" + } + return s, _args, err } } diff --git a/store/rdbms/ast_transformer_handlers.go b/store/rdbms/ast_transformer_handlers.go index 1b5d9d8f2..85cc0a66a 100644 --- a/store/rdbms/ast_transformer_handlers.go +++ b/store/rdbms/ast_transformer_handlers.go @@ -48,7 +48,12 @@ func (t *astTransformer) handleValue(n *qlng.ASTNode) (string, []interface{}, er } func makeGenericBoolHandler(op string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + if len(aa) < 1 { + err = fmt.Errorf("expecting 1 or more arguments, got %d", len(aa)) + return + } + outPts := make([]string, len(aa)) args = make([]interface{}, 0, 10) for i, a := range aa { @@ -62,7 +67,9 @@ func makeGenericBoolHandler(op string) HandlerSig { } func makeGenericBracketHandler(bb ...string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + if len(aa) != 1 { err = fmt.Errorf("expecting 1 argument, got %d", len(aa)) return @@ -75,7 +82,9 @@ func makeGenericBracketHandler(bb ...string) HandlerSig { } func makeGenericCompHandler(comp string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + if len(aa) != 2 { err = fmt.Errorf("expecting 2 arguments, got %d", len(aa)) return @@ -88,7 +97,9 @@ func makeGenericCompHandler(comp string) HandlerSig { } func makeGenericAggFncHandler(fnc string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + if fnc == "COUNT" && len(aa) == 0 { out = "COUNT(*)" return @@ -106,7 +117,9 @@ func makeGenericAggFncHandler(fnc string) HandlerSig { } func makeGenericTypecastHandler(t string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + if len(aa) != 1 { err = fmt.Errorf("expecting 1 argument, got %d", len(aa)) return @@ -119,9 +132,11 @@ func makeGenericTypecastHandler(t string) HandlerSig { } func makeGenericFilterFncHandler(fnc string) HandlerSig { - return func(aa ...FormattedASTArgs) (out string, args []interface{}, err error) { + return func(aa ...FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { + selfEnclosed = true + if len(aa) == 0 { - return fmt.Sprintf("%s()", fnc), nil, nil + return fmt.Sprintf("%s()", fnc), nil, selfEnclosed, nil } args = make([]interface{}, 0, len(aa)) diff --git a/store/rdbms/compose_record_datasource.go b/store/rdbms/compose_record_datasource.go index 6ead6139b..3b95dc355 100644 --- a/store/rdbms/compose_record_datasource.go +++ b/store/rdbms/compose_record_datasource.go @@ -196,7 +196,7 @@ func (r *recordDatasource) Group(d report.GroupDefinition, name string) (bool, e q = q.Column(squirrel.Alias(tr, c.Name)) } - if d.Filter != nil { + if d.Filter != nil && d.Filter.ASTNode != nil { q = q.Having(r.store.ASTTransformer(d.Filter.ASTNode)) } @@ -260,12 +260,12 @@ func (r *recordDatasource) preloadQuery(def *report.FrameDefinition) (squirrel.S // when filtering/sorting, wrap the base query in a sub-select, so we don't need to // worry about exact column names. - if def.Filter != nil || def.Sort != nil { + if def.Filter != nil && def.Filter.ASTNode != nil || def.Sort != nil { q = squirrel.Select("*").FromSelect(q, "w_base") } // - filtering - if def.Filter != nil { + if def.Filter != nil && def.Filter.ASTNode != nil { q = q.Where(r.store.ASTTransformer(def.Filter.ASTNode)) } @@ -430,7 +430,9 @@ func (r *recordDatasource) baseQuery(f *report.Filter) (sqb squirrel.SelectBuild return } - sqb = sqb.Where(r.store.ASTTransformer(f.ASTNode)) + if f != nil && f.ASTNode != nil { + sqb = sqb.Where(r.store.ASTTransformer(f.ASTNode)) + } } return sqb, nil diff --git a/store/sqlite3/sql_functions.go b/store/sqlite3/sql_functions.go index 7983531ab..8193ab6b5 100644 --- a/store/sqlite3/sql_functions.go +++ b/store/sqlite3/sql_functions.go @@ -13,7 +13,7 @@ var ( sqlExprRegistry = map[string]rdbms.HandlerSig{ // functions // - filtering - "now": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) { + "now": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { if len(aa) != 0 { err = fmt.Errorf("expecting 0 arguments, got %d", len(aa)) return @@ -22,7 +22,7 @@ var ( out = "DATE('now')" return }, - "quarter": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) { + "quarter": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { if len(aa) != 1 { err = fmt.Errorf("expecting 1 arguments, got %d", len(aa)) return @@ -32,7 +32,7 @@ var ( args = aa[0].Args return }, - "year": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) { + "year": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { if len(aa) != 1 { err = fmt.Errorf("expecting 1 arguments, got %d", len(aa)) return @@ -42,7 +42,7 @@ var ( args = aa[0].Args return }, - "date": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, err error) { + "date": func(aa ...rdbms.FormattedASTArgs) (out string, args []interface{}, selfEnclosed bool, err error) { if len(aa) != 1 { err = fmt.Errorf("expecting 1 arguments, got %d", len(aa)) return @@ -55,14 +55,8 @@ var ( } ) -func sqlASTFormatter(n *qlng.ASTNode, aa ...rdbms.FormattedASTArgs) (ok bool, out string, args []interface{}, err error) { - e, ok := sqlExprRegistry[n.Ref] - if !ok { - return - } - - out, args, err = e(aa...) - return +func sqlASTFormatter(n *qlng.ASTNode) rdbms.HandlerSig { + return sqlExprRegistry[n.Ref] } func sqlFunctionHandler(f ql.Function) (ql.ASTNode, error) { diff --git a/tests/reporter/9007_filtering_specials_test.go b/tests/reporter/9007_filtering_specials_test.go index 08f28a132..cd1a60d09 100644 --- a/tests/reporter/9007_filtering_specials_test.go +++ b/tests/reporter/9007_filtering_specials_test.go @@ -11,7 +11,6 @@ func Test9007_filtering_specials(t *testing.T) { ff = loadNoErrMulti(ctx, h, m, dd...) ) - h.a.Len(ff, 3) f := ff[0] h.a.Equal(12, f.Size()) h.a.Equal("empty_filter", f.Name) @@ -26,4 +25,19 @@ func Test9007_filtering_specials(t *testing.T) { h.a.Equal(12, f.Size()) h.a.Equal("true_filter", f.Name) h.a.Equal("first_name, last_name", f.Columns.String()) + + f = ff[3] + h.a.Equal(4, f.Size()) + h.a.Equal("weird_filter", f.Name) + h.a.Equal("first_name, last_name", f.Columns.String()) + checkRows(h, f, + "Maria, Königsmann", + "Engel, Loritz", + "Maria, Krüger", + "Engel, Kiefer") + + f = ff[4] + h.a.Equal(12, f.Size()) + h.a.Equal("empty_filter_object", f.Name) + h.a.Equal("first_name, last_name", f.Columns.String()) } diff --git a/tests/reporter/9008_filtering_eval_order_test.go b/tests/reporter/9008_filtering_eval_order_test.go new file mode 100644 index 000000000..a7cf1a14f --- /dev/null +++ b/tests/reporter/9008_filtering_eval_order_test.go @@ -0,0 +1,34 @@ +package reporter + +import ( + "testing" +) + +func Test9008_filtering_eval_order(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenarioOwnDM(ctx, s, t, h) + ff = loadNoErrMulti(ctx, h, m, dd...) + ) + + f := ff[0] + h.a.Equal(4, f.Size()) + h.a.Equal("c1", f.Name) + h.a.Equal("first_name, numbers", f.Columns.String()) + checkRows(h, f, + "test1, 11", + "test1, 11", + "test2, 11", + "test1, 14") + + f = ff[1] + h.a.Equal(4, f.Size()) + h.a.Equal("c2", f.Name) + h.a.Equal("first_name, numbers", f.Columns.String()) + checkRows(h, f, + "test1, 11", + "test1, 11", + "test2, 11", + "test1, 14") + +} diff --git a/tests/reporter/9009_filtering_vaidation_test.go b/tests/reporter/9009_filtering_vaidation_test.go new file mode 100644 index 000000000..a57f99cf0 --- /dev/null +++ b/tests/reporter/9009_filtering_vaidation_test.go @@ -0,0 +1,14 @@ +package reporter + +import ( + "testing" +) + +func Test9009_filtering_validation(t *testing.T) { + var ( + ctx, h, s = setup(t) + m, _, dd = loadScenario(ctx, s, t, h) + ) + + loadErr(ctx, h, m, dd[0], "could not build query: expecting 1 or more arguments, got 0") +} diff --git a/tests/reporter/main_test.go b/tests/reporter/main_test.go index 3d596d5e9..dc7cb61f6 100644 --- a/tests/reporter/main_test.go +++ b/tests/reporter/main_test.go @@ -182,6 +182,12 @@ func loadNoErr(ctx context.Context, h helper, m report.M, dd ...*report.FrameDef return } +func loadErr(ctx context.Context, h helper, m report.M, d *report.FrameDefinition, msg string) { + _, err := m.Load(ctx, d) + h.a.Error(err) + h.a.Contains(err.Error(), msg) +} + // loadNoErrMulti is a little wrapper that does some preprocessing on the frame definitions. // It is a copy from the system/service/report. // @@ -255,6 +261,25 @@ func loadScenarioWithName(ctx context.Context, s store.Storer, t *testing.T, h h return m, rr, rr.Frames } +func loadScenarioOwnDM(ctx context.Context, s store.Storer, t *testing.T, h helper) (report.M, *auxReport, report.FrameDefinitionSet) { + return loadScenarioOwnDMWithName(ctx, s, t, h, "S"+t.Name()[4:]) +} + +func loadScenarioOwnDMWithName(ctx context.Context, s store.Storer, t *testing.T, h helper, scenario string) (report.M, *auxReport, report.FrameDefinitionSet) { + var ( + providers = map[string]report.DatasourceProvider{ + "composeRecords": service.DefaultRecord, + } + ) + + cleanup(ctx, h, s) + parseEnvoy(ctx, s, h, path.Join("testdata", scenario, "data_model")) + rr := parseReport(h, path.Join("testdata", scenario, "report.json")) + m := modelReport(ctx, h, providers, rr) + + return m, rr, rr.Frames +} + func cleanup(ctx context.Context, h helper, s store.Storer) { h.noError(s.TruncateComposeNamespaces(ctx)) h.noError(s.TruncateComposeModules(ctx)) diff --git a/tests/reporter/testdata/S9007_filtering_specials/report.json b/tests/reporter/testdata/S9007_filtering_specials/report.json index 3e59e6fe7..8587a7467 100644 --- a/tests/reporter/testdata/S9007_filtering_specials/report.json +++ b/tests/reporter/testdata/S9007_filtering_specials/report.json @@ -7,7 +7,9 @@ "definition": { "module": "user", "namespace": "ns" - } + }, + + "filter": {} }}}], "frames": [{ "name": "empty_filter", @@ -36,5 +38,35 @@ ], "filter": "true" + }, { + "name": "weird_filter", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" } + ], + + "filter": { + "ref": "and", + "args": [ + { + "ref": "or", + "args": [ + { "raw": "first_name = 'Maria'" }, + { "raw": "first_name = 'Engel'" } + ] + }, + { "ref": "or", "args": [{ "raw": "number_of_numbers > 40" }] } + ] + } + }, { + "name": "empty_filter_object", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" } + ], + + "filter": {} }] } diff --git a/tests/reporter/testdata/S9008_filtering_eval_order/data_model/data_users.csv b/tests/reporter/testdata/S9008_filtering_eval_order/data_model/data_users.csv new file mode 100644 index 000000000..0592e904a --- /dev/null +++ b/tests/reporter/testdata/S9008_filtering_eval_order/data_model/data_users.csv @@ -0,0 +1,10 @@ +id,first_name,numbers +1,test1,1 +2,test1,11 +3,test2,1 +4,no,100 +5,test1,11 +6,test1,2 +7,test2,11 +8,test1,14 +9,no,100 \ No newline at end of file diff --git a/tests/reporter/testdata/S9008_filtering_eval_order/data_model/datamodel.yaml b/tests/reporter/testdata/S9008_filtering_eval_order/data_model/datamodel.yaml new file mode 100644 index 000000000..6e3a2a38a --- /dev/null +++ b/tests/reporter/testdata/S9008_filtering_eval_order/data_model/datamodel.yaml @@ -0,0 +1,23 @@ +namespaces: + ns: + name: ns name + +modules: + user: + records: + source: data_users.csv + key: id + mapping: + id: / + first_name: + field: first_name + numbers: + field: numbers + + fields: + first_name: + label: first_name label + kind: String + numbers: + label: numbers label + kind: Number diff --git a/tests/reporter/testdata/S9008_filtering_eval_order/report.json b/tests/reporter/testdata/S9008_filtering_eval_order/report.json new file mode 100644 index 000000000..3cb742207 --- /dev/null +++ b/tests/reporter/testdata/S9008_filtering_eval_order/report.json @@ -0,0 +1,82 @@ +{ + "handle": "testing_report", + "sources": [{ + "step": { "load": { + "name": "users", + "source": "composeRecords", + "definition": { + "module": "user", + "namespace": "ns" + } + }}}], + "frames": [{ + "name": "c1", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "numbers", "label": "numbers" } + ], + + "filter": { + "ref": "and", + "args": [ + { + "ref": "or", + "args": [ + { "raw": "first_name = 'test1'" }, + { "raw": "first_name = 'test2'" } + ] + }, + { "ref": "or", "args": [{ "raw": "numbers > 10" }] } + ] + } + }, { + "name": "c2", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "numbers", "label": "numbers" } + ], + + "filter": { + "ref": "and", + "args": [ + { + "ref": "or", + "args": [{ + "ref": "eq", + "args": [{ + "symbol": "first_name" + }, { + "value": { + "@type": "String", + "@value": "test1" + } + }] + }, { + "ref": "eq", + "args": [{ + "symbol": "first_name" + }, { + "value": { + "@type": "String", + "@value": "test2" + } + }] + }] + }, + { "ref": "or", "args": [{ + "ref": "gt", + "args": [{ + "symbol": "numbers" + }, { + "value": { + "@type": "Integer", + "@value": 10 + } + }] + }] } + ] + } + }] +} diff --git a/tests/reporter/testdata/S9009_filtering_validation/report.json b/tests/reporter/testdata/S9009_filtering_validation/report.json new file mode 100644 index 000000000..9765ecccd --- /dev/null +++ b/tests/reporter/testdata/S9009_filtering_validation/report.json @@ -0,0 +1,25 @@ +{ + "handle": "testing_report", + "sources": [{ + "step": { "load": { + "name": "users", + "source": "composeRecords", + "definition": { + "module": "user", + "namespace": "ns" + } + }}}], + "frames": [{ + "name": "empty_conjunction", + "source": "users", + "columns": [ + { "name": "first_name", "label": "first_name" }, + { "name": "last_name", "label": "last_name" } + ], + + "filter": { + "ref": "and", + "args": [] + } + }] +}