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

accept an optional ctx parameter on model methods #422

Merged
merged 7 commits into from
Nov 16, 2018

Conversation

gracenoah
Copy link
Contributor

@gracenoah gracenoah commented Nov 11, 2018

This PR allows you to take a context on model methods. This makes it much easier to transition from graph-gophers/graphql-go to this library. By configuring all of your graph-gophers resolvers as "models" in gqlgen, you can reuse existing code and migrate gradually instead of having to rewrite all resolvers in order to migrate.

Testing and docs coming up after I get some initial feedback on the concept and implementation.

Edit: I ran into one more thing that prevents me from being able to transition gradually. Models can't be returned from resolvers as pointers unless they are optional. All of my graph-gophers resolvers are pointers. I'm considering a temporary hack to make all models passed as pointers or converting all graph-gophers resolvers to non-pointers (there was no real reason for them to be pointers). I'd love some tips if anyone has done this before.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@vektah
Copy link
Collaborator

vektah commented Nov 11, 2018

Overall I think this makes sense, one thing you might run into though is gqlgen doesn't run bound model methods in parallel, its assumed they don't hit the database. Its an optimization mostly to avoid bound fields from spawning goroutines. Maybe context is an indicator that it should be run in parallel?

We are thinking about making all non primative return types pointers, which may help the second issue. See #375

@gracenoah
Copy link
Contributor Author

gracenoah commented Nov 12, 2018

Ah, thanks for the tip about not running in parallel. That might make it infeasible to reuse the resolvers without rewriting them. I'll try to address that in this PR as well. Parallelizing if there's a context makes sense to me.

P.S. Added my +1 on the pointers issue.

@gracenoah
Copy link
Contributor Author

I don't see any existing tests for model methods. Any suggestions on where I should add the tests?

@vektah
Copy link
Collaborator

vektah commented Nov 12, 2018

You can create a model in codegen/testserver and wire it into that config, then add tests to generated_test.go (or create a new file like modelctx_test.go) in that directory.

@gracenoah
Copy link
Contributor Author

Tests added. Please review.

@vektah
Copy link
Collaborator

vektah commented Nov 13, 2018

You need to run a go generate ./... and commit the changes. Seeing how the generated code has changed makes reviewing easier (even if it artificially inflates line counts)

@gracenoah
Copy link
Contributor Author

Sorry, didn't realize that resolver.go is generated. Updated.

@gracenoah gracenoah force-pushed the model-method-context branch 2 times, most recently from 9407014 to c0590c3 Compare November 14, 2018 00:02
@gracenoah
Copy link
Contributor Author

Sorry about the testing screwups. It's green now.

@vektah
Copy link
Collaborator

vektah commented Nov 16, 2018

Awesome, thanks for the PR.

Looks like we could use a guide for migrating from gophers, If anyone has a large codebase they have successfully migrated it would be great to get their notes (maybe in the recipes section?).

@vektah vektah merged commit d25e3b4 into 99designs:master Nov 16, 2018
cgxxv pushed a commit to cgxxv/gqlgen that referenced this pull request Mar 25, 2022
accept an optional ctx parameter on model methods
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.

2 participants