Migrate trending_books_api endpoint to FastAPI (#12000)

bhardwajparth51 · RayBB · web-flow · commit 88000ee010aa · 2026-03-23T21:24:36.000-07:00
Co-authored-by: RayBB &lt;RayBB@users.noreply.github.com&gt;
diff --git a/openlibrary/fastapi/internal/api.py b/openlibrary/fastapi/internal/api.py
@@ -9,37 +9,99 @@
 from __future__ import annotations
 
 import os
-from typing import Annotated
+from typing import Annotated, Literal
 
+import web
 from fastapi import APIRouter, Depends, Form, Path, Query
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, BeforeValidator, Field
 
 from openlibrary.core import lending
 from openlibrary.core.models import Booknotes
 from openlibrary.fastapi.auth import AuthenticatedUser, require_authenticated_user
-from openlibrary.fastapi.models import Pagination  # noqa: TC001
+from openlibrary.fastapi.models import Pagination, parse_comma_separated_list
 from openlibrary.utils import extract_numeric_id_from_olid
-
-router = APIRouter()
+from openlibrary.utils.request_context import site as site_ctx
+from openlibrary.views.loanstats import SINCE_DAYS, get_trending_books
 
 SHOW_INTERNAL_IN_SCHEMA = os.getenv("LOCAL_DEV") is not None
+router = APIRouter(tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA)
+
+# Valid period values — mirrors SINCE_DAYS keys
+# IMPORTANT: Keep this Literal in sync with the keys of views.loanstats.SINCE_DAYS!
+# This guarantees that our path parameter validated by FastAPI is always
+# a safe dictionary key when we pass it to the legacy backend function.
+TrendingPeriod = Literal["now", "daily", "weekly", "monthly", "yearly", "forever"]
 
 
-@router.get("/availability/v2", tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA)
+@router.get("/availability/v2")
 async def book_availability():
     pass
 
 
-@router.get("/trending/{period}.json", tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA)
-async def trending_books_api(period: str):
-    pass
+class TrendingRequestParams(Pagination):
+    limit: int = Field(100, ge=0, le=1000, description="Maximum number of results per page.")
+    hours: int = Field(0, ge=0, description="Custom number of hours to look back.")
+    days: int = Field(0, ge=0, description="Custom number of days to look back.")
+    sort_by_count: bool = Field(True, description="Sort results by total log count (most-logged first).")
+    minimum: int = Field(0, ge=0, description="Minimum log count a book must have to be included.")
+    fields: Annotated[list[str] | None, BeforeValidator(parse_comma_separated_list)] = Field(
+        None,
+        description="Comma-separated list of Solr fields to include in each work.",
+    )
+
+
+class SolrWork(BaseModel):
+    key: str
+    title: str | None = None
+    author_name: list[str] | None = None
+
+    model_config = {"extra": "allow"}
+
+
+class TrendingResponse(BaseModel):
+    query: str = Field(..., description="Echo of the request path, e.g. /trending/daily")
+    works: list[SolrWork] = Field(..., description="Trending work documents from Solr")
+    days: int | None = Field(default=None, description="Look-back window in days (None = all-time)")
+    hours: int = Field(..., description="Look-back window in hours")
 
 
 @router.get(
-    "/browse.json",
-    tags=["internal"],
-    include_in_schema=SHOW_INTERNAL_IN_SCHEMA,
+    "/trending/{period}.json",
+    response_model=TrendingResponse,
+    response_model_exclude_none=True,
+    description="Returns works sorted by recent activity (reads, loans, etc.)",
 )
+def trending_books_api(
+    period: Annotated[TrendingPeriod, Path(description="The time period for trending books")],
+    params: Annotated[TrendingRequestParams, Query()],
+) -> TrendingResponse:
+    """Fetch trending books for the given period."""
+    # ``period`` is always a key in SINCE_DAYS — guaranteed by the Literal type above.
+    since_days: int | None = SINCE_DAYS.get(period, params.days)
+
+    # Setting web.ctx.site is an ANTIPATTERN and we should avoid it elsewhere.
+    # It will be removed via #12178
+    web.ctx.site = site_ctx.get()
+
+    works = get_trending_books(
+        since_days=since_days,
+        since_hours=params.hours,
+        limit=params.limit,
+        page=params.page,
+        sort_by_count=params.sort_by_count,
+        minimum=params.minimum,
+        fields=params.fields,
+    )
+
+    return TrendingResponse(
+        query=f"/trending/{period}",
+        works=[SolrWork(**dict(work)) for work in works],
+        days=since_days,
+        hours=params.hours,
+    )
+
+
+@router.get("/browse.json")
 async def browse(
     pagination: Annotated[Pagination, Depends()],
     q: Annotated[str, Query()] = "",
@@ -73,12 +135,7 @@ class BooknoteResponse(BaseModel):
     success: str = Field(..., description="Status message")
 
 
-@router.post(
-    "/works/OL{work_id}W/notes",
-    response_model=BooknoteResponse,
-    tags=["internal"],
-    include_in_schema=SHOW_INTERNAL_IN_SCHEMA,
-)
+@router.post("/works/OL{work_id}W/notes", response_model=BooknoteResponse)
 async def booknotes_post(
     work_id: Annotated[int, Path(gt=0)],
     user: Annotated[AuthenticatedUser, Depends(require_authenticated_user)],
diff --git a/openlibrary/fastapi/models.py b/openlibrary/fastapi/models.py
@@ -30,6 +30,14 @@ class PaginationLimit20(Pagination):
     limit: int = Field(20, ge=0, description="Maximum number of results to return.")
 
 
+def parse_comma_separated_list(v: str | list[str] | None) -> list[str]:
+    if not v:
+        return []
+    if isinstance(v, str):
+        v = [v]
+    return [f.strip() for item in v for f in str(item).split(",") if f.strip()]
+
+
 class SolrInternalsParams(BaseModel):
     """
     Internal Solr query parameters for A/B testing search configurations.
diff --git a/openlibrary/fastapi/search.py b/openlibrary/fastapi/search.py
@@ -22,6 +22,7 @@
     Pagination,
     PaginationLimit20,
     SolrInternalsParams,
+    parse_comma_separated_list,
 )
 from openlibrary.plugins.worksearch.code import (
     default_spellcheck_count,
@@ -97,7 +98,7 @@ def parse_q_string(cls, v: str) -> str:
 
 
 class SearchRequestParams(PublicQueryOptions, Pagination):
-    fields: Annotated[list[str], BeforeValidator(parse_fields_string)] = Field(
+    fields: Annotated[list[str], BeforeValidator(parse_comma_separated_list)] = Field(
         sorted(WorkSearchScheme.default_fetched_fields),
         description="The fields to return.",
     )
@@ -110,12 +111,6 @@ class SearchRequestParams(PublicQueryOptions, Pagination):
         description="The number of spellcheck suggestions.",
     )
 
-    @staticmethod
-    def parse_fields_string(v: str | list[str]) -> list[str]:
-        if isinstance(v, str):
-            v = [v]
-        return [f.strip() for item in v for f in str(item).split(",") if f.strip()]
-
     @staticmethod
     def parse_query_json(v: str) -> dict[str, Any]:
         try:
diff --git a/openlibrary/plugins/openlibrary/api.py b/openlibrary/plugins/openlibrary/api.py
@@ -78,6 +78,7 @@ def get_book_availability(id_type, ids):
             return []
 
 
+@deprecated("migrated to fastapi")
 class trending_books_api(delegate.page):
     path = "/trending(/?.*)"
     # path = "/trending/(now|daily|weekly|monthly|yearly|forever)"
diff --git a/openlibrary/tests/fastapi/test_models.py b/openlibrary/tests/fastapi/test_models.py
@@ -1,4 +1,61 @@
-from openlibrary.fastapi.models import SolrInternalsParams
+import pytest
+
+from openlibrary.fastapi.models import SolrInternalsParams, parse_comma_separated_list
+
+
+class TestParseCommaSeparatedList:
+    """Tests for parse_comma_separated_list utility function.
+
+    This function is used by FastAPI endpoints to parse comma-separated
+    parameters from query strings into a clean list of values.
+    """
+
+    @pytest.mark.parametrize(
+        ("input_val", "expected"),
+        [
+            (None, []),
+            ("", []),
+            ([], []),
+        ],
+    )
+    def test_falsy_inputs_return_empty_list(self, input_val, expected):
+        assert parse_comma_separated_list(input_val) == expected
+
+    @pytest.mark.parametrize(
+        ("input_val", "expected"),
+        [
+            ("key", ["key"]),
+            (["key"], ["key"]),
+        ],
+    )
+    def test_single_item(self, input_val, expected):
+        assert parse_comma_separated_list(input_val) == expected
+
+    def test_multiple_comma_separated_items(self):
+        assert parse_comma_separated_list("key,title,author_name") == ["key", "title", "author_name"]
+
+    def test_whitespace_trimming(self):
+        assert parse_comma_separated_list(" key , title , author_name ") == ["key", "title", "author_name"]
+
+    def test_empty_values_filtered(self):
+        assert parse_comma_separated_list("key,,title") == ["key", "title"]
+
+    def test_list_with_comma_separated_strings(self):
+        assert parse_comma_separated_list(["key", "title,author_name"]) == ["key", "title", "author_name"]
+
+    def test_list_with_whitespace_and_empty_strings(self):
+        assert parse_comma_separated_list([" key ", "", " title "]) == ["key", "title"]
+
+    def test_real_world_complex_case(self):
+        assert parse_comma_separated_list("key,title,subtitle,author_name,author_key,cover_i,cover_edition_key") == [
+            "key",
+            "title",
+            "subtitle",
+            "author_name",
+            "author_key",
+            "cover_i",
+            "cover_edition_key",
+        ]
 
 
 class TestSolrInternalsParams:
diff --git a/openlibrary/tests/fastapi/test_trending.py b/openlibrary/tests/fastapi/test_trending.py
@@ -0,0 +1,155 @@
+"""Tests for the /trending/{period}.json FastAPI endpoint (internal API)."""
+
+import os
+from unittest.mock import patch
+
+import pytest
+
+MOCK_WORKS = [
+    {"key": "/works/OL1W", "title": "Popular Book 1", "author_name": ["Author A"]},
+    {"key": "/works/OL2W", "title": "Popular Book 2", "author_name": ["Author B"]},
+]
+
+
+@pytest.fixture
+def mock_get_trending_books():
+    with patch("openlibrary.fastapi.internal.api.get_trending_books", return_value=MOCK_WORKS) as mock:
+        yield mock
+
+
+class TestTrendingBooksEndpoint:
+    """Tests for GET /trending/{period}.json."""
+
+    @pytest.mark.parametrize(
+        ("period", "expected_days"),
+        [
+            ("daily", 1),
+            ("weekly", 7),
+            ("monthly", 30),
+            ("yearly", 365),
+            ("forever", None),
+            ("now", 0),
+        ],
+    )
+    def test_valid_periods(self, fastapi_client, mock_get_trending_books, period, expected_days):
+        response = fastapi_client.get(f"/trending/{period}.json")
+        response.raise_for_status()
+        data = response.json()
+        assert data["query"] == f"/trending/{period}"
+        # When expected_days is None (forever), 'days' shouldn't be in the response because of response_model_exclude_none=True
+        if expected_days is None:
+            assert "days" not in data
+        else:
+            assert data["days"] == expected_days
+        assert isinstance(data["works"], list)
+
+    def test_empty_works_list(self, fastapi_client, mock_get_trending_books):
+        mock_get_trending_books.return_value = []
+        response = fastapi_client.get("/trending/now.json")
+        response.raise_for_status()
+        assert response.json()["works"] == []
+
+    @pytest.mark.parametrize(
+        ("query_string", "expected_kwargs"),
+        [
+            ("hours=12", {"since_hours": 12}),
+            ("minimum=3", {"minimum": 3}),
+            ("fields=key,title", {"fields": ["key", "title"]}),
+            ("sort_by_count=false", {"sort_by_count": False}),
+            ("page=2&limit=10", {"page": 2, "limit": 10}),
+            ("fields=key,%20title", {"fields": ["key", "title"]}),
+            ("", {"sort_by_count": True, "fields": None}),
+            (
+                "page=3&limit=50&hours=6&sort_by_count=false&minimum=10&fields=key,title",
+                {"page": 3, "limit": 50, "since_hours": 6, "sort_by_count": False, "minimum": 10, "fields": ["key", "title"]},
+            ),
+            (
+                "fields=key,title,subtitle,author_name,author_key,cover_i,cover_edition_key",
+                {"fields": ["key", "title", "subtitle", "author_name", "author_key", "cover_i", "cover_edition_key"]},
+            ),
+            (
+                "fields=key,title,ia,ia_collection",
+                {"fields": ["key", "title", "ia", "ia_collection"]},
+            ),
+        ],
+    )
+    def test_query_params_forwarded(self, fastapi_client, mock_get_trending_books, query_string, expected_kwargs):
+        response = fastapi_client.get(f"/trending/daily.json?{query_string}")
+        response.raise_for_status()
+        for k, v in expected_kwargs.items():
+            assert mock_get_trending_books.call_args.kwargs[k] == v
+
+    @pytest.mark.parametrize(
+        ("url", "description"),
+        [
+            ("/trending/badperiod.json", "invalid period"),
+            ("/trending/daily.json?sort_by_count=maybe", "invalid boolean"),
+            ("/trending/daily.json?page=0", "page zero"),
+            ("/trending/daily.json?limit=1001", "limit exceeds max"),
+        ],
+    )
+    def test_validation_errors(self, fastapi_client, mock_get_trending_books, url, description):
+        assert fastapi_client.get(url).status_code == 422
+        mock_get_trending_books.assert_not_called()
+
+    @pytest.mark.parametrize(
+        ("hours_param", "expected_hours"),
+        [("6", 6), (None, 0)],
+    )
+    def test_hours_in_response(self, fastapi_client, mock_get_trending_books, hours_param, expected_hours):
+        url = "/trending/daily.json"
+        if hours_param:
+            url += f"?hours={hours_param}"
+        response = fastapi_client.get(url)
+        response.raise_for_status()
+        assert response.json()["hours"] == expected_hours
+
+    def test_limit_defaults_to_100(self, fastapi_client, mock_get_trending_books):
+        response = fastapi_client.get("/trending/daily.json")
+        response.raise_for_status()
+        assert mock_get_trending_books.call_args.kwargs["limit"] == 100
+
+    def test_trending_period_literal_matches_since_days(self):
+        from typing import get_args
+
+        from openlibrary.fastapi.internal.api import TrendingPeriod
+        from openlibrary.views.loanstats import SINCE_DAYS
+
+        literal_keys = set(get_args(TrendingPeriod))
+        since_days_keys = set(SINCE_DAYS.keys())
+
+        assert literal_keys == since_days_keys, (
+            "TrendingPeriod Literal must stay in sync with views.loanstats.SINCE_DAYS keys. "
+            f"Missing in Literal: {since_days_keys - literal_keys}. "
+            f"Extra in Literal: {literal_keys - since_days_keys}."
+        )
+
+    def test_works_content_in_response(self, fastapi_client, mock_get_trending_books):
+        response = fastapi_client.get("/trending/daily.json")
+        response.raise_for_status()
+        works = response.json()["works"]
+        assert len(works) == 2
+        assert works[0]["key"] == "/works/OL1W"
+        assert works[1]["key"] == "/works/OL2W"
+
+
+@pytest.mark.skipif(
+    os.getenv("LOCAL_DEV") is None,
+    reason="Trending endpoint is excluded from OpenAPI schema outside LOCAL_DEV (include_in_schema=False)",
+)
+class TestOpenAPIDocumentation:
+    """Verify the trending endpoint is correctly described in the OpenAPI schema."""
+
+    def test_openapi_contains_trending_endpoint(self, fastapi_client):
+        response = fastapi_client.get("/openapi.json")
+        assert response.status_code == 200
+        paths = response.json()["paths"]
+        assert "/trending/{period}.json" in paths
+
+    def test_openapi_trending_params_have_descriptions(self, fastapi_client):
+        response = fastapi_client.get("/openapi.json")
+        assert response.status_code == 200
+        params = response.json()["paths"]["/trending/{period}.json"]["get"]["parameters"]
+        by_name = {p["name"]: p for p in params}
+        assert by_name["period"]["description"]
+        assert by_name["hours"]["description"]
