Fix: Allow crawl node updates to set IP and port fields to null (#312)

## High Level Overview of Change

<!--
Please include a summary/list of the changes.
If too broad, please consider splitting into multiple PRs.
If a relevant Asana task, please link it here.
-->

If a user tries to update an existing crawls DB entry with a non-null IP
to have a null IP, the IP address does not update. This is because we
use merge in our saveNode, which has the behavior of excluding undefined
values during conflict resolution. As a result, we need to manually
override with the null fields we would like to retain (essentially
sanitize the new entry with nulls instead of undefined)

### Context of Change

<!--
Please include the context of a change.
If a bug fix, when was the bug introduced? What was the behavior?
If a new feature, why was this architecture chosen? What were the
alternatives?
If a refactor, how is this better than the previous implementation?

If there is a design document for this feature, please link it here.
-->

### Type of Change

<!--
Please check relevant options, delete irrelevant ones.
-->

- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- [ ] Refactor (non-breaking change that only restructures code)
- [ ] Tests (You added tests for code that already exists, or your new
feature included in this PR)
- [ ] Documentation Updates
- [ ] Release

## Before / After

<!--
If just refactoring / back-end changes, this can be just an in-English
description of the change at a technical level.
If a UI change, screenshots should be included.
-->

## Test Plan

<!--
Please describe the tests that you ran to verify your changes and
provide instructions so that others can reproduce.
-->

<!--
## Future Tasks
For future tasks related to PR.
-->

Author: achowdhry-ripple

.vscode/settings.json

@@ -25,7 +25,7 @@
         "enable": true
     },
     "editor.codeActionsOnSave": {
-        "source.fixAll.eslint": true
+        "source.fixAll.eslint": "explicit"
     },
     "files.insertFinalNewline": true,
     "files.trimFinalNewlines": true,

src/crawler/crawl.ts

