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

Better async support #1

Closed
matt-hensley opened this issue Dec 14, 2017 · 10 comments
Closed

Better async support #1

matt-hensley opened this issue Dec 14, 2017 · 10 comments

Comments

@matt-hensley
Copy link

Setup a quick test project - this library works very well and is a real layer of sanity on top of graphql-dotnet. I noticed the querying is all sync, and presumably async would be better?

Looks like graphql-dotnet supports awaiting resolvers:
graphql-dotnet/graphql-dotnet#205 (comment)

At first glance, it looks like just SqlQueryContext needs an ExecuteAsync companion method.

Would this make sense/be within the goals of your library?

@dougrday
Copy link
Member

I see no issues with adding support for this, async support would be beneficial.

@dougrday dougrday added this to the 0.2.1 milestone Mar 16, 2018
@benmccallum
Copy link
Contributor

I might be able to submit a PR for this with some pointing in the right direction. Let me know if you need help :)

@dougrday dougrday modified the milestones: 0.2.1, 0.2.1-beta Mar 28, 2018
dougrday added a commit that referenced this issue Mar 28, 2018
@dougrday
Copy link
Member

Thanks for the contribution, it needed a slight adjustment to the unit test to pass. Looks good!

@dougrday
Copy link
Member

This was released to NuGet in version 0.2.1-beta: https://www.nuget.org/packages/Dapper.GraphQL/0.2.1-beta

@benmccallum
Copy link
Contributor

Interesting, it passed for me when I ran it alone. But I did get one failing when I "ran all" the existing tests together before adding the new test.

Is this because this IDbConnection is being shared once per test run and a previous execution will close the connection so it needs to be re-opened? Otherwise I'd expect to see connection.Open() in all the resolvers before .Execute() or .ExecuteAsync()

If so, should the lifetime of the IDbConnection in DI be changed?

@dougrday
Copy link
Member

It's because the DbConnection that is used currently isn't thread-safe, so it's getting mangled by all the tests.

Plus, the unit tests are terribly slow because of the LocalDB design, I'm going to try some options to speed it up and avoid these issues altogether.

@benmccallum
Copy link
Contributor

I see, that makes sense, thanks.

I've been wondering myself about how best to manage the IDbConnection in my own asp.net core app. I see in the resolvers you're using the serviceProvider injected in the Query object, and so I should probably just configure a transient IDbConnection in my Startup like so:

services.AddTransient<IDbConnection>(
    x => new SqlConnection(Config.GetConnectionString("SqlDb"))
);

@benmccallum
Copy link
Contributor

Hey @dougrday, Do you think you could take a sec to release a new version with the rest of the async methods I added in there? Looking at using them now. Thanks mate! You can prob close this out then too.

dougrday added a commit that referenced this issue Apr 17, 2018
- Cleanup unit tests to succeed
dougrday added a commit that referenced this issue Apr 17, 2018
@dougrday dougrday modified the milestones: 0.2.1-beta, 0.2.2-beta Apr 17, 2018
@dougrday
Copy link
Member

Remaining features were released to NuGet in version 0.2.2-beta: https://www.nuget.org/packages/Dapper.GraphQL/0.2.2-beta

@benmccallum
Copy link
Contributor

Thanks Doug!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants