Nullability annotations for main System.Data.Common types

[roji]

General Notes

This PR is a proposal for annotating the main types in System.Data.Common (S.D.C). It focuses on types necessary to create ADO.NET providers, and leaves out various non-essential types; this will be revisited at a later time.

Unlike the usual NRT annotation exercise, here we're annotating a provider abstraction; there is very little actual logic in S.D.C we can consult to determine what should be nullable. In addition, specifications around nulls are unfortunately rarely present in the documentation. As such, the behavior of existing major providers was used as a guidance for annotation, as a "de-facto standard". Four providers were checked: Microsoft.Data.SqlClient, Npgsql, MySQL Connector and Microsoft.Data.SQLite. The AdoNetApiTest suite was used extensively to map out behavior across providers; this exercise also resulted in many test additions to that suite as well.

The goal here is to gather feedback from ADO.NET driver developers and other people working in the .NET database space, as well as from BCL experts and people experienced in NRT annotation.

/cc @cheenamalhotra @David-Engel @saurabh500 @Wraith2 @bgrainger @ErikEJ @FransBouma @mgravell @bricelam @ajcvickers @AndriySvyryd
/cc @stephentoub @cartermp @rynowak

Null-returning base method implementations

There are unfortunately several cases where a base S.D.C implementation returns null for a type that providers must provide. Examples include methods on DbProviderFactory (e.g. CreateConnection), DbConnection.DbProviderFactory, etc.; these properties really should have been made abstract, or at least throw instead of returning null.

We have three options:

1. Make these properties throw. This would be a breaking change (largely theoretical because providers really must implement these).
2. Annotate these properties as non-nullable. This would avoid the breaking change, but for people who opt into the NRE feature, and who use a badly-implemented provider, null would be exposed.
3. Annotate these properties as nullable. This is a disservice for the 99% case, where users would have to satisfy the compiler with needless checks.

Depending on our backwards-compatibility bar, I believe we should at least do 2, possibly 1.

I believe the breaking change is worth it (option 1).

See some previous discussion in https://github.com/dotnet/corefx/issues/35535, and specific discussion below.

De-facto normalized string properties

There are several cases of string properties which are documented to be an empty string by default, and which providers generally normalize to empty string when null is set:

• DbConnection.ConnectionString
• DbCommand.CommandText
• DbConnectionStringBuilder.ConnectionString
• DbParameter.ParameterName, SourceColumn

As per the BCL nullability guidelines:

DO define a parameter as nullable if the method checks the parameter for null and does something other than throw. This may include normalizing the input, e.g. treating null as string.Empty.

As the de-facto provider behavior is to allow nulls (and normalize to empty string), these properties are annotated as non-nullable with [AllowNull].

Note that the same holds for the setter of DbConnectionStringBuilder.this[]: setting a keyword to null means removing it.

---

Specific Type-By-Type Notes

This section details some annotation details which are noteworthy. Where annotation was straightforward no notes were made.

DbDataReader/IDataReader/DataReaderExtensions

• GetSchemaTable/GetColumnSchema: annotated non-nullable for now, as per DbDataReader.GetSchemaTable without a resultset should return empty table instead of null #509
• GetName/GetDataTypeName/GetFieldType/Get*: these throw when resultset isn't present, and should throw in any other error case. Annotating as non-nullable.
• GetBytes/GetChars: in SqlClient and Npgsql, passing a null buffer is valid and simply returns the number of bytes/chars in the column (docs). Annotating as nullable.

DbTransaction/IDbTransaction

DbTransaction.Connection: always non-null before commit/rollback, always null afterwards; so annotated it as nullable. It would have been better for DbTransaction to transition to a disposed state immediately after commit/rollback, in which case the Connection would have been non-nullable.

DbProviderFactories

No specific comments

DbProviderFactory

• All the Create* method base implementations unfortunately return null. There are two cases here:
  • Methods which providers must override (e.g. CreateConnection) - the provider cannot function correctly without these. I've changed these to throw NotSupportedException by default. See general note above.
  • Methods which providers may override (CreateCommandBuilder, CreateDataAdapter, CreateDataSourceEnumerator). These already have CanCreate* feature check properties, which provide a way for consumers to check support before calling the method. So I'm changing these to throw NotSupportedException as well. See previous discussion in https://github.com/dotnet/corefx/issues/35535.

DbCommand

• CommandText:
  • SqlClient and Npgsql have this as empty string by default (this is actually specified in the docs). Sqlite and MySqlConnector have null here (in process of being fixed). Setting to null normalizes to empty string (Npgsql, MySQLConnector and Sqlite throws but this is being fixed).
  • Annotating as non-nullable with [AllowNull] as per guidelines (see note above).
  • Issues: CommandText should be an empty string by default mysql-net/MySqlConnector#743, Sqlite: CommandText should be an empty string by default #743 efcore#19162, Minor nullability fixes npgsql/npgsql#2762
• ExecuteScalar returns null when there is no resultset or no rows (this is documented, and all provider implement this behavior).

DbConnection

• ConnectionString - similar to DbCommand.CommandText
  • All but Sqlite have an empty string by default, as per the docs.
  • All but Sqlite also normalize to empty string when setting to null.
  • Annotating as non-nullable with [AllowNull] as per guidelines (see note above).
• Database (on unopened connection)
  • The docs specify an empty string as the default value. Also, since applications can/should know if the connection is open or not, am annotating this as non-nullable.
  • Sqlite returns "main", SqlClient & MySqlConnector return empty string, Npgsql returns null (being fixed)
• DataSource (on unopened connection)
  • Sqlite returns null, SqlClient & MySqlConnector return empty string, Npgsql returns mangled value (being fixed)
  • The docs specify an empty string as the default value. Also, since applications can/should know if the connection is open or not, am annotating this as non-nullable.
• ServerVersion (on unopened connection)
  • All providers except Sqlite throw InvalidOperationException; this is kinda specified in the docs.
  • Annotating as non-nullable.
• DbProviderFactory
  • Internal, but the return value is exposed publicly through DbProviderFactories (so public in practice).
  • Defaults to null in DbConnection, but really expected to be non-null for any actual ADO provider.
  • All providers implement this correctly except Sqlite, which returns null.
  • Am annotating as non-nullable for now, we could even consider changing the default implementation to throw, as this must be provided.
• EnlistTransaction: There is evidence that passing a null parameter is a way to un-enlist from the current transaction, NHibernate seems to rely on this as well. Support enlistment with null npgsql/npgsql#1580, https://stackoverflow.com/questions/12863950/what-does-dbconnection-enlisttransaction-do. Annotating as nullable.
• GetSchema
  • Default base implementations throw NotSupportedException, and no provider returns null. Annotating return value as non-nullable.
  • The overload accepting a restrictions array allows nulls to request the default value for that restriction, annotating as nullable.

DbConnectionStringBuilder

• ConnectionString
  • Defaults to empty (documented and implemented by all providers) and normalizes null to empty string.
  • Annotating as non-nullable with [AllowNull] as per guidelines (see note above).
• In general, connection string values cannot be null - setting a value to null means removing it.
• Annotating the indexer as non-nullable with [AllowNull] as per guidelines (see note above).
• As an exception, annotating the parameter to Add as non-nullable, although it technically does allow removing a keyword by passing null. An explicit interface implementation of IDictionary.Add still exists which accepts nullable and uses the null-forgiving operator to delegate to the non-nullable implementation.

DbParameter

• ParameterName, SourceColumn
  • Defaults to empty string, as documented, on all providers except Sqlite (where it is null). Normalizes null to empty string.
  • Annotating as non-nullable with [AllowNull] as per guidelines (see note above).
• Value
  • Defaults to null and can be set to null on all providers. Executing with null parameter value makes sense for output (and in/out parameters) which will receive a database value, but for input parameters should always error (null is represented by DBNull.Value).
  • Annotating as nullable.

DbParameterCollection / IDataParameterCollection

• Null parameters (or parameter names) are not supported by any provider (Sqlite has some bugs around this). The ADO.NET to send a null value is to have a non-null parameter instance with a DBNull value.
• However, IDataParameterCollection implements IList, which allows nulls and therefore has nullable annotations on parameters and return values.
• As a result, explicit interface implementations for IList methods have been added with nullable parameters/return values, which delegate to the DbParameterCollection non-nullable implementations. No runtime behavior change has been introduced: the null-forgiving parameter is used, so if a null happens to be passed in, it gets passed as before to the overridden implementation which is expected to throw.

DbColumn

• According to the docs, ColumnName seems to be the only mandatory field (null is explicitly mentioned elsewhere).
• The only question mark in my mind is DataTypeName, which seems to be rather mandatory (unless we're dealing with some unrecognized/unsupported column or something - but I'm not aware of such a case). Am annotating as nullable.
• All others values can legitimately be null (e.g. for SELECT 1 there is no table, schema...).

---

Edit history

Date	Modification
2019-12-09	Initial version

[buyaa-n]

This will be needed for annotating the entire System.Data.Common, by dependency we are not that close to annotate System.Data.Common yet, but adding reference to main issue https://github.com/dotnet/corefx/issues/40623 for tracking

[carlossanlop]

@roji Your PR is adding new public APIs in the ref file. Please make sure to document them with triple slash comments so the reviewers can sign off your change. We want to make sure all PRs introducing new public APIs get properly documented before getting merged.

These are explicit interface implementations (EIIs) so it's ok if you copy-paste the documentation for the original APIs. You can add a remark with a message that looks like this (replace the two [keywords]):

<remark>
This member is an explicit interface member implementation. It can be used only when the [instanceName] instance is cast to an [interfaceName] interface.
</remark>

[roji]

Thanks @carlossanlop, this PR is on hold for now as I complete other work, but when I get back to it I'll definitely make sure to do the proper documentation.

[roji]

@carlossanlop I've revived this and added XML docs following the model for DataSet.IListSource.GetList which you linked to. If you prefer me to copy the actual docs from IList I can do that (or do we may support <inheritdoc />?

[stephentoub]

@carlossanlop, rather than requiring such copy/paste, can we just automatically handle that for explicitly implemented members?

[roji]

FYI have removed the new-api-needs-documentation (as the boilerplate XML docs have been copied in).

[carlossanlop]

The DocsPortingTool is now able to detect an EII API and automatically add the sentence I suggested if it does not yet exist in triple slash.
Feel free to remove that sentence from this PR. I wrote the original suggestion back when the feature wasn't available in the tool.

[stephentoub]

Great to hear that was added. Thanks.

[stephentoub]

this PR is on hold for now

Can we close it then and open it again when it's ready to be reviewed/merged? Thanks.

[roji]

@stephentoub closing, am assuming this is to do things bottoms-up as in #2339?

[stephentoub]

assuming this is to do things bottoms-up

That's how we prefer to do it, yes. But we also don't want "pull" requests sitting stale that aren't ready to be "pulled", and you stated it was on hold indefinitely :-)

[roji]

No problem, am indeed busy with other stuff at the moment anyway, and it makes sense for this to wait until the stack is annotated underneath.

For anyone else, please feel free to take a look at this and also to comment accordingly, we'll pick this up soon from where we left off.

[stephentoub]

Hey @roji, we've annotated most of the dependencies this has, so if you're interested in pushing this forward and want to open a PR with the annotations, it's a reasonable time. Thanks!

[roji]

I've rebased this and made some adjustments - it should be ready for reviewing.

[roji]

/cc @jeffhandley @stephentoub

[roji]

PS Build is passing aside from some Helix-only failures (WASM mono assertion issue...)

[stephentoub]

I do think we should consider not changing the behavior here, but still annotating these as non-nullable (as proposed above).

We should not do that. Either we're confident that no concrete implementations will return null (either explicitly or because they don't override the base method and get the base's null-returning behavior), or we're not. If we're confident it never happens, then the methods can be non-nullable and can be changed to throw. If we're not confident, then annotating them as non-nullable but returning null is making things worse: we're claiming to consumers they can trust they'll never get back null, but that's simply not true.

Just to make sure it's clear, too, concrete providers can be annotated to return non-nullable even when the base virtual is null-returning. So, for example, even if we have:

public abstract class DbProviderFactory
{
    public virtual DbCommand? CreateCommand() => null;
    ...
}

we can still have:

public sealed class SqlClientFactory : DbProviderFactory, IServiceProvider
{
    public override DbCommand CreateCommand() => new SqlCommand();
}

Anyone using SqlClientFactory directly would see that CreateCommand returns a non-nullable DbCommand; only when using the base provider would they see that it returns nullable.

We also don't need to do the same thing for every method in question. The ones we're talking about are these 8?

public abstract class DbProviderFactory
{
    public virtual DbCommand CreateCommand() => null; 
    public virtual DbCommandBuilder CreateCommandBuilder() => null;
    public virtual DbConnection CreateConnection() => null;
    public virtual DbConnectionStringBuilder CreateConnectionStringBuilder() => null;
    public virtual DbDataAdapter CreateDataAdapter() => null;
    public virtual DbParameter CreateParameter() => null;
    public virtual CodeAccessPermission CreatePermission(PermissionState state) => null;
    public virtual DbDataSourceEnumerator CreateDataSourceEnumerator() => null;
}

plus

public abstract class DbConnection
{
    protected virtual DbProviderFactory DbProviderFactory => null;
}

right?

Looking at https://source.dot.net and https://referencesource.microsoft.com/, I see every single code path that ends up hitting this (generally via DbProviderFactories.GetProvider) then null checks the result and throws if null was returned. And looking around on github, I see lots of code that just assumes it's not null and dereferences the result without checking. So (and please correct me if I'm wrong), that other than the type itself using its own protected override, null being returned is already effectively considered an error. If that's true, DbConnection.DbProviderFactory could be made non-nullable and changed to throw with minimal impact.

Looking just at dotnet/runtime, neither OdbcFactory nor OleDbFactory overrides CreateDataSourceEnumerator, so they're going to inherit the base method behavior; same for EntityProviderFactory in reference source. Even if we were to "fix" those, that's a key indicator that other implementations are unlikely to override it either. So that needs to be DbDataSourceEnumerator?.

Code Access Security is deprecated, so CreatePermission doesn't really matter. It can return CodeAccessPermission? and if folks get warnings, great, they should stop using it. Even without that, though, I see implementations not overriding it and thus inheriting the base null-returning implementation, e.g. https://github.com/Glimpse/Glimpse/blob/master/source/Glimpse.Ado/AlternateType/GlimpseDbProviderFactory.cs.

You mentioned that "CreateCommandBuilder and CreateDataAdapter return null on Microsoft.Data.Sqlite" (as can CreatePermission and CreateDataSourceEnumerator, per https://github.com/dotnet/efcore/blob/master/src/Microsoft.Data.Sqlite.Core/SqliteFactory.cs). If they return null for such a prominent implementation, that's a warning sign. On to of that, if memory serves these were introduced later than the other methods, which means implementations based on the original DbProviderFactory surface area wouldn't have overridden these because they didn't exist; that's exemplified in examples like https://github.com/mysql-net/MySqlConnector/blob/master/src/MySqlConnector/MySqlConnectorFactory.cs and https://github.com/jonwagner/Insight.Database/blob/master/Insight.Database.Core/DbConnectionWrapperProviderFactory.cs, where these methods won't be overridden when targeting surface area like .NET Standard 1.6 or .NET Core 1.1. So... CreateCommandBuilder and CreateDataAdapter should return nullable.

Here's an implementation that doesn't override either CreateParameter or CreateConnectionStringBuilder:
https://github.com/CollaboratingPlatypus/PetaPoco/blob/development/PetaPoco/OracleProvider.cs
And another:
https://github.com/cyq1162/cyqdata/blob/master/DAL/NoSql/NoSqlFactory.cs
And another that doesn't override CreateConnectionStringBuilder:
https://github.com/Lifeng-Liang/DbEntry/blob/master/src/Leafing.MockSql/RecorderFactory.cs
And here's one that explicitly returns null from CreateConnectionStringBuilder:
https://github.com/Dogwei/Swifter.Json/blob/master/Swifter.Data/ProxyProviderFactory.cs
and here's one that doesn't override anything other than CreateCommand or CreateConnection:
https://github.com/ChrisFulstow/NBlog/blob/e50a75d83a85e9a31e2ab7fc1a1c8fa7d29e1fa7/NBlog.Web/Models/PetaPoco.cs#L2175-L2207

I couldn't find any implementations that returned null from CreateConnection/CreateCommand. Maybe we could get away with making those two non-nullable and throwing from the base.

If all of the above makes sense, that would leave us with:

public abstract class DbProviderFactory
{
    public virtual DbCommand CreateCommand();
    public virtual DbConnection CreateConnection();

    public virtual DbCommandBuilder? CreateCommandBuilder();
    public virtual DbConnectionStringBuilder? CreateConnectionStringBuilder();
    public virtual DbDataAdapter? CreateDataAdapter();
    public virtual DbParameter? CreateParameter();
    public virtual CodeAccessPermission? CreatePermission(PermissionState state);
    public virtual DbDataSourceEnumerator? CreateDataSourceEnumerator();
}

public abstract class DbConnection
{
    protected virtual DbProviderFactory DbProviderFactory { get; }
}

but I'd like more data from @marklio's data sources, if possible. And with most of the above returning nullable, it may just make sense to do the simple / honest / non-breaking thing, and return nullable from all of them, reflecting exactly the implementation we have today.

[roji]

Just to make sure it's clear, too, concrete providers can be annotated to return non-nullable even when the base virtual is null-returning.

Sure, but the whole point of DbProviderFactory is for people to use it without referencing SqlClient types directly (e.g. SqlProviderFactory). If I'm coding directly against my ADO.NET provider (as most people do) - rather than writing a database-portable application/library - I can just instantiate an SqlConnection directly rather than using the factory. So the value of the annotations here is mostly about what happens in the abstract base class (DbProviderFactory).

The ones we're talking about are these 8?

Yes.

If that's true, DbConnection.DbProviderFactory could be made non-nullable and changed to throw with minimal impact.

I absolutely agree. Every ADO.NET provider must have an implementation of DbProviderFactory, and its DbConnection.DbProviderFactory must return that. It's good to know that usage confirms this, I will change it to throw.

Regarding the rest... I agree that for CreatePermission it doesn't really matter. On the others, I'd argue that in most cases, the providers not implementing CreateParameter and the like are buggy, i.e. not respecting the ADO.NET contract. In other words, it doesn't seem possible to successfully use these providers via the DbProviderFactory abstraction; this might not be a problem for users since the providers in question are probably not being used by portable applications. If that's true, then changes to DbProviderFactory won't impact these users in any case.

To summarize, changing to throw will only affect database-portable consumers which call CreateParameter (and the like), and check the result for null. But if null is indeed returned, it's not clear how that application can function properly.

However, I can also see what you're seeing. If the only remaining question here is about CreateConnection/CreateCommand, then as you suggest let's just annotate all of them as nullable and be done with it. Let me know what you decide.

PS Unrelated: is it OK to merge this PR with the unrelated failing WASM tests on Helix? I've just relaunched it in case it's flakiness.

[stephentoub]

i.e. not respecting the ADO.NET contract

That's the problem: they are. There's nothing about this in the docs that I can see, and the base implementations (which by definition adhere to the contract) are returning null. Is there some place where the contract is more explicitly defined? Are there docs anywhere that state null shouldn't be returned?

[roji]

No, ADO.NET is notoriously badly-defined - if our bar for changing anything is a clear sentence in specs/docs, then we definitely don't have that. I'll amend the PR to only throw for DbConnection.DbProviderFactory.

[marklio]

Sounds like we've landed on the following principles:

• Don't annotate contrary to the implementation
• If we don't have confidence in non-null, mark it nullable.

How compatible are .NET Framework implementations with the System.Data.Common types in Core? My conclusions below are based on an assumption that such implementations fall into our "best effort" compatibility, but the ADO.net API shapes in Core match well enough to expect them to generally work.

If that's true, there are 2405 (2314 if you only count public types) NuGet package ids (each with many versions) that contain assemblies that define at least 1 type that extends DbConnection (query below for those with access). These definitely include the "official" providers, some of which have both a .NET Core and .NET Framework version (ex. ODP). Many of the others appear to be wrappers, but some are constructions that allow different things (ex. datastructures, files, etc.) to behave like databases either for testing, or seemingly as parts of components. It seems entirely possible that such things "conform poorly" to our expectations of an ADO.net provider.

If our goal is to remain compatible and consistent with all the ADO.net-ish components and their consumers that are out there using this extensibility model, my opinion would be to not introduce any behavioral changes, and simply annotate the existing behaviors. At the point where we've given up on the broader set of changes, I'm not sure I see value in the one or two places where we might save a null check (but also might introduce a null-ref exception).

I'm happy to pull some more data here if anyone thinks they need to see more. I'm also happy to help folks who would like to dig in deeper themselves.

launch query

TiTypeDefs
| where BaseType == "System.Data.Common.DbConnection"
| summarize TypeCount=count() by BinHash
| join kind=inner (TiPkgToBin) on BinHash
| summarize TypeCount=sum(TypeCount) by PkgHash
| join kind=inner (TiNuGetPackageDetails) on PkgHash
| join kind=leftouter (TiNuGetPackageDownloads) on PkgHash
| summarize TypeCount=sum(TypeCount), DownloadCount=sum(Count) by Id
| order by DownloadCount desc

[stephentoub]

Sounds like we've landed on the following principles

The principles are already enumerated in https://github.com/dotnet/runtime/blob/master/docs/coding-guidelines/api-guidelines/nullability.md.

[marklio]

Sorry, I wasn't suggesting I had come up with new principles, or that others didn't exist. I was stating the things I felt were key to guiding resolution of the specific outstanding questions here.

[roji]

Thanks for the input @marklio, that's indeed the direction we're going.

@stephentoub I've changed the PR as discussed - the only behavioral change is to make DbConnection.DbProviderFactory throw; I made it throw NotSupportedException, though NotImplementedException may also be appropriate there are no more behavioral changes. Let me know if you're good for merging or have any final comments.

[PriyaPurkayastha]

@roji can you please also create a docs breaking change issue using this template https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md
cc @gewarren who will then pick this up for publishing to breaking changes on docs.

[roji]

FYI after discussion with @ajcvickers I'll simply revert the DbConnection.DbProviderFactory change as well. The value is very low on its own, and it just doesn't seem to justify communicating on it as a breaking change etc.

[stephentoub]

Thanks, @roji. I know this didn't land exactly where you wanted. Thank you for working on it.

[roji]

No worries at all, it's also a matter of me understanding the exact bar for breaking changes, our nullability guidelines etc.

Am now wrapping up a PR for the rest of System.Data, and plan to do System.Data.Odbc, System.Data.OleDb, System.ComponentModel.Annotations after that.