From 7c8974f88f27ca0435c89d0ec7fd29880ac8f658 Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Sat, 6 Feb 2021 15:48:29 +0900 Subject: [PATCH 1/7] Add unit tests for the IPBanList class --- app/pom.xml | 14 +++ .../roller/weblogger/util/IPBanList.java | 10 ++- .../roller/weblogger/util/IPBanListTest.java | 89 +++++++++++++++++++ 3 files changed, 109 insertions(+), 4 deletions(-) create mode 100644 app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java diff --git a/app/pom.xml b/app/pom.xml index f6670154e5..aba8f03f88 100644 --- a/app/pom.xml +++ b/app/pom.xml @@ -553,6 +553,20 @@ limitations under the License. test + + org.mockito + mockito-junit-jupiter + 3.7.7 + test + + + + org.assertj + assertj-core + 3.19.0 + test + + org.apache.ant ant diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 4fd3eb12cf..291cd8bc05 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -24,6 +24,8 @@ import java.io.PrintWriter; import java.util.HashSet; import java.util.Set; +import java.util.function.Supplier; + import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; import org.apache.roller.weblogger.config.WebloggerConfig; @@ -51,17 +53,17 @@ public final class IPBanList { static { - instance = new IPBanList(); + instance = new IPBanList(() -> WebloggerConfig.getProperty("ipbanlist.file")); } - // private because we are a singleton - private IPBanList() { + // package-private for unit tests + IPBanList(Supplier banIpsFilePathSupplier) { log.debug("INIT"); // load up set of denied ips - String banIpsFilePath = WebloggerConfig.getProperty("ipbanlist.file"); + String banIpsFilePath = banIpsFilePathSupplier.get(); if(banIpsFilePath != null) { ModifiedFile banIpsFile = new ModifiedFile(banIpsFilePath); diff --git a/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java new file mode 100644 index 0000000000..8e87f986ae --- /dev/null +++ b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java @@ -0,0 +1,89 @@ +package org.apache.roller.weblogger.util; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +class IPBanListTest { + + @TempDir + Path tmpDir; + Path ipBanList; + IPBanList sut; + + @BeforeEach + void setUp() throws IOException { + ipBanList = tmpDir.resolve("ipbanlist.txt"); + Files.createFile(ipBanList); + sut = new IPBanList(() -> ipBanList.toAbsolutePath().toString()); + } + + @Test + @DisplayName("addBanned() adds the given IP address to the file") + void addBannedAddsToFile() { + sut.addBannedIp("10.0.0.1"); + + assertThat(readIpBanList()).containsOnly("10.0.0.1"); + } + + @Test + @DisplayName("addBanned() ignores nulls") + void addBannedIgnoresNulls() { + sut.addBannedIp(null); + + assertThat(readIpBanList()).isEmpty(); + } + + @Test + @DisplayName("isBanned() returns true if the given IP address is banned") + void isBanned() { + sut.addBannedIp("10.0.0.1"); + + assertThat(sut.isBanned("10.0.0.1")).isTrue(); + } + + @Test + @DisplayName("isBanned() returns false if the given IP address it not banned") + void isBanned2() { + assertThat(sut.isBanned("10.0.0.1")).isFalse(); + } + + @Test + @DisplayName("isBanned() returns false if the given IP address is null") + void isBanned3() { + assertThat(sut.isBanned(null)).isFalse(); + } + + @Test + @DisplayName("isBanned() reads the file if needed") + void isBanned4() { + writeIpBanList("10.0.0.1"); + + assertThat(sut.isBanned("10.0.0.1")).isTrue(); + } + + private void writeIpBanList(String ipAddress) { + try { + Files.writeString(ipBanList, ipAddress); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + private List readIpBanList() { + try { + return Files.readAllLines(ipBanList); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } +} From 0fd0b39a65a8284f5e4bed7a98e54f7130a3277b Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Thu, 11 Feb 2021 08:57:51 +0900 Subject: [PATCH 2/7] Use a synchronized set as it's shared across multiple threads and gets mutated --- .../java/org/apache/roller/weblogger/util/IPBanList.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 291cd8bc05..4c1369ab21 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -22,6 +22,7 @@ import java.io.FileReader; import java.io.FileWriter; import java.io.PrintWriter; +import java.util.Collections; import java.util.HashSet; import java.util.Set; import java.util.function.Supplier; @@ -43,7 +44,7 @@ public final class IPBanList { private static Log log = LogFactory.getLog(IPBanList.class); // set of ips that are banned, use a set to ensure uniqueness - private Set bannedIps = new HashSet(); + private volatile Set bannedIps = Collections.synchronizedSet(new HashSet<>()); // file listing the ips that are banned private ModifiedFile bannedIpsFile = null; @@ -150,7 +151,7 @@ private synchronized void loadBannedIps() { // TODO: optimize this try (BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile))) { - HashSet newBannedIpList = new HashSet(); + Set newBannedIpList = new HashSet<>(); String ip = null; while((ip = in.readLine()) != null) { @@ -158,7 +159,7 @@ private synchronized void loadBannedIps() { } // list updated, reset modified file - this.bannedIps = newBannedIpList; + this.bannedIps = Collections.synchronizedSet(newBannedIpList); this.bannedIpsFile.clearChanged(); log.info(this.bannedIps.size()+" banned ips loaded"); From 491c3b70a724e24e6fa2ec93e6de42204faf279a Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Thu, 11 Feb 2021 09:04:25 +0900 Subject: [PATCH 3/7] Make a non-final log static field final, make an inner class static which doesn't have to be non-static --- .../main/java/org/apache/roller/weblogger/util/IPBanList.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 4c1369ab21..6b21ffe0ac 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -41,7 +41,7 @@ */ public final class IPBanList { - private static Log log = LogFactory.getLog(IPBanList.class); + private static final Log log = LogFactory.getLog(IPBanList.class); // set of ips that are banned, use a set to ensure uniqueness private volatile Set bannedIps = Collections.synchronizedSet(new HashSet<>()); @@ -173,7 +173,7 @@ private synchronized void loadBannedIps() { // a simple extension to the File class which tracks if the file has // changed since the last time we checked - private class ModifiedFile extends java.io.File { + private static class ModifiedFile extends java.io.File { private long myLastModified = 0; From 7620d3b456a44aac7d79d33ed7e44e6412ea5345 Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Thu, 11 Feb 2021 10:29:22 +0900 Subject: [PATCH 4/7] Use ConcurrentHashMap.newKeySet() instead of a synchronized set for better performance --- .../org/apache/roller/weblogger/util/IPBanList.java | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 6b21ffe0ac..1f9a3bb976 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -22,9 +22,8 @@ import java.io.FileReader; import java.io.FileWriter; import java.io.PrintWriter; -import java.util.Collections; -import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.function.Supplier; import org.apache.commons.logging.Log; @@ -44,7 +43,7 @@ public final class IPBanList { private static final Log log = LogFactory.getLog(IPBanList.class); // set of ips that are banned, use a set to ensure uniqueness - private volatile Set bannedIps = Collections.synchronizedSet(new HashSet<>()); + private volatile Set bannedIps = newThreadSafeSet(); // file listing the ips that are banned private ModifiedFile bannedIpsFile = null; @@ -151,7 +150,7 @@ private synchronized void loadBannedIps() { // TODO: optimize this try (BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile))) { - Set newBannedIpList = new HashSet<>(); + Set newBannedIpList = newThreadSafeSet(); String ip = null; while((ip = in.readLine()) != null) { @@ -159,7 +158,7 @@ private synchronized void loadBannedIps() { } // list updated, reset modified file - this.bannedIps = Collections.synchronizedSet(newBannedIpList); + this.bannedIps = newBannedIpList; this.bannedIpsFile.clearChanged(); log.info(this.bannedIps.size()+" banned ips loaded"); @@ -192,4 +191,7 @@ public void clearChanged() { } } + private static Set newThreadSafeSet() { + return ConcurrentHashMap.newKeySet(); + } } From a5417a5c3a6b7df83931871cc9b3721da19a97fd Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Sat, 13 Feb 2021 08:56:14 +0900 Subject: [PATCH 5/7] Remove the forceLoad parameter which is always false from the loadBannedIpsIfNeeded() method as per review comment --- .../java/org/apache/roller/weblogger/util/IPBanList.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java index 1f9a3bb976..2c9df32f8c 100644 --- a/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java +++ b/app/src/main/java/org/apache/roller/weblogger/util/IPBanList.java @@ -84,7 +84,7 @@ public static IPBanList getInstance() { public boolean isBanned(String ip) { // update the banned ips list if needed - this.loadBannedIpsIfNeeded(false); + this.loadBannedIpsIfNeeded(); if(ip != null) { return this.bannedIps.contains(ip); @@ -101,7 +101,7 @@ public void addBannedIp(String ip) { } // update the banned ips list if needed - this.loadBannedIpsIfNeeded(false); + this.loadBannedIpsIfNeeded(); if(!this.bannedIps.contains(ip) && (bannedIpsFile != null && bannedIpsFile.canWrite())) { @@ -129,10 +129,10 @@ public void addBannedIp(String ip) { /** * Check if the banned ips file has changed and needs to be reloaded. */ - private void loadBannedIpsIfNeeded(boolean forceLoad) { + private void loadBannedIpsIfNeeded() { if(bannedIpsFile != null && - (bannedIpsFile.hasChanged() || forceLoad)) { + (bannedIpsFile.hasChanged())) { // need to reload this.loadBannedIps(); From 15bf4733b099a78945e5f66e945c7106740ad71c Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Sat, 20 Feb 2021 14:57:45 +0900 Subject: [PATCH 6/7] Remove Mockito from pom.xml as it's not used at this point --- app/pom.xml | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/pom.xml b/app/pom.xml index aba8f03f88..d6942252f0 100644 --- a/app/pom.xml +++ b/app/pom.xml @@ -553,13 +553,6 @@ limitations under the License. test - - org.mockito - mockito-junit-jupiter - 3.7.7 - test - - org.assertj assertj-core From 9dca2dace7e179f17c4de42e352bc3bd2e167460 Mon Sep 17 00:00:00 2001 From: Kohei Nozaki Date: Sat, 22 May 2021 10:16:41 +0900 Subject: [PATCH 7/7] Remove AssertJ as per review comment --- app/pom.xml | 7 ------- .../roller/weblogger/util/IPBanListTest.java | 18 +++++++++++------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/app/pom.xml b/app/pom.xml index d6942252f0..f6670154e5 100644 --- a/app/pom.xml +++ b/app/pom.xml @@ -553,13 +553,6 @@ limitations under the License. test - - org.assertj - assertj-core - 3.19.0 - test - - org.apache.ant ant diff --git a/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java index 8e87f986ae..9d0a97a1d3 100644 --- a/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java +++ b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java @@ -11,7 +11,9 @@ import java.nio.file.Path; import java.util.List; -import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; class IPBanListTest { @@ -32,7 +34,9 @@ void setUp() throws IOException { void addBannedAddsToFile() { sut.addBannedIp("10.0.0.1"); - assertThat(readIpBanList()).containsOnly("10.0.0.1"); + List ipBanList = readIpBanList(); + assertTrue(ipBanList.contains("10.0.0.1")); + assertEquals(1, ipBanList.size()); } @Test @@ -40,7 +44,7 @@ void addBannedAddsToFile() { void addBannedIgnoresNulls() { sut.addBannedIp(null); - assertThat(readIpBanList()).isEmpty(); + assertTrue(readIpBanList().isEmpty()); } @Test @@ -48,19 +52,19 @@ void addBannedIgnoresNulls() { void isBanned() { sut.addBannedIp("10.0.0.1"); - assertThat(sut.isBanned("10.0.0.1")).isTrue(); + assertTrue(sut.isBanned("10.0.0.1")); } @Test @DisplayName("isBanned() returns false if the given IP address it not banned") void isBanned2() { - assertThat(sut.isBanned("10.0.0.1")).isFalse(); + assertFalse(sut.isBanned("10.0.0.1")); } @Test @DisplayName("isBanned() returns false if the given IP address is null") void isBanned3() { - assertThat(sut.isBanned(null)).isFalse(); + assertFalse(sut.isBanned(null)); } @Test @@ -68,7 +72,7 @@ void isBanned3() { void isBanned4() { writeIpBanList("10.0.0.1"); - assertThat(sut.isBanned("10.0.0.1")).isTrue(); + assertTrue(sut.isBanned("10.0.0.1")); } private void writeIpBanList(String ipAddress) {