Add some rest-level unit tests of new 'published' room creation config key

Bug introduced in 92b20713d7
which reversed the comparison when checking if a user existed
in the users table. Added UTs to prevent this happening again.

synapse/config/_base.py

@@ -148,7 +148,7 @@ class Config(object):
             if not config_args.config_path:
                 config_parser.error(
                     "Must supply a config file.\nA config file can be automatically"
-                    " generated using \"--generate-config -h SERVER_NAME"
+                    " generated using \"--generate-config -H SERVER_NAME"
                     " -c CONFIG-FILE\""
                 )
 
@@ -209,7 +209,7 @@ class Config(object):
         if not config_args.config_path:
             config_parser.error(
                 "Must supply a config file.\nA config file can be automatically"
-                " generated using \"--generate-config -h SERVER_NAME"
+                " generated using \"--generate-config -H SERVER_NAME"
                 " -c CONFIG-FILE\""
             )
 

synapse/handlers/appservice.py

@@ -177,7 +177,7 @@ class ApplicationServicesHandler(object):
             return
 
         user_info = yield self.store.get_user_by_id(user_id)
-        if not user_info:
+        if user_info:
             defer.returnValue(False)
             return
 

synapse/handlers/federation.py

@@ -189,7 +189,7 @@ class FederationHandler(BaseHandler):
                 yield self.store.store_room(
                     room_id=event.room_id,
                     room_creator_user_id="",
-                    is_public=False,
+                    published=False,
                 )
             except StoreError:
                 logger.exception("Failed to store room.")
@@ -594,7 +594,7 @@ class FederationHandler(BaseHandler):
                 yield self.store.store_room(
                     room_id=room_id,
                     room_creator_user_id="",
-                    is_public=False
+                    published=False,
                 )
             except:
                 # FIXME

synapse/handlers/message.py

@@ -278,7 +278,7 @@ class MessageHandler(BaseHandler):
             user, pagination_config.get_source_config("presence"), None
         )
 
-        public_room_ids = yield self.store.get_public_room_ids()
+        published_room_ids = yield self.store.get_published_room_ids()
 
         limit = pagin_config.limit
         if limit is None:
@@ -290,7 +290,8 @@ class MessageHandler(BaseHandler):
                 "room_id": event.room_id,
                 "membership": event.membership,
                 "visibility": (
-                    "public" if event.room_id in public_room_ids
+                    # TODO(paul): This should be specified as "published"
+                    "public" if event.room_id in published_room_ids
                     else "private"
                 ),
             }

synapse/handlers/room.py

@@ -77,6 +77,14 @@ class RoomCreationHandler(BaseHandler):
 
         is_public = config.get("visibility", None) == "public"
 
+        # By default, all public-joinable rooms are published. Allow overriding
+        # that decision.
+        # TODO(paul): Specify 'published' key
+        if "published" in config:
+            published = config["published"]
+        else:
+            published = is_public
+
         if room_id:
             # Ensure room_id is the correct type
             room_id_obj = RoomID.from_string(room_id)
@@ -86,7 +94,7 @@ class RoomCreationHandler(BaseHandler):
             yield self.store.store_room(
                 room_id=room_id,
                 room_creator_user_id=user_id,
-                is_public=is_public
+                published=published,
             )
         else:
             # autogen room IDs and try to create it. We may clash, so just
@@ -103,7 +111,7 @@ class RoomCreationHandler(BaseHandler):
                     yield self.store.store_room(
                         room_id=gen_room_id.to_string(),
                         room_creator_user_id=user_id,
-                        is_public=is_public
+                        published=published,
                     )
                     room_id = gen_room_id.to_string()
                     break
@@ -532,8 +540,8 @@ class RoomMemberHandler(BaseHandler):
 class RoomListHandler(BaseHandler):
 
     @defer.inlineCallbacks
