dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

Make interfaces as the official ADO.NET Provider API instead of classes #15288

Closed nvivo closed 4 years ago

nvivo commented 9 years ago

From what I can see currently on the corefx-progress page for System.Data.Common, the interfaces (IDbCommand, IDbConnection, etc) were removed in favor of the usage of abstract classes.

But in the new API, most of the main methods are not virtual or abstract. On DbCommand alone we can see this:

public DbConnection Connection { get; set; }
public DbParameterCollection Parameters { get; }
public DbTransaction Transaction { get; set; }
public DbParameter CreateParameter();
public Task<int> ExecuteNonQueryAsync();
public DbDataReader ExecuteReader();
public DbDataReader ExecuteReader(CommandBehavior behavior);
public Task<DbDataReader> ExecuteReaderAsync();
public Task<DbDataReader> ExecuteReaderAsync(CommandBehavior behavior);
public Task<DbDataReader> ExecuteReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken);
public Task<DbDataReader> ExecuteReaderAsync(CancellationToken cancellationToken);
public Task<object> ExecuteScalarAsync();

While these methods can certainly be made virtual or abstract, it would be much more useful to have the real interfaces back, and make any public API depend on these interfaces instead of the abstract classes.

This is mostly useful when developing libraries. Today it's very hard to mock a datareader to make it return a specific value for testing purposes. The same for ensuring that ExecuteReaderAsync was called, not ExecuteReader, etc.

I propose the provider factory instead should be made as an interface:

public interface IDbProviderFactory {
    IDbCommand CreateCommand();
    IDbConnection CreateConnection();
    IDbConnectionStringBuilder CreateConnectionStringBuilder();
    IDbParameter CreateParameter();
}

And then follow from there to the rest of the provider to things like IDbDataReader, IDbTransaction, etc.

We know the interfaces became out of sync for some reason in the past and the abstract classes were made the official API, but this doesn't need to be the case anymore in corefx.

Note that this doesn't mean removing the System.Data.Common in any way, but instead make the Common classes implement these interfaces, and you wouldn't use System.Data.Common unless you're implementing the provider. Applications would depend only on the interfaces instead.

Please consider this to make the API more testable on corefx 1.0.

Related to discussions on dotnet/runtime#14302 and dotnet/runtime#15269.

mythz commented 8 years ago

You would never get consensus for every ORM or ADO .NET provider maintainer to take on an external 3rd party dependency (pretty much guarantee SQL Server wont) and 2 libraries redefining a core BCL interface can't be used together. Even worse for high-level frameworks (like ServiceStack) that reference System.Data interfaces in core libs (that it'd now need to define) will no longer be able to use any ORM that didn't reference the same interfaces - which no-one would or should.

The only way you can ensure every library references the same System.Data interfaces is if they were restored with the base classes implementing them - which I'm still not clear on what harm this has.

thefringeninja commented 8 years ago

@mgravell ah transitive dependency, hadn't considered that. :+1:

jnewman30 commented 8 years ago

You know I don't see why this is an issue unless you are tightly coupling your code to code you don't own. Protect your code from it's dependencies! Wrap it up and abstract it away. There are many ways to do this and make your code testable. Many are mentioned above. You integration test the bits you don't own. That is the way it works. You should not be mocking BCL objects! If you are then your design is not good.

davkean commented 8 years ago

@nvivo I get that this is your original issue, but the direction of it has now turned into a thread about bringing back the v1 era interfaces for compat reasons. Let's keep it focused on that - if you would like to discuss making changes to the current surface area, please file new issues.

@mythz There's two issues that we had with the interfaces; 1) they brought in a (heavy) dependency on DataSet, which does not belong with policy-free abstractions and 2) they bring in a parallel set of abstractions to the base classes but are locked with the v1 surface area. We wanted to avoid that confusion.

I agree it is not viable for a 3rd party to provide these interfaces - they need to be implemented on the core abstractions to be useful.

nvivo commented 8 years ago

I get that this is your original issue, but the direction of it has now turned into a thread about bringing back the v1 era interfaces for compat reasons. Let's keep it focused on that - if you would like to discuss making changes to the current surface area, please file new issues.

This makes absolutely no sense.

mythz commented 8 years ago

