From f6b4954883146cb0b5e892be48cb4595e4bcd802 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Tue, 22 Dec 2020 12:39:51 +0100
Subject: [PATCH 01/13] Add `order_by` to list users' media admin API

---
 changelog.d/8978.feature                      |   1 +
 docs/admin_api/user_admin_api.rst             |  28 ++-
 synapse/rest/admin/users.py                   |  28 ++-
 .../databases/main/media_repository.py        |  60 ++++-
 tests/rest/admin/test_user.py                 | 217 ++++++++++++++++--
 5 files changed, 308 insertions(+), 26 deletions(-)
 create mode 100644 changelog.d/8978.feature

diff --git a/changelog.d/8978.feature b/changelog.d/8978.feature
new file mode 100644
index 000000000000..225f4d603c98
--- /dev/null
+++ b/changelog.d/8978.feature
@@ -0,0 +1 @@
+Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. This is a breaking change for default sort order to #8647 which was introduced in Synapse v1.23.0. Those who already use this API should check their scripts. Contributed by @dklimpel.
\ No newline at end of file
diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index e4d6f8203b7c..4df7de19a2d5 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -354,8 +354,8 @@ The following fields are returned in the JSON response body:
 List media of an user
 ================================
 Gets a list of all local media that a specific ``user_id`` has created.
-The response is ordered by creation date descending and media ID descending.
-The newest media is on top.
+The response is default ordered by creation date ascending and media ID ascending.
+The oldest media is on top. You can change the order with parameter ``order_by``.
 
 The API is::
 
@@ -412,6 +412,30 @@ The following parameters should be set in the URL:
   denoting the offset in the returned results. This should be treated as an opaque value and
   not explicitly set to anything other than the return value of ``next_token`` from a previous call.
   Defaults to ``0``.
+- ``order_by`` - The method in which to sort the returned list of media.
+  If the ordered field has duplicates, the second order is always by ``media_id`` ascending,
+  which guarantees a complete ordering. Valid values are:
+
+  - ``media_id`` - Media are ordered alphabetically by ``media_id``.
+  - ``upload_name`` - Media are ordered alphabetically by name the media was uploaded with.
+  - ``created_ts`` - Media are ordered by when the content was uploaded in ms.
+    Smallest to largest. This is the default.
+
+  - ``last_access_ts`` - Media are ordered by when the content was last accessed in ms.
+    Smallest to largest.
+
+  - ``media_length`` - Media are ordered by length of the media in bytes.
+    Smallest to largest.
+
+  - ``media_type`` - Media are ordered alphabetically by MIME-type.
+  - ``quarantined_by`` - Media are ordered alphabetically by the user ID that
+    initiated the quarantine request for this media.
+
+  - ``safe_from_quarantine`` - Media are ordered by the status if this media is safe
+    from quarantining.
+
+- ``dir`` - Direction of media order. Either ``f`` for forwards or ``b`` for backwards.
+  Setting this value to ``b`` will reverse the above sort order. Defaults to ``f``.
 
 **Response**
 
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 6658c2da5666..24d9cc410e0a 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -35,6 +35,7 @@
     assert_user_is_admin,
 )
 from synapse.rest.client.v2_alpha._base import client_patterns
+from synapse.storage.databases.main.media_repository import MediaSortOrder
 from synapse.types import JsonDict, UserID
 
 if TYPE_CHECKING:
@@ -816,8 +817,33 @@ async def on_GET(
                 errcode=Codes.INVALID_PARAM,
             )
 
+        order_by = parse_string(
+            request, "order_by", default=MediaSortOrder.CREATED_TS.value
+        )
+        if order_by not in (
+            MediaSortOrder.MEDIA_ID.value,
+            MediaSortOrder.UPLOAD_NAME.value,
+            MediaSortOrder.CREATED_TS.value,
+            MediaSortOrder.LAST_ACCESS_TS.value,
+            MediaSortOrder.MEDIA_LENGTH.value,
+            MediaSortOrder.MEDIA_TYPE.value,
+            MediaSortOrder.QUARANTINED_BY.value,
+            MediaSortOrder.SAFE_FROM_QUARANTINE.value,
+        ):
+            raise SynapseError(
+                400,
+                "Unknown value for order_by: %s" % (order_by,),
+                errcode=Codes.INVALID_PARAM,
+            )
+
+        direction = parse_string(request, "dir", default="f")
+        if direction not in ("f", "b"):
+            raise SynapseError(
+                400, "Unknown direction: %s" % (direction,), errcode=Codes.INVALID_PARAM
+            )
+
         media, total = await self.store.get_local_media_by_user_paginate(
-            start, limit, user_id
+            start, limit, user_id, order_by, direction
         )
 
         ret = {"media": media, "total": total}
diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py
index 4b2f22471894..a1ac0b5a83cc 100644
--- a/synapse/storage/databases/main/media_repository.py
+++ b/synapse/storage/databases/main/media_repository.py
@@ -12,8 +12,10 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+from enum import Enum
 from typing import Any, Dict, Iterable, List, Optional, Tuple
 
+from synapse.api.errors import StoreError
 from synapse.storage._base import SQLBaseStore
 from synapse.storage.database import DatabasePool
 
@@ -22,6 +24,22 @@
 )
 
 
+class MediaSortOrder(Enum):
+    """
+    Enum to define the sorting method used when returning media with
+    get_local_media_by_user_paginate
+    """
+
+    MEDIA_ID = "media_id"
+    UPLOAD_NAME = "upload_name"
+    CREATED_TS = "created_ts"
+    LAST_ACCESS_TS = "last_access_ts"
+    MEDIA_LENGTH = "media_length"
+    MEDIA_TYPE = "media_type"
+    QUARANTINED_BY = "quarantined_by"
+    SAFE_FROM_QUARANTINE = "safe_from_quarantine"
+
+
 class MediaRepositoryBackgroundUpdateStore(SQLBaseStore):
     def __init__(self, database: DatabasePool, db_conn, hs):
         super().__init__(database, db_conn, hs)