-    def get_public_room_list(self):
-        chunk = yield self.store.get_rooms(is_public=True)
+    def get_published_rooms(self):
+        chunk = yield self.store.get_published_rooms()
         results = yield defer.gatherResults(
             [
                 self.store.get_users_in_room(room["room_id"])

synapse/handlers/sync.py

@@ -147,8 +147,7 @@ class SyncHandler(BaseHandler):
             membership_list=[Membership.INVITE, Membership.JOIN]
         )
 
-        # TODO (mjark): Does public mean "published"?
-        published_rooms = yield self.store.get_rooms(is_public=True)
+        published_rooms = yield self.store.get_published_rooms()
         published_room_ids = set(r["room_id"] for r in published_rooms)
 
         rooms = []
@@ -231,8 +230,7 @@ class SyncHandler(BaseHandler):
         rm_handler = self.hs.get_handlers().room_member_handler
         room_ids = yield rm_handler.get_joined_rooms_for_user(sync_config.user)
 
-        # TODO (mjark): Does public mean "published"?
-        published_rooms = yield self.store.get_rooms(is_public=True)
+        published_rooms = yield self.store.get_published_rooms()
         published_room_ids = set(r["room_id"] for r in published_rooms)
 
         room_events, _ = yield self.store.get_room_events_stream(

synapse/notifier.py

@@ -21,7 +21,6 @@ from synapse.types import StreamToken
 import synapse.metrics
 
 import logging
-import time
 
 
 logger = logging.getLogger(__name__)
@@ -47,11 +46,8 @@ class _NotificationListener(object):
     notify the handler it is sufficient to resolve the deferred.
     """
 
-    def __init__(self, deferred, timeout):
+    def __init__(self, deferred):
         self.deferred = deferred
-        self.created = int(time.time() * 1000)
-        self.timeout = timeout
-        self.have_notified = False
 
     def notified(self):
         return self.deferred.called
@@ -59,7 +55,6 @@ class _NotificationListener(object):
     def notify(self, token):
         """ Inform whoever is listening about the new events.
         """
-        self.have_notified = True
         try:
             self.deferred.callback(token)
         except defer.AlreadyCalledError:
@@ -101,10 +96,7 @@ class _NotifierUserStream(object):
             listeners = self.listeners
             self.listeners = set()
             for listener in listeners:
-                try:
-                    listener.notify(self.current_token)
-                except:
-                    logger.exception("Failed to notify listener")
+                listener.notify(self.current_token)
 
     def remove(self, notifier):
         """ Remove this listener from all the indexes in the Notifier
@@ -316,10 +308,10 @@ class Notifier(object):
         else:
             current_token = user_stream.current_token
 
-        listeners = [_NotificationListener(deferred, timeout)]
+        listener = [_NotificationListener(deferred)]
 
         if timeout and not current_token.is_after(from_token):
-            user_stream.listeners.update(listeners)
+            user_stream.listeners.add(listener[0])
 
         if current_token.is_after(from_token):
             result = yield callback(from_token, current_token)
@@ -329,7 +321,7 @@ class Notifier(object):
         timer = [None]
 
         if result:
-            user_stream.listeners.difference_update(listeners)
+            user_stream.listeners.discard(listener[0])
             defer.returnValue(result)
             return
 
@@ -339,9 +331,8 @@ class Notifier(object):
             def _timeout_listener():
                 timed_out[0] = True
                 timer[0] = None
-                user_stream.listeners.difference_update(listeners)
-                for listener in listeners:
-                    listener.notify(current_token)
+                user_stream.listeners.discard(listener[0])
+                listener[0].notify(current_token)
 
             # We create multiple notification listeners so we have to manage
             # canceling the timeout ourselves.
@@ -349,16 +340,12 @@ class Notifier(object):
 
             while not result and not timed_out[0]:
                 new_token = yield deferred
-
+                deferred = defer.Deferred()
+                listener[0] = _NotificationListener(deferred)
+                user_stream.listeners.add(listener[0])
                 result = yield callback(current_token, new_token)
                 current_token = new_token
 
-                if not result:
-                    deferred = defer.Deferred()
-                    listener = _NotificationListener(deferred, timeout)
-                    listeners.append(listener)
-                    user_stream.listeners.add(listener)
-
         if timer[0] is not None:
             try:
                 self.clock.cancel_call_later(timer[0])

synapse/rest/client/v1/room.py

@@ -273,12 +273,13 @@ class JoinRoomAliasServlet(ClientV1RestServlet):
 
 # TODO: Needs unit testing
 class PublicRoomListRestServlet(ClientV1RestServlet):
+    # TODO(paul): Can't rename this now but 'v2 ought to call this publishedRooms
     PATTERN = client_path_pattern("/publicRooms$")
 
     @defer.inlineCallbacks
     def on_GET(self, request):
         handler = self.handlers.room_list_handler
-        data = yield handler.get_public_room_list()
+        data = yield handler.get_published_rooms()
         defer.returnValue((200, data))
 
 

synapse/storage/room.py

@@ -34,14 +34,14 @@ OpsLevel = collections.namedtuple(
 class RoomStore(SQLBaseStore):
 
     @defer.inlineCallbacks
-    def store_room(self, room_id, room_creator_user_id, is_public):
+    def store_room(self, room_id, room_creator_user_id, published):
         """Stores a room.
 
         Args:
             room_id (str): The desired room ID, can be None.
             room_creator_user_id (str): The user ID of the room creator.
-            is_public (bool): True to indicate that this room should appear in
-            public room lists.
+            published (bool): True to indicate that this room should appear in
+                published room lists.
         Raises:
             StoreError if the room could not be stored.
         """
@@ -51,7 +51,8 @@ class RoomStore(SQLBaseStore):
                 {
                     "room_id": room_id,
                     "creator": room_creator_user_id,
-                    "is_public": is_public,
+                    # TODO(paul): rename this table in the SQL schema
+                    "is_public": published,
                 },
                 desc="store_room",
             )
@@ -75,22 +76,20 @@ class RoomStore(SQLBaseStore):
             allow_none=True,
         )
 
-    def get_public_room_ids(self):
+    def get_published_room_ids(self):
         return self._simple_select_onecol(
             table="rooms",
             keyvalues={
                 "is_public": True,
             },
             retcol="room_id",
-            desc="get_public_room_ids",
+            desc="get_published_room_ids",
         )
 
     @defer.inlineCallbacks
-    def get_rooms(self, is_public):
-        """Retrieve a list of all public rooms.
+    def get_published_rooms(self):
+        """Retrieve a list of all published rooms.
 
-        Args:
-            is_public (bool): True if the rooms returned should be public.
         Returns:
             A list of room dicts containing at least a "room_id" key, a
             "topic" key if one is set, and a "name" key if one is set
@@ -119,14 +118,14 @@ class RoomStore(SQLBaseStore):
                 " FROM rooms AS r"
                 " LEFT JOIN (%(topic)s) AS t ON t.room_id = r.room_id"
                 " LEFT JOIN (%(name)s) AS n ON n.room_id = r.room_id"
-                " WHERE r.is_public = ?"
+                " WHERE r.is_public"
                 " GROUP BY r.room_id"
             ) % {
                 "topic": topic_subquery,
                 "name": name_subquery,
             }
 
