Add support for extensions

[janboll]

Is your feature request related to a problem? Please describe.
I want to access the data returned in the Extensions field, as defined in the response format: https://spec.graphql.org/October2021/#sec-Response-Format
A challenge is, that this data is untyped and not defined in the schema itself.

Describe the solution you'd like
Add a configuration flag to genqlient.yaml, like use_extensions, to toggle generating Extensions fields in resulting objects. Example for the resulting, generated code:

type GithubOrgsResponse struct {
	Githuborg_v1 []GithubOrgsGithuborg_v1GithubOrg_v1 `json:"githuborg_v1"`
	Extensions   map[string]interface{}
}

Extensions would be added after unmarshaling the data by the graphlql client.

Describe alternatives you've considered
Enabling it on a per-query basis via comment

Additional context
I am willing to provide a PR once the design is settled.

[benjaminjkraft]

Thanks for raising this! I think adding an option makes a lot of sense. The only thing I want to discuss a little is where and how to represent it.

For where, in GraphQL generally the extensions should be at the same level as the data and errors. So that would suggest to make it a separate result parameter parameter, i.e. we'd generate GithubOrgsResponse as-is but return (resp GithubOrgsResponse, ext map[string]interface{}, err error) or similar. It's a bit unwieldy to have 3 results, but it also means we can avoid worrying about confusion (or even name-collision) between whether Extensions is a query field with that name, or actually extensions. Does that seem sensible to you? We could also add another layer of struct, but that seems awkward in its own way.

(One slight awkwardness there is that we may need to change the Client interface, again. In principle shoving Extensions into the generated MyQueryResponse avoids that, but I think in practice it will make the life of a Client harder, so a simple breaking change is probably actually less bad. Or we could have two options, Client and ClientWithExtensions? Possibly the best thing is actually to make payload and response public and then do MakeRequest(ctx context.Context, req *payload, resp *response) as a more extensible interface. cc @csilvers or @dnerdy can you take a quick glance at the Khan client to see if that would in fact work and be reasonably extensible? Anyway, I'm not too worried about this, I think we can work something out when we get there, possibly in a separate PR in the same release. )

Then the other question is what type it should be. Of course map[string]interface{} works, but I wonder if we should let users specify the type. I suspect it's not much harder to implement and gives people more flexibility if they know what keys their extension will have (I assume that's the common case? Would it make sense for you?). So the option could be extensions: <type>. I think we have enough of the machinery in place (for bind etc.) that that should be fairly easy to implement, and of course we can revisit if not.

[janboll]

Thank you for that rapid response!
I really like the idea of having the extension an extra return parameter of the generated function. To be clear, if extensions are enabled the function would have the following signature,
(resp GithubOrgsResponse, ext map[string]interface{}, err error)
whereas with disabled extensions, it would look like that,
(resp GithubOrgsResponse, err error)
correct? But as I actually look at it, the whole purpose for putting it behind a toggle is to have backward compatibility. For me as a new user of this library having the signature containing the extension would suffice ;)

The signature of MakeRequest(ctx context.Context, req *payload, resp *response) looks a lot more concise. I do not fully understand the idea of having a ClientWithExtensions, since this method is hidden behind the generated method. A user would rely on that, and only notice change we alter the signature of the generated methods. But I guess I miss something here...

Regarding typing, at least speaking for me and my use case, I don't see the added benefit of having that in the config file. The reason being is, that validation at code generation is not possible, so type failures at runtime could happen. In that case, I would rather consume it as is (map[string]interface{}) in my extension code. But again, I might be way off here...

[benjaminjkraft]

To be clear, if extensions are enabled the function would have the following signature, (resp GithubOrgsResponse, ext map[string]interface{}, err error) whereas with disabled extensions, it would look like that, (resp GithubOrgsResponse, err error) correct? But as I actually look at it, the whole purpose for putting it behind a toggle is to have backward compatibility. For me as a new user of this library having the signature containing the extension would suffice ;)

Yeah, that's right. Having it behind an option is both for backwards compatibility, and to avoid having to always do resp, _, err := ... for people who never use extensions (which my guess is probably at least the majority and possibly almost all of users). Since it's a single global option, it hopefully shouldn't cause too much trouble for people who do want the extensions.

The signature of MakeRequest(ctx context.Context, req *payload, resp *response) looks a lot more concise. I do not fully understand the idea of having a ClientWithExtensions, since this method is hidden behind the generated method. A user would rely on that, and only notice change we alter the signature of the generated methods. But I guess I miss something here...

Heh, I hadn't fully thought it through, but I think the idea was the generated code would do a type-assertion to see if your Client also implements ClientWithExtensions, and if so use the latter (if the option is enabled)? And you'd presumably implement MakeRequest as a call to MakeRequestWithExtensions with no extensions. I think MakeRequest(ctx context.Context, req *payload, resp *response) is probably better; if none of the folks from @Khan have an opinion here I'm happy to go with it as my guess is not many others are using the full Client interface. If you are excited to, you can go ahead and implement that, or you can just shove extensions in as an extra parameter for now and I'll make the further changes in a separate PR once we're sure what they should be.

Regarding typing, at least speaking for me and my use case, I don't see the added benefit of having that in the config file. The reason being is, that validation at code generation is not possible, so type failures at runtime could happen. In that case, I would rather consume it as is (map[string]interface{}) in my extension code. But again, I might be way off here...

Sure, makes sense! We can just start with map[string]interface{} and extend it later if needed.

[csilvers]

The only use of MakeRequest that I see is in our adapter code, which just delegates to upstream after doing some configuration, and this code:

	// TODO(benkraft): It's a bit of a hack to use genqlient's MakeRequest
	// here; it's really intended only for genqlient's own use.  But it's more
	// convenient than the shurcooL client, for us.
	err := adapter.MakeRequest(ctx, operationName, operation, &result, variables)
	if err != nil {
		return errors.Wrap(err)
	}
	enc := json.NewEncoder(os.Stdout)
	enc.SetIndent("", "  ")
	return errors.Wrap(enc.Encode(result))

So I think it should be straightforward to change the code if the MakeRequest API changes.

[janboll]

That all sounds very reasonable to me! I'll try to come up with a PR in the next few days/weeks.