Migrate ratings endpoints from web.py to FastAPI by Shikhar-395 · Pull Request #12001 · internetarchive/openlibrary

Shikhar-395 · 2026-03-03T21:35:52Z

referring: #11999

Changes

`openlibrary/fastapi/internal/api.py`

Added get_ratings GET and POST as standalone decorated functions

`openlibrary/plugins/openlibrary/api.py`

`scripts/test_ratings_migration.sh`

Differences

Aspect	Legacy (web.py)	FastAPI
GET response JSON	identical	identical
POST response JSON	identical	identical
Authentication	Redirects to `/account/login` (303)	Returns 401 Unauthorized
Invalid rating	200 + `{"error": "invalid rating"}`	422 Unprocessable Entity

Stakeholder

@RayBB

for more information, see https://pre-commit.ci

RayBB

Few minor comments, but it's a good start. Please request another review when you push up the fixes.

for more information, see https://pre-commit.ci

Shikhar-395 · 2026-03-04T22:16:59Z

Also removed JSONResponse wrapping from (GET) rating endpoint.

RayBB

Two small things here, but it looks really good. I'm excited to test it soon.

RayBB

A few small changes and then I'll look again!

…y URLs

…endpoint

RayBB

I pushed up some very minor changes.
However, this PR isn't complete. It does not handle most of the options possible for the post request.

Make sure the functionality is compatible with the old endpoint. ajax, redir, redir_url, and page need to be implemented.

Before asking me to review this again please be sure to test all of this functionality yourself to ensure it has pretty much the same response as the old endpoint.

RayBB · 2026-03-23T20:00:09Z

@395ShikharSingh do you plan to keep working on this or can it be assigned to someone else?

…and tests

Shikhar-395 · 2026-03-24T00:49:04Z

Hey @RayBB sorry for the delay! I've added full legacy compatibility to the POST endpoint (redir, redir_url, page, ajax, edition_id) and added unit tests covering all scenarios. Tested locally and everything is working. Let me know if anything needs.

for more information, see https://pre-commit.ci

RayBB

Thanks for the quick followup. I know mine have been slow lately.f

We're in the right direction and there's still quite a bit more feedback to address here. When in doubt please follow the way that other fastapi endpoints look.

RayBB · 2026-03-24T03:00:49Z

+
+
+@pytest.fixture
+def fastapi_client(monkeypatch):


This already exists. Please do not recreate it.

RayBB · 2026-03-24T03:01:24Z

+
+
+@pytest.fixture
+def authenticated_client(fastapi_client):


We now have mock_authenticated_user that you can use instead as long as your PR is caught up with master

RayBB · 2026-03-24T03:02:23Z

+    return redir_url or edition_id or f"/works/OL{work_id}W"
+
+
+def _get_absolute_redirect_url(request: Request, path: str) -> str:


Please don't add one line function definitions unless there is a very good reason.

Please keep all of this as similar as possible to the old endpoint instead restructuring it when not needed.

RayBB · 2026-03-24T03:07:17Z

+@router.post("/works/OL{work_id}W/ratings", tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA, response_model=None)
+@router.post("/works/OL{work_id}W/ratings.json", tags=["internal"], include_in_schema=SHOW_INTERNAL_IN_SCHEMA, response_model=None)
+async def post_ratings(
+    request: Request,


We should only use a raw request in special cases. In this case, inline the values just like we do with the others in this file on master. This gives us clearer documentation and follows best practices from fastapi.

RayBB · 2026-03-24T03:07:51Z

+    if not user:
+        return RedirectResponse(_get_absolute_redirect_url(request, f"/account/login?redirect={key}"), status_code=303)


We don't need to do this. It's an authenticated endpoint.

RayBB · 2026-03-24T03:08:10Z

+async def post_ratings(
+    request: Request,
+    work_id: Annotated[int, Path()],
+    user: Annotated[AuthenticatedUser | None, Depends(get_authenticated_user)],


use require_authenticated_user because they must be logged in to use this.

RayBB · 2026-03-24T03:10:07Z

+    else:
+        try:
+            rating = int(data["rating"])
+            if rating not in models.Ratings.VALID_STAR_RATINGS:


We won't need this because we should validate using the gt and lt options in the endpoint params

RayBB · 2026-03-24T03:10:21Z

+        except (TypeError, ValueError):
+            return {"error": "invalid rating"}


we also will not need this

RayBB · 2026-03-24T03:10:50Z

+    if data.get("redir") and not data.get("ajax"):
+        return _build_rating_redirect_response(request, key, data.get("page"))


Don't make a new function here. Keep it inline and simple.

for more information, see https://pre-commit.ci

Shikhar-395 · 2026-03-24T14:13:53Z

One quick clarification before I push the next revision: for /works/OL{work_id}W/ratings, should legacy compatibility take priority over the usual FastAPI pattern when they conflict?
I’m asking because there are a few remaining cases where the old endpoint and the standard FastAPI approach differ:

unauthenticated POST: legacy redirects to login, while require_authenticated_user returns 401
invalid rating: legacy returns {"error": "invalid rating"} with 200, while param validation returns 422
legacy web.input() accepts POST query params like ajax, redir, redir_url, and page, while plain Form(...) handling does not

Since your earlier feedback was to keep this compatible with the old endpoint, I wanted to confirm whether for this endpoint I should preserve the legacy behavior exactly in those cases.

RayBB · 2026-03-24T16:45:35Z

These can differ (follow modern fastapi):

unauthenticated POST: legacy redirects to login, while require_authenticated_user returns 401
invalid rating: legacy returns {"error": "invalid rating"} with 200, while param validation returns 422

This is a good question. In this case please ensure we have the exact same functionality. If before it was only query params then now it should accept query params. If it has working with forms before it should work with forms now:

legacy web.input() accepts POST query params like ajax, redir, redir_url, and page, while plain Form(...) handling does not

Shikhar-395 · 2026-03-25T08:30:55Z

Addressed the remaining /ratings migration feedback.
POST now accepts the legacy inputs via both form data and query params (ajax, redir, redir_url, page, rating, edition_id) while keeping the modern FastAPI behavior we discussed for auth (401) and invalid rating validation (422).
I also switched the ratings tests to the shared FastAPI fixtures, and updated the shared FastAPI test client setup in conftest.py so these endpoint tests can use that common fixture cleanly.

RayBB

@395ShikharSingh really excellent work here.

Since this is an internal endpoint I ultimately ended up deciding that instead of making the code so complicated supporting query params and redirects that we could just drop those. We already were not using the query params and the redirects only triggered when there is no JS (and for little gain at that). Since we're now in an OL world where we expect JS it's ok to remove that.

Anyway, your work is appreciated and you can look at the last couple commits I added to learn more. Let me know if you have questions!

Shikhar-395 and others added 5 commits March 4, 2026 02:36

Migrate ratings endpoints from web.py to FastAPI

7d7c8f9

[pre-commit.ci] auto fixes from pre-commit.com hooks

e23528e

for more information, see https://pre-commit.ci

Ensure FastAPI rating POST response shape strictly matches legacy

281a998

[pre-commit.ci] auto fixes from pre-commit.com hooks

3139016

for more information, see https://pre-commit.ci

Move test_ratings_migration.sh to tests/integration/temporary

bd6551b

RayBB requested changes Mar 4, 2026

View reviewed changes

Shikhar-395 and others added 3 commits March 5, 2026 03:37

refactor: address code review feedback on ratings FastAPI endpoint

14b67bc

[pre-commit.ci] auto fixes from pre-commit.com hooks

e9d45ed

for more information, see https://pre-commit.ci

refactor: remove JSONResponse wrapping from get_ratings

b1e7cf1

Shikhar-395 requested a review from RayBB March 4, 2026 22:17

RayBB requested changes Mar 4, 2026

View reviewed changes

Comment thread openlibrary/fastapi/internal/api.py Outdated

Comment thread openlibrary/fastapi/internal/api.py Outdated

refactor: add response model and Path annotation to ratings endpoints

17f6a5e

Shikhar-395 requested a review from RayBB March 5, 2026 02:16

RayBB self-assigned this Mar 5, 2026

github-project-automation Bot added this to Ray's Project Mar 5, 2026

github-project-automation Bot moved this to Waiting Review/Merge from Staff in Ray's Project Mar 5, 2026

RayBB requested changes Mar 14, 2026

View reviewed changes

Comment thread openlibrary/fastapi/internal/api.py Outdated

Comment thread openlibrary/fastapi/internal/api.py Outdated

Comment thread openlibrary/fastapi/internal/api.py Outdated

Shikhar-395 added 2 commits March 15, 2026 00:14

refactor: update ratings endpoints to use .json suffix to match legac…

c1eecc0

…y URLs

refactor: simplify get_ratings return and add .json extension to GET …

70497d9

…endpoint

RayBB added the On Testing label Mar 15, 2026

include sortable, pluralize ratings, move user up

0c8383c

RayBB requested changes Mar 15, 2026

View reviewed changes

RayBB removed the On Testing label Mar 15, 2026

Shikhar-395 marked this pull request as draft March 17, 2026 15:53

RayBB mentioned this pull request Mar 23, 2026

Migrate internal endpoints to FastAPI #11999

Open

8 tasks

Shikhar-395 force-pushed the feature/migrate-ratings-to-fastapi branch from 2003c77 to 0c8383c Compare March 24, 2026 00:07

feat: complete ratings POST migration with full legacy compatibility …

a3bcde9

…and tests

Shikhar-395 marked this pull request as ready for review March 24, 2026 00:44

RayBB and others added 2 commits March 23, 2026 19:54

Merge branch 'master' into feature/migrate-ratings-to-fastapi

93ffbb0

[pre-commit.ci] auto fixes from pre-commit.com hooks

28f03a5

for more information, see https://pre-commit.ci

RayBB requested changes Mar 24, 2026

View reviewed changes

github-actions Bot added the Needs: Response Issues which require feedback from lead label Mar 24, 2026

Shikhar-395 and others added 2 commits March 24, 2026 18:52

Merge branch 'master' into feature/migrate-ratings-to-fastapi

21c8518

[pre-commit.ci] auto fixes from pre-commit.com hooks

6315177

for more information, see https://pre-commit.ci

RayBB added Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] and removed Needs: Response Issues which require feedback from lead labels Mar 24, 2026

Match legacy ratings POST inputs

14339b9

github-actions Bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Mar 25, 2026

github-actions Bot added the Needs: Response Issues which require feedback from lead label Mar 25, 2026

RayBB added 7 commits March 27, 2026 15:03

only support form because we only use form

71eab34

Delete tests/integration/temporary/test_ratings_migration.sh

0a7ac66

remove unneeded schema stuff

4f2101d

Merge branch 'master' into feature/migrate-ratings-to-fastapi

c898746

add note and improve tests

3078817

handle users not logged in and remove non-json endpoint

6b1fe41

simplify code by removing redirects

3764ae3

RayBB approved these changes Mar 27, 2026

View reviewed changes

RayBB merged commit 17a45a5 into internetarchive:master Mar 27, 2026
4 checks passed

github-project-automation Bot moved this from Waiting Review/Merge from Staff to Done in Ray's Project Mar 27, 2026

		return redir_url or edition_id or f"/works/OL{work_id}W"


		def _get_absolute_redirect_url(request: Request, path: str) -> str:

		if not user:
		return RedirectResponse(_get_absolute_redirect_url(request, f"/account/login?redirect={key}"), status_code=303)

		except (TypeError, ValueError):
		return {"error": "invalid rating"}

		if data.get("redir") and not data.get("ajax"):
		return _build_rating_redirect_response(request, key, data.get("page"))

Uh oh!

Conversation

Shikhar-395 commented Mar 3, 2026

Changes

openlibrary/fastapi/internal/api.py

openlibrary/plugins/openlibrary/api.py

scripts/test_ratings_migration.sh

Differences

Stakeholder

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shikhar-395 commented Mar 4, 2026

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

RayBB commented Mar 23, 2026

Uh oh!

Shikhar-395 commented Mar 24, 2026

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shikhar-395 commented Mar 24, 2026

Uh oh!

RayBB commented Mar 24, 2026

Uh oh!

Shikhar-395 commented Mar 25, 2026

Uh oh!

RayBB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

`openlibrary/fastapi/internal/api.py`

`openlibrary/plugins/openlibrary/api.py`

`scripts/test_ratings_migration.sh`