Repository navigation
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request initializes the ParameterName property of SpannerParameter to string.Empty by default and adds a corresponding unit test. The feedback highlights that setting ParameterName to null should also return string.Empty to comply with the ADO.NET specification, and suggests using a backing field to handle null coercion.
|
|
||
| /// <inheritdoc /> | ||
| public override string ParameterName { get; set; } | ||
| public override string ParameterName { get; set; } = string.Empty; |
There was a problem hiding this comment.
According to the ADO.NET specification for DbParameter.ParameterName, setting the property to null should also result in it returning an empty string (string.Empty). With an auto-implemented property, setting ParameterName to null will store and return null, which violates this contract and could lead to NullReferenceExceptions elsewhere. Consider using a backing field to coerce null values to string.Empty.
private string _parameterName = string.Empty;
public override string ParameterName
{
get => _parameterName;
set => _parameterName = value ?? string.Empty;
}6f9a0f7 to
2051964
Compare
Summary
Initialize
SpannerParameter.ParameterNametostring.Emptyand normalize null assignments back to the empty string. Add regression coverage for both cases.Resolves #16043.
Microsoft contract
Microsoft documents the default value of
DbParameter.ParameterNameas an empty string: https://learn.microsoft.com/dotnet/api/system.data.common.dbparameter.parameternameCompatibility
This changes the observable value from null to empty for a new parameter and when null is assigned. Explicit non-null assignments are unchanged. Consumers that distinguish null from empty may observe the change; empty is the documented ADO.NET default and avoids null propagation through provider code.
Validation
dotnet test apis/Google.Cloud.Spanner.Data/Google.Cloud.Spanner.Data.Tests/Google.Cloud.Spanner.Data.Tests.csproj --no-restore -f net8.0 -p:NoWarn=CS8981%3BxUnit2004%3BxUnit2013— 2,833 passed, 0 failed.Discovery
Found using pengdows.AdoNetApiTest.conformance.