Make addPersistableNetworkPayloadFromInitialRequest private

Now that the only user is internal, the API can be made private and the tests can be removed. This involved adding a few test cases to processGetDataResponse to ensure the invalid hash size condition was still covered.
bisq-network · Dec 3, 2019 · 0649323 · 0649323
1 parent 4fe19ae
commit 0649323
Show file tree

Hide file tree

Showing 2 changed files with 8 additions and 24 deletions.
diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
@@ -542,15 +542,13 @@ public boolean addPersistableNetworkPayload(PersistableNetworkPayload payload,
     // Overwriting an entry would be also no issue. We also skip notifying listeners as we get called before the domain
     // is ready so no listeners are set anyway. We might get called twice from a redundant call later, so listeners
     // might be added then but as we have the data already added calling them would be irrelevant as well.
-    public boolean addPersistableNetworkPayloadFromInitialRequest(PersistableNetworkPayload payload) {
+    private void addPersistableNetworkPayloadFromInitialRequest(PersistableNetworkPayload payload) {
         byte[] hash = payload.getHash();
         if (payload.verifyHashSize()) {
             ByteArray hashAsByteArray = new ByteArray(hash);
             appendOnlyDataStoreService.put(hashAsByteArray, payload);
-            return true;
         } else {
             log.warn("We got a hash exceeding our permitted size");
-            return false;
         }
     }
 

diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStoragePersistableNetworkPayloadTest.java
@@ -49,11 +49,10 @@
  * Each subclass (Payload type) can optionally add additional tests that verify functionality only relevant
  * to that payload.
  *
- * Each test case is run through 4 entry points to verify the correct behavior:
+ * Each test case is run through 3 entry points to verify the correct behavior:
  *
- * 1.    RequestData path [addPersistableNetworkPayloadFromInitialRequest]
- * 2 & 3 Client API [addPersistableNetworkPayload(reBroadcast=(true && false))]
- * 4.    onMessage() [onMessage(AddPersistableNetworkPayloadMessage)]
+ * 1 & 2 Client API [addPersistableNetworkPayload(reBroadcast=(true && false))]
+ * 3.    onMessage() [onMessage(AddPersistableNetworkPayloadMessage)]
  */
 @SuppressWarnings("unused")
 public class P2PDataStoragePersistableNetworkPayloadTest {
@@ -81,11 +80,6 @@ public abstract static class AddPersistableNetworkPayloadTest {
         enum TestCase {
             PUBLIC_API,
             ON_MESSAGE,
-            INIT,
-        }
-
-        boolean expectBroadcastOnStateChange() {
-            return this.testCase != TestCase.INIT;
         }
 
         boolean expectedIsDataOwner() {
@@ -95,9 +89,7 @@ boolean expectedIsDataOwner() {
         void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload, boolean expectedReturnValue, boolean expectedStateChange) {
             SavedTestState beforeState = this.testState.saveTestState(persistableNetworkPayload);
 
-            if (this.testCase == TestCase.INIT) {
-                Assert.assertEquals(expectedReturnValue, this.testState.mockedStorage.addPersistableNetworkPayloadFromInitialRequest(persistableNetworkPayload));
-            } else if (this.testCase == TestCase.PUBLIC_API) {
+            if (this.testCase == TestCase.PUBLIC_API) {
                 Assert.assertEquals(expectedReturnValue,
                         this.testState.mockedStorage.addPersistableNetworkPayload(persistableNetworkPayload, TestState.getTestNodeAddress(), true, this.allowBroadcast, this.reBroadcast, this.checkDate));
             } else { // onMessage
@@ -107,9 +99,7 @@ void doAddAndVerify(PersistableNetworkPayload persistableNetworkPayload, boolean
                 testState.mockedStorage.onMessage(new AddPersistableNetworkPayloadMessage(persistableNetworkPayload), mockedConnection);
             }
 
-            boolean expectedBroadcast = expectedStateChange && this.expectBroadcastOnStateChange();
-
-            this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedStateChange, expectedBroadcast, expectedBroadcast, this.expectedIsDataOwner());
+            this.testState.verifyPersistableAdd(beforeState, persistableNetworkPayload, expectedStateChange, expectedStateChange, expectedStateChange, this.expectedIsDataOwner());
         }
 
         @Before
@@ -123,9 +113,6 @@ public void setup() {
         public static Collection<Object[]> data() {
             List<Object[]> data = new ArrayList<>();
 
-            // Init doesn't use other parameters
-            data.add(new Object[] { TestCase.INIT, false, false, false });
-
             // onMessage doesn't use other parameters
             data.add(new Object[] { TestCase.ON_MESSAGE, false, false, false });
 
@@ -149,9 +136,8 @@ public void addPersistableNetworkPayload() {
         public void addPersistableNetworkPayloadDuplicate() {
             doAddAndVerify(this.persistableNetworkPayload, true, true);
 
-            // Second call only succeeds if reBroadcast was set or we are adding through the init
-            // path which just overwrites
-            boolean expectedReturnValue = this.reBroadcast || this.testCase == TestCase.INIT;
+            // Second call only succeeds if reBroadcast was set
+            boolean expectedReturnValue = this.reBroadcast;
             doAddAndVerify(this.persistableNetworkPayload, expectedReturnValue, false);
         }
     }