[tor-commits] [onionoo/master] Don't alter NodeStatus in DetailsDocumentWriter.

[karsten at torproject.org]

commit 51d883ba06bed1342311c8d8b78a8e6aea70f043
Author: Karsten Loesing <karsten.loesing at gmx.net>
Date:   Mon Apr 21 11:12:08 2014 +0200

    Don't alter NodeStatus in DetailsDocumentWriter.
    
    The following two things went wrong when splitting up NodeDataWriter into
    two classes in e152b67:
    
     - Writing exit addresses to details documents doesn't require adding
       these addresses to the NodeStatus instance only to filter out known OR
       addresses.  That's something we can also do in DetailsDocumentWriter.
    
     - Writing details documents is not the right place to learn the latest
       contact line of a relay to update our NodeStatus.  This is something we
       can do when parsing the relay descriptor in NodeDetailsStatusUpdater.
    
    These are no bugs, but things that make it harder to refactor
    DetailsDocumentWriter.
---
 .../torproject/onionoo/DetailsDocumentWriter.java  |   33 +++++++-------------
 .../onionoo/NodeDetailsStatusUpdater.java          |   18 ++++++++---
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/src/org/torproject/onionoo/DetailsDocumentWriter.java b/src/org/torproject/onionoo/DetailsDocumentWriter.java
index 1682d7d..950a332 100644
--- a/src/org/torproject/onionoo/DetailsDocumentWriter.java
+++ b/src/org/torproject/onionoo/DetailsDocumentWriter.java
@@ -244,25 +244,29 @@ public class DetailsDocumentWriter implements DescriptorListener,
 
       /* Add exit addresses if at least one of them is distinct from the
        * onion-routing addresses. */
-      if (exitListEntries.containsKey(fingerprint)) {
+      SortedSet<String> exitAddresses = new TreeSet<String>();
+      if (this.exitListEntries.containsKey(fingerprint)) {
         for (ExitListEntry exitListEntry :
-            exitListEntries.get(fingerprint)) {
-          entry.addExitAddress(exitListEntry.getExitAddress());
+            this.exitListEntries.get(fingerprint)) {
+          String exitAddress = exitListEntry.getExitAddress();
+          if (exitAddress.length() > 0 &&
+              !entry.getAddress().equals(exitAddress) &&
+              !entry.getOrAddresses().contains(exitAddress)) {
+            exitAddresses.add(exitAddress);
+          }
         }
       }
-      if (!entry.getExitAddresses().isEmpty()) {
+      if (!exitAddresses.isEmpty()) {
         sb.append(",\n\"exit_addresses\":[");
         int written = 0;
-        for (String exitAddress : entry.getExitAddresses()) {
+        for (String exitAddress : exitAddresses) {
           sb.append((written++ > 0 ? "," : "") + "\""
               + exitAddress.toLowerCase() + "\"");
         }
         sb.append("]");
       }
 
-      /* Append descriptor-specific part from details status file, and
-       * update contact in node status. */
-      /* TODO Updating the contact here seems like a pretty bad hack. */
+      /* Append descriptor-specific part from details status file. */
       DetailsStatus detailsStatus = this.documentStore.retrieve(
           DetailsStatus.class, false, fingerprint);
       if (detailsStatus != null &&
@@ -275,18 +279,9 @@ public class DetailsDocumentWriter implements DescriptorListener,
           if (line.startsWith("\"desc_published\":")) {
             continue;
           }
-          if (line.startsWith("\"contact\":")) {
-            int start = "\"contact\":\"".length(),
-                end = line.length() - 1;
-            if (line.endsWith(",")) {
-              end--;
-            }
-            contact = unescapeJSON(line.substring(start, end));
-          }
           sb.append("\n" + line);
         }
         s.close();
-        entry.setContact(contact);
       }
 
       /* Finish details string. */
@@ -364,10 +359,6 @@ public class DetailsDocumentWriter implements DescriptorListener,
     return StringEscapeUtils.escapeJavaScript(s).replaceAll("\\\\'", "'");
   }
 
-  private static String unescapeJSON(String s) {
-    return StringEscapeUtils.unescapeJavaScript(s.replaceAll("'", "\\'"));
-  }
-
   public String getStatsString() {
     /* TODO Add statistics string. */
     return null;
diff --git a/src/org/torproject/onionoo/NodeDetailsStatusUpdater.java b/src/org/torproject/onionoo/NodeDetailsStatusUpdater.java
index 4d32b2c..4cfdd33 100644
--- a/src/org/torproject/onionoo/NodeDetailsStatusUpdater.java
+++ b/src/org/torproject/onionoo/NodeDetailsStatusUpdater.java
@@ -37,6 +37,8 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
   private SortedMap<String, NodeStatus> knownNodes =
       new TreeMap<String, NodeStatus>();
 
+  private Map<String, String> contacts = new HashMap<String, String>();
+
   private SortedMap<String, NodeStatus> relays;
 
   private SortedMap<String, NodeStatus> bridges;
@@ -99,8 +101,8 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
     Logger.printStatusTime("Started reverse domain name lookups");
     this.lookUpCitiesAndASes();
     Logger.printStatusTime("Looked up cities and ASes");
-    this.setRunningBits();
-    Logger.printStatusTime("Set running bits");
+    this.setRunningBitsAndContacts();
+    Logger.printStatusTime("Set running bits and contacts");
     this.calculatePathSelectionProbabilities();
     Logger.printStatusTime("Calculated path selection probabilities");
     this.finishReverseDomainNameLookups();
@@ -209,8 +211,10 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
     }
   }
 
-  private void setRunningBits() {
-    for (NodeStatus node : this.knownNodes.values()) {
+  private void setRunningBitsAndContacts() {
+    for (Map.Entry<String, NodeStatus> e : this.knownNodes.entrySet()) {
+      String fingerprint = e.getKey();
+      NodeStatus node = e.getValue();
       if (node.isRelay() && node.getRelayFlags().contains("Running") &&
           node.getLastSeenMillis() == this.relaysLastValidAfterMillis) {
         node.setRunning(true);
@@ -219,6 +223,9 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
           node.getLastSeenMillis() == this.bridgesLastPublishedMillis) {
         node.setRunning(true);
       }
+      if (this.contacts.containsKey(fingerprint)) {
+        node.setContact(this.contacts.get(fingerprint));
+      }
     }
   }
 
@@ -363,6 +370,9 @@ public class NodeDetailsStatusUpdater implements DescriptorListener,
     detailsStatus = new DetailsStatus();
     detailsStatus.setDocumentString(sb.toString());
     this.documentStore.store(detailsStatus, fingerprint);
+    if (descriptor.getContact() != null) {
+      this.contacts.put(fingerprint, descriptor.getContact());
+    }
   }
 
   private void processBridgeServerDescriptor(