Add "struct_references" configuration #155

nathanstitt · 2021-11-08T16:02:02Z

Discussion around this: #149

Adds a "use_weak_references" configuration that will:

Use a pointer type for struct fields that are a complex type
- The behaviour can be overridden by setting "pointer: false" in the
  proceeding comment blocks
Sets the "omitempty: true" flag on fields matching the criteria.
This can also be overriden by setting "omitempty: false"

Comments & suggestions are very welcome, I'm not at all sure this is the best way to implement this.

Although we'd debated not setting the pointer for array elements, it did turn out to be simpler to set them everywhere and also made the documentation cleaner to have a single rule to explain.

Adds a "use_weak_references" configuration that will: * Use a pointer type for struct fields that are a complex type * The behaviour can be overridden by setting "pointer: false" in the proceeding comment blocks * Sets the "omitempty: true" flag on fields matching the criteria. This can also be overriden by setting "omitempty: false"

nathanstitt · 2021-11-08T16:03:23Z

@benjaminjkraft as I mentioned, any and all suggestions are welcomed 😁 No idea if this conforms to what you'd expect for the codebase

benjaminjkraft

Thanks so much for doing a PR of this! The way you did it it's not nearly as awkward as I feared it might be. As you say, I think it's worth trying to make the rules as simple as possible, so several of my comments are on things I feel are a bit inconsistent.

The one other thing that jumps out is I don't like the name. (I know, you asked for my opinion earlier and I was noncommital, sorry.) I don't have any great ideas and I don't think we'll find something perfect, but "weak references" has too much of a specific meaning elsewhere (i.e. a reference that doesn't protect the target from GC). Maybe struct_references: true or struct_default: pointer_and_omitemty or something?

benjaminjkraft · 2021-11-10T04:29:32Z

docs/genqlient.yaml

@@ -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
+# the "pointer: true, omitempty: true" flag


Suggested change

# the "pointer: true, omitempty: true" flag

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

benjaminjkraft · 2021-11-10T04:30:53Z

docs/genqlient.yaml

@@ -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


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.)

benjaminjkraft · 2021-11-10T04:30:59Z

docs/genqlient.yaml

+# 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


Suggested change

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

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

benjaminjkraft · 2021-11-10T04:32:52Z

generate/convert.go

+	if g.getWeakReference(options, isStructField, def) {
+		if options.Omitempty == nil {
+			oe := true
+			options.Omitempty = &oe


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.

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

Yeah, fair enough!

benjaminjkraft · 2021-11-10T04:35:48Z

generate/convert.go

+	isStructField bool,
+	def *ast.Definition,
+) bool {
+	return options.Pointer == nil &&


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.

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

benjaminjkraft · 2021-11-10T04:37:50Z

generate/convert.go

+	return options.Pointer == nil &&
+		isStructField &&
+		g.Config.WeakReferences &&
+		len(def.Fields) > 0


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).)

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

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).

benjaminjkraft · 2021-11-10T04:39:36Z

generate/convert.go

@@ -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)


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.

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.

The logic check was subtly flawed and would override any omitempty: false comments

nathanstitt · 2021-11-12T21:46:52Z

@benjaminjkraft ready for another look whenever you're able. Sorry for the delay, I reworked my project to use this to double check it's all ok. It worked great, I still had to add approx 10 "omitempty" statements but that's quite manageable.

arjunyel · 2021-11-13T00:59:00Z

@nathanstitt hey I just wanted to thank you so much for doing this PR, you're helping a lot of people :D

benjaminjkraft

Neat, this looks great! One question about whether we should do this for output types at all -- I'm ok either way as long as the name matches so as-is is fine unless you want to change it. (Long term maybe we'd do both as options if the need arises.) And it looks like a couple of lint errors crept in, but once you fix those it's good to go!

Ah and I almost forgot (our bot is down) -- could you sign our CLA at https://www.khanacademy.org/r/cla, if you haven't?

nathanstitt · 2021-11-13T03:48:15Z