@nvivo it means, you're not the one with the issue despite having filed it - evident by having closed it. The issue is about restoring the System.Data interfaces to ease the burden of supporting the entire ecosystem that's reliant on them. You seem to be ok with:

It's 2015, everything breaks all the time and we're used to it.

But this is not a satisfactory strategy for those of us having to support existing code bases and our Customer's code-bases, and should definitely not be the default strategy for custodians of BCL libraries affecting all of .NET.

nvivo commented 8 years ago

But this is not a satisfactory strategy for those of us having to support existing code bases

@mythz This is out of context. That's not what I meant. Evertybody here have to support existing code bases, i doubt there is any newcomer to .NET in the discussion.

The issue with what this conversation turned into is that it doesn't make much sense. .NET Core is a new framework, not an upgrade. A lot of the existing full .NET API is not there and won't be. Backward compatibility won't work like that anyways.

mythz commented 8 years ago

@nvivo This exact sentiment is why this issue doesn't apply to you. If you think backwards compatibility is not important, you've never tried supporting a meaningful code-base targeting multiple platforms - you're also not speaking on behalf of the CoreCLR team. These libraries were not developed from scratch, if you read above you'll find that it's a primary goal for CoreCLR libraries to run on the full .NET Framework. CoreCLR is another platform target of which porting existing libraries is vital to its success, something the .NET team is actively encouraging and which these missing interfaces are currently hindering.

ghost commented 8 years ago

With all this talk about interfaces not being version friendly it makes me think about how the Go programming language sidesteps this problem with implicit interfaces.

davkean commented 8 years ago

I was asked to expand on what I meant by policy-free abstractions.

Basically, by policy-free I mean that the System.Data.Common abstractions contain almost zero business rules - all they do is provide a shape of APIs that a given provider must implement. This is in contrast to DataSet, which has lots of business rules and code. Policy-free types, due to their very construction, tend to version less frequently than types that contain policy, as there's less code, hence less bugs and design changes. We want abstractions and types that are exchanged[1] between 3rd party libraries to version infrequently[2] to reduce the number of issues you encounter when coordinating dependencies across a large package graph. You cannot embed or ilmerge exchange types as part your application, whereas, non-exchange types you can. Hence, why we want them split.

[1] By exchanged, I mean types that tend to appear on public APIs. For example, we consider Collection<T> an exchange type, but not List<T>. [2] Using semantic versioning for these would be cause breaks almost identical to above with the removed interfaces, because you "fork" the ecosystem; libraries need to make a decision whether to target the library before or after the break, or fork themselves to handle the split.

niemyjski commented 8 years ago

@mythz I have to support orms/customers as part of a commercial app and existing code.... I'm not saying I agree with either side but the fact of the matter is that dnx is completely new framework/runtime. If it doesn't have some interfaces deal with it... with a compiler directive.

FransBouma commented 8 years ago

If it doesn't have some interfaces deal with it... with a compiler directive.

oh? how about a method that returns IDataReader, or a method that accepts IDbConnection? Or IDbTransaction? How are you going to #ifdev around that, if your customers code uses that api?

'Deal with it'... what kind of arrogance is that?

niemyjski commented 8 years ago

Simple, update the underlying package (your org) to return your own type or the base type that they've supported since 2.0. If you are targeting older versions of .net you could use a directive to return the interface instead and mark it as deprecated.

Yes it does suck, but I'm sure they (really smart individuals who think about this all the time) did it for a good reason at the time (back in .net 2.0). Can a talk occur and maybe this gets changed sure.. But the fact of the matter is people upgrading to the new runtime will have to do some work. It's not click a button in a fancy ui and have there work cut out for them.. But at the same time, I agree with you that upgrading to the new framework should be easier than going and replacing a bunch of types.

phillip-haydon commented 8 years ago

@FransBouma he's probably someone who advocates strong naming too.

niemyjski commented 8 years ago

@FransBouma @phillip-haydon can take your trolling other places, you can't expect to act like that and people to take you seriously. Take a look at my open source contributions / projects involved with if you have any doubts... I will have to deal with this... either way...

For the record I'm against strong naming.

phillip-haydon commented 8 years ago

deal with it...

And you say I'm trolling?

niemyjski commented 8 years ago

