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

Add MS-specific dependency injection extensions #1571

Closed
wants to merge 25 commits into from
Closed

Add MS-specific dependency injection extensions #1571

wants to merge 25 commits into from

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Feb 24, 2020

This is a follow-up to the discussion in #1345

This PR adds Services back to ISchema and adds RequestServices to IResolveFieldContext and ExecutionOptions. This allows implementers to access the currently executing request's scoped service provider. No additional dependencies or functionality is added to the base GraphQL project.

The naming of the IResolveFieldContext.RequestServices property matches Asp.Net Core, where their HttpContext has a RequestServices property to access the service provider of the executing request.

I also added the recently-removed Services property back into the ISchema interface, so code can access the service provider used to create the graph type and other schema-specific classes if desired.

By adding RequestServices into the GraphQL pipeline, resolvers that are based on scoped services are free from framework-specific dependencies, such as IHttpContextAccessor.HttpContext.RequestServices, or needing to pass the service provider or services through the IResolveFieldContext.UserContext property. It also alleviates the need to create a custom service accessor singleton.

Anyone that wishes to keep their dependent services listed in the constructor can simply register the schema as a scoped service, as they have always done. For those of us that want to keep the performance of the schema as a singleton,

I've also added a GraphQL.Extensions.DI.Microsoft project, which provides an easy way to have parallel graphql resolvers execute scoped services not designed for multithreaded operation (as most scoped services are not, such as Entity Framework). Simply call Field().ResolveScoped or Field().ResolveAsyncScoped with the same field resolver, and the extension will create a DI scope during the execution of the resolver, disposing of it appropriately when complete. This project works with any DI container compatible with the necessary interface - notably, autofac is also compatible. I feel that these additional extension methods would be convenient, but not necessary as they can always use the SerialExecutionStrategy and ignore this issue. I would include this functionality as notes in the documentation, but it seems like a bit too much code for that. Perhaps it can be simply a link in the documentation to a project with these extra classes, or perhaps it should be released as another nuget package.

I had one additional idea, not in this PR, to reduce the calls to RequestServices: We could add additional Resolve overloads for resolvers that need services. For example, one could write:

Field<ListGraphType<UserType>, IEnumerable<User>>("Users")
    .ResolveAsync<MyDbContext>((context, dbContext) => dbContext.Users.ToListAsync());

The resolver would simply pull the requested services from the context's RequestServices property prior to calling the lambda function.

Unfortunately it cannot be implemented as an extension method very well, as the call to ResolveAsync would need to include the TSourceType and TReturnType types also. So it doesn't lend itself well to adding these methods to a third party library. If we add this feature, it would have to be directly on the FieldBuilder<TSourceType, TReturnType> class.

@sungam3r sungam3r added discussion The issue is under discussion and a common opinion has not yet been formed enhancement New feature or request needs review labels Feb 24, 2020
@Shane32
Copy link
Member Author

Shane32 commented Aug 14, 2020

@sungam3r We've already added the RequestServices property to IResolveFieldContext. The rest of this PR contains some proposed extensions for adding scoped field resolvers. Are you interested in:

  1. Keeping this PR open for now, just for reference
  2. Replace with a new PR containing the suggestions that are not yet merged
  3. Close for now

I'm okay either way. Now that we have the RequestServices property, I can throw these extensions directly into my projects, so it's no big deal. I might even consider posting a separate nuget package with some of these extensions if you don't want them as part of the base graphql project. Just lmk.

@sungam3r
Copy link
Member

I'm absolutely sure that after #1730, we have to go ahead and include these changes, so option 2 or resolve conflicts/rebase here.

@Shane32
Copy link
Member Author

Shane32 commented Aug 14, 2020

I merged changes here, and removed Services from ISchema.

@sungam3r
Copy link
Member

Only additions now, no deletions. Good. Will review soon.

@sungam3r
Copy link
Member

@Shane32

