Migrate existing trending_books_api endpoint to FastAPI and added tests

[bhardwajparth51]

Part of #11999

Migrates the /trending/{period}.json endpoint from the legacy web.py to FastAPI at /trending/{period}.json.

Technical

1. Routing: Wired up the endpoint at /trending/{period}.json in openlibrary/fastapi/internal/api.py
2. Validation: Switched to Pydantic and FastAPI's Query annotations so params like hours, page, and limit are bounds-checked and cast to the right types automatically — no manual parsing needed
3. Response Modeling: Added a SolrWork Pydantic model and used response_model_exclude_none=True to match the legacy output faithfully, including intentionally dropping days from the forever response
4. Error Handling: Wrapped the asyncio.to_thread(get_trending_books, ...) call in a try/except so any database failure comes back as a clean 502 Bad Gateway instead of a raw 500
5. Testing: Moved to the shared fastapi_client fixture from conftest.py and leaned heavily on @pytest.mark.parametrize to cover all periods and query param combinations without repetition. Also added a test_trending_comparison.sh integration script to verify 1:1 parity with the legacy endpoint

Testing

1. Run the unit tests locally via Docker:

docker compose exec web pytest openlibrary/tests/fastapi/test_trending.py -v

2. Optionally, run the parity script against both servers to confirm the responses match:

./tests/integration/temporary/test_trending_comparison.sh http://localhost:8080 http://localhost:18080

Stakeholders

@RayBB

[RayBB]

It should be the exact same url don't prepend internal.

[bhardwajparth51]

Fixed - removed the /internal prefix from the router. URL is now /trending/{period}.json

[RayBB]

It's a good start. Left some initial feedback.

[RayBB]

As a note to myself here's a list of endpoints to test when I test this in the future

Trending Endpoint Test URLs

Trending API (JSON)

Basic Period Tests

• http://localhost:8080/trending/now.json
• http://localhost:8080/trending/daily.json
• http://localhost:8080/trending/weekly.json
• http://localhost:8080/trending/monthly.json
• http://localhost:8080/trending/yearly.json
• http://localhost:8080/trending/forever.json

Limit Parameter Tests

• http://localhost:8080/trending/daily.json?limit=10
• http://localhost:8080/trending/daily.json?limit=50
• http://localhost:8080/trending/daily.json?limit=100
• http://localhost:8080/trending/weekly.json?limit=20
• http://localhost:8080/trending/monthly.json?limit=5

Page Parameter Tests

• http://localhost:8080/trending/daily.json?page=1
• http://localhost:8080/trending/daily.json?page=2
• http://localhost:8080/trending/daily.json?page=3
• http://localhost:8080/trending/weekly.json?page=1&limit=50

Sort By Count Tests

• http://localhost:8080/trending/daily.json?sort_by_count=true
• http://localhost:8080/trending/daily.json?sort_by_count=false
• http://localhost:8080/trending/weekly.json?sort_by_count=true

Minimum Count Filter Tests

• http://localhost:8080/trending/daily.json?minimum=5
• http://localhost:8080/trending/daily.json?minimum=10
• http://localhost:8080/trending/daily.json?minimum=50
• http://localhost:8080/trending/weekly.json?minimum=20

Custom Days Override Tests

• http://localhost:8080/trending/daily.json?days=3
• http://localhost:8080/trending/daily.json?days=14
• http://localhost:8080/trending/weekly.json?days=30

Custom Hours Override Tests

• http://localhost:8080/trending/now.json?hours=1
• http://localhost:8080/trending/now.json?hours=6
• http://localhost:8080/trending/now.json?hours=12

Days and Hours Combined Tests

• http://localhost:8080/trending/daily.json?days=1&hours=6

Fields Parameter Tests

Minimal Fields

• http://localhost:8080/trending/daily.json?fields=key
• http://localhost:8080/trending/daily.json?fields=key,title

Title and Author Fields

• http://localhost:8080/trending/daily.json?fields=key,title,author_name,author_key

Cover Image Fields

• http://localhost:8080/trending/daily.json?fields=key,title,cover_i,cover_edition_key

Edition Fields

