Skip to content
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.

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

Added Range Manipulation APIs to Collection<T> and ObservableCollection<T> #35772

Open
wants to merge 8 commits into
base: master
from

Conversation

@ahoefling
Copy link
Contributor

ahoefling commented Mar 5, 2019

Added Range Manipulation APIs to Collection<T> and ObservableCollection<T>.

Fixes: #10752

Added the following new API that was approved:

    // Adds a range to the end of the collection.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void AddRange(IEnumerable<T> collection) => InsertItemsRange(0, collection);

    // Inserts a range
    // Raises CollectionChanged (NotifyCollectionChangedAction.Add)
    public void InsertRange(int index, IEnumerable<T> collection) => InsertItemsRange(index, collection);

    // Removes a range.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Remove)
    public void RemoveRange(int index, int count) => RemoveItemsRange(index, count);

    // Will allow to replace a range with fewer, equal, or more items.
    // Raises CollectionChanged (NotifyCollectionChangedAction.Replace)
    public void ReplaceRange(int index, int count, IEnumerable<T> collection)
    {
         RemoveItemsRange(index, count);
         InsertItemsRange(index, collection);
    }

    #region virtual methods
    protected virtual void InsertItemsRange(int index, IEnumerable<T> collection);
    protected virtual void RemoveItemsRange(int index, int count);
    #endregion

Added Unit Tests

  • Collection<T>
  • ObservableCollection<T>

I have certified this code working using the newly added xUnit tests for both ObservableCollection<T> and Collection<T>.

Looking forward to everyone's feedback on this

@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Mar 5, 2019

CLA assistant check
All CLA requirements met.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 5, 2019

The build is failing due to CoreCLR components, while this is my first PR for .NET Core after reading the docs it appears that the linked PR in dotnet/coreclr#23018 needs to be merged first then the builds should start passing.

If there is anything else I need to do for this, let me know.

@ahsonkhan ahsonkhan requested a review from safern Mar 5, 2019

@ahsonkhan ahsonkhan added this to the 3.0 milestone Mar 5, 2019

@justinvp
Copy link
Collaborator

justinvp left a comment

I've seen subclasses of Collection<T> and ObservableCollection<T> in the wild that override InsertItem and SetItem to validate all the items added to the collection.

For example, the following subclass prevents null from being added to the collection:

public class NonNullCollection<T> : Collection<T> where T : class
{
    protected override void InsertItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.InsertItem(index, item);
    }

    protected override void SetItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.SetItem(index, item);
    }
}

Now that we're adding new ways to Add/Insert items in these collections, I'd love to see some tests that verify we're not unknowingly opening a hole that callers could use to bypass such validation.

For Collection<T>, a test like:

[Fact]
public void AddInsert_NonNullCollection_Throws()
{
    var collection = new NonNullCollection<string>();
    Assert.Throws<ArgumentNullException>(() => collection.Add(null));
    Assert.Throws<ArgumentNullException>(() => collection.Insert(0, null));
    Assert.Throws<ArgumentNullException>(() => collection.AddRange(new string[1]));
    Assert.Throws<ArgumentNullException>(() => collection.InsertRange(0, new string[1]));
}

internal class NonNullCollection<T> : Collection<T> where T : class
{
    protected override void InsertItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.InsertItem(index, item);
    }

    protected override void SetItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.SetItem(index, item);
    }
}

And for ObservableCollection<T>, a test like:

[Fact]
public void AddInsert_NonNullObservableCollection_Throws()
{
    var collection = new NonNullObservableCollection<string>();
    Assert.Throws<ArgumentNullException>(() => collection.Add(null));
    Assert.Throws<ArgumentNullException>(() => collection.Insert(0, null));
    Assert.Throws<ArgumentNullException>(() => collection.AddRange(new string[1]));
    Assert.Throws<ArgumentNullException>(() => collection.InsertRange(0, new string[1]));
}

internal class NonNullObservableCollection<T> : ObservableCollection<T> where T : class
{
    protected override void InsertItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.InsertItem(index, item);
    }

    protected override void SetItem(int index, T item)
    {
        if (item is null) {
            throw new ArgumentNullException(nameof(item));
        }
        base.SetItem(index, item);
    }
}

This way, if the implementation of Collection<T> or ObservableCollection<T> is tweaked, or optimizations added, the tests will ensure we're not unknowingly opening up an unwanted hole.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 6, 2019

@justinvp thanks for all the great feedback. I am going to look into adding test coverage for NonNullCollection<T> as you recommended

@justinvp

This comment has been minimized.

Copy link
Collaborator

justinvp commented Mar 9, 2019

@ahoefling, apologies for all my piecemeal comments. I haven't had much time to look at this (particularly this week), but I am interested in seeing it come together correctly. Thanks for working on it and being receptive to the feedback.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 10, 2019

@justinvp I updated the PR to handle several the instances you mentioned in review. I think we have everything covered now. Let me know if there is anything else you think should be fixed.

I also rebased this PR with all the changes that have made it into master which include the PR that was merged from the dotnet/coreclr.

@ahsonkhan @safern - Let me know if there is anything else I need to do this PR.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 11, 2019

@safern @ahsonkhan I was looking through the files changed and noticed there is a lot of things included now that I rebased this branch.

I thought that was what I was supposed to do, did I miss something?

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 15, 2019

@justinvp @safern @ahsonkhan I am planning to get to these changes today. I am looking for some feedback on how to properly fix the PR. The history got messed up when I thought I needed to rebase. Should I try a squash at this point? Is there a doc that gives guidance on updating a PR to make sure I follow the correct process?

@safern

This comment has been minimized.

Copy link
Member

safern commented Mar 15, 2019

@justinvp @safern @ahsonkhan I am planning to get to these changes today. I am looking for some feedback on how to properly fix the PR. The history got messed up when I thought I needed to rebase. Should I try a squash at this point? Is there a doc that gives guidance on updating a PR to make sure I follow the correct process?

It seemed like your rebase got messed up. What I would do is the following:

Squash all of your commits into 1 commit.
Checkout a new branch that is up to date with master.
Cherry-pick your new commit (which is a squash of all of them)
Reset your PR's branch to this new branch.

After squashing your commits into 1 follow

git checkout -b temp
git fetch <remote-to-dotnet/corefx> master
git reset --hard <remote-to-dotnet/corefx>/master
git cherry-pick <sha-squashed-commit>
git checkout -b observablecollection_addrange
git reset --hard temp
git push upstream observablecollection_addrange -f
@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 15, 2019

@safern this is perfect and just the advice I was looking for. I had a couple ideas but wanted to get clarification before proceeding

Added Collection<T> AddRange and InsertRange and added new XUnit test…
…s to cover new APIs

Added missing APIs from Collection<T>; Added new unit tests for RemoveRange, ReplaceRange, InsertItemsRange and RemoveItemsRange

Updated AddRange in Collection<T> to add items to the end of the collection

Added CollectionChanged tests for ObservableCollection<T> for InsertRange, AddRange, RemoveRange and ReplaceRange

Removing API changes as these will be mirrored from CoreCLR

Updated array assertions to use Span<T> to simplify logic

Sorted order of new API methods

Simplified CollectionTests Assertion to use Span<T> instead of calling out each item in the array

Added missing API ReplaceItemsRange and updated unit tests for check for overflow errors on RemoveItems

Added overrides for ObservableCollection<T> ItemsRange API to only fire CollectionChanged once per API call. Before this was firing for each item in the collection which is not the desired result. Updated unit tests to verify this logic

Updated RemoveItemsRange to prevent int.MaxValue overflow errors

Added int.MaxValue overflow tests for Collection<T> and ObservableCollection<T> when invoking RemoveItemsRange API

Added additional RemoveRange unit tests to cover when Count is Less than 0 or when Count is 0

Updated RemoveRange overflow tests to check for ArgumentException for new business rules

Updated ObservableCollection overflow test to assert ArgumentException

Added try-finally block to ObservableCollection<T> to certify that _skipCollectionChanged is always set to false after a collection manipulation in case an exception is thrown in a subclass. Added new test class to test the new rules on the example of a NonNullObservableCollection<T>

CollectionChanged events do not fire on RemoveRange if the count == 0

Updated ObservableCollection<T> to only throw the 3 required events when ReplaceItemsRange is invoked. Updated OnChangedEvent methods to check for IList and cast as necessary. Added additional exception handling to reset _skipRaisingEvents bool. Added unit tests to verify event and event parameters are being passed correctly.

Added Collection<T> AddRange and InsertRange and added new XUnit tests to cover new APIs

Simplified CollectionTests Assertion to use Span<T> instead of calling out each item in the array

Added additional RemoveRange unit tests to cover when Count is Less than 0 or when Count is 0

Updated ObservableCollection<T> to only throw the 3 required events when ReplaceItemsRange is invoked. Updated OnChangedEvent methods to check for IList and cast as necessary. Added additional exception handling to reset _skipRaisingEvents bool. Added unit tests to verify event and event parameters are being passed correctly.

Updated ReplaceRange event to pass in the OldItems and NewItems to match the event docs

Simplified CollectionChanged 'NewItems for InsertItemsRange

Updated removedItems to use an array instead of a List<T>

Added new OnCollectionChanged overloads to reduce need for additional if checks.

Removed stale code

Optimized ReplaceItemsRange by simplifying the itemsToReplace which is no an array instead of a List<T>

Removed (IList) cast in ReplaceItemsRange when raising CollectionChanged

Removed temp array in RemoveRange which decreases memory allocation needed

@ahoefling ahoefling force-pushed the HoeflingSoftware:observablecollection_addrange branch from bf4b07b to 70da65f Mar 16, 2019

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 16, 2019

@safern gotta love using the force push command when fixing messed up branches. 👍

@justinvp I took all of your feedback and implemented it in the latest revisions. We were able to make all the changes you requested.

@safern @ahsonkhan - I think the PR is good at this point (or so I hope). I am waiting for dotnet/coreclr#23166 to be mirrored over. If we merge this prior to that change being mirrored we will have 1 unit test fail. I assume that code will be mirrored over by Monday and I can make the final update to this PR then.

Added Collection<T> AddRange and InsertRange and added new XUnit test…
…s to cover new APIs

Added missing APIs from Collection<T>; Added new unit tests for RemoveRange, ReplaceRange, InsertItemsRange and RemoveItemsRange

Updated AddRange in Collection<T> to add items to the end of the collection

Added CollectionChanged tests for ObservableCollection<T> for InsertRange, AddRange, RemoveRange and ReplaceRange

Removing API changes as these will be mirrored from CoreCLR

Updated array assertions to use Span<T> to simplify logic

Sorted order of new API methods

Simplified CollectionTests Assertion to use Span<T> instead of calling out each item in the array

Added missing API ReplaceItemsRange and updated unit tests for check for overflow errors on RemoveItems

Added overrides for ObservableCollection<T> ItemsRange API to only fire CollectionChanged once per API call. Before this was firing for each item in the collection which is not the desired result. Updated unit tests to verify this logic

Updated RemoveItemsRange to prevent int.MaxValue overflow errors

Added int.MaxValue overflow tests for Collection<T> and ObservableCollection<T> when invoking RemoveItemsRange API

Added additional RemoveRange unit tests to cover when Count is Less than 0 or when Count is 0

Updated RemoveRange overflow tests to check for ArgumentException for new business rules

Updated ObservableCollection overflow test to assert ArgumentException

Added try-finally block to ObservableCollection<T> to certify that _skipCollectionChanged is always set to false after a collection manipulation in case an exception is thrown in a subclass. Added new test class to test the new rules on the example of a NonNullObservableCollection<T>

CollectionChanged events do not fire on RemoveRange if the count == 0

Updated ObservableCollection<T> to only throw the 3 required events when ReplaceItemsRange is invoked. Updated OnChangedEvent methods to check for IList and cast as necessary. Added additional exception handling to reset _skipRaisingEvents bool. Added unit tests to verify event and event parameters are being passed correctly.

Added Collection<T> AddRange and InsertRange and added new XUnit tests to cover new APIs

Simplified CollectionTests Assertion to use Span<T> instead of calling out each item in the array

Added additional RemoveRange unit tests to cover when Count is Less than 0 or when Count is 0

Updated ObservableCollection<T> to only throw the 3 required events when ReplaceItemsRange is invoked. Updated OnChangedEvent methods to check for IList and cast as necessary. Added additional exception handling to reset _skipRaisingEvents bool. Added unit tests to verify event and event parameters are being passed correctly.

Updated ReplaceRange event to pass in the OldItems and NewItems to match the event docs

Simplified CollectionChanged 'NewItems for InsertItemsRange

Updated removedItems to use an array instead of a List<T>

Added new OnCollectionChanged overloads to reduce need for additional if checks.

Removed stale code

Optimized ReplaceItemsRange by simplifying the itemsToReplace which is no an array instead of a List<T>

Removed (IList) cast in ReplaceItemsRange when raising CollectionChanged

Removed temp array in RemoveRange which decreases memory allocation needed

@ahoefling ahoefling force-pushed the HoeflingSoftware:observablecollection_addrange branch from 70da65f to 5621cc2 Mar 17, 2019

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 17, 2019

I have rebased the PR with the latest changes and all the tests are passing.

@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Mar 27, 2019

@safern is there anything else I need to do with this PR?

@karelz

This comment has been minimized.

Copy link
Member

karelz commented Apr 1, 2019

@safern can you please take a look / answer questions above?

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Apr 12, 2019

/azp run

@azure-pipelines

This comment has been minimized.

Copy link

azure-pipelines bot commented Apr 12, 2019

Azure Pipelines successfully started running 4 pipeline(s).

@danmosemsft

This comment has been minimized.

Copy link
Member

danmosemsft commented Apr 12, 2019

Cc @safern

@safern

This comment has been minimized.

Copy link
Member

safern commented Apr 13, 2019

Sorry, missed the notifications... Taking a look.

@safern
Copy link
Member

safern left a comment

Left some comments. It seems to me like we could add more negative tests when the events shouldn't be raised in the case of Count being <= 0.

ahoefling added some commits Apr 22, 2019

Added additional check in InsertRange to verify the input collection.…
…Count is greater than 0 before raising the events
@ahoefling

This comment has been minimized.

Copy link
Contributor Author

ahoefling commented Apr 22, 2019

@safern

It seems to me like we could add more negative tests when the events shouldn't be raised in the case of Count being <= 0.

I started going through adding negative tests but a lot of the negative tests aren't really allowed or appropriate as the Collection<T> base class throws exceptions if the input range is <= 0. Do you have any examples of what you are thinking, as I am not sure how to proceed on adding negative tests here. If it is as simple as just certifying the base class throws an exception that is no problem to add.

@safern @karelz @ahsonkhan I have made additional updates and everything appears to be working. AFAIK and understand the builds are failing because the current build is not using the CoreCLR changes we have already made.

Please advise if there is any additional changes that need to be made here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.