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

Use Struct Field Resolver instead of Method #194

Closed
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
@salman-ahmad
Copy link
Contributor

salman-ahmad commented Apr 11, 2018

This fixes #28. Essentially, it reduces lot of boilerplate code by making use of struct field as resolvers instead of creating methods for each struct field.

Now, methods are required only when

  1. a struct implements a GraphQL interface. In this case, you create methods for fields which overlap
  2. a struct exposes a GraphQL field that accepts additional arguments i.e., first: Int, last: Int etc.
  3. there is GraphQL union type

By using struct fields as resolvers, one could also benefit from DRY codebase by not having a pair of a struct which look the same (one for GraphQL and one for DB)

Does this break existing API?

No. This change is completely transparent and you can continue using your existing codebase.

Can I still continue using methods as resolvers?

Yes, by default, it uses method resolvers.

How can I use struct fields as resolvers?

When invoking graphql.ParseSchema() or graphql.MustParseSchema() to parse the schema, you pass in graphql.UseFieldResolvers() as an argument. For an example, take a look at ./example/social/server/server.go

Additional Notes

  • also fixes #124
  • for an example, take a look at ./example/social/social.go & ./example/social/server/server.go
  • Unit tests are missing

Salman Ahmad and others added some commits Mar 29, 2018

Merge pull request #1 from DealTap/impl-field-resolver
Use Struct Field Resolver instead of Method
@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented Apr 11, 2018

@tonyghita, please take a look at this when you get a chance, thanks.

@mytototo

This comment has been minimized.

Copy link

mytototo commented Apr 18, 2018

Finally ! Thanks a lot @salmana1 this looks promising.
Any milestone to see support this officially out-of-the-box ?

Salman Ahmad and others added some commits Apr 20, 2018

Salman Ahmad
Handle parsing of `time.Time` type struct fields
Also updated `social.go` example
Merge pull request #3 from DealTap/handle-go-time
Handle parsing of `time.Time` type struct fields
Merge pull request #4 from DealTap/bug-union-type
Fixed bug to handle Union type
@mbinge

This comment has been minimized.

Copy link

mbinge commented Apr 25, 2018

please merge request!!!

@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented May 8, 2018

@tonyghita any ETA on when you would have some time to review (and approve) this?

@pavelnikolov
Copy link
Member

pavelnikolov left a comment

