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

Support for resolver ExecutionResult return type (aka extensions support) #205

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

nikordaris
Copy link

@nikordaris nikordaris commented Aug 27, 2018

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

@nikordaris nikordaris changed the title Support for resolver ExecutionResult return type Support for resolver ExecutionResult return type (aka extensions support) Aug 28, 2018
@wapiflapi
Copy link

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

@wapiflapi
Copy link

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

@nikordaris
Copy link
Author

nikordaris commented Sep 10, 2018

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

@wapiflapi
Copy link

wapiflapi commented Sep 11, 2018

@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
Copy link
Author

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
Copy link

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
Copy link
Author

nikordaris commented Sep 11, 2018

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
Copy link
Author

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
Copy link

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
Copy link
Author

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
Copy link

wapiflapi commented Sep 13, 2018

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
Copy link

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

@nikordaris
Copy link
Author

@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
Copy link
Author

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

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.

Expose ExecutionResult extensions
2 participants