@@ -117,7 +135,12 @@ async def get_local_media(self, media_id: str) -> Optional[Dict[str, Any]]:
         )
 
     async def get_local_media_by_user_paginate(
-        self, start: int, limit: int, user_id: str
+        self,
+        start: int,
+        limit: int,
+        user_id: str,
+        order_by: MediaSortOrder = MediaSortOrder.CREATED_TS.value,
+        direction: str = "f",
     ) -> Tuple[List[Dict[str, Any]], int]:
         """Get a paginated list of metadata for a local piece of media
         which an user_id has uploaded
@@ -126,6 +149,8 @@ async def get_local_media_by_user_paginate(
             start: offset in the list
             limit: maximum amount of media_ids to retrieve
             user_id: fully-qualified user id
+            order_by: the sort order of the returned list
+            direction: sort ascending or descending
         Returns:
             A paginated list of all metadata of user's media,
             plus the total count of all the user's media
@@ -133,6 +158,33 @@ async def get_local_media_by_user_paginate(
 
         def get_local_media_by_user_paginate_txn(txn):
 
+            # Set ordering
+            if MediaSortOrder(order_by) == MediaSortOrder.MEDIA_ID:
+                order_by_column = "media_id"
+            elif MediaSortOrder(order_by) == MediaSortOrder.UPLOAD_NAME:
+                order_by_column = "upload_name"
+            elif MediaSortOrder(order_by) == MediaSortOrder.CREATED_TS:
+                order_by_column = "created_ts"
+            elif MediaSortOrder(order_by) == MediaSortOrder.LAST_ACCESS_TS:
+                order_by_column = "last_access_ts"
+            elif MediaSortOrder(order_by) == MediaSortOrder.MEDIA_LENGTH:
+                order_by_column = "media_length"
+            elif MediaSortOrder(order_by) == MediaSortOrder.MEDIA_TYPE:
+                order_by_column = "media_type"
+            elif MediaSortOrder(order_by) == MediaSortOrder.QUARANTINED_BY:
+                order_by_column = "quarantined_by"
+            elif MediaSortOrder(order_by) == MediaSortOrder.SAFE_FROM_QUARANTINE:
+                order_by_column = "safe_from_quarantine"
+            else:
+                raise StoreError(
+                    500, "Incorrect value for order_by provided: %s" % order_by
+                )
+
+            if direction == "b":
+                order = "DESC"
+            else:
+                order = "ASC"
+
             args = [user_id]
             sql = """
                 SELECT COUNT(*) as total_media
@@ -154,9 +206,11 @@ def get_local_media_by_user_paginate_txn(txn):
                     "safe_from_quarantine"
                 FROM local_media_repository
                 WHERE user_id = ?
-                ORDER BY created_ts DESC, media_id DESC
+                ORDER BY {order_by_column} {order}, media_id ASC
                 LIMIT ? OFFSET ?
-            """
+            """.format(
+                order_by_column=order_by_column, order=order,
+            )
 
             args += [limit, start]
             txn.execute(sql, args)
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 9b2e4765f69f..7a0ab44764f4 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -18,7 +18,7 @@
 import json
 import urllib.parse
 from binascii import unhexlify
-from typing import Optional
+from typing import List, Optional
 
 from mock import Mock
 
@@ -27,8 +27,10 @@
 from synapse.api.errors import Codes, HttpResponseException, ResourceLimitError
 from synapse.rest.client.v1 import login, logout, profile, room
 from synapse.rest.client.v2_alpha import devices, sync
+from synapse.types import JsonDict
 
 from tests import unittest
+from tests.server import FakeSite, make_request
 from tests.test_utils import make_awaitable
 from tests.unittest import override_config
 
@@ -1460,7 +1462,7 @@ def test_limit(self):
 
         number_media = 20
         other_user_tok = self.login("user", "pass")
