Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Remove System.Collection.NonGeneric from SqlClient contract #6527

Merged
merged 2 commits into from
Mar 1, 2016
Merged

Remove System.Collection.NonGeneric from SqlClient contract #6527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9a89b74
SqlClient contract: Remove dependency on System.Collections.NonGeneric
justinvp Feb 27, 2016
fb5c7f2
SqlBulkCopyColumnMappingCollection: Remove dependency on CollectionBase
justinvp Feb 28, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/System.Data.SqlClient/ref/System.Data.SqlClient.Manual.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,25 @@

namespace System.Data.SqlClient
{
// Declaring members from stripped base class CollectionBase
public sealed partial class SqlBulkCopyColumnMappingCollection : System.Collections.ICollection, System.Collections.IEnumerable, System.Collections.IList
Copy link
Contributor

Choose a reason for hiding this comment

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

With CollectionBase the property int Capacity was available as well. With the implementation of the interfaces, the property cannot be used by the clients.
This will break any clients using the property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is Capacity a commonly used property on SqlBulkCopyColumnMappingCollection? None of the other collections in CoreFX that implement CollectionBase in the full framework include it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another thing to think about: if SqlBulkCopyColumnMappingCollection were a new API being added to CoreFX today, it'd just derive from System.Collections.ObjectModel.Collection<T> which does not have a public Capacity property.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure about the usage statistics. I was talking strictly in terms of backward compatibility and wanted to know your thoughts on Capacity property.
I know that the APIs have been removed from CoreFX to streamline the surface area and to better programming models.

{
public int Count { get { return default(int); } }
bool System.Collections.ICollection.IsSynchronized { get { return default(bool); } }
object System.Collections.ICollection.SyncRoot { get { return default(object); } }
bool System.Collections.IList.IsFixedSize { get { return default(bool); } }
bool System.Collections.IList.IsReadOnly { get { return default(bool); } }
object System.Collections.IList.this[int index] { get { return default(object); } set { } }
public void Clear() { }
public System.Collections.IEnumerator GetEnumerator() { return default(System.Collections.IEnumerator); }
public void RemoveAt(int index) { }
void System.Collections.ICollection.CopyTo(System.Array array, int index) { }
int System.Collections.IList.Add(object value) { return default(int); }
bool System.Collections.IList.Contains(object value) { return default(bool); }
int System.Collections.IList.IndexOf(object value) { return default(int); }
void System.Collections.IList.Insert(int index, object value) { }
void System.Collections.IList.Remove(object value) { }
}
public sealed partial class SqlCommand : System.Data.Common.DbCommand
{
// SqlCommand expects IDisposable methods to be implemented via System.ComponentModel.Component, which it no longer inherits from
Expand Down
4 changes: 1 addition & 3 deletions src/System.Data.SqlClient/ref/System.Data.SqlClient.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,21 +247,19 @@ public sealed partial class SqlBulkCopyColumnMapping
public string SourceColumn { get { return default(string); } set { } }
public int SourceOrdinal { get { return default(int); } set { } }
}
public sealed partial class SqlBulkCopyColumnMappingCollection : System.Collections.CollectionBase
public sealed partial class SqlBulkCopyColumnMappingCollection
{
internal SqlBulkCopyColumnMappingCollection() { }
public System.Data.SqlClient.SqlBulkCopyColumnMapping Add(System.Data.SqlClient.SqlBulkCopyColumnMapping bulkCopyColumnMapping) { return default(System.Data.SqlClient.SqlBulkCopyColumnMapping); }
public System.Data.SqlClient.SqlBulkCopyColumnMapping Add(int sourceColumnIndex, int destinationColumnIndex) { return default(System.Data.SqlClient.SqlBulkCopyColumnMapping); }
public System.Data.SqlClient.SqlBulkCopyColumnMapping Add(int sourceColumnIndex, string destinationColumn) { return default(System.Data.SqlClient.SqlBulkCopyColumnMapping); }
public System.Data.SqlClient.SqlBulkCopyColumnMapping Add(string sourceColumn, int destinationColumnIndex) { return default(System.Data.SqlClient.SqlBulkCopyColumnMapping); }
public System.Data.SqlClient.SqlBulkCopyColumnMapping Add(string sourceColumn, string destinationColumn) { return default(System.Data.SqlClient.SqlBulkCopyColumnMapping); }
public new void Clear() { }
public bool Contains(System.Data.SqlClient.SqlBulkCopyColumnMapping value) { return default(bool); }
public void CopyTo(System.Data.SqlClient.SqlBulkCopyColumnMapping[] array, int index) { }
public int IndexOf(System.Data.SqlClient.SqlBulkCopyColumnMapping value) { return default(int); }
public void Insert(int index, System.Data.SqlClient.SqlBulkCopyColumnMapping value) { }
public void Remove(System.Data.SqlClient.SqlBulkCopyColumnMapping value) { }
public new void RemoveAt(int index) { }
}
[System.FlagsAttribute]
public enum SqlBulkCopyOptions
Expand Down
3 changes: 1 addition & 2 deletions src/System.Data.SqlClient/ref/project.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@
"System.IO": "4.0.0",
"System.Runtime": "4.0.0",
"System.Threading.Tasks": "4.0.0",
"System.Xml.ReaderWriter": "4.0.0",
"System.Collections.NonGeneric": "4.0.0",
"System.Xml.ReaderWriter": "4.0.0"
},
"frameworks": {
"netstandard1.1": {
Expand Down
3 changes: 3 additions & 0 deletions src/System.Data.SqlClient/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,9 @@
<data name="ADP_NonPooledOpenTimeout" xml:space="preserve">
<value>Timeout attempting to open the connection. The time period elapsed prior to attempting to open the connection has been exceeded. This may have occurred because of too many simultaneous non-pooled connection attempts.</value>
</data>
<data name="Arg_RemoveArgNotFound" xml:space="preserve">
<value>Cannot remove the specified item because it was not found in the specified Collection.</value>
</data>
<data name="Data_InvalidOffsetLength" xml:space="preserve">
<value>Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection.</value>
</data>
Expand Down
162 changes: 91 additions & 71 deletions src/System.Data.SqlClient/src/System/Data/SqlClient/SqlBulkCopyColumnMappingCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,14 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.



//------------------------------------------------------------------------------


using System.Data.Common;

using System.Collections;
using System.Collections.Generic;
using System.Data.Common;
using System.Diagnostics;

namespace System.Data.SqlClient
{
public sealed class SqlBulkCopyColumnMappingCollection : CollectionBase
public sealed class SqlBulkCopyColumnMappingCollection : ICollection, IEnumerable, IList
{
private enum MappingSchema
{
Expand All @@ -25,33 +20,45 @@ private enum MappingSchema
OrdinalsOrdinals = 4,
}

private bool _readOnly;
private readonly List<object> _list = new List<object>();
private MappingSchema _mappingSchema = MappingSchema.Undefined;

internal SqlBulkCopyColumnMappingCollection()
{
}

public SqlBulkCopyColumnMapping this[int index]
{
get
{
return (SqlBulkCopyColumnMapping)this.List[index];
}
}
public int Count => _list.Count;

internal bool ReadOnly { get; set; }

bool ICollection.IsSynchronized => false;

object ICollection.SyncRoot => NonGenericList.SyncRoot;

bool IList.IsFixedSize => false;

// This always returns false in the full framework, regardless
// of the value of the internal ReadOnly property.
bool IList.IsReadOnly => false;

internal bool ReadOnly
object IList.this[int index]
{
get
{
return _readOnly;
return NonGenericList[index];
}
set
{
_readOnly = value;
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

NonGenericList[index] = value;
}
}

public SqlBulkCopyColumnMapping this[int index] => (SqlBulkCopyColumnMapping)_list[index];

public SqlBulkCopyColumnMapping Add(SqlBulkCopyColumnMapping bulkCopyColumnMapping)
{
Expand All @@ -62,35 +69,31 @@ public SqlBulkCopyColumnMapping Add(SqlBulkCopyColumnMapping bulkCopyColumnMappi
{
throw SQL.BulkLoadNonMatchingColumnMapping();
}
InnerList.Add(bulkCopyColumnMapping);
_list.Add(bulkCopyColumnMapping);
return bulkCopyColumnMapping;
}

public SqlBulkCopyColumnMapping Add(string sourceColumn, string destinationColumn)
{
AssertWriteAccess();
SqlBulkCopyColumnMapping column = new SqlBulkCopyColumnMapping(sourceColumn, destinationColumn);
return Add(column);
return Add(new SqlBulkCopyColumnMapping(sourceColumn, destinationColumn));
}

public SqlBulkCopyColumnMapping Add(int sourceColumnIndex, string destinationColumn)
{
AssertWriteAccess();
SqlBulkCopyColumnMapping column = new SqlBulkCopyColumnMapping(sourceColumnIndex, destinationColumn);
return Add(column);
return Add(new SqlBulkCopyColumnMapping(sourceColumnIndex, destinationColumn));
}

public SqlBulkCopyColumnMapping Add(string sourceColumn, int destinationColumnIndex)
{
AssertWriteAccess();
SqlBulkCopyColumnMapping column = new SqlBulkCopyColumnMapping(sourceColumn, destinationColumnIndex);
return Add(column);
return Add(new SqlBulkCopyColumnMapping(sourceColumn, destinationColumnIndex));
}
public SqlBulkCopyColumnMapping Add(int sourceColumnIndex, int destinationColumnIndex)
{
AssertWriteAccess();
SqlBulkCopyColumnMapping column = new SqlBulkCopyColumnMapping(sourceColumnIndex, destinationColumnIndex);
return Add(column);
return Add(new SqlBulkCopyColumnMapping(sourceColumnIndex, destinationColumnIndex));
}

private void AssertWriteAccess()
Expand All @@ -101,80 +104,54 @@ private void AssertWriteAccess()
}
}

new public void Clear()
public void Clear()
{
AssertWriteAccess();
base.Clear();
_list.Clear();
}

public bool Contains(SqlBulkCopyColumnMapping value)
{
return (-1 != InnerList.IndexOf(value));
}
public bool Contains(SqlBulkCopyColumnMapping value) => _list.Contains(value);

public void CopyTo(SqlBulkCopyColumnMapping[] array, int index)
{
InnerList.CopyTo(array, index);
}
public void CopyTo(SqlBulkCopyColumnMapping[] array, int index) => _list.CopyTo(array, index);

internal void CreateDefaultMapping(int columnCount)
{
for (int i = 0; i < columnCount; i++)
{
InnerList.Add(new SqlBulkCopyColumnMapping(i, i));
_list.Add(new SqlBulkCopyColumnMapping(i, i));
}
}

public int IndexOf(SqlBulkCopyColumnMapping value)
{
return InnerList.IndexOf(value);
}
public IEnumerator GetEnumerator() => _list.GetEnumerator();

public int IndexOf(SqlBulkCopyColumnMapping value) => _list.IndexOf(value);

public void Insert(int index, SqlBulkCopyColumnMapping value)
{
AssertWriteAccess();
InnerList.Insert(index, value);
_list.Insert(index, value);
}

public void Remove(SqlBulkCopyColumnMapping value)
{
AssertWriteAccess();
InnerList.Remove(value);
_list.Remove(value);
}

new public void RemoveAt(int index)
public void RemoveAt(int index)
{
AssertWriteAccess();
base.RemoveAt(index);
_list.RemoveAt(index);
}

internal void ValidateCollection()
{
MappingSchema mappingSchema;
foreach (SqlBulkCopyColumnMapping a in this)
foreach (SqlBulkCopyColumnMapping a in _list)
{
if (a.SourceOrdinal != -1)
{
if (a.DestinationOrdinal != -1)
{
mappingSchema = MappingSchema.OrdinalsOrdinals;
}
else
{
mappingSchema = MappingSchema.OrdinalsNames;
}
}
else
{
if (a.DestinationOrdinal != -1)
{
mappingSchema = MappingSchema.NemesOrdinals;
}
else
{
mappingSchema = MappingSchema.NamesNames;
}
}
mappingSchema = a.SourceOrdinal != -1 ?
(a.DestinationOrdinal != -1 ? MappingSchema.OrdinalsOrdinals : MappingSchema.OrdinalsNames) :
(a.DestinationOrdinal != -1 ? MappingSchema.NemesOrdinals : MappingSchema.NamesNames);

if (_mappingSchema == MappingSchema.Undefined)
{
Expand All @@ -189,6 +166,49 @@ internal void ValidateCollection()
}
}
}

void ICollection.CopyTo(Array array, int index) => NonGenericList.CopyTo(array, index);

int IList.Add(object value)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

return NonGenericList.Add(value);
}

bool IList.Contains(object value) => NonGenericList.Contains(value);

int IList.IndexOf(object value) => NonGenericList.IndexOf(value);

void IList.Insert(int index, object value)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

NonGenericList.Insert(index, value);
}

void IList.Remove(object value)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}

bool removed = _list.Remove(value);

// This throws on full framework, so it will also throw here.
if (!removed)
{
throw new ArgumentException(SR.Arg_RemoveArgNotFound);
}
}

private IList NonGenericList => _list;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the NonGenericList needed?
In the code the _list is being referenced using NonGenericList and _list member. Can we have this being referenced consistently?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to call some of the explicitly implemented non-generic collection interface members on List<T> and having this property is a convenient way to do that. Otherwise, we'd have to cast the List<T> to the non-generic interfaces at the callsites, e.g.:

void ICollection.CopyTo(Array array, int index) => ((ICollection)_list).CopyTo(array, index);

We did the same thing in X509CertificateCollection to avoid the casts at the callsites.

corefx/src/System.Security.Cryptography.X509Certificates/src/System/Security/Cryptography/X509Certificates/X509CertificateCollection.cs

Lines 247 to 250 in c20fcbe

private IList NonGenericList
{
get { return _list; }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

OK 👍

}
}