Repository navigation
Add POST /oed/contributions for the OED data contribution form - #158
matthewberry wants to merge 3 commits into
Conversation
The About page form uploaded to POST /oed-contributions/upload, which has returned 400 "Unknown bucket" since 15b2ed8 restricted that route to job types. Give contributions their own endpoint instead of widening the upload route's allow-list. The endpoint requires both agreements from the form, accepts only .xlsx/.xls/.csv up to 50 MB, and stores each contribution under a generated id with a submission.json holding the contributor's details, so the email address is no longer part of the object name.
matthewberry
left a comment
matthewberry
left a comment
There was a problem hiding this comment.
Review notes. Nothing blocking. The email-validation mismatch with the frontend form is posted on moleculemaker/OpenEnzymeDB-frontend#89.
| raise HTTPException(status_code=400, detail=f'File must be one of: {", ".join(CONTRIBUTION_EXTENSIONS)}') | ||
|
|
||
| # Read one byte past the limit so an oversized upload is detected without holding it all. | ||
| content = await file.read(CONTRIBUTION_MAX_BYTES + 1) |
There was a problem hiding this comment.
This 50 MB check protects MinIO, not the pod. Starlette parses the whole multipart body, writing the file to a spooled temp file on disk, before this handler runs. So a 1 GB upload is fully received and written to the pod's temp disk before we return 413. Nothing in chart/ sets an ingress body-size limit (grepped body-size/proxy-body-size), and the endpoint has no login and no rate limit. That isn't a regression, since the old /oed-contributions/upload route had none of these either. But the frontend picker allows 1 GB, so people will hit this by accident, not just abusers. Lowering the picker's maxFileSize to 50 MB in #89's repo would close the accidental case. An ingress body-size limit on this route would close the rest.
There was a problem hiding this comment.
Confirmed against the pinned versions. FastAPI 0.115.6 awaits request.form() (routing.py:247) before resolving any parameter. Starlette 0.41.3's max_file_size is only the SpooledTemporaryFile rollover threshold, and file parts have no size cap. One correction: the ingress is Traefik, so body-size/proxy-body-size couldn't have matched anything. The conclusion still holds, though. There's no buffering middleware in chart/, and the only rate limits are on /chemscraper/analyze and POST /v1/tools*. /oed/contributions goes through the catch-all / Ingress with CORS only.
The picker fix is in #89 (635cd35), but lowering maxFileSize alone wouldn't have worked. PrimeNG 17's onSelect still lists a file that failed the size check in event.files, and only currentFiles leaves it out. The form read event.files[0], so it now reads currentFiles. I'm leaving the ingress body-size limit out of this PR. It would need a Traefik buffering middleware on a dedicated route, which should be its own chart change.
|
|
||
|
|
||
| @router.post( | ||
| "/oed/contributions", tags=['OED'], |
There was a problem hiding this comment.
The 'OED' tag makes openapi-generator emit oED.service.ts / OEDService on the frontend. It's cosmetic, but it's hard to rename once the generated client depends on it. 'OpenEnzymeDB' or 'Oed' gives a normal file name. If you change it, do it before merging, because #89's hand-edited spec and generated service would both need regenerating.
|
|
||
| if (minio.ensure_bucket_exists(CONTRIBUTIONS_BUCKET) | ||
| and minio.upload_file(CONTRIBUTIONS_BUCKET, f'{contribution_id}/{filename}', content) | ||
| and minio.upload_file(CONTRIBUTIONS_BUCKET, f'{contribution_id}/submission.json', json.dumps(submission, indent=2))): |
There was a problem hiding this comment.
If the data file uploads but this submission.json upload fails, the data stays in the bucket with no record of who sent it. The user gets a 500 and will likely retry, so the impact is small. It could still leave unexplained folders in oed-contributions. One option is to write submission.json first, so a failure leaves a record with no data rather than data with no record. Another is to remove the data object on failure.
There was a problem hiding this comment.
Fixed in d7239c2: submission.json is now written before the data file. test_failed_data_upload_leaves_the_submission_record fails on the old order and passes on the new one. The full suite passes (636).
The MinIO client blocks, so as an async handler a 50 MB upload stalled every other request on the event loop. As a plain def, FastAPI runs it in its threadpool.
…e OpenEnzymeDB Writing submission.json first means a failed data upload leaves a record of who sent it, instead of an unattributed file in oed-contributions. The 'OED' tag made openapi-generator emit oED.service.ts / OEDService on the frontend; 'OpenEnzymeDB' gives openEnzymeDB.service.ts / OpenEnzymeDBService.
Summary
The OpenEnzymeDB About page's data contribution form has been failing on every submission (moleculemaker/OpenEnzymeDB-frontend#88). It uploads to
POST /oed-contributions/upload, and since 15b2ed8 that route returns400 Unknown bucketfor anything that isn't a job type.This PR adds a dedicated endpoint instead of allow-listing
oed-contributionson the upload route, so the upload route still can't be used to create arbitrary buckets.POST /oed/contributionsMultipart form with fields
file,email,license_acceptedandrights_confirmed.201 {"contributionID", "submitted_at"}.xlsx/.xls/.csv, or is emptyStorage: each contribution goes in bucket
oed-contributionsas<uuid>/<original filename>, with a<uuid>/submission.jsonbeside it holding the email, file name, size, time, license (CC-BY-4.0) and rights confirmation. Before, the form put the email in the object name (<email>_<filename>) and the checkbox answers weren't sent at all. A client path in the file name is reduced to the base name.For reviewers: where to find contributions is written in the endpoint's OpenAPI description.
Choices worth a look
Testing
tests/test_oed_contributions.py: 15 tests covering storage layout, email kept out of the object key, unique ids, path stripping, both agreements, file types, invalid email, empty file, the size limit at the boundary, storage failure → 500,POST /oed-contributions/uploadstill returning 400, and the handler being sync (a plaindef, so FastAPI runs its blocking MinIO calls in the threadpool rather than on the event loop).FakeMinIOfromconftest.py.Deploy
This has to be deployed before the frontend change (moleculemaker/OpenEnzymeDB-frontend#89), which switches the form to this endpoint.