-            txn.execute(sql, (is_public,))
+            txn.execute(sql)
 
             rows = txn.fetchall()
 
@@ -146,7 +145,7 @@ class RoomStore(SQLBaseStore):
             return rows
 
         rows = yield self.runInteraction(
-            "get_rooms", f
+            "get_published_rooms", f
         )
 
         ret = [

tests/handlers/test_appservice.py

@@ -57,6 +57,49 @@ class AppServiceHandlerTestCase(unittest.TestCase):
             interested_service, event
         )
 
+    @defer.inlineCallbacks
+    def test_query_user_exists_unknown_user(self):
+        user_id = "@someone:anywhere"
+        services = [self._mkservice(is_interested=True)]
+        services[0].is_interested_in_user = Mock(return_value=True)
+        self.mock_store.get_app_services = Mock(return_value=services)
+        self.mock_store.get_user_by_id = Mock(return_value=None)
+
+        event = Mock(
+            sender=user_id,
+            type="m.room.message",
+            room_id="!foo:bar"
+        )
+        self.mock_as_api.push = Mock()
+        self.mock_as_api.query_user = Mock()
+        yield self.handler.notify_interested_services(event)
+        self.mock_as_api.query_user.assert_called_once_with(
+            services[0], user_id
+        )
+
+    @defer.inlineCallbacks
+    def test_query_user_exists_known_user(self):
+        user_id = "@someone:anywhere"
+        services = [self._mkservice(is_interested=True)]
+        services[0].is_interested_in_user = Mock(return_value=True)
+        self.mock_store.get_app_services = Mock(return_value=services)
+        self.mock_store.get_user_by_id = Mock(return_value={
+            "name": user_id
+        })
+
+        event = Mock(
+            sender=user_id,
+            type="m.room.message",
+            room_id="!foo:bar"
+        )
+        self.mock_as_api.push = Mock()
+        self.mock_as_api.query_user = Mock()
+        yield self.handler.notify_interested_services(event)
+        self.assertFalse(
+            self.mock_as_api.query_user.called,
+            "query_user called when it shouldn't have been."
+        )
+
     @defer.inlineCallbacks
     def test_query_room_alias_exists(self):
         room_alias_str = "#foo:bar"

tests/rest/client/v1/test_rooms.py

@@ -558,6 +558,70 @@ class RoomsCreateTestCase(RestTestCase):
         self.assertEquals(200, code)
         self.assertTrue("room_id" in response)
 
+        (code, response) = yield self.mock_resource.trigger(
+            "GET",
+            "/publicRooms",
+            "")
+        self.assertEquals(200, code)
+        self.assertEquals({
+            "start": "START",
+            "end": "END",
+            "chunk": [],
+        }, response)
+
+    @defer.inlineCallbacks
+    def test_post_room_visibility_public_key(self):
+        (code, response) = yield self.mock_resource.trigger(
+            "POST",
+            "/createRoom",
+            '{"visibility":"public", '
+            '"room_alias_name": "my-alias-test"}')
+        self.assertEquals(200, code)
+        self.assertTrue("room_id" in response)
+
+        room_id = response["room_id"]
+
+        (code, response) = yield self.mock_resource.trigger(
+            "GET",
+            "/publicRooms",
+            "")
+        self.assertEquals(200, code)
+        self.assertEquals({
+            "chunk": [
+                {
+                    "room_id": room_id,
+                    "name": None,
+                    "topic": None,
+                    "num_joined_members": 1,
+                    "aliases": ["#my-alias-test:red"],
+                },
+            ],
+            "start": "START",
+            "end": "END",
+        }, response)
+
+    @defer.inlineCallbacks
+    def test_post_room_visibility_public_unpublished_key(self):
+        (code, response) = yield self.mock_resource.trigger(
+            "POST",
+            "/createRoom",
+            '{"visibility":"public", '
+            '"room_alias_name": "my-alias-test", '
+            '"published": false}')
+        self.assertEquals(200, code)
+        self.assertTrue("room_id" in response)
+
+        (code, response) = yield self.mock_resource.trigger(
+            "GET",
+            "/publicRooms",
+            "")
+        self.assertEquals(200, code)
+        self.assertEquals({
+            "chunk": [],
+            "start": "START",
+            "end": "END",
+        }, response)
+
     @defer.inlineCallbacks
     def test_post_room_custom_key(self):
         # POST with custom config keys, expect new room id

tests/storage/test_room.py

@@ -39,7 +39,7 @@ class RoomStoreTestCase(unittest.TestCase):
 
         yield self.store.store_room(self.room.to_string(),
             room_creator_user_id=self.u_creator.to_string(),
-            is_public=True
+            published=True,
         )
 
     @defer.inlineCallbacks
@@ -52,10 +52,10 @@ class RoomStoreTestCase(unittest.TestCase):
         )
 
     @defer.inlineCallbacks
-    def test_get_rooms(self):
-        # get_rooms does an INNER JOIN on the room_aliases table :(
+    def test_get_published_rooms(self):
+        # get_published_rooms does an INNER JOIN on the room_aliases table :(
 
-        rooms = yield self.store.get_rooms(is_public=True)
+        rooms = yield self.store.get_published_rooms()
         # Should be empty before we add the alias
         self.assertEquals([], rooms)
 
@@ -65,7 +65,7 @@ class RoomStoreTestCase(unittest.TestCase):
             servers=["test"]
         )
 
-        rooms = yield self.store.get_rooms(is_public=True)
+        rooms = yield self.store.get_published_rooms()
 
         self.assertEquals(1, len(rooms))
         self.assertEquals({
@@ -91,7 +91,7 @@ class RoomEventsStoreTestCase(unittest.TestCase):
 
         yield self.store.store_room(self.room.to_string(),
             room_creator_user_id="@creator:text",
-            is_public=True
+            published=False,
         )
 
     @defer.inlineCallbacks