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

Bugfix: Nested collection replacment #970

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

import java.util.Collection;
import java.util.LinkedHashSet;
import java.util.NoSuchElementException;

public class DefaultCollectionMutator<E, M extends CollectionMutator<E, M>> implements CollectionMutator<E, M> {

Expand All @@ -46,6 +47,33 @@ private boolean doAdd(E e) {
return this.collection.add(e);
}

public M replace(E e) {
lhy-hoyin marked this conversation as resolved.
Show resolved Hide resolved
if (Objects.isEmpty(e)) {
if (e == null) throw new NullPointerException("Cannot be null.");
else throw new IllegalArgumentException("Cannot be empty.");
}
lhy-hoyin marked this conversation as resolved.
Show resolved Hide resolved

// 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;
}
}
lhy-hoyin marked this conversation as resolved.
Show resolved Hide resolved

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)
lhy-hoyin marked this conversation as resolved.
Show resolved Hide resolved

return self();
}

@Override
public M add(E e) {
if (doAdd(e)) changed();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.*

/**
Expand Down Expand Up @@ -128,6 +132,35 @@ class DefaultCollectionMutatorTest {
assertEquals 0, changeCount
}

@Test
void replace() {
lhy-hoyin marked this conversation as resolved.
Show resolved Hide resolved
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'])
Expand Down