-        self._create_media(other_user_tok, number_media)
+        self._create_media_for_user(other_user_tok, number_media)
 
         channel = self.make_request(
             "GET", self.url + "?limit=5", access_token=self.admin_user_tok,
@@ -1479,7 +1481,7 @@ def test_from(self):
 
         number_media = 20
         other_user_tok = self.login("user", "pass")
-        self._create_media(other_user_tok, number_media)
+        self._create_media_for_user(other_user_tok, number_media)
 
         channel = self.make_request(
             "GET", self.url + "?from=5", access_token=self.admin_user_tok,
@@ -1498,7 +1500,7 @@ def test_limit_and_from(self):
 
         number_media = 20
         other_user_tok = self.login("user", "pass")
-        self._create_media(other_user_tok, number_media)
+        self._create_media_for_user(other_user_tok, number_media)
 
         channel = self.make_request(
             "GET", self.url + "?from=5&limit=10", access_token=self.admin_user_tok,
@@ -1510,23 +1512,35 @@ def test_limit_and_from(self):
         self.assertEqual(len(channel.json_body["media"]), 10)
         self._check_fields(channel.json_body["media"])
 
-    def test_limit_is_negative(self):
+    def test_invalid_parameter(self):
         """
-        Testing that a negative limit parameter returns a 400
+        If parameters are invalid, an error is returned.
         """
+        # unkown order_by
+        channel = self.make_request(
+            "GET", self.url + "?order_by=bar", access_token=self.admin_user_tok,
+        )
 
+        self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
+
+        # invalid search order
         channel = self.make_request(
-            "GET", self.url + "?limit=-5", access_token=self.admin_user_tok,
+            "GET", self.url + "?dir=bar", access_token=self.admin_user_tok,
         )
 
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
         self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
 
-    def test_from_is_negative(self):
-        """
-        Testing that a negative from parameter returns a 400
-        """
+        # negative limit
+        channel = self.make_request(
+            "GET", self.url + "?limit=-5", access_token=self.admin_user_tok,
+        )
+
+        self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
+        self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
 
+        # negative from
         channel = self.make_request(
             "GET", self.url + "?from=-5", access_token=self.admin_user_tok,
         )
@@ -1541,7 +1555,7 @@ def test_next_token(self):
 
         number_media = 20
         other_user_tok = self.login("user", "pass")
-        self._create_media(other_user_tok, number_media)
+        self._create_media_for_user(other_user_tok, number_media)
 
         #  `next_token` does not appear
         # Number of results is the number of entries
@@ -1607,7 +1621,7 @@ def test_get_media(self):
 
         number_media = 5
         other_user_tok = self.login("user", "pass")
-        self._create_media(other_user_tok, number_media)
+        self._create_media_for_user(other_user_tok, number_media)
 
         channel = self.make_request("GET", self.url, access_token=self.admin_user_tok,)
 
@@ -1617,11 +1631,106 @@ def test_get_media(self):
         self.assertNotIn("next_token", channel.json_body)
         self._check_fields(channel.json_body["media"])
 
-    def _create_media(self, user_token, number_media):
+    def test_order_by(self):
+        """
+        Testing order list with parameter `order_by`
+        """
+
+        other_user_tok = self.login("user", "pass")
+
+        # Resolution: 1×1, MIME type: image/png, Extension: png, Size: 67 B
+        image_data1 = unhexlify(
+            b"89504e470d0a1a0a0000000d4948445200000001000000010806"
+            b"0000001f15c4890000000a49444154789c63000100000500010d"
+            b"0a2db40000000049454e44ae426082"
+        )
+        # Resolution: 1×1, MIME type: image/gif, Extension: gif, Size: 35 B
+        image_data2 = unhexlify(
+            b"47494638376101000100800100000000"
+            b"ffffff2c00000000010001000002024c"
+            b"01003b"
+        )
+        # Resolution: 1×1, MIME type: image/bmp, Extension: bmp, Size: 54 B
+        image_data3 = unhexlify(
+            b"424d3a0000000000000036000000280000000100000001000000"
+            b"0100180000000000040000000000000000000000000000000000"
+            b"0000"
+        )
+
+        # create media and make sure they do not have the same timestamp
+        media1 = self._create_media_and_access(other_user_tok, image_data1, "image.png")
+        self.pump(1.0)
+        media2 = self._create_media_and_access(other_user_tok, image_data2, "image.gif")
+        self.pump(1.0)
+        media3 = self._create_media_and_access(other_user_tok, image_data3, "image.bmp")
+        self.pump(1.0)
+
+        # Mark one media as safe from quarantine.
+        self.get_success(self.store.mark_local_media_as_safe(media2))
+        # Quarantine one media
+        self.get_success(
+            self.store.quarantine_media_by_id("test", media3, self.admin_user)
+        )
+
+        # sort by media_id
+        sorted_media = sorted([media1, media2, media3], reverse=False)
+        sorted_media_reverse = sorted(sorted_media, reverse=True)
+
+        # order by media_id
+        self._order_test("media_id", sorted_media)
+        self._order_test("media_id", sorted_media, "f")
+        self._order_test("media_id", sorted_media_reverse, "b")
+
+        # order by upload_name
+        self._order_test("upload_name", [media3, media2, media1])
+        self._order_test("upload_name", [media3, media2, media1], "f")
+        self._order_test("upload_name", [media1, media2, media3], "b")
+
+        # order by media_type
+        # result is ordered by media_id
+        # because of uploaded media_type is always 'application/json'
+        self._order_test("media_type", sorted_media)
+        self._order_test("media_type", sorted_media, "f")
+        self._order_test("media_type", sorted_media, "b")
+
+        # order by media_length
+        self._order_test("media_length", [media2, media3, media1])
+        self._order_test("media_length", [media2, media3, media1], "f")
+        self._order_test("media_length", [media1, media3, media2], "b")
+
+        # order by created_ts
+        self._order_test("created_ts", [media1, media2, media3])
+        self._order_test("created_ts", [media1, media2, media3], "f")
+        self._order_test("created_ts", [media3, media2, media1], "b")
+
+        # order by last_access_ts
+        self._order_test("last_access_ts", [media1, media2, media3])
+        self._order_test("last_access_ts", [media1, media2, media3], "f")
+        self._order_test("last_access_ts", [media3, media2, media1], "b")
+
+        # order by quarantined_by
+        # one media is in quarantine, others are ordered by media_ids
+        self._order_test("quarantined_by", sorted([media1, media2]) + [media3])
+        self._order_test("quarantined_by", sorted([media1, media2]) + [media3], "f")
+        self._order_test("quarantined_by", [media3] + sorted([media1, media2]), "b")
+
+        # order by safe_from_quarantine
+        # one media is safe from quarantine, others are ordered by media_ids
+        self._order_test("safe_from_quarantine", sorted([media1, media3]) + [media2])
+        self._order_test(
+            "safe_from_quarantine", sorted([media1, media3]) + [media2], "f"
+        )
+        self._order_test(
+            "safe_from_quarantine", [media2] + sorted([media1, media3]), "b"
+        )
+
+    def _create_media_for_user(self, user_token: str, number_media: int):
         """
         Create a number of media for a specific user
+        Args:
+            user_token: Access token of the user
+            number_media: Number of media to be created for the user
         """
-        upload_resource = self.media_repo.children[b"upload"]
         for i in range(number_media):
             # file size is 67 Byte
             image_data = unhexlify(
@@ -1630,13 +1739,56 @@ def _create_media(self, user_token, number_media):
                 b"0a2db40000000049454e44ae426082"
             )
 
-            # Upload some media into the room
-            self.helper.upload_media(
-                upload_resource, image_data, tok=user_token, expect_code=200
-            )
+            self._create_media_and_access(user_token, image_data)
+
+    def _create_media_and_access(
+        self, user_token: str, image_data: bytes, filename: str = "image1.png",
+    ) -> str:
+        """
+        Create one media for a specific user, access and returns `media_id`
+        Args:
+            user_token: Access token of the user
+            image_data: binary data of image
+            filename: The filename of the media to be uploaded
+        Returns:
+            The ID of the newly created media.
+        """
+        upload_resource = self.media_repo.children[b"upload"]
+        download_resource = self.media_repo.children[b"download"]
+
+        # Upload some media into the room
+        response = self.helper.upload_media(
+            upload_resource, image_data, user_token, filename, expect_code=200
+        )
+
+        # Extract media ID from the response
+        server_and_media_id = response["content_uri"][6:]  # Cut off 'mxc://'
+        media_id = server_and_media_id.split("/")[1]
 
-    def _check_fields(self, content):
+        # Try to access a media and to create `last_access_ts`
+        channel = make_request(
+            self.reactor,
+            FakeSite(download_resource),
+            "GET",
+            server_and_media_id,
+            shorthand=False,
+            access_token=user_token,
+        )
+
+        self.assertEqual(
+            200,
+            channel.code,
+            msg=(
+                "Expected to receive a 200 on accessing media: %s" % server_and_media_id
+            ),
+        )
+
+        return media_id
+
+    def _check_fields(self, content: JsonDict):
         """Checks that all attributes are present in content
+        Args:
+            content: List that is checked for content
         """
         for m in content:
             self.assertIn("media_id", m)
@@ -1648,6 +1800,31 @@ def _check_fields(self, content):
             self.assertIn("quarantined_by", m)
             self.assertIn("safe_from_quarantine", m)
 
+    def _order_test(
+        self, order_type: str, expected_media_list: List[str], dir: Optional[str] = None
+    ):
+        """Request the list of media in a certain order. Assert that order is what
+        we expect
+        Args:
+            order_type: The type of ordering to give the server
+            expected_media_list: The list of media_ids in the order we expect to get
+                back from the server
+            dir: The direction of ordering to give the server
+        """
+
+        url = self.url + "?order_by=%s" % (order_type,)
+        if dir is not None and dir in ("b", "f"):
+            url += "&dir=%s" % (dir,)
+        channel = self.make_request(
+            "GET", url.encode("ascii"), access_token=self.admin_user_tok,
+        )
+        self.assertEqual(200, channel.code, msg=channel.json_body)
+        self.assertEqual(channel.json_body["total"], len(expected_media_list))
+
+        returned_order = [row["media_id"] for row in channel.json_body["media"]]
+        self.assertListEqual(expected_media_list, returned_order)
+        self._check_fields(channel.json_body["media"])
+
 
 class UserTokenRestTestCase(unittest.HomeserverTestCase):
     """Test for /_synapse/admin/v1/users/<user>/login

From 23a731e6340ea8a7b008fffb821dfc7e7162fc86 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Tue, 22 Dec 2020 14:02:57 +0100
Subject: [PATCH 02/13] update test - mismatch SQlite and PostreSQL

---
 tests/rest/admin/test_user.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 7a0ab44764f4..60034b747d1f 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -1710,9 +1710,10 @@ def test_order_by(self):
 
         # order by quarantined_by
         # one media is in quarantine, others are ordered by media_ids
-        self._order_test("quarantined_by", sorted([media1, media2]) + [media3])
-        self._order_test("quarantined_by", sorted([media1, media2]) + [media3], "f")
-        self._order_test("quarantined_by", [media3] + sorted([media1, media2]), "b")
+        # Different sort order of SQlite and PostreSQL foor bool
+        # self._order_test("quarantined_by", sorted([media1, media2]) + [media3])
+        # self._order_test("quarantined_by", sorted([media1, media2]) + [media3], "f")
+        # self._order_test("quarantined_by", [media3] + sorted([media1, media2]), "b")
 
         # order by safe_from_quarantine
         # one media is safe from quarantine, others are ordered by media_ids

From e0b5345ac48a30b04f074368d6f06bb32b5d8f45 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Mon, 4 Jan 2021 11:43:10 +0100
Subject: [PATCH 03/13] Change default sort order to prevent breaking change

---
 changelog.d/8978.feature          |  2 +-
 docs/admin_api/user_admin_api.rst |  6 +++++-
 synapse/rest/admin/users.py       |  9 +++++++++
 tests/rest/admin/test_user.py     | 17 ++++++++++++++---
 4 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/changelog.d/8978.feature b/changelog.d/8978.feature
index 225f4d603c98..042e257bf06a 100644
--- a/changelog.d/8978.feature
+++ b/changelog.d/8978.feature
@@ -1 +1 @@
-Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. This is a breaking change for default sort order to #8647 which was introduced in Synapse v1.23.0. Those who already use this API should check their scripts. Contributed by @dklimpel.
\ No newline at end of file
+Add `order_by` to the admin API `GET /_synapse/admin/v1/users/<user_id>/media`. Contributed by @dklimpel.
\ No newline at end of file
diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index 4df7de19a2d5..b584ea72c697 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -355,7 +355,8 @@ List media of an user
 ================================
 Gets a list of all local media that a specific ``user_id`` has created.
 The response is default ordered by creation date ascending and media ID ascending.
-The oldest media is on top. You can change the order with parameter ``order_by``.
+The newest media is on top. You can change the order with parameters
+``order_by`` and ``dir``.
 
 The API is::
 
@@ -437,6 +438,9 @@ The following parameters should be set in the URL:
 - ``dir`` - Direction of media order. Either ``f`` for forwards or ``b`` for backwards.
   Setting this value to ``b`` will reverse the above sort order. Defaults to ``f``.
 
+If neither ``order_by`` nor ``dir`` is set, the default order is newest media on top
+(corresponds to ``order_by`` = ``created_ts`` and ``dir`` = ``b``).
+
 **Response**
 
 The following fields are returned in the JSON response body:
diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 24d9cc410e0a..7fb0e01b27d6 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -842,6 +842,15 @@ async def on_GET(
                 400, "Unknown direction: %s" % (direction,), errcode=Codes.INVALID_PARAM
             )
 
+        # If neither `order_by` nor `dir` is set, set the default order
+        # to newest media is on top for backward compatibility.
+        if (
+            parse_string(request, "order_by") is None
+            and parse_string(request, "dir") is None
+        ):
+            order_by = MediaSortOrder.CREATED_TS.value
+            direction = "b"
+
         media, total = await self.store.get_local_media_by_user_paginate(
             start, limit, user_id, order_by, direction
         )
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 60034b747d1f..d30e3b3a3d66 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -1672,6 +1672,12 @@ def test_order_by(self):
             self.store.quarantine_media_by_id("test", media3, self.admin_user)
         )
 
+        # order by default ("created_ts")
+        # default is backwards
+        self._order_test(None, [media3, media2, media1])
+        self._order_test(None, [media1, media2, media3], "f")
+        self._order_test(None, [media3, media2, media1], "b")
+
         # sort by media_id
         sorted_media = sorted([media1, media2, media3], reverse=False)
         sorted_media_reverse = sorted(sorted_media, reverse=True)
@@ -1802,7 +1808,10 @@ def _check_fields(self, content: JsonDict):
             self.assertIn("safe_from_quarantine", m)
 
     def _order_test(
-        self, order_type: str, expected_media_list: List[str], dir: Optional[str] = None
+        self,
+        order_type: Optional[str],
+        expected_media_list: List[str],
+        dir: Optional[str] = None,
     ):
         """Request the list of media in a certain order. Assert that order is what
         we expect
@@ -1813,9 +1822,11 @@ def _order_test(
             dir: The direction of ordering to give the server
         """
 
-        url = self.url + "?order_by=%s" % (order_type,)
+        url = self.url + "?"
+        if order_type is not None:
+            url += "order_by=%s&" % (order_type,)
         if dir is not None and dir in ("b", "f"):
-            url += "&dir=%s" % (dir,)
+            url += "dir=%s" % (dir,)
         channel = self.make_request(
             "GET", url.encode("ascii"), access_token=self.admin_user_tok,
         )

From c8f1904f410f6ef2fbab3514f138e89b3154ccdb Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Mon, 11 Jan 2021 15:05:56 +0100
Subject: [PATCH 04/13] Add performance warning

---
 docs/admin_api/user_admin_api.rst | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index b584ea72c697..4a3bf4c83b26 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -352,9 +352,9 @@ The following fields are returned in the JSON response body:
 
 
 List media of an user
-================================
+=====================
 Gets a list of all local media that a specific ``user_id`` has created.
-The response is default ordered by creation date ascending and media ID ascending.
+The response is default ordered by creation date descending and media ID ascending.
 The newest media is on top. You can change the order with parameters
 ``order_by`` and ``dir``.
 
@@ -421,17 +421,13 @@ The following parameters should be set in the URL:
   - ``upload_name`` - Media are ordered alphabetically by name the media was uploaded with.
   - ``created_ts`` - Media are ordered by when the content was uploaded in ms.
     Smallest to largest. This is the default.
-
   - ``last_access_ts`` - Media are ordered by when the content was last accessed in ms.
     Smallest to largest.
-
   - ``media_length`` - Media are ordered by length of the media in bytes.
     Smallest to largest.
-
   - ``media_type`` - Media are ordered alphabetically by MIME-type.
   - ``quarantined_by`` - Media are ordered alphabetically by the user ID that
     initiated the quarantine request for this media.
-
   - ``safe_from_quarantine`` - Media are ordered by the status if this media is safe
     from quarantining.
 
@@ -441,6 +437,12 @@ The following parameters should be set in the URL:
 If neither ``order_by`` nor ``dir`` is set, the default order is newest media on top
 (corresponds to ``order_by`` = ``created_ts`` and ``dir`` = ``b``).
 
+Caution. The database has only indexes in the following columns ``media_id``,
+``user_id`` and ``created_ts``. This means that if a different sort order is used
+(``upload_name``, ``last_access_ts``, ``media_length``, ``media_type``,
+``quarantined_by`` or ``safe_from_quarantine``), this can cause a large load on the
+database, especially for large environments.
+
 **Response**
 
 The following fields are returned in the JSON response body:

From 84ab2b676e6c2297201d2f0034b494d0bb65cb87 Mon Sep 17 00:00:00 2001
From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 11 Feb 2021 20:49:27 +0100
Subject: [PATCH 05/13] Apply suggestions from code review - Part 1

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
---
 docs/admin_api/user_admin_api.rst                  | 10 +++++-----
 synapse/storage/databases/main/media_repository.py |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index 4a3bf4c83b26..c4a61456bf17 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -351,10 +351,10 @@ The following fields are returned in the JSON response body:
 - ``total`` - Number of rooms.
 
 
-List media of an user
+List media of a user
 =====================
 Gets a list of all local media that a specific ``user_id`` has created.
-The response is default ordered by creation date descending and media ID ascending.
+By default, the response is ordered by descending creation date and ascending media ID.
 The newest media is on top. You can change the order with parameters
 ``order_by`` and ``dir``.
 
@@ -413,8 +413,8 @@ The following parameters should be set in the URL:
   denoting the offset in the returned results. This should be treated as an opaque value and
   not explicitly set to anything other than the return value of ``next_token`` from a previous call.
   Defaults to ``0``.
-- ``order_by`` - The method in which to sort the returned list of media.
-  If the ordered field has duplicates, the second order is always by ``media_id`` ascending,
+- ``order_by`` - The method by which to sort the returned list of media.
+  If the ordered field has duplicates, the second order is always by ascending ``media_id``,
   which guarantees a complete ordering. Valid values are:
 
   - ``media_id`` - Media are ordered alphabetically by ``media_id``.
@@ -437,7 +437,7 @@ The following parameters should be set in the URL:
 If neither ``order_by`` nor ``dir`` is set, the default order is newest media on top
 (corresponds to ``order_by`` = ``created_ts`` and ``dir`` = ``b``).
 
-Caution. The database has only indexes in the following columns ``media_id``,
+Caution. The database has only indexes on the columns ``media_id``,
 ``user_id`` and ``created_ts``. This means that if a different sort order is used
 (``upload_name``, ``last_access_ts``, ``media_length``, ``media_type``,
 ``quarantined_by`` or ``safe_from_quarantine``), this can cause a large load on the
diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py
index a1ac0b5a83cc..9864f21e5d40 100644
--- a/synapse/storage/databases/main/media_repository.py
+++ b/synapse/storage/databases/main/media_repository.py
@@ -176,8 +176,8 @@ def get_local_media_by_user_paginate_txn(txn):
             elif MediaSortOrder(order_by) == MediaSortOrder.SAFE_FROM_QUARANTINE:
                 order_by_column = "safe_from_quarantine"
             else:
-                raise StoreError(
-                    500, "Incorrect value for order_by provided: %s" % order_by
+                raise ValueError(
+                    "Incorrect value for order_by provided: %s" % (order_by,)
                 )
 
             if direction == "b":

From ef3a96fa56b1df52ec8ba9b666142911652bde78 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 11 Feb 2021 22:02:43 +0100
Subject: [PATCH 06/13] Apply suggestions from code review - Part 2

---
 synapse/rest/admin/users.py                   | 49 ++++++++-----------
 .../databases/main/media_repository.py        |  1 -
 tests/rest/admin/test_user.py                 |  4 +-
 3 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/synapse/rest/admin/users.py b/synapse/rest/admin/users.py
index 7fb0e01b27d6..c4322898cb42 100644
--- a/synapse/rest/admin/users.py
+++ b/synapse/rest/admin/users.py
@@ -817,39 +817,30 @@ async def on_GET(
                 errcode=Codes.INVALID_PARAM,
             )
 
-        order_by = parse_string(
-            request, "order_by", default=MediaSortOrder.CREATED_TS.value
-        )
-        if order_by not in (
-            MediaSortOrder.MEDIA_ID.value,
-            MediaSortOrder.UPLOAD_NAME.value,
-            MediaSortOrder.CREATED_TS.value,
-            MediaSortOrder.LAST_ACCESS_TS.value,
-            MediaSortOrder.MEDIA_LENGTH.value,
-            MediaSortOrder.MEDIA_TYPE.value,
-            MediaSortOrder.QUARANTINED_BY.value,
-            MediaSortOrder.SAFE_FROM_QUARANTINE.value,
-        ):
-            raise SynapseError(
-                400,
-                "Unknown value for order_by: %s" % (order_by,),
-                errcode=Codes.INVALID_PARAM,
-            )
-
-        direction = parse_string(request, "dir", default="f")
-        if direction not in ("f", "b"):
-            raise SynapseError(
-                400, "Unknown direction: %s" % (direction,), errcode=Codes.INVALID_PARAM
-            )
-
         # If neither `order_by` nor `dir` is set, set the default order
         # to newest media is on top for backward compatibility.
-        if (
-            parse_string(request, "order_by") is None
-            and parse_string(request, "dir") is None
-        ):
+        if b"order_by" not in request.args and b"dir" not in request.args:
             order_by = MediaSortOrder.CREATED_TS.value
             direction = "b"
+        else:
+            order_by = parse_string(
+                request,
+                "order_by",
+                default=MediaSortOrder.CREATED_TS.value,
+                allowed_values=(
+                    MediaSortOrder.MEDIA_ID.value,
+                    MediaSortOrder.UPLOAD_NAME.value,
+                    MediaSortOrder.CREATED_TS.value,
+                    MediaSortOrder.LAST_ACCESS_TS.value,
+                    MediaSortOrder.MEDIA_LENGTH.value,
+                    MediaSortOrder.MEDIA_TYPE.value,
+                    MediaSortOrder.QUARANTINED_BY.value,
+                    MediaSortOrder.SAFE_FROM_QUARANTINE.value,
+                ),
+            )
+            direction = parse_string(
+                request, "dir", default="f", allowed_values=("f", "b")
+            )
 
         media, total = await self.store.get_local_media_by_user_paginate(
             start, limit, user_id, order_by, direction
diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py
index 9864f21e5d40..dee789861a37 100644
--- a/synapse/storage/databases/main/media_repository.py
+++ b/synapse/storage/databases/main/media_repository.py
@@ -15,7 +15,6 @@
 from enum import Enum
 from typing import Any, Dict, Iterable, List, Optional, Tuple
 
-from synapse.api.errors import StoreError
 from synapse.storage._base import SQLBaseStore
 from synapse.storage.database import DatabasePool
 
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index d30e3b3a3d66..cd7f054e86fc 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -1522,7 +1522,7 @@ def test_invalid_parameter(self):
         )
 
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
-        self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
+        self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
 
         # invalid search order
         channel = self.make_request(
@@ -1530,7 +1530,7 @@ def test_invalid_parameter(self):
         )
 
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
-        self.assertEqual(Codes.INVALID_PARAM, channel.json_body["errcode"])
+        self.assertEqual(Codes.UNKNOWN, channel.json_body["errcode"])
 
         # negative limit
         channel = self.make_request(

From cd85a9d59cffed69a37a0155039c2c2859bc814d Mon Sep 17 00:00:00 2001
From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>
Date: Fri, 12 Feb 2021 08:15:34 +0100
Subject: [PATCH 07/13] Add missing `store` to UserMediaRestTestCase

---
 tests/rest/admin/test_user.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index b35aa70344ba..01de1fd99a4b 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -1835,6 +1835,7 @@ class UserMediaRestTestCase(unittest.HomeserverTestCase):
     ]
 
     def prepare(self, reactor, clock, hs):
+        self.store = hs.get_datastore()
         self.media_repo = hs.get_media_repository_resource()
 
         self.admin_user = self.register_user("admin", "pass", admin=True)

From 222af2f2d1535a23ebaf3e3e32ca7e568f519387 Mon Sep 17 00:00:00 2001
From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 14:33:18 +0100
Subject: [PATCH 08/13] Apply suggestions from code review - Part 1

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 docs/admin_api/user_admin_api.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/docs/admin_api/user_admin_api.rst b/docs/admin_api/user_admin_api.rst
index 24e83e71bf95..b7d6d239a040 100644
--- a/docs/admin_api/user_admin_api.rst
+++ b/docs/admin_api/user_admin_api.rst
@@ -377,7 +377,7 @@ The following fields are returned in the JSON response body:
 
 
 List media of a user
-=====================
+====================
 Gets a list of all local media that a specific ``user_id`` has created.
 By default, the response is ordered by descending creation date and ascending media ID.
 The newest media is on top. You can change the order with parameters
@@ -440,7 +440,7 @@ The following parameters should be set in the URL:
   Defaults to ``0``.
 - ``order_by`` - The method by which to sort the returned list of media.
   If the ordered field has duplicates, the second order is always by ascending ``media_id``,
-  which guarantees a complete ordering. Valid values are:
+  which guarantees a stable ordering. Valid values are:
 
   - ``media_id`` - Media are ordered alphabetically by ``media_id``.
   - ``upload_name`` - Media are ordered alphabetically by name the media was uploaded with.
@@ -462,7 +462,7 @@ The following parameters should be set in the URL:
 If neither ``order_by`` nor ``dir`` is set, the default order is newest media on top
 (corresponds to ``order_by`` = ``created_ts`` and ``dir`` = ``b``).
 
-Caution. The database has only indexes on the columns ``media_id``,
+Caution. The database only has indexes on the columns ``media_id``,
 ``user_id`` and ``created_ts``. This means that if a different sort order is used
 (``upload_name``, ``last_access_ts``, ``media_length``, ``media_type``,
 ``quarantined_by`` or ``safe_from_quarantine``), this can cause a large load on the

From 179dc3cddb21e034de3a78950c3f28948ce90552 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 14:56:44 +0100
Subject: [PATCH 09/13] Apply suggestions from code review - Part 2

order test parameter
replace MediaSortOrder block
---
 .../databases/main/media_repository.py        | 21 +-------
 tests/rest/admin/test_user.py                 | 48 +++++++++----------
 2 files changed, 25 insertions(+), 44 deletions(-)

diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py
index d9caa756cae7..7c575ca55c92 100644
--- a/synapse/storage/databases/main/media_repository.py
+++ b/synapse/storage/databases/main/media_repository.py
@@ -159,26 +159,7 @@ async def get_local_media_by_user_paginate(
         def get_local_media_by_user_paginate_txn(txn):
 
             # Set ordering
-            if MediaSortOrder(order_by) == MediaSortOrder.MEDIA_ID:
-                order_by_column = "media_id"
-            elif MediaSortOrder(order_by) == MediaSortOrder.UPLOAD_NAME:
-                order_by_column = "upload_name"
-            elif MediaSortOrder(order_by) == MediaSortOrder.CREATED_TS:
-                order_by_column = "created_ts"
-            elif MediaSortOrder(order_by) == MediaSortOrder.LAST_ACCESS_TS:
-                order_by_column = "last_access_ts"
-            elif MediaSortOrder(order_by) == MediaSortOrder.MEDIA_LENGTH:
-                order_by_column = "media_length"
-            elif MediaSortOrder(order_by) == MediaSortOrder.MEDIA_TYPE:
-                order_by_column = "media_type"
-            elif MediaSortOrder(order_by) == MediaSortOrder.QUARANTINED_BY:
-                order_by_column = "quarantined_by"
-            elif MediaSortOrder(order_by) == MediaSortOrder.SAFE_FROM_QUARANTINE:
-                order_by_column = "safe_from_quarantine"
-            else:
-                raise ValueError(
-                    "Incorrect value for order_by provided: %s" % (order_by,)
-                )
+            order_by_column = MediaSortOrder(order_by).value
 
             if direction == "b":
                 order = "DESC"
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 01de1fd99a4b..786f88216016 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2107,8 +2107,8 @@ def test_order_by(self):
         # order by default ("created_ts")
         # default is backwards
         self._order_test(None, [media3, media2, media1])
-        self._order_test(None, [media1, media2, media3], "f")
-        self._order_test(None, [media3, media2, media1], "b")
+        self._order_test(None, "f", [media1, media2, media3])
+        self._order_test(None, "b", [media3, media2, media1])
 
         # sort by media_id
         sorted_media = sorted([media1, media2, media3], reverse=False)
@@ -2116,51 +2116,51 @@ def test_order_by(self):
 
         # order by media_id
         self._order_test("media_id", sorted_media)
-        self._order_test("media_id", sorted_media, "f")
-        self._order_test("media_id", sorted_media_reverse, "b")
+        self._order_test("media_id", "f", sorted_media)
+        self._order_test("media_id", "b", sorted_media_reverse)
 
         # order by upload_name
         self._order_test("upload_name", [media3, media2, media1])
-        self._order_test("upload_name", [media3, media2, media1], "f")
-        self._order_test("upload_name", [media1, media2, media3], "b")
+        self._order_test("upload_name", "f", [media3, media2, media1])
+        self._order_test("upload_name", "b", [media1, media2, media3])
 
         # order by media_type
         # result is ordered by media_id
         # because of uploaded media_type is always 'application/json'
         self._order_test("media_type", sorted_media)
-        self._order_test("media_type", sorted_media, "f")
-        self._order_test("media_type", sorted_media, "b")
+        self._order_test("media_type", "f", sorted_media)
+        self._order_test("media_type", "b", sorted_media)
 
         # order by media_length
         self._order_test("media_length", [media2, media3, media1])
-        self._order_test("media_length", [media2, media3, media1], "f")
-        self._order_test("media_length", [media1, media3, media2], "b")
+        self._order_test("media_length", "f", [media2, media3, media1])
+        self._order_test("media_length", "b", [media1, media3, media2])
 
         # order by created_ts
         self._order_test("created_ts", [media1, media2, media3])
-        self._order_test("created_ts", [media1, media2, media3], "f")
-        self._order_test("created_ts", [media3, media2, media1], "b")
+        self._order_test("created_ts", "f", [media1, media2, media3])
+        self._order_test("created_ts", "b", [media3, media2, media1])
 
         # order by last_access_ts
         self._order_test("last_access_ts", [media1, media2, media3])
-        self._order_test("last_access_ts", [media1, media2, media3], "f")
-        self._order_test("last_access_ts", [media3, media2, media1], "b")
+        self._order_test("last_access_ts", "f", [media1, media2, media3])
+        self._order_test("last_access_ts", "b", [media3, media2, media1])
 
         # order by quarantined_by
         # one media is in quarantine, others are ordered by media_ids
         # Different sort order of SQlite and PostreSQL foor bool
         # self._order_test("quarantined_by", sorted([media1, media2]) + [media3])
-        # self._order_test("quarantined_by", sorted([media1, media2]) + [media3], "f")
-        # self._order_test("quarantined_by", [media3] + sorted([media1, media2]), "b")
+        # self._order_test("quarantined_by", "f", sorted([media1, media2]) + [media3])
+        # self._order_test("quarantined_by", "b", [media3] + sorted([media1, media2]))
 
         # order by safe_from_quarantine
         # one media is safe from quarantine, others are ordered by media_ids
         self._order_test("safe_from_quarantine", sorted([media1, media3]) + [media2])
         self._order_test(
-            "safe_from_quarantine", sorted([media1, media3]) + [media2], "f"
+            "safe_from_quarantine", "f", sorted([media1, media3]) + [media2]
         )
         self._order_test(
-            "safe_from_quarantine", [media2] + sorted([media1, media3]), "b"
+            "safe_from_quarantine", "b", [media2] + sorted([media1, media3])
         )
 
     def _create_media_for_user(self, user_token: str, number_media: int):
@@ -2241,22 +2241,22 @@ def _check_fields(self, content: JsonDict):
 
     def _order_test(
         self,
-        order_type: Optional[str],
-        expected_media_list: List[str],
+        order_by: Optional[str],
         dir: Optional[str] = None,
+        expected_media_list: List[str],
     ):
         """Request the list of media in a certain order. Assert that order is what
         we expect
         Args:
-            order_type: The type of ordering to give the server
+            order_by: The type of ordering to give the server
+            dir: The direction of ordering to give the server
             expected_media_list: The list of media_ids in the order we expect to get
                 back from the server
-            dir: The direction of ordering to give the server
         """
 
         url = self.url + "?"
-        if order_type is not None:
-            url += "order_by=%s&" % (order_type,)
+        if order_by is not None:
+            url += "order_by=%s&" % (order_by,)
         if dir is not None and dir in ("b", "f"):
             url += "dir=%s" % (dir,)
         channel = self.make_request(

From 50e6c679d4c7696e6ebe05986e3b0183b750ad24 Mon Sep 17 00:00:00 2001
From: Dirk Klimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 15:19:00 +0100
Subject: [PATCH 10/13] Update tests/rest/admin/test_user.py

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
---
 tests/rest/admin/test_user.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 786f88216016..9331181804c9 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2266,7 +2266,7 @@ def _order_test(
         self.assertEqual(channel.json_body["total"], len(expected_media_list))
 
         returned_order = [row["media_id"] for row in channel.json_body["media"]]
-        self.assertListEqual(expected_media_list, returned_order)
+        self.assertEqual(expected_media_list, returned_order)
         self._check_fields(channel.json_body["media"])
 
 

From 15947d621c0213e0a32af5f84382074daaf9e256 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 16:11:36 +0100
Subject: [PATCH 11/13] re-order test

---
 tests/rest/admin/test_user.py | 62 +++++++++++++++++------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 9331181804c9..dc9aa0b51b2e 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2106,61 +2106,61 @@ def test_order_by(self):
 
         # order by default ("created_ts")
         # default is backwards
-        self._order_test(None, [media3, media2, media1])
-        self._order_test(None, "f", [media1, media2, media3])
-        self._order_test(None, "b", [media3, media2, media1])
+        self._order_test([media3, media2, media1], None)
+        self._order_test([media1, media2, media3], None, "f")
+        self._order_test([media3, media2, media1], None, "b")
 
         # sort by media_id
         sorted_media = sorted([media1, media2, media3], reverse=False)
         sorted_media_reverse = sorted(sorted_media, reverse=True)
 
         # order by media_id
-        self._order_test("media_id", sorted_media)
-        self._order_test("media_id", "f", sorted_media)
-        self._order_test("media_id", "b", sorted_media_reverse)
+        self._order_test(sorted_media, "media_id")
+        self._order_test(sorted_media, "media_id", "f")
+        self._order_test(sorted_media_reverse, "media_id", "b")
 
         # order by upload_name
-        self._order_test("upload_name", [media3, media2, media1])
-        self._order_test("upload_name", "f", [media3, media2, media1])
-        self._order_test("upload_name", "b", [media1, media2, media3])
+        self._order_test([media3, media2, media1], "upload_name")
+        self._order_test([media3, media2, media1], "upload_name", "f")
+        self._order_test([media1, media2, media3], "upload_name", "b")
 
         # order by media_type
         # result is ordered by media_id
         # because of uploaded media_type is always 'application/json'
-        self._order_test("media_type", sorted_media)
-        self._order_test("media_type", "f", sorted_media)
-        self._order_test("media_type", "b", sorted_media)
+        self._order_test(sorted_media, "media_type")
+        self._order_test(sorted_media, "media_type", "f")
+        self._order_test(sorted_media, "media_type", "b")
 
         # order by media_length
-        self._order_test("media_length", [media2, media3, media1])
-        self._order_test("media_length", "f", [media2, media3, media1])
-        self._order_test("media_length", "b", [media1, media3, media2])
+        self._order_test([media2, media3, media1], "media_length")
+        self._order_test([media2, media3, media1], "media_length", "f")
+        self._order_test([media1, media3, media2], "media_length", "b")
 
         # order by created_ts
-        self._order_test("created_ts", [media1, media2, media3])
-        self._order_test("created_ts", "f", [media1, media2, media3])
-        self._order_test("created_ts", "b", [media3, media2, media1])
+        self._order_test([media1, media2, media3], "created_ts")
+        self._order_test([media1, media2, media3], "created_ts", "f")
+        self._order_test([media3, media2, media1], "created_ts", "b")
 
         # order by last_access_ts
-        self._order_test("last_access_ts", [media1, media2, media3])
-        self._order_test("last_access_ts", "f", [media1, media2, media3])
-        self._order_test("last_access_ts", "b", [media3, media2, media1])
+        self._order_test([media1, media2, media3], "last_access_ts")
+        self._order_test([media1, media2, media3], "last_access_ts", "f")
+        self._order_test([media3, media2, media1], "last_access_ts", "b")
 
         # order by quarantined_by
         # one media is in quarantine, others are ordered by media_ids
-        # Different sort order of SQlite and PostreSQL foor bool
-        # self._order_test("quarantined_by", sorted([media1, media2]) + [media3])
-        # self._order_test("quarantined_by", "f", sorted([media1, media2]) + [media3])
-        # self._order_test("quarantined_by", "b", [media3] + sorted([media1, media2]))
+        # Different sort order of SQlite and PostreSQL for bool
+        # self._order_test(sorted([media1, media2]) + [media3], "quarantined_by")
+        # self._order_test(sorted([media1, media2]) + [media3], "quarantined_by", "f")
+        # self._order_test([media3] + sorted([media1, media2]), "quarantined_by", "b")
 
         # order by safe_from_quarantine
         # one media is safe from quarantine, others are ordered by media_ids
-        self._order_test("safe_from_quarantine", sorted([media1, media3]) + [media2])
+        self._order_test(sorted([media1, media3]) + [media2]), "safe_from_quarantine"
         self._order_test(
-            "safe_from_quarantine", "f", sorted([media1, media3]) + [media2]
+            sorted([media1, media3]) + [media2], "safe_from_quarantine", "f"
         )
         self._order_test(
-            "safe_from_quarantine", "b", [media2] + sorted([media1, media3])
+            [media2] + sorted([media1, media3]), "safe_from_quarantine", "b"
         )
 
     def _create_media_for_user(self, user_token: str, number_media: int):
@@ -2241,17 +2241,17 @@ def _check_fields(self, content: JsonDict):
 
     def _order_test(
         self,
+        expected_media_list: List[str],
         order_by: Optional[str],
         dir: Optional[str] = None,
-        expected_media_list: List[str],
     ):
         """Request the list of media in a certain order. Assert that order is what
         we expect
         Args:
-            order_by: The type of ordering to give the server
-            dir: The direction of ordering to give the server
             expected_media_list: The list of media_ids in the order we expect to get
                 back from the server
+            order_by: The type of ordering to give the server
+            dir: The direction of ordering to give the server
         """
 
         url = self.url + "?"

From fc6c3450eda537cdd86a43a2c98a269f060644e8 Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 16:35:24 +0100
Subject: [PATCH 12/13] fix wrong merge and code style

---
 .../databases/main/media_repository.py        |  3 +-
 tests/rest/admin/test_user.py                 | 35 ++++++++++++-------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/synapse/storage/databases/main/media_repository.py b/synapse/storage/databases/main/media_repository.py
index bffbd044559e..570afbba4f39 100644
--- a/synapse/storage/databases/main/media_repository.py
+++ b/synapse/storage/databases/main/media_repository.py
@@ -190,7 +190,8 @@ def get_local_media_by_user_paginate_txn(txn):
                 ORDER BY {order_by_column} {order}, media_id ASC
                 LIMIT ? OFFSET ?
             """.format(
-                order_by_column=order_by_column, order=order,
+                order_by_column=order_by_column,
+                order=order,
             )
 
             args += [limit, start]
diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 5ab68674e614..2106da6811c5 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2088,7 +2088,9 @@ def test_invalid_parameter(self):
         """
         # unkown order_by
         channel = self.make_request(
-            "GET", self.url + "?order_by=bar", access_token=self.admin_user_tok,
+            "GET",
+            self.url + "?order_by=bar",
+            access_token=self.admin_user_tok,
         )
 
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
@@ -2096,7 +2098,9 @@ def test_invalid_parameter(self):
 
         # invalid search order
         channel = self.make_request(
-            "GET", self.url + "?dir=bar", access_token=self.admin_user_tok,
+            "GET",
+            self.url + "?dir=bar",
+            access_token=self.admin_user_tok,
         )
 
         self.assertEqual(400, int(channel.result["code"]), msg=channel.result["body"])
@@ -2339,7 +2343,10 @@ def _create_media_for_user(self, user_token: str, number_media: int):
             self._create_media_and_access(user_token, image_data)
 
     def _create_media_and_access(
-        self, user_token: str, image_data: bytes, filename: str = "image1.png",
+        self,
+        user_token: str,
+        image_data: bytes,
+        filename: str = "image1.png",
     ) -> str:
         """
         Create one media for a specific user, access and returns `media_id`
@@ -2387,15 +2394,15 @@ def _check_fields(self, content: JsonDict):
         Args:
             content: List that is checked for content
         """
-        for u in content:
-            self.assertIn("name", u)
-            self.assertIn("is_guest", u)
-            self.assertIn("admin", u)
-            self.assertIn("user_type", u)
-            self.assertIn("deactivated", u)
-            self.assertIn("shadow_banned", u)
-            self.assertIn("displayname", u)
-            self.assertIn("avatar_url", u)
+        for m in content:
+            self.assertIn("media_id", m)
+            self.assertIn("media_type", m)
+            self.assertIn("media_length", m)
+            self.assertIn("upload_name", m)
+            self.assertIn("created_ts", m)
+            self.assertIn("last_access_ts", m)
+            self.assertIn("quarantined_by", m)
+            self.assertIn("safe_from_quarantine", m)
 
     def _order_test(
         self,
@@ -2418,7 +2425,9 @@ def _order_test(
         if dir is not None and dir in ("b", "f"):
             url += "dir=%s" % (dir,)
         channel = self.make_request(
-            "GET", url.encode("ascii"), access_token=self.admin_user_tok,
+            "GET",
+            url.encode("ascii"),
+            access_token=self.admin_user_tok,
         )
         self.assertEqual(200, channel.code, msg=channel.json_body)
         self.assertEqual(channel.json_body["total"], len(expected_media_list))

From dd69590f3f7903078014208bce1351154446591f Mon Sep 17 00:00:00 2001
From: dklimpel <5740567+dklimpel@users.noreply.github.com>
Date: Thu, 18 Feb 2021 17:31:00 +0100
Subject: [PATCH 13/13] fix test

---
 tests/rest/admin/test_user.py | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/rest/admin/test_user.py b/tests/rest/admin/test_user.py
index 2106da6811c5..e58d5cf0dbda 100644
--- a/tests/rest/admin/test_user.py
+++ b/tests/rest/admin/test_user.py
@@ -2310,14 +2310,19 @@ def test_order_by(self):
 
         # order by quarantined_by
         # one media is in quarantine, others are ordered by media_ids
-        # Different sort order of SQlite and PostreSQL for bool
+
+        # Different sort order of SQlite and PostreSQL
+        # If a media is not in quarantine `quarantined_by` is NULL
+        # SQLite considers NULL to be smaller than any other value.
+        # PostreSQL considers NULL to be larger than any other value.
+
         # self._order_test(sorted([media1, media2]) + [media3], "quarantined_by")
         # self._order_test(sorted([media1, media2]) + [media3], "quarantined_by", "f")
         # self._order_test([media3] + sorted([media1, media2]), "quarantined_by", "b")
 
         # order by safe_from_quarantine
         # one media is safe from quarantine, others are ordered by media_ids
-        self._order_test(sorted([media1, media3]) + [media2]), "safe_from_quarantine"
+        self._order_test(sorted([media1, media3]) + [media2], "safe_from_quarantine")
         self._order_test(
             sorted([media1, media3]) + [media2], "safe_from_quarantine", "f"
         )