Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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 IsNumericOperator(op Operator) bool {
return op == OperatorGreaterThan ||
op == OperatorGreaterThanOrEqual ||
op == OperatorLessThan ||
op == OperatorLessThanOrEqual
}

Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
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",
],
)
164 changes: 161 additions & 3 deletions pkg/datastore/filedb/filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,177 @@
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 🙏

fcol, ok := col.(filterable)
if !ok {
if ok {
return fcol.Match(e, filters)
}

// remarshal entity as map[string]interface{} struct.
raw, _ := json.Marshal(e)
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
var omap map[string]interface{}
if err := json.Unmarshal(raw, &omap); err != nil {
return false, err
}
Comment on lines +43 to +51

Copy link
Copy Markdown
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.

@khanhtc1202 khanhtc1202 Mar 15, 2022

Copy link
Copy Markdown
Member Author

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
Copy Markdown
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
Copy Markdown
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 thread
khanhtc1202 marked this conversation as resolved.
Comment on lines +56 to +57

Copy link
Copy Markdown
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
Copy Markdown
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, uint64:
valNum = reflect.ValueOf(v).Int()
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
valNum = reflect.ValueOf(v).Int()
valNum = int64(reflect.ValueOf(v).Uint())

Sorry for the lack of explanation.
if the underlying type is uint, Int() will be panic(https://pkg.go.dev/reflect#Value.Int). So calling Uint() and then casting to int is better.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oops, thanks for explanation 👍

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()
Comment thread
khanhtc1202 marked this conversation as resolved.
Outdated
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 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
Copy Markdown
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
Copy Markdown
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
}
}

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
}

return fcol.Match(e, filters)
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