Bugfix: Nested collection replacment #970

lhy-hoyin · 2024-10-16T09:11:20Z

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

by combining remove and add into a single method. Handle null and empty by throwing NullPointerException and IllegalArgumentException respectively. Throws NoSuchElementException if the element to replace cannot be found. Method will rollback to previous element (before replacement) if either the remove step or add step fails.

Test happy path. Test null handling. Test empty handling. Test missing handling.

lhy-hoyin · 2024-10-16T09:23:04Z

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!

impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java

impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy

Overloaded with a message and no-message variants.

lhy-hoyin · 2024-10-17T08:35:03Z

.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 · 2024-10-18T20:55:01Z

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

Remove rollback code

lhy-hoyin · 2024-10-19T09:01:22Z

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.

to also check equality for minKeyBitLength, apart from ID and jcaName (both inherited from CryptoAlgorithm).

Add tests addIdentifiableWithSameIdEvictsExisting and replaceSameObject

add() is used instead of the previous replace(), but behaviour should still be the same.

Bug was that newElement is always added to the back of the collection regardless of the position of existingElement. Add unit test to check ordering after replace

lhy-hoyin · 2024-10-27T12:26:15Z

@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

to reduce code complexity. All tests passed.

lhy-hoyin · 2024-10-29T03:25:25Z

@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!

…s due to failing Oracle JDK 17 download. (jwtk#981)

Use SPDX identifier in POMs This replaces the custom name with an SPDX identifier to enable tooling to automatically detect the correct license. See https://spdx.org/licenses/Apache-2.0.html

by combining remove and add into a single method. Handle null and empty by throwing NullPointerException and IllegalArgumentException respectively. Throws NoSuchElementException if the element to replace cannot be found. Method will rollback to previous element (before replacement) if either the remove step or add step fails.

Test happy path. Test null handling. Test empty handling. Test missing handling.

Overloaded with a message and no-message variants.

Remove rollback code

to also check equality for minKeyBitLength, apart from ID and jcaName (both inherited from CryptoAlgorithm).

Add tests addIdentifiableWithSameIdEvictsExisting and replaceSameObject

add() is used instead of the previous replace(), but behaviour should still be the same.

Bug was that newElement is always added to the back of the collection regardless of the position of existingElement. Add unit test to check ordering after replace

to reduce code complexity. All tests passed.

…yin/jjwt into lhy-hoyin-nested-collection-replace

lhazlewood · 2025-03-13T23:24:18Z

@lhy-hoyin sorry for the long delay, but I rebased from origin/master and there seems to be a Groovy syntax error in one of your files that is failing the JDK 7 build. Do you want to take a peek and edit it for a clean build?

lhazlewood

Overall a good PR! Nice work - just have some requested changes and comments.

lhazlewood · 2025-03-13T23:30:14Z

api/src/main/java/io/jsonwebtoken/lang/Assert.java

+     * @param object the object to check
+     * @throws IllegalArgumentException if the object is <code>null</code> or empty
+     */
+    public static void notEmpty(Object object) {


I just did a code search, and this method isn't used anywhere, so it's best to remove it, as well as the newly added notEmpty(Object, String) method above.

lhazlewood · 2025-03-13T23:32:16Z

impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java

@@ -222,4 +222,16 @@ protected boolean doVerify(VerifySecureDigestRequest<SecretKey> request) {
        byte[] computedSignature = digest(request);
        return MessageDigest.isEqual(providedSignature, computedSignature);
    }
+
+    @Override
+    public boolean equals(Object obj) {


Ordinarily, I agree that sub-type checking with specific properties should be performed in sub-classes, but in this case, all crypto JWT algorithms are guaranteed to be uniquely defined by their ID (and JCA name), regardless of internal implementation details. (In this particular case, the minKeyBitLength is really an internal convenience and not needed for algorithm equality check).

Additionally, if we changed DefaultMacAlgorithm, we'd have to change every other subclass of the parent CryptoAlgorithm and that could have far reaching consequences.

Based on this, we don't want to override equals at all, so please remove this method.

lhazlewood · 2025-03-14T00:05:15Z

impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java

@@ -48,7 +49,45 @@ private boolean doAdd(E e) {

    @Override
    public M add(E e) {


This is a fairly complex implementation - it almost makes me think we should create a specific abstract subclass for Algorithm/ID-specific logic, e.g. AlgorithmCollectionMutator or something like that.

Such a subclass could also be more efficient, potentially using a Set<String> or a Set<String,Identifiable> to keep track of what IDs exist already instead of iterating over the internal collection.

Finally, there's no method contract that collection element ordering will remain unchanged, so we don't need to perform step 3 - that will implicitly create a performance penalty by saving trailing elements and adding them back in to the end of the collection after replacing the found element.

Thoughts?

lhy-hoyin added 2 commits October 16, 2024 16:42

tests: Add tests for DefaultCollectionMutator.replace(E e)

508227c

Test happy path. Test null handling. Test empty handling. Test missing handling.

bdemers requested changes Oct 16, 2024

View reviewed changes

Implement overloaded Assert.notEmpty for asserting Object

01003bf

Overloaded with a message and no-message variants.

Change method signature of replace

da96e8b

Remove rollback code

lhy-hoyin added 4 commits October 19, 2024 17:24

Implement DefaultMacAlgorithm.equals

8e8fd6c

to also check equality for minKeyBitLength, apart from ID and jcaName (both inherited from CryptoAlgorithm).

Modify add(e) logic to replace existing with same ID

fd8dc5e

Add tests addIdentifiableWithSameIdEvictsExisting and replaceSameObject

Modify tests to explicitly test for situation mentioned in jwtk#961

0c15467

add() is used instead of the previous replace(), but behaviour should still be the same.

fix: Fix issue where replace does not do a in-place replacement

57e4268

Bug was that newElement is always added to the back of the collection regardless of the position of existingElement. Add unit test to check ordering after replace

lhy-hoyin requested a review from bdemers October 19, 2024 11:47

bdemers requested changes Oct 28, 2024

View reviewed changes

Integrate replacement logic directly into add

a480d1e

to reduce code complexity. All tests passed.

lhy-hoyin requested a review from bdemers October 29, 2024 03:19

bdemers approved these changes Oct 29, 2024

View reviewed changes

lhazlewood and others added 10 commits March 13, 2025 18:39

Changing JDK 17 build to use GitHub's 'setup-java' instead of Oracle'…

f973a3d

…s due to failing Oracle JDK 17 download. (jwtk#981)

Use SPDX identifier in POMs (jwtk#979)

053855f

Use SPDX identifier in POMs This replaces the custom name with an SPDX identifier to enable tooling to automatically detect the correct license. See https://spdx.org/licenses/Apache-2.0.html

tests: Add tests for DefaultCollectionMutator.replace(E e)

3e407f6

Test happy path. Test null handling. Test empty handling. Test missing handling.

Implement overloaded Assert.notEmpty for asserting Object

de9f3d9

Overloaded with a message and no-message variants.

Change method signature of replace

db865ee

Remove rollback code

Implement DefaultMacAlgorithm.equals

d0bad35

to also check equality for minKeyBitLength, apart from ID and jcaName (both inherited from CryptoAlgorithm).

Modify add(e) logic to replace existing with same ID

6d7a100

Add tests addIdentifiableWithSameIdEvictsExisting and replaceSameObject

Modify tests to explicitly test for situation mentioned in jwtk#961

8e36dce

add() is used instead of the previous replace(), but behaviour should still be the same.

fix: Fix issue where replace does not do a in-place replacement

8ab77ec

Bug was that newElement is always added to the back of the collection regardless of the position of existingElement. Add unit test to check ordering after replace

lhy-hoyin and others added 2 commits March 13, 2025 19:15

Integrate replacement logic directly into add

d50700f

to reduce code complexity. All tests passed.

Merge branch 'nested-collection-replace' of https://github.com/lhy-ho…

484872b

…yin/jjwt into lhy-hoyin-nested-collection-replace

lhazlewood requested changes Mar 14, 2025

View reviewed changes

lhazlewood added this to the 0.12.7 milestone Apr 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix: Nested collection replacment #970

Bugfix: Nested collection replacment #970

lhy-hoyin commented Oct 16, 2024 •

edited

Loading

lhy-hoyin commented Oct 16, 2024

bdemers left a comment

lhy-hoyin commented Oct 17, 2024 •

edited

Loading

bdemers commented Oct 18, 2024

lhy-hoyin commented Oct 19, 2024

lhy-hoyin commented Oct 27, 2024

bdemers left a comment

lhy-hoyin commented Oct 29, 2024 •

edited

Loading

bdemers left a comment

lhazlewood commented Mar 13, 2025

lhazlewood left a comment

lhazlewood Mar 13, 2025

lhazlewood Mar 13, 2025

lhazlewood Mar 14, 2025

		@@ -48,7 +49,45 @@ private boolean doAdd(E e) {

		@Override
		public M add(E e) {

Bugfix: Nested collection replacment #970

Are you sure you want to change the base?

Bugfix: Nested collection replacment #970

Conversation

lhy-hoyin commented Oct 16, 2024 • edited Loading

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

(2) Implement DefaultMacAlgorithm.equals

lhy-hoyin commented Oct 16, 2024

bdemers left a comment

Choose a reason for hiding this comment

lhy-hoyin commented Oct 17, 2024 • edited Loading

bdemers commented Oct 18, 2024

lhy-hoyin commented Oct 19, 2024

lhy-hoyin commented Oct 27, 2024

bdemers left a comment

Choose a reason for hiding this comment

lhy-hoyin commented Oct 29, 2024 • edited Loading

bdemers left a comment

Choose a reason for hiding this comment

lhazlewood commented Mar 13, 2025

lhazlewood left a comment

Choose a reason for hiding this comment

lhazlewood Mar 13, 2025

Choose a reason for hiding this comment

lhazlewood Mar 13, 2025

Choose a reason for hiding this comment

lhazlewood Mar 14, 2025

Choose a reason for hiding this comment

lhy-hoyin commented Oct 16, 2024 •

edited

Loading

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

(2) Implement `DefaultMacAlgorithm.equals`

lhy-hoyin commented Oct 17, 2024 •

edited

Loading

lhy-hoyin commented Oct 29, 2024 •

edited

Loading