ADO.NET factory methods should throw instead of returning null

[roji]

Some ADO.NET APIs create objects which not all providers support. For example, DbProviderFactory.CreateCommandBuilder() returns a DbCommandBuilder, but some providers don't support this functionality (e.g. SQLite if I'm not mistaken, @bricelam). The current implementation for all the methods on DbProviderFactory is to return null by default, which serves as a sort of "feature detection mechanism" - consumers can check if the result is null to know if the feature is supported or not.

Aside being a generally problematic mechanism (users get NullReferenceException instead of the clearer NotSupportedException), this mechanism is also not suited for C# 8 nullability. If certain providers can return null to indicate that the feature isn't supported, than it seems like the Create*() method will return a nullable reference. This would impose on everyone to either check for null, or use the bang (!) operator to tell the compiler that no null is actually possible there - not good practice since users simply know that their specific provider supports the feature. Conversely, if the Create*() method returns a non-nullable reference, then you may get warnings for checking it for null.

Instead, we should change these methods to throw NotSupportedException (breaking change), and add Can*() virtual bool properties alongside these methods to indicate whether the feature is supported or not (similar to how DbProviderFactory.CanCreateDataSourceEnumerator exists alongside DbProviderFactory.CreateDataSourceEnumerator(). The default implementation of these capability properties could be to call the corresponding method, catching NotSupportedException and returning based on that.

This should be done for .NET Standard 2.1 if possible.

Originally discussed in https://github.com/dotnet/corefx/issues/35135#issuecomment-466452344

[roji]

/cc @divega @bricelam @NickCraver @mgravell

[benaadams]

Would that be a breaking change? https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/breaking-change-rules.md#exceptions

/cc @terrajobst

[roji]

@benaadams the current methods don't throw any exceptions, so methods which previously returned a null (which could be checked) would now start throwing NotSupportedException - I think that's a breaking change, no?

[mgravell]

Sounds undesirable and breaking to me. If it was being done like this in the first place: fine, but after the fact? not good IMO. Personally I'd be OK with making it return an advertised nullable reference, as long as that only impacts people who have opted in to nullable testing. Just my tuppence.

[mgravell]

Follow up question that may change things: is the common pattern here to just override? Or is it to redeclare/hide (new) the method and override another method? The latter pattern is very common in ADO.NET, and if it is the case here: no problem. Subclasses that hide the method can advertise it as non-nullable, and if you're talking to the base class: you should already be testing.

[divega]

@mgravell that pattern is not used in DbProviderFactory implementations, at least not commonly. But on the other hand, I know of very few (only one actually) provider implementations that don’t override all the required methods.

[roji]

@mgravell, while this change would apply to all methods on DbProviderFactory, most methods there are "mandatory" and it's very hard to imagine a provider which hasn't overridden them (e.g. DbProviderFactory.CreateCommand()). In practice, this will only affect CreateDataAdapter(), CreateCommandBuilder() and CreateDataSourceEnumerator(), which seem like they're rarely-used. Given that that's the case, do you still think there's danger of significant breakage/messiness here?

Personally I'd be OK with making it return an advertised nullable reference, as long as that only impacts people who have opted in to nullable testing.

I think that the idea is to make the new nullability checking recommended and 1st-class experience, so forcing people who've opted into it to check all the time doesn't seem like a good way forward...

@divega should we also add a CanCreateDataAdapter() to DbProviderFactory? It seems like an optional method but there's no method for it currently.

An a final thought: for the mandatory methods on DbProviderFactory, we could go a bit further and make them abstract (rather than throw). Unless I'm mistaken, assuming providers have implemented them, this wouldn't even be a breaking change.

[ajcvickers]

Triage: We decided not to make the breaking change here as the value doesn't seem high enough. @roji to open a new issue with a different approach.