[GrandComicsDatabase/gcd-django] feat(api-v2): scaffold v2 API foundation at /api/v2/ (PR #711)
Adam Hernandez
Foundation work for a v2 REST API at /api/v2/, sitting alongside the existing v1 API at /api/ which is untouched. No data endpoints in this PR — it's pure scaffolding so the structure can be reviewed before serializer logic starts piling up. Series, Issues, and Publishers come in the next PR.
What's in here
A new apps/api_v2/ Django app with a read-only base viewset, token auth wired at /api/v2/auth/token/, drf-spectacular schema + Swagger UI + ReDoc, page-number pagination (default 50, max 200), three throttle buckets (anon 30/hr, token-or-basic 2000/day, session 2000/day), and conditional-request helpers ready for endpoints to plug into. Tests are set up via pytest-django with shared fixtures in conftest.py. CI is a small lint-only GitHub Actions workflow that triggers on changes inside apps/api_v2/ only — it deliberately stays out of the rest of the codebase.
The URL routing is split into urls_www.py and urls_my.py and dispatched on settings.MYCOMICS. This mirrors the existing two-instance deployment model: www.comics.org (MYCOMICS=False) serves approved GCD data, my.comics.org (MYCOMICS=True) serves user data. The dispatcher includes exactly one surface module per instance, plus the cross-cutting routes (token, schema) on both. Right now both surface modules have empty routers, so the externally visible behavior is the same as before, but the structure is in place for collections and reading-orders to land cleanly on the my-side in a later PR.
Outside apps/api_v2/
Touch list kept minimal:
settings.py: addapps.api_v2andrest_framework.authtokentoINSTALLED_APPS(two lines).urls.py: one line,path('api/v2/', include('apps.api_v2.urls')).requirements.txt: one line, the conditional package.pyproject.toml: new file, ruff + pytest config scoped toapps/api_v2/.
The authtoken migration runs on first deploy and creates the three-column authtoken_token table.
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/711
Commit Summary
- a2a1c4d feat(api-v2): add pyproject.toml ruff + pytest config
- 70bab6d feat(api-v2): scaffold apps/api_v2 app structure
- 39ec87c feat(api-v2): wire empty DefaultRouter at /api/v2/
- 9cf5e96 feat(api-v2): add GCDBaseViewSet with deleted=False filtering
- e3ed549 feat(api-v2): wire obtain_auth_token at /api/v2/auth/token/
- 8dcae1e feat(api-v2): pin DRF auth, perms, pagination, throttles, schema
- 1d5265f feat(api-v2): split user throttle by auth method, add Swagger + ReDoc
- dededf5 feat(api-v2): conditional request helpers + bring in dep
- 1c7784e test(api-v2): add conftest with base fixtures for v2 tests
- a4d8476 ci(api-v2): add scoped lint workflow for v2 code
- 08514ab fix(api-v2): make /api/v2/ root view anon-readable
- 9d93734 refactor(api-v2): split urls into www/my surfaces with mycomics dispatch
File Changes
(21 files)
- A .github/workflows/api-v2-ci.yml (35)
- A apps/api_v2/__init__.py (0)
- A apps/api_v2/filters/__init__.py (0)
- A apps/api_v2/pagination.py (15)
- A apps/api_v2/routers.py (39)
- A apps/api_v2/serializers/__init__.py (0)
- A apps/api_v2/tests/__init__.py (0)
- A apps/api_v2/tests/conftest.py (120)
- A apps/api_v2/tests/test_url_dispatch.py (100)
- A apps/api_v2/throttling.py (60)
- A apps/api_v2/urls.py (56)
- A apps/api_v2/urls_my.py (17)
- A apps/api_v2/urls_www.py (17)
- A apps/api_v2/utils/__init__.py (0)
- A apps/api_v2/utils/conditional.py (42)
- A apps/api_v2/utils/spectacular.py (15)
- A apps/api_v2/views/__init__.py (60)
- A pyproject.toml (61)
- M requirements.txt (1)
- M settings.py (2)
- M urls.py (1)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/711.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/711.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request establishes the foundation for the v2 API, introducing custom routers, pagination, throttling, and a base viewset that handles soft-delete filtering. It also includes utility helpers for conditional requests and OpenAPI schema generation, along with a comprehensive testing setup using pytest. Feedback was provided to ensure fixture isolation in tests by avoiding side effects on shared API clients and to use tuples for class attributes in viewsets to maintain consistency with the rest of the codebase.
In apps/api_v2/tests/conftest.py:
> +def authenticated_client(api_client, authenticated_token):
+ """Return an ``APIClient`` pre-configured with the test user's token."""
+ api_client.credentials(
+ HTTP_AUTHORIZATION=f'Token {authenticated_token.key}',
+ )
+ return api_client
The authenticated_client fixture modifies the api_client fixture instance it receives. This creates a side effect: any other test or fixture that uses api_client within the same scope will unexpectedly get an authenticated client, breaking the "unauthenticated" contract of the api_client fixture. This can lead to subtle and hard-to-debug test failures.
To ensure fixture isolation and prevent side effects, authenticated_client should create its own APIClient instance instead of modifying the shared one.
-def authenticated_client(api_client, authenticated_token):
- """Return an ``APIClient`` pre-configured with the test user's token."""
- api_client.credentials(
- HTTP_AUTHORIZATION=f'Token {authenticated_token.key}',
- )
- return api_client
+def authenticated_client(authenticated_token):
+ """Return an APIClient pre-configured with the test user's token."""
+ client = APIClient()
+ client.credentials(
+ HTTP_AUTHORIZATION=f"Token {authenticated_token.key}",
+ )
+ return client
In apps/api_v2/views/__init__.py:
> + authentication_classes = [ + TokenAuthentication, + BasicAuthentication, + SessionAuthentication, + ] + permission_classes = [AllowAny] + pagination_class = V2PageNumberPagination + throttle_classes = [ + V2AnonRateThrottle, + V2TokenUserRateThrottle, + V2SessionUserRateThrottle, + ]
For consistency and to signal immutability, it's better to use tuples instead of lists for class attributes like authentication_classes, permission_classes, and throttle_classes. This matches the style used in apps/api_v2/routers.py for similar attributes.
- authentication_classes = [ - TokenAuthentication, - BasicAuthentication, - SessionAuthentication, - ] - permission_classes = [AllowAny] - pagination_class = V2PageNumberPagination - throttle_classes = [ - V2AnonRateThrottle, - V2TokenUserRateThrottle, - V2SessionUserRateThrottle, - ] + authentication_classes = ( + TokenAuthentication, + BasicAuthentication, + SessionAuthentication, + ) + permission_classes = (AllowAny,) + pagination_class = V2PageNumberPagination + throttle_classes = ( + V2AnonRateThrottle, + V2TokenUserRateThrottle, + V2SessionUserRateThrottle, + )
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco pushed 1 commit.
- 81f620f fix(api-v2): isolate test client fixture state
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/api_v2/tests/conftest.py:
> +def authenticated_client(api_client, authenticated_token):
+ """Return an ``APIClient`` pre-configured with the test user's token."""
+ api_client.credentials(
+ HTTP_AUTHORIZATION=f'Token {authenticated_token.key}',
+ )
+ return api_client
Fixed in 81f620f6. I changed authenticated_client to create its own APIClient instead of mutating the shared api_client fixture, so auth state no longer leaks into tests that expect an anonymous client.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In apps/api_v2/views/__init__.py:
> + authentication_classes = [ + TokenAuthentication, + BasicAuthentication, + SessionAuthentication, + ] + permission_classes = [AllowAny] + pagination_class = V2PageNumberPagination + throttle_classes = [ + V2AnonRateThrottle, + V2TokenUserRateThrottle, + V2SessionUserRateThrottle, + ]
Fixed in 81f620f6. I switched those base viewset class attributes to tuples so they match the existing V2APIRootView style and read as fixed class-level configuration rather than mutable lists.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco pushed 1 commit.
- cd9429c fix(api-v2): align shared routes and conditionals
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Richard Thomson
@LegalizeAdulthood approved this pull request.
I only skimmed over it, but other than a couple nits, I have no other comments.
I think your approach is good: bring things in one bit at a time in reasonably sized commits that can be reviewed without reading code longer than War and Peace. :)
Probably should get a review from Jochen before merging.
On apps/api_v2/filters/__init__.py:
What's changing if you're only changing whitespace?
In .github/workflows/api-v2-ci.yml:
> @@ -0,0 +1,35 @@ +name: API v2 CI
The existing files don't have it, but it probably wouldn't hurt for these new files to put SPDX license identifier comments on them. Top-level license file is GPL v3, I think.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
On apps/api_v2/filters/__init__.py:
Nothing is changing its just creation of the init file.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
In .github/workflows/api-v2-ci.yml:
> @@ -0,0 +1,35 @@ +name: API v2 CI
How about I add them in the next PR so I don't create a lot of noise here? If you want them included now, I’m happy to add them here.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
JochenGCD
jochengcd left a comment (GrandComicsDatabase/gcd-django#711)I created an api-v2 branch, where this should go to. For longer devs, we use specific branches.
Please change the target branch to that one.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
JochenGCD
@jochengcd commented on this pull request.
> + + scope = 'v2_anon' + rate = '30/hour' + + +class V2TokenUserRateThrottle(UserRateThrottle): + """2000 requests per day for Token or Basic authenticated users. + + Both methods carry credentials per request and target programmatic + consumers (third-party tools, scripts, CI). The PRD lists the rate + explicitly for Token; Basic is grouped here because it is the same + use case. + """ + + scope = 'v2_user_token' + rate = '2000/day'
down the road these configs should be in settings. Authenticated users likely should also have a short-term burst limit.
Both these are fines as TODOs in the file for now
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco pushed 1 commit.
- 87a2df1 chore(api-v2): note throttling config follow-up
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
> + + scope = 'v2_anon' + rate = '30/hour' + + +class V2TokenUserRateThrottle(UserRateThrottle): + """2000 requests per day for Token or Basic authenticated users. + + Both methods carry credentials per request and target programmatic + consumers (third-party tools, scripts, CI). The PRD lists the rate + explicitly for Token; Basic is grouped here because it is the same + use case. + """ + + scope = 'v2_user_token' + rate = '2000/day'
TODO an minor modification added.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you are subscribed to this thread.