Skip to content

Fix | Pass Empty List for SqlDataRecord (#2971) #3518

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Fix | Pass Empty List for SqlDataRecord (#2971) #3518

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
453e915
fix #2971
sharafabacery Jul 26, 2025
1d6ee75
fix #2971
sharafabacery Jul 30, 2025
c5b7d30
Revert "fix #2971"
sharafabacery Aug 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
Expand Down Expand Up @@ -1065,13 +1066,7 @@ public override void Cancel()
}

if (!_pendingCancel)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this change at all, especially since they seem to be completely unrelated to the focus of the PR. We don't want to be removing braces for if statements, and in this particular instance it completely upends the logic of the method.

// Do nothing if already pending.
// Before attempting actual cancel, set the _pendingCancel flag to false.
// This denotes to other thread before obtaining stateObject from the
// session pool that there is another thread wishing to cancel.
// The period in question is between entering the ExecuteAPI and obtaining
// a stateObject.
_pendingCancel = true;

TdsParserStateObject stateObj = _stateObj;
Expand Down Expand Up @@ -1763,7 +1758,6 @@ private Task InternalExecuteNonQuery(
asyncWrite,
isRetry,
methodName);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really help readability.

if (reader != null)
{
if (task != null)
Expand Down Expand Up @@ -2587,7 +2581,6 @@ long firstAttemptStart
{
completion.TrySetResult(retryTask.Result);
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change also makes no sense, and I don't see how it compiles at all. Why was this change made?

state: globalCompletion,
TaskScheduler.Default
);
Expand Down Expand Up @@ -3105,7 +3098,6 @@ private async Task<object> ExecuteScalarUntilEndAsync(SqlDataReader reader, Canc
}

/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlCommand.xml' path='docs/members[@name="SqlCommand"]/ExecuteXmlReaderAsync[@name="default"]/*'/>
public Task<XmlReader> ExecuteXmlReaderAsync() =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems completely random, unrelated to the focus of the PR, and likely doesn't compile.

ExecuteXmlReaderAsync(CancellationToken.None);

/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlCommand.xml' path='docs/members[@name="SqlCommand"]/ExecuteXmlReaderAsync[@name="CancellationToken"]/*'/>
Expand Down Expand Up @@ -4871,7 +4863,6 @@ internal SqlDataReader RunExecuteReader(
task: out unused,
usedCache: out _,
method: method);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, removing this line doesn't really aid readability and is unrelated to the PR focus.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes above could be because I enable formatting document after saving
@benrr101

Debug.Assert(unused == null, "returned task during synchronous execution");
return reader;
}
Expand Down Expand Up @@ -6192,7 +6183,8 @@ private void SetUpRPCParameters(_SqlRPC rpc, bool inSchema, SqlParameterCollecti
// Don't assume a default value exists for parameters in the case when
// the user is simply requesting schema.
// TVPs use DEFAULT and do not allow NULL, even for schema only.
if (parameter.Value == null && (!inSchema || SqlDbType.Structured == parameter.SqlDbType))
if ((parameter.Value == null || (parameter.Value is ICollection collection && collection.Count == 0)) &&
(!inSchema || parameter.SqlDbType == SqlDbType.Structured))
{
options |= TdsEnums.RPC_PARAM_DEFAULT;
}
Expand Down
7 changes: 2 additions & 5 deletions src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,10 +1444,7 @@ private void GetActualFieldsAndProperties(out List<SmiExtendedMetaData> fields,
throw SQL.NotEnoughColumnsInStructuredType();
}
}
else
{
throw SQL.IEnumerableOfSqlDataRecordHasNoRows();
}

}
finally
{
Expand Down Expand Up @@ -2044,7 +2041,7 @@ internal void Validate(int index, bool isCommandProc)
GetCoercedValue();
}

if (metaType.SqlDbType == SqlDbTypeExtensions.Vector &&
if (metaType.SqlDbType == SqlDbTypeExtensions.Vector &&
(_value == null || _value == DBNull.Value) &&
(Direction == ParameterDirection.Output || Direction == ParameterDirection.InputOutput))
{
Expand Down
107 changes: 107 additions & 0 deletions src/Microsoft.Data.SqlClient/tests/FunctionalTests/SqlCommandTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Data;
using Microsoft.Data.Sql;
using Microsoft.Data.SqlClient.Server;
using Xunit;

namespace Microsoft.Data.SqlClient.Tests
Expand Down Expand Up @@ -518,5 +520,110 @@ public void ParameterCollectionTest()
cmd.Parameters.Remove(cmd.Parameters[0]);
}
}

#region fixing "Change SqlParameter to allow empty IEnumerable<SqlDataRecord>? #2971"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to do about these tests. I think someone with a bit more experience in the test area could provide more concrete feedback. @mdaigle ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All changes above could be because I enable formatting document after saving

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benrr101

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there any test need to be done i will do it?

public const string DbName = "TVPTestDb";
public const string ServerConnection = @"Server=.;Integrated Security=true;Encrypt=false;";
public static string DbConnection => ServerConnection + $"Initial Catalog={DbName};";
private static class SqlDataRecordTestCases
{
public static IEnumerable<object[]> TestSqlDataRecordParameters => new List<object[]>
{
new object[] { null },
new object[] { new List<SqlDataRecord>() } // empty list, and the issue it throw an Arrgument Error Exception
};
}

private static void CreateDatabaseAndSchema()
{
string createDb = $@"
IF DB_ID('{DbName}') IS NULL
CREATE DATABASE {DbName};";

string createSchema = @"
USE TVPTestDb;

IF NOT EXISTS (SELECT * FROM sys.types WHERE is_table_type = 1 AND name = 'MySimpleTableType')
BEGIN
CREATE TYPE dbo.MySimpleTableType AS TABLE
(
Id INT,
Name NVARCHAR(100)
);
END;

IF OBJECT_ID('dbo.TargetTable', 'U') IS NULL
BEGIN
CREATE TABLE dbo.TargetTable
(
Id INT PRIMARY KEY,
Name NVARCHAR(100)
);
END;

IF OBJECT_ID('dbo.InsertFromTVP', 'P') IS NULL
BEGIN
EXEC('
CREATE PROCEDURE dbo.InsertFromTVP
@MyTVP dbo.MySimpleTableType READONLY
AS
BEGIN
SET NOCOUNT ON;
INSERT INTO dbo.TargetTable (Id, Name)
SELECT Id, Name FROM @MyTVP;
END
');
END;";

using var conn = new SqlConnection(ServerConnection);
conn.Open();
new SqlCommand(createDb, conn).ExecuteNonQuery();

using var connDb = new SqlConnection(DbConnection);
connDb.Open();
new SqlCommand(createSchema, connDb).ExecuteNonQuery();
}

private static void DeleteDatabase()
{
string dropDb = $@"
IF DB_ID('{DbName}') IS NOT NULL
BEGIN
ALTER DATABASE {DbName} SET SINGLE_USER WITH ROLLBACK IMMEDIATE;
DROP DATABASE {DbName};
END;";
using var conn = new SqlConnection(ServerConnection);
conn.Open();
new SqlCommand(dropDb, conn).ExecuteNonQuery();
}
private int perpare_SqlRecord_tests(List<SqlDataRecord> records = null)
{
SqlConnection cn = new SqlConnection(DbConnection);

SqlCommand cmd;
// Text, with parameters
cmd = new SqlCommand("dbo.InsertFromTVP", cn);
cmd.CommandType = CommandType.StoredProcedure;

var paramEmpty = cmd.Parameters.AddWithValue("@MyTVP", records);
paramEmpty.SqlDbType = SqlDbType.Structured;
paramEmpty.TypeName = "dbo.MySimpleTableType";
cn.Open();
return cmd.ExecuteNonQuery();

}
[Theory]
[MemberData(nameof(SqlDataRecordTestCases.TestSqlDataRecordParameters), MemberType = typeof(SqlDataRecordTestCases))]
public void SqlDataRecord_TABLE_deafult(List<SqlDataRecord> parameters)
{
CreateDatabaseAndSchema();
int rowAffects = perpare_SqlRecord_tests(parameters);
DeleteDatabase();
Assert.Equal(-1, rowAffects);

}
#endregion


}
}
Loading