"@FransBouma he's probably someone who advocates strong naming too." is off topic, and not very helpful to this talk. Yes it feels like your trolling..

nvivo commented 8 years ago

One important fact to consider in this discussion is that the IDb* interfaces were deprecated as an API for ADO.NET in .NET 2.0 a decade ago when the base classes were introduced. They weren't marked deprecated, but anything built since that time depends on the base classes instead. App.config providers and connection string support comes to mind.

If you have code depending on those interfaces, you're coding against a very obsolete API with no support to things like async methods, which means you'll need to update it anyway if you want people to keep using it.

FransBouma commented 8 years ago

Simple, update the underlying package (your org) to return your own type or the base type that they've supported since 2.0. If you are targeting older versions of .net you could use a directive to return the interface instead and mark it as deprecated.

It's a public API, used by many thousands of applications and the API has been public and in use since .net 1.0. It's not 'simple', on the contrary. We can't just change the API because Microsoft thinks that's what they have to do to make our lives better: it will be a big burden for our users and thus for us.

Yes it does suck, but I'm sure they (really smart individuals who think about this all the time) did it for a good reason at the time (back in .net 2.0). Can a talk occur and maybe this gets changed sure.. But the fact of the matter is people upgrading to the new runtime will have to do some work. It's not click a button in a fancy ui and have there work cut out for them.. But at the same time, I agree with you that upgrading to the new framework should be easier than going and replacing a bunch of types.

that's the thing: it's not seen as a 'new runtime'. If that was the case, like I already discussed, it wouldn't be a problem. However Microsoft has the idea that CoreFX targeting dlls have to work on .NET full as well, so there's no new framework. For maintainers of libraries targeting .NET full as well, with functionality not in CoreFX (so a lot of the libraries out there today) are in for a lot of 'fun' as they will have to maintain 2 versions. Not 2 or 3 #ifdevs and the problem is solved, but many many of them. Perhaps it's different for you as you can perhaps bill your client for the hours you have to spend on changing the apis. It's different when you create a general purpose system that's used by many.

If compact framework support is any guideline, supporting an ORM on full .net and CoreCLR will be a terrible time sink, lots of frustration and actually not really much gained: you won't get new features, you only work around the absence of them.