@@ -202,7 +202,8 @@ class Crawler {
 
     const { this_node, active_nodes } = nodes
 
-    const promises: Array<Promise<void>> = [saveNode(this_node)]
+    // If we are saving the node outside of peer iterations, we do not want to force missing parameters to null
+    const promises: Array<Promise<void>> = [saveNode(this_node, false)]
 
     for (const node of active_nodes) {
       const normalizedPublicKey = Crawler.normalizePublicKey(node.public_key)
@@ -233,7 +234,9 @@ class Crawler {
       }
 
       this.publicKeysSeen.add(normalizedPublicKey)
-      promises.push(saveNode(dbNode))
+
+      // If a peer has the node IP as null, we want to overwrite in the database
+      promises.push(saveNode(dbNode, true))
 
       if (node.ip === undefined || node.port === undefined) {
         continue

src/shared/database/index.ts

@@ -49,17 +49,25 @@ export async function getNetworks(): Promise<Network[]> {
  * Saves a Node to database.
  *
  * @param node - Node to write to database.
+ * @param shouldOverwriteNull - If IP/port are not provided, will force update to null in DB.
  * @returns Void.
  */
-export async function saveNode(node: Node): Promise<void> {
+export async function saveNode(
+  node: Node,
+  shouldOverwriteNull: boolean,
+): Promise<void> {
   if (node.complete_ledgers && node.complete_ledgers.length > 255) {
     const ledgersSplit = node.complete_ledgers.split(',')
     node.complete_ledgers = ledgersSplit[ledgersSplit.length - 1]
   }
+
+  const sanitizedMergeNode = shouldOverwriteNull
+    ? { ip: null, port: null, ...node }
+    : node
   query('crawls')
     .insert(node)
     .onConflict('public_key')
-    .merge()
+    .merge(sanitizedMergeNode)
     .catch((err: Error) => log.error(err.message))
 }
 

test/crawler/crawler.test.ts

@@ -5,30 +5,9 @@ import { destroy, query, setupTables } from '../../src/shared/database'
 import { Node } from '../../src/shared/types'
 
 import network2 from './fixtures/cyclic-network.json'
+import nullNodeNetwork from './fixtures/null-ip-three-node-crawl.json'
 import network1 from './fixtures/three-node-crawl.json'
 
-function mock(): void {
-  // Sets up mocking at endpoints specified in network1
-  Object.keys(network1.peers).forEach((peer: string) => {
-    nock(`https://${peer}:51235`)
-      .get('/crawl')
-      .reply(
-        200,
-        (network1.peers as Record<string, Record<string, unknown>>)[peer],
-      )
-  })
-
-  // Sets up mocking at endpoints specified in network2
-  Object.keys(network2.peers).forEach((peer: string) => {
-    nock(`https://${peer}:51235`)
-      .get('/crawl')
-      .reply(
-        200,
-        (network2.peers as Record<string, Record<string, unknown>>)[peer],
-      )
-  })
-}
-
 async function crawl(ip: string): Promise<void> {
   await new Crawler().crawl({
     id: 'main',
@@ -40,7 +19,6 @@ async function crawl(ip: string): Promise<void> {
 describe('Runs test crawl', () => {
   beforeAll(async () => {
     await setupTables()
-    mock()
   })
 
   afterAll(async () => {
@@ -52,6 +30,15 @@ describe('Runs test crawl', () => {
   })
 
   test('successfully crawls 3 node network', async () => {
+    // Sets up mocking at endpoints specified in network1
+    Object.keys(network1.peers).forEach((peer: string) => {
+      nock(`https://${peer}:51235`)
+        .get('/crawl')
+        .reply(
+          200,
+          (network1.peers as Record<string, Record<string, unknown>>)[peer],
+        )
+    })
     await crawl('1.1.1.1')
 
     const results: Node[] = await query('crawls').select([
@@ -65,7 +52,65 @@ describe('Runs test crawl', () => {
     expect(results).toContainEqual(network1.result[2])
   })
 
+  test('successfully updates an existing ip and port to null', async () => {
+    // Manually set endpoints to standard 3 node network
+    Object.keys(network1.peers).forEach((peer: string) => {
+      nock(`https://${peer}:51235`)
+        .get('/crawl')
+        .reply(
+          200,
+          (network1.peers as Record<string, Record<string, unknown>>)[peer],
+        )
+    })
+
+    // Manually set same node endpoints to new network with a null ip/port
+    Object.keys(nullNodeNetwork.peers).forEach((peer: string) => {
+      nock(`https://${peer}:51235`)
+        .get('/crawl')
+        .reply(
+          200,
+          (nullNodeNetwork.peers as Record<string, Record<string, unknown>>)[
+            peer
+          ],
+        )
+    })
+    await crawl('1.1.1.1')
+
+    const initResults: Node[] = await query('crawls').select([
+      'ip',
+      'port',
+      'public_key',
+    ])
+
+    // Ensure DB has registered standard nodes with IP addresses
+    expect(initResults).toContainEqual(network1.result[0])
+    expect(initResults).toContainEqual(network1.result[1])
+    expect(initResults).toContainEqual(network1.result[2])
+
+    await crawl('1.1.1.1')
+
+    const modifiedResults: Node[] = await query('crawls').select([
+      'ip',
+      'port',
+      'public_key',
+    ])
+
+    // Ensure DB has registered new nodes with a null ip/port
+    expect(modifiedResults).toContainEqual(nullNodeNetwork.result[0])
+    expect(modifiedResults).toContainEqual(nullNodeNetwork.result[1])
+    expect(modifiedResults).toContainEqual(nullNodeNetwork.result[2])
+  })
+
   test('successfully crawls cyclic node network', async () => {
+    // Sets up mocking at endpoints specified in network2
+    Object.keys(network2.peers).forEach((peer: string) => {
+      nock(`https://${peer}:51235`)
+        .get('/crawl')
+        .reply(
+          200,
+          (network2.peers as Record<string, Record<string, unknown>>)[peer],
+        )
+    })
     await crawl('2.2.2.2')
 
     const results: Node[] = await query('crawls').select([

test/crawler/fixtures/null-ip-three-node-crawl.json

@@ -0,0 +1,92 @@
+{
+  "peers": {
+    "1.1.1.1": {
+      "overlay": {
+        "active": [
+          {
+            "public_key": "n9Kt2gdtxsYV6h4SAfHkRFUMU8gB6rDQ3MktiB3MMEDS9s9obu9b",
+            "complete_ledgers": "9-29"
+          },
+          {
+            "ip": "1.1.1.23",
+            "port": 51235,
+            "public_key": "n9LgkNWaPfRnxLKAgnSCMHRCyHBZsSdGBdRmyaYEArL3SBoq1EqK",
+            "complete_ledgers": "10-20"
+          }
+        ]
+      },
+      "server": {
+        "complete_ledgers": "10-20",
+        "pubkey_node" : "n9Mh83gUuY4hBXVD9geWHsyVwz5h32rjauLWQCZJVTEbCb5TYs21",
+        "server_state": "connected",
+        "io_latency_ms": "1",
+        "load_factor_server": "1",
+        "uptime": "1",
+        "version": "1.6.0"
+      },
+      "unl": {
+        "validator_sites": [
+          {
+            "last_refresh_status": "same_sequence",
+            "last_refresh_time": "2022-May-03 21:59:20.652097009 UTC",
+            "next_refresh_time": "2022-May-03 22:04:20.636643972 UTC",
+            "refresh_interval_min": 5,
+            "uri":"https://vl.fake.example.com"
+          }
+        ]
+      }
+    },
+    "1.1.1.13": {
+      "overlay": {
+        "active": [
+          {
+            "ip": "1.1.1.23",
+            "port": 51235,
+            "public_key": "n9LgkNWaPfRnxLKAgnSCMHRCyHBZsSdGBdRmyaYEArL3SBoq1EqK",
+            "complete_ledgers": "10-20"
+          }
+        ]
+      },
+      "server": {
+        "complete_ledgers": "10-20",
+        "pubkey_node" : "n9Kt2gdtxsYV6h4SAfHkRFUMU8gB6rDQ3MktiB3MMEDS9s9obu9b",
+        "server_state": "connected",
+        "io_latency_ms": "1",
+        "load_factor_server": "1",
+        "uptime": "1",
+        "version": "1.6.0"
+      }
+    },
+    "1.1.1.23": {
+      "overlay": {
+        "active": []
+      },
+      "server": {
+        "complete_ledgers": "10-20",
+        "pubkey_node" : "n9LgkNWaPfRnxLKAgnSCMHRCyHBZsSdGBdRmyaYEArL3SBoq1EqK",
+        "server_state": "connected",
+        "io_latency_ms": "1",
+        "load_factor_server": "1",
+        "uptime": "1",
+        "version": "1.6.0"
+      }
+    }
+  },
+  "result": [
+    {
+      "public_key" : "n9Mh83gUuY4hBXVD9geWHsyVwz5h32rjauLWQCZJVTEbCb5TYs21",
+      "ip": null,
+      "port": null
+    },
+    {
+      "public_key": "n9Kt2gdtxsYV6h4SAfHkRFUMU8gB6rDQ3MktiB3MMEDS9s9obu9b",
+      "ip": null,
+      "port": null
+    },
+    {
+      "public_key": "n9LgkNWaPfRnxLKAgnSCMHRCyHBZsSdGBdRmyaYEArL3SBoq1EqK",
+      "ip": "1.1.1.23",
+      "port": 51235
+    }
+  ]
+}