From b83bd89990f64b7bcfdcf946e956f31f8cb31844 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Wed, 16 Oct 2024 16:42:24 +0800 Subject: [PATCH 1/9] Implement DefaultCollectionMutator.replace(E e) 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. --- .../impl/lang/DefaultCollectionMutator.java | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index cdb7fd8fc..8c7061758 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -23,6 +23,7 @@ import java.util.Collection; import java.util.LinkedHashSet; +import java.util.NoSuchElementException; public class DefaultCollectionMutator> implements CollectionMutator { @@ -46,6 +47,33 @@ private boolean doAdd(E e) { return this.collection.add(e); } + public M replace(E e) { + if (Objects.isEmpty(e)) { + if (e == null) throw new NullPointerException("Cannot be null."); + else throw new IllegalArgumentException("Cannot be empty."); + } + + // Keep a copy of the original, in case remove/add fails, and need to rollback + E old = null; + for (E element : this.collection) { + if (element.equals(e)) { + old = element; + break; + } + } + + if (old == null || !this.collection.contains(e)) { + String msg = this.getClass() + " does not contain " + e + "."; + throw new NoSuchElementException(msg); + } + + if (this.collection.remove(e)) + if (doAdd(e)) changed(); // removed and add successfully, notify changed() + else this.collection.add(old); // remove successfully but add failed, add back old (not considered a change) + + return self(); + } + @Override public M add(E e) { if (doAdd(e)) changed(); From 508227cf4d8b59a80df2f7062406b0c47bf624e1 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Wed, 16 Oct 2024 16:45:25 +0800 Subject: [PATCH 2/9] tests: Add tests for DefaultCollectionMutator.replace(E e) Test happy path. Test null handling. Test empty handling. Test missing handling. --- .../lang/DefaultCollectionMutatorTest.groovy | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index e1b6d4021..a15822bcd 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -16,10 +16,14 @@ package io.jsonwebtoken.impl.lang import io.jsonwebtoken.Identifiable +import io.jsonwebtoken.Jwts import io.jsonwebtoken.lang.Strings +import io.jsonwebtoken.security.MacAlgorithm import org.junit.Before import org.junit.Test +import java.lang.reflect.Constructor + import static org.junit.Assert.* /** @@ -128,6 +132,35 @@ class DefaultCollectionMutatorTest { assertEquals 0, changeCount } + @Test + void replace() { + Class c = Class.forName("io.jsonwebtoken.impl.security.DefaultMacAlgorithm") + Constructor ctor = c.getDeclaredConstructor(String.class, String.class, int.class) + ctor.setAccessible(true) + MacAlgorithm custom = (MacAlgorithm) ctor.newInstance('HS512', 'HmacSHA512', 80) + + m.add(Jwts.SIG.HS512) + m.replace(custom) + assertEquals 2, changeCount // replace is count as one change + assertEquals 1, m.getCollection().size() // existing is removed as part of replacement + assertEquals 80, ((MacAlgorithm) m.getCollection().toArray()[0]).getKeyBitLength() + } + + @Test(expected = NoSuchElementException) + void replaceMissing() { + m.replace('foo') + } + + @Test(expected = NullPointerException) + void replaceNull() { + m.replace(null) + } + + @Test(expected = IllegalArgumentException) + void replaceEmpty() { + m.replace(Strings.EMPTY) + } + @Test void clear() { m.add('one').add('two').add(['three', 'four']) From 01003bfdd295bf68a4f06d89bac797f7312b2196 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Thu, 17 Oct 2024 15:48:35 +0800 Subject: [PATCH 3/9] Implement overloaded Assert.notEmpty for asserting Object Overloaded with a message and no-message variants. --- .../java/io/jsonwebtoken/lang/Assert.java | 27 +++++++++++++++++++ .../impl/lang/DefaultCollectionMutator.java | 6 ++--- .../lang/DefaultCollectionMutatorTest.groovy | 2 +- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/api/src/main/java/io/jsonwebtoken/lang/Assert.java b/api/src/main/java/io/jsonwebtoken/lang/Assert.java index 022d58f9a..2c54dec69 100644 --- a/api/src/main/java/io/jsonwebtoken/lang/Assert.java +++ b/api/src/main/java/io/jsonwebtoken/lang/Assert.java @@ -347,6 +347,33 @@ public static void notEmpty(Map map) { notEmpty(map, "[Assertion failed] - this map must not be empty; it must contain at least one entry"); } + /** + * Assert that an Object is not null or empty. + * 
Assert.notEmpty(object, "Object cannot be null or empty");
+ * + * @param object the object to check + * @param message the exception message to use if the assertion fails + * @return the non-null, non-empty object + * @throws IllegalArgumentException if the object is null or empty + */ + public static Object notEmpty(Object object, String message) { + if (Objects.isEmpty(object)) { + throw new IllegalArgumentException(message); + } + return object; + } + + /** + * Assert that an Object is not null or empty. + * 
Assert.notEmpty(object);
+ * + * @param object the object to check + * @throws IllegalArgumentException if the object is null or empty + */ + public static void notEmpty(Object object) { + notEmpty(object, "[Assertion failed] - this object must not be null or empty"); + } + /** * Assert that the provided object is an instance of the provided class. diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index 8c7061758..d878846fe 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -16,6 +16,7 @@ package io.jsonwebtoken.impl.lang; import io.jsonwebtoken.Identifiable; +import io.jsonwebtoken.lang.Assert; import io.jsonwebtoken.lang.CollectionMutator; import io.jsonwebtoken.lang.Collections; import io.jsonwebtoken.lang.Objects; @@ -48,10 +49,7 @@ private boolean doAdd(E e) { } public M replace(E e) { - if (Objects.isEmpty(e)) { - if (e == null) throw new NullPointerException("Cannot be null."); - else throw new IllegalArgumentException("Cannot be empty."); - } + Assert.notEmpty(e, this.collection.getClass().getSimpleName() + " element cannot be null or empty."); // Keep a copy of the original, in case remove/add fails, and need to rollback E old = null; diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index a15822bcd..e25a0e82b 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -151,7 +151,7 @@ class DefaultCollectionMutatorTest { m.replace('foo') } - @Test(expected = NullPointerException) + @Test(expected = IllegalArgumentException) void replaceNull() { m.replace(null) } From da96e8b6a03dd9777cfead1a37dc9fd2719572fe Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Sat, 19 Oct 2024 15:22:48 +0800 Subject: [PATCH 4/9] Change method signature of replace Remove rollback code --- .../impl/lang/DefaultCollectionMutator.java | 24 +++++++------------ .../lang/DefaultCollectionMutatorTest.groovy | 8 +++---- 2 files changed, 12 insertions(+), 20 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index d878846fe..96bfbfdcd 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -48,26 +48,18 @@ private boolean doAdd(E e) { return this.collection.add(e); } - public M replace(E e) { - Assert.notEmpty(e, this.collection.getClass().getSimpleName() + " element cannot be null or empty."); - - // Keep a copy of the original, in case remove/add fails, and need to rollback - E old = null; - for (E element : this.collection) { - if (element.equals(e)) { - old = element; - break; - } - } + public M replace(E existingElement, E newElement) { + Assert.notEmpty(newElement, "newElement cannot be null or empty."); - if (old == null || !this.collection.contains(e)) { - String msg = this.getClass() + " does not contain " + e + "."; + if (!this.collection.contains(existingElement)) { + String msg = this.getClass() + " does not contain " + existingElement + "."; throw new NoSuchElementException(msg); } - if (this.collection.remove(e)) - if (doAdd(e)) changed(); // removed and add successfully, notify changed() - else this.collection.add(old); // remove successfully but add failed, add back old (not considered a change) + // FIXME: Ordering is not correct + if (this.collection.remove(existingElement)) + if (doAdd(newElement)) + changed(); // removed and add successfully, notify changed() return self(); } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index e25a0e82b..afbeb2927 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -140,7 +140,7 @@ class DefaultCollectionMutatorTest { MacAlgorithm custom = (MacAlgorithm) ctor.newInstance('HS512', 'HmacSHA512', 80) m.add(Jwts.SIG.HS512) - m.replace(custom) + m.replace(Jwts.SIG.HS512, custom) assertEquals 2, changeCount // replace is count as one change assertEquals 1, m.getCollection().size() // existing is removed as part of replacement assertEquals 80, ((MacAlgorithm) m.getCollection().toArray()[0]).getKeyBitLength() @@ -148,17 +148,17 @@ class DefaultCollectionMutatorTest { @Test(expected = NoSuchElementException) void replaceMissing() { - m.replace('foo') + m.replace('foo', 'bar') } @Test(expected = IllegalArgumentException) void replaceNull() { - m.replace(null) + m.replace('foo', null) } @Test(expected = IllegalArgumentException) void replaceEmpty() { - m.replace(Strings.EMPTY) + m.replace('foo', Strings.EMPTY) } @Test From 8e8fd6c95a0e6f534fb56c8514d3779cb62072ae Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Sat, 19 Oct 2024 17:24:23 +0800 Subject: [PATCH 5/9] Implement DefaultMacAlgorithm.equals to also check equality for minKeyBitLength, apart from ID and jcaName (both inherited from CryptoAlgorithm). --- .../impl/security/DefaultMacAlgorithm.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java b/impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java index e92277aed..11648f187 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/security/DefaultMacAlgorithm.java @@ -222,4 +222,16 @@ protected boolean doVerify(VerifySecureDigestRequest request) { byte[] computedSignature = digest(request); return MessageDigest.isEqual(providedSignature, computedSignature); } + + @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; + } } From fd8dc5efca5c2df8e2135f7ada0b620fab36f349 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Sat, 19 Oct 2024 17:31:38 +0800 Subject: [PATCH 6/9] Modify add(e) logic to replace existing with same ID Add tests addIdentifiableWithSameIdEvictsExisting and replaceSameObject --- .../impl/lang/DefaultCollectionMutator.java | 22 ++++++++- .../lang/DefaultCollectionMutatorTest.groovy | 47 +++++++++++++------ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index 96bfbfdcd..f6240341f 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -49,8 +49,14 @@ private boolean doAdd(E e) { } public M replace(E existingElement, E newElement) { + Assert.notEmpty(existingElement, "existingElement cannot be null or empty."); Assert.notEmpty(newElement, "newElement cannot be null or empty."); + // Same item, nothing to do + if (existingElement.equals(newElement)) + return self(); + + // Does not contain existingElement to replace if (!this.collection.contains(existingElement)) { String msg = this.getClass() + " does not contain " + existingElement + "."; throw new NoSuchElementException(msg); @@ -66,7 +72,21 @@ public M replace(E existingElement, E newElement) { @Override public M add(E e) { - if (doAdd(e)) changed(); + E existing = null; + for (E item : collection) { + boolean bothIdentifiable = e instanceof Identifiable && item instanceof Identifiable; + boolean sameId = bothIdentifiable && ((Identifiable) item).getId().equals(((Identifiable) e).getId()); + if (sameId) { + existing = item; + break; + } + } + + if (Objects.isEmpty(existing)) { + if (doAdd(e)) changed(); + } + else replace(existing, e); + return self(); } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index afbeb2927..7ed956034 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -99,24 +99,21 @@ class DefaultCollectionMutatorTest { @Test(expected = IllegalArgumentException) void addIdentifiableWithNullId() { - def e = new Identifiable() { - @Override - String getId() { - return null - } - } - m.add(e) + m.add(new IdentifiableObject(null, null)) } @Test(expected = IllegalArgumentException) void addIdentifiableWithEmptyId() { - def e = new Identifiable() { - @Override - String getId() { - return ' ' - } - } - m.add(e) + m.add(new IdentifiableObject(' ', null)) + } + + @Test + void addIdentifiableWithSameIdEvictsExisting() { + m.add(new IdentifiableObject('sameId', 'foo')) + m.add(new IdentifiableObject('sameId', 'bar')) + assertEquals 2, changeCount + assertEquals 1, m.collection.size() // second 'add' should evict first + assertEquals 'bar', ((IdentifiableObject) m.collection.toArray()[0]).obj } @Test @@ -146,6 +143,13 @@ class DefaultCollectionMutatorTest { assertEquals 80, ((MacAlgorithm) m.getCollection().toArray()[0]).getKeyBitLength() } + @Test + void replaceSameObject() { + m.add('hello') + m.replace('hello', 'hello') // replace with the same object, no change should be reflected + assertEquals 1, changeCount + } + @Test(expected = NoSuchElementException) void replaceMissing() { m.replace('foo', 'bar') @@ -168,4 +172,19 @@ class DefaultCollectionMutatorTest { m.clear() assertTrue m.getCollection().isEmpty() } + + private class IdentifiableObject implements Identifiable { + String id + String obj + + IdentifiableObject(String id, String obj) { + this.id = id + this.obj = obj + } + + @Override + String getId() { + return id + } + } } From 0c15467ac4d9575d2d0fd1f666577a234d8e1119 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Sat, 19 Oct 2024 18:40:12 +0800 Subject: [PATCH 7/9] Modify tests to explicitly test for situation mentioned in #961 add() is used instead of the previous replace(), but behaviour should still be the same. --- .../lang/DefaultCollectionMutatorTest.groovy | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index 7ed956034..d8df6c6f6 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -116,6 +116,20 @@ class DefaultCollectionMutatorTest { assertEquals 'bar', ((IdentifiableObject) m.collection.toArray()[0]).obj } + @Test + void addSecureDigestAlgorithmWithSameIdReplacesExisting() { + Class c = Class.forName("io.jsonwebtoken.impl.security.DefaultMacAlgorithm") + Constructor ctor = c.getDeclaredConstructor(String.class, String.class, int.class) + ctor.setAccessible(true) + MacAlgorithm custom = (MacAlgorithm) ctor.newInstance('HS512', 'HmacSHA512', 80) + + m.add(Jwts.SIG.HS512) + m.add(custom) + assertEquals 2, changeCount // replace is count as one change + assertEquals 1, m.getCollection().size() // existing is removed as part of replacement + assertEquals 80, ((MacAlgorithm) m.getCollection().toArray()[0]).getKeyBitLength() + } + @Test void remove() { m.add('hello').add('world') @@ -131,16 +145,14 @@ class DefaultCollectionMutatorTest { @Test void replace() { - Class c = Class.forName("io.jsonwebtoken.impl.security.DefaultMacAlgorithm") - Constructor ctor = c.getDeclaredConstructor(String.class, String.class, int.class) - ctor.setAccessible(true) - MacAlgorithm custom = (MacAlgorithm) ctor.newInstance('HS512', 'HmacSHA512', 80) + def e1 = new IdentifiableObject('sameId', 'e1') + def e2 = new IdentifiableObject('sameId', 'e2') - m.add(Jwts.SIG.HS512) - m.replace(Jwts.SIG.HS512, custom) + m.add(e1) + m.replace(e1, e2) assertEquals 2, changeCount // replace is count as one change assertEquals 1, m.getCollection().size() // existing is removed as part of replacement - assertEquals 80, ((MacAlgorithm) m.getCollection().toArray()[0]).getKeyBitLength() + assertEquals 'e2', ((IdentifiableObject) m.getCollection().toArray()[0]).obj } @Test @@ -175,9 +187,9 @@ class DefaultCollectionMutatorTest { private class IdentifiableObject implements Identifiable { String id - String obj + Object obj - IdentifiableObject(String id, String obj) { + IdentifiableObject(String id, Object obj) { this.id = id this.obj = obj } From 57e42684b7b19b7920abfa8bbaf382dbcdf84822 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Sat, 19 Oct 2024 19:33:30 +0800 Subject: [PATCH 8/9] fix: Fix issue where replace does not do a in-place replacement 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 --- .../impl/lang/DefaultCollectionMutator.java | 24 +++++++++++++++---- .../lang/DefaultCollectionMutatorTest.groovy | 7 ++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index f6240341f..5130e3160 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -23,6 +23,7 @@ import io.jsonwebtoken.lang.Strings; import java.util.Collection; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.NoSuchElementException; @@ -62,10 +63,25 @@ public M replace(E existingElement, E newElement) { throw new NoSuchElementException(msg); } - // FIXME: Ordering is not correct - if (this.collection.remove(existingElement)) - if (doAdd(newElement)) - changed(); // removed and add successfully, notify changed() + // Replacement step 1: iterate until element to replace + Iterator it = this.collection.iterator(); + while (it.hasNext()) + if (it.next().equals(existingElement)) { + it.remove(); // step 2: remove existingElement + break; + } + + // Replacement step 3: collect and remove elements after element to replace + Collection elementsAfterExisting = new LinkedHashSet<>(); + while (it.hasNext()) { + elementsAfterExisting.add(it.next()); + it.remove(); + } + + this.doAdd(newElement); // step 4: add replacer element (position will be at the existingElement) + this.collection.addAll(elementsAfterExisting); // step 5: add back the elemnts found after existingElement + + changed(); // trigger changed() return self(); } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index d8df6c6f6..356cd8c4f 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -177,6 +177,13 @@ class DefaultCollectionMutatorTest { m.replace('foo', Strings.EMPTY) } + @Test + void replaceMaintainsOrder() { + m.add(['1', '2', '3']) + m.replace('2', 'B') + assertArrayEquals(new Object[] {'1', 'B', '3'}, m.collection.toArray()) + } + @Test void clear() { m.add('one').add('two').add(['three', 'four']) From a480d1e656ef85809ce86acb4a30db5d3285d874 Mon Sep 17 00:00:00 2001 From: lhy-hoyin <88828097+lhy-hoyin@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:17:44 +0800 Subject: [PATCH 9/9] Integrate replacement logic directly into add to reduce code complexity. All tests passed. --- .../impl/lang/DefaultCollectionMutator.java | 67 +++++++------------ .../lang/DefaultCollectionMutatorTest.groovy | 62 ++++++----------- 2 files changed, 47 insertions(+), 82 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java index 5130e3160..f2f1286f8 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/DefaultCollectionMutator.java @@ -16,7 +16,6 @@ package io.jsonwebtoken.impl.lang; import io.jsonwebtoken.Identifiable; -import io.jsonwebtoken.lang.Assert; import io.jsonwebtoken.lang.CollectionMutator; import io.jsonwebtoken.lang.Collections; import io.jsonwebtoken.lang.Objects; @@ -25,7 +24,6 @@ import java.util.Collection; import java.util.Iterator; import java.util.LinkedHashSet; -import java.util.NoSuchElementException; public class DefaultCollectionMutator> implements CollectionMutator { @@ -49,59 +47,46 @@ private boolean doAdd(E e) { return this.collection.add(e); } - public M replace(E existingElement, E newElement) { - Assert.notEmpty(existingElement, "existingElement cannot be null or empty."); - Assert.notEmpty(newElement, "newElement cannot be null or empty."); - - // Same item, nothing to do - if (existingElement.equals(newElement)) - return self(); - - // Does not contain existingElement to replace - if (!this.collection.contains(existingElement)) { - String msg = this.getClass() + " does not contain " + existingElement + "."; - throw new NoSuchElementException(msg); - } + @Override + public M add(E e) { + boolean doReplace = false; - // Replacement step 1: iterate until element to replace + // Replacement step 1: iterate until element to replace (if any) + E item; Iterator it = this.collection.iterator(); - while (it.hasNext()) - if (it.next().equals(existingElement)) { - it.remove(); // step 2: remove existingElement - break; - } - - // Replacement step 3: collect and remove elements after element to replace - Collection elementsAfterExisting = new LinkedHashSet<>(); while (it.hasNext()) { - elementsAfterExisting.add(it.next()); - it.remove(); - } - - this.doAdd(newElement); // step 4: add replacer element (position will be at the existingElement) - this.collection.addAll(elementsAfterExisting); // step 5: add back the elemnts found after existingElement + item = it.next(); - changed(); // trigger changed() + // Same item, nothing to do + if (item.equals(e)) + return self(); - return self(); - } - - @Override - public M add(E e) { - E existing = null; - for (E item : collection) { boolean bothIdentifiable = e instanceof Identifiable && item instanceof Identifiable; boolean sameId = bothIdentifiable && ((Identifiable) item).getId().equals(((Identifiable) e).getId()); if (sameId) { - existing = item; + it.remove(); // step 2: remove existing item + doReplace = true; break; } } - if (Objects.isEmpty(existing)) { + if (doReplace) { + // Replacement step 3: collect and remove elements after element to replace + Collection elementsAfterExisting = new LinkedHashSet<>(); + while (it.hasNext()) { + elementsAfterExisting.add(it.next()); + it.remove(); + } + + this.doAdd(e); // step 4: add replacer element (position will be at the existing item) + this.collection.addAll(elementsAfterExisting); // step 5: add back the elements found after existing item + + changed(); // trigger changed() + } + else { + // No replacement, do add instead if (doAdd(e)) changed(); } - else replace(existing, e); return self(); } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy index 356cd8c4f..ce5797d0e 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/DefaultCollectionMutatorTest.groovy @@ -52,9 +52,17 @@ class DefaultCollectionMutatorTest { assertTrue c.isEmpty() } + @Test + void addNull() { + m.add(null) + assertEquals 0, changeCount + assertTrue m.getCollection().isEmpty() // wasn't added + } + @Test void addEmpty() { m.add(Strings.EMPTY) + assertEquals 0, changeCount assertTrue m.getCollection().isEmpty() // wasn't added } @@ -116,6 +124,19 @@ class DefaultCollectionMutatorTest { assertEquals 'bar', ((IdentifiableObject) m.collection.toArray()[0]).obj } + @Test + void addIdentifiableWithSameIdMaintainsOrder() { + IdentifiableObject e1 = new IdentifiableObject('1', 'e1') + IdentifiableObject e2 = new IdentifiableObject('sameId', 'e2') + IdentifiableObject e3 = new IdentifiableObject('3', 'e3') + IdentifiableObject eB = new IdentifiableObject('sameId', 'eB') + + m.add([e1, e2, e3]) + m.add(eB) // replace e2 with eB + assertEquals 2, changeCount + assertArrayEquals(new Object[] {e1, eB, e3}, m.collection.toArray()) + } + @Test void addSecureDigestAlgorithmWithSameIdReplacesExisting() { Class c = Class.forName("io.jsonwebtoken.impl.security.DefaultMacAlgorithm") @@ -143,47 +164,6 @@ class DefaultCollectionMutatorTest { assertEquals 0, changeCount } - @Test - void replace() { - def e1 = new IdentifiableObject('sameId', 'e1') - def e2 = new IdentifiableObject('sameId', 'e2') - - m.add(e1) - m.replace(e1, e2) - assertEquals 2, changeCount // replace is count as one change - assertEquals 1, m.getCollection().size() // existing is removed as part of replacement - assertEquals 'e2', ((IdentifiableObject) m.getCollection().toArray()[0]).obj - } - - @Test - void replaceSameObject() { - m.add('hello') - m.replace('hello', 'hello') // replace with the same object, no change should be reflected - assertEquals 1, changeCount - } - - @Test(expected = NoSuchElementException) - void replaceMissing() { - m.replace('foo', 'bar') - } - - @Test(expected = IllegalArgumentException) - void replaceNull() { - m.replace('foo', null) - } - - @Test(expected = IllegalArgumentException) - void replaceEmpty() { - m.replace('foo', Strings.EMPTY) - } - - @Test - void replaceMaintainsOrder() { - m.add(['1', '2', '3']) - m.replace('2', 'B') - assertArrayEquals(new Object[] {'1', 'B', '3'}, m.collection.toArray()) - } - @Test void clear() { m.add('one').add('two').add(['three', 'four'])