C:\Program Files\dotnet\sdk\3.1.302\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(187,6): warning : This project cannot be packaged because packaging has been disabled. Add <IsPackable>true</IsPackable> to the project file to enable producing a package from this project. [C:\projects\graphql-dotnet\src\GraphQL.Harness\GraphQL.Harness.csproj]
C:\Program Files\dotnet\sdk\3.1.302\Sdks\NuGet.Build.Tasks.Pack\build\NuGet.Build.Tasks.Pack.targets(198,5): error NU5046: The icon file 'logo.64x64.png' does not exist in the package. [C:\projects\graphql-dotnet\src\GraphQL.Extensions.DI.Microsoft\GraphQL.Extensions.DI.Microsoft.csproj]

@Shane32
Copy link
Member Author

Shane32 commented Aug 16, 2020

I've added a rough draft of a 'resolver injection' technique to this PR. It requires review. Since it's impossible with extension methods, I added builder classes to facilitate this. Perhaps you have a better idea @sungam3r but I'm not sure how else to do it. Anyway, with this sample, you could write code like this:

Field<StringGraphType, string>("test")
    .Argument(...)
    .Resolve()
        .WithScope()
        .WithService<MyService>()
        .WithService<MyOtherService>()
        .ResolveAsync(async (context, service, service2) => await service.GetAnswer(context.Source))
    .Description("hello");

Of course, the WithScope would only be needed if you specifically wanted to create a scope for this field resolver. Probably when using the parallel execution method with EF or similar. Whereas with the serial execution method, you would not need WithScope and it would pull the services directly from the service provider.

This resolve builder is within the GraphQL.Extensions.DI.Microsoft project. However, the builder is not really specific to MS at all - only creating the scope relies on Microsoft DI's CreateScope extension method, which simply relies on an implementation of IServiceScopeFactory to be registered with the base service provider, and then retrieves the IServiceProvider from the provided IServiceScope interface returned from the factory.

Personally I think we should combine the two projects together. Someone can always register their own implementation of an IServiceScopeFactory if they want to create scopes with an alternate service provider. We could of course create our own interface for creating service scopes - but there's no reason to rewrite good code that already exists, especially as an abstraction. It's certainly an option, however, if we want to publish this project with no dependencies whatsoever.

Finally, I would not let this hold up 3.0. Nothing here is going to be a breaking change, so there's no reason it can't make it into 3.1 if we have not the time to review it fully. I would like to keep this on the top of the pile, however, and if we can get this done quickly, that's great!

@Shane32
Copy link
Member Author

Shane32 commented Aug 16, 2020

@sungam3r If you like the idea, let me know, and I can add tests and so on. I just threw this together real quick.

@Shane32 Shane32 added this to the 3.1 milestone Aug 21, 2020
Co-authored-by: Ivan Maximov <sungam3r@yandex.ru>
@Shane32 Shane32 changed the base branch from master to develop January 19, 2021 17:31
@Shane32
Copy link
Member Author

Shane32 commented Jan 19, 2021

@sungam3r We had planned to release this for 3.1, but never did. Can we release this with 4.0? We need to review:

  • Name of project/nuget package
  • Resolver builders -- probably at the same time as we review the argument builders

I should add some tests and a few xml comments.

@sungam3r
Copy link
Member

Give me 1-2 days. And side note - dotnet/runtime#47113. I found this by accident. Perhaps this is the reason for the falling test.

src/GraphQL.sln Outdated Show resolved Hide resolved

namespace GraphQL.Extensions.DI.Microsoft
{
public class ScopedFieldResolver<TReturnType> : FuncFieldResolver<TReturnType>
Copy link
Member

Choose a reason for hiding this comment

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

Is ScopedFieldResolver<TReturnType> class used anywhere (ScopedAsyncFieldResolver<TReturnType> as well) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - ScopedFieldBuilderExtensions.cs

Copy link
Member

Choose a reason for hiding this comment

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

I talked about ScopedFieldResolver<TReturnType> not ScopedFieldResolver<TSourceType, TReturnType>

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. They might not be.

@Shane32
Copy link
Member Author

Shane32 commented Jan 23, 2021

Good comments. I'll work on it.

@Shane32 Shane32 mentioned this pull request Jan 27, 2021
@sungam3r
Copy link
Member

I probably won't be working on other tasks until v4. Let's wrap up this one.

@Shane32
Copy link
Member Author

Shane32 commented Feb 7, 2021

Replaced with #2261

@Shane32 Shane32 closed this Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion The issue is under discussion and a common opinion has not yet been formed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants