sts-sponsors team mailing list archive

[Jacopo Rota <mp+443254@xxxxxxxxxxxxxxxxxx>]

Jacopo Rota has proposed merging ~r00ta/maas-site-manager:MAASENG-1675 into maas-site-manager:main.

Commit message:
Add sorting capabilities to GET sites endpoint 

Requested reviews:
  MAAS Committers (maas-committers)

For more details, see:
https://code.launchpad.net/~r00ta/maas-site-manager/+git/site-manager/+merge/443254

What's included:
- add sorting capabilities to the GET sites endpoint as per https://warthogs.atlassian.net/browse/MAASENG-1675. 

What's not included
- Exception/error response handling as the exception middleware is not implemented yet; i.e. the user specifies a parameter that does not exist, the result would be 500 - Internal server error with no more information. 
-- 
Your team MAAS Committers is requested to review the proposed merge of ~r00ta/maas-site-manager:MAASENG-1675 into maas-site-manager:main.

diff --git a/backend/msm/db/queries.py b/backend/msm/db/queries.py
index 77b173e..c63e8a1 100644
--- a/backend/msm/db/queries.py
+++ b/backend/msm/db/queries.py
@@ -5,24 +5,29 @@ from datetime import (
 )
 from functools import reduce
 from operator import or_
+from re import sub
 from typing import Any
 from uuid import UUID
 
 from sqlalchemy import (
+    asc,
     case,
     ColumnOperators,
     delete,
+    desc,
     func,
     select,
     String,
     Table,
     Text,
+    UnaryExpression,
     update,
 )
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.sql.expression import FromClause
 
 from ..schema import MAX_PAGE_SIZE
