From a38335e1ba938497e0277a99c3c4b4bd02f89cc4 Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Sat, 12 Mar 2022 03:31:45 +0900 Subject: [PATCH 1/7] Add filedb filter logic --- pkg/datastore/filedb/BUILD.bazel | 11 ++- pkg/datastore/filedb/filter.go | 108 +++++++++++++++++++++++- pkg/datastore/filedb/filter_test.go | 124 ++++++++++++++++++++++++++++ 3 files changed, 238 insertions(+), 5 deletions(-) create mode 100644 pkg/datastore/filedb/filter_test.go diff --git a/pkg/datastore/filedb/BUILD.bazel b/pkg/datastore/filedb/BUILD.bazel index 30d573309e..551e1bd1ef 100644 --- a/pkg/datastore/filedb/BUILD.bazel +++ b/pkg/datastore/filedb/BUILD.bazel @@ -22,7 +22,14 @@ go_library( go_test( name = "go_default_test", size = "small", - srcs = ["codec_test.go"], + srcs = [ + "codec_test.go", + "filter_test.go", + ], embed = [":go_default_library"], - deps = ["@com_github_stretchr_testify//assert:go_default_library"], + deps = [ + "//pkg/datastore:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", + "@com_github_stretchr_testify//require:go_default_library", + ], ) diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index cddfce190b..f0ea758e6b 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -15,19 +15,121 @@ package filedb import ( + "encoding/json" + "fmt" + "reflect" + "strings" + "github.com/pipe-cd/pipecd/pkg/datastore" ) -// TODO: Implement filterable interface for each collection. type filterable interface { Match(e interface{}, filters []datastore.ListFilter) (bool, error) } func filter(col datastore.Collection, e interface{}, filters []datastore.ListFilter) (bool, error) { fcol, ok := col.(filterable) - if !ok { + if ok { + return fcol.Match(e, filters) + } + + // remarshal entity as map[string]interface{} struct. + raw, _ := json.Marshal(e) + var omap map[string]interface{} + if err := json.Unmarshal(raw, &omap); err != nil { + return false, err + } + + for _, filter := range filters { + if strings.Contains(filter.Field, ".") { + // TODO: Handle nested field name such as SyncState.Status. + return false, datastore.ErrUnsupported + } + + val, ok := omap[filter.Field] + // If the object does not contain given field name in filter, return false immidiately. + if !ok { + return false, nil + } + + cmp, err := compare(val, filter.Value, filter.Operator) + if err != nil { + return false, err + } + + if !cmp { + return false, nil + } + } + + return true, nil +} + +func compare(val, operand interface{}, op datastore.Operator) (bool, error) { + switch op { + case datastore.OperatorEqual: + return val == operand, nil + case datastore.OperatorNotEqual: + return val != operand, nil + case datastore.OperatorGreaterThan: + return val.(int64) > operand.(int64), nil + case datastore.OperatorGreaterThanOrEqual: + return val.(int64) >= operand.(int64), nil + case datastore.OperatorLessThan: + return val.(int64) < operand.(int64), nil + case datastore.OperatorLessThanOrEqual: + return val.(int64) <= operand.(int64), nil + case datastore.OperatorIn: + os, err := makeSliceOfInterfaces(operand) + if err != nil { + return false, fmt.Errorf("operand error: %w", err) + } + + for _, o := range os { + if o == val { + return true, nil + } + } + return false, nil + case datastore.OperatorNotIn: + os, err := makeSliceOfInterfaces(operand) + if err != nil { + return false, fmt.Errorf("operand error: %w", err) + } + + for _, o := range os { + if o == val { + return false, nil + } + } + return true, nil + case datastore.OperatorContains: + vs, err := makeSliceOfInterfaces(val) + if err != nil { + return false, fmt.Errorf("value error: %w", err) + } + + for _, v := range vs { + if v == operand { + return true, nil + } + } + return false, nil + default: return false, datastore.ErrUnsupported } +} + +func makeSliceOfInterfaces(v interface{}) ([]interface{}, error) { + rv := reflect.ValueOf(v) + if rv.Kind() != reflect.Slice { + return nil, fmt.Errorf("value is not a slide") + } + + vs := make([]interface{}, rv.Len()) + for i := 0; i < rv.Len(); i++ { + vs[i] = rv.Index(i).Interface() + } - return fcol.Match(e, filters) + return vs, nil } diff --git a/pkg/datastore/filedb/filter_test.go b/pkg/datastore/filedb/filter_test.go new file mode 100644 index 0000000000..23f1b82889 --- /dev/null +++ b/pkg/datastore/filedb/filter_test.go @@ -0,0 +1,124 @@ +// Copyright 2022 The PipeCD Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package filedb + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/pipe-cd/pipecd/pkg/datastore" +) + +func TestCompare(t *testing.T) { + testcases := []struct { + name string + val interface{} + operand interface{} + operator datastore.Operator + expect bool + }{ + { + name: "equal number int", + val: 5, + operand: 5, + operator: datastore.OperatorEqual, + expect: true, + }, + { + name: "equal string", + val: "text", + operand: "text", + operator: datastore.OperatorEqual, + expect: true, + }, + { + name: "not equal int", + val: 3, + operand: 2, + operator: datastore.OperatorNotEqual, + expect: true, + }, + { + name: "not equal string", + val: "text_val", + operand: "text_operand", + operator: datastore.OperatorNotEqual, + expect: true, + }, + // { + // name: "greater than int", + // val: 3, + // operand: 1, + // operator: datastore.OperatorGreaterThan, + // expect: true, + // }, + // { + // name: "greater than string", + // val: "text_2", + // operand: "text_1", + // operator: datastore.OperatorGreaterThan, + // expect: true, + // }, + { + name: "in int", + val: 1, + operand: []int{1, 2, 3}, + operator: datastore.OperatorIn, + expect: true, + }, + { + name: "in int false", + val: 4, + operand: []int{1, 2, 3}, + operator: datastore.OperatorIn, + expect: false, + }, + { + name: "not in int", + val: 4, + operand: []int{1, 2, 3}, + operator: datastore.OperatorNotIn, + expect: true, + }, + { + name: "not in int false", + val: 1, + operand: []int{1, 2, 3}, + operator: datastore.OperatorNotIn, + expect: false, + }, + { + name: "contains int", + val: []int{1, 2, 3}, + operand: 1, + operator: datastore.OperatorContains, + expect: true, + }, + } + + for _, tc := range testcases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + res, err := compare(tc.val, tc.operand, tc.operator) + require.Nil(t, err) + assert.Equal(t, tc.expect, res) + }) + } +} From 29994d50acf5e609260a07afad7fe31d03c161b6 Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Sun, 13 Mar 2022 23:33:47 +0900 Subject: [PATCH 2/7] Update filter logic --- pkg/datastore/filedb/filter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index f0ea758e6b..f424c403f6 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -122,8 +122,8 @@ func compare(val, operand interface{}, op datastore.Operator) (bool, error) { func makeSliceOfInterfaces(v interface{}) ([]interface{}, error) { rv := reflect.ValueOf(v) - if rv.Kind() != reflect.Slice { - return nil, fmt.Errorf("value is not a slide") + if rv.Kind() != reflect.Slice && rv.Kind() != reflect.Array { + return nil, fmt.Errorf("value is not a slide or array") } vs := make([]interface{}, rv.Len()) From 66bfda5c816c12067ba41cf6f8396bd7f039005c Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Mon, 14 Mar 2022 11:14:35 +0700 Subject: [PATCH 3/7] Update converting logic --- pkg/datastore/datastore.go | 7 ++ pkg/datastore/filedb/BUILD.bazel | 1 + pkg/datastore/filedb/filter.go | 68 ++++++++++- pkg/datastore/filedb/filter_test.go | 175 ++++++++++++++++++++++++---- 4 files changed, 221 insertions(+), 30 deletions(-) diff --git a/pkg/datastore/datastore.go b/pkg/datastore/datastore.go index 16ec32e218..03b1c54ef2 100644 --- a/pkg/datastore/datastore.go +++ b/pkg/datastore/datastore.go @@ -51,6 +51,13 @@ const ( OperatorContains ) +func IsNumericOperator(op Operator) bool { + return op == OperatorGreaterThan || + op == OperatorGreaterThanOrEqual || + op == OperatorLessThan || + op == OperatorLessThanOrEqual +} + var ( ErrNotFound = errors.New("not found") ErrInvalidArgument = errors.New("invalid argument") diff --git a/pkg/datastore/filedb/BUILD.bazel b/pkg/datastore/filedb/BUILD.bazel index 551e1bd1ef..75705d480a 100644 --- a/pkg/datastore/filedb/BUILD.bazel +++ b/pkg/datastore/filedb/BUILD.bazel @@ -29,6 +29,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/datastore:go_default_library", + "//pkg/model:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", ], diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index f424c403f6..3047b73e00 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -19,6 +19,7 @@ import ( "fmt" "reflect" "strings" + "unicode" "github.com/pipe-cd/pipecd/pkg/datastore" ) @@ -41,12 +42,13 @@ func filter(col datastore.Collection, e interface{}, filters []datastore.ListFil } for _, filter := range filters { - if strings.Contains(filter.Field, ".") { + field := convertCamelToSnake(filter.Field) + if strings.Contains(field, ".") { // TODO: Handle nested field name such as SyncState.Status. return false, datastore.ErrUnsupported } - val, ok := omap[filter.Field] + val, ok := omap[field] // If the object does not contain given field name in filter, return false immidiately. if !ok { return false, nil @@ -66,19 +68,41 @@ func filter(col datastore.Collection, e interface{}, filters []datastore.ListFil } func compare(val, operand interface{}, op datastore.Operator) (bool, error) { + var valNum, operandNum int64 + switch v := val.(type) { + case int, int8, int16, int32, int64: + valNum = reflect.ValueOf(v).Int() + case uint, uint8, uint16, uint32, uint64: + valNum = reflect.ValueOf(v).Int() + default: + if datastore.IsNumericOperator(op) { + return false, fmt.Errorf("value of type unsupported") + } + } + switch o := operand.(type) { + case int, int8, int16, int32, int64: + operandNum = reflect.ValueOf(o).Int() + case uint, uint8, uint16, uint32, uint64: + operandNum = reflect.ValueOf(o).Int() + default: + if datastore.IsNumericOperator(op) { + return false, fmt.Errorf("value of type unsupported") + } + } + switch op { case datastore.OperatorEqual: return val == operand, nil case datastore.OperatorNotEqual: return val != operand, nil case datastore.OperatorGreaterThan: - return val.(int64) > operand.(int64), nil + return valNum > operandNum, nil case datastore.OperatorGreaterThanOrEqual: - return val.(int64) >= operand.(int64), nil + return valNum >= operandNum, nil case datastore.OperatorLessThan: - return val.(int64) < operand.(int64), nil + return valNum < operandNum, nil case datastore.OperatorLessThanOrEqual: - return val.(int64) <= operand.(int64), nil + return valNum <= operandNum, nil case datastore.OperatorIn: os, err := makeSliceOfInterfaces(operand) if err != nil { @@ -133,3 +157,35 @@ func makeSliceOfInterfaces(v interface{}) ([]interface{}, error) { return vs, nil } + +func convertCamelToSnake(key string) string { + runeToLower := func(r rune) string { + return strings.ToLower(string(r)) + } + + var out string + for i, v := range key { + if i == 0 { + out += runeToLower(v) + continue + } + + if i == len(key)-1 { + out += runeToLower(v) + break + } + + if unicode.IsUpper(v) && unicode.IsLower(rune(key[i+1])) { + out += fmt.Sprintf("_%s", runeToLower(v)) + continue + } + + if unicode.IsUpper(v) { + out += runeToLower(v) + continue + } + + out += string(v) + } + return out +} diff --git a/pkg/datastore/filedb/filter_test.go b/pkg/datastore/filedb/filter_test.go index 23f1b82889..3f242d1f62 100644 --- a/pkg/datastore/filedb/filter_test.go +++ b/pkg/datastore/filedb/filter_test.go @@ -21,15 +21,53 @@ import ( "github.com/stretchr/testify/require" "github.com/pipe-cd/pipecd/pkg/datastore" + "github.com/pipe-cd/pipecd/pkg/model" ) +func TestConvertCamelToSnake(t *testing.T) { + testcases := []struct { + name string + camel string + snake string + }{ + { + name: "single camel", + camel: "Id", + snake: "id", + }, + { + name: "full of upper cases", + camel: "API", + snake: "api", + }, + { + name: "mix with full of upper cases word", + camel: "APIKey", + snake: "api_key", + }, + { + name: "formal camel", + camel: "StaticAdminDisabled", + snake: "static_admin_disabled", + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + out := convertCamelToSnake(tc.camel) + assert.Equal(t, tc.snake, out) + }) + } +} + func TestCompare(t *testing.T) { testcases := []struct { - name string - val interface{} - operand interface{} - operator datastore.Operator - expect bool + name string + val interface{} + operand interface{} + operator datastore.Operator + expect bool + expectErr bool }{ { name: "equal number int", @@ -59,20 +97,20 @@ func TestCompare(t *testing.T) { operator: datastore.OperatorNotEqual, expect: true, }, - // { - // name: "greater than int", - // val: 3, - // operand: 1, - // operator: datastore.OperatorGreaterThan, - // expect: true, - // }, - // { - // name: "greater than string", - // val: "text_2", - // operand: "text_1", - // operator: datastore.OperatorGreaterThan, - // expect: true, - // }, + { + name: "greater than int", + val: 3, + operand: 1, + operator: datastore.OperatorGreaterThan, + expect: true, + }, + { + name: "greater than or equal int", + val: 3, + operand: 3, + operator: datastore.OperatorGreaterThanOrEqual, + expect: true, + }, { name: "in int", val: 1, @@ -108,17 +146,106 @@ func TestCompare(t *testing.T) { operator: datastore.OperatorContains, expect: true, }, + { + name: "error on query for numeric only operator with wrong value", + val: "string_1", + operand: "string_0", + operator: datastore.OperatorGreaterThan, + expectErr: true, + }, + { + name: "error on query in operator with not operand of type slide/array", + val: 1, + operand: 1, + operator: datastore.OperatorIn, + expectErr: true, + }, } for _, tc := range testcases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - res, err := compare(tc.val, tc.operand, tc.operator) + require.Equal(t, tc.expectErr, err != nil) + + if err != nil { + assert.Equal(t, tc.expect, res) + } + }) + } +} + +func TestFilter(t *testing.T) { + testcases := []struct { + name string + entity interface{} + filters []datastore.ListFilter + expect bool + }{ + { + name: "filter single condition - passed", + entity: &model.Project{Id: "project_1"}, + filters: []datastore.ListFilter{ + { + Field: "Id", + Operator: datastore.OperatorEqual, + Value: "project_1", + }, + }, + expect: true, + }, + { + name: "filter single condition - not passed", + entity: &model.Project{Id: "project_1"}, + filters: []datastore.ListFilter{ + { + Field: "Id", + Operator: datastore.OperatorEqual, + Value: "project_2", + }, + }, + expect: false, + }, + { + name: "filter multiple conditions - passed", + entity: &model.Project{Id: "project_1", StaticAdminDisabled: true}, + filters: []datastore.ListFilter{ + { + Field: "Id", + Operator: datastore.OperatorEqual, + Value: "project_1", + }, + { + Field: "StaticAdminDisabled", + Operator: datastore.OperatorEqual, + Value: true, + }, + }, + expect: true, + }, + { + name: "filter multiple conditions - not passed", + entity: &model.Project{Id: "project_1", StaticAdminDisabled: true}, + filters: []datastore.ListFilter{ + { + Field: "Id", + Operator: datastore.OperatorEqual, + Value: "project_1", + }, + { + Field: "StaticAdminDisabled", + Operator: datastore.OperatorEqual, + Value: false, + }, + }, + expect: false, + }, + } + + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + passed, err := filter(nil, tc.entity, tc.filters) require.Nil(t, err) - assert.Equal(t, tc.expect, res) + assert.Equal(t, tc.expect, passed) }) } } From cf416f05e23e24da1083daab2517be8da26001db Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Mon, 14 Mar 2022 11:45:53 +0700 Subject: [PATCH 4/7] test run parallel --- pkg/datastore/filedb/filter_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/datastore/filedb/filter_test.go b/pkg/datastore/filedb/filter_test.go index 3f242d1f62..141209c940 100644 --- a/pkg/datastore/filedb/filter_test.go +++ b/pkg/datastore/filedb/filter_test.go @@ -25,6 +25,8 @@ import ( ) func TestConvertCamelToSnake(t *testing.T) { + t.Parallel() + testcases := []struct { name string camel string @@ -61,6 +63,8 @@ func TestConvertCamelToSnake(t *testing.T) { } func TestCompare(t *testing.T) { + t.Parallel() + testcases := []struct { name string val interface{} @@ -175,6 +179,8 @@ func TestCompare(t *testing.T) { } func TestFilter(t *testing.T) { + t.Parallel() + testcases := []struct { name string entity interface{} From 73530f4c3550517181a4a4c3c51010d10d0800aa Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Mon, 14 Mar 2022 15:28:35 +0700 Subject: [PATCH 5/7] Update follow comments --- pkg/datastore/datastore.go | 10 +++++----- pkg/datastore/filedb/filter.go | 16 ++++++++++------ 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/pkg/datastore/datastore.go b/pkg/datastore/datastore.go index 03b1c54ef2..54a3523b0e 100644 --- a/pkg/datastore/datastore.go +++ b/pkg/datastore/datastore.go @@ -51,11 +51,11 @@ const ( OperatorContains ) -func IsNumericOperator(op Operator) bool { - return op == OperatorGreaterThan || - op == OperatorGreaterThanOrEqual || - op == OperatorLessThan || - op == OperatorLessThanOrEqual +func (o Operator) IsNumericOperator() bool { + return o == OperatorGreaterThan || + o == OperatorGreaterThanOrEqual || + o == OperatorLessThan || + o == OperatorLessThanOrEqual } var ( diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index 3047b73e00..c087ebe0a6 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -29,13 +29,17 @@ type filterable interface { } func filter(col datastore.Collection, e interface{}, filters []datastore.ListFilter) (bool, error) { + // If the collection implement filterable interface, use it. fcol, ok := col.(filterable) if ok { return fcol.Match(e, filters) } // remarshal entity as map[string]interface{} struct. - raw, _ := json.Marshal(e) + raw, err := json.Marshal(e) + if err != nil { + return false, err + } var omap map[string]interface{} if err := json.Unmarshal(raw, &omap); err != nil { return false, err @@ -72,21 +76,21 @@ func compare(val, operand interface{}, op datastore.Operator) (bool, error) { switch v := val.(type) { case int, int8, int16, int32, int64: valNum = reflect.ValueOf(v).Int() - case uint, uint8, uint16, uint32, uint64: + case uint, uint8, uint16, uint32: valNum = reflect.ValueOf(v).Int() default: - if datastore.IsNumericOperator(op) { + if op.IsNumericOperator() { return false, fmt.Errorf("value of type unsupported") } } switch o := operand.(type) { case int, int8, int16, int32, int64: operandNum = reflect.ValueOf(o).Int() - case uint, uint8, uint16, uint32, uint64: + case uint, uint8, uint16, uint32: operandNum = reflect.ValueOf(o).Int() default: - if datastore.IsNumericOperator(op) { - return false, fmt.Errorf("value of type unsupported") + if op.IsNumericOperator() { + return false, fmt.Errorf("operand of type unsupported") } } From aa1437aca600c6215b430953059d6474094e61a7 Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Mon, 14 Mar 2022 15:46:24 +0700 Subject: [PATCH 6/7] Avoid panic on wrong type --- pkg/datastore/filedb/filter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index c087ebe0a6..6823a2ff42 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -77,7 +77,7 @@ func compare(val, operand interface{}, op datastore.Operator) (bool, error) { case int, int8, int16, int32, int64: valNum = reflect.ValueOf(v).Int() case uint, uint8, uint16, uint32: - valNum = reflect.ValueOf(v).Int() + valNum = int64(reflect.ValueOf(v).Uint()) default: if op.IsNumericOperator() { return false, fmt.Errorf("value of type unsupported") @@ -87,7 +87,7 @@ func compare(val, operand interface{}, op datastore.Operator) (bool, error) { case int, int8, int16, int32, int64: operandNum = reflect.ValueOf(o).Int() case uint, uint8, uint16, uint32: - operandNum = reflect.ValueOf(o).Int() + operandNum = int64(reflect.ValueOf(o).Uint()) default: if op.IsNumericOperator() { return false, fmt.Errorf("operand of type unsupported") From fad86153a3965ee0083b1ac2c76394d6eafdd36e Mon Sep 17 00:00:00 2001 From: khanhtc1202 Date: Tue, 15 Mar 2022 09:29:01 +0700 Subject: [PATCH 7/7] Return fast in case there is no filter --- pkg/datastore/filedb/filter.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/datastore/filedb/filter.go b/pkg/datastore/filedb/filter.go index 6823a2ff42..6dcc6cb551 100644 --- a/pkg/datastore/filedb/filter.go +++ b/pkg/datastore/filedb/filter.go @@ -29,6 +29,11 @@ type filterable interface { } func filter(col datastore.Collection, e interface{}, filters []datastore.ListFilter) (bool, error) { + // Always pass, if there is no filter. + if len(filters) == 0 { + return true, nil + } + // If the collection implement filterable interface, use it. fcol, ok := col.(filterable) if ok {