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 configuration AutoBindings method #169

Merged
merged 8 commits into from
Jun 17, 2022

Conversation

NuVivo314
Copy link
Contributor

This proposal is same idea config file: gqlgen AutoBind.

My personal issue is,

I server generate by gqlgen, and model is created by generator.

Without auto_bindings, genqlient make duplicated structure.

And i 300 types, i can't manual binds.

This change:

  • Add in config file auto_bindings string
  • Add in exemple file.

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

Neat idea, thanks for the contribution! The main thing I'd like to work out before merging this is to document clearly what it does and make sure that we make sure the configuration API is right. To do that I'd like to understand your use cases a bit more.

For the documentation, based on the implementation, I think it's this:

Set to a package path to automatically add all the types in that package to bindings below. This is equivalent to adding an entry
TypeName:
type: path/to/package.TypeName
for each exported type in path/to/package. Explicit entries in bindings take precedence.

But the other question that adds is: is that actually ever the right behavior? It seems to me that that's only useful behavior in cases where the server and client are sharing a codebase, which I would think isn't very common. And it means that you'll often have a bunch of fields that don't get set (because they aren't requested by the query). To me that seems like a big foot-gun; if there are good reasons to add it it's not much code so I don't mind, but I'd like to make sure we document clearly when it might be useful and what the drawbacks are. Could you explain a bit more how you're using genqlient (and gqlgen) such that this comes up?

The other question is whether this is the right behavior. Thinking out loud about few places where it might not be:

  • initialisms; for a type MyURL it'll probably be MyUrl in GraphQL. I'm not sure if there's a good way to figure that out (maybe we could look at gqlgen config but that seems way too magical); so probably people will just have to add an explicit entry.
  • if there's a need to set marshalers or unmarshalers; that would seem rare so it's probably fine that people just have to add an explicit entry
  • setting expect_exact_fields (or in the future "includes" and "subset of"); in principle one could have a setting to auto-detect that as well based on struct fields. But I wonder if that's ever right; this goes back to the issue above where you probably don't want to always ask for all the fields, but now that potentially means trouble for your clients.
  • does it make sense to bind every type, even, I don't know, non-structs? For gqlgen packages for the same schema probably it's fine.
  • might you want multiple packages? This is easy via StringList, as I comment below, but maybe there's no sensible way to want to do it.
    None of those seem like major issues, but if we want to add space for expansion points we could make the config look more like
auto_bindings:
- package: path/to/package
  # then in the future we can add
  some_option: true
- package: path/to/other/package

(which in Go is just []AutoBinding where AutoBinding is struct{ Package string; ... }).
I think it will be easiest to understand that once I understand your use cases a bit better.

generate/config.go Outdated Show resolved Hide resolved
generate/config.go Outdated Show resolved Hide resolved
@benjaminjkraft
Copy link
Collaborator

Ah, one more thing: assuming we do go forward with it this will need some tests! I think the easiest thing is probably to add it to TestGenerateWithConfig in generate/generate_test.go -- perhaps just point it to the gqlgen models in internal/integration/server.

benjaminjkraft added a commit that referenced this pull request Feb 10, 2022
It's been a while, time for a release!  This commit updates the
changelog, and I'll tag it with the release once it lands.

Fixes #163.  I'll wait for #169 and #170 to land before merging this.

Test plan: Craig tested a fairly recent main branch in webapp.
@benjaminjkraft benjaminjkraft mentioned this pull request Feb 10, 2022
6 tasks
@NuVivo314
Copy link
Contributor Author

Hello @benjaminjkraft,

I prepare your request.
But i will answer some questions.
Yes this first implement is naive. One package. And no-filtering if use on query.

My use case is, server (dgraph) for him I am looking for a client graphql.
And my api exposed with GraphQL server, but y don't want seeing client call directly dgraph.
DGraph help me to make GQL type and input, and i generate hard structure (no interface{} or map[string]string).
(dgraph eat schema and in output a consolidated version with all the CRUD directives)

And y expose most dgraph new type with qglgen server (yes this link to #170 ) and my own Query/Mutation

2 solutions,

  • update genqlient to auto-binding feature
  • make bridge with map mapstructure between gqlgen type and genqlient type.

here is my use of autobinding

@csilvers csilvers removed their request for review March 1, 2022 17:36
@benjaminjkraft
Copy link
Collaborator

Thanks for writing back! If I understand correctly, I guess what you're doing is to have your server act as a proxy:

end-user --> your server --> dgraph

so your server wants to be both a client and a server of the (mostly-?)same GraphQL schema. It's hard to say if genqlient will be the best client for you, without knowing more, but this is at least helpful to understand, and I can see why you'd want this. It sounds in some ways similar to building a gateway, so I want to cc @dnerdy to see if he has any thoughts on gotchas or whether this is a sensible thing, but my inclination is since it's not too invasive to implement it's fine to err on the side of adding it. (Or we could decide it's time for plugins, but that's a more complicated question and probably it's not.)

I agree it's not important to support all possible things people might want out of such a feature in the first version. The main thing I'm concerned about is just avoiding breaking changes later; thus the structure of config I suggested. It's fine if for now we can only allow one package, although I suspect if we do it fairly naively it's not hard to allow several (just loop over them).

@benjaminjkraft
Copy link
Collaborator

Thanks for the updates! I've pushed a few updates mostly to the documentation; the only thing remaining is if you could sign the Khan Academy CLA, then we can merge this.

@NuVivo314
Copy link
Contributor Author

NuVivo314 commented Jun 16, 2022

Hello,

I resolve minor merge conflit.

Ready to merge ?

Thx.

@benjaminjkraft benjaminjkraft merged commit 093054e into Khan:main Jun 17, 2022
@benjaminjkraft
Copy link
Collaborator

Done, thanks for your contributions!

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