-
Notifications
You must be signed in to change notification settings - Fork 116
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
Case insensitive matching of field names #337
Conversation
Please give some more details of the fix and please avoid using internal tickets. |
gorm/fields.go
Outdated
if !ok { | ||
fmt.Printf("===> no field found") |
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.
Use log package
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.
couple of minor things and a question to be addressed
gorm/fields.go
Outdated
@@ -3,37 +3,39 @@ package gorm | |||
import ( | |||
"context" | |||
"fmt" | |||
"github.com/infobloxopen/atlas-app-toolkit/util" |
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 revert the change in import order. usually you can run go fmt ./...
in the parent directory and it will sort these for you
gorm/fields.go
Outdated
sf, ok := objType.FieldByName(util.Camel(f.GetName())) | ||
func handlePreloads(f *query.Field, objType reflect.Type, ignoreCase ...bool) ([]string, error) { | ||
queryFieldName := f.GetName() | ||
fmt.Printf("Query name = %v\n", queryFieldName) |
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.
use log package here instead of fmt
gorm/fields.go
Outdated
|
||
var sf reflect.StructField | ||
var ok bool | ||
if len(ignoreCase) > 0 && ignoreCase[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.
maybe I'm missing something but why is ignoreCase
a ...bool
instead of just a plain old bool
?
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.
This is an "optional" parameter for backward compatibility.
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.
ok that makes sense. if that's the case then make sure that there is a unti test for ignoreCase
being false
, and one for there being no ignoreCase
value at all
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.
Yes, there are. In test file, the function has been called as followed
FieldSelectionStringToGorm(context.Background(), test.fs, &Model{}) // no "ignoreCase" = false
FieldSelectionStringToGorm(context.Background(), test.fs, &Model{}, true)
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 might be nice to have at least one FieldSelectionStringToGorm(context.Background(), test.fs, &Model{}, false)
just for completion's sake
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
gorm/collection_operators.go
Outdated
@@ -16,7 +16,7 @@ type SortingCriteriaConverter interface { | |||
} | |||
|
|||
type FieldSelectionConverter interface { | |||
FieldSelectionToGorm(ctx context.Context, fs *query.FieldSelection, obj interface{}) ([]string, error) | |||
FieldSelectionToGorm(ctx context.Context, fs *query.FieldSelection, obj interface{}, ignoreCase ...bool) ([]string, error) |
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.
this needs to be a option struct{} not a boolean. consider the meaning is for multiple options.
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.
there is an issue returning the values for an object when requesting the list of DNS Server Groups (API object auth_nsg) via API when using the fields.
When there is no specific filter/tag applied in the query, everything looks good and the complete data is displayed.