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.

Sign up for GitHub

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 "struct_references" configuration #155

Merged
merged 4 commits into from
Nov 13, 2021
Merged

Add "struct_references" configuration #155

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
19726c7
Add "weak_references" configuration
nathanstitt Nov 8, 2021
77da957
rename config to "StructReferences" & rework check
nathanstitt Nov 11, 2021
bf8b563
typo
benjaminjkraft Nov 13, 2021
d0bb6ad
code cleanup to fix lint warnings
nathanstitt Nov 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions docs/genqlient.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,17 @@ context_type: context.Context
# without making a query.
client_getter: "github.com/you/yourpkg.GetClient"


# if set, fields with a complex type will default to having
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# if set, fields with a complex type will default to having
# If set, fields with a struct type will default to having

(I think it's worth being explicit what types we mean.)

# the "pointer: true, omitempty: true" flag
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# the "pointer: true, omitempty: true" flag
# the "pointer: true, omitempty: true" flag.

#
# This can be useful for complex schema where it would be burdensome
# to manually set the flags on a large number of fields
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# to manually set the flags on a large number of fields
# to manually set the flags on a large number of fields.

#
# Defaults to false.
use_weak_references: boolean


# A map from GraphQL type name to Go fully-qualified type name to override
# the Go type genqlient will use for this GraphQL type.
#
Expand Down
1 change: 1 addition & 0 deletions generate/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ type Config struct {
ContextType string `yaml:"context_type"`
ClientGetter string `yaml:"client_getter"`
Bindings map[string]*TypeBinding `yaml:"bindings"`
WeakReferences bool `yaml:"use_weak_references"`

// Set to true to use features that aren't fully ready to use.
//
Expand Down
30 changes: 25 additions & 5 deletions generate/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func (g *generator) convertArguments(
// names.go) and the selection-set (we use all the input type's fields,
// and so on recursively). See also the `case ast.InputObject` in
// convertDefinition, below.
goTyp, err := g.convertType(nil, arg.Type, nil, options, queryOptions)
goTyp, err := g.convertType(nil, arg.Type, false, nil, options, queryOptions)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting, do you think we don't want to apply this to arguments? I would think we would; while of course there it's easier to set it seems like a more consistent default would be better. If you feel this is clearer I'm ok with that. Either way probably worth an explicit mention in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yes Interesting. I hadn't really considered allowing arguments, but don't see why not. Plus, allowing it will simplify the code by removing the need to pass around the isStructField argument.

if err != nil {
return nil, err
}
Expand Down Expand Up @@ -218,6 +218,7 @@ func (g *generator) convertArguments(
func (g *generator) convertType(
namePrefix *prefixList,
typ *ast.Type,
isStructField bool,
selectionSet ast.SelectionSet,
options, queryOptions *genqlientDirective,
) (goType, error) {
Expand All @@ -235,16 +236,23 @@ func (g *generator) convertType(
if typ.Elem != nil {
// Type is a list.
elem, err := g.convertType(
namePrefix, typ.Elem, selectionSet, options, queryOptions)
namePrefix, typ.Elem, isStructField, selectionSet, options, queryOptions)
return &goSliceType{elem}, err
}

// If this is a builtin type or custom scalar, just refer to it.
def := g.schema.Types[typ.Name()]

goTyp, err := g.convertDefinition(
namePrefix, def, typ.Position, selectionSet, options, queryOptions)

if options.GetPointer() {
if g.getWeakReference(options, isStructField, def) {
if options.Omitempty == nil {
oe := true
options.Omitempty = &oe
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mutating this seems a bit awkward. Can we instead just put this in where we call GetOmitempty as well? That will also help with the comment below I suspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, but I played with a few different options here but didn't find anything better. GetOmitempty is called in a few spots, and we'd have to pass the Config and field definition into it and then duplicating the logic of the getStructReference check.

The updated check I've came up with here is a bit cleaner IMO, so see what you think

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, fair enough!

}
goTyp = &goPointerType{goTyp}
} else if options.GetPointer() {
// Whatever we get, wrap it in a pointer. (Because of the way the
// options work, recursing here isn't as connvenient.)
// Note this does []*T or [][]*T, not e.g. *[][]T. See #16.
Expand All @@ -253,6 +261,18 @@ func (g *generator) convertType(
return goTyp, err
}

// getWeakReference decides if a field should be of pointer type and have the omitempty flag set.
func (g *generator) getWeakReference(
options *genqlientDirective,
isStructField bool,
def *ast.Definition,
) bool {
return options.Pointer == nil &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this will be a little confusing in that it will mean if you do omitempty: false it will be ignored, but if you do pointer: false it will not only work but also turn off omitempty. (And in fact if you do pointer: true it will also turn off omitempty!) If we can, I think it'll be better to check each setting separately, so that if you set pointer explicitly that overrides pointer, like you have it, but not omitempty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, you're correct. The logic is slightly flawed here as you describe. I'll rework

isStructField &&
g.Config.WeakReferences &&
len(def.Fields) > 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh so actually a question here is whether use_weak_references should apply to interface-typed fields. Unless you see a strong reason, my feeling is not -- pointer-to-interface types are super awkward in Go. In that case this should be def.Kind == ast.Object || def.Kind == ast.InputObject (or something like that).)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, I hadn't considered the interface-typed fields. I don't see why we shouldn't use pointers for those, that way the rule doesn't have any gotchas

Copy link
Collaborator

Choose a reason for hiding this comment

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

They just tend to be awkward in that you have to explicitly dereference to call the interface's methods (i.e. (*resp.F).M()) and confusing in that the value inside the interface is often itself a pointer. I think it's clean to explain either way -- if anything since the option is "struct_references" that would suggest it's just structs. We could rename it and do interfaces but I feel that's only going to be annoying.

Another question that we had discussed in the issue is whether it applies to output objects. I think "what is easiest to explain" is also a good criterion there, but it could go either way (if inputs, we'd call it "input_references" and it makes the interfaces question moot since GraphQL doesn't have interfaces in inputs).

}

// convertDefinition decides the Go type we will generate corresponding to a
// particular GraphQL named type.
//
Expand Down Expand Up @@ -404,7 +424,7 @@ func (g *generator) convertDefinition(
// will be ignored? We know field.Type is a scalar, enum, or input
// type. But plumbing that is a bit tricky in practice.
fieldGoType, err := g.convertType(
namePrefix, field.Type, nil, fieldOptions, queryOptions)
namePrefix, field.Type, true, nil, fieldOptions, queryOptions)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -827,7 +847,7 @@ func (g *generator) convertField(
namePrefix = nextPrefix(namePrefix, field)

fieldGoType, err := g.convertType(
namePrefix, field.Definition.Type, field.SelectionSet,
namePrefix, field.Definition.Type, true, field.SelectionSet,
fieldOptions, queryOptions)
if err != nil {
return nil, err
Expand Down
4 changes: 4 additions & 0 deletions generate/generate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ func TestGenerateWithConfig(t *testing.T) {
Generated: "generated.go",
ContextType: "github.com/Khan/genqlient/internal/testutil.MyContext",
}},
{"WeakReferences", "", &Config{
WeakReferences: true,
Generated: "generated-weakrefs.go",
}},
{"NoContext", "", &Config{
Generated: "generated.go",
ContextType: "-",
Expand Down
59 changes: 59 additions & 0 deletions ...ta/snapshots/TestGenerateWithConfig-WeakReferences-testdata-queries-generated-weakrefs.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.