Thanks @benjaminjkraft!

I think output types should be affected by the rule because:

It's a bit better (IMO) to check for missing association by testing for nil vs making sure the pkey field is included and then checking it for it's zero value. However, this is a bit unsafer and runs a risk of panics, so it really could go either way. Personally I'd rather the code blow up if there's a misconception about a missing value vs continuing to run with garbage data. I could probably be convinced either way though so 🤷
It's better to use a single rule everywhere if possible. That's both easier to document and also to keep straight mentally.

nathanstitt · 2021-11-13T03:50:31Z

Oh and I also signed CLA, so should be good to merge whenever you're good with it

benjaminjkraft · 2021-11-13T03:53:36Z

Thanks very much!

nathanstitt requested review from benjaminjkraft, csilvers, dnerdy and StevenACoffman as code owners November 8, 2021 16:02

benjaminjkraft requested changes Nov 10, 2021

View reviewed changes

rename config to "StructReferences" & rework check

77da957

The logic check was subtly flawed and would override any omitempty: false comments

nathanstitt force-pushed the weak-refs branch from f312d05 to 77da957 Compare November 11, 2021 01:33

nathanstitt changed the title ~~Add "weak_references" configuration~~ Add "struct_references" configuration Nov 11, 2021

Deardrops mentioned this pull request Nov 11, 2021

Add "weak_references" configuration iomesh/genqlient#1

Merged

nathanstitt requested a review from benjaminjkraft November 12, 2021 21:45

typo

bf8b563

benjaminjkraft approved these changes Nov 13, 2021

View reviewed changes

code cleanup to fix lint warnings

d0bb6ad

benjaminjkraft merged commit 2fdbb62 into Khan:main Nov 13, 2021

benjaminjkraft mentioned this pull request Nov 13, 2021

pointer & omitempty is cumbersome/impossible with a large schema #149

Closed

nathanstitt deleted the weak-refs branch November 13, 2021 03:56

benjaminjkraft mentioned this pull request Jan 21, 2022

release 0.4.0 #163

Closed

nedredmond mentioned this pull request Jan 26, 2022

Correctly handle self-recursive types automatically #152

Open

benjaminjkraft mentioned this pull request Jan 26, 2022

directive to apply omitempty on nested fields #165

Closed

benjaminjkraft mentioned this pull request Mar 23, 2022

Option to use pointers for optional fields #178

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "struct_references" configuration #155

Add "struct_references" configuration #155

nathanstitt commented Nov 8, 2021

nathanstitt commented Nov 8, 2021

benjaminjkraft left a comment

benjaminjkraft Nov 10, 2021

benjaminjkraft Nov 10, 2021

benjaminjkraft Nov 10, 2021

benjaminjkraft Nov 10, 2021

nathanstitt Nov 11, 2021

benjaminjkraft Nov 13, 2021

benjaminjkraft Nov 10, 2021

nathanstitt Nov 11, 2021

benjaminjkraft Nov 10, 2021

nathanstitt Nov 11, 2021

benjaminjkraft Nov 13, 2021

benjaminjkraft Nov 10, 2021

nathanstitt Nov 11, 2021

nathanstitt commented Nov 12, 2021

arjunyel commented Nov 13, 2021

benjaminjkraft left a comment •

edited

Loading

nathanstitt commented Nov 13, 2021

nathanstitt commented Nov 13, 2021

benjaminjkraft commented Nov 13, 2021

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

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

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

Add "struct_references" configuration #155

Add "struct_references" configuration #155

Conversation

nathanstitt commented Nov 8, 2021

nathanstitt commented Nov 8, 2021

benjaminjkraft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanstitt commented Nov 12, 2021

arjunyel commented Nov 13, 2021

benjaminjkraft left a comment • edited Loading

Choose a reason for hiding this comment

nathanstitt commented Nov 13, 2021

nathanstitt commented Nov 13, 2021

benjaminjkraft commented Nov 13, 2021

benjaminjkraft left a comment •

edited

Loading