(and before someone starts with: "but it runs on linux, you'll gain that": our stuff runs on Mono since many years. So no, it's not really a new feature to gain, it was already there).

SellMeThisFramework. Oh why am I even bothering.

mgravell commented 8 years ago

but anything built since that time depends on the base classes instead

cough Linq-to-SQL DataContext cough

As, indeed, do many non-MS ORMs, hence the problem. For dapper we just bit the bullet and migrated to DbConnection. If we had the time over again, I would strongly suggest MS use [Obsolete] when they obsolete something. But: we can't change the past.

It is a very painful problem to solve, though, especially since most library authors need to continue with both APIs (compiling one way for net40 etc, and another way for DNX). I posted previously the godawful mess that dapper uses to do this: it is not pretty, and it is not as simple as

if.

On 27 Nov 2015 20:07, "Natan Vivo" notifications@github.com wrote:

One important fact to consider in this discussion is that the IDb* interfaces were deprecated as an API for ADO.NET in .NET 2.0 a decade ago when the base classes were introduced. They weren't marked deprecated, but anything built since that time depends on the base classes instead. App.config providers and connection string support comes to mind.

If you have code depending on those interfaces, you're coding against a very obsolete API with no support to things like async methods, which means you'll need to update it anyway if you want people to keep using it.

— Reply to this email directly or view it on GitHub https://github.com/dotnet/corefx/issues/3480#issuecomment-160198453.

jgauffin commented 8 years ago

I fail to see the point with that async methods couldn't be added as a result of using interfaces. You could have created new interfaces for the async methods. IAsyncDbCommand, IAsyncDataReader etc. Then you could make the base classes implement both types of interfaces.

The ADO.NET users are either using the async version or the synchronous versions, not both. So that would have worked just great.

For library developers it doesn't really matter if functionality grows and the interfaces remain the same. Isn't that the purpose? Introduce new interfaces for new functionality. Working with base classes is just a pain.

mgravell commented 8 years ago

Can I just summarise the thread here?

Multiple independent acknowledged community experts on .NET data tooling, including multiple ORM authors and maintainers are telling you - quite clearly - that this represents a significant set of problems. I don't think any of us are ignorant of the subtleties, or naïve of programming principles, and most if not all of us know all of the back story fine, because we were there at the time.

The official response seems to be "it seems fine to us, and EF is happy". Yes, we know that, because the decision was made in the first place.

Well, we've all expressed our opinions, even if it wasn't fruitful. On 27 Nov 2015 20:41, "Jonas Gauffin" notifications@github.com wrote:

I fail to see the point with that async methods couldn't be added as a result of using interfaces. You could have created new interfaces for the async methods. IAsyncDbCommand, IAsyncDataReader etc. Then you could make the base classes implement both types of interfaces.

The ADO.NET users are either using the async version or the synchronous versions, not both. So that would have worked just great.

For library developers it doesn't really matter if functionality grows and the interfaces remain the same. Isn't that the purpose? Introduce new interfaces for new functionality. Working with base classes is just a pain.

— Reply to this email directly or view it on GitHub https://github.com/dotnet/corefx/issues/3480#issuecomment-160201361.

luisrudge commented 8 years ago

Dudes.. update your code and bump your major version. Done.

niemyjski commented 8 years ago

Yes, but there is no reason you couldn't pollyfill that interface with a compiler directive when targeting core. I've done that with some of our pcl packages.

I do think Microsoft needs to reiterate that core isn't dot net full but that still doesn't help.. I think that Microsoft needs to cleanup some interfaces to be honest. There was a blog post going around recently where there are very inconsistent interfaces and you never know which one to pick.. I think that defining a second async interface sucks. Would be nice if everything was async..

niemyjski commented 8 years ago

Would be nice if the full framework was gone through to make sure things that need to be marked as deprecated are... And released as a 4.6.2

FransBouma commented 8 years ago

@mgravell +100. Well said, 100% agreed.

leppie commented 8 years ago

How much is really affected? We talking about coreclr here? .NET desktop will be alive for many years to come until coreclr can catch up. Exactly for those complaining, what you coverage loss here? Many people are basically saying it is the end of the world.

NickCraver commented 8 years ago

@leppie Indeed, it will be around for many years. And we'll have to maintain these hacks and workarounds in our code for years to come as well. The point of contention here is removal of the common bridge between the two. That workload has been shifted to all of the library developers instead of in the BCL. I understand both sides of the interface pros and cons, but I don't understand the "it's minor, move on" attitude from some here.

Let's be blunt here: If library consumers all have to do the same thing, it should be in the BCL. The debate is "what form does that take?"

On the plus side to removing the interfaces, there's an additional versioning mechanism with the new packaging model now in play: which classes are available in X, Y and Z is now much better supported by tooling. E.g. dotnet5.2 vs 5.4 currently. But then there are drawbacks there as well. For example SqlClient still isn't to the point of implementing the interfaces as it stands today (see dotnet/runtime#14302 and dotnet/runtime#15269), and given what @YoungGah said (unless I'm misreading) we're waiting on 5.4 or 5.5 for that same level of support for schema, bulk inserts, etc.

So what happens with 5.6 (1.5)? If more members are added to the abstract classes each data provider is expected to keep pace with the moving target that can change with every moving person? Every consumer needs to make a version determination on features available in each? We need to compile a version of the assembly for every version of the platform moving forward to match the abstract base of the class being passed in? How all of this works going forward with additions isn't 100% clear. An interface that isn't changing is far clearer. On top of this is documentation: which features, methods, etc. are available in which versions and platforms is going to be a huge pain point for all library authors moving forward. That concern is only semi-related here, but it's in play.

As for now, I'm anxiously awaiting the update in the next 2 weeks. My fear is it will effectively be, as the messaging the entire time has been: "We're doing X because it works for SQL and Entity Framework, and that's good enough" - without using any of those words. The frustration on the library author side, to me, has been a lack of progress (in both code and discussion) on these fronts for months now.

niemyjski commented 8 years ago

100% agree.

sapiens commented 8 years ago

When I've designed v2 (and up) of SqlFu (my Micro ORM), I had to decide where to attach the extension methods: to DbConnection/DbCommand or to the interfaces. I found this and I've decided to go for the abstract classes.

Therefore, I'm not THAT affected, although I am affected by IDataReader removal, because MS in their wisdom decided not to make it very clear which interfaces should be treated as obsolete and which shouldn't. But in my case, it's not hard to replace the interface.

However, I see the value of having dedicated interfaces and I don't think it's THAT hard for MS to keep/add them back. If they decide the old interfaces are not that well designed, fine! Design the new ones to be more specific. At least in the future we won't have to deal with the same problem.

davkean commented 8 years ago

(This is Thanksgiving week in the US, so responses by Microsofties will be pretty limited until they get back in the office next week)

Just want to reiterate - so that the don't lose any good suggestions/bugs in this thread, if you have issues with the current base classes/surface area, and/or the way they version going forward, please file a new issue, let keep this discussion solely about the v1 interfaces.

@NickCraver Similar to version-to-version compatibility of .NET Framework, there will be no breaking changes between versions of the .NET Core surface area. For example, the addition of abstract members to the base classes would be an example of a change that will not be made.

NickCraver commented 8 years ago

@davkean how confident are you that won't happen? Given that we're seeing an incomplete surface area and no guarantees that'll change, it's hard to believe the missing pieces won't show up later if at all. However, I'd think that no breaking changes is a more important thing, which I'm sure most library authors here would assume as well. That means it's even more critical these items are taken care of well before RTM hits.

For the record there separate issues filed on the surface area, see dotnet/runtime#14302 and dotnet/runtime#15269 with latest updates from Microsoft on Sep 25th and October 2nd respectively - despite asking for updates and activity several times after that. That's 2 months and 2 releases that have come on gone, with silence. This is despite dotnet/runtime#14302 being the most active issue in this repo (and this one has just become the 2nd). Can you understand our frustration?

davkean commented 8 years ago

I'm absolutely confident we won't make breaking changes, Data Common team is looking at making them without introducing abstract members.

@NickCraver Sorry we're doing badly here - these issues have not been forgotten, @YoungGah provided an update above about them, I'll make sure she updates the issues with the progress. Lots of MS is still getting used to this working in the open thing, it will get better over time - thanks for calling us out on it.

mythz commented 8 years ago

@niemyjski

dnx is completely new framework/runtime

If you think dnx runtime and corefx libraries manifested out of thin air, you're seriously underestimating the time it would take to develop it from scratch. The fact that CoreFx libraries runs on the full .NET Framework should give you a clue that, no, it's not completely new.

If it doesn't have some interfaces deal with it... with a compiler directive.

Yes, but there is no reason you couldn't pollyfill that interface with a compiler directive when targeting core.

If you've bothered reading the comments before flying into this thread you'll know that a) there are reasons and b) this is a broken unworkable strategy for core BCL interfaces.

