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

Remove System.Collection.NonGeneric from SqlClient contract #6527

Merged
merged 2 commits into from Mar 1, 2016

Conversation

justinvp
Copy link
Contributor

A new dependency on System.Collections.NonGeneric was recently added to System.Data.SqlClient in #6396 as part of exposing SqlBulkCopyColumnMappingCollection, which derives from CollectionBase.

This PR is made up of two commits:

  1. The first commit removes the dependency from the contract, by changingSqlBulkCopyColumnMappingCollection to derive from IList directly instead of CollectionBase. This is consistent with other collections in CoreFX that derive from CollectionBase in the full framework (e.g. X509CertificateCollection [manual], XmlArrayItemAttributes [manual], XmlAnyElementAttributes [manual], etc.).
  2. The second commit changes the actual SqlBulkCopyColumnMappingCollection implementation to implement IList directly instead of deriving from CollectionBase. Tests are included to verify the behavior remains the same as when the implementation came from CollectionBase (I also ran these tests on the full framework).

There are two additional related changes I'd like to address in separate PRs:

  • Removing the dependency on System.Collections.NonGeneric from the SqlClient implementation. There are a few remaining uses of ArrayList and Hashtable in SqlClient that can be compatibly changed to no longer depend on the legacy collections. I plan to submit a separate PR that does this.
  • This PR changes the implementation of SqlBulkCopyColumnMappingCollection to use List<object> as the backing collection, to maintain the same behavior as when it derived from CollectionBase. However, it'd make more sense to strongly type it as List<SqlBulkCopyColumnMapping> as this collection is only intended to hold SqlBulkCopyColumnMapping objects. I didn't do that as part of this PR because it would change the behavior of some of the non-generic IList members (for the better, IMO). For example, IList.Add(object) currently allows adding an object of any type, even though it should really throw if the object isn't SqlBulkCopyColumnMapping (we've made similar changes to other collections in CoreFX that previously derived from CollectionBase in the full framework like X509CertificateCollection, changing it to use List<X509Certificate> as the backing collection in Cleanup and harden X509Certificate collections #2824).

Fixes #6406.

cc: @saurabh500

Change SqlBulkCopyColumnMappingCollection to no longer inherit from
CollectionBase in the SqlClient contract, along the lines of other
collections like X509CertificateCollection, XmlArrayItemAttributes,
XmlAnyElementAttributes, etc.
Changes SqlBulkCopyColumnMappingCollection to implement IList
directly instead of deriving from CollectionBase, using List<T> as
the private backing collection. Includes tests to verify the behavior
remains the same as when the implementation came from CollectionBase.
@saurabh500
Copy link
Contributor

@justinvp Thanks for the changes
I will go through them soon.

}
}

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK 👍

@saurabh500
Copy link
Contributor

LGTM.

saurabh500 added a commit that referenced this pull request Mar 1, 2016
Remove System.Collection.NonGeneric from SqlClient contract
@saurabh500 saurabh500 merged commit 36f5cf3 into dotnet:master Mar 1, 2016
@justinvp justinvp deleted the sqlclient_collectionbase branch March 1, 2016 17:13
@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…ionbase

Remove System.Collection.NonGeneric from SqlClient contract

Commit migrated from dotnet/corefx@36f5cf3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants