Bugfix: Nested collection replacment

[lhy-hoyin]

This commit

(1) fix #961 , where DefaultCollectionMutator.add(E e) does not overrides existing

add(E e) will check for Identifiable with the same ID in collection. If there is, an in-place replacement (order not changed) is done and calls changed(). Otherwise, it doAdd and calls changed().

Nothing is done if e is the same object as the existing element.

(2) Implement DefaultMacAlgorithm.equals

All attributes need to be equals

• ID (inherited from CryptoAlgorithm)
• jcaName (inherited from CryptoAlgorithm)
• minKeyBitLength

---

New unit tests are also added.

Fixes: #961

[lhy-hoyin]

I had considered (but eventually decided against) an alternative behavior when e cannot be identified to replace something.

Currently, it throws NoSuchElementException to indicate that there is nothing "similar" to replace.

As an alternative, when no "similar" element is found, replace could behave the same as doAdd. Essentially this means "replace if found, otherwise add as new".

I would appreciate thoughts on this alternative.

[bdemers]

Thanks for opening the PR @lhy-hoyin!

I've left a few overly verbose comments on the your PR, hopefully you find them helpful.

I know you proposed a new replace method, and there still might be value in that, I'd suggest starting with a test that described the original problem in #961.

Basically, the docs say that .add() should replace the any existing item, most importantly with Identifiable objects. (e.g. where a user wants to replace an algorithm with a different implementation)

Again, thanks for taking the time to look into this issue!

[lhy-hoyin]

.add() should replace the any existing item

In the case of .add(item) where the item is Identifiable, I suppose an existing item would be one with the same id.

Otherwise, when item is not Identifiable, I suppose that an existing item would be determined through the use of .equals().

In other words, the pseudocode would be like

add(E e) {
    for (item : collection):
        break when {
            item is Identifiable and item.id == e.id, or
            item.equals(e)
        } -> assign item to old

    if old is empty:
        doAdd(e)
    else:
        replace(old, e)
}

Did I interpret correctly?

[bdemers]

add(E e) {
    for (item : collection):
        break when {
            item is Identifiable and item.id == e.id, or
            item.equals(e)
        } -> assign item to old

    if old is empty:
        doAdd(e)
    else:
        replace(old, e)
}

Did I interpret correctly?

Yes, possibly with the minor issue of equality. if item.equals(e) there is nothing to do, as e is already contained in the collection, and we would not want to trigger the changed() method

[lhy-hoyin]

I noticed that DefaultMacAlgorithm did not override equals(), hence its using CryptoAlgorithm's implementation of equals().

In CryptoAlgorithm, it has ID and jacName as attributes.
In DefaultMacAlgorithm, apart from inheriting ID and jacName, it also has it own attribute minKeyBitLength.

I would like to implement equals in DefaultMacAlgorithm (as it simplifies the logic in replace), but is unsure if that would be a breaking change (for end users).

    @Override
    public boolean equals(Object obj) {
        if (this == obj) {
            return true;
        }
        if (obj instanceof DefaultMacAlgorithm) {
            DefaultMacAlgorithm other = (DefaultMacAlgorithm) obj;
            return super.equals(obj) && this.minKeyBitLength == other.minKeyBitLength;
        }
        return false;
    }

With the above implementation, I am able to pass all tests.

[lhy-hoyin]

@bdemers Would you be able to take a look and approve the PR before the end of the month? I am hoping for this PR to contribute to my hacktoberfest progress (which cuts off by the end of the month).

[bdemers]

Sorry about the delay. We can try to wrap this up this week!

The current add flow looks like it needs to iterate over the list twice (once to check if an item matches, and then again to do the replacement.
My suggestion would be to remove the public replace method (use a private method if needed), since the main problem is the add(Identifiable) use case.

That should reduce the complexity of the method/logic

[lhy-hoyin]

@bdemers I have integrated the logic for replacement directly into add. This reduced the code complexity. I have also adjusted the tests accordingly.

replace is fully removed.

[bdemers]

This looks good!
Thank you!