@nvivo

One important fact to consider in this discussion is that the IDb* interfaces were deprecated as an API for ADO.NET in .NET 2.0 a decade ago when the base classes were introduced.

If that were the case then you wouldn't have opened an issue telling them to revert to using interfaces you knowingly knew were deprecated by base classes a decade ago. The way you communicate an API is deprecated is to use the [Obsolete] attribute which is its sole purpose for existing. If it's not widely communicated, it's not deprecated regardless of what your thoughts are now. The fact that most non-MS .NET ORM's are reliant on them should give an indication that its deprecation was poorly communicated, if at all.

If you have code depending on those interfaces, you're coding against a very obsolete API with no support to things like async methods, which means you'll need to update it anyway if you want people to keep using it.

A false strawman - one does not imply the other. We've already added support for async API's, and no, adding them didn't break existing Customer code-bases nor did any of the existing API's need to change.

ryanbnl commented 8 years ago

Okay, this whole making a clean start thing is great, but can I ask one question: What compromises have been made to support your own frameworks? What horrors of the past have been migrated because they're needed to get, say, Entity Framework running?

It would be a shame to make the MicroORMs disappear, they make .Net code somewhat performant (EF's an unusable beast for applications where 500ms to load a few rows is not acceptable).

As for interfaces vs base classes: base classes are great as long as everything that can ever be re-used is virtual. For example one of the most irritating design decisions in WCF is the copious use of sealed classes which contain a lot of functionality. Say you have to make a tiny tweak to the way say XML messages are handled (because: interop). Instead of inheriting and overriding one small function, you have to re-implement. In the WCF example the S in SOLID was skipped so you're usually left implementing a large interface with none of the tests that are required to ensure it is of production quality.

So: base classes which we can adapt are a good thing.

FransBouma commented 8 years ago

I'm absolutely confident we won't make breaking changes, Data Common team is looking at making them without introducing abstract members.

@davkean That's impossible to guarantee, and you know that. ADO.NET is a subsystem to communicate with 3rd party software with a wide variety of features, which are exposed through a common API with some extension points. Things change, even in database-land, and these changes ripple through to the API used to communicate and consume these external database services. Besides: changes in behavior is also a breaking change. And we've seen those too in ADO.NET (e.g. about error handling) in the past years.

The whole ADO.NET API is full with the side effects of those changes, often driven by SQL Server; it's hardly been the case that things were designed in general and then moved to SQL Client, but the other way around (there are e.g. not many, if any, features in the base classes which are ignored by SqlClient). Added to that things needed by 3rd parties never made it to the API.

So in short, it's an API that, at the start with .NET 1.0 had a general design (which proved to be seriously flawed in many areas) and which since then as been patched up with features left and right to cope with the changes in the landscape. Most of those are still in the API (if not all). And now Microsoft will remove one: the interfaces.

There's absolutely nothing gained by removing a random piece of the API: no feature is added through this (you could spend the time on that instead of pushing back here for instance), but code that leverages on that piece of API will not work. If the point behind all this cheese moving is 'to start over' then by all means, do, but do it by redesigning the API to make it a true general purpose API, get rid of all the cruft that's been piled up over the years.

But that's not done. No-one has to wonder why, we all know why. We also know that if a team within MS would have been hurt badly by the removal of the interfaces, they would never have been removed in the first place.

If Microsoft can add new functionality through base classes so 3rd party providers automatically implement 'a form of' the feature (like with Async, where the async method falls back to the sync method if the provider doesn't implement it), great: that means us 3rd party ORM developers (and face it: many many developers access your API through the code of 3rd party (micro)ORMs) can simply target one class and that's it.

But it's not guaranteed you'll do that. E.g. no-one within Microsoft has ever bothered with specific features for Oracle or PostgreSql to be added to ADO.NET. E.g. usage of UDTs, Document trees, multiple resultsets through cursors, every ADO.NET provider has to find their own way of dealing with these features. What if (when?) SQL Server gets a document tree feature e.g. with JSON docs, will ADO.NET be updated then with a new API for that? Like you have done in the past with error reporting from ADO.NET providers?

You can't make guarantees like that, and it's unwise to state here that MS won't break anything in the future. They always have and sometimes they even had to: after all every API has flaws, and ADO.NET is full of them; one way or the other some day someone will 'fix' them.

So, to recap: the interfaces are part of ADO.NET as also the botched up parts elsewhere in the API are part of ADO.NET. Those aren't removed to fix the API, neither is the API refactored to make it a more general purpose API: it's been left as-is, with some elements removed like DataSet/Table depending elements because these aren't ported (and there are other issues debating that with similar progress), except... interfaces are removed.

From that point of view alone it already doesn't make any sense.

nvivo commented 8 years ago

@mythz

If that were the case then you wouldn't have opened an issue telling them to revert to using interfaces you knowingly knew were deprecated

You can't possibly read the OP and understand that. These discussions are getting too religious and you're just assuming things that nobody said.

I opened this issue because I believe interfaces are better at describing an api and help with testing. If it's done, I don't think they should be compatible with a 15 year old api that had its issues. Backwards compatibility was never a point in the issue until you guys moved the discussion to that.

It's not that I believe things should break just for the sake of it. But interface versioning is a problem of the past. If corefx changes something between major versions, its expected major versions to have breaking changes. If they break an interface between minor versions, that's just sloppiness.

We've already added support for async API's

You can't add an async api on top of a sync api. If you did that using IDbConnection or IDbCommand, you did it wrong. If you're not using these interfaces, then you don't actually have any point defending any backwards compatibility with them.

FransBouma commented 8 years ago

We've already added support for async API's

You can't add an async api on top of a sync api. If you did that using IDbConnection or IDbCommand, you did it wrong. If you're not using these interfaces, then you don't actually have any point defending any backwards compatibility with them.

In ADO.NET that's what they did: the Async methods by default fall back to the sync variants. This way all ADO.NET providers who don't support Async (read: all of them except SqlServer at this moment) don't have to implement things they don't support: 3rd party code in ORMs offering an Async api can program against the Async methods in ADO.NET and if the ado.net provider doesn't support async, no-one will actually know. Well... you will know because it's slower, but that aside.

Now it's also a good illustration of the absence of any 'design' or general architecture within ADO.NET: there's no way to create a transactional savepoint in the general API. Though almost all databases support that with a 'Save(string)' method on their derived class from DbTransaction. All except OleDbTransaction (as MS Access doesn't support it, at least that's my suspicion).

It's not easy, but no-one said it to be easy. This problem isn't new, OleDB and ODBC have dealt with it for many many years, JDBC has found a way to solve it, Microsoft doesn't have to re-invent the wheel to overcome things like this. It's also not unique to the DB realm: e.g. every videocard out there supports a different subset of features through its API, exposed to the developer through Direct3D/X. It's actually interesting how designs go in these other worlds: the API is designed, and the parties who need to support it (JDBC drivers, OleDB driver writers etc.) have to implement these. Your driver won't support X? Your driver isn't compliant with X. "Oracle doesn't support ADO.NET v10". No-one within Oracle wants to read that. Instead SqlClient is the lead, and what falls off the wagon is added to ADO.NET and that's it.

nvivo commented 8 years ago

In ADO.NET that's what they did: the Async methods by default fall back to the sync variants.

No, it isn't. The API expose async methods that fallback to sync methods by default, but providers override with real async operations. What @mythz is stating is that he is using IDbCommand and IDbConnection and doing that.

This is not possible, period. If you do it, you either are not doing it right or you're not using the interface. You can't invent async if the underlying api is not async.

FransBouma commented 8 years ago

No, it isn't. The API expose async methods that fallback to sync methods by default, but providers override with real async operations. What @mythz is stating is that he is using IDbCommand and IDbConnection and doing that.

No official provider does that except SqlClient, all other, e.g. ODP.NET, don't implement any form of async code and thus calling code falls back to the sync variants (the async methods in DbDataReader/DbCommand etc. which actually execute sync code). so user code calls an async variant, which is under the hood doing sync operations. Which results in no async being performed in practice (as all code is simply sync in practice). Perhaps devart's providers implement an async api in their own implementation, not sure.

Anyway, it's not about doing it right, it's about versioning of APIs.

mythz commented 8 years ago

@nvivo

I opened this issue because I believe interfaces are better at describing an api and help with testing. If it's done, I don't think they should be compatible with a 15 year old api that had its issues. Backwards compatibility was never a point in the issue until you guys moved the discussion to that.

Ok so you knew the core ADO.NET interfaces were deprecated 10 years ago, with everything moved to base classes, yet you thought they should just abandon that now and move back to interfaces, coincidentally using the same names as the existing interfaces, but the existing interfaces should no longer exist, because backwards-compatibility not required right? sure, sounds legit.

If you want to move a platform forward, you evolve the API's over time and support them in parallel, giving everyone the ability to also support parallel API's and allow them to plan their way and their Customers off them. Ripping them out without warning unnecessarily breaks the ecosystem relying on them and pushes the complexity down to every downstream dependency.

You can't add an async api on top of a sync api. If you did that using IDbConnection or IDbCommand, you did it wrong. If you're not using these interfaces, then you don't actually have any point defending any backwards compatibility with them.

I wish you'd stop polluting this thread with comments on things you clearly have no knowledge about. Read the source code if you want to know how the Async API's are implemented - and stop blindly spreading falsehoods. It's impossible for a 3rd party library to extend System.Data interfaces. We provide a implementation-agnostic API which in order to support every major RDBMS, exposes the minimum dependency that every ADO.NET provider implements in its external facing API - namely the core System.Data interfaces. Async API's are extension methods off IDbConnection which behind-the-scenes leverages async API's on the concrete ADO.NET providers that have support for them. Internally there are already concrete dependencies on each supported ADO.NET provider, irrespective of async support. Your suggestion that we "don't actually have any point defending any backwards compatibility with them" is inexperienced and completely groundless.

NickCraver commented 8 years ago

Let me ask this to the Microsoft side of the fence (cc @davkean @YoungGah): Let's say it's a perfect world and nothing ever came up. When do you want to break things? Major versions like 6.0? Some other time? The argument against interfaces is that they don't version. Well yes, that's valid - but if we're not changing the abstract classes either, it's also a moot point. So...can we get some clarity there?

Follow-ups: If the answer is yes (at some post-RTM point there will be changes), then what kind of breaks would we see? Additions, new methods? If I inherit the base class for my provider then what happens when you add a conflicting method people are using, etc.?

If the answer is no (never): why not just add the interfaces back?

This thread is a bit hung on discussing right now - which is mostly a good thing because this stuff needs fixing ASAP. Every library author here knows how hard getting anything after a release done is, and it's why we push so hard. Unfortunately, lack of a clear plan for future additions and changes, if any, makes for more under-informed argument.

What are the plans for the future?

phillip-haydon commented 8 years ago

We shouldn't be forcing implementation via abstract classes in this case. IMO

JamesNK commented 8 years ago

Microsoft won't make changes that are breaking for .NET 4.5. It's part of Windows. Compatibility is king.

Microsoft could make changes that are breaking in .NET Core that don't impact on 4.5. I doubt that they will post 1.0 RTM on something low level like ADO.NET, but the bar is lower. It's not part of Windows and .NET Core versions can be deployed side by side.

Abstract classes can be changed - that's not breaking. Just add a method that is virtual with a default implementation. It's already been done with the ADO.NET async methods. With the introduction of .NET Core I believe changes in shared classes would need to be done in concert with .NET 4.5 releases to keep compatibility. Someone correct me if that is wrong and only applies to interfaces :grin:

nvivo commented 8 years ago

@FransBouma

No official provider does that except SqlClient, all other, e.g. ODP.NET, don't implement any form of async code and thus calling code falls back to the sync variants

You're right, but that's not an issue with the API, and more lazyness or lack of understanding by the implementers. MySql connector for instance re-implemented all their async methods by creating a TaskCompletionSource and completing them with sync methods, which is ridiculous. They could just delete half of their code base and remain with the same behavior.

Not saying that interfaces would solve that, but not having a default behavior for async would at least make some of them think this through. The fact that 90% of the very technical people don't understand async operations doesn't help too.

phillip-haydon commented 8 years ago

Abstract classes can be changed - that's not breaking. Just add a method that is virtual with a default implementation. It's already been done with the ADO.NET async methods.

This IS breaking. It's breaking for all libraries which sub classed this implementation without knowing it was added and people then consume this thinking. Oh this implementation is now supported with postgresql BAM ERROR wtf happened...

Forced implementation for a db abstraction is wrong.

Does not matter if its interfaces or a base class. There will be breaking changes. But forced predefined implementation is wrong.

JamesNK commented 8 years ago

Polymorphism doesn't work that way. You can't override a method without knowing it. If your reference is a DbConnection and you call QueryAsync, it will only call that method or whatever it has been overriden as. A method called QueryAsync that happened to already exist in a subclass won't be called.

You're confusing overriding a method verses hiding it with the same name.

phillip-haydon commented 8 years ago

@JamesNK

If the methods are defined as abstract, no implementation exists in the base class. This breaks the contract for the 3rd parties as it requires them to add implementation in the sub class.

If you make the method virtual so it can be overridden, then implementation exists in the base class that could make no sense to the sub class. This is still breaking because implementation exists that was not implemented by the library author. Sure your app can compile, and everything is hunky dory, but someone calls that method, and its not valid for the sub class. That's wrong. That is forced implementation that does not belong to the sub class.

So, abstract class where implementation can exist that does not belong to the sub class. Or interfaces where no default implementation exists for the 3rd parties.

ryanbnl commented 8 years ago

@phillip-haydon that's why it's implemented as a virtual method, not an abstract method.

You can add stuff it will only break subclasses which already have a member with the same signature (name/args). If the args are different it could introduce subtle bugs if developers mistake the overloads.

JamesNK commented 8 years ago

That is forced implementation that does not belong to the sub class.

Then don't put it there.