Allow passing null for SqlParameter value

[mburbea]

Is your feature request related to a problem? Please describe.

Currently, if you pass null to the Value in a SqlParameter constructor or use the AddWithValue method, you receive a rather cryptic exception message::
e.g.

using var conn = new SqlConnection(...);
await conn.OpenAsync();
using var cmd = new SqlCommand("select @", conn);
cmd.Parameters.Add(new("@", null) { DbType = DbType.Int32 });
var res = await cmd.ExecuteScalarAsync();

Microsoft.Data.SqlClient.SqlException : The parameterized query '(@ int)select @' expects the parameter '@', which was not supplied.

The message is entirely unclear, and makes you have to google it, only to discover that the correct resolution of this problem is to replace null with DbNull.Value.

There are many, many threads about this on stackoverflow and various forums.

Describe the solution you'd like

When you create a parameter with null, it internally stores down DbNull.Value.

This is technically a breaking change, but I'm not sure who actually benefits from this obtuse behavior where null is an exception rather than doing the logical thing.

Describe alternatives you've considered

Additional context

[Wraith2]

I'm not sure who actually benefits from this obtuse behavior where null is an exception rather than doing the logical thing.

You, the developer, do. By extension your users do.

Language null and sql null aren't conceptually the same thing and they don't directly map onto one another so you are required to indicate what should happen if a parameter is null:

1. is it the result of a progamming error in your .net code so that a reference type (usually string ime) which shouldn't have been a null has somehow made it to the insert stage without a value
2. is it an appropriate value for the parameter? indicating you want to insert NULL
   Without you making a specific choice it can be either but if it's the first you're going to get an error which will consume io, and go slowly as well as possibly rolling back any transaction you have open. All around expensive.

This situation would be different if generics and nullable type annotations had existed at the time that the api was designed, there would probably be no need for DBNull at all and mapping would be easier.

[mburbea]

I don't buy it. At minimum the error could at least say something like...

If you meant to pass null, use DBNull.Value

Of course, this goes back to the problem of, if the library knew what I was doing why is it making me work for it?
This is a sqlserver error the same one you get with sp_executeSql without supplying a parameter:

exec sp_executesql N'select @',N'@ int'

The parameterized query '(@ int)select @' expects the parameter '@', which was not supplied.

So it appears that the driver sends the metadata, but then doesn't bother sending the actual value. The error sucks from sql server, but here I can at least see that oh hey, I never did specify a value for @, and if I truly never want to avoid sending a value, I can specify in the metadata declaration that @ int=null or whatever default I want.

I almost always end up using a tool like Dapper, or efcore which will magically wrap away this behavior. e.g.
await conn.ExecuteScalarAsync<int?>("select @foo",new {foo=new int?()}), will do the right thing and not throw an exception at me. I generally forget this happens when I try to not use an abstraction layer. But there are reasons to avoid using an abstraction layer, and the behavior shouldn't be so awful for working with the raw database operations.

1. is it the result of a progamming error in your .net code so that a reference type (usually string ime) which shouldn't have been a null has somehow made it to the insert stage without a value

In my example, and my real code that I needed to do this I was using a nullable int.

is it an appropriate value for the parameter? indicating you want to insert NULL
Without you making a specific choice it can be either but if it's the first you're going to get an error which will consume io, and go slowly as well as possibly rolling back any transaction you have open. All around expensive.

Null is an entirely valid for sql queries and for sql databases, so the questioning is disingenuous. E.g. you very well might want to leave address_line2.
e.g.

insert into dbo.address(user_id,address_line,address_line2,city,state,zip)
values(@userId,@addressLine,@addressLine2,@city,@state,@zip);

And similarly, you may want to query data for keys that contains null. Like finding all running instances of a particular job type for a date that have not completed yet.

select batch_id
from dbo.batch_run 
where exists (
    select job_type,as_of_date,completion_time
    intersect
    select @jobType,@asOfDate,@completionTime
)

I am unsure what a more recently written driver like npgsql does, but I'm going to guess that sending null with a dbtype of int isn't an error with an obtuse message.

[Wraith2]

Of course, this goes back to the problem of, if the library knew what I was doing why is it making me work for it?

It doesn't. You didn't tell it. SqlParameter.Value is of object type and you can put anything you want in there. You want to pass in nothing and have it infer that language null means database null but that may not be the case because it could equally be an error of input data. You, the programmer, are supposed to give it unambiguous instructions which is what DBNull is for. DBNull is a value which indicates to the library that yes you really do mean to send null to the database.

I am unsure what a more recently written driver like npgsql does,

I'm pretty sure that because it uses generically typed parameter classes it is able to make safer assumptions and if you provide a language null to a field which is declared as being able to hold a null it will send it as database null. SqlClient uses object for the value container because it predates generics and so DBNull is needed as a sentinel value. /cc @roji

[roji]

Npgsql does have a generic parameter API (e.g. to avoid boxing), but this isn't standard ADO.NET (see dotnet/runtime#17446), and Npgsql also supports the standard non-generic API. One tricky point with the generic API, is that it's not possible to use it to write nulls for value types.

I've never quite investigated the entire question of ADO.NET, .NET null and database null, or how feasible it would be to allow .NET null to be interpreted as database null. IIRC the difference is significant in some places, e.g. when inserting a new row, .NET null means "use the column's default value" whereas database null means database null (but I'm not 100% sure on this anymore).

I do know that it's not something we can just consider doing without a lot of careful analysis. IMO this is also something that needs to be considered from a cross-database perspective, and not as a pure SqlClient change.

[mburbea]

I guess the question then if I use a standard DbParameter in npgsql, does it consider null to be DbNull or does it throw an obscure message?

It seems most client abstraction libraries like EFCore and Dapper will hide the difference for the user when using parameterized queries.

Now I suppose in the case of a store procedure call, a parameter that has no value will invoke the default. (analogous to doing the following in sql.

exec sp_executesql N'select @',N'@ int=null'
-- is equivalent to the following
exec sp_executesql N'select @',N'@ int=null',@=default

Of course, if this is the desired nonsense behavior than perhaps it could be solved with a boolean on SqlParameter like IsDefault or whatever.

The crux I keep hearing is that sqlclient was written in .net 1.0 times, and all of it's cruft and unwieldiness is an unfortunate consequence of its legacy.

[cheenamalhotra]

Hi @mburbea

Yes I agree driver is generating: exec sp_executesql N'select @',N'@ int',@=default here.

I tried with null and that works so maybe it can be fixed:
exec sp_executesql N'select @',N'@ int',@=null

[roji]

@mburbea an important point here, is that if you change an ADO.NET driver to suddenly treat language null as database null (instead of default), you're potentially breaking a whole lot of programs out there which rely on this behavior (which has been around since forever).

Of course, if there's a reliable way to detect the error situation and error with a better message, that's always a good idea (but I'm not sure that's the case here).

[Wraith2]

Just out of interest, with generic parameter types if you add new Parameter<int>() to a command is the default value for the type used if the parameter value isn't directly set after it's created?

[mburbea]

I'm going to say that the behavior is safe to treat null as DbNull when the CommandType property is set to
CommandType.Text (the default) for SqlParameters.

I am also not suggesting to make this change in the DataReader or in ExecuteScalar. I still expect those to unfortunately require a call to IsDbNull{Async}

Since procedures, may want this behavior where unsupplied parameters are treated as default, and I have no idea what the CommandType.TableDirect (edit: docs seem to suggest this only works for OLEDB) option does for parameters, we can safely make this change.

I suppose if anyone was testing for this exception, they would break. Like I acknowledged earlier, it's a breaking change, but outside of testing for an exception, I can't imagine anyone relying on this behavior.

[Wraith2]

Like I acknowledged earlier, it's a breaking change, but outside of testing for an exception, I can't imagine anyone relying on this behavior.

The problem with being part of the runtime for so long is that I can be certain that someone somewhere has relied on the current behaviour and a change will break it and absolutely ruin some poorly paid maintenance developers day/week/month. Much like the BCL behavioural changes are extremely hard to justify in old apis, subtle behavioural changes in this library even though it's optional are hard to justify. The upgrade path from System to Microsoft versions needs to be smooth to allow adoption in LOB software.

Now, when we get generic parameters like postgres has that'll be all new surface area with no need to use DBNull and then we can do things more appropriate for the language and runtime we have today.

[mburbea]

The runtime has already made some breaking changes to allow behavior that has always thrown an exception.
e.g.

var dict = new Dictionary<int, int> { [0] = 0, [1] = 1 };
foreach (var kvp in dict)
{
     if (kvp.Key == 0) dict.Remove(kvp.Key);
}

In .net framework 4.8 this throws an exception ( Collection was modified; enumeration operation may not execute). In .net core 3/.net 5 it does not. Again, since this is only an exception, and it is very unlikely to be caught in anything but a unit test testing for an exception they still made the change.

I think that being so cautious means that the library can never evolve and we are simply stuck with crappy behavior that nobody uses, and basically everyone is required to adopt an abstraction layer.

[cheenamalhotra]

FYI (I guess this is known workaround already)
This issue is solved by passing parameter as below:

cmd.Parameters.Add(new SqlParameter("@", DBNull.Value) { DbType = DbType.Int32 });

DbType is optional.

Server receives:
exec sp_executesql N'select @',N'@ int',@=NULL

[Wraith2]

The runtime has already made some breaking changes to allow behavior that has always thrown an exception.
e.g.

I know, I made that change 😁 dotnet/coreclr#18854

I'm also sure that it will have broken some code being ported from netfx. going from netcore to netfx is a porting process and that particular break was documented and called out in release notes iirc.

Going from System to Microsoft versions of this library really needs not to contain breaking changes if at all possible. I'm not saying it won't (I know cancellation exceptions are going to change soon) but the bar for approval of those changes is pretty high.

In context I don't think there is a lot that can be done with the legacy parameter mechanism without high risk of breaking someone's code, even if they wrote bad code I still don't want to break it if I can help it.

[mburbea]

@Wraith2, ha didn't realize you made that change. That's pretty funny as that was the first breaking change that came to my mind. (and a welcome one).
@cheenamalhotra, I'm aware that works.

Edit: I guess like #255, we worst case can introduce a switch and default it to false in .net framework and true in .netcore side?

Also nit:
If I don't specify dbtype the inferred type by the library is nvarchar(4000), where as sqlservers inferred default type is int. Like issue #255, I can assume this incongruity is in the hell freezing over category before this ever gets fixed.

[Wraith2]

#255 I can probably fix quite easily because of the suggested appcontext switch it just wasn't on my radar.

[roji]

@mburbea

I'm going to say that the behavior is safe to treat null as DbNull when the CommandType property is set to
CommandType.Text (the default) for SqlParameters.

I am also not suggesting to make this change in the DataReader or in ExecuteScalar. I still expect those to unfortunately require a call to IsDbNull{Async}

I don't see how CommandType is related, or how it's safe. Unless I'm missing something, if you create a SqlCommand today with CommandType.Text, set text to INSERT INTO table (column) VALUES (@p) and have null as the parameter value, you will get a row with the default value defined on the column. The change you propose will insert database NULL instead, which is a pretty major breaking change.

IsDbNull{Async} is for the reading side, which is a different discussion - up to now we've been discussing the writing side.

The runtime has already made some breaking changes to allow behavior that has always thrown an exception.

Changing behavior to not throw is by definition not considered a breaking change. What you're proposing would change the meaning of existing, working code to do something entirely different.

To be honest, I don't really see the point of this discussion... The current way things work don't block anyone - as @cheenamalhotra mentioned above, users simply need to pass DBNull.Value instead of null; regardless of what we think of it, it is what it is and that's how ADO.NET works. If users accidentally use null in the wrong way, it's unfortunate that they get a cryptic message, but that certainly doesn't seem worth a potentially huge breaking change. That isn't to say things shouldn't be improved if they can be, but it's better to concentrate on more valuable things.

[mburbea]

@roji, the CommandType matters because default does not work as you described in this case.

create table dbo.t(c nvarchar(4000) default 'horse');

If I try this...

using (var cmd = new SqlCommand("insert into dbo.t(c)values(@)", conn))
{
    cmd.Parameters.AddWithValue("@", null);
    await cmd.ExecuteNonQueryAsync();
}

I get that cryptic exception

Microsoft.Data.SqlClient.SqlException (0x80131904): The parameterized query '(@ nvarchar(4000))insert into dbo.t(c)values(@)' expects the parameter '@', which was not supplied.

However, if I use a store procedure which does have a default value supplied for a parameter like::

create or alter procedure dbo.p(@ nvarchar(4000)='horse') 
as 
insert into dbo.t(c) values(@);

Then the following works as you describe (note that I have to set the mode to procedure).

 using (var cmd = new SqlCommand("dbo.p", conn) { CommandType = CommandType.StoredProcedure })
{
    cmd.Parameters.AddWithValue("@", null);
    await cmd.ExecuteNonQueryAsync();
}

This is why I mentioned to keep the behavior of null meaning default for procedures. For text where it can ONLY be an error, I think we can unblock the change safely.

I say it's an error and an odd design because lots of people run into this problem. (Search for the exception on stackoverflow if you think i'm alone (~13,000 results)). I disagree with the viewpoint that it doesn't block people so it shouldn't be fixed. Ultimately, many libraries like efcore, dapper, and now my internal layer now has code akin to new($"@{paramName}", value ?? DbNull.Value). I don't think just because the workaround is simple, doesn't mean we shouldn't correct the problem.

[Wraith2]

I can't think of any case where calling AddWithValue with a null value is ever the right thing to have done and I think I'd have said it should throw an exception. We can't change that either.

If the exception message needs to be changed to better inform people about the problem then that can probably be done but changing the behaviour will cause a massive compatibility problem.

[mburbea]

@Wraith2, I'm still trying to find this case where the behavior works as you describe such that it is a breaking change. How does this impact compatability?

If we make the change only occur when CommandType = CommandType.Text then the only people who would be impacted are those who were intentionally trying to get this exception.

[cheenamalhotra]

Also to add, when trying to mock this in SSMS, it seems like setting default keyword as value to a SQL Variable is just not allowed, and that's how we could pass parameter in SSMS queries, if I'm not wrong. So if driver sets this as value, in an RPC call, it could be possible it's internally doing the same thing and that's why does not accept it and fails?

exec sp_executesql N'insert into t1 values (@1, @2)',N'@1 nvarchar(4000),@2 int',@1=default,@2=NULL
-- throws: The parameterized query '(@ nvarchar(4000))select @' expects the parameter '@', which was not supplied.

It's also different than passing value directly in the query in a non-parameterized way or via another procedure internally (like explained by @mburbea above), and that's out of context for null parameter value.

The stored procedure calls exec dbo.p @=default on server, but leads to inserting "Horse" in the table as the parameter value. So we would want to retain that behavior for RPC command types to ensure it does not insert null, but we could consider this change for CommandType.Text as that never works.

exec sp_executesql N'dbo.p',N'@ nvarchar(4000)',@=default
-- The parameterized query '(@ nvarchar(4000))dbo.p' expects the parameter '@', which was not supplied.

[roji]

@mburbea

• FWIW I agree it's odd/inconsistent that language null is accepted and interpreted as default with CommandType.StoredProcedure, but not with CommandType.CommandText.
• However, treating language null as database null for CommandType.Text would introduce a subtle change between the two; a command converted from StoredProcedure to CommandText would suddenly start sending database NULL instead of default, whereas today it at least throws - which seems much better (users are made aware that things won't work in the same way as before).
• The discussion above is only based on the observed behavior of SqlClient. Have we checked what the Odbc/Ole drivers do? How about other database drivers such as MySQL, Oracle, PostgreSQL? It's easy enough to propose a behavior change for SqlClient, but .NET has a database abstraction (ADO.NET), and we want to keep behavior consistent across drivers.
• There are already other places in ADO.NET where language null and database NULL are distinct - notably on the read side. ExecuteScalar, for example, returns DBNull.Value for database null, and language null if no resultset was provided; this proposal would introduce more inconsistency.

I feel like I'm repeating myself, but I still am not seeing the big problem here. Yes - the API might not be ideal, and new users to ADO.NET are likely to bump into this. However, the error message seems very explicit to me: The parameterized query '(@p nvarchar(4000))INSERT INTO foo (bar) VALUES (@p)' expects the parameter '@p', which was not supplied. - the specific parameter name is mentioned, and the user has all the info to successfully understand what they need to do. The workaround is extremely simple (replace null with DBNull.Value), and after bumping into this once, users aren't likely to do this again.

To summarize, I really am not against improving things in general, and even think that the occasional breaking change makes sense if it serves a very valuable purpose. IHMO that simply isn't the case here - the value of this change seems very small weighed against the larger potential impact.

[mburbea]

@roji, to answer a few of your points::
I have not tested in other providers. My suggestion was to this repository, SqlClient. I imagine you can answer the postgres question better than anyone else.
And I don't think it logically makes sense to change the read side of the equation. Nor do I see the problem. The sqlclient is already inconsistent in how it treats language null vs db null. If you use a DataTable as a source for a SqlBulkCopy, it will treat language null as db null. Simmilarly, a tvp using a DataTable source will also treat null values as dbNull. (Using DataTable.CreateReader() for both also works without it vomiting).

var dt = new DataTable { Columns = { { "C", typeof(string) } }, Rows = { { new object[] { null } } } };
using var bulkCopy = new SqlBulkCopy(conn, SqlBulkCopyOptions.KeepNulls, null){DestinationTableName = "dbo.t"};
await bulkCopy.WriteToServerAsync(dt);
using (var cmd = new SqlCommand(@"insert into t select * from @",conn))
{
    cmd.Parameters.Add(new("@", dt) { SqlDbType = SqlDbType.Structured, TypeName = "dbo.tvp" });
    await cmd.ExecuteNonQueryAsync();
}

I think the message is unclear. I supplied the parameter @. It makes it seem like I forgot a parameter when calling the proc. The message I think at minimum in text mode, should say something akin to "If you meant to use null send DbNull.Value"

[roji]

I do agree that adding some text to help guide users in the direction of DBNull.Value makes sense, along the lines of the message you propose. But I'd be careful about doing a behavioral change over this point without some thorough research into the other providers and general null handling in ADO.NET.

[ScottRFrost]

FYI this bug still exists as of Jan 2023.

For anyone who ended up on this GitHub issue from a web search like I did, I solved it by writing a little wrapper:

public static async AdHocNonQuery(string query, params KeyValuePair<string, object>[] parameters)
{
    using var connection = new SqlConnection(YourConnectionStringHere);
    connection.Open();
    using var sqlCommand = new SqlCommand(query, connection)
    {
        CommandType = CommandType.Text
    };
    foreach (var parameter in parameters)
    {
        if(parameter.Value == null)
        {
            sqlCommand.Parameters.AddWithValue(parameter.Key, DBNull.Value);
        }
        else
        {
            sqlCommand.Parameters.AddWithValue(parameter.Key, parameter.Value);
        }
    }
    await sqlCommand.ExecuteNonQueryAsync().ConfigureAwait(false);
}

private static KeyValuePair<string, object> Param(string paramName, object paramValue)
{
    return new KeyValuePair<string, object>(paramName, paramValue);
}

Then you can call it like so:

await AdHocNonQuery("UPDATE [User] SET [DateOfBirth]=@DOB WHERE [UserID]=@UserID",
 Param("@DOB", null), Param("@UserID", 42));

[roji]

@ScottRFrost this is not a bug; this is how the ADO.NET API was designed to work.

Of course, that doesn't mean it can't be changed at some point.

[jbuhacoff]

@Wraith2 wrote:

You want to pass in nothing and have it infer that language null means database null but that may not be the case because it could equally be an error of input data.

Why are we making this distinction between "language null" and "database null", but not between:

• "language int" and "database int"
• "language string" and "database string (varchar/text/etc)"
• "language bool" and "database bool (bit)"
• "language datetime" and "database datetime"
• and so on

Developers can pass in all these other values as-is. If a language didn't have a null token, then something would be required to indicate that the developer intends to pass a null instead of, say, an empty string. But when a language has a null token, as is the case in C#, "language null" and "database null" are the same thing and they mean "no value" for the variable or column.

The error message saying a parameter wasn't provided sounds like it wasn't provided at all, not with any value including the non-value null, which in "language" terms should mean AddWithValue wasn't even called for that parameter.

I understand the point about not changing the API and breaking things for people, so I have these suggestions to contribute:

1. If AddWithValue requires a non-null value, it should be throwing an exception there and not even making a call to the database. The exception message can then make it clear that a value is required and if the developer intends to send null, the developer should pass DBNull.Value. An exception is already being thrown in this case but this one would be more useful.

2. Add a new method AddWithNull("@ParameterName") which would be a thin wrapper around AddWithValue("@ParameterName", DBNull.Value) and hopefully will show up for new developers with auto-complete and help them avoid this issue altogether.

[Wraith2]

If AddWithValue requires a non-null value, it should be throwing an exception there and not even making a call to the database.

It is legal and common to call AddWithValue to create the parameter and then later, often in a loop, set the value to a non-null. value. This is because it's often awkward to call a different parameter ctor or Add overload, AddWithValue is quick common and easy. Changing to an exception now would break code working code at runtime, not a fun experience for users.

Add a new method AddWithNull("@ParameterName") which would be a thin wrapper around AddWithValue("@ParameterName", DBNull.Value) and hopefully will show up for new developers with auto-complete and help them avoid this issue altogether.

This could work. It's something that should probably be added to the DbCommand with a default implementation that can be overloaded by each provider rather than something we do specifically in SqlClient. What do you think @roji ?