-
-
Notifications
You must be signed in to change notification settings - Fork 24
Use the IDb interfaces instead of concrete implementations. #37
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
base: develop
Are you sure you want to change the base?
Conversation
See #12. This always breaks backwards compatibility in quite a big way I'm afraid, so it's a no-go. What exactly is your problem? Is ServiceStack.OrmLite giving you an IDbConnection, which you want to use with Simple.Migrations? If so, you can probably just cast it to a DbConnection (which is what it should have been all along). |
I'm aware there was some rumblings about removing the interfaces, but they didn't. They are a part of the .NET Standard. https://docs.microsoft.com/en-us/dotnet/api/system.data.idbconnection?view=netcore-2.0 How do you suggest I use ServiceStack.OrmLite which creates it's own (see here)? |
I'm having trouble seeing your concern. |
Why do you say this? |
Considering using |
Since I give the user a reference to the IDbConnection in the migrations, I give them the ability to use the new methods which have been added since the concrete classes replaced the interfaces. All of this is moot though, as I'm not going to break backwards compatibility over this. Your best bet is probably to subclass DbConnection and proxy method calls to IDbConnection. I would accept a PR which adds a class which does this. Also as I noted in the other issue, Dapper does require a DbConnection, not an IDbConnection. It will cast the IDbConnection to a DbConnection. It seems they made the mistake of relying on IDbConnection to start with, and couldn't change their minds when it became apparent they needed stuff that DbConnection introduced (async, particularly). Also, there's the potential of adding async support in the future. IDbConnection would rule this out. This was also discussed in the linked issue. |
Fair enough.
Can you expand on this? |
I would still like to understand your use-case though: is the problem that ServiceStack is creating your DbConnection for you, and you want to preserve that? Also, it looks like OrmLiteConnection is just a wrapper around an inner IDbConnection. You can probably just grab this property and cast it to DbConnection. I don't know of any actual database providers (not wrappers around then, like OrmLiteConnection) which implement IDbConnection but not DbConnection. |
Exactly. I see now that I can cast a few times to get the inner But I still fail to see the benefit of using the concrete classes (apart from backwards compat with older Simple.Migrations). The interfaces are a part of .NET Standard, and since the standard is cumulative, it will be here forever. |
My understanding is that the interfaces (IDbConnection, etc) were introduced first, then Microsoft hit the versioning problem. They couldn't expand those interfaces to cover things like a Close() method, or async support, since changing the interfaces would break all of the classes which implemented them. So they moved to use abstract classes (DbConnection, etc): those could have new methods added to them in a backwards-compatible way, so long as the new methods were non-abstract (you'll see that the async methods they added have default implementations, which just call their non-async equivalents on a background thread, for example). In order to maintain backwards compatibility, the abstract classes implement the interfaces. So the interfaces were a mistake, and the abstract classes replaced them, at the very first point where Microsoft needed to add things. However, there were still a number of people implementing the interfaces before the abstract base classes were introduced. Even after the abstract classes were introduced, people still thought they should be implementing the interface and not the abstract base class, not understanding that the latter replaced the former. Backwards compatibility being what it is, the interfaces remained, and more people kept implementing them (looking at you, ServiceStack.OrmLite). |
I mentioned these earlier, but I'll spell them out again:
|
Your final option of course is something like this: public class ProxyDbConnection : DbConnection
{
private readonly IDbConnection dbConnection;
public ProxyDbConnection(IDbConnection dbConnection)
{
this.dbConnection = dbConnection ?? throw new ArgumentNullException(nameof(dbConnection));
}
public override string DataSource => throw new NotSupportedException();
public override string ServerVersion => throw new NotSupportedException();
public override void Close() => throw new NotSupportedException();
public override string ConnectionString
{
get => this.dbConnection.ConnectionString;
set => this.dbConnection.ConnectionString = value;
}
public override string Database => this.dbConnection.Database;
public override ConnectionState State => this.dbConnection.State;
public override void ChangeDatabase(string databaseName) => this.dbConnection.ChangeDatabase(databaseName);
public override void Open() => this.dbConnection.Open();
protected override DbTransaction BeginDbTransaction(IsolationLevel isolationLevel) => (DbTransaction)this.dbConnection.BeginTransaction(isolationLevel);
protected override DbCommand CreateDbCommand() => (DbCommand)this.dbConnection.CreateCommand();
protected override void Dispose(bool disposing)
{
if (disposing)
{
this.dbConnection.Dispose();
}
}
} |
Yeah, I like that approach better. Any time I cast interfaces to implementations, I get nervous. The API I'm using (ServiceStack.OrmLite) may choose to internal the implementation, or change it, and since I am not using the API as-is, it may "just break". That also brings up another point that I think ServiceStack.OrmLite and Dapper got it right. Use IDbConnection, and cast to DbConnection if you need a newer method. Any ways, let's agree to disagree. |
But that just moves the problem to runtime! If you try and use your OrmLite connection with Dapper, it will mysteriously fail with InvalidCastException in some specific cases. Surely that's worse. My other two points are still valid. Even if we disagree on the third, I am not breaking binary compatibility for every single user. Doing so would be incredibly irresponsible. That is something I'm not willing to change my stance on, I'm afraid.
If they have even the slightest appreciation of backwards compatibility, they won't do this. Most people with public projects do care about not breaking users (that's what got us into the IDbConnection/DbConnection mess in the first place). Alternatively, if you're willing to break backwards compatibility for all of my users, surely you won't mind if OrmLite occasionally breaks things which you have to fix 🙂 |
Also, why not submit a PR to OrmLite changing their IDConnection to a DbConnection? That should actually be backwards compatible... |
I'm using ormlite with simple migrator.
|
This would allow me to use ServiceStack.OrmLite's connections.