Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/datastore/datastore.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ const (
OperatorContains
)

func (o Operator) IsNumericOperator() bool {
return o == OperatorGreaterThan ||
o == OperatorGreaterThanOrEqual ||
o == OperatorLessThan ||
o == OperatorLessThanOrEqual
}

var (
ErrNotFound = errors.New("not found")
ErrInvalidArgument = errors.New("invalid argument")
Expand Down
12 changes: 10 additions & 2 deletions pkg/datastore/filedb/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,15 @@ 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",
"//pkg/model:go_default_library",
"@com_github_stretchr_testify//assert:go_default_library",
"@com_github_stretchr_testify//require:go_default_library",
],
)
173 changes: 170 additions & 3 deletions pkg/datastore/filedb/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,186 @@
package filedb

import (
"encoding/json"
"fmt"
"reflect"
"strings"
"unicode"

"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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return fast when there was no filter to avoid unneeded marshaling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch 💯

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed by fad8615 🙏

// 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 {
if ok {
return fcol.Match(e, filters)
}

// remarshal entity as map[string]interface{} struct.
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
}
Comment on lines +43 to +51
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way to deal with the entity struct directly instead of remarshal like this?
I feel that we are having many marshal-unmarshal around this part.

Copy link
Member Author

@khanhtc1202 khanhtc1202 Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To iterate throw struct and get field name as string, we only have using reflect or remarshal it to map[string]interface{} for now. I tried to do filter before first unmarshal (from object parts to object full data) but it does not work, since in some cases, the fields' values are stored duplicated between object parts, which make it's required to merge (unmarshal all object parts and build the whole object) once, then filter based on the data fulfilled object.

Besides, I read around the JSON marshal logic of the standard lib, and looks like it's called good/fast enough compare with others marshaling packages. So for now, I guess, I would like to make a try this method. If it's too costly this way, I will reconsider using reflect package or such. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree about that. Let's give it a try and see how it work. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 🙌


for _, filter := range filters {
field := convertCamelToSnake(filter.Field)
if strings.Contains(field, ".") {
// TODO: Handle nested field name such as SyncState.Status.
return false, datastore.ErrUnsupported
Comment on lines +56 to +57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: Or to be simple, we can avoid using nested fields by double storing the SyncState_Status in the Application model.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I planned to make logic like flatten nested json and do filter logic on the output map. But you're right, we may avoid complexity by unuse nested fields query. Will think about it and address with another PR 🙏

}

val, ok := omap[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) {
var valNum, operandNum int64
switch v := val.(type) {
case int, int8, int16, int32, int64:
valNum = reflect.ValueOf(v).Int()
case uint, uint8, uint16, uint32:
valNum = int64(reflect.ValueOf(v).Uint())
default:
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:
operandNum = int64(reflect.ValueOf(o).Uint())
default:
if op.IsNumericOperator() {
return false, fmt.Errorf("operand of type unsupported")
}
}

switch op {
case datastore.OperatorEqual:
return val == operand, nil
case datastore.OperatorNotEqual:
return val != operand, nil
case datastore.OperatorGreaterThan:
return valNum > operandNum, nil
case datastore.OperatorGreaterThanOrEqual:
return valNum >= operandNum, nil
case datastore.OperatorLessThan:
return valNum < operandNum, nil
case datastore.OperatorLessThanOrEqual:
return valNum <= operandNum, 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
Comment on lines +116 to +126
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits, it would be better to make this implementation a function and return directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not that different since the logic to return is different between cases 🤔

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
}
}

return fcol.Match(e, filters)
func makeSliceOfInterfaces(v interface{}) ([]interface{}, error) {
rv := reflect.ValueOf(v)
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())
for i := 0; i < rv.Len(); i++ {
vs[i] = rv.Index(i).Interface()
}

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
}
Loading