This is an awesome PR! Thank you very much 🎉
I'd would vote 👍 for this change!
However, it would be better if you submit unrelated changes in separate PRs. Please, submit a separate PRs for:

  1. resolving IDs as strings (I'd vote 👎 )
  2. <field_name>Resolver to be used if available (I'd vote 👎 )

Sending multiple changes in a single PR makes it much harder to review. Also, if someone doesn't agree with all of the changes, it would take a lot of time to get your PR merged.

Thanks again! 🥇 🏆


To run this server

`go ./example/field-resolvers/server/server.go`

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

Did you mean go run?


func init() {
opts := []graphql.SchemaOpt{graphql.UseFieldResolvers(), graphql.MaxParallelism(20)}
schema = graphql.MustParseSchema(social.Schema, &social.Resolver{}, opts...)

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

(minor) Is there any reason for these to be in the init() func? They could easily be just live in a var( block or live in main().

type Query {
admin(id: ID!, role: Role = ADMIN): Admin!
user(id: ID!): User!
search(text: String!): [SearchResult]!

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

Fix indentation, please.

case *string:
implementsType = (t.Name == "String")
// allow ID of type string
implementsType = t.Name == "String" || t.Name == "ID"

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

This is unrelated change. Please, send a separate PR for it.

}

type user struct {
Id string

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

Please, renameId -> ID.

func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, possibleTypes []*schema.Object, nonNull bool, resolverType reflect.Type) (*Object, error) {
func (b *execBuilder) makeObjectExec(typeName string, fields schema.FieldList, possibleTypes []*schema.Object,
nonNull bool, resolverType reflect.Type) (*Object, error) {

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

Please, remove empty line.

// 1.2) Or field has arguments
// 1.3) Or it is configured to use method
// 1.4) Or it is an interface
// 2) Otherwise use resolver type's field

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

Please, reformat the comment as:

		// 1. Use method resolver when:
		//	a. __Type and __Schema requests; or
		// 	b. The requested field has arguments; or
		//	c. The schema is not configured to use field resolvers; or
		//	d. It is an interface
		// 2. Otherwise, use a field resolver
// 1.4) Or it is an interface
// 2) Otherwise use resolver type's field
if isResolverSchemaOrType(rt) == true || len(f.Args) > 0 ||
b.schema.UseFieldResolvers == false || rt.Kind() == reflect.Interface {

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

UseFieldResolver is already a boolean, right? Can't you just use !b.schema.UseFieldResolvers? Or you want it to be more verbose?

This comment has been minimized.

@salman-ahmad

salman-ahmad May 14, 2018

Author Contributor

imho, ! can be overlooked sometime so more verbose make it clear that it's a check for false. I can change this to !

This comment has been minimized.

@pavelnikolov

pavelnikolov Oct 10, 2018

Member

I prefer using ! because it'll be consistent with the rest of the code in this library.

}
if err := b.assignExec(&a.TypeExec, impl, resolverType.Method(methodIndex).Type.Out(0)); err != nil {
return nil, err
if b.schema.UseFieldResolvers == false || resolverType.Kind() != reflect.Interface {

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

again <bool> == false vs !<bool? Do we want the more verbose version? IMHO, it's better to use !...

func findMethod(t reflect.Type, name string) int {
resName := name + "Resolver"

This comment has been minimized.

@pavelnikolov

pavelnikolov May 10, 2018

Member

This is completely unrelated change (i.e. finding method with name <field>Resolver). Please, send this in a separate PR. I'd personally, vote 👎 for this specific change, since adds unnecessary complexity to the library. Less is more in this case.

This comment has been minimized.

@salman-ahmad

salman-ahmad May 14, 2018

Author Contributor

This is not an unrelated change. As mentioned in the PR notes, when a struct implements an interface, then struct's resolver methods are used for each interface field. Hence, when a struct has a field Email and interface has the same field, then there will be a conflict between struct field and resolver method. So a Resolver suffix can be used to resolve that conflict. If you can suggest something else to achieve the same goal then I will be happy to implement that

This comment has been minimized.

@pavelnikolov

pavelnikolov May 14, 2018

Member

As a workaround you can just use different struct fields. If you are implementing an interface, it's just the methods that will be used, right? This would mean that you can use differnt field names.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented May 10, 2018

@tonyghita, please, review this change as it is opt-in and would save us from writing a lot of boilerplate code. 🎉 🆒 😎 👍 ❤️

I'm curious to see the benchmark comparison between the field and method resolvers. 📈 📉 📊

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented May 10, 2018

@neelance, is this getting a step closer to what you suggested in #88 (comment) or is it a move in a different direction?

@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented May 14, 2018

@pavelnikolov thanks for reviewing the code and I will fix most of them.

I totally understand what you are saying about multiple PRs. Please note that, this PR is coming from downsteam master branch which has bunch of fixes and we needed them for our usecase. I do not have the permission to change dowstream branch so I will try to do some git magic to avoid multiple things in this PR.

resolving IDs as strings (I'd vote 👎 )

I think this is useful change and it's working well for my company. We don't have to wrap or unwrap graphql.ID to string every time. But I can create a separate PR for it.

<field_name>Resolver to be used if available (I'd vote 👎 )

This is relevant to this PR and I have replied to your review comment

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented May 15, 2018

This is relevant to this PR and I have replied to your review comment

I'm still not 100% convinced that this is necessary. You can definitely change the field names in the struct or use a different struct and still implement an interface. I think that it should live in a separate PR.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented May 16, 2018

I'll create a test schema and will run benchmarks comparing the master branch against your branch with both fields and methods

@spiffyjr

This comment has been minimized.

Copy link

spiffyjr commented Jun 23, 2018

Can this get some attention? This is pretty handy for reducing the amount of boilerplate needed.

@iamtakingiteasy

This comment has been minimized.

Copy link

iamtakingiteasy commented Aug 15, 2018

+1, This feature would be very practical.

@eveld

This comment has been minimized.

Copy link

eveld commented Aug 22, 2018

In large applications this would take away a lot of boilerplate code. Is there anything that still needs to happen for this to be merged?

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Aug 22, 2018

@tonyghita is this in your roadmap? Any feedback about this PR?

@marcinwyszynski

This comment has been minimized.

Copy link

marcinwyszynski commented Sep 3, 2018

Thanks for working on that @salmana1 ! 🙇🏼‍♂️

@ppwfx

This comment has been minimized.

Copy link

ppwfx commented Oct 6, 2018

push

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Oct 8, 2018

@salmana1

I totally understand what you are saying about multiple PRs. Please note that, this PR is coming from downsteam master branch which has bunch of fixes and we needed them for our usecase. I do not have the permission to change dowstream branch so I will try to do some git magic to avoid multiple things in this PR.

I'd be happy to consider the struct field resolvers once you split this PR.

@bazaglia

This comment has been minimized.

Copy link

bazaglia commented Oct 9, 2018

+1!!

This is a must-have for reducing drastically the codebase.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Oct 10, 2018

@salmana1 are you willing to split this PR? I'd be happy to review/merge the field resolvers.

@bazaglia

This comment has been minimized.

Copy link

bazaglia commented Oct 11, 2018

@salmana1 Let me know if you need any help.

Also, when passing graphql.UseFieldResolvers(), it ALWAYS uses the value from struct, except in the cases you described (like a field has arguments). What I expected was to use the struct value only if fieldResolver doesn't exists. Is that possible at all with the current implementation?

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Oct 11, 2018

@bazaglia Thanks for the feedback. Any feedback is highly appreciated! Can you please add some samples / unit-tests?

@bazaglia

This comment has been minimized.

Copy link

bazaglia commented Oct 11, 2018

@pavelnikolov I don't have enough Go experience to write unit tests. I just tested the functional part of the PR, and it worked really well, resulting in much cleaner code. My only consideration is the way it's currently implemented, it looks like the developer has to choose OR to use struct fields as resolver OR to use the methods as the resolver (which is the standard in current graphql-go).

My suggestion is to go for a hybrid approach: resolve data from the struct value, which is the default way, but also allows to override the struct value, using a method to do it. This approach is also really close to graphql-js implementation: there's a default resolver, which is the value of the field from struct, but also allows implementing a function, to set the resolver.

I really wish I had experience with Go to help with this functionality, I think this PR is so great as it makes the code much more legible & readable.

@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented Oct 12, 2018

Sorry for not replying sooner.

I will try to update this PR soon. I don't have access to the original forked branch; so, I will probably have to close this PR and recreate it.

On a separate note, I would recommend to take a look at gqlgen library. At my last job, we switched to that library after we realized this library was not actively developed and PRs weren't getting merged.

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Oct 13, 2018

Thanks for your contribution, it is highly appreciated!
This project is now actively maintained again and PRs/issues are responded promptly.

P.S. It might take a wile until all existing issues and PRs are cleaned up. Thank you all for your patience. The current priority is subscriptions...

@tobstarr

This comment has been minimized.

Copy link

tobstarr commented Oct 25, 2018

I would really like to see this PR getting merged!

@pavelnikolov

This comment has been minimized.

Copy link
Member

pavelnikolov commented Oct 28, 2018

@tobstarr I am thinking of an implementation that would keep the current method implementation as default but would allow the users of the library to override the resolving implementation as they see fit. I'll post an update soon. Chances are that this PR will not be merged in its current form.

@bazaglia

This comment has been minimized.

Copy link

bazaglia commented Oct 29, 2018

@pavelnikolov nice, that's my upvote also, just like I commented before.

@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented Oct 30, 2018

I will be pushing out a cleaned up PR tomorrow and will close this PR.

@bazaglia

Your suggestion is really good and I am happy to discuss it with you guys in detail.

@salman-ahmad

This comment has been minimized.

Copy link
Contributor Author

salman-ahmad commented Oct 30, 2018

Closing this PR - please see the new PR #282

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.