Support for resolver ExecutionResult return type (aka extensions support)

[nikordaris]

The main intention of this PR is to expose extensions to middleware and resolvers. The execution is responsible for creating the ExecutionResult and does not currently provide a means to inject extensions data. This PR gives all resolvers the option to return the ExecutionResult instance instead of the raw data object. In order to prevent field level resolvers overriding the extensions data I added it to the ExecutionContext so it can be maintained and updated across all the resolvers.

Fixes #206

[wapiflapi]

I've been using this today to implement a middleware for apollo engine tracing data and it seems to be working great!

[wapiflapi]

@syrusakbary if there is anything I can do to help get this merged please let me know!

[nikordaris]

@wapiflapi do you plan on publishing your tracing middleware once you are finished? I would be interested in it once this gets merged

[wapiflapi]

@nikordaris definitely will. Just trying to figure out what repo it would live in, but probably graphql-core I imagine. I'm waiting for this PR before submitting mine so that the diff isn't too big. Thanks for your code! Works great.

[nikordaris]

One thing this PR could have done differently is merge resolver extensions in the core. At the time I didn't want to make that decision on how to merge multiple extension definitions and so I passed the current extensions data in the info object. This forces the middleware to explicitly grab the existing extensions data and merge theirs into it. The more I think about it the more I don't think I like that clunky and error prone API. I'm still on the fence about it so if you guys have a stronger opinion on how that should be done I'm open to changing it.

[wapiflapi]

I have a middleware updating the same 'tracing' extension each time it is run and in that regard it is handy to have access to what is already in there.

Merging automatically seems complicated and prevents middleware from updating previously written values. Having the middleware explicitly merge it's data to what is already recorded doesn't seem bad in itself. But your idea of this being error prone because it's easy to write middleware that overwrite other middleware's data worries me too.

[nikordaris]

So I think we should do both. We provide extensions in info so they can merge it manually if they want. We then merge that extensions data in with the one already on the ExecutionContext and set an override strategy favoring the incoming extensions. That way we don't have badly written middleware stomping on other middleware extensions definitions even though the namespace keys don't conflict.

Update: forgot I was already doing a shallow merge but I think there is a bug in how I was updating it. Started a review of how I think the shallow merge should be fixed. I'm not against doing a deep merge but I think there could be unintended consequences if we do that. Giving the resolver access to the extensions and doing a shallow merge should be sufficient.

[nikordaris]

Ok, I fixed the shallow merge for extensions. I think this PR is good to go now once we get the upvotes on it. A middleware dev doesn't have to merge extensions with other middleware unless they want to share extensions key namespaces. The shallow merge overrides the top level key of the extensions dict with the last middleware executed. Middleware that don't share key namespaces only need to set their extensions dict in the ExecutionResult object they return and it will get shallow merged with the other middleware extensions.

[wapiflapi]

I tested the latest version of your code @nikordaris it still works fine for me! Here is the tracing middleware I've been using this for: https://gist.github.com/wapiflapi/6131ed5e8e5dfd5aa0b0a638f86edf16 I'll submit a pull request to add it to the repo so it's available for all since it seems useful in a lot of usecases. I'm sharing it as a gist now so that it can be used for testing and showing interest in this PR.

[nikordaris]

So i'm starting to think that this might cause too many issues with other middleware attempting to manipulate or inspect the result data. I'm wondering if putting an update_extensions function on the info object is the better solution. The function would then update the ExecutionContext's extensions object. @wapiflapi

[wapiflapi]

We might be over-reacting on this. What problems are we worried about exactly?

I understood the fear about middleware unintentional overwriting other middleware's data, which could go undetected for a while until "incompatible" middleware is installed. The merge at the extensions level should take care about this problem. If we still think two middleware might update say tracing at the same time we could issue a warning about that at run time but that's just as likely to not get noticed?

Regarding middleware intentionally manipulating or inspecting the result data, I don't think there is anything we can do to prevent this. There will always be a way to inspect and modify if they really want it. I don't think we should prevent this. I don't think it's that big of a problem, and it could be useful in some situations where a middleware could augment data from another middleware (and don't do anything if the information it needs is not present). I'm not sure that would be an anti-pattern.

Do you have any examples of where this would cause unforeseen problems for the users in practice?

[wapiflapi]

@nikordaris Do you know who can comment on this to get it merged eventually?

[nikordaris]

@syrusakbary and @jkimbo have merged some of my PRs in the past but I know they are really busy with graphql-python so hopefully they'll get some time to merge it soon. I'd definitely like their input on whether this should be a helper utility off of the info or a returned value.

[nikordaris]

@syrusakbary have you gotten a chance to think about the best way to support extensions? The more I think about it I can see 3 options.

1. Support ExecutionResult return types (this PR)
2. Added a utility to info to inject extension data
3. Add a separate extensions middleware that would return extensions data instead of the resolved data.

I'm kind of leaning towards option 3 now because it reduces complexity of the resolvers and won't break existing middleware.