+from ..user_api._forms import SiteSortParams
 from ._tables import (
     Site,
     SiteData,
@@ -120,10 +125,51 @@ def filters_from_arguments(
     ]
 
 
+def order_by_from_arguments(
+    table: Table, sort_params: SiteSortParams
+) -> list[UnaryExpression]:
+    """Return the list of ORDER BY clauses to sort the sites.
+    This enables to convert query params such as
+
+      ?sort_by=property1-desc,property2-asc
+
+    to a where clause such as
+
+      ORDER BY property1 DESC, property2 ASC
+
+    :param table: the table to create the ORDER BY clauses for
+    :param sort_params: a comma-separated string of parameters matching the
+                        table's column name. The column names might have a
+                        suffix '-asc' or '-desc' to sort by ascendant or
+                        descendant order. If not specified, the sorting is
+                        ascendant.
+
+    :returns: a list with clauses to order the sites.
+    """
+
+    def build_expr(parameter: str) -> UnaryExpression:
+        order_by_desc: bool = parameter.endswith("-desc")
+        column_name: str = (
+            sub("-desc$", "", parameter)
+            if order_by_desc
+            else sub("-asc$", "", parameter)
+        )
+        column = table.c[column_name]
+        return desc(column) if order_by_desc else asc(column)
+
+    if sort_params.sort_by:
+        return [
+            build_expr(parameter)
+            for parameter in sort_params.sort_by.split(",")
+        ]
+    return []
+
+
 async def get_sites(
     session: AsyncSession,
     offset: int = 0,
     limit: int = MAX_PAGE_SIZE,
+    sort_params: SiteSortParams | None = None,
     city: list[str] | None = None,
     country: list[str] | None = None,
     name: list[str] | None = None,
@@ -145,6 +191,7 @@ async def get_sites(
         url=url,
     )
     filters.append(Site.c.accepted == True)  # noqa
+    order_by = order_by_from_arguments(table=Site, sort_params=sort_params)
     count = await row_count(session, Site, *filters)
     stmt = (
         select(
@@ -193,7 +240,7 @@ async def get_sites(
             Site.join(SiteData, SiteData.c.site_id == Site.c.id, isouter=True)
         )
         .where(*filters)  # type: ignore[arg-type]
-        .order_by(Site.c.id)
+        .order_by(*order_by)
         .limit(limit)
         .offset(offset)
     )
diff --git a/backend/msm/user_api/_forms.py b/backend/msm/user_api/_forms.py
index d6d2cf7..49ebef5 100644
--- a/backend/msm/user_api/_forms.py
+++ b/backend/msm/user_api/_forms.py
@@ -16,6 +16,12 @@ class SiteFilterParams(NamedTuple):
     url: list[str] | None
 
 
+class SiteSortParams(NamedTuple):
+    """Site sorting parameters."""
+
+    sort_by: str | None
+
+
 async def site_filter_parameters(
     city: list[str] | None = Query(default=None, title="Filter for cities"),
     country: list[str]
@@ -39,3 +45,10 @@ async def site_filter_parameters(
         timezone=timezone,
         url=url,
     )
+
+
+async def site_sort_parameters(
+    sort_by: str | None = Query(default=None, title="Sort by properties"),
+) -> SiteSortParams | None:
+    """Return parameters for site filtering."""
+    return SiteSortParams(sort_by=sort_by)
diff --git a/backend/msm/user_api/_handlers.py b/backend/msm/user_api/_handlers.py
index f7835dc..d1201f6 100644
--- a/backend/msm/user_api/_handlers.py
+++ b/backend/msm/user_api/_handlers.py
@@ -22,7 +22,9 @@ from ..schema import (
 from ..settings import SETTINGS
 from ._forms import (
     site_filter_parameters,
+    site_sort_parameters,
     SiteFilterParams,
+    SiteSortParams,
 )
 from ._jwt import (
     authenticate_user,
@@ -52,12 +54,14 @@ async def sites_get(
     authenticated_user: Annotated[User, Depends(get_authenticated_user)],
     pagination_params: PaginationParams = Depends(pagination_params),
     filter_params: SiteFilterParams = Depends(site_filter_parameters),
+    sort_params: SiteSortParams = Depends(site_sort_parameters),
 ) -> SitesGetResponse:
     """Return accepted sites."""
     total, results = await queries.get_sites(
         session,
         offset=pagination_params.offset,
         limit=pagination_params.size,
+        sort_params=sort_params,
         **filter_params._asdict(),
     )
     return SitesGetResponse(
diff --git a/backend/tests/user_api/test_handlers.py b/backend/tests/user_api/test_handlers.py
index b4ae28c..8aea9fb 100644
--- a/backend/tests/user_api/test_handlers.py
+++ b/backend/tests/user_api/test_handlers.py
@@ -4,7 +4,10 @@ from datetime import (
 )
 from typing import Any
 
-from httpx import AsyncClient
+from httpx import (
+    AsyncClient,
+    Response,
+)
 import pytest
 
 from msm import __version__
@@ -180,6 +183,48 @@ class TestSitesHandler:
             "items": [site],
         }
 
+    async def test_get_with_sorting(
+        self, authenticated_user_app_client: AuthAsyncClient, fixture: Fixture
+    ) -> None:
+        def extract_cities(resp: Response):
+            return [site["city"] for site in resp.json()["items"]]
+
+        await fixture.create(
+            "site", [site_details(city="Milan", country="IT")]
+        )
+        await fixture.create(
+            "site", [site_details(city="Paris", country="FR")]
+        )
+        await fixture.create("site", [site_details(city="Rome", country="IT")])
+        await fixture.create(
+            "site", [site_details(city="London", country="GB")]
+        )
+
+        response = await authenticated_user_app_client.get(
+            "/sites", params="sort_by=city-asc"
+        )
+        assert extract_cities(response) == ["London", "Milan", "Paris", "Rome"]
+
+        response = await authenticated_user_app_client.get(
+            "/sites", params="sort_by=city-desc"
+        )
+        assert extract_cities(response) == ["Rome", "Paris", "Milan", "London"]
+
+        response = await authenticated_user_app_client.get(
+            "/sites", params="sort_by=country,city-desc"
+        )
+        assert extract_cities(response) == ["Paris", "London", "Rome", "Milan"]
+
+        response = await authenticated_user_app_client.get(
+            "/sites", params="sort_by=country,city-asc"
+        )
+        assert extract_cities(response) == ["Paris", "London", "Milan", "Rome"]
+
+        response = await authenticated_user_app_client.get(
+            "/sites", params="page=2&size=2&sort_by=country,city-asc"
+        )
+        assert extract_cities(response) == ["Milan", "Rome"]
+
 
 @pytest.mark.asyncio
 class TestPendingSitesHandler: