-
Notifications
You must be signed in to change notification settings - Fork 209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add feature LIKE & NOT LIKE condition. #75
Conversation
@@ -245,6 +245,8 @@ One common reason to use this is to prevent string concatenation in a loop. | |||
* Gte | |||
* Lt | |||
* Lte | |||
* Lk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Like' will be better than 'Lk'
return nil | ||
} | ||
v := reflect.ValueOf(value) | ||
if v.Kind() == reflect.Slice { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not good to hide errors. need to return error or pass value as is.
I think array of runs is valid argument for like, but it has type slice.
// Otherwise it will be translated to `LIKE`. | ||
func Lk(column string, value interface{}) Builder { | ||
return BuildFunc(func(d Dialect, buf Buffer) error { | ||
if value == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO: it would be better avoid some logic here and pass arguments as is to underlayed level. if aguments is not valid, user will be notified about this via query builder error or via sql error.
@@ -245,6 +245,8 @@ One common reason to use this is to prevent string concatenation in a loop. | |||
* Gte | |||
* Lt | |||
* Lte | |||
* Lk | |||
* Nlk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NotLike will be better.
cond: Lk("col", nil), | ||
query: "", | ||
value: nil, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add test with array of runs.
@bgaifullin Thanks for your feedback! |
return fmt.Errorf("Column %s NOT LIKE nil is Not Supported", column) | ||
} | ||
|
||
v := reflect.ValueOf(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the common part of this 2 functions can be moved to separate function.
something like this:
func Like(column string, value interface{}) Builder { return BuildFunc(func(d Dialect, buf Buffer) error { return buildLikeCmp(d, buf, "NOT LIKE", column, value) }
case v.Kind() == reflect.TypeOf([]rune{}).Kind() && v.Len() != 0: | ||
return buildCmp(d, buf, "NOT LIKE", column, string(value.([]rune))) | ||
|
||
case v.Kind() == reflect.TypeOf([0]rune{}).Kind() && v.Len() != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
v := reflect.ValueOf(value) | ||
switch { | ||
case v.Kind() == reflect.TypeOf([]rune{}).Kind() && v.Len() != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is not needed to check that v.Len() != 0, becuase 'LIKE ""' is valid expression.
|
||
v := reflect.ValueOf(value) | ||
switch { | ||
case v.Kind() == reflect.TypeOf([]rune{}).Kind() && v.Len() != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please clarify why does condition 'v.Len() != 0' need here.
I think it is valid to use empty string in like operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the empty int slice and the byte slice were caught on Detect of the Rune slice so I temporarily left it.
I will fix it with the next commit so please wait for a while.
case v.Type() == reflect.TypeOf([]rune{}): | ||
return buildCmp(d, buf, pred, column, string(value.([]rune))) | ||
|
||
case v.Kind() == reflect.TypeOf([0]rune{}).Kind(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to avoid type checkin here. Because there is a lot of cases where value can be converted to proper string value.
there is also implemented logic to handle different interfaces and properly convert them into valid database value. (method encodePlaceholder)
In any case if user pass not valid value to your method, it will see result as failed sql query.
If you want to strict check that passed value is valid, I think it would be better to rewrite this function like this:
v := reflect.ValueOf(value)
switch v.Kind() {
case reflect.String:
// pass as is
return buildCmp(d, buf, pred, column, value)
case reflect.Ptr, reflect.Interface:
return buildLikeCmp(d, buf, pred, column, v.Elem.Interface())
case reflect.Slice:
switch v.Type().Elem().Kind() {
case reflect.Uint8: // bytes
// interpolator will handle this case
return buildCmp(d, buf, pred, column, value)
case reflect.Int32: // rune
// need to convert into string
// I am not sure that it is good idea to convert rune here
// I think it would be better to ask user to convert into string
return buildCmp(d, buf, pred, column, string(value([]rune)))
}
fallthrough
default:
return fmt.Errorf("Column %s %s Invalid Value, string expected", column, pred)
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed encodePlaceholder, it's a good idea, thank you! I will correct it!
}, | ||
{ | ||
cond: Lte("col", 1), | ||
query: "`col` <= ?", | ||
value: []interface{}{1}, | ||
isErr: false, | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
IMO: it would be great also add tests case for []byte
@kpango Thanks for this! Really useful for my current project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great feature, great tests. Just a couple small issues.
|
||
func buildLikeCmp(d Dialect, buf Buffer, pred string, column string, value interface{}) error { | ||
if value == nil { | ||
return fmt.Errorf("Column %s %s nil is Not Supported", column, pred) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use an exported error value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyler-smith I’m sorry, but because there was a problem with my repository management, I issued a new PR and the same content PR. And I fixed the pointed out matter.
} | ||
fallthrough | ||
default: | ||
return fmt.Errorf("Column %s %s Invalid Value, string expected", column, pred) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use an exported error value.
} | ||
} | ||
|
||
// Lk is `LIKE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the correct function name for golint.
}) | ||
} | ||
|
||
// Nlk is `NOT LIKE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the correct function name for golint.
Add LIKE & NOT LIKE feature to condition.go for fuzzy search using LIKE
LIKE syntax is below
NOT LIKE syntax is below