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

Added an option for a custom ProfiledDbDataReader implementation #325

Merged
merged 3 commits into from
Sep 16, 2018

Conversation

kekekeks
Copy link
Contributor

@kekekeks kekekeks commented Sep 13, 2018

Some ADO providers have extended API for database-specific functions, a notable example is Npgsql.

Some ORMs (e. g. linq2db) are aware of said functions and try to use them. Fortunately linq2db doesn't hardcode any type references and discovers needed APIs via reflection, it even allows to configure the expected reader type when needed.

This PR adds an option to substitute the standard ProfiledDbDataReader with a derived type that can reimplement said database-specific API.

New constructors are added to keep binary compatibility with existing code.

@NickCraver
Copy link
Member

I see the intent here, but I think we can improve the approach. The overall issue I see is this creates a lot of complicating to overload just one of the types involved (Connection, Command, Reader, Transaction, Adapter, ProviderFactory). To have this expand to be extensible for everything would be quite a lot of code (and more than a little messy honestly).

The intent of MiniProfiler is to allow extensibility here, but the approach intent is inheritance (as MiniProfiler itself does). For example in this case could be accomplished like this:

public class MyProfiledConnection : ProfiledDbConnection
{
    public MyProfiledConnection(DbConnection connection, IDbProfiler profiler) : base(connection, profiler) { }
    protected override DbCommand CreateDbCommand() => new MyProfiledCommand(WrappedConnection.CreateCommand(), this, Profiler);
}

public class MyProfiledCommand : ProfiledDbCommand
{
    public MyProfiledCommand(DbCommand command, DbConnection connection, IDbProfiler profiler) : base(command, connection, profiler) { }
    protected override DbDataReader ExecuteDbDataReader(CommandBehavior behavior) => ...
}
// and so on...

It may be more code for a single user to have, but it's the only realistic way to override the whole tree and have something maintainable for all. Thoughts?

@kekekeks
Copy link
Contributor Author

Switched to protected virtual extension points

Copy link
Member

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

The convenient method to override is 👍, let's just get the naming consistent :)

/// <summary>
/// Creates a wrapper data reader for <see cref="ExecuteDbDataReader"/> and <see cref="ExecuteDbDataReaderAsync"/> />
/// </summary>
protected virtual DbDataReader CreateDataReader(DbDataReader original, IDbProfiler profiler)
Copy link
Member

Choose a reason for hiding this comment

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

Let's align the naming here - we have CreateDbCommand with Db elsewhere and CreateDataReader here - can you please change this to CreateDbDataReader (more correct given the return type).

Copy link
Member

@NickCraver NickCraver left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

@NickCraver NickCraver merged commit 022b6bd into MiniProfiler:master Sep 16, 2018
@NickCraver
Copy link
Member

I’m away this week, but this build should be up on the MyGet feed in a few minutes for release, thanks again for finding a good solution here with me :)

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.

2 participants