diff --git a/src/libraries/Common/src/System/Data/Common/DbConnectionOptions.Common.cs b/src/libraries/Common/src/System/Data/Common/DbConnectionOptions.Common.cs index da10c9fe31f694..647eebf6113fa2 100644 --- a/src/libraries/Common/src/System/Data/Common/DbConnectionOptions.Common.cs +++ b/src/libraries/Common/src/System/Data/Common/DbConnectionOptions.Common.cs @@ -114,13 +114,12 @@ private string UsersConnectionString(bool hidePassword, bool forceHidePassword) public bool ConvertValueToBoolean(string keyName, bool defaultValue) { string? value; - // TODO: Is it possible for _parsetable to contain a null value here? If so there's a bug here, investigate. return _parsetable.TryGetValue(keyName, out value) ? - ConvertValueToBooleanInternal(keyName, value!) : + ConvertValueToBooleanInternal(keyName, value) : defaultValue; } - internal static bool ConvertValueToBooleanInternal(string keyName, string stringValue) + internal static bool ConvertValueToBooleanInternal(string keyName, string? stringValue) { if (CompareInsensitiveInvariant(stringValue, "true") || CompareInsensitiveInvariant(stringValue, "yes")) return true; @@ -128,7 +127,7 @@ internal static bool ConvertValueToBooleanInternal(string keyName, string string return false; else { - string tmp = stringValue.Trim(); // Remove leading & trailing whitespace. + string? tmp = stringValue?.Trim(); // Remove leading & trailing whitespace. if (CompareInsensitiveInvariant(tmp, "true") || CompareInsensitiveInvariant(tmp, "yes")) return true; else if (CompareInsensitiveInvariant(tmp, "false") || CompareInsensitiveInvariant(tmp, "no")) @@ -140,7 +139,7 @@ internal static bool ConvertValueToBooleanInternal(string keyName, string string } } - private static bool CompareInsensitiveInvariant(string strvalue, string strconst) => + private static bool CompareInsensitiveInvariant(string? strvalue, string strconst) => (0 == StringComparer.OrdinalIgnoreCase.Compare(strvalue, strconst)); [System.Diagnostics.Conditional("DEBUG")] diff --git a/src/libraries/System.Data.Common/src/System/Data/DataTable.cs b/src/libraries/System.Data.Common/src/System/Data/DataTable.cs index f1c6e04e367646..279cfcbdba01a2 100644 --- a/src/libraries/System.Data.Common/src/System/Data/DataTable.cs +++ b/src/libraries/System.Data.Common/src/System/Data/DataTable.cs @@ -7202,8 +7202,13 @@ internal void EvaluateDependentExpressions(List? columns, DataRow ro for (int i = 0; i < count; i++) { DataColumn dc = columns[i]; + if (dc.DataExpression == null) + { + continue; + } + // if this column is NOT in my table or it is in the table and is not a local aggregate (self refs) - if (dc.Table != this || (dc.DataExpression != null && !dc.DataExpression.HasLocalAggregate())) + if (dc.Table != this || !dc.DataExpression.HasLocalAggregate()) { DataRowVersion foreignVer = (version == DataRowVersion.Proposed) ? DataRowVersion.Default : version; @@ -7225,9 +7230,7 @@ internal void EvaluateDependentExpressions(List? columns, DataRow ro if (cachedRow != null && ((cachedRow.RowState != DataRowState.Deleted) && (version != DataRowVersion.Original || cachedRow._oldRecord != -1))) { - // if deleted GetRecordFromVersion will throw - // TODO: Possible bug, dc.DataExpression may be null - object newValue = dc.DataExpression!.Evaluate(cachedRow, foreignVer); + object newValue = dc.DataExpression.Evaluate(cachedRow, foreignVer); SilentlySetValue(cachedRow, dc, foreignVer, newValue); } } @@ -7257,9 +7260,7 @@ internal void EvaluateDependentExpressions(List? columns, DataRow ro if (parentRow != null && ((parentRow.RowState != DataRowState.Deleted) && (version != DataRowVersion.Original || parentRow._oldRecord != -1))) { - // if deleted GetRecordFromVersion will throw - // TODO: Possible bug, dc.DataExpression may be null - object newValue = dc.DataExpression!.Evaluate(parentRow, foreignVer); + object newValue = dc.DataExpression.Evaluate(parentRow, foreignVer); SilentlySetValue(parentRow, dc, foreignVer, newValue); } } @@ -7289,9 +7290,7 @@ internal void EvaluateDependentExpressions(List? columns, DataRow ro if (childRow != null && ((childRow.RowState != DataRowState.Deleted) && (version != DataRowVersion.Original || childRow._oldRecord != -1))) { - // if deleted GetRecordFromVersion will throw - // TODO: Possible bug, dc.DataExpression may be null - object newValue = dc.DataExpression!.Evaluate(childRow, foreignVer); + object newValue = dc.DataExpression.Evaluate(childRow, foreignVer); SilentlySetValue(childRow, dc, foreignVer, newValue); } } diff --git a/src/libraries/System.Data.Common/src/System/Data/Filter/DataExpression.cs b/src/libraries/System.Data.Common/src/System/Data/Filter/DataExpression.cs index 3c97fb60228937..de96a6a02c3a9b 100644 --- a/src/libraries/System.Data.Common/src/System/Data/Filter/DataExpression.cs +++ b/src/libraries/System.Data.Common/src/System/Data/Filter/DataExpression.cs @@ -138,10 +138,9 @@ internal object Evaluate(DataRow? row, DataRowVersion version) // we need to convert the return value to the column.Type; try { - if (StorageType.Object != _storageType) + if (_dataType != null && StorageType.Object != _storageType) { - // TODO: _dataType can be null, probably a bug - result = SqlConvert.ChangeType2(result, _storageType, _dataType!, _table!.FormatProvider); + result = SqlConvert.ChangeType2(result, _storageType, _dataType, _table == null ? System.Globalization.CultureInfo.CurrentCulture : _table.FormatProvider); } } catch (Exception e) when (ADP.IsCatchableExceptionType(e)) diff --git a/src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs b/src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs index 656c1b2c3d7698..94fd026b138d8e 100644 --- a/src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs +++ b/src/libraries/System.Data.Common/tests/System/Data/Common/DbConnectionStringBuilderTest.cs @@ -2281,5 +2281,19 @@ private void AssertValueTest(string[][] tests1, string[][] tests2, bool useOdbc) } } } + + [Fact] + public void DbConnectionOptions_EmptyBooleanValue_ThrowsArgumentException() + { + Type type = typeof(System.Data.Common.DbConnectionStringBuilder).Assembly.GetType("System.Data.Common.DbConnectionOptions"); + Assert.NotNull(type); + + object options = Activator.CreateInstance(type, new object[] { "persist security info=;", null, false }); + System.Reflection.MethodInfo convertMethod = type.GetMethod("ConvertValueToBoolean", new Type[] { typeof(string), typeof(bool) }); + Assert.NotNull(convertMethod); + + var ex = Assert.Throws(() => convertMethod.Invoke(options, new object[] { "persist security info", false })); + Assert.IsType(ex.InnerException); + } } } diff --git a/src/libraries/System.Data.Common/tests/System/Data/DataTableTest.cs b/src/libraries/System.Data.Common/tests/System/Data/DataTableTest.cs index 399b2265cb2c04..0e1bc5c2f7efe2 100644 --- a/src/libraries/System.Data.Common/tests/System/Data/DataTableTest.cs +++ b/src/libraries/System.Data.Common/tests/System/Data/DataTableTest.cs @@ -3720,5 +3720,44 @@ private static void Select(DataTable tbl) DataRow[] rows = tbl.Select(filter); Assert.Equal(1, rows.Length); } + + [Fact] + public void EvaluateDependentExpressions_NullDataExpression_DoesNotThrow() + { + DataTable table = new DataTable(); + table.Columns.Add("Id", typeof(int)); + table.Columns.Add("Value", typeof(int)); + DataColumn computedCol = table.Columns.Add("Computed", typeof(int), "Id + Value"); + + table.Rows.Add(1, 10); + table.Rows.Add(2, 20); + + // Nulling out the expression + computedCol.Expression = null; + + // Triggering data change. + // The underlying EvaluateDependentExpressions method now gracefully handles null expressions. + var exception = Record.Exception(() => table.Rows[0]["Id"] = 5); + Assert.Null(exception); + } + + [Fact] + public void DataExpression_Evaluate_NullDataType_DoesNotThrowArgumentNullException() + { + DataTable table = new DataTable(); + table.Columns.Add("A", typeof(int)); + table.Rows.Add(5); + + Type expressionType = typeof(DataTable).Assembly.GetType("System.Data.DataExpression"); + Assert.NotNull(expressionType); + + object expr = Activator.CreateInstance(expressionType!, System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Public, null, new object[] { table, "A + 5", null }, null); + + System.Reflection.MethodInfo evaluateMethod = expressionType.GetMethod("Evaluate", new Type[] { typeof(DataRow), typeof(DataRowVersion) }); + Assert.NotNull(evaluateMethod); + object result = evaluateMethod!.Invoke(expr, new object[] { table.Rows[0], DataRowVersion.Default }); + + Assert.Equal(10, Convert.ToInt32(result)); + } } }