You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@roller.apache.org by sn...@apache.org on 2021/07/18 20:24:48 UTC
[roller] 01/03: Fix flakey test.
This is an automated email from the ASF dual-hosted git repository.
snoopdave pushed a commit to branch roller-6.0.x
in repository https://gitbox.apache.org/repos/asf/roller.git
commit 66c9d578e08654d228faac3d401b098230ea91e0
Author: David M. Johnson <sn...@apache.org>
AuthorDate: Sun Jul 18 15:44:34 2021 -0400
Fix flakey test.
---
.../apache/roller/weblogger/util/IPBanList.java | 171 +++++++++++----------
.../roller/weblogger/util/IPBanListTest.java | 113 ++++++++++++++
2 files changed, 199 insertions(+), 85 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 822ca93..fb430a3 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
@@ -1,20 +1,20 @@
/*
-* Licensed to the Apache Software Foundation (ASF) under one or more
-* contributor license agreements. The ASF licenses this file to You
-* under the Apache License, Version 2.0 (the "License"); you may not
-* use this file except in compliance with the License.
-* You may obtain a copy of the License at
-*
-* http://www.apache.org/licenses/LICENSE-2.0
-*
-* Unless required by applicable law or agreed to in writing, software
-* distributed under the License is distributed on an "AS IS" BASIS,
-* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-* See the License for the specific language governing permissions and
-* limitations under the License. For additional information regarding
-* copyright in this work, please see the NOTICE file in the top level
-* directory of this distribution.
-*/
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
package org.apache.roller.weblogger.util;
@@ -22,8 +22,10 @@ import java.io.BufferedReader;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.PrintWriter;
-import java.util.HashSet;
import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Supplier;
+
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.apache.roller.weblogger.config.WebloggerConfig;
@@ -37,73 +39,73 @@ import org.apache.roller.weblogger.config.WebloggerConfig;
* automatically re-read the file and update the list when that happens.
*/
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 Set bannedIps = new HashSet();
-
+ private volatile Set<String> bannedIps = newThreadSafeSet();
+
// file listing the ips that are banned
private ModifiedFile bannedIpsFile = null;
-
+
// reference to our singleton instance
private static IPBanList instance = null;
-
-
+
+
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<String> 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);
-
+
if(banIpsFile.exists() && banIpsFile.canRead()) {
this.bannedIpsFile = banIpsFile;
this.loadBannedIps();
}
}
}
-
-
+
+
// access to the singleton instance
public static IPBanList getInstance() {
return instance;
}
-
-
+
+
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);
} else {
return false;
}
}
-
-
+
+
public void addBannedIp(String ip) {
-
+
if(ip == null) {
return;
}
-
+
// update the banned ips list if needed
- this.loadBannedIpsIfNeeded(false);
-
- if(!this.bannedIps.contains(ip) &&
- (bannedIpsFile != null && bannedIpsFile.canWrite())) {
-
+ this.loadBannedIpsIfNeeded();
+
+ if(!this.bannedIps.contains(ip) &&
+ (bannedIpsFile != null && bannedIpsFile.canWrite())) {
+
try {
synchronized(this) {
// add to file
@@ -111,86 +113,85 @@ public final class IPBanList {
out.println(ip);
out.close();
this.bannedIpsFile.clearChanged();
-
+
// add to Set
this.bannedIps.add(ip);
}
-
+
log.debug("ADDED "+ip);
} catch(Exception e) {
log.error("Error adding banned ip to file", e);
}
}
}
-
-
+
+
/**
* Check if the banned ips file has changed and needs to be reloaded.
*/
- private void loadBannedIpsIfNeeded(boolean forceLoad) {
-
- if(bannedIpsFile != null &&
- (bannedIpsFile.hasChanged() || forceLoad)) {
-
+ private void loadBannedIpsIfNeeded() {
+
+ if(bannedIpsFile != null &&
+ (bannedIpsFile.hasChanged())) {
+
// need to reload
this.loadBannedIps();
}
}
-
-
+
+
/**
* Load the list of banned ips from a file. This clears the old list and
* loads exactly what is in the file.
*/
private synchronized void loadBannedIps() {
-
+
if(bannedIpsFile != null) {
-
- try {
- HashSet newBannedIpList = new HashSet();
-
- // TODO: optimize this
- BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile));
-
+
+ // TODO: optimize this
+ try (BufferedReader in = new BufferedReader(new FileReader(this.bannedIpsFile))) {
+ Set<String> newBannedIpList = newThreadSafeSet();
+
String ip = null;
while((ip = in.readLine()) != null) {
newBannedIpList.add(ip);
}
-
- in.close();
-
+
// list updated, reset modified file
this.bannedIps = newBannedIpList;
this.bannedIpsFile.clearChanged();
-
+
log.info(this.bannedIps.size()+" banned ips loaded");
} catch(Exception ex) {
- log.error("Error loading banned ips from file", ex);
+ log.error("Error loading banned ips from file", ex);
}
-
+
}
}
-
-
- // a simple extension to the File class which tracks if the file has
+
+
+ // 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;
-
+
public ModifiedFile(String filePath) {
super(filePath);
-
+
this.myLastModified = lastModified();
}
public boolean hasChanged() {
return lastModified() != myLastModified;
}
-
+
public void clearChanged() {
myLastModified = lastModified();
}
}
-
-}
+
+ private static <T> Set<T> newThreadSafeSet() {
+ return ConcurrentHashMap.newKeySet();
+ }
+}
\ No newline at end of file
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 0000000..745664e
--- /dev/null
+++ b/app/src/test/java/org/apache/roller/weblogger/util/IPBanListTest.java
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. The ASF licenses this file to You
+ * under the Apache License, Version 2.0 (the "License"); you may not
+ * use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License. For additional information regarding
+ * copyright in this work, please see the NOTICE file in the top level
+ * directory of this distribution.
+ */
+
+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.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class IPBanListTest {
+
+ @TempDir
+ Path tmpDir;
+ Path ipBanListPath;
+ IPBanList ipBanList;
+
+ @BeforeEach
+ void setUp() throws IOException {
+ ipBanListPath = tmpDir.resolve("ipbanlist.txt");
+ Files.createFile(ipBanListPath);
+ ipBanList = new IPBanList(() -> ipBanListPath.toAbsolutePath().toString());
+ }
+
+ @Test
+ @DisplayName("addBanned() adds the given IP address to the file")
+ void addBannedAddsToFile() {
+ ipBanList.addBannedIp("10.0.0.1");
+
+ List<String> ipBanList = readIpBanList();
+ assertTrue(ipBanList.contains("10.0.0.1"));
+ assertEquals(1, ipBanList.size());
+ }
+
+ @Test
+ @DisplayName("addBanned() ignores nulls")
+ void addBannedIgnoresNulls() {
+ ipBanList.addBannedIp(null);
+
+ assertTrue(readIpBanList().isEmpty());
+ }
+
+ @Test
+ @DisplayName("isBanned() returns true if the given IP address is banned")
+ void isBanned() {
+ ipBanList.addBannedIp("10.0.0.1");
+ try { // work around for intermittently failing test
+ Thread.sleep(500);
+ } catch (InterruptedException ignored) {}
+ assertTrue(ipBanList.isBanned("10.0.0.1"));
+ }
+
+ @Test
+ @DisplayName("isBanned() returns false if the given IP address it not banned")
+ void isBanned2() {
+ assertFalse(ipBanList.isBanned("10.0.0.1"));
+ }
+
+ @Test
+ @DisplayName("isBanned() returns false if the given IP address is null")
+ void isBanned3() {
+ assertFalse(ipBanList.isBanned(null));
+ }
+
+ @Test
+ @DisplayName("isBanned() reads the file if needed")
+ void isBanned4() {
+ writeIpBanList("10.0.0.1");
+
+ assertTrue(ipBanList.isBanned("10.0.0.1"));
+ }
+
+ private void writeIpBanList(String ipAddress) {
+ try {
+ Files.writeString(ipBanListPath, ipAddress);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+
+ private List<String> readIpBanList() {
+ try {
+ return Files.readAllLines(ipBanListPath);
+ } catch (IOException e) {
+ throw new UncheckedIOException(e);
+ }
+ }
+}
\ No newline at end of file