Skip to content
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 BindFields function tool #197

Merged
merged 7 commits into from Jul 10, 2017
Merged

Add BindFields function tool #197

merged 7 commits into from Jul 10, 2017

Conversation

jekiapp
Copy link
Contributor

@jekiapp jekiapp commented Apr 8, 2017

I've been working with graphql-go lately and realize I'm pretty lazy to create object type for every struct I had. So I made a binder/wrapper function to easily bind all fields in my struct into graphql object.

So if you have this struct

type Person struct {
    Name    string  `graph:"name"`
    Address string  `graph:"address"`
}

you can create person type with

personType := graphql.NewObject(graphql.ObjectConfig{
        Name:   "Person",
        Fields: BindFields(Person{}),
    })

in file util_test.go I made an example with Person struct that looked like this:

type Person struct {
	Human //embedded Fields
	Name    string   `json:"name"` // standard scalar type
	Home    Address  `json:"home"` // struct type
	Hobbies []string `json:"hobbies"` // slice of scalar type 
	Friends []Friend `json:"friends"` // slice of struct type
}

type Human struct {
	Alive  bool    `json:"alive"`
	Age    int     `json:"age"`
	Weight float64 `json:"weight"`
}

type Friend struct {
	Name    string `json:"name"`
	Address string `json:"address"`
}

type Address struct {
	Street string `json:"street"`
	City   string `json:"city"`
}

because this function will scan through empty struct so this function cannot accept recursive type. that would make a stack-overflow error. And one more utility function which is BindArgs that can bind the args depending on provided params.

@coveralls
Copy link

coveralls commented Apr 8, 2017

Coverage Status

Coverage increased (+0.05%) to 82.65% when pulling 40986ba on ahmadmuzakki29:util into 170c5b5 on graphql-go:master.

bbuck
bbuck previously requested changes Apr 14, 2017
util.go Outdated

func getGraphList(tipe reflect.Type) *List {
switch tipe {
case reflect.TypeOf([]int{}):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a fan of all the reflect usage here, you've already got a reflect.Type, you can use it. like here case tipe.Kind() == reflect.Slice && tipe.Elem().Kind() == reflect.Int and other tests case tipe.Kind() == reflect.Slice && tipe.Elem().Kind() == reflect.Int8, etc...

@bbuck
Copy link
Collaborator

bbuck commented Apr 14, 2017

I'm still not a fan of auto-binding nested structs, which is exactly the opposite of how I (and all the demos I've ever seen of how to use/configure GraphQL recommend). The point is to make sub records searchable in their own right. For example, you should be able to bind recursively. If you have a me query endpoint that returns a Person type, and a Person struct has a field Friends []Person that should return a [Person] and GraphQL's system handle it perfectly. That is what it's designed to do (not just recursive, but nested/linked data in this manner).

@jekiapp
Copy link
Contributor Author

jekiapp commented Apr 15, 2017

if you mean nested structs by Person struct is Human then Human struct is supposed to be on the same level. so the Person struct is like having all the fields that Human has.

type Person struct{
	Alive  bool    `json:"alive"`
	Age    int     `json:"age"`
	Weight float64 `json:"weight"`
	Name string    `json:"name"`
       Address Address `json:"address"`
       // and so on ....
}

in that struct the sub record is address, so yes you can query Address in their own right.
But if you take example like Friends with type of Person this function will loop forever because it will bind the Person recursively.
appreciated your input, but I'm interested to hear another people opinion about this approach.

@zeucxb
Copy link

zeucxb commented Jun 4, 2017

Waiting

@coveralls
Copy link

coveralls commented Jul 8, 2017

Coverage Status

Coverage increased (+0.05%) to 82.129% when pulling 6611f08 on ahmadmuzakki29:util into fe52096 on graphql-go:master.

@jekiapp
Copy link
Contributor Author

jekiapp commented Jul 8, 2017

@zeucxb I updated the PR and hope it can be accepted to merge, if it's not then feel free to just copy util.go and place it in your package

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 82.129% when pulling 6611f08 on ahmadmuzakki29:util into fe52096 on graphql-go:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 82.129% when pulling 6611f08 on ahmadmuzakki29:util into fe52096 on graphql-go:master.

@chris-ramon chris-ramon dismissed bbuck’s stale review July 10, 2017 00:40

Thanks for reviewing @bbuck, agree on that, but it's good enough as it is FMHO

@chris-ramon
Copy link
Member

This util function is very awesome!, @ahmadmuzakki29 thanks a lot for taking the time.

I believe this is the initial function for more useful always optional time-savers functions, let's improve the recursive part and other improvements on new PR's!, thanks! 🚢

@chris-ramon chris-ramon merged commit 3e619b6 into graphql-go:master Jul 10, 2017
@jekiapp
Copy link
Contributor Author

jekiapp commented Jul 10, 2017

Sure thanks!

@axetroy
Copy link

axetroy commented Nov 20, 2017

I got the error

"reflect: call of reflect.Value.NumField on ptr Value"

and here is my struct, can anyone help?

type Post struct {
	ID        string       `json:"id"`
	UID       string       `json:"uid"`
	Cid       string       `json:"cid"`
	ShortId   string       `json:"shortId"`
	Title     string       `json:"title"`
	Summary   string       `json:"summary"`
	Content   string       `json:"content"`
	Gallery   []string     `json:"gallery"`
	Like      int32        `json:"like"`
	Comment   int32        `json:"comment"`
	Share     int32        `json:"share"`
	View      int32        `json:"view"`
	Tags      []string     `json:"tags"`
	IsShow    bool         `json:"isShow"`
	IsActive  bool         `json:"isActive"`
	IsTop     bool         `json:"isTop"`
	CreatedAt rpcbase.Time `json:"createdAt"`
	UpdatedAt rpcbase.Time `json:"updatedAt"`
}

@bbuck
Copy link
Collaborator

bbuck commented Nov 20, 2017

@axetroy How are you calling the tool? Also it might be better to create your own issue for this, as this issue is related to merging the tool in, not bugs and/or issues arising from using it.

@jekiapp
Copy link
Contributor Author

jekiapp commented Nov 21, 2017

@axetroy I suspect rpcbase.Time causing it. please create the fields manually for CreatedAt and UpdatedAt, and set it like this

CreatedAt rpcbase.Time `json:"-"`
UpdatedAt rpcbase.Time `json:"-"`

also I'm considering to return the string representation of struct instead of creating an object of it. to remove this kind of problem.

@limoli
Copy link

limoli commented Oct 23, 2018

Same problem with a struct having custom fields time.Time.
I think that bindFields doesn't handle time types.
@jekiapp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants