[tor-commits] [stem/master] ConfChangedEvent could not represent multiple config values

[atagar at torproject.org]

commit f19b9273630b05c42b8fe4d59f449f001d870cba
Author: Damian Johnson <atagar at torproject.org>
Date:   Wed May 16 13:01:28 2018 -0700

    ConfChangedEvent could not represent multiple config values
    
    Great catch from dmr. The ConfChangedEvent's config was a simple {str => str}
    mapping, but some tor configuration options can have multiple values. For
    instance...
    
      650-CONF_CHANGED
      650-ExitPolicy=accept 34.3.4.5
      650-ExitPolicy=accept 3.4.53.3
      650 OK
    
    We can't change the event's config attribute without breaking backward
    compatability, but that's ok. Fiddling with this event a bit it's usage
    was actually kinda clunky since we bundled config unsetting with
    modifications. As such, splitting the 'config' attribute into a 'changed' and
    'unset'. This time with changed as a {str => list} mapping.
    
    Here's a little demo of what the new usage might look like...
    
      def print_config_changes(event):
        for key, values in my_event.changed:
          print('We changed %s to %s' % (key, ', '.join(values))
    
        for key in my_event.unset:
          print('We removed our configuration of %s' % key)
    
      my_controller.add_event_listener(print_config_changes, EventType.CONF_CHANGED)
---
 docs/change_log.rst          |  1 +
 stem/response/events.py      | 19 ++++++++++++++++---
 test/unit/response/events.py | 33 ++++++++++++++++++++++++++++++---
 3 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/docs/change_log.rst b/docs/change_log.rst
index 54a1e628..549e89c5 100644
--- a/docs/change_log.rst
+++ b/docs/change_log.rst
@@ -52,6 +52,7 @@ The following are only available within Stem's `git repository
   * Stacktrace if :func:`stem.connection.connect` had a string port argument
   * More reliable ExitPolicy resolution (:trac:`25739`)
   * Added the delivered_read, delivered_written, overhead_read, and overhead_written attributes to :class:`~stem.response.events.CircuitBandwidthEvent` (:spec:`fbb38ec`)
+  * Replaced the *config* attribute of :class:`~stem.response.events.ConfChangedEvent` with a *changed* and *unset* attributes
   * Replaced socket's :func:`~stem.socket.ControlPort.get_address`, :func:`~stem.socket.ControlPort.get_port`, and :func:`~stem.socket.ControlSocketFile.get_socket_path` with attributes
   * Removed 'raw' argument from :func:`~stem.socket.ControlSocket.send`
 
diff --git a/stem/response/events.py b/stem/response/events.py
index 1048e2ba..a50d90f8 100644
--- a/stem/response/events.py
+++ b/stem/response/events.py
@@ -570,15 +570,26 @@ class ConfChangedEvent(Event):
 
   The CONF_CHANGED event was introduced in tor version 0.2.3.3-alpha.
 
-  :var dict config: mapping of configuration options to their new values
-    (**None** if the option is being unset)
+  .. deprecated:: 1.7.0
+     Deprecated the *config* attribute. Some tor configuration options (like
+     ExitPolicy) can have multiple values, so a simple 'str => str' mapping
+     meant that we only provided the last.
+
+  .. versionchanged:: 1.7.0
+     Added the changed and unset attributes.
+
+  :var dict changed: mapping of configuration options to a list of their new
+    values
+  :var list unset: configuration options that have been unset
   """
 
   _SKIP_PARSING = True
   _VERSION_ADDED = stem.version.Requirement.EVENT_CONF_CHANGED
 
   def _parse(self):
-    self.config = {}
+    self.changed = {}
+    self.unset = []
+    self.config = {}  # TODO: remove in stem 2.0
 
     # Skip first and last line since they're the header and footer. For
     # instance...
@@ -592,8 +603,10 @@ class ConfChangedEvent(Event):
     for line in str(self).splitlines()[1:-1]:
       if '=' in line:
         key, value = line.split('=', 1)
+        self.changed.setdefault(key, []).append(value)
       else:
         key, value = line, None
+        self.unset.append(key)
 
       self.config[key] = value
 
diff --git a/test/unit/response/events.py b/test/unit/response/events.py
index 69a3624b..82506e6d 100644
--- a/test/unit/response/events.py
+++ b/test/unit/response/events.py
@@ -252,6 +252,13 @@ CONF_CHANGED_EVENT = """650-CONF_CHANGED
 650 OK
 """
 
+CONF_CHANGED_EVENT_MULTIPLE = """650-CONF_CHANGED
+650-ExitPolicy=accept 34.3.4.5
+650-ExitPolicy=accept 3.4.53.3
+650-MaxCircuitDirtiness=20
+650 OK
+"""
+
 # GUARD events from tor v0.2.1.30.
 
 GUARD_NEW = '650 GUARD ENTRY $36B5DBA788246E8369DBAF58577C6BC044A9A374 NEW'
@@ -863,15 +870,35 @@ class TestEvents(unittest.TestCase):
 
   def test_conf_changed(self):
     event = _get_event(CONF_CHANGED_EVENT)
+    self.assertTrue(isinstance(event, stem.response.events.ConfChangedEvent))
+
+    self.assertEqual({
+      'ExitNodes': ['caerSidi'],
+      'MaxCircuitDirtiness': ['20'],
+    }, event.changed)
 
-    expected_config = {
+    self.assertEqual(['ExitPolicy'], event.unset)
+
+    self.assertEqual({
       'ExitNodes': 'caerSidi',
       'MaxCircuitDirtiness': '20',
       'ExitPolicy': None,
-    }
+    }, event.config)
 
+    event = _get_event(CONF_CHANGED_EVENT_MULTIPLE)
     self.assertTrue(isinstance(event, stem.response.events.ConfChangedEvent))
-    self.assertEqual(expected_config, event.config)
+
+    self.assertEqual({
+      'ExitPolicy': ['accept 34.3.4.5', 'accept 3.4.53.3'],
+      'MaxCircuitDirtiness': ['20'],
+    }, event.changed)
+
+    self.assertEqual([], event.unset)
+
+    self.assertEqual({
+      'ExitPolicy': 'accept 3.4.53.3',  # overwrote with second value
+      'MaxCircuitDirtiness': '20',
+    }, event.config)
 
   def test_descchanged_event(self):
     # all we can check for is that the event is properly parsed as a