• http://localhost:8080/trending/daily.json?fields=key,title,edition_count

Publishing Information

• http://localhost:8080/trending/daily.json?fields=key,title,first_publish_year

Language Fields

• http://localhost:8080/trending/daily.json?fields=key,title,language

eBook and Availability Fields

• http://localhost:8080/trending/daily.json?fields=key,title,has_fulltext,ebook_access

Series Information

• http://localhost:8080/trending/daily.json?fields=key,title,series_key,series_name,series_position

Ratings Fields

• http://localhost:8080/trending/daily.json?fields=key,title,ratings_average,ratings_count,want_to_read_count

External ID Fields

• http://localhost:8080/trending/daily.json?fields=key,title,id_project_gutenberg,id_librivox,id_standard_ebooks

Comprehensive Field Set

• http://localhost:8080/trending/daily.json?fields=key,title,subtitle,author_name,author_key,cover_i,cover_edition_key,edition_count,first_publish_year,language,has_fulltext,ebook_access,ratings_average,ratings_count,want_to_read_count

Combined Parameter Tests

• http://localhost:8080/trending/daily.json?limit=20&page=2&minimum=5&sort_by_count=true
• http://localhost:8080/trending/weekly.json?limit=30&page=1&minimum=10&fields=key,title,author_name,cover_i
• http://localhost:8080/trending/monthly.json?limit=50&page=1&sort_by_count=false&fields=key,title,ratings_average,ratings_count

Edge Case Tests

• http://localhost:8080/trending/now.json?limit=1
• http://localhost:8080/trending/daily.json?minimum=1000
• http://localhost:8080/trending/forever.json?limit=100&page=1
• http://localhost:8080/trending/now.json?hours=24

Special Field Combinations

• http://localhost:8080/trending/daily.json?fields=key,title,public_scan_b,lending_edition_s,lending_identifier_s
• http://localhost:8080/trending/daily.json?fields=key,title,ia,ia_collection

Testing Notes

Available Period Values

• now - 0 days (uses hours if specified)
• daily - 1 day
• weekly - 7 days
• monthly - 30 days
• yearly - 365 days
• forever - all time

Query Parameters

• page - Page number for pagination (default: 1)
• limit - Number of results per page (default: 100, max: 1000)
• days - Override the default days for the period
• hours - Number of hours for "now" period (default: 0)
• sort_by_count - Sort by count (true/false, default: false)
• minimum - Minimum count threshold (default: 0)
• fields - Comma-separated list of Solr fields to return

Common Fields Available

• key, title, subtitle
• author_name, author_key
• cover_i, cover_edition_key
• edition_count, first_publish_year
• language, has_fulltext, ebook_access
• ratings_average, ratings_count, want_to_read_count
• series_key, series_name, series_position
• ia, ia_collection, public_scan_b
• lending_edition_s, lending_identifier_s
• id_project_gutenberg, id_librivox, id_standard_ebooks, id_openstax, id_cita_press, id_wikisource

Response Format (JSON)

{
  "query": "/trending/daily",
  "works": [...],
  "days": 1,
  "hours": 0
}

[bhardwajparth51]

Hey @RayBB, thanks for the feedback! I've pushed an update addressing your points: I switched to the shared Pagination model, removed the deprecated days param, and cleaned up the logger, extra docstrings, and responses block. Let me know if there's anything else

[bhardwajparth51]

Two quick notes for your review/testing:

1. Testing Notes Update: Per your suggestion ("If it has no effect lets remove it"), I entirely removed the days parameter. Just a heads up that the URLs in your testing notes that use it (like ?days=3) won't have an effect anymore — turns out it was never actually doing anything in the original web.py either, since SINCE_DAYS.get(period) always overrode it.

2. Future Cleanup: I noticed the parse_comma_separated_list logic we added is identical to a list comprehension currently used in search.py. I left it local to api.py for now to keep this PR's scope tight, but it's a good candidate for a shared fastapi/utils.py in a follow-up.

[bhardwajparth51]

Hey @RayBB, just checking in on this when you get a chance!

[RayBB]

Good start with this. In general it looks close.

There's a bug thought.

Steps to reproduce:

1. Visit: http://localhost:8080/books/OL24152177M/Robinson_Crusoe
2. Click want to read
3. Visit http://localhost:18080/trending/now.json
4. See error

Unfortunately, in this case, we should probably just have it keep running on the main thead. One day we'll be able to have an async postgres driver but we are not there yet.

[bhardwajparth51]

Hey @RayBB, I've addressed all the requested changes:

1. Created TrendingRequestParams(Pagination) to centralize hours, sort_by_count, minimum, and fields into a single request object.
2. Moved parse_fields_string to models.py and removed the local static method from search.py so both APIs share the same implementation.
3. Removed asyncio.to_thread — get_trending_books now runs on the main thread as a standard def endpoint.

All 45 tests passing. Ready for your review!

[RayBB]

Can you please provide a video of this working on
http://localhost:8080/trending/now.json
and
http://localhost:18080/trending/now.json

It doesn't work at all when I open it.
Be sure to add at least one book to a want to read list or rating first so there's data.

[bhardwajparth51]

Note on web.ctx.site: To get this working I had to bridge the ContextVar to web.ctx.site via web.ctx.site = site_ctx.get() since get_trending_books calls deep into legacy code (add_non_solr_fields → get_solr_works) that still uses web.ctx.site. I noticed search.py line 306 has the same pattern. Since we're moving away from web.ctx, fully removing this dependency would require an architectural change across multiple files (loanstats.py, bookshelves.py, worksearch/code.py, schemes/works.py) — should that be tracked as a separate issue?

[bhardwajparth51]

Here is the screen recording showing parity between legacy (8080) and FastAPI (18080) — added 3 books to Want to Read (Alice's Adventures in Wonderland, Gulliver's Travels, Robinson Crusoe) and both endpoints return identical results.
Screencast from 15-03-26 05:25:41 PM IST.webm

[bhardwajparth51]

Hey @RayBB, just checking in — all 45 tests passing and the screen recording shows parity between legacy and FastAPI. Let me know if there's anything else to address!

[RayBB]

It looks very close but two questions for you!

[RayBB]

The Issue: The FastAPI version defaults sort_by_count to True, but the legacy endpoint defaults it to False. The comment even incorrectly claims "Defaults to True, matching the legacy endpoint behaviour."

Impact: Users who don't explicitly specify the sort_by_count parameter will get different sorting behavior between the legacy and FastAPI endpoints, breaking backward compatibility.

[RayBB]

Can you help me figure out why we need this here? In general we want to avoid using web.ctx.site in fastapi. However, it seems that without this we get a bug. Why is that? Is there any way we can avoid this? It would be very helfpul to understand this

[RayBB]

Ah it seems this is in schemes/works.py line 651
things = cast(list[Work | Edition], web.ctx.site.get_many(keys))

We could change that to site.get now but it does seem to touch quite a bit of code so maybe we should do it in a separate PR.

[bhardwajparth51]

Yeah exactly, traced it to schemes/works.py line 651 — web.ctx.site.get_many(keys) inside add_non_solr_fields. FastAPI never sets up web.ctx.site so without the bridge it just blows up. Makes sense to keep the refactor in a separate PR

[RayBB]

opened an issue here #12178

[RayBB]

Excellent work here. I pushed a few small tweeks up but overall this is exactly what we need!

Testing the following urls to confirm it's the same in fastapi and web.py:

• https://testing.openlibrary.org/trending/daily.json
• https://testing.openlibrary.org/trending/daily.json?limit=10
• https://testing.openlibrary.org/trending/daily.json?sort_by_count=false
• https://testing.openlibrary.org/trending/daily.json?minimum=5
• https://testing.openlibrary.org/trending/daily.json?days=30
• https://testing.openlibrary.org/trending/daily.json?hours=1
• https://testing.openlibrary.org/trending/daily.json?fields=key,title
• https://testing.openlibrary.org/trending/monthly.json?limit=50&page=1&sort_by_count=false&fields=key,title,ratings_average,ratings_count

[RayBB]

@bhardwajparth51 congrats on this one. It was a littler trickier than it looked at first. Please closely review the changes I added to your branch before merging so you can learn a bit more!