From 560b8a603d6ce55a89259525eeec6e6b7d09f5ba Mon Sep 17 00:00:00 2001 From: Quang <20378quang@gmail.com> Date: Tue, 25 Aug 2026 09:47:24 -0400 Subject: [PATCH] feat(submissions): authenticated retrieval, export and retention cleanup --- .env.example | 21 + README.md | 19 +- docs/api/deliveries.md | 11 +- docs/api/errors.md | 20 +- docs/api/submission-management.md | 157 +++++ docs/api/submissions.md | 11 +- docs/architecture/security.md | 6 +- docs/guides/delivery-replay.md | 11 +- docs/guides/exporting-submissions.md | 186 ++++++ docs/guides/idempotency.md | 8 +- docs/guides/submission-management.md | 143 +++++ docs/index.md | 39 +- docs/operations/configuration-reference.md | 15 + docs/operations/retention.md | 171 +++++ docs/reference/data-handling.md | 106 ++++ docs/reference/limitations.md | 41 +- mkdocs.yml | 5 + src/hymical_forms/api/deliveries.py | 35 +- .../api/submission_management.py | 589 ++++++++++++++++++ src/hymical_forms/app.py | 3 +- src/hymical_forms/cli.py | 154 ++++- src/hymical_forms/config.py | 28 + src/hymical_forms/export.py | 202 ++++++ .../0005_20260825_submission_retention.py | 201 ++++++ src/hymical_forms/models.py | 42 +- src/hymical_forms/retention.py | 100 +++ src/hymical_forms/storage.py | 373 +++++++++-- src/hymical_forms/worker.py | 4 +- tests/conftest.py | 64 +- tests/integration/support.py | 2 + .../integration/test_constraints_postgres.py | 1 + tests/integration/test_migrations_postgres.py | 217 ++++++- tests/integration/test_replay_postgres.py | 4 +- tests/integration/test_retention_postgres.py | 327 ++++++++++ tests/test_openapi.py | 32 +- tests/test_retention.py | 539 ++++++++++++++++ tests/test_submission_export.py | 428 +++++++++++++ tests/test_submissions_api.py | 394 ++++++++++++ 38 files changed, 4583 insertions(+), 126 deletions(-) create mode 100644 docs/api/submission-management.md create mode 100644 docs/guides/exporting-submissions.md create mode 100644 docs/guides/submission-management.md create mode 100644 docs/operations/retention.md create mode 100644 docs/reference/data-handling.md create mode 100644 src/hymical_forms/api/submission_management.py create mode 100644 src/hymical_forms/export.py create mode 100644 src/hymical_forms/migrations/versions/0005_20260825_submission_retention.py create mode 100644 src/hymical_forms/retention.py create mode 100644 tests/integration/test_retention_postgres.py create mode 100644 tests/test_retention.py create mode 100644 tests/test_submission_export.py create mode 100644 tests/test_submissions_api.py diff --git a/.env.example b/.env.example index 720b848..3b1c24f 100644 --- a/.env.example +++ b/.env.example @@ -63,6 +63,27 @@ FORMS_DATABASE_URL=postgresql+psycopg://forms:forms@localhost:5432/forms # production lets anyone who can create an endpoint reach your internal network. # FORMS_ALLOW_PRIVATE_WEBHOOK_TARGETS=false +# --- submission retrieval and retention --------------------------------------- + +# How many days a stored submission is kept before an operator's cleanup command +# may delete it. 0, the default, keeps submissions indefinitely. +# +# Nothing is ever deleted automatically. Setting this only makes older +# submissions eligible; the sweep is a command you run: +# +# python -m hymical_forms.cli cleanup-submissions --dry-run +# python -m hymical_forms.cli cleanup-submissions +# +# A submission whose webhook delivery is still pending, processing, or failed and +# therefore replayable is never deleted, however old it is, because the payload +# is built from the submission at the moment it is sent. +# FORMS_SUBMISSION_RETENTION_DAYS=0 + +# Largest number of submissions one export may return. A filter matching more +# than this is refused rather than silently truncated, so an export is either +# complete or an error. Narrow the range and export it in parts. +# FORMS_EXPORT_MAX_SUBMISSIONS=10000 + # --- public ingestion rate limiting ------------------------------------------- # # These apply only to POST /f/{endpoint_id}. Management routes and /health are diff --git a/README.md b/README.md index 352f4d9..c93c59f 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,10 @@ crash cannot lose work the service already acknowledged. - **Management API keys**, created by an operator CLI and stored only as digests - **Endpoint and delivery operations**: reconfigure, inspect attempt history, replay a failed delivery +- **Submission retrieval and export**: browse, filter by endpoint and time, read + one back, export a filtered range as JSON or CSV +- **Retention cleanup** driven by an operator command, which never deletes a + submission a delivery could still need - **Distributed rate limiting** on public ingestion, per source address and per endpoint, shared across API processes - **Alembic migrations** with a startup revision check and model drift tests @@ -142,10 +146,11 @@ Full walkthrough: | Section | Covers | | --- | --- | | [Getting Started](https://hymical.github.io/forms/getting-started/installation/) | Install, configure, migrate, first submission | -| [Guides](https://hymical.github.io/forms/guides/form-ingestion/) | Ingestion, idempotency, webhooks, rate limiting, endpoints, replay | +| [Guides](https://hymical.github.io/forms/guides/form-ingestion/) | Ingestion, idempotency, webhooks, rate limiting, endpoints, replay, submissions, export | | [API Reference](https://hymical.github.io/forms/api/authentication/) | Every route, its parameters and responses, and the complete error table | -| [Operations](https://hymical.github.io/forms/operations/worker/) | Worker, migrations, reverse proxy, every configuration variable | +| [Operations](https://hymical.github.io/forms/operations/worker/) | Worker, migrations, retention, reverse proxy, every configuration variable | | [Architecture](https://hymical.github.io/forms/architecture/overview/) | Transactional outbox, delivery semantics, concurrency, security | +| [Data handling](https://hymical.github.io/forms/reference/data-handling/) | Where submitted values go, and where they never go | | [Limitations](https://hymical.github.io/forms/reference/limitations/) | An honest list of what this build does not do yet | ## Project status @@ -165,18 +170,24 @@ concurrency. | Delivery inspection and manual replay | Implemented | | Public ingestion rate limiting | Implemented | | Schema migrations | Implemented | -| Submission retrieval, export, retention | **Not implemented** | +| Submission retrieval and filtering | Implemented | +| Submission export, JSON and CSV | Implemented | +| Retention cleanup, operator-run | Implemented | +| Scheduled retention | **Not implemented** | +| Submission search | **Not implemented** | | Endpoint deletion | **Not implemented** | | Spam handling, CAPTCHA | **Not implemented** | | Dashboards | **Not implemented** | -Three things are worth knowing before you deploy it: +Four things are worth knowing before you deploy it: - **Delivery is at-least-once, not exactly-once.** Deduplicate on the submission `id` in the signed payload. - **Rate limiting is traffic protection, not spam protection.** It bounds volume and has no opinion about content. - **SSRF protection is partial.** Webhook hostnames are not resolved. +- **Nothing is deleted until you delete it.** Retention is a command an operator + runs, and it never removes a submission a delivery could still need. The full list is in [Limitations](https://hymical.github.io/forms/reference/limitations/). diff --git a/docs/api/deliveries.md b/docs/api/deliveries.md index 7921764..f6fdc75 100644 --- a/docs/api/deliveries.md +++ b/docs/api/deliveries.md @@ -27,8 +27,8 @@ Paging works exactly as it does for endpoints. See | Field | Meaning | | --- | --- | | `id` | Delivery identifier | -| `submission_id` | The submission this delivery carries | -| `endpoint_id` | The endpoint that submission was addressed to | +| `submission_id` | The submission this delivery carries, or `null` once retention has removed it | +| `endpoint_id` | The endpoint the submission was addressed to, recorded on the delivery | | `state` | `pending`, `processing`, `delivered` or `failed` | | `destination_url` | The URL snapshotted when the submission was accepted | | `attempt_count` | Every request ever made for this delivery | @@ -78,8 +78,10 @@ Attempts are ordered by `attempt_number`, ascending. !!! note "What is never in these responses" Submitted field values, the snapshotted signing secret, the request headers, - and the response body. The first three are never returned by any route; the - last is never stored. + and the response body. The signing secret and the headers are never returned + by any route, and the response body is never stored. Field values are + returned only by the authenticated + [submission routes](submission-management.md). ## `POST /deliveries/{delivery_id}/replay` @@ -107,4 +109,5 @@ and leaves `attempt_count` and every historical attempt row untouched. - [Delivery inspection and replay](../guides/delivery-replay.md) - [Webhook delivery](../guides/webhooks.md) for the retry schedule +- [Submission Management](submission-management.md) for what a delivery carried - [Errors](errors.md) for the full error table diff --git a/docs/api/errors.md b/docs/api/errors.md index b046a55..79313fe 100644 --- a/docs/api/errors.md +++ b/docs/api/errors.md @@ -27,6 +27,7 @@ names or SQL. | 404 | `invalid_endpoint_id` | Submission path is not a well-formed endpoint ID | | 404 | `endpoint_not_found` | Endpoint ID is well formed but no such endpoint exists | | 404 | `delivery_not_found` | No delivery with that ID exists | +| 404 | `submission_not_found` | No submission with that ID exists | | 404 | `not_found` | Unknown path | | 405 | `method_not_allowed` | Wrong method for a known path | | 409 | `endpoint_inactive` | Endpoint exists but is not accepting submissions | @@ -40,6 +41,9 @@ names or SQL. | 422 | `invalid_endpoint_id` | Endpoint ID in a request body breaks the ID rules | | 422 | `invalid_request` | Request body or query parameters failed schema validation | | 422 | `invalid_webhook_url` | Webhook destination is malformed or not permitted | +| 422 | `invalid_time_range` | `received_after` is not strictly earlier than `received_before` | +| 422 | `export_too_large` | An export matches more than `FORMS_EXPORT_MAX_SUBMISSIONS` | +| 422 | `unsupported_export_format` | Export `format` is not `json` or `csv` | | 422 | `file_upload_not_supported` | A multipart part carried a file | | 422 | ingestion rule codes | See below | | 429 | `rate_limit_exceeded` | A public ingestion rate limit was exhausted | @@ -86,6 +90,20 @@ not permitted, which needs a permission model this build does not have. alike, so that a guesser cannot sort their attempts into "nearly right" and "wrong". The credential a request sent is never echoed back in an error. +### `export_too_large` is a `422`, not a `413` + +The request is well formed, and what has to change is the filter, which is part +of the request. It is refused rather than truncated so that an export is either +everything that matched or an error, never a quietly incomplete file. See +[Exporting submissions](../guides/exporting-submissions.md#size-limit). + +### `invalid_time_range` is refused, an empty match is not + +A filter matching nothing is an empty page, because that is a fact about the +data. A range where `received_after` is on or after `received_before` can never +match anything at all, which is a mistake in the request; answering it with an +empty page would hide the mistake. + ### `storage_unavailable` rather than `500` A database failure returns `503`, because the request itself was fine and @@ -101,5 +119,5 @@ published. ## Related -- [Submissions](submissions.md), [Endpoints](endpoints.md) and [Deliveries](deliveries.md) for which codes each route can return +- [Submissions](submissions.md), [Endpoints](endpoints.md), [Deliveries](deliveries.md) and [Submission Management](submission-management.md) for which codes each route can return - [Authentication](authentication.md) for the `401` cases diff --git a/docs/api/submission-management.md b/docs/api/submission-management.md new file mode 100644 index 0000000..1bb7b62 --- /dev/null +++ b/docs/api/submission-management.md @@ -0,0 +1,157 @@ +# Submission Management API + +Every route on this page requires a management API key. See +[Authentication](authentication.md). + +These are the only routes that return what somebody typed into a form. Public +ingestion answers with an acknowledgement, and the delivery routes report +operational state. Reading a submission back is authenticated, always. + +For what these routes are for, see +[Browsing submissions](../guides/submission-management.md) and +[Exporting submissions](../guides/exporting-submissions.md). + +## `GET /submissions` + +Lists stored submissions, newest first. + +A listing is metadata. It reports how many values a submission carried, never +what they were, so walking a busy endpoint does not spread form content across +pages nobody asked for. Use the detail route or an export for the values. + +**Query parameters** + +| Parameter | Default | Meaning | +| --- | --- | --- | +| `endpoint_id` | none | Only submissions for one endpoint | +| `received_after` | none | Only submissions received **strictly after** this instant | +| `received_before` | none | Only submissions received **strictly before** this instant | +| `limit` | `50` | Page size, 1 to 100 | +| `cursor` | none | The previous page's `next_cursor` | + +Both time bounds are ISO 8601 and both are exclusive: a submission received at +exactly the given instant is not returned by either. That is what lets you page +forward through a range by passing the last timestamp you saw back as +`received_after` without re-reading the row you took it from. + +Paging works exactly as it does for endpoints. See +[Pagination](endpoints.md#pagination). + +**Item fields** + +| Field | Meaning | +| --- | --- | +| `id` | Submission identifier | +| `endpoint_id` | The endpoint the submission was addressed to | +| `received_at` | When the API accepted the body | +| `field_count` | Number of name/value pairs, counting a repeated field once per value | +| `idempotent` | Whether it was sent with an `Idempotency-Key` | +| `delivery` | The webhook delivery it owes, or `null` | + +`delivery`, when present, carries `id`, `state` and `attempt_count`. A submission +owes at most one delivery. + +**Responses** + +| Status | Code | Cause | +| --- | --- | --- | +| `200` | | A page of submissions | +| `401` | `authentication_required`, `invalid_api_key` | Credential missing or unusable | +| `422` | `invalid_time_range` | `received_after` is not strictly earlier than `received_before` | +| `422` | `invalid_cursor` | The cursor does not continue from a known row | +| `422` | `invalid_request` | Unparseable timestamp, or `limit` outside 1 to 100 | +| `503` | `storage_unavailable` | The database could not be reached | + +A filter that matches nothing is not an error. It is a filter that selected no +rows, and the answer is an empty page. A range that *cannot* match anything, such +as `received_after` on or later than `received_before`, is refused instead: +that is a mistake in the request rather than a fact about the data. + +## `GET /submissions/{submission_id}` + +One submission, including the values it carried. + +**Fields** + +Everything a listing item carries, plus: + +| Field | Meaning | +| --- | --- | +| `fields` | The submitted field names and their ordered values | + +`fields` is exactly what was stored, and every value is a list: + +```json +{ + "email": ["dev@example.com"], + "topics": ["billing", "api"] +} +``` + +A field submitted once is a one-element list rather than a bare string, and a +repeated field keeps its values in the order they were sent. That is the same +shape the signed webhook payload uses, so a receiver and an operator see the same +thing. + +**Responses** + +| Status | Code | Cause | +| --- | --- | --- | +| `200` | | The submission and its fields | +| `401` | `authentication_required`, `invalid_api_key` | Credential missing or unusable | +| `404` | `submission_not_found` | No submission with that ID exists | +| `503` | `storage_unavailable` | The database could not be reached | + +A submission that [retention](../operations/retention.md) has deleted answers +`404`, the same as one that never existed. From outside, both mean this service +does not hold it. + +!!! note "What is never in these responses" + + The payload fingerprint, the `Idempotency-Key` the submission was sent with, + any webhook signing secret, and any management credential. The fingerprint is + an internal detail of how a retry is recognised. The idempotency key is a + secret in practice: anyone holding it can resolve it to a submission through + the public ingestion route. + +## `GET /submissions/export` + +Exports the submissions a filter matches, as a downloadable file. + +**Query parameters** + +The same `endpoint_id`, `received_after` and `received_before` as the listing, +with the same exclusive bounds, plus: + +| Parameter | Default | Meaning | +| --- | --- | --- | +| `format` | `json` | `json` or `csv` | + +There is no `limit` and no `cursor`. An export is the whole filtered set or an +error, never a page. + +**Responses** + +| Status | Code | Cause | +| --- | --- | --- | +| `200` | | The export, as an attachment | +| `401` | `authentication_required`, `invalid_api_key` | Credential missing or unusable | +| `422` | `export_too_large` | The filter matches more than `FORMS_EXPORT_MAX_SUBMISSIONS` | +| `422` | `unsupported_export_format` | `format` is not `json` or `csv` | +| `422` | `invalid_time_range` | `received_after` is not strictly earlier than `received_before` | +| `503` | `storage_unavailable` | The database could not be reached | + +Both formats are sent with `Content-Disposition: attachment` and a generated +filename such as `hymical-submissions-2026-08-25.json`. Nothing a caller supplied +reaches the filename. + +The format and the size limit are described in +[Exporting submissions](../guides/exporting-submissions.md). + +## Related + +- [Browsing submissions](../guides/submission-management.md) +- [Exporting submissions](../guides/exporting-submissions.md) +- [Retention](../operations/retention.md) +- [Data handling](../reference/data-handling.md) +- [Errors](errors.md) for the full error table diff --git a/docs/api/submissions.md b/docs/api/submissions.md index 745794e..d0c6acf 100644 --- a/docs/api/submissions.md +++ b/docs/api/submissions.md @@ -95,12 +95,17 @@ whether to restart the process. It is not a readiness check. ## Reading submissions back -There is **no route that returns a stored submission**. Submitted values are not -exposed by any endpoint, including the delivery views. Retrieval, export and -retention are not implemented. See [Limitations](../reference/limitations.md). +**No public route returns a stored submission.** This one answers with an +acknowledgement, and the delivery views carry no submitted values either. + +Reading a submission back is authenticated, on the routes in +[Submission Management](submission-management.md). See +[Data handling](../reference/data-handling.md) for everywhere a submitted value +does and does not go. ## Related - [Form ingestion](../guides/form-ingestion.md) for content types, limits and repeated fields - [Idempotency](../guides/idempotency.md) +- [Submission Management](submission-management.md) for reading submissions back - [Errors](errors.md) for the full error table diff --git a/docs/architecture/security.md b/docs/architecture/security.md index 024525a..8c4ec6c 100644 --- a/docs/architecture/security.md +++ b/docs/architecture/security.md @@ -74,8 +74,10 @@ it is optional rather than required. - The credential a request sent is never echoed back. - An idempotency conflict says the content differs. It never describes the earlier submission, so a key cannot be used to read back somebody else's form. -- Submitted field values are never returned by any route, including the delivery - views. +- Submitted field values are never returned by a public route, and never by a + delivery view. The only routes that return them are the authenticated + submission detail and export routes, where returning them is the request. See + [Data handling](../reference/data-handling.md). ## SSRF guardrails diff --git a/docs/guides/delivery-replay.md b/docs/guides/delivery-replay.md index ba13fd3..3e2db24 100644 --- a/docs/guides/delivery-replay.md +++ b/docs/guides/delivery-replay.md @@ -44,10 +44,15 @@ An unknown `state` is refused with `422 invalid_request`. An `endpoint_id` that matches nothing is not an error: it is a filter that selected no rows, and the answer is an empty page. -!!! note "Submitted field values are never returned" +!!! note "A delivery view carries no submitted field values" - Not in the listing and not in the detail. There is no route that reads a - submission back, and a delivery view is not a way around that. + Not in the listing and not in the detail. To see what a delivery was + carrying, take its `submission_id` to + [`GET /submissions/{id}`](../api/submission-management.md). + +`submission_id` is `null` when [retention](../operations/retention.md) has removed +the submission. That only ever happens to a delivery that already succeeded: a +failed delivery keeps its submission precisely so that it stays replayable. ## One delivery, with its attempt history diff --git a/docs/guides/exporting-submissions.md b/docs/guides/exporting-submissions.md new file mode 100644 index 0000000..e149d2a --- /dev/null +++ b/docs/guides/exporting-submissions.md @@ -0,0 +1,186 @@ +# Exporting Submissions + +One authenticated route writes a filtered set of submissions as a downloadable +file, in JSON or in CSV. + +```bash +curl -OJ -G "http://127.0.0.1:8000/submissions/export" \ + -H "Authorization: Bearer $HYMICAL_KEY" \ + --data-urlencode "endpoint_id=contact-form" \ + --data-urlencode "received_after=2026-08-01T00:00:00Z" +``` + +The filters are the same three the listing takes, with the same exclusive +bounds. See [Browsing submissions](submission-management.md#filtering). + +## JSON + +The default. One object with one key: + +```json +{ + "submissions": [ + { + "id": "sub_48984534f33749c49a88de2d59400dce", + "endpoint_id": "contact-form", + "received_at": "2026-08-25T10:00:00Z", + "fields": { + "email": ["dev@example.com"], + "topics": ["billing", "api"] + } + } + ] +} +``` + +`fields` is exactly what the detail route returns, and exactly what is stored. + +The document is written as the rows arrive rather than built in memory first, so +an export of thousands of submissions starts sending immediately. + +## CSV + +```bash +curl -OJ -G "http://127.0.0.1:8000/submissions/export" \ + -H "Authorization: Bearer $HYMICAL_KEY" \ + --data-urlencode "format=csv" +``` + +```csv +submission_id,endpoint_id,received_at,email,topics +sub_48984534f33749c49a88de2d59400dce,contact-form,2026-08-25T10:00:00Z,"[""dev@example.com""]","[""billing"",""api""]" +``` + +Three fixed metadata columns come first. After them there is one column per field +name, and the set of columns is the union of every field name in the export, in +the order each was first met. + +That ordering means a CSV of one endpoint's submissions comes out roughly in the +order that endpoint's form asks its questions. Sorting alphabetically would be +just as deterministic and would read worse. + +### Why every value cell is a JSON array + +A form field can be submitted more than once, so a cell has to be able to hold +several values. Almost every separator you might reach for, a comma, a semicolon, +a pipe, can also appear inside somebody's answer, and then the cell is ambiguous +and nothing can tell the two apart afterwards. + +So a cell holds a JSON array: + +| Submitted | Cell | +| --- | --- | +| `topics=billing&topics=api` | `["billing","api"]` | +| `email=dev@example.com` | `["dev@example.com"]` | +| `phone=` | `[""]` | +| field not submitted at all | empty cell | + +A field submitted once is still a one-element array, so a column has one shape +rather than two. An absent field and a field submitted empty stay distinguishable, +which a shared separator could not manage. + +Escaping happens twice and neither layer is hand-written: JSON quotes the values +inside the cell, and Python's `csv` module quotes the cell inside the row. A +comma, a double quote, a newline or a tab in an answer survives both and parses +back to exactly what was submitted. + +### Spreadsheet formulas + +A spreadsheet reads a cell beginning with `=`, `+`, `-` or `@` as a formula +rather than as text. Exports get opened in spreadsheets, and both field names and +field values are written by whoever filled the form in. + +Any cell that would begin with one of those characters, or with a tab or a +carriage return, is written with a leading apostrophe. An apostrophe is what a +spreadsheet itself writes to mean "this is text", so the cell displays the +original characters and is not evaluated. + +In practice a **value** cell never needs it, because every value cell is a JSON +array and so begins with `[`. A **field name** is another matter: a form is free +to call a field `=cmd()`, and that name becomes a header cell. The rule is +applied to every cell so that the property holds regardless. + +Only the export representation is changed, and only by prefixing. Nothing stored +is altered, nothing is dropped, and the original characters are still there to be +read. The API's JSON responses and the webhook payload are untouched. + +### Reading a CSV back + +Parse the metadata columns as strings and every other cell as JSON: + +```python +import csv, json + +with open("hymical-submissions-2026-08-25.csv", newline="", encoding="utf-8") as handle: + for row in csv.DictReader(handle): + fields = { + name: json.loads(cell) + for name, cell in row.items() + if name not in ("submission_id", "endpoint_id", "received_at") and cell + } + print(row["submission_id"], fields) +``` + +If you enabled the spreadsheet escaping path on a field name, strip a single +leading apostrophe from the header before matching on it. + +## Size limit + +An export returns the whole filtered set, so the set has to be bounded. A filter +matching more than `FORMS_EXPORT_MAX_SUBMISSIONS`, which defaults to `10000`, is +refused: + +```json +{ + "error": { + "code": "export_too_large", + "message": "This filter matches more than 10000 submissions, ...", + "details": { "limit": 10000 } + } +} +``` + +Refused rather than truncated, on purpose. A silently shortened export is a file +somebody archives and only discovers is incomplete much later. + +To export more than the limit, narrow the range and take it in parts: + +```bash +for month in 01 02 03; do + curl -OJ -G "http://127.0.0.1:8000/submissions/export" \ + -H "Authorization: Bearer $HYMICAL_KEY" \ + --data-urlencode "received_after=2026-${month}-01T00:00:00Z" \ + --data-urlencode "received_before=2026-${month}-28T00:00:00Z" +done +``` + +Raising `FORMS_EXPORT_MAX_SUBMISSIONS` is the other option. Bear in mind what it +buys: a CSV is built in one pass before it is sent, because a CSV header is the +union of every field name in the export and that is only known once the last row +has been read. The limit is what bounds how much that pass holds. JSON has no +such dependency and streams. + +## Filenames + +Both formats are sent with `Content-Disposition: attachment` and a generated +name: + +``` +hymical-submissions-2026-08-25.json +hymical-submissions-2026-08-25.csv +``` + +`curl -OJ` and every browser will use it. The date is the day the export was +requested, in UTC. + +The filename is generated entirely by this service. No endpoint identifier and no +filter value goes into it, so there is no caller input in it to sanitise and +nothing that could break out of the header. It also means two exports taken on +the same day share a name; rename them, or pass `-o` yourself. + +## Related + +- [Submission Management API](../api/submission-management.md) +- [Browsing submissions](submission-management.md) +- [Data handling](../reference/data-handling.md) +- [Retention](../operations/retention.md) diff --git a/docs/guides/idempotency.md b/docs/guides/idempotency.md index c8bfbf9..48e8a47 100644 --- a/docs/guides/idempotency.md +++ b/docs/guides/idempotency.md @@ -77,9 +77,11 @@ fingerprint, so an honest retry always matches. A key belongs to **one endpoint**. The same key may be used once per endpoint without conflicting. -There is no expiry: a key is spent for as long as its submission is stored. That -is a known limitation, and it belongs with retention, which is not implemented. -See [Limitations](../reference/limitations.md). +A key has no expiry of its own: it is spent for as long as its submission is +stored, because the uniqueness constraint lives on that row. Configuring +[retention](../operations/retention.md) is therefore what eventually releases +one, and without it the table only grows. Retention ages are far longer than any +client's retry window, so this is bookkeeping rather than a behaviour change. ## Key format diff --git a/docs/guides/submission-management.md b/docs/guides/submission-management.md new file mode 100644 index 0000000..7ce1372 --- /dev/null +++ b/docs/guides/submission-management.md @@ -0,0 +1,143 @@ +# Browsing Submissions + +Submissions have always been stored. Until now the only way to see one was to +receive its webhook. This guide covers reading them back through the management +API. + +Every route here needs a management API key. The values somebody typed into your +form are never returned by a public route. + +## Listing + +```bash +curl "http://127.0.0.1:8000/submissions" \ + -H "Authorization: Bearer $HYMICAL_KEY" +``` + +```json +{ + "items": [ + { + "id": "sub_48984534f33749c49a88de2d59400dce", + "endpoint_id": "contact-form", + "received_at": "2026-08-25T10:00:00Z", + "field_count": 3, + "idempotent": false, + "delivery": { + "id": "whd_9f2c1a7b4e8d4c3fa1b6d0e5c8a72f31", + "state": "delivered", + "attempt_count": 1 + } + } + ], + "next_cursor": null +} +``` + +The listing is metadata only. `field_count` says how much the submission carried, +not what it was. A field submitted three times counts three times, the same as it +does on the ingestion response. + +This is deliberate. Walking a busy endpoint means fetching page after page, and +each of those pages would otherwise be a copy of somebody's form data sitting in +a log, a proxy cache or a terminal scrollback. Ask for the values when you want +the values. + +## One submission + +```bash +curl "http://127.0.0.1:8000/submissions/sub_48984534f33749c49a88de2d59400dce" \ + -H "Authorization: Bearer $HYMICAL_KEY" +``` + +```json +{ + "id": "sub_48984534f33749c49a88de2d59400dce", + "endpoint_id": "contact-form", + "received_at": "2026-08-25T10:00:00Z", + "field_count": 3, + "idempotent": false, + "delivery": { "id": "whd_9f2c...", "state": "delivered", "attempt_count": 1 }, + "fields": { + "email": ["dev@example.com"], + "topics": ["billing", "api"] + } +} +``` + +Every value is a list, always. A field submitted once is a one-element list +rather than a bare string, so a consumer never has to check which of the two it +got. Repeated values keep the order they were submitted in, because a checkbox +group is what repeated field names are for and collapsing them would discard +what somebody chose. + +That is the same shape a signed webhook payload carries, so an operator reading +a submission and a receiver handling it see the same thing. + +## Filtering + +Three filters, all optional and all combinable: + +```bash +curl -G "http://127.0.0.1:8000/submissions" \ + -H "Authorization: Bearer $HYMICAL_KEY" \ + --data-urlencode "endpoint_id=contact-form" \ + --data-urlencode "received_after=2026-08-01T00:00:00Z" \ + --data-urlencode "received_before=2026-09-01T00:00:00Z" +``` + +Both time bounds are **exclusive**. A submission received at exactly +`2026-08-01T00:00:00Z` is not returned by either bound. + +That is what makes a walk forward through time safe: take the `received_at` of +the oldest row you have seen, pass it as `received_before`, and the next request +continues rather than repeating it. + +A range that cannot match anything, where `received_after` is on or later than +`received_before`, is refused with `invalid_time_range` rather than answered with +an empty page. An empty page would hide the mistake. + +There is no search over field values, and none is planned for this build. See +[Limitations](../reference/limitations.md). + +## Paging + +Cursor paging, exactly as for endpoints and deliveries: + +```bash +curl -G "http://127.0.0.1:8000/submissions" \ + -H "Authorization: Bearer $HYMICAL_KEY" \ + --data-urlencode "limit=100" \ + --data-urlencode "cursor=sub_48984534f33749c49a88de2d59400dce" +``` + +Ordering is newest first, and it is total: submissions received in the same +instant are separated by their identifier, so a page boundary never repeats or +skips a row. A full page always hands back a cursor, so a walk ends on one empty +page rather than on a null cursor. + +Filters must stay the same across a walk. Changing one part way through moves the +boundary the cursor was taken from. + +## Finding what a delivery carried + +A delivery reports its `submission_id`, and a submission reports its delivery. So +a failed delivery leads to the content that failed to arrive: + +```bash +curl "http://127.0.0.1:8000/deliveries?state=failed" \ + -H "Authorization: Bearer $HYMICAL_KEY" +``` + +Take a `submission_id` from that page and read it back. That is usually enough to +tell a receiver that was down from a payload the receiver refused. + +If `submission_id` is `null`, [retention](../operations/retention.md) has removed +the submission. That only ever happens to a delivery that already succeeded. + +## Related + +- [Submission Management API](../api/submission-management.md) for every parameter +- [Exporting submissions](exporting-submissions.md) +- [Retention](../operations/retention.md) +- [Data handling](../reference/data-handling.md) diff --git a/docs/index.md b/docs/index.md index 4bee089..ddc1654 100644 --- a/docs/index.md +++ b/docs/index.md @@ -55,6 +55,20 @@ your webhook with an HMAC signature and a bounded retry schedule. [Idempotency](guides/idempotency.md) +- __Submissions you can read back__ + + Browse and filter what your forms collected, read one submission in full, and + export a range as JSON or CSV. Authenticated, always. + + [Browsing submissions](guides/submission-management.md) + +- __Retention you control__ + + Nothing is deleted until you run the cleanup command, and it never removes a + submission a delivery could still need. + + [Retention](operations/retention.md) + ## Requirements @@ -117,8 +131,8 @@ response in about five minutes. - __Guides__ - How ingestion, idempotency, webhooks, rate limiting, endpoint management and - delivery replay actually behave. + How ingestion, idempotency, webhooks, rate limiting, endpoint management, + delivery replay and submission export actually behave. [Read the guides](guides/form-ingestion.md) @@ -130,8 +144,8 @@ response in about five minutes. - __Operations__ - Running the worker, applying migrations, sitting behind a reverse proxy, and - every configuration variable. + Running the worker, applying migrations, sweeping expired submissions, + sitting behind a reverse proxy, and every configuration variable. [Operate it](operations/worker.md) @@ -142,6 +156,13 @@ response in about five minutes. [Understand it](architecture/overview.md) +- __Data handling__ + + Where a submitted value goes, where it never goes, and what deleting one + does and does not remove. + + [Handle it carefully](reference/data-handling.md) + - __Limitations__ An honest list of what this build does not do yet. Worth reading before you @@ -156,12 +177,14 @@ response in about five minutes. **Early development.** The service registers endpoints, stores submissions with the durable obligation to deliver them, and runs a worker that performs the signed delivery and retries it. Endpoint management, delivery inspection, manual -replay and public ingestion rate limiting are all implemented and covered by -tests, including a PostgreSQL suite that exercises real concurrency. +replay, public ingestion rate limiting, submission retrieval and export, and +operator-run retention cleanup are all implemented and covered by tests, +including a PostgreSQL suite that exercises real concurrency. There is no spam protection, no CAPTCHA and no content classification. Rate -limiting bounds volume, not junk. See [Limitations](reference/limitations.md) for -the full picture. +limiting bounds volume, not junk. Retention is never automatic: nothing is +deleted until an operator runs the cleanup command. See +[Limitations](reference/limitations.md) for the full picture. ## License diff --git a/docs/operations/configuration-reference.md b/docs/operations/configuration-reference.md index 53aeebb..367a98a 100644 --- a/docs/operations/configuration-reference.md +++ b/docs/operations/configuration-reference.md @@ -78,6 +78,21 @@ value. It must be at least 16 characters. The lease must comfortably outlast the connect and read timeouts combined. See [Worker](worker.md). +## Submission retrieval and retention + +| Variable | Default | Meaning | +| --- | --- | --- | +| `FORMS_SUBMISSION_RETENTION_DAYS` | `0` | Days a submission is kept before it becomes eligible for deletion | +| `FORMS_EXPORT_MAX_SUBMISSIONS` | `10000` | Largest number of submissions one export may return | + +`0`, the default, keeps submissions indefinitely. A positive value makes older +submissions eligible for deletion, and deletes nothing on its own: cleanup is a +command an operator runs. See [Retention](retention.md). + +An export matching more than `FORMS_EXPORT_MAX_SUBMISSIONS` is refused rather +than truncated. See +[Exporting submissions](../guides/exporting-submissions.md#size-limit). + ## There is no management key setting Deliberately. Keys live in the database so that creating and revoking one needs diff --git a/docs/operations/retention.md b/docs/operations/retention.md new file mode 100644 index 0000000..22cb303 --- /dev/null +++ b/docs/operations/retention.md @@ -0,0 +1,171 @@ +# Submission Retention + +Stored submissions are kept forever unless you configure otherwise, and nothing +is ever deleted automatically. Retention is one setting and one command an +operator runs. + +## Configuring it + +```bash +FORMS_SUBMISSION_RETENTION_DAYS=90 +``` + +Unset, or `0`, means keep submissions indefinitely. That is the default, and it +is the only safe thing an unconfigured value can mean: a service that started +deleting form data because nobody had set a variable would be indefensible. + +Setting it makes submissions older than that age *eligible* for deletion. It does +not delete anything on its own. + +## Running the sweep + +```bash +python -m hymical_forms.cli cleanup-submissions --dry-run +``` + +``` +Retention keeps submissions for 90 days, so the cutoff is 2026-05-27T12:00:00+00:00. +412 submission(s) received before then are eligible for deletion. A submission +whose delivery is still pending, processing or replayable is not eligible, +however old it is. +Dry run: nothing was deleted. +``` + +A dry run performs one counting query and writes nothing at all. Drop the flag to +delete: + +```bash +python -m hymical_forms.cli cleanup-submissions +``` + +``` +Deleted 412 submission(s) in batches of up to 500. Delivery records and their +attempt history were left in place. +``` + +The command reads `FORMS_DATABASE_URL` like every other operator command, and +checks the schema revision before it touches anything. + +### Sweeping without configuring retention + +```bash +python -m hymical_forms.cli cleanup-submissions --older-than-days 365 +``` + +An explicit age overrides the configured one, which is what lets a deployment +that keeps submissions indefinitely still clear out one old range by hand. + +With no retention configured and no `--older-than-days`, the command refuses: + +``` +No submission retention is configured, so nothing is eligible for deletion. Set +FORMS_SUBMISSION_RETENTION_DAYS, or pass --older-than-days to sweep this once +without configuring anything. +``` + +Refused rather than treated as "delete nothing", so an operator who expected a +sweep to happen finds out that it did not. + +### Scheduling + +There is no daemon, and no scheduler ships with this service. Use whatever +already runs your periodic work: + +```cron +17 4 * * * cd /srv/forms && /srv/forms/.venv/bin/python -m hymical_forms.cli cleanup-submissions +``` + +A sweep that deletes stored form data should be something a person set up +deliberately against a database they named, not something an API process does on +the side of serving a request. + +## What is eligible, and what is not + +This is the part worth understanding, because age alone does not decide it. + +A queued webhook delivery does **not** carry a copy of the submitted fields. The +worker loads the submission and builds the payload from it at the moment it +sends. So the submission is needed for as long as any further attempt is +possible. + +A submission older than the cutoff is deleted when: + +- it owes no delivery at all, because its endpoint has no webhook; **or** +- its delivery is `delivered`, and so will never be sent again. + +It is kept, however old it is, when its delivery is: + +| State | Why it is kept | +| --- | --- | +| `pending` | Waiting for its due time. The payload has not been sent yet | +| `processing` | A worker is holding it right now | +| `failed` | Replayable. A replay rebuilds the payload from the submission | + +`failed` is the one that catches people out. A terminally failed delivery is not +finished, it is waiting for an operator to +[replay it](../guides/delivery-replay.md), and a replay reads the submission +again. Retention that took the payload would leave a delivery that can be +requeued and can never succeed. So a failed delivery protects its submission +indefinitely. + +The practical consequence: if you want retention to reach those, resolve them +first. Replay the ones worth replaying, and accept the ones that are not. + +## What a sweep never destroys + +Deleting a submission does not delete the record of what this service did about +it. The delivery row and every attempt made for it stay exactly where they are; +the database unlinks them from the submission and leaves them standing. + +After a sweep, such a delivery still reports its state, its attempt counts, its +timings, its destination and its endpoint. Its `submission_id` reads `null`. Its +attempt history is still readable through +`GET /deliveries/{delivery_id}`. + +Operational history is worth more than the form content it was carrying, and this +is where that judgement is made concrete. It is why the foreign keys are +`ON DELETE SET NULL` rather than `ON DELETE CASCADE`, and why a delivery records +its own endpoint rather than reaching it through the submission. + +## Batching and safety + +Deletion is many short committed transactions rather than one long one. Each +batch takes at most `--batch-size` submissions, defaulting to 500, deletes them +and commits. + +```bash +python -m hymical_forms.cli cleanup-submissions --batch-size 100 +``` + +A single statement over a large backlog would hold locks on every row it touched +for as long as the whole sweep took. Batching means a busy database keeps +serving, and a run that is interrupted has still durably removed everything it +reported. + +A run also stops after a fixed number of batches so that a sweep against an +enormous backlog comes back rather than running unbounded. When that happens the +command says so, and running it again continues. + +## What deletion frees + +Deleting a submission releases the `Idempotency-Key` it was sent with, since the +uniqueness constraint is on the row. A client retrying with that key long after +the submission has been swept creates a new submission rather than resolving to +the old one. In practice retention ages are far longer than any client's retry +window. + +## Limits + +- **Deletion is permanent.** There is no soft delete, no archive and no undo. Take + an [export](../guides/exporting-submissions.md) first if you want a copy. +- **Nothing is scheduled for you.** Retention only happens when the command runs. +- **A failed delivery pins its submission indefinitely.** See above. +- **Delivery records are never removed.** They accumulate. Delivery retention is + not implemented. + +## Related + +- [Exporting submissions](../guides/exporting-submissions.md) to keep a copy first +- [Delivery replay](../guides/delivery-replay.md) for resolving what is pinned +- [Data handling](../reference/data-handling.md) +- [Configuration reference](configuration-reference.md) diff --git a/docs/reference/data-handling.md b/docs/reference/data-handling.md new file mode 100644 index 0000000..5ce55c8 --- /dev/null +++ b/docs/reference/data-handling.md @@ -0,0 +1,106 @@ +# Data Handling + +What this service does with the contents of a submission, and what it does not. +This page describes behaviour. It is not legal or compliance advice, and no +claim of compliance with any regime is made here or anywhere else in this +documentation. + +**You are responsible for the data you collect.** This service stores and +forwards what your forms ask for. What you ask for, why, what you tell people +about it, and how long you keep it are your decisions. + +## Where submitted values go + +A submitted field value reaches exactly four places: + +| Destination | When | +| --- | --- | +| The `submissions` table | Always, on acceptance | +| Your webhook receiver | If the endpoint has a webhook, in the signed payload | +| `GET /submissions/{id}` | When an authenticated operator asks for it | +| `GET /submissions/export` | When an authenticated operator exports it | + +Nowhere else. In particular: + +- **No public route returns a submitted value.** `POST /f/{endpoint_id}` answers + with an acknowledgement: an identifier, a timestamp, a count. It does not echo + what was sent, even though the sender already has it. +- **Nothing is logged.** No field name and no field value is written to the + application log, including by the routes that return them. What is logged about + an export is who asked, for what endpoint filter, in what format, and how many + rows it came to. +- **Nothing appears in an error.** An idempotency conflict says the content + differs, never how. A rate limit refusal names the scope and the wait, never a + subject or a payload. +- **Nothing reaches the rate limit tables.** Those hold a limiter, a subject and a + count. The per-address subject is a digest, and no raw address is stored. +- **Delivery records carry no submitted values.** A delivery holds its + destination, its state and its counters. Attempt records hold an outcome and a + bounded error message. Receiver response bodies are not stored at all. + +## Reading submissions back is authenticated + +Every route that returns submitted values requires a management API key. There is +no unauthenticated read path and no signed-URL scheme. + +A management key administers the whole service. There is no way to issue a key +that can read one endpoint's submissions and not another's. See +[Limitations](limitations.md). + +## Listings are metadata + +`GET /submissions` returns a count of values, never the values. Walking a busy +endpoint means fetching page after page, and each of those pages would otherwise +be a copy of somebody's form data in a proxy cache or a terminal scrollback. Ask +for the values when you want the values. + +## Exports leave this service + +An export is a file. Once it is downloaded, this service has no further say in +where it goes, who opens it or how long it lives. Treat one the way you would +treat a database dump. + +CSV exports are additionally written so that a spreadsheet does not evaluate a +cell as a formula. See +[Exporting submissions](../guides/exporting-submissions.md#spreadsheet-formulas). + +## What is never returned + +| Not returned | Why | +| --- | --- | +| The payload fingerprint | An internal detail of how a retry is recognised | +| The `Idempotency-Key` | A secret in practice: it resolves to a submission through the public route | +| A webhook signing secret | Leaves the service once, in the response that generated it | +| A management credential | Only a digest is stored, so there is nothing to return | + +## Deletion + +Submissions are kept indefinitely unless you configure +[retention](../operations/retention.md) and run the cleanup command. + +There is no per-submission delete route, no endpoint deletion and no bulk erase +by field value. Removing one person's data means finding their submissions, +which you can do with the listing filters and an export, and deleting them +directly in the database. That is a real gap; see +[Limitations](limitations.md). + +Retention deletion is permanent. It also unlinks, rather than removes, the +delivery records that referenced the submission: what this service tried to do +survives, the form content does not. See +[Retention](../operations/retention.md#what-a-sweep-never-destroys). + +## Transport + +Nothing here encrypts anything at rest. Submitted values are stored as ordinary +JSON in PostgreSQL, readable by anyone who can read the database. Disk +encryption, database access control and backup handling are yours. + +This service does not terminate TLS. Run it behind a reverse proxy that does. See +[Reverse proxy](../operations/reverse-proxy.md). + +## Related + +- [Retention](../operations/retention.md) +- [Exporting submissions](../guides/exporting-submissions.md) +- [Security](../architecture/security.md) +- [Limitations](limitations.md) diff --git a/docs/reference/limitations.md b/docs/reference/limitations.md index 180667e..62b50c4 100644 --- a/docs/reference/limitations.md +++ b/docs/reference/limitations.md @@ -72,8 +72,11 @@ deploy it, and it is kept complete rather than flattering. ## Idempotency -- **Idempotency keys never expire.** A key stays spent for as long as its - submission is stored, so the table only grows. Expiry belongs with retention. +- **Idempotency keys expire only with their submission.** A key stays spent for as + long as its submission is stored. Configuring + [retention](../operations/retention.md) is what eventually releases one, because + the uniqueness constraint lives on the row; without retention the table only + grows. - **Idempotency keys are shared across all clients of an endpoint,** because there is nothing to scope them to yet. Guessing another client's key returns that submission's ID and timestamp, though never its contents. Random keys of the @@ -87,9 +90,28 @@ deploy it, and it is kept complete rather than flattering. ## Data and API surface -- **No way to read submissions back over the API.** They are stored, and a - delivery can be inspected, but the submitted values themselves are deliberately - not exposed by any route. Retrieval, export and retention are not implemented. +- **Submissions cannot be searched by content.** The listing filters by endpoint + and by received time and nothing else. There is no field-value search, no + full-text search, and no filtering by delivery state. Export the range and grep + it. See [Browsing submissions](../guides/submission-management.md). +- **There is no per-submission delete route.** Removing one person's data means + finding their submissions through the listing filters and deleting the rows + directly in the database. Retention deletes by age, not by who sent something. +- **A failed delivery pins its submission indefinitely.** Retention will not + delete a submission whose delivery could still be replayed, because a replay + rebuilds the payload from it. Resolve failed deliveries if you want retention to + reach them. See [Retention](../operations/retention.md). +- **Delivery records are never deleted.** Retention covers submissions only, so + the delivery and attempt tables grow without bound. Deleting a submission + unlinks its delivery rather than removing it. +- **Retention is never automatic.** Nothing is deleted until an operator runs + `cleanup-submissions`. There is no scheduler and no daemon; use cron. +- **An export is capped and refused rather than truncated,** at + `FORMS_EXPORT_MAX_SUBMISSIONS`. Larger sets have to be taken in ranges. There is + no background export job and no download that resumes. +- **A CSV export is built in one pass before it is sent,** because its header is + the union of every field name in the export. The size cap is what bounds that. + JSON exports stream. - **Endpoints cannot be deleted, and an endpoint ID cannot be changed.** Disabling an endpoint is the way to stop it accepting submissions. - **No file uploads.** Multipart text fields are accepted; file parts are @@ -97,6 +119,8 @@ deploy it, and it is kept complete rather than flattering. - **`multipart/form-data` bodies are buffered in memory,** bounded by `FORMS_MAX_BODY_BYTES`. - **Submission IDs are opaque and not yet guaranteed stable in format.** +- **Nothing is encrypted at rest.** Submitted values are ordinary JSON in + PostgreSQL. See [Data handling](data-handling.md). ## Operations @@ -120,12 +144,13 @@ deploy it, and it is kept complete rather than flattering. ## Not implemented at all -Dashboards, a frontend, export, retention, submission browsing, endpoint -deletion, spam filtering, CAPTCHA, email verification, user accounts, tenancy, -per-endpoint or per-tenant quotas, and billing. +Dashboards, a frontend, submission search, scheduled retention, delivery +retention, endpoint deletion, spam filtering, CAPTCHA, email verification, user +accounts, tenancy, per-endpoint or per-tenant quotas, and billing. ## Related +- [Data handling](data-handling.md) for what happens to submitted values - [Security](../architecture/security.md) for the boundaries that do exist - [Delivery semantics](../architecture/delivery-semantics.md) - [Rate limiting](../guides/rate-limiting.md) diff --git a/mkdocs.yml b/mkdocs.yml index e9a1550..ebec788 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -94,15 +94,19 @@ nav: - Rate Limiting: guides/rate-limiting.md - Endpoint Management: guides/endpoint-management.md - Delivery Replay: guides/delivery-replay.md + - Browsing Submissions: guides/submission-management.md + - Exporting Submissions: guides/exporting-submissions.md - API Reference: - Authentication: api/authentication.md - Endpoints: api/endpoints.md - Submissions: api/submissions.md + - Submission Management: api/submission-management.md - Deliveries: api/deliveries.md - Errors: api/errors.md - Operations: - Worker: operations/worker.md - Database Migrations: operations/migrations.md + - Submission Retention: operations/retention.md - Reverse Proxy: operations/reverse-proxy.md - Configuration Reference: operations/configuration-reference.md - Architecture: @@ -115,4 +119,5 @@ nav: - Testing: development/testing.md - Contributing: development/contributing.md - Reference: + - Data Handling: reference/data-handling.md - Limitations: reference/limitations.md diff --git a/src/hymical_forms/api/deliveries.py b/src/hymical_forms/api/deliveries.py index 7304de9..5e7bb4c 100644 --- a/src/hymical_forms/api/deliveries.py +++ b/src/hymical_forms/api/deliveries.py @@ -133,8 +133,20 @@ class DeliveryView(BaseModel): """ id: str = Field(description="Opaque identifier for this logical delivery.") - submission_id: str = Field(description="The submission this delivery carries.") - endpoint_id: str = Field(description="The endpoint that submission was addressed to.") + submission_id: str | None = Field( + description=( + "The submission this delivery carries, or null once retention has removed " + "it. Only a delivery that has already been delivered can lose its " + "submission: every state a delivery can still be attempted from keeps the " + "payload it would need." + ) + ) + endpoint_id: str = Field( + description=( + "The endpoint the submission was addressed to, recorded on the delivery " + "when it was queued so that it survives the submission being removed." + ) + ) state: DeliveryState = Field(description="Where this delivery has got to.") destination_url: str = Field( description=( @@ -228,8 +240,8 @@ def list_deliveries( raise InvalidCursor() from exc return DeliveryPage( - items=[_view(record) for record in page], - next_cursor=next_cursor([record.delivery.id for record in page], limit=limit), + items=[_view(delivery) for delivery in page], + next_cursor=next_cursor([delivery.id for delivery in page], limit=limit), ) @@ -255,13 +267,13 @@ def get_delivery( :returns: the delivery and its attempts, carrying no signing secret :raises DeliveryNotFound: if no delivery holds that identifier """ - record = storage.get_delivery(session, delivery_id) - if record is None: + delivery = storage.get_delivery(session, delivery_id) + if delivery is None: raise DeliveryNotFound(delivery_id) attempts = storage.list_delivery_attempts(session, delivery_id) return DeliveryDetail( - **_view(record).model_dump(), + **_view(delivery).model_dump(), attempts=[_attempt(attempt) for attempt in attempts], ) @@ -305,7 +317,7 @@ def replay_delivery( # Either the delivery was never failed, or two operators replayed it at # once and this is the loser. Both are answered from the state the # database settled on, so the answer is the same however the race went. - raise DeliveryNotReplayable(delivery_id, outcome.record.delivery.state) + raise DeliveryNotReplayable(delivery_id, outcome.record.state) logger.info( "delivery %s requeued by management key %s (%s)", @@ -316,17 +328,16 @@ def replay_delivery( return _view(outcome.record) -def _view(record: storage.DeliveryRecord) -> DeliveryView: +def _view(delivery: models.WebhookDelivery) -> DeliveryView: """ render a delivery for a management read - :param record: the delivery and the endpoint it belongs to + :param delivery: the persisted delivery :returns: the delivery's operational state, carrying no signing secret """ - delivery = record.delivery return DeliveryView( id=delivery.id, submission_id=delivery.submission_id, - endpoint_id=record.endpoint_id, + endpoint_id=delivery.endpoint_id, state=DeliveryState(delivery.state), destination_url=delivery.destination_url, attempt_count=delivery.attempts, diff --git a/src/hymical_forms/api/submission_management.py b/src/hymical_forms/api/submission_management.py new file mode 100644 index 0000000..873a1f0 --- /dev/null +++ b/src/hymical_forms/api/submission_management.py @@ -0,0 +1,589 @@ +""" +submission retrieval and export: the first supported way to read a form back + +Every route here is behind the management authentication boundary, declared the +same way as the rest of the management API, by asking for +:data:`~hymical_forms.api.security.ManagementKeyDep`. Nothing here is reachable +without a management API key, and the public ingestion route deliberately still +answers with an acknowledgement rather than with anything a form carried. + +These are the only routes in the service that return submitted values. That makes +the boundary worth stating rather than assuming: + +* a listing is metadata only. It reports how many values a submission carried, + never what they were, so paging through a busy endpoint does not spread form + content across pages nobody asked for; +* the detail route and the exports return the values themselves, in the shape + they are stored, because reading one submission back is the whole point of + asking for it; +* nothing here logs a field name or a field value. What is logged about an export + is who asked, for what filter, and how many rows it came to. + +The payload fingerprint and the idempotency key are never returned. The +fingerprint is an internal detail of how a retry is recognised, and the key is a +secret in practice: anyone holding it can resolve it to a submission through the +public ingestion route. +""" + +from __future__ import annotations + +import logging +from collections.abc import Iterator +from datetime import datetime +from http import HTTPStatus +from typing import Annotated + +from fastapi import APIRouter, Query, Request +from pydantic import BaseModel, Field +from sqlalchemy.orm import Session, sessionmaker +from starlette.responses import Response, StreamingResponse + +from hymical_forms import export, models, storage +from hymical_forms.api.pagination import ( + DEFAULT_PAGE_SIZE, + CursorQuery, + InvalidCursor, + LimitQuery, + next_cursor, +) +from hymical_forms.api.security import ManagementKeyDep +from hymical_forms.config import Settings +from hymical_forms.db import SessionDep +from hymical_forms.errors import ApiError, ErrorResponse +from hymical_forms.ingestion import ENDPOINT_ID_MAX_LENGTH +from hymical_forms.models import utcnow +from hymical_forms.webhooks import DeliveryState + +logger = logging.getLogger(__name__) + +router = APIRouter(tags=["submission management"]) + +UNAUTHENTICATED = { + "model": ErrorResponse, + "description": "Missing or invalid management API key", +} + +EndpointFilter = Annotated[ + str | None, + Query( + max_length=ENDPOINT_ID_MAX_LENGTH, + description="Only submissions for this endpoint. Omit for every endpoint.", + ), +] + +ReceivedAfterFilter = Annotated[ + datetime | None, + Query( + description=( + "Only submissions received strictly after this ISO 8601 instant. " + "A submission received exactly on it is excluded." + ), + ), +] + +ReceivedBeforeFilter = Annotated[ + datetime | None, + Query( + description=( + "Only submissions received strictly before this ISO 8601 instant. " + "A submission received exactly on it is excluded." + ), + ), +] + +FormatQuery = Annotated[ + str | None, + Query( + description="The export format, `json` or `csv`. Defaults to `json`.", + ), +] + +JSON_FORMAT = "json" +CSV_FORMAT = "csv" +EXPORT_FORMATS = (JSON_FORMAT, CSV_FORMAT) + + +class SubmissionNotFound(ApiError): + """ + raised when a management route addresses a submission that does not exist + """ + + status_code = HTTPStatus.NOT_FOUND + code = "submission_not_found" + + def __init__(self, submission_id: str) -> None: + """ + name the submission identifier that could not be resolved + :param submission_id: the identifier taken from the request path + """ + # A submission that retention has removed answers exactly the same way as + # one that never existed, which is right: from outside, both mean this + # service does not hold it. + super().__init__( + f"No submission with the ID {submission_id!r} exists.", + details={"submission_id": submission_id}, + ) + + +class InvalidTimeRange(ApiError): + """ + raised when the two time bounds cannot both be satisfied + """ + + status_code = HTTPStatus.UNPROCESSABLE_ENTITY + code = "invalid_time_range" + + def __init__(self) -> None: + """ + state the relationship the two bounds have to be in + """ + # Refused rather than answered with an empty page, because a range that + # cannot match anything is a mistake in the request rather than a fact + # about the data, and answering it with nothing hides the mistake. + super().__init__( + "received_after must be strictly earlier than received_before. Both bounds " + "are exclusive, so a range where they are equal matches nothing.", + details={"fields": ["received_after", "received_before"]}, + ) + + +class UnsupportedExportFormat(ApiError): + """ + raised when an export was asked for in a format this service does not write + """ + + status_code = HTTPStatus.UNPROCESSABLE_ENTITY + code = "unsupported_export_format" + + def __init__(self, requested: str) -> None: + """ + report the rejected format alongside the supported ones + :param requested: the format the caller asked for + """ + super().__init__( + f"Exports are written as {' or '.join(EXPORT_FORMATS)}.", + details={"field": "format", "supported": list(EXPORT_FORMATS)}, + ) + + +class ExportTooLarge(ApiError): + """ + raised when a filter matches more submissions than one export may return + """ + + # A 422 rather than a 413: the request is well formed and it is the filter, + # which is part of the request, that has to change. Refused rather than + # truncated, so an export is either everything that matched or an error, and + # never a quietly incomplete file somebody archives. + status_code = HTTPStatus.UNPROCESSABLE_ENTITY + code = "export_too_large" + + def __init__(self, maximum: int) -> None: + """ + report the ceiling the filter would have exceeded + :param maximum: the most submissions one export may return + """ + super().__init__( + f"This filter matches more than {maximum} submissions, which is the most one " + "export may return. Narrow it with received_after and received_before, or " + "with endpoint_id, and export the range in parts.", + details={"limit": maximum}, + ) + + +class DeliverySummary(BaseModel): + """ + the delivery a submission owes, as seen from the submission + """ + + # Enough to tell an operator whether this submission reached its destination + # and where to look next, and no more. The signing secret is not here, and + # neither is the attempt history: that is what the delivery routes are for. + id: str = Field(description="Opaque identifier of the delivery carrying this submission.") + state: DeliveryState = Field(description="Where that delivery has got to.") + attempt_count: int = Field( + description="How many requests have ever been made for that delivery." + ) + + +class SubmissionSummary(BaseModel): + """ + one stored submission as a listing reports it + """ + + # There is no ``fields`` property on this model at all, rather than one that + # is sometimes populated. A listing that cannot name the submitted values + # cannot leak them, however the route above it changes. + id: str = Field(description="Opaque identifier for this submission.") + endpoint_id: str = Field(description="The endpoint the submission was addressed to.") + received_at: datetime = Field(description="UTC timestamp of when the API accepted the body.") + field_count: int = Field( + description=( + "Number of name/value pairs the submission carried. A field submitted three " + "times counts three times, the same as it does on the ingestion response." + ) + ) + idempotent: bool = Field( + description=( + "True when the submission was sent with an Idempotency-Key header. The key " + "itself is never returned: anyone holding it can resolve it to this " + "submission through the public ingestion route." + ) + ) + delivery: DeliverySummary | None = Field( + description=( + "The webhook delivery this submission owes, or null when its endpoint has " + "no webhook. A submission owes at most one delivery." + ) + ) + + +class SubmissionDetail(SubmissionSummary): + """ + one stored submission together with the values it carried + """ + + fields: dict[str, list[str]] = Field( + description=( + "The submitted fields, in the order they arrived, with every value as a " + "list. A field submitted once is a one-element list rather than a bare " + "string, and a repeated field keeps its values in the order they were sent." + ) + ) + + +class SubmissionExport(BaseModel): + """ + one submission as a JSON export writes it + + Declared for the OpenAPI document rather than used to build a response: the + export is written incrementally by :mod:`hymical_forms.export` so that a + large one does not have to exist in memory before it can be sent. + """ + + id: str = Field(description="Opaque identifier for this submission.") + endpoint_id: str = Field(description="The endpoint the submission was addressed to.") + received_at: datetime = Field(description="UTC timestamp of when the API accepted the body.") + fields: dict[str, list[str]] = Field( + description="The submitted fields, with every value as a list, exactly as stored." + ) + + +class SubmissionExportDocument(BaseModel): + """ + the document a JSON export writes + """ + + submissions: list[SubmissionExport] = Field( + description="Every matching submission, newest first." + ) + + +class SubmissionPage(BaseModel): + """ + one page of stored submissions + """ + + items: list[SubmissionSummary] = Field( + description="The submissions on this page, newest first." + ) + next_cursor: str | None = Field( + description=( + "Pass as `cursor` to read the next page, or null when this is certainly " + "the last one. A full page always carries a cursor, so the final request " + "of a walk returns an empty page." + ) + ) + + +@router.get( + "/submissions", + summary="List stored submissions", + responses={ + 401: UNAUTHENTICATED, + 422: {"model": ErrorResponse, "description": "Invalid filter, page size or cursor"}, + 503: {"model": ErrorResponse, "description": "Database unavailable"}, + }, +) +def list_submissions( + session: SessionDep, + principal: ManagementKeyDep, + endpoint_id: EndpointFilter = None, + received_after: ReceivedAfterFilter = None, + received_before: ReceivedBeforeFilter = None, + limit: LimitQuery = DEFAULT_PAGE_SIZE, + cursor: CursorQuery = None, +) -> SubmissionPage: + """ + read a page of the submissions this service holds + :param session: the session this request does its database work through + :param principal: the management key this request authenticated as + :param endpoint_id: only submissions for this endpoint, or None for every endpoint + :param received_after: only submissions strictly newer than this instant + :param received_before: only submissions strictly older than this instant + :param limit: the most submissions to return + :param cursor: the previous page's cursor, or None to read the first page + :returns: one page of submission summaries, newest first + :raises InvalidTimeRange: if the two time bounds cannot both be satisfied + :raises InvalidCursor: if the cursor does not continue from a known submission + """ + filters = _filters(endpoint_id, received_after, received_before) + try: + page = storage.list_submissions(session, filters=filters, limit=limit, after=cursor) + except storage.UnknownCursor as exc: + raise InvalidCursor() from exc + + return SubmissionPage( + items=[_summary(record) for record in page], + next_cursor=next_cursor([record.submission.id for record in page], limit=limit), + ) + + +# Declared before ``/submissions/{submission_id}``, because routes are matched in +# the order they are added and ``export`` would otherwise be read as an +# identifier and answered with a 404. +@router.get( + "/submissions/export", + summary="Export stored submissions", + response_class=Response, + responses={ + 200: { + "model": SubmissionExportDocument, + "description": ( + "The matching submissions as a downloadable file, in the requested " + "format. Sent with a Content-Disposition attachment filename." + ), + # The CSV form has no schema worth writing beyond its media type: its + # columns depend on which field names the export turns out to hold. + "content": {"text/csv": {"schema": {"type": "string"}}}, + }, + 401: UNAUTHENTICATED, + 422: { + "model": ErrorResponse, + "description": "Invalid filter or format, or more matches than one export may return", + }, + 503: {"model": ErrorResponse, "description": "Database unavailable"}, + }, +) +def export_submissions( + request: Request, + session: SessionDep, + principal: ManagementKeyDep, + endpoint_id: EndpointFilter = None, + received_after: ReceivedAfterFilter = None, + received_before: ReceivedBeforeFilter = None, + format: FormatQuery = None, +) -> Response: + """ + export the submissions a filter matches, as a downloadable file + :param request: the incoming request, read for the configuration and session factory + :param session: the session this request does its database work through + :param principal: the management key this request authenticated as + :param endpoint_id: only submissions for this endpoint, or None for every endpoint + :param received_after: only submissions strictly newer than this instant + :param received_before: only submissions strictly older than this instant + :param format: the format to write, ``json`` or ``csv``, defaulting to ``json`` + :returns: the export, offered as an attachment + :raises UnsupportedExportFormat: if the requested format is not one this service writes + :raises InvalidTimeRange: if the two time bounds cannot both be satisfied + :raises ExportTooLarge: if the filter matches more submissions than one export may return + """ + chosen = (format or JSON_FORMAT).lower() + if chosen not in EXPORT_FORMATS: + raise UnsupportedExportFormat(chosen) + + settings: Settings = request.app.state.settings + maximum = settings.export_max_submissions + filters = _filters(endpoint_id, received_after, received_before) + + # Counted before anything is written, because once a body has started there + # is no way back to an error response. The count is bounded by the maximum + # itself, so an enormous filter costs a walk of that many index entries + # rather than a walk of the table. Past the check it is an exact number, and + # the log below is the one place it is worth having. + matched = storage.count_submissions(session, filters=filters, ceiling=maximum + 1) + if matched > maximum: + raise ExportTooLarge(maximum) + + # Who asked, for what, and how much came out. Not one field name and not one + # field value: an export is the one request whose whole purpose is to move + # form content, and duplicating it into the log would undo that boundary. + logger.info( + "%d submission(s) exported as %s by management key %s (%s), endpoint %s", + matched, + chosen, + principal.key_id, + principal.name, + endpoint_id or "any", + ) + if chosen == CSV_FORMAT: + return _csv_export(session, filters=filters, maximum=maximum) + return _json_export(request, filters=filters, maximum=maximum) + + +@router.get( + "/submissions/{submission_id}", + summary="Inspect one stored submission", + responses={ + 401: UNAUTHENTICATED, + 404: {"model": ErrorResponse, "description": "No such submission"}, + 503: {"model": ErrorResponse, "description": "Database unavailable"}, + }, +) +def get_submission( + submission_id: str, + session: SessionDep, + principal: ManagementKeyDep, +) -> SubmissionDetail: + """ + read one submission and the values it carried + :param submission_id: submission identifier taken from the request path + :param session: the session this request does its database work through + :param principal: the management key this request authenticated as + :returns: the submission, its fields and the delivery it owes if it owes one + :raises SubmissionNotFound: if no submission holds that identifier + """ + record = storage.get_submission(session, submission_id) + if record is None: + raise SubmissionNotFound(submission_id) + + # Nothing about the fields is logged, here or anywhere below. They are + # returned to the caller that authenticated and asked for them, and that is + # the whole of where they go. + return SubmissionDetail( + **_summary(record).model_dump(), + fields={name: list(values) for name, values in record.submission.fields.items()}, + ) + + +def _json_export( + request: Request, *, filters: storage.SubmissionFilter, maximum: int +) -> StreamingResponse: + """ + build a streamed JSON export of the matching submissions + :param request: the incoming request, read for the application's session factory + :param filters: the endpoint and time bounds to export within + :param maximum: the most submissions the export may return + :returns: a streaming response carrying the export as an attachment + """ + # The body is produced after this function returns, by which time the + # session the request was served on may already have been closed. So the + # stream opens one of its own and closes it when the generator finishes, + # which is also what happens if the client disconnects part way through. + factory: sessionmaker[Session] = request.app.state.session_factory + + def body() -> Iterator[str]: + """ + write the document as the rows arrive + :returns: an iterator over the pieces of the JSON document + """ + with factory() as stream_session: + rows = storage.stream_submissions(stream_session, filters=filters, limit=maximum) + yield from export.json_document(_exportable(row) for row in rows) + + filename = export.export_filename(JSON_FORMAT, now=utcnow()) + return StreamingResponse( + body(), + media_type=export.JSON_MEDIA_TYPE, + headers={"Content-Disposition": export.content_disposition(filename)}, + ) + + +def _csv_export(session: Session, *, filters: storage.SubmissionFilter, maximum: int) -> Response: + """ + build a CSV export of the matching submissions + :param session: the session this request does its database work through + :param filters: the endpoint and time bounds to export within + :param maximum: the most submissions the export may return + :returns: a response carrying the export as an attachment + """ + # Not streamed, and the reason is in the format rather than in the plumbing: + # a CSV header is the union of every field name in the export, which is only + # known once the last row has been read. The set is already bounded by the + # export maximum, which is what makes building it in one pass safe. + rows = [ + _exportable(row) + for row in storage.stream_submissions(session, filters=filters, limit=maximum) + ] + filename = export.export_filename(CSV_FORMAT, now=utcnow()) + return Response( + content=export.csv_document(rows), + media_type=export.CSV_MEDIA_TYPE, + headers={"Content-Disposition": export.content_disposition(filename)}, + ) + + +def _exportable(submission: models.Submission) -> export.ExportedSubmission: + """ + narrow a stored submission to the parts an export writes + :param submission: the persisted submission + :returns: the submission's identity and content, with no internal columns + """ + return export.ExportedSubmission( + id=submission.id, + endpoint_id=submission.endpoint_id, + received_at=submission.received_at, + fields=submission.fields, + ) + + +def _filters( + endpoint_id: str | None, + received_after: datetime | None, + received_before: datetime | None, +) -> storage.SubmissionFilter: + """ + gather and check the bounds a submission read asked for + :param endpoint_id: only submissions for this endpoint, or None for every endpoint + :param received_after: only submissions strictly newer than this instant + :param received_before: only submissions strictly older than this instant + :returns: the filter the storage layer applies + :raises InvalidTimeRange: if the two bounds cannot both be satisfied + """ + if ( + received_after is not None + and received_before is not None + and received_after >= received_before + ): + raise InvalidTimeRange() + return storage.SubmissionFilter( + endpoint_id=endpoint_id, + received_after=received_after, + received_before=received_before, + ) + + +def _summary(record: storage.SubmissionRecord) -> SubmissionSummary: + """ + render a submission for a management listing + :param record: the submission and the delivery it owes, if it owes one + :returns: the submission's metadata, carrying none of its submitted values + """ + # ``fields`` is read here only to be counted. The stored mapping came along + # with the row because a page is one query, and this is the one place the + # boundary between metadata and content is a decision in code rather than a + # column that was never selected. The model above has nowhere to put them. + submission = record.submission + return SubmissionSummary( + id=submission.id, + endpoint_id=submission.endpoint_id, + received_at=submission.received_at, + field_count=sum(len(values) for values in submission.fields.values()), + idempotent=submission.idempotency_key is not None, + delivery=_delivery(record.delivery), + ) + + +def _delivery(delivery: models.WebhookDelivery | None) -> DeliverySummary | None: + """ + render the delivery a submission owes, if it owes one + :param delivery: the persisted delivery, or None when the endpoint has no webhook + :returns: the delivery's state, or None when there is no delivery + """ + if delivery is None: + return None + return DeliverySummary( + id=delivery.id, + state=DeliveryState(delivery.state), + attempt_count=delivery.attempts, + ) diff --git a/src/hymical_forms/app.py b/src/hymical_forms/app.py index edbad6f..02ecd23 100644 --- a/src/hymical_forms/app.py +++ b/src/hymical_forms/app.py @@ -10,7 +10,7 @@ from fastapi import FastAPI from hymical_forms import __version__ -from hymical_forms.api import deliveries, endpoints, health, submissions +from hymical_forms.api import deliveries, endpoints, health, submission_management, submissions from hymical_forms.config import Settings from hymical_forms.db import create_engine_from_url, create_session_factory from hymical_forms.errors import register_exception_handlers @@ -78,6 +78,7 @@ def create_app(settings: Settings | None = None) -> FastAPI: app.include_router(health.router) app.include_router(endpoints.router) app.include_router(deliveries.router) + app.include_router(submission_management.router) app.include_router(submissions.router) return app diff --git a/src/hymical_forms/cli.py b/src/hymical_forms/cli.py index 9508fde..a3f512a 100644 --- a/src/hymical_forms/cli.py +++ b/src/hymical_forms/cli.py @@ -1,11 +1,13 @@ """ -the operator command line for management API keys +the operator command line Run it as its own process, against the database ``FORMS_DATABASE_URL`` names:: python -m hymical_forms.cli create-key --name local-admin python -m hymical_forms.cli list-keys python -m hymical_forms.cli revoke-key mk_1f0c9a... + python -m hymical_forms.cli cleanup-submissions --dry-run + python -m hymical_forms.cli cleanup-submissions Keys are minted here rather than over HTTP, and that is the whole answer to how the first one comes into being. A route that issued a management credential @@ -13,6 +15,12 @@ access, which is the thing this boundary exists to remove. Nothing is generated at startup and no key is shipped in this repository, so a deployment has exactly the credentials an operator deliberately created. + +Retention cleanup is here for a related reason: it deletes stored form data, and +that should be something a person runs deliberately against a database they +named, not something an API process does on the side of serving a request. +Scheduling it is left to cron, a systemd timer, or whatever already runs the +rest of your periodic work; this service ships no daemon for it. """ from __future__ import annotations @@ -28,10 +36,11 @@ from sqlalchemy.exc import ArgumentError, SQLAlchemyError from sqlalchemy.orm import Session -from hymical_forms import apikeys, storage +from hymical_forms import apikeys, retention, storage from hymical_forms.config import Settings from hymical_forms.db import create_engine_from_url, create_session_factory from hymical_forms.models import ManagementApiKey, utcnow +from hymical_forms.retention import RetentionPolicy from hymical_forms.schema import SchemaNotReady, verify_schema PROGRAM = "python -m hymical_forms.cli" @@ -85,7 +94,10 @@ def build_parser() -> argparse.ArgumentParser: """ parser = argparse.ArgumentParser( prog=PROGRAM, - description="Create, list and revoke Hymical Forms management API keys.", + description=( + "Administer a Hymical Forms deployment: manage its API keys, and delete " + "stored submissions that have outlived the configured retention." + ), ) subcommands = parser.add_subparsers(dest="command", required=True) @@ -110,6 +122,42 @@ def build_parser() -> argparse.ArgumentParser: ) revoke.add_argument("key_id", help="The key ID, as shown by list-keys.") + cleanup = subcommands.add_parser( + "cleanup-submissions", + help="Delete stored submissions older than the configured retention.", + description=( + "Delete stored submissions that have outlived FORMS_SUBMISSION_RETENTION_DAYS. " + "A submission is only removed once nothing still needs it: one whose webhook " + "delivery is pending, processing, or failed and therefore replayable is kept, " + "however old it is. Delivery records and their attempt history are never " + "deleted; they are unlinked from the submission and left in place." + ), + ) + cleanup.add_argument( + "--dry-run", + action="store_true", + help="Report what would be deleted and change nothing.", + ) + cleanup.add_argument( + "--older-than-days", + type=int, + metavar="DAYS", + help=( + "Delete submissions older than this many days, instead of the configured " + "retention. Required when no retention is configured." + ), + ) + cleanup.add_argument( + "--batch-size", + type=int, + default=retention.DEFAULT_BATCH_SIZE, + metavar="ROWS", + help=( + "How many submissions to delete per transaction. " + f"Defaults to {retention.DEFAULT_BATCH_SIZE}." + ), + ) + return parser @@ -129,16 +177,19 @@ def _run(arguments: argparse.Namespace, settings: Settings, *, out: TextIO) -> i # understand. verify_schema(engine) with create_session_factory(engine)() as session: - return _dispatch(arguments, session, out=out) + return _dispatch(arguments, session, settings, out=out) finally: engine.dispose() -def _dispatch(arguments: argparse.Namespace, session: Session, *, out: TextIO) -> int: +def _dispatch( + arguments: argparse.Namespace, session: Session, settings: Settings, *, out: TextIO +) -> int: """ run the chosen command against an open session :param arguments: the parsed command line :param session: the session to do the work through + :param settings: the configuration this invocation was built from :param out: the stream ordinary output is written to :returns: the process exit code """ @@ -146,6 +197,8 @@ def _dispatch(arguments: argparse.Namespace, session: Session, *, out: TextIO) - return _create_key(session, name=arguments.name, out=out) if arguments.command == "list-keys": return _list_keys(session, out=out) + if arguments.command == "cleanup-submissions": + return _cleanup_submissions(session, settings, arguments, out=out) return _revoke_key(session, key_id=arguments.key_id, out=out) @@ -246,6 +299,97 @@ def _revoke_key(session: Session, *, key_id: str, out: TextIO) -> int: return EXIT_OK +def _cleanup_submissions( + session: Session, + settings: Settings, + arguments: argparse.Namespace, + *, + out: TextIO, +) -> int: + """ + delete stored submissions that have outlived their retention + :param session: the session to write through + :param settings: the configuration naming the retention age + :param arguments: the parsed command line, read for the run's options + :param out: the stream ordinary output is written to + :returns: the process exit code + """ + policy = _cleanup_policy(settings, arguments.older_than_days) + if policy is None: + # Refused rather than treated as "delete nothing", so an operator who + # expected a sweep to happen finds out that it did not. + print( + "No submission retention is configured, so nothing is eligible for deletion. " + "Set FORMS_SUBMISSION_RETENTION_DAYS, or pass --older-than-days to sweep " + "this once without configuring anything.", + file=sys.stderr, + ) + return EXIT_FAILED + if arguments.batch_size < 1: + print("--batch-size must be at least 1.", file=sys.stderr) + return EXIT_FAILED + + cutoff = policy.cutoff(utcnow()) + eligible = storage.count_expired_submissions(session, before=cutoff) + + print( + f"Retention keeps submissions for {policy.days} days, so the cutoff is {_moment(cutoff)}.", + file=out, + ) + print( + f"{eligible} submission(s) received before then are eligible for deletion. " + "A submission whose delivery is still pending, processing or replayable is " + "not eligible, however old it is.", + file=out, + ) + + if arguments.dry_run: + # Nothing above this line wrote anything, and nothing below it runs. The + # counting query is the whole of what a dry run does. + print("Dry run: nothing was deleted.", file=out) + return EXIT_OK + if eligible == 0: + print("Nothing to delete.", file=out) + return EXIT_OK + + removed = storage.delete_expired_submissions( + session, + before=cutoff, + batch_size=arguments.batch_size, + max_batches=retention.MAX_BATCHES, + ) + print( + f"Deleted {removed} submission(s) in batches of up to {arguments.batch_size}. " + "Delivery records and their attempt history were left in place.", + file=out, + ) + if removed < eligible: + # Either this run hit its batch ceiling, or a delivery finished between + # the count and the sweep and took its submission out of scope. Both are + # answered the same way: run it again. + print( + "Fewer were deleted than were counted. Run the command again to continue.", + file=out, + ) + return EXIT_OK + + +def _cleanup_policy(settings: Settings, older_than_days: int | None) -> RetentionPolicy | None: + """ + work out which retention age this run should sweep against + :param settings: the configuration naming the retention age + :param older_than_days: an age given on the command line, or None if there was none + :returns: the policy to sweep with, or None if nothing is eligible under either + """ + # An explicit age on the command line wins, which is what lets an operator + # who keeps submissions indefinitely still clear out one old range by hand. + if older_than_days is not None: + chosen = RetentionPolicy(days=older_than_days) + else: + chosen = settings.retention_policy() + return chosen if chosen.enabled else None + + def _revocation_moment(session: Session, key_id: str) -> datetime | None: """ read when a key was revoked, before this command possibly revokes it diff --git a/src/hymical_forms/config.py b/src/hymical_forms/config.py index 5371616..7fdbb45 100644 --- a/src/hymical_forms/config.py +++ b/src/hymical_forms/config.py @@ -12,6 +12,7 @@ from pydantic_settings import BaseSettings, SettingsConfigDict from hymical_forms.ratelimit import RateLimit +from hymical_forms.retention import RetentionPolicy from hymical_forms.webhooks import RetryPolicy @@ -157,6 +158,26 @@ class Settings(BaseSettings): ), ) + submission_retention_days: int = Field( + default=0, + ge=0, + description=( + "How many days a stored submission is kept before an operator's cleanup " + "command may delete it. 0, the default, keeps submissions indefinitely. " + "Nothing is ever deleted automatically: the sweep is a command an operator " + "runs deliberately." + ), + ) + export_max_submissions: int = Field( + default=10_000, + ge=1, + description=( + "Largest number of submissions one export may return. A filter matching " + "more than this is refused rather than silently truncated, so an export is " + "either complete or an error." + ), + ) + def retry_policy(self) -> RetryPolicy: """ gather the retry settings into the value the delivery code works with @@ -187,3 +208,10 @@ def endpoint_rate_limit(self) -> RateLimit: requests=self.rate_limit_endpoint_requests, window_seconds=self.rate_limit_endpoint_window_seconds, ) + + def retention_policy(self) -> RetentionPolicy: + """ + gather the retention setting into the value the cleanup command works with + :returns: the configured retention policy + """ + return RetentionPolicy(days=self.submission_retention_days) diff --git a/src/hymical_forms/export.py b/src/hymical_forms/export.py new file mode 100644 index 0000000..96f900b --- /dev/null +++ b/src/hymical_forms/export.py @@ -0,0 +1,202 @@ +""" +how an exported submission is written, in JSON and in CSV + +Nothing here touches the database, HTTP or the clock. Rendering is kept apart +from fetching so that the awkward parts, which are all in the CSV, can be +reasoned about and tested on their own. + +The JSON export is written incrementally, one submission at a time, so that a +response can start before the last row has been read. + +The CSV is not. A CSV needs a header, the header is the union of every field name +in the export, and that union is only known once the last row has been read. So +the CSV is built in one pass over a set already bounded by the export maximum +rather than streamed, and that bound is what keeps it honest. + +Field values keep the shape they are stored in. A cell holds a JSON array, so a +field submitted three times is one cell with three ordered values in it and a +field submitted once is a one-element array rather than a bare string. Nothing +has to be escaped by hand: :mod:`csv` quotes and escapes the cell, and JSON +quotes and escapes the values inside it, so a comma, a quote or a newline in +somebody's answer survives both layers intact. +""" + +from __future__ import annotations + +import csv +import io +import json +from collections.abc import Iterable, Iterator, Mapping +from dataclasses import dataclass +from datetime import UTC, datetime + +JSON_MEDIA_TYPE = "application/json" +CSV_MEDIA_TYPE = "text/csv; charset=utf-8" + +# The columns every export starts with, in this order, before the form's own +# field names. They are the submission's identity rather than its content. +METADATA_COLUMNS = ("submission_id", "endpoint_id", "received_at") + +FILENAME_STEM = "hymical-submissions" + +# A leading one of these makes a spreadsheet treat a cell as a formula rather +# than as text. The set is the usual one, plus the two whitespace characters some +# spreadsheets skip over before deciding. +_FORMULA_LEADERS = ("=", "+", "-", "@", "\t", "\r") + +# What a value that would be read as a formula is prefixed with. An apostrophe is +# what a spreadsheet itself writes to mean "this is text", so the cell reads as +# the original characters rather than being evaluated. +_TEXT_MARKER = "'" + + +@dataclass(frozen=True, slots=True) +class ExportedSubmission: + """ + one submission in the shape an export writes it + """ + + # Deliberately not the persisted row. Nothing internal can reach an export by + # accident, because the payload fingerprint and the idempotency key have + # nowhere to be: this type has no field for either. + id: str + endpoint_id: str + received_at: datetime + fields: Mapping[str, list[str]] + + +def export_filename(suffix: str, *, now: datetime) -> str: + """ + build the filename an export is offered under + :param suffix: the file extension, without a dot + :param now: the instant the export was requested + :returns: a filename such as ``hymical-submissions-2026-08-25.csv`` + """ + # Every part of this is generated here. No endpoint identifier, no filter + # value and nothing else a caller supplied reaches the filename, so there is + # no user input in it to sanitise and no header for one to break out of. + return f"{FILENAME_STEM}-{now.astimezone(UTC).date().isoformat()}.{suffix}" + + +def content_disposition(filename: str) -> str: + """ + build the header that offers an export as a download + :param filename: the generated filename to offer it under + :returns: a Content-Disposition header value + """ + return f'attachment; filename="{filename}"' + + +def json_document(submissions: Iterable[ExportedSubmission]) -> Iterator[str]: + """ + render an export as JSON, one submission at a time + :param submissions: the submissions to write, in export order + :returns: an iterator over the pieces of the document, in order + """ + # One top-level object with one key, rather than a bare array, so that the + # document has somewhere to grow a summary later without becoming a different + # shape. Written by hand at this level and by ``json.dumps`` at every level + # below it, which is where the escaping that matters happens. + yield '{"submissions":[' + separator = "" + for submission in submissions: + yield separator + json.dumps(_as_object(submission), separators=(",", ":")) + separator = "," + yield "]}" + + +def csv_document(submissions: list[ExportedSubmission]) -> str: + """ + render an export as CSV, including a column for every field name in it + :param submissions: the submissions to write, in export order + :returns: the whole CSV document + """ + # The header is the union of the field names across the export, in the order + # they are first met, so a CSV of one endpoint's submissions comes out in the + # order that endpoint's form asks its questions. Sorting alphabetically would + # be just as deterministic and would read worse. + names = _field_names(submissions) + + buffer = io.StringIO() + # ``\r\n`` line endings, which is what RFC 4180 asks for and what every + # spreadsheet expects. Stated rather than left to the default so that a value + # containing a newline is unambiguously inside its quoted cell. + writer = csv.writer(buffer, lineterminator="\r\n") + writer.writerow([_safe(column) for column in (*METADATA_COLUMNS, *names)]) + for submission in submissions: + writer.writerow(_row(submission, names)) + return buffer.getvalue() + + +def _row(submission: ExportedSubmission, names: list[str]) -> list[str]: + """ + render one submission as its CSV cells + :param submission: the submission to render + :param names: the field names the export has a column for, in column order + :returns: the cells of one row, in column order + """ + # A field the submission does not carry gets an empty cell, which is distinct + # from a field it carries with an empty value: that is ``[""]``. + cells = [submission.id, submission.endpoint_id, _timestamp(submission.received_at)] + for name in names: + values = submission.fields.get(name) + cells.append( + "" if values is None else json.dumps(values, separators=(",", ":"), ensure_ascii=False) + ) + return [_safe(cell) for cell in cells] + + +def _field_names(submissions: Iterable[ExportedSubmission]) -> list[str]: + """ + collect the field names an export needs a column for + :param submissions: the submissions being exported + :returns: every field name met, in the order it was first met + """ + names: dict[str, None] = {} + for submission in submissions: + for name in submission.fields: + names[name] = None + return list(names) + + +def _as_object(submission: ExportedSubmission) -> dict[str, object]: + """ + render one submission as the object a JSON export carries + :param submission: the submission to render + :returns: the object to serialize + """ + return { + "id": submission.id, + "endpoint_id": submission.endpoint_id, + "received_at": _timestamp(submission.received_at), + # Repeated values stay a list, and a single value stays a one-element + # list, exactly as stored and exactly as a webhook payload carries them. + "fields": {name: list(values) for name, values in submission.fields.items()}, + } + + +def _safe(cell: str) -> str: + """ + keep a spreadsheet from reading an exported value as a formula + :param cell: the cell contents as they would otherwise be written + :returns: the cell, prefixed with a text marker if it would be evaluated + """ + # Only the export representation is changed, and only by prefixing. The + # stored value is untouched, and nothing is dropped or rewritten, so the + # original characters are still there to be read. + # + # In practice a field value cell never needs this, because every one of them + # is a JSON array and so begins with a bracket. A field *name* is another + # matter: a form is free to call a field ``=cmd()``, and that name becomes a + # header cell. Applying the rule to every cell means the property holds + # whatever a later change does to the encoding. + return _TEXT_MARKER + cell if cell.startswith(_FORMULA_LEADERS) else cell + + +def _timestamp(moment: datetime) -> str: + """ + render a timestamp the way the rest of this service renders timestamps + :param moment: the instant to render + :returns: an RFC 3339 timestamp in UTC, ending in Z + """ + return moment.astimezone(UTC).isoformat().replace("+00:00", "Z") diff --git a/src/hymical_forms/migrations/versions/0005_20260825_submission_retention.py b/src/hymical_forms/migrations/versions/0005_20260825_submission_retention.py new file mode 100644 index 0000000..5080b6b --- /dev/null +++ b/src/hymical_forms/migrations/versions/0005_20260825_submission_retention.py @@ -0,0 +1,201 @@ +""" +submission retention and self-contained delivery records + +Retention has to be able to remove a stored submission without taking the record +of what this service did about it. Until now it could not: a delivery and every +attempt made for it pointed at the submission with a NOT NULL foreign key, so +deleting a submission either failed outright or, with a cascade, would have taken +the operational history with it. + +This revision makes a delivery record stand on its own. + +``webhook_deliveries.endpoint_id`` is new. The delivery listing used to reach the +endpoint through the submission, which stops working the moment a submission can +be absent. It is snapshotted for the same reason the destination and the signing +secret already are: it describes what the delivery was for, not what the endpoint +happens to be configured as now. Existing rows are backfilled from the submission +they carry, which is exactly the value the join produced. + +``webhook_deliveries.submission_id`` and ``delivery_attempts.submission_id`` +become nullable, with ``ON DELETE SET NULL`` rather than ``ON DELETE CASCADE``. +That is the whole point: removing a submission unlinks the history from it and +leaves the history itself alone. Only a delivery that has already been delivered +can ever lose its submission, because every state a delivery can still be +attempted from protects the payload it would need. + +The submission indexes change to match what submission management reads. The +plain endpoint index is replaced by one that also carries the received timestamp +and the identifier, because a listing is always ordered newest first and the +leftmost column still answers a lookup by endpoint on its own. A second index +over the timestamp and the identifier serves the unfiltered listing and the range +the retention sweep deletes over. + +The downgrade removes delivery records whose submission has already been retained +away. The older schema cannot represent a delivery without a submission, so there +is nowhere for those rows to go. Nothing else is touched. + +Timestamps are written as ``sa.DateTime(timezone=True)`` rather than the +application's ``UtcDateTime`` decorator, for the reason given in ``0001``: a +migration is a frozen record of a change, not a view of the current models. + +This revision alters columns and foreign keys directly rather than through +Alembic's batch mode. Batch mode exists to give SQLite a way to change a column, +by copying the table, and that cannot work here: rebuilding a table other tables +point at fails while SQLite is enforcing foreign keys, which this service turns +on. SQLite is not a migration target. It backs the test suite, whose schema is +built from the models rather than migrated, and it is not a production database +for this service. PostgreSQL performs every operation below in place. + +revision: 0005 +revises: 0004 +created: 2026-08-25 +""" + +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "0005" +down_revision: str | None = "0004" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + + +def upgrade() -> None: + """ + let a delivery record outlive the submission it carried + """ + # Added nullable so a populated table can take the column at all, then + # backfilled from the submission, then tightened. Every existing delivery has + # a submission, because that is what the current schema requires, so the + # backfill can never leave a row without an endpoint. + op.add_column( + "webhook_deliveries", sa.Column("endpoint_id", sa.String(length=64), nullable=True) + ) + op.execute( + "UPDATE webhook_deliveries SET endpoint_id = (" + "SELECT submissions.endpoint_id FROM submissions " + "WHERE submissions.id = webhook_deliveries.submission_id)" + ) + op.alter_column( + "webhook_deliveries", "endpoint_id", existing_type=sa.String(length=64), nullable=False + ) + op.create_foreign_key( + "fk_webhook_deliveries_endpoint_id_endpoints", + "webhook_deliveries", + "endpoints", + ["endpoint_id"], + ["id"], + ) + + # The delete rule is the substance of this revision. SET NULL unlinks the + # history from the submission; CASCADE would delete it along with the + # submission, which is the outcome this exists to prevent. + op.alter_column( + "webhook_deliveries", "submission_id", existing_type=sa.String(length=36), nullable=True + ) + op.drop_constraint( + "fk_webhook_deliveries_submission_id_submissions", + "webhook_deliveries", + type_="foreignkey", + ) + op.create_foreign_key( + "fk_webhook_deliveries_submission_id_submissions", + "webhook_deliveries", + "submissions", + ["submission_id"], + ["id"], + ondelete="SET NULL", + ) + + op.alter_column( + "delivery_attempts", "submission_id", existing_type=sa.String(length=36), nullable=True + ) + op.drop_constraint( + "fk_delivery_attempts_submission_id_submissions", "delivery_attempts", type_="foreignkey" + ) + op.create_foreign_key( + "fk_delivery_attempts_submission_id_submissions", + "delivery_attempts", + "submissions", + ["submission_id"], + ["id"], + ondelete="SET NULL", + ) + + op.create_index( + "ix_webhook_deliveries_endpoint_id_created_at_id", + "webhook_deliveries", + ["endpoint_id", "created_at", "id"], + unique=False, + ) + + op.drop_index(op.f("ix_submissions_endpoint_id"), table_name="submissions") + op.create_index( + "ix_submissions_received_at_id", "submissions", ["received_at", "id"], unique=False + ) + op.create_index( + "ix_submissions_endpoint_id_received_at_id", + "submissions", + ["endpoint_id", "received_at", "id"], + unique=False, + ) + + +def downgrade() -> None: + """ + return to a schema where a delivery cannot exist without its submission + """ + op.drop_index("ix_submissions_endpoint_id_received_at_id", table_name="submissions") + op.drop_index("ix_submissions_received_at_id", table_name="submissions") + op.create_index( + op.f("ix_submissions_endpoint_id"), "submissions", ["endpoint_id"], unique=False + ) + op.drop_index( + "ix_webhook_deliveries_endpoint_id_created_at_id", table_name="webhook_deliveries" + ) + + # A delivery whose submission has been retained away cannot be expressed in + # the schema this returns to, so it is removed along with its attempts. This + # is the one thing the downgrade destroys, and it destroys nothing an + # operator still had the submitted content for. Attempts go first, because + # they point at the deliveries. + op.execute("DELETE FROM delivery_attempts WHERE submission_id IS NULL") + op.execute("DELETE FROM webhook_deliveries WHERE submission_id IS NULL") + + op.drop_constraint( + "fk_delivery_attempts_submission_id_submissions", "delivery_attempts", type_="foreignkey" + ) + op.create_foreign_key( + "fk_delivery_attempts_submission_id_submissions", + "delivery_attempts", + "submissions", + ["submission_id"], + ["id"], + ) + op.alter_column( + "delivery_attempts", "submission_id", existing_type=sa.String(length=36), nullable=False + ) + + op.drop_constraint( + "fk_webhook_deliveries_submission_id_submissions", + "webhook_deliveries", + type_="foreignkey", + ) + op.create_foreign_key( + "fk_webhook_deliveries_submission_id_submissions", + "webhook_deliveries", + "submissions", + ["submission_id"], + ["id"], + ) + op.alter_column( + "webhook_deliveries", "submission_id", existing_type=sa.String(length=36), nullable=False + ) + op.drop_constraint( + "fk_webhook_deliveries_endpoint_id_endpoints", "webhook_deliveries", type_="foreignkey" + ) + op.drop_column("webhook_deliveries", "endpoint_id") diff --git a/src/hymical_forms/models.py b/src/hymical_forms/models.py index e3539af..5a07947 100644 --- a/src/hymical_forms/models.py +++ b/src/hymical_forms/models.py @@ -13,6 +13,7 @@ CheckConstraint, DateTime, ForeignKey, + Index, MetaData, String, TypeDecorator, @@ -177,13 +178,22 @@ class Submission(Base): "(idempotency_key IS NULL) = (payload_fingerprint IS NULL)", name="idempotency_identity", ), + # The order every submission management read walks in, newest first, and + # the range the retention sweep deletes over. Both columns are in it + # because the page boundary is a row-value comparison over the timestamp + # and the identifier together, so an index stopping at the timestamp + # would still leave the ties to be sorted. + Index("ix_submissions_received_at_id", "received_at", "id"), + # The same walk narrowed to one endpoint. Its leftmost column answers a + # lookup by endpoint on its own just as well, so this replaces the plain + # endpoint index rather than standing beside it. + Index("ix_submissions_endpoint_id_received_at_id", "endpoint_id", "received_at", "id"), ) id: Mapped[str] = mapped_column(String(SUBMISSION_ID_MAX_LENGTH), primary_key=True) endpoint_id: Mapped[str] = mapped_column( String(ENDPOINT_ID_MAX_LENGTH), ForeignKey("endpoints.id"), - index=True, ) received_at: Mapped[datetime] = mapped_column(UtcDateTime) @@ -264,11 +274,29 @@ class WebhookDelivery(Base): "(state IN ('delivered', 'failed')) = (completed_at IS NOT NULL)", name="completion", ), + # The order and the filter the delivery listing reads in. It used to + # reach the endpoint through the submission, so this index is what + # replaces the join now that the column is here. + Index("ix_webhook_deliveries_endpoint_id_created_at_id", "endpoint_id", "created_at", "id"), ) id: Mapped[str] = mapped_column(String(WEBHOOK_DELIVERY_ID_MAX_LENGTH), primary_key=True) - submission_id: Mapped[str] = mapped_column( - String(SUBMISSION_ID_MAX_LENGTH), ForeignKey("submissions.id") + + # Null once retention has removed the submission this delivery carried, which + # it only ever does for a delivery that has already been delivered. Every + # state a delivery can still be attempted from protects its submission, so a + # delivery a worker can claim always still has the payload it needs. + submission_id: Mapped[str | None] = mapped_column( + String(SUBMISSION_ID_MAX_LENGTH), + ForeignKey("submissions.id", ondelete="SET NULL"), + ) + + # Recorded on the delivery rather than reached through the submission, so an + # operational record still says which endpoint it belongs to after the + # submission it carried has been retained away. It is snapshotted for the + # same reason the destination is: it describes what this delivery was for. + endpoint_id: Mapped[str] = mapped_column( + String(ENDPOINT_ID_MAX_LENGTH), ForeignKey("endpoints.id") ) # The destination and secret are snapshotted, not read through to the @@ -323,9 +351,13 @@ class DeliveryAttempt(Base): ForeignKey("webhook_deliveries.id"), index=True, ) - submission_id: Mapped[str] = mapped_column( + # Null once retention has removed the submission, for the same reason and + # under the same rule as on the delivery. The attempt itself is never + # removed: what a delivery did is operational history, and it stays readable + # through its delivery whether or not the submitted content still exists. + submission_id: Mapped[str | None] = mapped_column( String(SUBMISSION_ID_MAX_LENGTH), - ForeignKey("submissions.id"), + ForeignKey("submissions.id", ondelete="SET NULL"), index=True, ) attempt_number: Mapped[int] = mapped_column() diff --git a/src/hymical_forms/retention.py b/src/hymical_forms/retention.py new file mode 100644 index 0000000..c339cba --- /dev/null +++ b/src/hymical_forms/retention.py @@ -0,0 +1,100 @@ +""" +what it means for a stored submission to have outlived its usefulness + +This module holds the retention rule and nothing else: no queries, no HTTP, no +process. It exists so that the one decision worth arguing about is written down +in one place rather than inferred from a WHERE clause. + +The rule +-------- + +A submission may be deleted when it is older than the cutoff **and** the service +will never need its content again. The second half is not a matter of taste. A +queued webhook delivery does not carry a copy of the submitted fields: the worker +loads the submission and builds the payload from it at the moment it sends. So +the payload is needed for as long as any further attempt is possible, which is: + +* ``pending``, waiting for its due time; +* ``processing``, claimed by a worker right now; +* ``failed``, because a failed delivery is replayable by an operator and a replay + puts it back into exactly the state above. + +That leaves two cases where nothing will ever read the fields again: a submission +with no delivery at all, because its endpoint has no webhook, and a submission +whose delivery has been ``delivered``. Those, and only those, are eligible. + +Delivery history is not part of the bargain. Removing a submission unlinks the +delivery and its attempts from it and leaves both rows exactly where they are: +what this service tried to do, when, and how it went is operational history worth +more than the form content it was carrying. That is what the ``ON DELETE SET +NULL`` in revision ``0005`` buys, and why the delivery carries its own endpoint. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime, timedelta + +from hymical_forms.webhooks import DeliveryState + +# The delivery states that keep a submission alive however old it is. Written as +# the states that protect rather than the states that release, so a state added +# later is protective until somebody deliberately decides otherwise. +PROTECTED_DELIVERY_STATES = ( + DeliveryState.PENDING, + DeliveryState.PROCESSING, + DeliveryState.FAILED, +) + +# How many submissions one delete statement takes. Small enough that a sweep of a +# large backlog is many short transactions rather than one long one holding locks +# across the whole table, and large enough that the round trips do not dominate. +DEFAULT_BATCH_SIZE = 500 + +# A ceiling on how many batches one run will do, so an operator who starts a +# sweep against an enormous backlog gets it back rather than watching it run +# unbounded. What is left over is removed by running the command again. +MAX_BATCHES = 10_000 + + +class RetentionDisabled(Exception): + """ + raised when a cutoff was asked for and no retention age is configured + """ + + def __init__(self) -> None: + """ + say that nothing is eligible because no age has been set + """ + super().__init__("no submission retention age is configured") + + +@dataclass(frozen=True, slots=True) +class RetentionPolicy: + """ + how long submissions are kept before they become eligible for deletion + """ + + # Zero means keep indefinitely, which is the default and the only safe thing + # for an unset value to mean: a service that started deleting form data + # because nobody configured it would be indefensible. + days: int + + @property + def enabled(self) -> bool: + """ + report whether this policy makes anything eligible + :returns: True if a positive retention age is configured + """ + return self.days > 0 + + def cutoff(self, now: datetime) -> datetime: + """ + work out the instant a submission has to predate to be eligible + :param now: the instant the sweep is being run at + :returns: the cutoff in UTC, exclusive, so a submission exactly on it is kept + :raises RetentionDisabled: if no retention age is configured + """ + if not self.enabled: + raise RetentionDisabled() + return now - timedelta(days=self.days) diff --git a/src/hymical_forms/storage.py b/src/hymical_forms/storage.py index 1f914e3..4905349 100644 --- a/src/hymical_forms/storage.py +++ b/src/hymical_forms/storage.py @@ -12,27 +12,41 @@ and :func:`requeue_failed_delivery`, because each is the whole of what its management request came to do; the two management key writes, :func:`revoke_management_key` and :func:`record_management_key_use`, for the -same reason; and the two rate limit operations, :func:`consume_rate_limit` and +same reason; the two rate limit operations, :func:`consume_rate_limit` and :func:`delete_expired_rate_limit_counters`, because abuse accounting has to -outlive the request it was accounting for. +outlive the request it was accounting for; and +:func:`delete_expired_submissions`, because a retention sweep is deliberately +many small committed batches rather than one long transaction. """ from __future__ import annotations +from collections.abc import Iterator from dataclasses import dataclass from datetime import datetime, timedelta from typing import Any, TypeVar, cast -from sqlalchemy import ColumnElement, Select, and_, delete, or_, select, tuple_, update +from sqlalchemy import ( + ColumnElement, + Select, + and_, + delete, + func, + or_, + select, + tuple_, + update, +) from sqlalchemy.dialects.postgresql import insert as postgresql_insert from sqlalchemy.dialects.sqlite import insert as sqlite_insert from sqlalchemy.engine import CursorResult from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm import Session +from sqlalchemy.orm import Mapped, Session from hymical_forms import models from hymical_forms.ingestion import Submission from hymical_forms.ratelimit import Limiter +from hymical_forms.retention import PROTECTED_DELIVERY_STATES from hymical_forms.webhooks import ( DeliveryOutcome, DeliveryResult, @@ -44,10 +58,20 @@ new_webhook_delivery_id, ) -# The two tables management routes page through. Both are keyed by an opaque -# identifier and carry a creation timestamp, which is all cursor pagination here -# asks of a table. -_Paged = TypeVar("_Paged", models.Endpoint, models.WebhookDelivery) +# The three tables management routes page through. Each is keyed by an opaque +# identifier and carries a timestamp it is ordered by, which is all cursor +# pagination here asks of a table. Which timestamp differs, so callers name it. +_Paged = TypeVar("_Paged", models.Endpoint, models.WebhookDelivery, models.Submission) + +# Any submission select, whatever it happens to be selecting. The submission +# filters are applied to a listing, an export and a bounded count alike, and +# narrowing a select never changes what it returns. +_Statement = TypeVar("_Statement", bound=Select[Any]) + +# How many rows a streamed export pulls from the server at a time. Bounded so +# that an export builds its response as it goes rather than materialising every +# matching row before the first byte is written. +EXPORT_FETCH_SIZE = 200 class UnknownCursor(Exception): @@ -140,7 +164,9 @@ def list_endpoints( endpoint = models.Endpoint statement = select(endpoint).order_by(endpoint.created_at.desc(), endpoint.id.desc()) if after is not None: - statement = statement.where(_page_after(session, endpoint, after)) + statement = statement.where( + _page_after(session, endpoint, after, ordered_by=endpoint.created_at) + ) return list(session.scalars(statement.limit(limit))) @@ -335,6 +361,7 @@ def _add_submission( models.WebhookDelivery( id=new_webhook_delivery_id(), submission_id=submission.id, + endpoint_id=submission.endpoint_id, destination_url=webhook.url, signing_secret=webhook.secret, state=DeliveryState.PENDING, @@ -347,6 +374,224 @@ def _add_submission( ) +@dataclass(frozen=True, slots=True) +class SubmissionFilter: + """ + the bounded set of stored submissions a management read is asking for + """ + + # Both bounds are exclusive, which is the contract the API documents. Kept + # together in one value so that the listing, the export and the count that + # guards the export cannot end up asking three slightly different questions. + endpoint_id: str | None = None + received_after: datetime | None = None + received_before: datetime | None = None + + +@dataclass(frozen=True, slots=True) +class SubmissionRecord: + """ + a stored submission together with the delivery it owes, if it owes one + """ + + # There is at most one delivery per submission, enforced by a unique + # constraint, so this is a value rather than a list. It is None for a + # submission whose endpoint has no webhook. + submission: models.Submission + delivery: models.WebhookDelivery | None + + +def list_submissions( + session: Session, + *, + filters: SubmissionFilter, + limit: int, + after: str | None = None, +) -> list[SubmissionRecord]: + """ + read one page of stored submissions, newest first + :param session: the session to query through + :param filters: the endpoint and time bounds to narrow the page to + :param limit: the most submissions to return + :param after: identifier of the last submission on the previous page, or None to start + :returns: the page, at most ``limit`` long + :raises UnknownCursor: if ``after`` does not name an existing submission + """ + submission = models.Submission + delivery = models.WebhookDelivery + statement = ( + # An outer join, because a submission to an endpoint with no webhook owes + # no delivery and must still be listed. One join rather than a lookup per + # row, and it rides the unique constraint on the delivery's submission. + select(submission, delivery) + .outerjoin(delivery, delivery.submission_id == submission.id) + .order_by(submission.received_at.desc(), submission.id.desc()) + ) + statement = _narrowed(statement, filters) + if after is not None: + statement = statement.where( + _page_after(session, submission, after, ordered_by=submission.received_at) + ) + + return [SubmissionRecord(row, owed) for row, owed in session.execute(statement.limit(limit))] + + +def get_submission(session: Session, submission_id: str) -> SubmissionRecord | None: + """ + look one stored submission up by its identifier + :param session: the session to query through + :param submission_id: the identifier to resolve + :returns: the submission and its delivery, or None if no submission holds that identifier + """ + submission = models.Submission + delivery = models.WebhookDelivery + row = session.execute( + select(submission, delivery) + .outerjoin(delivery, delivery.submission_id == submission.id) + .where(submission.id == submission_id) + ).one_or_none() + return SubmissionRecord(row[0], row[1]) if row is not None else None + + +def count_submissions(session: Session, *, filters: SubmissionFilter, ceiling: int) -> int: + """ + count the matching submissions, giving up once a ceiling is reached + :param session: the session to query through + :param filters: the endpoint and time bounds to count within + :param ceiling: the most rows to count before stopping + :returns: the number of matches, or ``ceiling`` when there are at least that many + """ + # Counting through a bounded subquery rather than counting the table, so a + # filter matching millions costs a walk of ``ceiling`` index entries and not + # a walk of everything. The caller only needs to know whether the export fits. + bounded = _narrowed(select(models.Submission.id), filters).limit(ceiling).subquery() + return cast(int, session.scalar(select(func.count()).select_from(bounded))) + + +def stream_submissions( + session: Session, *, filters: SubmissionFilter, limit: int +) -> Iterator[models.Submission]: + """ + read the matching submissions in export order, a chunk at a time + :param session: the session to query through + :param filters: the endpoint and time bounds to export within + :param limit: the most submissions to yield, whatever the filter matches + :returns: an iterator over the matching submissions, newest first + """ + # ``yield_per`` is what makes this a stream rather than a list wearing an + # iterator's clothes: rows arrive from the server in chunks, so an export + # writes its first bytes without every matching row being in memory. The + # limit is still applied, so even a runaway cursor is bounded. + submission = models.Submission + statement = _narrowed( + select(submission).order_by(submission.received_at.desc(), submission.id.desc()), + filters, + ).limit(limit) + return iter(session.scalars(statement.execution_options(yield_per=EXPORT_FETCH_SIZE))) + + +def count_expired_submissions(session: Session, *, before: datetime) -> int: + """ + count the submissions a retention sweep would delete + :param session: the session to query through + :param before: submissions received strictly before this instant are eligible + :returns: how many submissions are eligible for deletion + """ + return cast( + int, + session.scalar( + select(func.count()).select_from(models.Submission).where(_expired_condition(before)) + ), + ) + + +def delete_expired_submissions( + session: Session, *, before: datetime, batch_size: int, max_batches: int +) -> int: + """ + delete eligible submissions in committed batches until none are left + :param session: the session to write through + :param before: submissions received strictly before this instant are eligible + :param batch_size: the most submissions to remove in one transaction + :param max_batches: the most batches this run will do before stopping + :returns: how many submissions were deleted + """ + # Many short transactions rather than one long one. A single DELETE over a + # large backlog would hold locks on every row it touched for as long as the + # whole sweep took; this way each batch is committed and released, and a run + # that is interrupted has still durably removed everything it reported. + # + # The eligible identifiers are read first and deleted by identifier, so the + # delete is a primary key lookup and the eligibility test is evaluated once + # rather than being re-planned inside a delete. + removed = 0 + for _ in range(max_batches): + ids = list( + session.scalars( + select(models.Submission.id).where(_expired_condition(before)).limit(batch_size) + ) + ) + if not ids: + break + + # The delivery and the attempts that referenced these submissions are not + # touched. Their foreign keys are ``ON DELETE SET NULL``, so the database + # unlinks them and leaves the operational history standing. + result = cast( + "CursorResult[Any]", + session.execute( + delete(models.Submission) + .where(models.Submission.id.in_(ids)) + .execution_options(synchronize_session=False) + ), + ) + session.commit() + removed += result.rowcount + if len(ids) < batch_size: + break + + return removed + + +def _expired_condition(before: datetime) -> ColumnElement[bool]: + """ + build the test for a submission a retention sweep may delete + :param before: submissions received strictly before this instant are eligible + :returns: a SQL condition matching only submissions nothing still needs + """ + # Old enough, and not carrying a delivery that could still be attempted. The + # payload is built from the submission at send time, so a pending, processing + # or replayable failed delivery would be left with nothing to send. A + # submission with no delivery, and one whose delivery is already delivered, + # both pass, because nothing will read their fields again. + delivery = models.WebhookDelivery + still_needed = select(delivery.id).where( + delivery.submission_id == models.Submission.id, + delivery.state.in_(PROTECTED_DELIVERY_STATES), + ) + return and_(models.Submission.received_at < before, ~still_needed.exists()) + + +def _narrowed(statement: _Statement, filters: SubmissionFilter) -> _Statement: + """ + apply the endpoint and time bounds a management read asked for + :param statement: the select to narrow + :param filters: the endpoint and time bounds to apply + :returns: the same select, narrowed + """ + # Both time bounds are strict. An operator paging forward by passing the last + # timestamp back as ``received_after`` gets the next submissions rather than + # the one they already have, which is the behaviour the API documents. + submission = models.Submission + if filters.endpoint_id is not None: + statement = statement.where(submission.endpoint_id == filters.endpoint_id) + if filters.received_after is not None: + statement = statement.where(submission.received_at > filters.received_after) + if filters.received_before is not None: + statement = statement.where(submission.received_at < filters.received_before) + return statement + + def due_condition(now: datetime) -> ColumnElement[bool]: """ build the test for a delivery a worker is allowed to pick up @@ -356,10 +601,19 @@ def due_condition(now: datetime) -> ColumnElement[bool]: # Two ways to be claimable: waiting and due, or claimed by a worker whose # lease has run out. The second is what recovers work from a worker that died # holding a job, rather than leaving it stuck in ``processing`` forever. + # + # A delivery with no submission is never due, whichever state it is in. The + # payload is built from the submission at send time, so a delivery without + # one has nothing to send. Retention can only unlink a delivery that has + # already been delivered, which this condition excludes anyway; the test is + # here so that the guarantee is the query's rather than the sweep's. delivery = models.WebhookDelivery - return or_( - and_(delivery.state == DeliveryState.PENDING, delivery.next_attempt_at <= now), - and_(delivery.state == DeliveryState.PROCESSING, delivery.claim_expires_at <= now), + return and_( + delivery.submission_id.is_not(None), + or_( + and_(delivery.state == DeliveryState.PENDING, delivery.next_attempt_at <= now), + and_(delivery.state == DeliveryState.PROCESSING, delivery.claim_expires_at <= now), + ), ) @@ -408,18 +662,30 @@ def claim_due_deliveries( return claimed -def load_submissions(session: Session, submission_ids: list[str]) -> dict[str, Submission]: +def load_submissions( + session: Session, deliveries: list[models.WebhookDelivery] +) -> dict[str, Submission]: """ - load the submissions a batch of deliveries is carrying + load the submission each claimed delivery is carrying :param session: the session to query through - :param submission_ids: the submissions to fetch - :returns: the submissions in domain form, keyed by id + :param deliveries: the deliveries a worker has just claimed + :returns: the submissions in domain form, keyed by the delivery that carries each one """ + # Keyed by delivery rather than by submission, because the delivery is what + # the caller is holding and because the link between them is now nullable. + # Nothing due can have lost its submission, since only a delivered delivery + # is ever unlinked and a delivered delivery is not claimable, so this filter + # states that rather than relying on it. + carried = {job.id: job.submission_id for job in deliveries if job.submission_id is not None} + # One query for the batch rather than a lookup per delivery. - rows = session.scalars( - select(models.Submission).where(models.Submission.id.in_(submission_ids)) - ) - return {row.id: row.to_domain() for row in rows} + rows = { + row.id: row.to_domain() + for row in session.scalars( + select(models.Submission).where(models.Submission.id.in_(carried.values())) + ) + } + return {job_id: rows[submission_id] for job_id, submission_id in carried.items()} def complete_attempt( @@ -483,19 +749,6 @@ def complete_attempt( return attempt -@dataclass(frozen=True, slots=True) -class DeliveryRecord: - """ - a delivery together with the endpoint whose submission it carries - """ - - # The endpoint is not a column on the delivery: it is reached through the - # submission. Carrying it alongside means a management response can report - # which endpoint a delivery belongs to without a second query per row. - delivery: models.WebhookDelivery - endpoint_id: str - - def list_deliveries( session: Session, *, @@ -503,7 +756,7 @@ def list_deliveries( after: str | None = None, endpoint_id: str | None = None, state: str | None = None, -) -> list[DeliveryRecord]: +) -> list[models.WebhookDelivery]: """ read one page of the delivery queue, newest first :param session: the session to query through @@ -514,31 +767,31 @@ def list_deliveries( :returns: the page, at most ``limit`` long :raises UnknownCursor: if ``after`` does not name an existing delivery """ + # No join. The endpoint is a column on the delivery, so a delivery whose + # submission has been retained away is still listed, still says which + # endpoint it belonged to, and is still reachable through this filter. delivery = models.WebhookDelivery - statement = _delivery_query().order_by(delivery.created_at.desc(), delivery.id.desc()) + statement = select(delivery).order_by(delivery.created_at.desc(), delivery.id.desc()) if endpoint_id is not None: - statement = statement.where(models.Submission.endpoint_id == endpoint_id) + statement = statement.where(delivery.endpoint_id == endpoint_id) if state is not None: statement = statement.where(delivery.state == state) if after is not None: - statement = statement.where(_page_after(session, delivery, after)) + statement = statement.where( + _page_after(session, delivery, after, ordered_by=delivery.created_at) + ) - return [ - DeliveryRecord(row, endpoint) for row, endpoint in session.execute(statement.limit(limit)) - ] + return list(session.scalars(statement.limit(limit))) -def get_delivery(session: Session, delivery_id: str) -> DeliveryRecord | None: +def get_delivery(session: Session, delivery_id: str) -> models.WebhookDelivery | None: """ look one delivery up by its identifier :param session: the session to query through :param delivery_id: the identifier to resolve - :returns: the delivery and its endpoint, or None if no delivery holds that identifier + :returns: the delivery, or None if no delivery holds that identifier """ - row = session.execute( - _delivery_query().where(models.WebhookDelivery.id == delivery_id) - ).one_or_none() - return DeliveryRecord(row[0], row[1]) if row is not None else None + return session.get(models.WebhookDelivery, delivery_id) def list_delivery_attempts(session: Session, delivery_id: str) -> list[models.DeliveryAttempt]: @@ -570,7 +823,7 @@ class ReplayOutcome: # ``record`` is None only when the delivery does not exist. ``requeued`` is # False for a delivery that was not failed, which includes the loser of two # simultaneous replays: it reads back the pending delivery the winner made. - record: DeliveryRecord | None + record: models.WebhookDelivery | None requeued: bool @@ -617,16 +870,6 @@ def requeue_failed_delivery(session: Session, delivery_id: str, *, now: datetime return ReplayOutcome(get_delivery(session, delivery_id), requeued=result.rowcount == 1) -def _delivery_query() -> Select[tuple[models.WebhookDelivery, str]]: - """ - build the read every delivery management route starts from - :returns: a select of deliveries joined to the endpoint they belong to - """ - return select(models.WebhookDelivery, models.Submission.endpoint_id).join( - models.Submission, models.Submission.id == models.WebhookDelivery.submission_id - ) - - def create_management_key( session: Session, *, @@ -856,16 +1099,26 @@ def _upsert_for(session: Session) -> Any: raise UnsupportedRateLimitBackend(dialect) -def _page_after(session: Session, model: type[_Paged], cursor: str) -> ColumnElement[bool]: +def _page_after( + session: Session, + model: type[_Paged], + cursor: str, + *, + ordered_by: Mapped[datetime], +) -> ColumnElement[bool]: """ build the test for rows that come after a cursor, in newest-first order :param session: the session to resolve the cursor through :param model: the table being paged :param cursor: the identifier of the last row on the previous page + :param ordered_by: the timestamp column the page is ordered by, newest first :returns: a SQL condition matching only the rows that follow it :raises UnknownCursor: if the cursor does not name an existing row """ - anchor = session.get(model, cursor) + # The anchor's ordering value is read through the column the caller named + # rather than off a loaded row, so this works for any table without knowing + # what its timestamp is called. + anchor = session.execute(select(ordered_by, model.id).where(model.id == cursor)).one_or_none() if anchor is None: # Refused rather than treated as the first page, so a caller that pages # past a row somebody deleted learns about it instead of silently @@ -876,7 +1129,7 @@ def _page_after(session: Session, model: type[_Paged], cursor: str) -> ColumnEle # rather than two that have to be kept in agreement. Both timestamps and both # identifiers are compared, which is what makes a page boundary total even # when several rows were created in the same transaction. - return tuple_(model.created_at, model.id) < (anchor.created_at, anchor.id) + return tuple_(ordered_by, model.id) < (anchor[0], anchor[1]) def _settle(existing: models.Submission, payload_fingerprint: str | None) -> Submission: diff --git a/src/hymical_forms/worker.py b/src/hymical_forms/worker.py index 56d60c9..3229290 100644 --- a/src/hymical_forms/worker.py +++ b/src/hymical_forms/worker.py @@ -55,14 +55,14 @@ async def process_batch( if not claimed: return 0 - submissions = storage.load_submissions(session, [job.submission_id for job in claimed]) + submissions = storage.load_submissions(session, claimed) # The network calls overlap so that one unresponsive destination does not # hold up the rest of the batch for its whole timeout. They are made with no # database transaction open: holding one across somebody else's server would # pin a connection for as long as they take to answer. bodies = { - job.id: webhooks.serialize_payload(webhooks.build_payload(submissions[job.submission_id])) + job.id: webhooks.serialize_payload(webhooks.build_payload(submissions[job.id])) for job in claimed } results = await asyncio.gather( diff --git a/tests/conftest.py b/tests/conftest.py index 7e1fd11..d945595 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -28,12 +28,14 @@ from pydantic_settings import SettingsConfigDict from sqlalchemy.orm import Session, sessionmaker -from hymical_forms import apikeys, storage +from hymical_forms import apikeys, models, storage from hymical_forms.app import create_app from hymical_forms.config import Settings from hymical_forms.delivery import create_webhook_client +from hymical_forms.ingestion import new_submission_id from hymical_forms.models import utcnow from hymical_forms.schema import create_all +from hymical_forms.webhooks import DeliveryState, new_webhook_delivery_id from hymical_forms.worker import process_batch from webhook_server import WebhookRecorder @@ -108,6 +110,66 @@ def create_endpoint( return cast(dict[str, Any], response.json()) +def seed_submission( + client: TestClient, + *, + received_at: datetime, + endpoint_id: str = DEFAULT_ENDPOINT_ID, + fields: dict[str, list[str]] | None = None, + idempotency_key: str | None = None, + delivery_state: DeliveryState | None = None, + attempts: int = 0, +) -> str: + """ + write a submission straight into a client's database, at a chosen moment + :param client: the client whose application database should hold it + :param received_at: the instant the submission should claim it was accepted + :param endpoint_id: the endpoint it belongs to + :param fields: the submitted values, defaulting to one email field + :param idempotency_key: the key it was sent with, or None if it was sent without one + :param delivery_state: the state of the delivery it owes, or None to owe none + :param attempts: how many requests have been made for that delivery + :returns: the submission identifier + """ + # Ingestion decides ``received_at`` from the clock, so anything about time + # ranges or retention has to write the row rather than post to the endpoint. + # It goes in through the models the API reads back, not through raw SQL, so a + # seeded submission is indistinguishable from a submitted one. + submission_id = new_submission_id() + terminal = delivery_state in (DeliveryState.DELIVERED, DeliveryState.FAILED) + with open_session(client) as session: + session.add( + models.Submission( + id=submission_id, + endpoint_id=endpoint_id, + received_at=received_at, + fields=fields if fields is not None else {"email": ["dev@example.com"]}, + idempotency_key=idempotency_key, + # Paired with the key by a check constraint, and never read back + # by anything these tests assert on. + payload_fingerprint="f" * 64 if idempotency_key is not None else None, + ) + ) + if delivery_state is not None: + session.add( + models.WebhookDelivery( + id=new_webhook_delivery_id(), + submission_id=submission_id, + endpoint_id=endpoint_id, + destination_url="https://example.invalid/hook", + signing_secret="whsec_" + "a" * 64, + state=delivery_state, + attempts=attempts, + cycle_attempts=attempts, + next_attempt_at=received_at, + created_at=received_at, + completed_at=received_at if terminal else None, + ) + ) + session.commit() + return submission_id + + def bearer(api_key: str) -> dict[str, str]: """ build the authorization header a management request carries diff --git a/tests/integration/support.py b/tests/integration/support.py index f294b76..1a29af3 100644 --- a/tests/integration/support.py +++ b/tests/integration/support.py @@ -142,6 +142,7 @@ def seed_failed_delivery( models.WebhookDelivery( id=delivery_id, submission_id=submission_id, + endpoint_id=endpoint_id, destination_url="https://example.invalid/hook", signing_secret="whsec_" + "a" * 64, state=DeliveryState.FAILED, @@ -201,6 +202,7 @@ def seed_due_deliveries( models.WebhookDelivery( id=delivery_id, submission_id=submission_id, + endpoint_id=endpoint_id, destination_url="https://example.invalid/hook", signing_secret="whsec_" + "a" * 64, state=DeliveryState.PENDING, diff --git a/tests/integration/test_constraints_postgres.py b/tests/integration/test_constraints_postgres.py index 0635429..f2e1e01 100644 --- a/tests/integration/test_constraints_postgres.py +++ b/tests/integration/test_constraints_postgres.py @@ -54,6 +54,7 @@ def a_delivery(delivery_id: str, submission_id: str) -> models.WebhookDelivery: return models.WebhookDelivery( id=delivery_id, submission_id=submission_id, + endpoint_id="contact-form", destination_url="https://example.invalid/hook", signing_secret="whsec_" + "a" * 64, state=DeliveryState.PENDING, diff --git a/tests/integration/test_migrations_postgres.py b/tests/integration/test_migrations_postgres.py index eee06dd..3e13d6f 100644 --- a/tests/integration/test_migrations_postgres.py +++ b/tests/integration/test_migrations_postgres.py @@ -12,6 +12,7 @@ from contextlib import contextmanager from datetime import UTC, datetime +import pytest from alembic import command from alembic.autogenerate import compare_metadata from alembic.config import Config @@ -98,6 +99,7 @@ def test_the_migration_creates_the_constraints_the_application_relies_on( "ck_webhook_deliveries_completion", "fk_submissions_endpoint_id_endpoints", "fk_webhook_deliveries_submission_id_submissions", + "fk_webhook_deliveries_endpoint_id_endpoints", "fk_delivery_attempts_delivery_id_webhook_deliveries", "fk_delivery_attempts_submission_id_submissions", } <= names @@ -440,7 +442,7 @@ def test_a_populated_0003_survives_the_whole_round_trip(postgres_url: str) -> No command.upgrade(config, "0004") command.downgrade(config, "0003") - command.upgrade(config, "0004") + command.upgrade(config, "head") assert current_revision(engine) == head_revision() with engine.connect() as connection: @@ -456,6 +458,219 @@ def test_a_populated_0003_survives_the_whole_round_trip(postgres_url: str) -> No assert difference == [], f"migrated schema differs from the models: {difference}" +# --- the retention schema 0005 introduced ------------------------------------ +# +# 0005 is the first revision that loosens a constraint rather than adding one, so +# what it has to prove is that the loosening is deliberate and that the column it +# backfills ends up holding what the join it replaces used to produce. + + +def test_upgrading_a_populated_0004_backfills_the_delivery_endpoint(postgres_url: str) -> None: + """ + the column that replaces a join must arrive holding what the join produced + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + command.upgrade(config, "0004") + + command.upgrade(config, "0005") + + assert current_revision(engine) == "0005" + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + endpoint = connection.scalar( + text("select endpoint_id from webhook_deliveries where id = :id"), + {"id": SEEDED_DELIVERY}, + ) + assert endpoint == SEEDED_ENDPOINT + + +def test_the_delivery_endpoint_is_not_nullable(postgres_url: str) -> None: + with _database_at_baseline(postgres_url) as (config, engine): + command.upgrade(config, "0005") + + with engine.connect() as connection: + nullable = connection.scalar( + text( + "select is_nullable from information_schema.columns " + "where table_name = 'webhook_deliveries' and column_name = 'endpoint_id'" + ) + ) + + assert nullable == "NO" + + +@pytest.mark.parametrize("table", ["webhook_deliveries", "delivery_attempts"]) +def test_the_submission_link_becomes_nullable(postgres_url: str, table: str) -> None: + """ + a submission cannot be deleted without somewhere for its history to point instead + :param postgres_url: a URL on the PostgreSQL server to work against + :param table: the table whose link to the submission is being checked + """ + with _database_at_baseline(postgres_url) as (config, engine): + command.upgrade(config, "0005") + + with engine.connect() as connection: + nullable = connection.scalar( + text( + "select is_nullable from information_schema.columns " + "where table_name = :table and column_name = 'submission_id'" + ), + {"table": table}, + ) + + assert nullable == "YES" + + +@pytest.mark.parametrize( + "constraint", + [ + "fk_webhook_deliveries_submission_id_submissions", + "fk_delivery_attempts_submission_id_submissions", + ], +) +def test_deleting_a_submission_unlinks_history_rather_than_cascading( + postgres_url: str, constraint: str +) -> None: + """ + a cascade here would take the operational history with the form content + :param postgres_url: a URL on the PostgreSQL server to work against + :param constraint: the foreign key whose delete rule is being checked + """ + with _database_at_baseline(postgres_url) as (config, engine): + command.upgrade(config, "0005") + + with engine.connect() as connection: + rule = connection.scalar( + text( + "select confdeltype from pg_constraint where conname = :name", + ), + {"name": constraint}, + ) + + # ``n`` is SET NULL. ``c`` would be CASCADE, which is the mistake this asserts + # against, and ``a`` would be NO ACTION, which is what 0004 had. + assert rule == "n" + + +def test_the_submission_indexes_match_what_management_reads(postgres_url: str) -> None: + with _database_at_baseline(postgres_url) as (config, engine): + command.upgrade(config, "0005") + + with engine.connect() as connection: + indexed = set( + connection.scalars( + text("select indexname from pg_indexes where tablename = 'submissions'") + ) + ) + + assert "ix_submissions_received_at_id" in indexed + assert "ix_submissions_endpoint_id_received_at_id" in indexed + # Replaced rather than joined by the composite, whose leftmost column answers + # the same lookup. + assert "ix_submissions_endpoint_id" not in indexed + + +def test_downgrading_from_0005_removes_what_it_added(postgres_url: str) -> None: + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + command.upgrade(config, "0005") + + command.downgrade(config, "0004") + + assert current_revision(engine) == "0004" + columns = {column["name"] for column in inspect(engine).get_columns("webhook_deliveries")} + assert "endpoint_id" not in columns + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + + +def test_downgrading_removes_only_the_deliveries_that_lost_their_submission( + postgres_url: str, +) -> None: + """ + the older schema cannot hold an unlinked delivery, and everything else must survive + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + command.upgrade(config, "0005") + with engine.begin() as connection: + connection.execute( + text( + "insert into submissions (id, endpoint_id, received_at, fields) values " + "(:id, :endpoint, :now, :fields)" + ), + { + "id": "sub_44444444444444444444444444444444", + "endpoint": SEEDED_ENDPOINT, + "now": SEEDED_AT, + "fields": '{"email": ["gone@example.com"]}', + }, + ) + connection.execute( + text( + "insert into webhook_deliveries (id, submission_id, endpoint_id, " + "destination_url, signing_secret, state, attempts, cycle_attempts, " + "next_attempt_at, created_at, completed_at) values " + "(:id, :submission, :endpoint, 'https://example.invalid/hook', :secret, " + "'delivered', 1, 1, :now, :now, :now)" + ), + { + "id": "whd_55555555555555555555555555555555", + "submission": "sub_44444444444444444444444444444444", + "endpoint": SEEDED_ENDPOINT, + "now": SEEDED_AT, + "secret": "whsec_" + "a" * 64, + }, + ) + # Retention taking a delivered submission, which is the only way a + # delivery ever ends up without one. + connection.execute( + text("delete from submissions where id = :id"), + {"id": "sub_44444444444444444444444444444444"}, + ) + + command.downgrade(config, "0004") + + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + remaining = set(connection.scalars(text("select id from webhook_deliveries"))) + assert remaining == {SEEDED_DELIVERY} + + +def test_a_populated_0004_survives_the_whole_round_trip(postgres_url: str) -> None: + """ + populated 0004 to 0005 and back and forward again must end with zero drift + :param postgres_url: a URL on the PostgreSQL server to work against + """ + with _database_at_baseline(postgres_url) as (config, engine): + with engine.begin() as connection: + _seed_baseline_data(connection) + command.upgrade(config, "0004") + + command.upgrade(config, "0005") + command.downgrade(config, "0004") + command.upgrade(config, "head") + + assert current_revision(engine) == head_revision() + with engine.connect() as connection: + _assert_baseline_data_intact(connection) + assert ( + connection.scalar( + text("select endpoint_id from webhook_deliveries where id = :id"), + {"id": SEEDED_DELIVERY}, + ) + == SEEDED_ENDPOINT + ) + difference = compare_metadata(MigrationContext.configure(connection), Base.metadata) + assert difference == [], f"migrated schema differs from the models: {difference}" + + @contextmanager def _database_at_baseline(postgres_url: str) -> Iterator[tuple[Config, Engine]]: """ diff --git a/tests/integration/test_replay_postgres.py b/tests/integration/test_replay_postgres.py index 82a7ea2..1c71f1c 100644 --- a/tests/integration/test_replay_postgres.py +++ b/tests/integration/test_replay_postgres.py @@ -70,7 +70,7 @@ def replay() -> storage.ReplayOutcome: # than the one it hoped for, so its answer is the same however the race went. loser = first if not first.requeued else second assert loser.record is not None - assert loser.record.delivery.state == DeliveryState.PENDING + assert loser.record.state == DeliveryState.PENDING def test_a_concurrent_replay_duplicates_no_work(sessions: sessionmaker[Session]) -> None: @@ -131,7 +131,7 @@ def test_a_delivery_that_was_never_failed_is_refused(sessions: sessionmaker[Sess assert outcome.requeued is False assert outcome.record is not None - assert outcome.record.delivery.state == DeliveryState.DELIVERED + assert outcome.record.state == DeliveryState.DELIVERED def test_replay_over_http_answers_from_the_settled_state( diff --git a/tests/integration/test_retention_postgres.py b/tests/integration/test_retention_postgres.py new file mode 100644 index 0000000..895142a --- /dev/null +++ b/tests/integration/test_retention_postgres.py @@ -0,0 +1,327 @@ +""" +retention against a real PostgreSQL database + +SQLite enforces the foreign keys this suite runs on, but only because the test +harness turns enforcement on, and it has no opinion about what a real batch of +deletes costs. The point of running retention here is that the ``ON DELETE SET +NULL`` behaviour, the batching and the export queries meet the database this +service is actually deployed against. +""" + +from __future__ import annotations + +import csv +import io +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session, sessionmaker + +from hymical_forms import models, storage +from hymical_forms.webhooks import DeliveryState +from integration.support import seed_endpoint + +NOW = datetime(2026, 8, 25, 12, 0, tzinfo=UTC) +CUTOFF = NOW - timedelta(days=30) + +ENDPOINT_ID = "contact-form" + + +def seed_submission( + session: Session, + *, + received_at: datetime, + fields: dict[str, list[str]] | None = None, + delivery_state: DeliveryState | None = None, + attempts: int = 0, +) -> str: + """ + insert one submission, optionally owing a delivery with a history + :param session: the session to insert through + :param received_at: the instant the submission claims it was accepted + :param fields: the submitted values, defaulting to one email field + :param delivery_state: the state of the delivery it owes, or None to owe none + :param attempts: how many requests have been made for that delivery + :returns: the submission identifier + """ + submission_id = f"sub_{uuid.uuid4().hex}" + delivery_id = f"whd_{uuid.uuid4().hex}" + terminal = delivery_state in (DeliveryState.DELIVERED, DeliveryState.FAILED) + session.add( + models.Submission( + id=submission_id, + endpoint_id=ENDPOINT_ID, + received_at=received_at, + fields=fields if fields is not None else {"email": ["dev@example.com"]}, + ) + ) + if delivery_state is not None: + session.add( + models.WebhookDelivery( + id=delivery_id, + submission_id=submission_id, + endpoint_id=ENDPOINT_ID, + destination_url="https://example.invalid/hook", + signing_secret="whsec_" + "a" * 64, + state=delivery_state, + attempts=attempts, + cycle_attempts=attempts, + next_attempt_at=received_at, + created_at=received_at, + completed_at=received_at if terminal else None, + ) + ) + session.flush() + for number in range(1, attempts + 1): + session.add( + models.DeliveryAttempt( + id=f"att_{uuid.uuid4().hex}", + delivery_id=delivery_id, + submission_id=submission_id, + attempt_number=number, + destination_url="https://example.invalid/hook", + attempted_at=received_at, + outcome="http_error", + response_status=503, + ) + ) + session.commit() + return submission_id + + +# --- foreign keys ------------------------------------------------------------ + + +def test_deleting_a_submission_unlinks_its_delivery(sessions: sessionmaker[Session]) -> None: + """ + the database, not the sweep, is what keeps the history when the content goes + :param sessions: factory handing out independent connections + """ + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + seed_submission( + session, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + attempts=2, + ) + + with sessions() as session: + removed = storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=100, max_batches=10 + ) + + assert removed == 1 + with sessions() as session: + delivery = session.scalars(select(models.WebhookDelivery)).one() + attempts = list(session.scalars(select(models.DeliveryAttempt))) + assert delivery.submission_id is None + assert delivery.state == DeliveryState.DELIVERED + assert delivery.endpoint_id == ENDPOINT_ID + assert [attempt.attempt_number for attempt in attempts] == [1, 2] + assert all(attempt.submission_id is None for attempt in attempts) + + +def test_a_delivery_still_cannot_name_a_submission_that_never_existed( + sessions: sessionmaker[Session], +) -> None: + """ + loosening the column to nullable must not have loosened the reference itself + :param sessions: factory handing out independent connections + """ + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + session.add( + models.WebhookDelivery( + id=f"whd_{uuid.uuid4().hex}", + submission_id="sub_does_not_exist", + endpoint_id=ENDPOINT_ID, + destination_url="https://example.invalid/hook", + signing_secret="whsec_" + "a" * 64, + state=DeliveryState.PENDING, + attempts=0, + cycle_attempts=0, + next_attempt_at=NOW, + created_at=NOW, + ) + ) + with pytest.raises(IntegrityError): + session.commit() + + +def test_a_protected_submission_survives_a_sweep(sessions: sessionmaker[Session]) -> None: + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + pending = seed_submission( + session, + received_at=CUTOFF - timedelta(days=365), + delivery_state=DeliveryState.PENDING, + ) + failed = seed_submission( + session, + received_at=CUTOFF - timedelta(days=365), + delivery_state=DeliveryState.FAILED, + attempts=5, + ) + + with sessions() as session: + removed = storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=100, max_batches=10 + ) + + assert removed == 0 + with sessions() as session: + assert set(session.scalars(select(models.Submission.id))) == {pending, failed} + + +# --- batching ---------------------------------------------------------------- + + +def test_a_sweep_clears_a_backlog_in_batches(sessions: sessionmaker[Session]) -> None: + """ + many small committed transactions rather than one that holds locks over everything + :param sessions: factory handing out independent connections + """ + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + for index in range(25): + seed_submission(session, received_at=CUTOFF - timedelta(days=1 + index)) + + with sessions() as session: + removed = storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=4, max_batches=100 + ) + + assert removed == 25 + with sessions() as session: + assert session.scalars(select(models.Submission.id)).all() == [] + + +def test_a_sweep_that_is_cut_short_has_still_committed_what_it_removed( + sessions: sessionmaker[Session], +) -> None: + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + for index in range(10): + seed_submission(session, received_at=CUTOFF - timedelta(days=1 + index)) + + with sessions() as session: + removed = storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=3, max_batches=2 + ) + + assert removed == 6 + # Read on a different connection, so what is asserted is what was committed + # rather than what one session happens to be holding. + with sessions() as session: + assert len(list(session.scalars(select(models.Submission.id)))) == 4 + + +# --- the management surface over real postgres ------------------------------- + + +def test_submissions_are_listed_over_http( + pg_client: TestClient, sessions: sessionmaker[Session] +) -> None: + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + older = seed_submission(session, received_at=NOW - timedelta(hours=2)) + newer = seed_submission(session, received_at=NOW) + + body = pg_client.get("/submissions").json() + + assert [item["id"] for item in body["items"]] == [newer, older] + + +def test_a_filtered_page_narrows_over_http( + pg_client: TestClient, sessions: sessionmaker[Session] +) -> None: + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + seed_submission(session, received_at=NOW - timedelta(days=2)) + wanted = seed_submission(session, received_at=NOW) + + body = pg_client.get( + "/submissions", + params={ + "endpoint_id": ENDPOINT_ID, + "received_after": (NOW - timedelta(days=1)).isoformat(), + }, + ).json() + + assert [item["id"] for item in body["items"]] == [wanted] + + +def test_an_export_streams_the_filtered_set_over_http( + pg_client: TestClient, sessions: sessionmaker[Session] +) -> None: + """ + the export opens a session of its own, which only a real backend proves works + :param pg_client: an API client on the migrated PostgreSQL database + :param sessions: factory handing out independent connections + """ + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + for index in range(30): + seed_submission( + session, + received_at=NOW - timedelta(minutes=index), + fields={"email": [f"dev{index}@example.com"], "topics": ["api", "billing"]}, + ) + + response = pg_client.get("/submissions/export") + + assert response.status_code == 200 + exported = response.json()["submissions"] + assert len(exported) == 30 + assert exported[0]["fields"]["topics"] == ["api", "billing"] + + +def test_a_csv_export_works_over_http( + pg_client: TestClient, sessions: sessionmaker[Session] +) -> None: + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + seed_submission(session, received_at=NOW, fields={"naam": ["Zoë"], "topics": ["api"]}) + + response = pg_client.get("/submissions/export", params={"format": "csv"}) + + assert response.status_code == 200 + header, row = list(csv.reader(io.StringIO(response.text))) + assert header == ["submission_id", "endpoint_id", "received_at", "naam", "topics"] + assert row[3] == '["Zoë"]' + assert row[4] == '["api"]' + + +def test_a_delivery_whose_submission_was_swept_is_still_listed( + pg_client: TestClient, sessions: sessionmaker[Session] +) -> None: + """ + operational history has to stay readable after the form content has gone + :param pg_client: an API client on the migrated PostgreSQL database + :param sessions: factory handing out independent connections + """ + with sessions() as session: + seed_endpoint(session, ENDPOINT_ID) + seed_submission( + session, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + attempts=1, + ) + with sessions() as session: + storage.delete_expired_submissions(session, before=CUTOFF, batch_size=100, max_batches=10) + + listed = pg_client.get("/deliveries", params={"endpoint_id": ENDPOINT_ID}).json()["items"] + + assert len(listed) == 1 + assert listed[0]["submission_id"] is None + assert listed[0]["endpoint_id"] == ENDPOINT_ID + assert listed[0]["state"] == DeliveryState.DELIVERED + + detail = pg_client.get(f"/deliveries/{listed[0]['id']}").json() + assert len(detail["attempts"]) == 1 diff --git a/tests/test_openapi.py b/tests/test_openapi.py index a8be3fd..6e6adaa 100644 --- a/tests/test_openapi.py +++ b/tests/test_openapi.py @@ -26,6 +26,9 @@ ("/deliveries", "get"), ("/deliveries/{delivery_id}", "get"), ("/deliveries/{delivery_id}/replay", "post"), + ("/submissions", "get"), + ("/submissions/export", "get"), + ("/submissions/{submission_id}", "get"), ] PUBLIC_OPERATIONS = [ @@ -144,5 +147,32 @@ def test_the_delivery_views_carry_no_submitted_fields(client: TestClient) -> Non def test_a_page_response_has_the_shared_shape(client: TestClient) -> None: components = schema(client)["components"]["schemas"] - for name in ("EndpointPage", "DeliveryPage"): + for name in ("EndpointPage", "DeliveryPage", "SubmissionPage"): assert set(components[name]["properties"]) == {"items", "next_cursor"}, name + + +def test_a_submission_listing_cannot_name_the_submitted_values(client: TestClient) -> None: + """ + a summary model with no fields property cannot leak one however the route changes + :param client: test client whose app holds the default endpoint + """ + components = schema(client)["components"]["schemas"] + + assert "fields" not in components["SubmissionSummary"]["properties"] + # The detail model is the one that is meant to carry them. + assert "fields" in components["SubmissionDetail"]["properties"] + + +def test_no_submission_response_exposes_an_internal_column(client: TestClient) -> None: + components = schema(client)["components"]["schemas"] + + for name in ("SubmissionSummary", "SubmissionDetail", "SubmissionExport"): + properties = set(components[name]["properties"]) + assert "payload_fingerprint" not in properties, name + assert "idempotency_key" not in properties, name + + +def test_the_export_route_documents_both_formats(client: TestClient) -> None: + content = operation(client, "/submissions/export", "get")["responses"]["200"]["content"] + + assert set(content) == {"application/json", "text/csv"} diff --git a/tests/test_retention.py b/tests/test_retention.py new file mode 100644 index 0000000..dfd0dc6 --- /dev/null +++ b/tests/test_retention.py @@ -0,0 +1,539 @@ +""" +the retention rule, and the operator command that applies it + +The rule is the interesting part. A submission is only deleted once nothing will +read its content again, and what still needs it is decided by the state of the +delivery it owes rather than by its age alone: a queued webhook payload is built +from the submission at the moment it is sent, so anything still deliverable keeps +its submission however old that submission is. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Iterator +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest +from sqlalchemy import select, update +from sqlalchemy.orm import Session, sessionmaker + +from conftest import IsolatedSettings, build_settings +from hymical_forms import cli, models, storage +from hymical_forms.db import create_engine_from_url, create_session_factory +from hymical_forms.retention import RetentionDisabled, RetentionPolicy +from hymical_forms.schema import create_all +from hymical_forms.webhooks import DeliveryState + +NOW = datetime(2026, 8, 25, 12, 0, tzinfo=UTC) +CUTOFF = NOW - timedelta(days=30) + +ENDPOINT_ID = "contact-form" + + +@pytest.fixture +def sessions(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[sessionmaker[Session]]: + """ + provide a session factory on a migrated SQLite file the CLI will also find + :param tmp_path: pytest fixture giving this test a directory of its own + :param monkeypatch: pytest fixture used to point the CLI at that database + :returns: an iterator yielding the session factory + """ + # A file rather than an in-memory database, because the CLI opens its own + # connection to whatever FORMS_DATABASE_URL names, exactly as it would in + # real life. + url = f"sqlite:///{tmp_path / 'forms.db'}" + engine = create_engine_from_url(url) + create_all(engine) + + monkeypatch.setenv("FORMS_DATABASE_URL", url) + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + + with create_session_factory(engine)() as session: + session.add(models.Endpoint(id=ENDPOINT_ID, name="Contact form", created_at=NOW)) + session.commit() + + yield create_session_factory(engine) + engine.dispose() + + +def seed( + sessions: sessionmaker[Session], + *, + received_at: datetime, + delivery_state: DeliveryState | None = None, + attempts: int = 0, +) -> str: + """ + insert one submission, optionally owing a delivery in a chosen state + :param sessions: the session factory to insert through + :param received_at: the instant the submission claims it was accepted + :param delivery_state: the state of the delivery it owes, or None to owe none + :param attempts: how many requests have been made for that delivery + :returns: the submission identifier + """ + submission_id = f"sub_{uuid.uuid4().hex}" + delivery_id = f"whd_{uuid.uuid4().hex}" + terminal = delivery_state in (DeliveryState.DELIVERED, DeliveryState.FAILED) + with sessions() as session: + session.add( + models.Submission( + id=submission_id, + endpoint_id=ENDPOINT_ID, + received_at=received_at, + fields={"email": ["dev@example.com"]}, + ) + ) + if delivery_state is not None: + session.add( + models.WebhookDelivery( + id=delivery_id, + submission_id=submission_id, + endpoint_id=ENDPOINT_ID, + destination_url="https://example.invalid/hook", + signing_secret="whsec_" + "a" * 64, + state=delivery_state, + attempts=attempts, + cycle_attempts=attempts, + next_attempt_at=received_at, + created_at=received_at, + completed_at=received_at if terminal else None, + ) + ) + session.flush() + for number in range(1, attempts + 1): + session.add( + models.DeliveryAttempt( + id=f"att_{uuid.uuid4().hex}", + delivery_id=delivery_id, + submission_id=submission_id, + attempt_number=number, + destination_url="https://example.invalid/hook", + attempted_at=received_at, + outcome="http_error", + response_status=503, + ) + ) + session.commit() + return submission_id + + +def remaining(sessions: sessionmaker[Session]) -> set[str]: + """ + read which submissions are still stored + :param sessions: the session factory to query through + :returns: the identifiers still present + """ + with sessions() as session: + return set(session.scalars(select(models.Submission.id))) + + +def sweep(sessions: sessionmaker[Session], *, batch_size: int = 500) -> int: + """ + run a retention sweep against the fixed cutoff + :param sessions: the session factory to write through + :param batch_size: how many submissions to delete per transaction + :returns: how many submissions were deleted + """ + with sessions() as session: + return storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=batch_size, max_batches=100 + ) + + +# --- the policy -------------------------------------------------------------- + + +def test_an_unset_retention_keeps_everything_indefinitely() -> None: + assert build_settings().retention_policy().enabled is False + + +def test_a_disabled_policy_has_no_cutoff_to_offer() -> None: + with pytest.raises(RetentionDisabled): + RetentionPolicy(days=0).cutoff(NOW) + + +def test_a_cutoff_is_the_configured_age_before_now() -> None: + assert RetentionPolicy(days=30).cutoff(NOW) == NOW - timedelta(days=30) + + +def test_a_configured_retention_reaches_the_policy() -> None: + assert build_settings(submission_retention_days=7).retention_policy().days == 7 + + +# --- what is eligible -------------------------------------------------------- + + +def test_an_old_submission_with_no_delivery_is_deleted( + sessions: sessionmaker[Session], +) -> None: + old = seed(sessions, received_at=CUTOFF - timedelta(days=1)) + + assert sweep(sessions) == 1 + assert old not in remaining(sessions) + + +def test_a_recent_submission_is_kept(sessions: sessionmaker[Session]) -> None: + recent = seed(sessions, received_at=CUTOFF + timedelta(seconds=1)) + + assert sweep(sessions) == 0 + assert remaining(sessions) == {recent} + + +def test_a_submission_exactly_on_the_cutoff_is_kept(sessions: sessionmaker[Session]) -> None: + """ + the cutoff is exclusive, so a boundary row survives rather than being taken early + :param sessions: session factory on a migrated database + """ + boundary = seed(sessions, received_at=CUTOFF) + + assert sweep(sessions) == 0 + assert remaining(sessions) == {boundary} + + +def test_an_old_delivered_submission_is_deleted(sessions: sessionmaker[Session]) -> None: + """ + nothing will build a payload from a delivered submission again + :param sessions: session factory on a migrated database + """ + delivered = seed( + sessions, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + attempts=1, + ) + + assert sweep(sessions) == 1 + assert delivered not in remaining(sessions) + + +@pytest.mark.parametrize( + "state", + [DeliveryState.PENDING, DeliveryState.PROCESSING, DeliveryState.FAILED], +) +def test_a_submission_a_delivery_still_needs_is_kept( + sessions: sessionmaker[Session], state: DeliveryState +) -> None: + """ + the payload is built from the submission at send time, so anything deliverable keeps it + :param sessions: session factory on a migrated database + :param state: the delivery state that must protect its submission + """ + protected = seed(sessions, received_at=CUTOFF - timedelta(days=365), delivery_state=state) + + assert sweep(sessions) == 0 + assert remaining(sessions) == {protected} + + +def test_a_sweep_takes_only_what_is_eligible(sessions: sessionmaker[Session]) -> None: + delivered = seed( + sessions, received_at=CUTOFF - timedelta(days=1), delivery_state=DeliveryState.DELIVERED + ) + orphan = seed(sessions, received_at=CUTOFF - timedelta(days=1)) + pending = seed( + sessions, received_at=CUTOFF - timedelta(days=1), delivery_state=DeliveryState.PENDING + ) + recent = seed(sessions, received_at=NOW) + + assert sweep(sessions) == 2 + assert remaining(sessions) == {pending, recent} + assert delivered not in remaining(sessions) + assert orphan not in remaining(sessions) + + +# --- what a sweep must not destroy ------------------------------------------- + + +def test_a_deleted_submission_leaves_its_delivery_standing( + sessions: sessionmaker[Session], +) -> None: + """ + what this service did about a submission outlives the form content it carried + :param sessions: session factory on a migrated database + """ + seed( + sessions, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + attempts=3, + ) + + sweep(sessions) + + with sessions() as session: + delivery = session.scalars(select(models.WebhookDelivery)).one() + assert delivery.state == DeliveryState.DELIVERED + assert delivery.attempts == 3 + assert delivery.endpoint_id == ENDPOINT_ID + # Unlinked rather than removed, which is the whole point of the SET NULL. + assert delivery.submission_id is None + + +def test_a_deleted_submission_leaves_its_attempt_history_standing( + sessions: sessionmaker[Session], +) -> None: + seed( + sessions, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + attempts=3, + ) + + sweep(sessions) + + with sessions() as session: + attempts = list(session.scalars(select(models.DeliveryAttempt))) + assert [attempt.attempt_number for attempt in attempts] == [1, 2, 3] + assert all(attempt.submission_id is None for attempt in attempts) + assert all(attempt.delivery_id is not None for attempt in attempts) + + +def test_an_unlinked_delivery_is_never_claimed_by_a_worker( + sessions: sessionmaker[Session], +) -> None: + """ + a delivery with nothing to send must not be picked up and retried forever + :param sessions: session factory on a migrated database + """ + seed( + sessions, + received_at=CUTOFF - timedelta(days=1), + delivery_state=DeliveryState.DELIVERED, + ) + sweep(sessions) + with sessions() as session: + # Forced back into the queue behind the sweep's back, which is the only + # way this state could ever arise. + session.execute( + update(models.WebhookDelivery).values( + state=DeliveryState.PENDING, completed_at=None, next_attempt_at=CUTOFF + ) + ) + session.commit() + + with sessions() as session: + claimed = storage.claim_due_deliveries(session, now=NOW, lease_seconds=60, limit=10) + + assert claimed == [] + + +def test_a_failed_delivery_keeps_what_a_replay_would_need( + sessions: sessionmaker[Session], +) -> None: + """ + a replay rebuilds the payload from the submission, so retention must not take it + :param sessions: session factory on a migrated database + """ + seed( + sessions, + received_at=CUTOFF - timedelta(days=365), + delivery_state=DeliveryState.FAILED, + attempts=5, + ) + sweep(sessions) + + with sessions() as session: + delivery = session.scalars(select(models.WebhookDelivery)).one() + outcome = storage.requeue_failed_delivery(session, delivery.id, now=NOW) + claimed = storage.claim_due_deliveries(session, now=NOW, lease_seconds=60, limit=10) + payloads = storage.load_submissions(session, claimed) + + assert outcome.requeued is True + assert [job.id for job in claimed] == [delivery.id] + assert payloads[delivery.id].fields == {"email": ("dev@example.com",)} + + +# --- batching ---------------------------------------------------------------- + + +def test_a_sweep_deletes_everything_eligible_across_batches( + sessions: sessionmaker[Session], +) -> None: + for index in range(7): + seed(sessions, received_at=CUTOFF - timedelta(days=1 + index)) + + assert sweep(sessions, batch_size=2) == 7 + assert remaining(sessions) == set() + + +def test_a_sweep_stops_at_its_batch_ceiling(sessions: sessionmaker[Session]) -> None: + """ + a run against an enormous backlog has to come back rather than go on forever + :param sessions: session factory on a migrated database + """ + for index in range(5): + seed(sessions, received_at=CUTOFF - timedelta(days=1 + index)) + + with sessions() as session: + removed = storage.delete_expired_submissions( + session, before=CUTOFF, batch_size=2, max_batches=1 + ) + + assert removed == 2 + assert len(remaining(sessions)) == 3 + + +def test_counting_and_deleting_agree(sessions: sessionmaker[Session]) -> None: + for index in range(4): + seed(sessions, received_at=CUTOFF - timedelta(days=1 + index)) + seed(sessions, received_at=NOW, delivery_state=DeliveryState.PENDING) + + with sessions() as session: + counted = storage.count_expired_submissions(session, before=CUTOFF) + + assert counted == 4 + assert sweep(sessions) == 4 + + +# --- the operator command ---------------------------------------------------- + + +def test_cleanup_refuses_to_run_with_no_retention_configured( + sessions: sessionmaker[Session], capsys: pytest.CaptureFixture[str] +) -> None: + seed(sessions, received_at=CUTOFF - timedelta(days=1)) + + assert cli.main(["cleanup-submissions"]) == 1 + + captured = capsys.readouterr() + assert "No submission retention is configured" in captured.err + assert captured.out == "" + assert len(remaining(sessions)) == 1 + + +def test_cleanup_deletes_what_the_configured_retention_releases( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + old = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=31)) + recent = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=1)) + + assert cli.main(["cleanup-submissions"]) == 0 + + assert "Deleted 1 submission(s)" in capsys.readouterr().out + assert remaining(sessions) == {recent} + assert old not in remaining(sessions) + + +def test_cleanup_reports_the_cutoff_it_worked_out( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + + cli.main(["cleanup-submissions", "--dry-run"]) + + output = capsys.readouterr().out + assert "keeps submissions for 30 days" in output + assert "eligible for deletion" in output + + +def test_a_dry_run_deletes_nothing( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + old = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=31)) + + assert cli.main(["cleanup-submissions", "--dry-run"]) == 0 + + output = capsys.readouterr().out + assert "1 submission(s)" in output + assert "Dry run: nothing was deleted." in output + assert remaining(sessions) == {old} + + +def test_an_explicit_age_sweeps_without_any_configuration( + sessions: sessionmaker[Session], capsys: pytest.CaptureFixture[str] +) -> None: + """ + an operator who keeps everything must still be able to clear one old range by hand + :param sessions: session factory on a migrated database + :param capsys: pytest fixture capturing what the command printed + """ + old = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=400)) + recent = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=10)) + + assert cli.main(["cleanup-submissions", "--older-than-days", "365"]) == 0 + + assert remaining(sessions) == {recent} + assert old not in remaining(sessions) + assert "Deleted 1 submission(s)" in capsys.readouterr().out + + +def test_an_explicit_age_overrides_the_configured_one( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "1") + kept = seed(sessions, received_at=datetime.now(UTC) - timedelta(days=10)) + + assert cli.main(["cleanup-submissions", "--older-than-days", "365"]) == 0 + + assert remaining(sessions) == {kept} + + +def test_cleanup_says_so_when_there_is_nothing_to_do( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + seed(sessions, received_at=datetime.now(UTC)) + + assert cli.main(["cleanup-submissions"]) == 0 + + assert "Nothing to delete." in capsys.readouterr().out + + +def test_cleanup_honours_a_batch_size( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + for index in range(5): + seed(sessions, received_at=datetime.now(UTC) - timedelta(days=31 + index)) + + assert cli.main(["cleanup-submissions", "--batch-size", "2"]) == 0 + + assert "Deleted 5 submission(s) in batches of up to 2." in capsys.readouterr().out + assert remaining(sessions) == set() + + +def test_a_useless_batch_size_is_refused( + sessions: sessionmaker[Session], + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + + assert cli.main(["cleanup-submissions", "--batch-size", "0"]) == 1 + + assert "--batch-size must be at least 1." in capsys.readouterr().err + + +def test_cleanup_never_reports_the_database_password( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str] +) -> None: + """ + a command that deletes data must not put credentials on an operator's terminal + :param tmp_path: pytest fixture giving this test a directory of its own + :param monkeypatch: pytest fixture used to point the CLI at an unusable database + :param capsys: pytest fixture capturing what the command printed + """ + monkeypatch.setenv("FORMS_DATABASE_URL", f"sqlite:///{tmp_path / 'missing' / 'forms.db'}") + monkeypatch.setenv("FORMS_SUBMISSION_RETENTION_DAYS", "30") + monkeypatch.setattr(cli, "Settings", IsolatedSettings) + + assert cli.main(["cleanup-submissions"]) == 1 + + captured = capsys.readouterr() + assert "The database could not be used" in captured.err + assert captured.out == "" diff --git a/tests/test_submission_export.py b/tests/test_submission_export.py new file mode 100644 index 0000000..3097938 --- /dev/null +++ b/tests/test_submission_export.py @@ -0,0 +1,428 @@ +""" +exporting stored submissions as JSON and as CSV + +The CSV is where the awkwardness lives: different submissions carry different +field names, a field name can be repeated, and both names and values are written +by whoever filled the form in. Most of what is asserted here is about that. +""" + +from __future__ import annotations + +import csv +import io +import json +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +from conftest import URLENCODED_HEADERS, ClientFactory, create_endpoint, seed_submission +from hymical_forms import export + +EXPORT = "/submissions/export" +ENDPOINT = "/f/contact-form" + +NOON = datetime(2026, 8, 25, 12, 0, tzinfo=UTC) + + +def exported_json(client: TestClient, **params: Any) -> list[dict[str, Any]]: + """ + read a JSON export and unwrap it + :param client: the client to export through + :param params: query parameters to send + :returns: the exported submissions + """ + response = client.get(EXPORT, params=params) + assert response.status_code == 200, response.text + return list(json.loads(response.text)["submissions"]) + + +def exported_rows(client: TestClient, **params: Any) -> list[list[str]]: + """ + read a CSV export and parse it back into rows + :param client: the client to export through + :param params: query parameters to send + :returns: every row including the header, in order + """ + response = client.get(EXPORT, params={"format": "csv", **params}) + assert response.status_code == 200, response.text + return list(csv.reader(io.StringIO(response.text))) + + +# --- authentication ---------------------------------------------------------- + + +@pytest.mark.parametrize("params", [{}, {"format": "csv"}]) +def test_an_export_requires_a_management_key( + make_client: ClientFactory, params: dict[str, str] +) -> None: + client = make_client(authenticate=False) + + response = client.get(EXPORT, params=params) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "authentication_required" + + +# --- shared behaviour -------------------------------------------------------- + + +def test_an_unknown_format_is_refused(client: TestClient) -> None: + response = client.get(EXPORT, params={"format": "xlsx"}) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "unsupported_export_format" + + +def test_an_impossible_time_range_is_refused(client: TestClient) -> None: + response = client.get( + EXPORT, + params={ + "received_after": NOON.isoformat(), + "received_before": (NOON - timedelta(days=1)).isoformat(), + }, + ) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "invalid_time_range" + + +@pytest.mark.parametrize( + ("params", "suffix"), + [({}, "json"), ({"format": "csv"}, "csv")], +) +def test_an_export_is_offered_as_a_download( + client: TestClient, params: dict[str, str], suffix: str +) -> None: + seed_submission(client, received_at=NOON) + + disposition = client.get(EXPORT, params=params).headers["content-disposition"] + + assert disposition.startswith("attachment; ") + assert disposition.endswith(f'.{suffix}"') + assert "hymical-submissions-" in disposition + + +def test_an_export_filename_carries_nothing_a_caller_supplied( + make_client: ClientFactory, +) -> None: + """ + a filename built from user input is a header injection waiting to happen + :param make_client: factory for clients bound to a configured app + """ + client = make_client() + create_endpoint(client, "quote-request", name="Quotes") + seed_submission(client, received_at=NOON, endpoint_id="quote-request") + + disposition = client.get(EXPORT, params={"endpoint_id": "quote-request"}).headers[ + "content-disposition" + ] + + assert "quote-request" not in disposition + + +def test_the_export_filters_match_the_listing(make_client: ClientFactory) -> None: + client = make_client() + create_endpoint(client, "waitlist", name="Waitlist") + wanted = seed_submission(client, received_at=NOON, endpoint_id="waitlist") + seed_submission(client, received_at=NOON, endpoint_id="contact-form") + seed_submission(client, received_at=NOON - timedelta(days=2), endpoint_id="waitlist") + + exported = exported_json( + client, + endpoint_id="waitlist", + received_after=(NOON - timedelta(days=1)).isoformat(), + ) + + assert [item["id"] for item in exported] == [wanted] + + +@pytest.mark.parametrize("params", [{}, {"format": "csv"}]) +def test_an_export_larger_than_the_maximum_is_refused( + make_client: ClientFactory, params: dict[str, str] +) -> None: + """ + a truncated export is worse than an error, because nobody notices it + :param make_client: factory for clients bound to a configured app + :param params: the format to ask for + """ + client = make_client(export_max_submissions=2) + for index in range(3): + seed_submission(client, received_at=NOON + timedelta(seconds=index)) + + response = client.get(EXPORT, params=params) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "export_too_large" + assert response.json()["error"]["details"]["limit"] == 2 + + +def test_an_export_exactly_at_the_maximum_is_allowed(make_client: ClientFactory) -> None: + client = make_client(export_max_submissions=2) + for index in range(2): + seed_submission(client, received_at=NOON + timedelta(seconds=index)) + + assert len(exported_json(client)) == 2 + + +def test_a_narrowed_filter_brings_an_export_back_under_the_maximum( + make_client: ClientFactory, +) -> None: + client = make_client(export_max_submissions=2) + for index in range(3): + seed_submission(client, received_at=NOON + timedelta(days=index)) + + exported = exported_json(client, received_after=(NOON + timedelta(hours=12)).isoformat()) + + assert len(exported) == 2 + + +# --- json -------------------------------------------------------------------- + + +def test_a_json_export_is_json(client: TestClient) -> None: + seed_submission(client, received_at=NOON) + + response = client.get(EXPORT) + + assert response.headers["content-type"].startswith("application/json") + + +def test_a_json_export_carries_the_submitted_values(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"email": ["dev@example.com"]}) + + assert exported_json(client)[0]["fields"] == {"email": ["dev@example.com"]} + + +def test_a_json_export_preserves_repeated_values(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"topics": ["billing", "api", "billing"]}) + + assert exported_json(client)[0]["fields"] == {"topics": ["billing", "api", "billing"]} + + +def test_a_json_export_is_newest_first(client: TestClient) -> None: + older = seed_submission(client, received_at=NOON - timedelta(hours=1)) + newer = seed_submission(client, received_at=NOON) + + assert [item["id"] for item in exported_json(client)] == [newer, older] + + +def test_a_json_export_carries_no_internal_columns(client: TestClient) -> None: + client.post( + ENDPOINT, + content=b"email=dev%40example.com", + headers=URLENCODED_HEADERS | {"Idempotency-Key": "b8f1c2d4e5a67890b8f1c2d4e5a67890"}, + ) + + exported = exported_json(client)[0] + + assert set(exported) == {"id", "endpoint_id", "received_at", "fields"} + + +def test_an_empty_json_export_is_still_a_document(client: TestClient) -> None: + response = client.get(EXPORT) + + assert response.status_code == 200 + assert json.loads(response.text) == {"submissions": []} + + +# --- csv --------------------------------------------------------------------- + + +def test_a_csv_export_is_csv(client: TestClient) -> None: + seed_submission(client, received_at=NOON) + + response = client.get(EXPORT, params={"format": "csv"}) + + assert response.headers["content-type"].startswith("text/csv") + + +def test_a_csv_export_starts_with_the_metadata_columns(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"email": ["dev@example.com"]}) + + header = exported_rows(client)[0] + + assert header[:3] == ["submission_id", "endpoint_id", "received_at"] + assert header[3:] == ["email"] + + +def test_a_csv_header_is_the_union_of_every_field_name(client: TestClient) -> None: + seed_submission(client, received_at=NOON - timedelta(hours=1), fields={"email": ["a@b.c"]}) + seed_submission(client, received_at=NOON, fields={"email": ["d@e.f"], "phone": ["123"]}) + + header = exported_rows(client)[0] + + assert set(header[3:]) == {"email", "phone"} + + +def test_a_missing_field_becomes_an_empty_cell(client: TestClient) -> None: + """ + an absent field and a field submitted empty are different things + :param client: test client whose app already holds the default endpoint + """ + seed_submission(client, received_at=NOON - timedelta(hours=1), fields={"phone": [""]}) + seed_submission(client, received_at=NOON, fields={"email": ["d@e.f"]}) + + header, newer, older = exported_rows(client) + phone = header.index("phone") + + assert newer[phone] == "" + assert older[phone] == '[""]' + + +def test_repeated_values_are_one_cell_holding_an_array(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"topics": ["api", "billing"]}) + + header, row = exported_rows(client) + + assert row[header.index("topics")] == '["api","billing"]' + + +def test_a_single_value_is_still_an_array(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"email": ["dev@example.com"]}) + + header, row = exported_rows(client) + + assert row[header.index("email")] == '["dev@example.com"]' + + +@pytest.mark.parametrize( + "value", + [ + "one, two", + 'she said "hello"', + "first line\nsecond line", + "tab\tseparated", + "back\\slash", + ], +) +def test_awkward_characters_survive_a_round_trip(client: TestClient, value: str) -> None: + """ + a comma, a quote or a newline in an answer must come back out as it went in + :param client: test client whose app already holds the default endpoint + :param value: the submitted value to round trip + """ + seed_submission(client, received_at=NOON, fields={"message": [value]}) + + header, row = exported_rows(client) + + assert json.loads(row[header.index("message")]) == [value] + + +def test_unicode_survives_a_round_trip(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"naam": ["Zoë", "日本語", "🙂"]}) + + header, row = exported_rows(client) + + assert json.loads(row[header.index("naam")]) == ["Zoë", "日本語", "🙂"] + + +def test_a_csv_export_is_utf8(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"naam": ["Zoë"]}) + + response = client.get(EXPORT, params={"format": "csv"}) + + assert "charset=utf-8" in response.headers["content-type"] + assert "Zoë".encode() in response.content + + +def test_an_empty_csv_export_is_an_empty_document(client: TestClient) -> None: + rows = exported_rows(client) + + assert rows == [["submission_id", "endpoint_id", "received_at"]] + + +# --- formula injection ------------------------------------------------------- + + +def test_a_value_that_looks_like_a_formula_is_never_at_the_start_of_a_cell( + client: TestClient, +) -> None: + """ + the array encoding is what makes a value cell start with a bracket rather than an equals + :param client: test client whose app already holds the default endpoint + """ + seed_submission(client, received_at=NOON, fields={"message": ["=1+1"]}) + + header, row = exported_rows(client) + cell = row[header.index("message")] + + assert cell.startswith("[") + # And the value itself is still there to be read, unaltered. + assert json.loads(cell) == ["=1+1"] + + +@pytest.mark.parametrize("name", ["=cmd()", "+1", "-1", "@SUM(A1)"]) +def test_a_field_name_that_looks_like_a_formula_is_marked_as_text( + client: TestClient, name: str +) -> None: + """ + a field name becomes a header cell, and a form is free to call a field anything + :param client: test client whose app already holds the default endpoint + :param name: the submitted field name to export + """ + seed_submission(client, received_at=NOON, fields={name: ["value"]}) + + header = exported_rows(client)[0] + + assert header[3] == f"'{name}" + + +def test_an_ordinary_field_name_is_left_alone(client: TestClient) -> None: + seed_submission(client, received_at=NOON, fields={"email": ["dev@example.com"]}) + + assert exported_rows(client)[0][3] == "email" + + +@pytest.mark.parametrize("value", ["=1+1", "+1", "-1", "@x", "\tx", "\rx"]) +def test_the_escaping_rule_marks_every_formula_leader(value: str) -> None: + marked = export._safe(value) + + assert marked.startswith("'") + assert marked.removeprefix("'") == value + + +@pytest.mark.parametrize("value", ["email", "[1]", "1+1", "", "x=1"]) +def test_the_escaping_rule_leaves_anything_else_alone(value: str) -> None: + assert export._safe(value) == value + + +# --- logging ----------------------------------------------------------------- + + +@pytest.mark.parametrize("params", [{}, {"format": "csv"}]) +def test_an_export_logs_who_and_how_much_but_never_what( + client: TestClient, caplog: pytest.LogCaptureFixture, params: dict[str, str] +) -> None: + """ + an export moves form content on purpose, so duplicating it into the log undoes that + :param client: test client whose app already holds the default endpoint + :param caplog: pytest fixture capturing what was logged + :param params: the format to ask for + """ + seed_submission( + client, received_at=NOON, fields={"confession": ["something private"], "topics": ["api"]} + ) + + with caplog.at_level("DEBUG"): + assert client.get(EXPORT, params=params).status_code == 200 + + logged = caplog.text + assert "confession" not in logged + assert "something private" not in logged + assert "1 submission(s) exported" in logged + + +def test_reading_one_submission_back_logs_nothing_about_it( + client: TestClient, caplog: pytest.LogCaptureFixture +) -> None: + submission_id = seed_submission( + client, received_at=NOON, fields={"confession": ["something private"]} + ) + + with caplog.at_level("DEBUG"): + assert client.get(f"/submissions/{submission_id}").status_code == 200 + + assert "confession" not in caplog.text + assert "something private" not in caplog.text diff --git a/tests/test_submissions_api.py b/tests/test_submissions_api.py new file mode 100644 index 0000000..70734e2 --- /dev/null +++ b/tests/test_submissions_api.py @@ -0,0 +1,394 @@ +""" +the authenticated submission listing and detail routes + +These are the first routes that can return what somebody typed into a form, so +what they hand back and what they refuse to hand back are both worth asserting +rather than assuming. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +from conftest import ( + URLENCODED_HEADERS, + ClientFactory, + create_endpoint, + seed_submission, +) +from hymical_forms.webhooks import DeliveryState + +ENDPOINT = "/f/contact-form" +LISTING = "/submissions" + +NOON = datetime(2026, 8, 25, 12, 0, tzinfo=UTC) + + +def submit(client: TestClient, body: bytes = b"email=dev%40example.com") -> str: + """ + send one submission through the public route + :param client: the client to submit through + :param body: the urlencoded body to send + :returns: the identifier the API generated for it + """ + response = client.post(ENDPOINT, content=body, headers=URLENCODED_HEADERS) + assert response.status_code == 202, response.text + return str(response.json()["submission_id"]) + + +def listed(client: TestClient, **params: Any) -> list[dict[str, Any]]: + """ + read one page of the submission listing + :param client: the client to read through + :param params: query parameters to send + :returns: the items on the page + """ + response = client.get(LISTING, params=params) + assert response.status_code == 200, response.text + return list(response.json()["items"]) + + +# --- authentication ---------------------------------------------------------- + + +@pytest.mark.parametrize("path", [LISTING, "/submissions/sub_whatever"]) +def test_a_submission_route_requires_a_management_key( + make_client: ClientFactory, path: str +) -> None: + client = make_client(authenticate=False) + + response = client.get(path) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "authentication_required" + + +def test_a_submission_route_refuses_a_bad_key(client: TestClient) -> None: + response = client.get(LISTING, headers={"Authorization": "Bearer hym_live_nonsense"}) + + assert response.status_code == 401 + assert response.json()["error"]["code"] == "invalid_api_key" + + +# --- listing ----------------------------------------------------------------- + + +def test_an_empty_service_lists_nothing(client: TestClient) -> None: + body = client.get(LISTING).json() + + assert body["items"] == [] + assert body["next_cursor"] is None + + +def test_submissions_are_listed_newest_first(client: TestClient) -> None: + first = submit(client) + second = submit(client) + third = submit(client) + + assert [item["id"] for item in listed(client)] == [third, second, first] + + +def test_a_summary_reports_the_submission_metadata(client: TestClient) -> None: + submission_id = submit(client, b"email=dev%40example.com&topics=api&topics=billing") + + item = listed(client)[0] + + assert item["id"] == submission_id + assert item["endpoint_id"] == "contact-form" + assert item["field_count"] == 3 + assert item["idempotent"] is False + assert datetime.fromisoformat(item["received_at"]).tzinfo is not None + + +def test_a_summary_never_carries_the_submitted_values(client: TestClient) -> None: + """ + a listing is metadata, so paging a busy endpoint must not spread form content around + :param client: test client whose app already holds the default endpoint + """ + submit(client, b"secret=hunter2") + + item = listed(client)[0] + + assert "fields" not in item + assert "hunter2" not in str(item) + + +def test_a_summary_reports_that_a_key_was_used_without_reporting_the_key( + client: TestClient, +) -> None: + key = "b8f1c2d4e5a67890b8f1c2d4e5a67890" + client.post( + ENDPOINT, + content=b"email=dev%40example.com", + headers=URLENCODED_HEADERS | {"Idempotency-Key": key}, + ) + + item = listed(client)[0] + + assert item["idempotent"] is True + assert key not in str(item) + + +def test_a_summary_carries_no_payload_fingerprint(client: TestClient) -> None: + client.post( + ENDPOINT, + content=b"email=dev%40example.com", + headers=URLENCODED_HEADERS | {"Idempotency-Key": "b8f1c2d4e5a67890b8f1c2d4e5a67890"}, + ) + + assert "payload_fingerprint" not in str(listed(client)[0]) + + +def test_a_submission_with_no_webhook_reports_no_delivery(client: TestClient) -> None: + submit(client) + + assert listed(client)[0]["delivery"] is None + + +def test_a_submission_with_a_webhook_reports_its_delivery( + make_client: ClientFactory, webhook: Any +) -> None: + client = make_client(seed_endpoint=False, allow_private_webhook_targets=True) + create_endpoint(client, webhook_url=webhook.url) + submit(client) + + delivery = listed(client)[0]["delivery"] + + assert delivery["state"] == DeliveryState.PENDING + assert delivery["attempt_count"] == 0 + assert delivery["id"].startswith("whd_") + + +# --- pagination -------------------------------------------------------------- + + +def test_a_full_page_hands_back_a_cursor(client: TestClient) -> None: + for _ in range(3): + submit(client) + + body = client.get(LISTING, params={"limit": 2}).json() + + assert len(body["items"]) == 2 + assert body["next_cursor"] == body["items"][-1]["id"] + + +def test_a_cursor_continues_where_the_page_stopped(client: TestClient) -> None: + identifiers = [submit(client) for _ in range(5)] + + first = client.get(LISTING, params={"limit": 2}).json() + second = client.get(LISTING, params={"limit": 2, "cursor": first["next_cursor"]}).json() + + walked = [item["id"] for item in first["items"] + second["items"]] + assert walked == list(reversed(identifiers))[:4] + + +def test_a_short_page_hands_back_no_cursor(client: TestClient) -> None: + submit(client) + + assert client.get(LISTING, params={"limit": 50}).json()["next_cursor"] is None + + +def test_an_unknown_cursor_is_refused(client: TestClient) -> None: + response = client.get(LISTING, params={"cursor": "sub_does_not_exist"}) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "invalid_cursor" + + +def test_a_page_size_above_the_ceiling_is_refused(client: TestClient) -> None: + assert client.get(LISTING, params={"limit": 101}).status_code == 422 + + +# --- filters ----------------------------------------------------------------- + + +def test_the_endpoint_filter_narrows_the_page(make_client: ClientFactory) -> None: + client = make_client() + create_endpoint(client, "waitlist", name="Waitlist") + mine = seed_submission(client, received_at=NOON, endpoint_id="waitlist") + seed_submission(client, received_at=NOON, endpoint_id="contact-form") + + assert [item["id"] for item in listed(client, endpoint_id="waitlist")] == [mine] + + +def test_an_endpoint_with_nothing_lists_nothing(make_client: ClientFactory) -> None: + client = make_client() + create_endpoint(client, "waitlist", name="Waitlist") + seed_submission(client, received_at=NOON) + + assert listed(client, endpoint_id="waitlist") == [] + + +def test_received_after_is_strictly_exclusive(client: TestClient) -> None: + """ + a bound that included its own instant would hand back the row it was taken from + :param client: test client whose app already holds the default endpoint + """ + on_the_bound = seed_submission(client, received_at=NOON) + newer = seed_submission(client, received_at=NOON + timedelta(seconds=1)) + + identifiers = [item["id"] for item in listed(client, received_after=NOON.isoformat())] + + assert identifiers == [newer] + assert on_the_bound not in identifiers + + +def test_received_before_is_strictly_exclusive(client: TestClient) -> None: + older = seed_submission(client, received_at=NOON - timedelta(seconds=1)) + on_the_bound = seed_submission(client, received_at=NOON) + + identifiers = [item["id"] for item in listed(client, received_before=NOON.isoformat())] + + assert identifiers == [older] + assert on_the_bound not in identifiers + + +def test_both_bounds_together_select_the_window(client: TestClient) -> None: + seed_submission(client, received_at=NOON - timedelta(hours=2)) + inside = seed_submission(client, received_at=NOON) + seed_submission(client, received_at=NOON + timedelta(hours=2)) + + identifiers = [ + item["id"] + for item in listed( + client, + received_after=(NOON - timedelta(hours=1)).isoformat(), + received_before=(NOON + timedelta(hours=1)).isoformat(), + ) + ] + + assert identifiers == [inside] + + +def test_the_endpoint_and_time_filters_combine(make_client: ClientFactory) -> None: + client = make_client() + create_endpoint(client, "waitlist", name="Waitlist") + wanted = seed_submission(client, received_at=NOON, endpoint_id="waitlist") + seed_submission(client, received_at=NOON, endpoint_id="contact-form") + seed_submission(client, received_at=NOON - timedelta(days=1), endpoint_id="waitlist") + + identifiers = [ + item["id"] + for item in listed( + client, + endpoint_id="waitlist", + received_after=(NOON - timedelta(hours=1)).isoformat(), + ) + ] + + assert identifiers == [wanted] + + +@pytest.mark.parametrize("offset", [0, 1]) +def test_an_impossible_time_range_is_refused(client: TestClient, offset: int) -> None: + response = client.get( + LISTING, + params={ + "received_after": NOON.isoformat(), + "received_before": (NOON - timedelta(seconds=offset)).isoformat(), + }, + ) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "invalid_time_range" + + +def test_an_unparseable_timestamp_is_refused(client: TestClient) -> None: + response = client.get(LISTING, params={"received_after": "yesterday"}) + + assert response.status_code == 422 + assert response.json()["error"]["code"] == "invalid_request" + + +# --- detail ------------------------------------------------------------------ + + +def test_a_known_submission_can_be_read_back(client: TestClient) -> None: + submission_id = submit(client, b"email=dev%40example.com&message=hello") + + body = client.get(f"/submissions/{submission_id}").json() + + assert body["id"] == submission_id + assert body["endpoint_id"] == "contact-form" + assert body["field_count"] == 2 + assert body["fields"] == {"email": ["dev@example.com"], "message": ["hello"]} + + +def test_an_unknown_submission_is_a_stable_404(client: TestClient) -> None: + response = client.get("/submissions/sub_does_not_exist") + + assert response.status_code == 404 + assert response.json()["error"]["code"] == "submission_not_found" + + +def test_repeated_values_survive_being_read_back(client: TestClient) -> None: + """ + a checkbox group is what repeated field names are for, so the order has to hold + :param client: test client whose app already holds the default endpoint + """ + submission_id = submit(client, b"topics=billing&topics=api&topics=billing") + + fields = client.get(f"/submissions/{submission_id}").json()["fields"] + + assert fields == {"topics": ["billing", "api", "billing"]} + + +def test_a_single_value_stays_a_list(client: TestClient) -> None: + submission_id = submit(client, b"email=dev%40example.com") + + fields = client.get(f"/submissions/{submission_id}").json()["fields"] + + assert fields == {"email": ["dev@example.com"]} + + +def test_detail_carries_no_internal_columns(client: TestClient) -> None: + key = "b8f1c2d4e5a67890b8f1c2d4e5a67890" + response = client.post( + ENDPOINT, + content=b"email=dev%40example.com", + headers=URLENCODED_HEADERS | {"Idempotency-Key": key}, + ) + submission_id = response.json()["submission_id"] + + body = client.get(f"/submissions/{submission_id}").json() + + assert set(body) == { + "id", + "endpoint_id", + "received_at", + "field_count", + "idempotent", + "delivery", + "fields", + } + assert key not in str(body) + + +def test_detail_carries_no_signing_secret(make_client: ClientFactory, webhook: Any) -> None: + client = make_client(seed_endpoint=False, allow_private_webhook_targets=True) + created = create_endpoint(client, webhook_url=webhook.url) + submission_id = submit(client) + + body = client.get(f"/submissions/{submission_id}").json() + + assert created["webhook_secret"] not in str(body) + assert "signing_secret" not in str(body) + + +def test_detail_reports_a_delivery_that_has_been_made( + make_client: ClientFactory, webhook: Any +) -> None: + client = make_client(seed_endpoint=False, allow_private_webhook_targets=True) + create_endpoint(client, webhook_url=webhook.url) + submission_id = seed_submission( + client, received_at=NOON, delivery_state=DeliveryState.DELIVERED, attempts=2 + ) + + delivery = client.get(f"/submissions/{submission_id}").json()["delivery"] + + assert delivery["state"] == DeliveryState.DELIVERED + assert delivery["attempt_count"] == 2