From 8815f6060c94b9f9ad83e38a58bdc7115a82dad9 Mon Sep 17 00:00:00 2001 From: Gani Georgiev Date: Wed, 14 Dec 2022 12:29:43 +0200 Subject: [PATCH] reduced the parenthesis in the generated filter sql query --- tools/search/filter.go | 68 +++++++++++++++++++-- tools/search/filter_test.go | 108 ++++++++++++++++++++-------------- tools/search/provider_test.go | 27 +++++---- 3 files changed, 142 insertions(+), 61 deletions(-) diff --git a/tools/search/filter.go b/tools/search/filter.go index 3135f58ef..239f3fbe2 100644 --- a/tools/search/filter.go +++ b/tools/search/filter.go @@ -47,7 +47,7 @@ func (f FilterData) build(data []fexpr.ExprGroup, fieldResolver FieldResolver) ( return nil, errors.New("Empty filter expression.") } - var result dbx.Expression + result := &concatExpr{separator: " "} for _, group := range data { var expr dbx.Expression @@ -68,11 +68,17 @@ func (f FilterData) build(data []fexpr.ExprGroup, fieldResolver FieldResolver) ( return nil, exprErr } - if group.Join == fexpr.JoinAnd { - result = dbx.And(result, expr) - } else { - result = dbx.Or(result, expr) + if len(result.parts) > 0 { + var op string + if group.Join == fexpr.JoinOr { + op = "OR" + } else { + op = "AND" + } + result.parts = append(result.parts, &opExpr{op}) } + + result.parts = append(result.parts, expr) } return result, nil @@ -209,3 +215,55 @@ func wrapLikeParams(params dbx.Params) dbx.Params { return result } + +// ------------------------------------------------------------------- + +// opExpr defines an expression that contains a raw sql operator string. +type opExpr struct { + op string +} + +// Build converts an expression into a SQL fragment. +// +// Implements [dbx.Expression] interface. +func (e *opExpr) Build(db *dbx.DB, params dbx.Params) string { + return e.op +} + +// concatExpr defines an expression that concatenates multiple +// other expressions with a specified separator. +type concatExpr struct { + parts []dbx.Expression + separator string +} + +// Build converts an expression into a SQL fragment. +// +// Implements [dbx.Expression] interface. +func (e *concatExpr) Build(db *dbx.DB, params dbx.Params) string { + if len(e.parts) == 0 { + return "" + } + + stringParts := make([]string, 0, len(e.parts)) + + for _, a := range e.parts { + if a == nil { + continue + } + + if sql := a.Build(db, params); sql != "" { + stringParts = append(stringParts, sql) + } + } + + // skip extra parenthesis for single concat expression + if len(stringParts) == 1 && + // check for already concatenated raw/plain expressions + !strings.Contains(strings.ToUpper(stringParts[0]), " AND ") && + !strings.Contains(strings.ToUpper(stringParts[0]), " OR ") { + return stringParts[0] + } + + return "(" + strings.Join(stringParts, e.separator) + ")" +} diff --git a/tools/search/filter_test.go b/tools/search/filter_test.go index cfdc21de3..9444630ea 100644 --- a/tools/search/filter_test.go +++ b/tools/search/filter_test.go @@ -12,118 +12,140 @@ func TestFilterDataBuildExpr(t *testing.T) { resolver := search.NewSimpleFieldResolver("test1", "test2", "test3", "test4.sub") scenarios := []struct { + name string filterData search.FilterData expectError bool expectPattern string }{ - // empty - {"", true, ""}, - // invalid format - {"(test1 > 1", true, ""}, - // invalid operator - {"test1 + 123", true, ""}, - // unknown field - {"test1 = 'example' && unknown > 1", true, ""}, - // simple expression - {"test1 > 1", false, + { + "empty", + "", + true, + "", + }, + { + "invalid format", + "(test1 > 1", true, ""}, + { + "invalid operator", + "test1 + 123", + true, + "", + }, + { + "unknown field", + "test1 = 'example' && unknown > 1", + true, + "", + }, + { + "simple expression", + "test1 > 1", false, "^" + regexp.QuoteMeta("[[test1]] > {:") + ".+" + regexp.QuoteMeta("}") + "$", }, - // like with 2 columns - {"test1 ~ test2", false, + { + "like with 2 columns", + "test1 ~ test2", false, "^" + regexp.QuoteMeta("[[test1]] LIKE ('%' || [[test2]] || '%') ESCAPE '\\'") + "$", }, - // like with right column operand - {"'lorem' ~ test1", false, + { + "like with right column operand", + "'lorem' ~ test1", false, "^" + regexp.QuoteMeta("{:") + ".+" + regexp.QuoteMeta("} LIKE ('%' || [[test1]] || '%') ESCAPE '\\'") + "$", }, - // like with left column operand and text as right operand - {"test1 ~ 'lorem'", false, + { + "like with left column operand and text as right operand", + "test1 ~ 'lorem'", false, "^" + regexp.QuoteMeta("[[test1]] LIKE {:") + ".+" + regexp.QuoteMeta("} ESCAPE '\\'") + "$", }, - // not like with 2 columns - {"test1 !~ test2", false, + { + "not like with 2 columns", + "test1 !~ test2", false, "^" + regexp.QuoteMeta("[[test1]] NOT LIKE ('%' || [[test2]] || '%') ESCAPE '\\'") + "$", }, - // not like with right column operand - {"'lorem' !~ test1", false, + { + "not like with right column operand", + "'lorem' !~ test1", false, "^" + regexp.QuoteMeta("{:") + ".+" + regexp.QuoteMeta("} NOT LIKE ('%' || [[test1]] || '%') ESCAPE '\\'") + "$", }, - // like with left column operand and text as right operand - {"test1 !~ 'lorem'", false, + { + "like with left column operand and text as right operand", + "test1 !~ 'lorem'", false, "^" + regexp.QuoteMeta("[[test1]] NOT LIKE {:") + ".+" + regexp.QuoteMeta("} ESCAPE '\\'") + "$", }, - // current datetime constant - {"test1 > @now", false, + { + "current datetime constant", + "test1 > @now", false, "^" + regexp.QuoteMeta("[[test1]] > {:") + ".+" + regexp.QuoteMeta("}") + "$", }, - // complex expression { + "complex expression", "((test1 > 1) || (test2 != 2)) && test3 ~ '%%example' && test4.sub = null", false, "^" + - regexp.QuoteMeta("((([[test1]] > {:") + + regexp.QuoteMeta("(([[test1]] > {:") + ".+" + - regexp.QuoteMeta("}) OR (COALESCE([[test2]], '') != COALESCE({:") + + regexp.QuoteMeta("} OR COALESCE([[test2]], '') != COALESCE({:") + ".+" + - regexp.QuoteMeta("}, ''))) AND ([[test3]] LIKE {:") + + regexp.QuoteMeta("}, '')) AND [[test3]] LIKE {:") + ".+" + - regexp.QuoteMeta("} ESCAPE '\\')) AND (COALESCE([[test4.sub]], '') = COALESCE(NULL, ''))") + + regexp.QuoteMeta("} ESCAPE '\\' AND COALESCE([[test4.sub]], '') = COALESCE(NULL, ''))") + "$", }, - // combination of special literals (null, true, false) { + "combination of special literals (null, true, false)", "test1=true && test2 != false && test3 = null || test4.sub != null", false, - "^" + regexp.QuoteMeta("(((COALESCE([[test1]], '') = COALESCE(1, '')) AND (COALESCE([[test2]], '') != COALESCE(0, ''))) AND (COALESCE([[test3]], '') = COALESCE(NULL, ''))) OR (COALESCE([[test4.sub]], '') != COALESCE(NULL, ''))") + "$", + "^" + regexp.QuoteMeta("(COALESCE([[test1]], '') = COALESCE(1, '') AND COALESCE([[test2]], '') != COALESCE(0, '') AND COALESCE([[test3]], '') = COALESCE(NULL, '') OR COALESCE([[test4.sub]], '') != COALESCE(NULL, ''))") + "$", }, - // all operators { + "all operators", "(test1 = test2 || test2 != test3) && (test2 ~ 'example' || test2 !~ '%%abc') && 'switch1%%' ~ test1 && 'switch2' !~ test2 && test3 > 1 && test3 >= 0 && test3 <= 4 && 2 < 5", false, "^" + - regexp.QuoteMeta("((((((((COALESCE([[test1]], '') = COALESCE([[test2]], '')) OR (COALESCE([[test2]], '') != COALESCE([[test3]], ''))) AND (([[test2]] LIKE {:") + + regexp.QuoteMeta("((COALESCE([[test1]], '') = COALESCE([[test2]], '') OR COALESCE([[test2]], '') != COALESCE([[test3]], '')) AND ([[test2]] LIKE {:") + ".+" + - regexp.QuoteMeta("} ESCAPE '\\') OR ([[test2]] NOT LIKE {:") + + regexp.QuoteMeta("} ESCAPE '\\' OR [[test2]] NOT LIKE {:") + ".+" + - regexp.QuoteMeta("} ESCAPE '\\'))) AND ({:") + + regexp.QuoteMeta("} ESCAPE '\\') AND {:") + ".+" + - regexp.QuoteMeta("} LIKE ('%' || [[test1]] || '%') ESCAPE '\\')) AND ({:") + + regexp.QuoteMeta("} LIKE ('%' || [[test1]] || '%') ESCAPE '\\' AND {:") + ".+" + - regexp.QuoteMeta("} NOT LIKE ('%' || [[test2]] || '%') ESCAPE '\\')) AND ([[test3]] > {:") + + regexp.QuoteMeta("} NOT LIKE ('%' || [[test2]] || '%') ESCAPE '\\' AND [[test3]] > {:") + ".+" + - regexp.QuoteMeta("})) AND ([[test3]] >= {:") + + regexp.QuoteMeta("} AND [[test3]] >= {:") + ".+" + - regexp.QuoteMeta("})) AND ([[test3]] <= {:") + + regexp.QuoteMeta("} AND [[test3]] <= {:") + ".+" + - regexp.QuoteMeta("})) AND ({:") + + regexp.QuoteMeta("} AND {:") + ".+" + regexp.QuoteMeta("} < {:") + ".+" + @@ -132,12 +154,12 @@ func TestFilterDataBuildExpr(t *testing.T) { }, } - for i, s := range scenarios { + for _, s := range scenarios { expr, err := s.filterData.BuildExpr(resolver) hasErr := err != nil if hasErr != s.expectError { - t.Errorf("(%d) Expected hasErr %v, got %v (%v)", i, s.expectError, hasErr, err) + t.Errorf("[%s] Expected hasErr %v, got %v (%v)", s.name, s.expectError, hasErr, err) continue } @@ -150,7 +172,7 @@ func TestFilterDataBuildExpr(t *testing.T) { pattern := regexp.MustCompile(s.expectPattern) if !pattern.MatchString(rawSql) { - t.Errorf("(%d) Pattern %v don't match with expression: \n%v", i, s.expectPattern, rawSql) + t.Errorf("[%s] Pattern %v don't match with expression: \n%v", s.name, s.expectPattern, rawSql) } } } diff --git a/tools/search/provider_test.go b/tools/search/provider_test.go index c65fe7c33..e76ed89d8 100644 --- a/tools/search/provider_test.go +++ b/tools/search/provider_test.go @@ -210,6 +210,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { OrderBy("test1 ASC") scenarios := []struct { + name string page int perPage int sort []SortField @@ -218,8 +219,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { expectResult string expectQueries []string }{ - // page normalization { + "page normalization", -1, 10, []SortField{}, @@ -231,8 +232,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "SELECT * FROM `test` WHERE NOT (`test1` IS NULL) ORDER BY `test1` ASC LIMIT 10", }, }, - // perPage normalization { + "perPage normalization", 10, // will be capped by total count 0, // fallback to default []SortField{}, @@ -244,8 +245,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "SELECT * FROM `test` WHERE NOT (`test1` IS NULL) ORDER BY `test1` ASC LIMIT 30", }, }, - // invalid sort field { + "invalid sort field", 1, 10, []SortField{{"unknown", SortAsc}}, @@ -254,8 +255,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "", nil, }, - // invalid filter { + "invalid filter", 1, 10, []SortField{}, @@ -264,8 +265,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "", nil, }, - // valid sort and filter fields { + "valid sort and filter fields", 1, 5555, // will be limited by MaxPerPage []SortField{{"test2", SortDesc}}, @@ -277,8 +278,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "SELECT * FROM `test` WHERE ((NOT (`test1` IS NULL)) AND (COALESCE(test2, '') != COALESCE(null, ''))) AND (test1 >= 2) ORDER BY `test1` ASC, `test2` DESC LIMIT 500", }, }, - // valid sort and filter fields (zero results) { + "valid sort and filter fields (zero results)", 1, 10, []SortField{{"test3", SortAsc}}, @@ -290,8 +291,8 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { "SELECT * FROM `test` WHERE (NOT (`test1` IS NULL)) AND (COALESCE(test3, '') != COALESCE('', '')) ORDER BY `test1` ASC, `test3` ASC LIMIT 10", }, }, - // pagination test { + "pagination test", 3, 1, []SortField{}, @@ -305,7 +306,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { }, } - for i, s := range scenarios { + for _, s := range scenarios { testDB.CalledQueries = []string{} // reset testResolver := &testFieldResolver{} @@ -320,7 +321,7 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { hasErr := err != nil if hasErr != s.expectError { - t.Errorf("(%d) Expected hasErr %v, got %v (%v)", i, s.expectError, hasErr, err) + t.Errorf("[%s] Expected hasErr %v, got %v (%v)", s.name, s.expectError, hasErr, err) continue } @@ -329,22 +330,22 @@ func TestProviderExecNonEmptyQuery(t *testing.T) { } if testResolver.UpdateQueryCalls != 1 { - t.Errorf("(%d) Expected resolver.Update to be called %d, got %d", i, 1, testResolver.UpdateQueryCalls) + t.Errorf("[%s] Expected resolver.Update to be called %d, got %d", s.name, 1, testResolver.UpdateQueryCalls) } encoded, _ := json.Marshal(result) if string(encoded) != s.expectResult { - t.Errorf("(%d) Expected result %v, got \n%v", i, s.expectResult, string(encoded)) + t.Errorf("[%s] Expected result %v, got \n%v", s.name, s.expectResult, string(encoded)) } if len(s.expectQueries) != len(testDB.CalledQueries) { - t.Errorf("(%d) Expected %d queries, got %d: \n%v", i, len(s.expectQueries), len(testDB.CalledQueries), testDB.CalledQueries) + t.Errorf("[%s] Expected %d queries, got %d: \n%v", s.name, len(s.expectQueries), len(testDB.CalledQueries), testDB.CalledQueries) continue } for _, q := range testDB.CalledQueries { if !list.ExistInSliceWithRegex(q, s.expectQueries) { - t.Errorf("(%d) Didn't expect query \n%v in \n%v", i, q, testDB.CalledQueries) + t.Errorf("[%s] Didn't expect query \n%v in \n%v", s.name, q, testDB.CalledQueries) } } }