feat: new http api v2 - #243
Conversation
cdd4c1f to
12c359f
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/new-http-foundation #243 +/- ##
============================================================
+ Coverage 18.96% 23.96% +5.00%
============================================================
Files 24 67 +43
Lines 10619 12412 +1793
Branches 2061 2307 +246
============================================================
+ Hits 2014 2975 +961
- Misses 8409 9140 +731
- Partials 196 297 +101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
12c359f to
b46c2db
Compare
fe5f1b8 to
c4bec49
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if record["full_name"] is None: | ||
| record["full_name"] = f"{record['first_name']} {record['last_name']}" | ||
| if errors: | ||
| return None, errors |
| raise InvalidInputError("Expected an array for 'articles'.", "NoArticlesField") from None | ||
| except validator.ValidationException as error: | ||
| raise InvalidInputError(error.message, error.code) from error | ||
| except (IndexError, TypeError): |
| assigned_uuid = uri_to_uuid(review["assigned_to"]) | ||
| if reviewer_account["uuid"] == assigned_uuid: | ||
| db.dataset_update_seen_by_reviewer(dataset["uuid"]) | ||
| except (TypeError, IndexError): |
| dataset["container_uuid"], dataset["uuid"], account["uuid"], dataset["account_uuid"] | ||
| ): | ||
| return Response(status_code=204) | ||
| except NotFoundError: |
| db.delete_item_from_list( | ||
| collection["uri"], "categories", URIRef(uuid_to_uri(category["uuid"], "category")) | ||
| ) | ||
| except (TypeError, IndexError, KeyError): |
| ) | ||
| if cat and "uuid" in cat: | ||
| new_cat_records.append(cat) | ||
| except (TypeError, IndexError, KeyError): |
| db.delete_item_from_list( | ||
| dataset["uri"], "categories", URIRef(uuid_to_uri(cat["uuid"], "category")) | ||
| ) | ||
| except (TypeError, IndexError, KeyError): |
| else: | ||
| if db.update_collection(item["uuid"], account_uuid, **more_parm): | ||
| return doi | ||
| except KeyError: |
c4bec49 to
2cfc20d
Compare
|
@kairoaraujo please rebase onto the updated foundation so then I can review this PR properly :) |
Signed-off-by: Kairo de Araujo <kairo@dearaujo.nl>
Framework-neutral infrastructure lifted AS-IS from the refact/fastapi spike,
extracted from the legacy wsgi.py handlers. imaging handles thumbnail creation;
storage wraps the S3/filesystem access. Both depend only on the neutral core
(djehuty.web.{s3,config}, djehuty.utils.convenience), never on the WSGI app.
Lifted AS-IS from the refact/fastapi spike: the pieces shared by every API group. - dependencies.py: get_db, get_token, get_current_account, require_auth, require_admin, pagination_params, reviewer/impersonator helpers. get_token is faithful to legacy token_from_request: cookie first, then the raw Authorization value, stripping a "token " prefix only if present, so both `Authorization: token X` and `Authorization: X` (what Swagger sends) authenticate. A "Session token" APIKeyHeader security scheme makes authenticated endpoints advertise the header in the OpenAPI docs (auto_error=False; cookie auth still works). - exceptions.py: the domain errors and register_exception_handlers. - models/ and services/: pydantic models and article/collection services. Nothing here imports djehuty.web.wsgi.
…mples Faithful ports of the legacy api_* v2 handlers, organized by resource so each file stays small and focused: - top level: articles.py, collections.py, categories.py, licenses.py (public). - account/: the token-protected /v2/account/* endpoints, one module per resource (account, authors, funding, institutions, oauth), with articles/ and collections/ split further into per-sub-resource files sharing a _shared.py helper (the _resolve_private_* resolver + the _ok example builder). Swagger grouping: every endpoint carries a consistent `V2 / ...` tag (set per router), so the docs read hierarchically (V2 / Account / Articles / Authors, ...) and no endpoint lands in the "default" group. Public and authenticated endpoints carry request/response examples from the v2 API reference (doc/api.tex). Auth-failure responses are documented as 403 to match the behaviour the contract suite (PR #132) verifies.
- route_groups: register RouteGroup("api-v2", "/v2/") so /v2/ resolves to the
new stack. The api-docs group and the OpenAPI docs live in the foundation.
- application.py: the umbrella adds CORS, the shared exception handlers, mounts
the v2 router, and documents authentication (the Authorization: token header)
in an "Authentication" section at the top of /api/docs.
- example configs: add the api-v2 line to web-service.groups (set "new").
…n guardrail - test_api_v2_group.py: /v2/ resolution + legacy override, the umbrella serves a v2 endpoint, authenticated endpoints document the Session token header, and both the raw and `token X` Authorization forms authenticate (AS-IS). - test_new_stack_isolation.py: scan djehuty.api and djehuty.services for imports of the legacy WSGI app.
2cfc20d to
11f4f37
Compare
Done! :) |
There was a problem hiding this comment.
Thanks for the PR! I went through every /v2 endpoint in the Swagger UI, running each one against both toggles on the same restored next database and confirming which stack answered via the Server header each time. Findings are mostly observed differences that I tried to trace back to code.
The overall shape is good!
Three things I'd like fixed before merge:
-
GET /v2/collections/{id}/articlesreturns 500 on every request.collection_datasets(container_uri=…)but the method takescollection_uripositionally; the call can never succeed. Legacy returns 200. The unit tests cover the account variant but not this one from what I checked. -
POST /v2/account/collections/searchignorespage/page_size. Every page returns the first 10 records; nothing past record 10 is reachable, with no error. The public collections search and the articles search both handlepagecorrectly, so this is the odd one out. -
CORS is applied globally with
Authorizationinallow_headers. Legacy sets CORS on 8 call sites, all public reads, and never permitsAuthorization. Not exploitable today sinceallow_credentialsis off — but that's one word away, and it's the kind of setting that should be chosen rather than inherited from a middleware default.
There are some other things that likely need to be fixed that I left as comments so you can have a look at them yourself and see if that is the case!
| allow_headers=["Content-Type", "Authorization"], | ||
| expose_headers=["Number-Of-Records", "Number-Of-Returned-Records"], |
There was a problem hiding this comment.
Two things:
- The middleware applies CORS more broadly than legacy did, so this is a slight difference to the AS-IS principle the PR wants to carry out. Legacy allows only Content-Type (wsgi.py:5077). Legacy sends CORS headers from only 8 call sites in wsgi.py; a few public read endpoints. Every other endpoint sends no CORS header at all, so the browser default applies. As middleware, this now sends them on all 55 /v2 endpoints including account writes. allow_origins=["*"] matches legacy so shouldn't be modified. What changed currently is how many endpoints advertise it, plus Authorization in the allowed headers, which legacy never permits (wsgi.py:5077).
This isn't exploitable now as allow_credentials. The concern is that it's one line from becoming a real issue: adding allow_credentials=True later, which is exactly what you'd do to support browser login, would make every account endpoint attackable from any origin.
- Line 55 sets expose_headers to two named headers; legacy sends Access-Control-Expose-Headers: * (wsgi.py:5078). Any client reading another response header cross-origin would break I think.
| token = request.cookies.get("djehuty_session") | ||
| if token is not None: | ||
| return token | ||
|
|
||
| auth = request.headers.get("Authorization") | ||
| if not auth: | ||
| return None | ||
| if auth.startswith("token "): | ||
| return auth[6:] | ||
| return auth |
There was a problem hiding this comment.
nit: the docstring says this is faithful to legacy token_from_request, and it's very close, but legacy applies the token strip after resolving both sources, so it strips cookies too. The early return here skips that for the cookie path.
if token is not None: return tokenreturns immediately, strip never runsreturn auth[6:]strip only on the header path
| def create_app(db, email=None) -> FastAPI: | ||
| app = FastAPI( |
There was a problem hiding this comment.
minor AS-IS gap but doesn't affect user or anything -> legacy sets Server: config.site_name on essentially every response while the new stack sends no Server header at all.
I compared the same request on both toggles:
legacy: server: Werkzeug/3.1.8 Python/3.14.7,4TU.ResearchData
new: server: Werkzeug/3.1.8 Python/3.14.7
| order: OrderField = Query("published_date", description="Field to sort by"), | ||
| order_direction: OrderDirection = Query("desc", description="Sort direction"), |
There was a problem hiding this comment.
the new stack applies a default sort that legacy doesn't.
order and order_direction default to "published_date" / "desc" here, and the same in ArticleSearchRequest (models/articles.py:104-105). Legacy defaults both to None (wsgi.py:1229-1230), and sparql_suffix then emits no ORDER BY at all (rdf.py:186) so results come back in whatever order the store produces.
I tried POST /v2/articles/search with {"search_for": "djehuty"} on both toggles (legacy and new). Same two records came up in opposite order:
legacy: 99e3237e "Hello world" (2024-03-19), 780fb6cc "Djehuty - Git test" (2024-11-19)
new: 780fb6cc "Djehuty - Git test" (2024-11-19), 99e3237e "Hello world" (2024-03-19)
This affects both GET /v2/articles and POST /v2/articles/search, so any caller who doesn't pass order directly.
To be fair, the new behaviour is arguably better as legacy's unordered results make pagination unreliable but it isn't AS-IS, and someone paging through results will get a different set per page after the switch.
| def get_article_versions( | ||
| self, container_uuid, order="version", order_direction="desc", limit=None, offset=None | ||
| ) -> list[dict]: | ||
| """Return version records for a dataset container.""" | ||
| container_uri = f"container:{container_uuid}" | ||
| records = self.db.dataset_versions( | ||
| container_uri=container_uri, | ||
| order=order, | ||
| order_direction=order_direction, | ||
| limit=limit, | ||
| offset=offset, | ||
| ) | ||
| return [formatter.format_dataset_version_record(r) for r in records] |
There was a problem hiding this comment.
never injects base_url.
found using swaggwer api UI:
legacy: "version": 1,
"url": "http://localhost:8080/v2/articles/5d40e250-23bd-4a79-9626-6c9d6003c747/versions/1"
new: "version": 1,
"url": "None/versions/1"
| summary="Delete embargo", | ||
| ) | ||
| def delete_article_embargo(dataset_id: str, account=Depends(require_auth), db=Depends(get_db)): | ||
| dataset = _resolve_private_dataset(db, dataset_id, account["uuid"]) |
There was a problem hiding this comment.
DELETE .../embargo returns 404 where legacy 500s -> many instance of this pattern found earlier.
Same request on both toggles, using a dataset id that doesn't exist:
DELETE /v2/account/articles/53eb35d9-ac04-42ac-8044-7e3c3e8b48e4/embargo
legacy: 500 TypeError: 'NoneType' object is not subscriptable
wsgi.py:6136 — self.db.delete_dataset_embargo(dataset_uri = dataset["uri"], …)
new: 404 {"message": "This resource does not exist."}
| remaining = [f for f in fundings if f.get("uuid") != funding_id] | ||
| uris = uris_from_records(remaining, "funding", "uuid") | ||
| db.update_item_list(dataset["uuid"], account["uuid"], uris, "funding_list") | ||
| return Response(status_code=204) |
There was a problem hiding this comment.
found while testing using swagger API UI:
DELETE .../funding/1
legacy: 404 {"message": "This resource does not exist."}
new: 204
| account=Depends(require_auth), | ||
| service: CollectionService = Depends(_get_service), | ||
| ): | ||
| limit = body.limit or 10 |
There was a problem hiding this comment.
page and page_size are ignored here, so page-based pagination always returns page 1.
CollectionSearchRequest declares both fields (models/collections.py:70-71) and Swagger advertises them, but this handler only reads limit and offset. A client paginating by page example:
{"page": 3, "page_size": 20}
falls through to limit = 10, offset = 0 -> the first 10 records. Every page returns the same rows, and nothing past record 10 is reachable.
The mixing validation is gone too. Comparing the same request on both toggles with page: 1, page_size: 1, limit: 1, offset: 0:
legacy: 400 [{"field_name": "page_size",
"message": "Either use page/page-size or offset/limit. Mixing is not supported."}]
new: 200 []
| from djehuty.utils.rdf import uris_from_records | ||
|
|
||
| if body.get("categories") is None: | ||
| raise InvalidInputError("Missing 'categories' parameter.", "MissingRequiredField") |
There was a problem hiding this comment.
PUT and POST with a body lacking categories: produces different message and code ->
legacy: 400 {"message": "Expected an array for 'categories'.", "code": "NoCategoriesField"}
new: 400 {"message": "Missing 'categories' parameter.", "code": "MissingRequiredField"}
| def list_collection_categories( | ||
| collection_id: str, account=Depends(require_auth), db=Depends(get_db) | ||
| ): | ||
| collection = _resolve_private_collection(db, collection_id, account["uuid"]) |
There was a problem hiding this comment.
Testing against a published collection, so _resolve_private_collection (which queries is_published=False) raises NotFoundError before either handler body runs:
GET .../categories legacy: 500 TypeError at wsgi.py:5979 (item["uri"] where item is None)
new: 404 {"message": "This resource does not exist."}
DELETE .../categories/1 legacy: 403 {"message": "Not allowed."}
new: 404 {"message": "This resource does not exist."}
2 participants
Summary
New http for /api/v2
Changes
src/djehuty/services/{imaging,storage}.py: framework-neutral thumbnail andS3/filesystem services, lifted AS-IS from the legacy handlers (legacy copies
untouched, per the migration plan).
src/djehuty/api/dependencies.py: shared FastAPI dependencies — db, token,current account, admin/reviewer guards, pagination; legacy token semantics.
src/djehuty/api/exceptions.py: domain errors and their handlers.src/djehuty/api/models/,src/djehuty/api/services/: pydantic models and thearticle/collection services shared by the routers.
src/djehuty/api/v2/: public routers (articles, collections, categories,licenses);
account/holds the token-protected/v2/account/*routers, onemodule per resource, under consistent
V2 / ...OpenAPI tags.src/djehuty/route_groups.py: register theapi-v2group for/v2/.src/djehuty/application.py: mount the v2 router; CORS, shared exceptionhandlers, and an Authentication section in the OpenAPI docs.
etc/djehuty/*.json|.xml: add theapi-v2line toweb-service.groups.tests/unit/test_api_v2_group.py: group resolution, the legacy override, bothAuthorizationforms, and serving a v2 endpoint through the umbrella.tests/unit/test_new_stack_isolation.py: extend the no-wsgi.py-importguardrail to
djehuty.apianddjehuty.services.Approval Checklist
Issue Reference (optional - PRs may not be associated with an issue)
Closes #253
Screenshots (optional)
N/A —
/api/docsnow lists every v2 endpoint under hierarchicalV2 / ...groups; a screenshot can be added if useful.
Notes (optional)
"api-v2": "legacy"underweb-service.groupsand restart —no rebuild, no code change.