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

Expose IDbConnection through DI #2761

Merged
merged 9 commits into from
Nov 30, 2018
Merged

Expose IDbConnection through DI #2761

merged 9 commits into from
Nov 30, 2018

Conversation

mdockal
Copy link
Contributor

@mdockal mdockal commented Nov 29, 2018

Fixes #2757, and fixes #2248. Ran into some issues with async /await inside lambda expression, had to hack it a little bit.

Copy link
Member

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

That will be an issue. You will need to create a wrapper around the connection (implementing) idbconnection that will be built using the session. This way we can return it without asynchronous calls. Also, register this instance with the interface in the AddScoped call so it's a lazy initialization instead.

@mdockal
Copy link
Contributor Author

mdockal commented Nov 29, 2018

@sebastienros I am not sure what you mean? Why are we using async anyway just for a connection object? I (speaking as the end - developer trying to extend the data model) don't want an open connection nor a transaction. My Micro ORM / ORM will handle that or I will handle it explicitly in the module. I don't think is a good idea to open a connection without explicitly trying to read or update the data.

So I looked at the DemandAsync() code and it turns out it is calling DBConnection.OpenAsync() that is the reason why it is async. However, I did find something in DemandAsync() that was exactly what I think I and other developers need.

_connection = _store.Configuration.ConnectionFactory.CreateConnection() as DbConnection;

So if I use the IStore instance to simply get the connection factory we get something like this:

 services.AddScoped(sp =>
                {
                    var store = sp.GetService<IStore>();

                    if (store == null)
                    {
                        return null;
                    }

                    var connection = store.Configuration.ConnectionFactory.CreateConnection();

                    return connection;
                });

I think it's all that really needed, and no async / await method Calls!

@sebastienros
Copy link
Member

You have a GetAwaiter().GetResult() line which will be called from an async path (a controller for instance).

@mdockal
Copy link
Contributor Author

mdockal commented Nov 29, 2018

You have a GetAwaiter().GetResult() line which will be called from an async path (a controller for instance).

Agreed, I had read that wasn't a really good way to do things.

If we simply use:
var connection = store.Configuration.ConnectionFactory.CreateConnection();

We don't need to do async because all we need is the IDbConnection instance.

@sebastienros
Copy link
Member

It's not the issue. The issue is that someone will do Resolve<IDbConnection>(), and the code it will run will contain a blocking call to DemandAsync().

It might be necessary to use a provider interface with a GetConnectionAsync().

.gitignore Outdated Show resolved Hide resolved
mdockal and others added 2 commits November 29, 2018 18:31
Didn't want to add this to pull request
@jtkech
Copy link
Member

jtkech commented Nov 30, 2018

@mdockal i re-added the .gitignore but without the added line directly in your repo, so that here we now have only one modified file.

@mdockal
Copy link
Contributor Author

mdockal commented Nov 30, 2018

@mdockal i re-added the .gitignore but without the added line directly in your repo, so that here we now have only one modified file.

Thanks @jtkech I am learning how to do git. I come from using Visual Source Safe and VSTFS background so git doesn't work like I expect it to.

Mark Dockal, Jr added 2 commits November 29, 2018 23:57
…entation. Registered IDbConnectionWrapper in OrchardCoreBuilderExtensions.AddDataAccess
@mdockal
Copy link
Contributor Author

mdockal commented Nov 30, 2018

I created an IDbConnectionAccessorr interface and DbConnectoinAccessor and registered it. The developer will call IDbConnectionAccessor.GetConnectionAsync() which calls Session.DemandAsync() and returns an open IDbConnection async for the developer.

public class SomeController : Controller
{
    private IDbConnectionAccessor _connectionAccessor;

    public SomeController(IDbConnectionAccessor connectionAccessor)
    {
          _connectionAccessor = connectionAccessor
    }
    
    public async Task<IActionResult> Index()
    {
        IDbConnection connection = await _connectionAccessor.GetConnectionAsyc();
    }
}

Changed ArgumentExeption to ArgumentNullException
Cleaned up usings.
Copy link
Member

@sebastienros sebastienros left a comment

Choose a reason for hiding this comment

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

Last details and it's all good.

…o OrchardCore.Data.Abstractions. Renamed DbConnectionWrapper to DbConnectionAccessor and added using for OrchardCore.Data.Abstractions
@sebastienros sebastienros merged commit eaff283 into OrchardCMS:dev Nov 30, 2018
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 IDbConnection through DI Documentation for CRUD in the Database
3 participants