[US-15.1 / OBT-250] Public creation requests for languages and projects - #102
[US-15.1 / OBT-250] Public creation requests for languages and projects#102levigtri wants to merge 12 commits into
Conversation
Stores pending requests to create a language or project submitted by visitors without an account. Mirrors the access_requests review shape (status, reviewed_by, reviewed_at, review_reason) but carries requester contact columns (name, email) instead of a user FK, since there is no authenticated user. Includes the Alembic migration.
create_language_request rejects codes that already exist in languages or in another pending public request (409). create_project_request requires an existing language (404). verify_recaptcha validates the token against Google siteverify using RECAPTCHA_SECRET_KEY; an empty secret disables verification for dev and tests.
GET /api/public/languages returns a slim list (id, name, code) for the public project form combobox. POST /api/public/requests/language and /requests/project store pending creation requests after reCAPTCHA verification. Router registered without auth dependencies; service tests cover happy paths, duplicate code conflicts, unknown language and captcha outcomes.
Public POSTs are limited to 5/minute and 20/hour per IP, the languages listing to 30/minute, using the existing slowapi limiter. The recaptcha_token becomes optional: when RECAPTCHA_SECRET_KEY is unset the app runs with rate limiting only; when set, a missing or invalid token is rejected with 400. Lets the feature ship before the reCAPTCHA keys are provisioned.
The public router imports public_request_service from app.services; without this registration the app fails to import.
A public project request now accepts either an existing language_id or
a new_language_name + new_language_code pair, mirroring the
authenticated flow. Language uniqueness is enforced on BOTH name and
code, case-insensitively, against the languages table and every
pending public request (including new languages proposed inside
project requests) — 'English/en' and 'english/EN' are the same
identifier.
Hardening from review: language codes are restricted to ASCII letters
(^[A-Za-z]{3}$) so lowercasing can never overflow the VARCHAR(3)
column, and httpx transport failures during reCAPTCHA verification
surface as 400 instead of unhandled 500s. The public_requests
migration (still unmerged/unreleased) gains the two new columns.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesPublic Request Workflows
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicAPI
participant PublicRequestService
participant GoogleSiteverify
participant Database
Client->>PublicAPI: Submit language or project request
PublicAPI->>PublicRequestService: Verify reCAPTCHA token
PublicRequestService->>GoogleSiteverify: POST secret and token
GoogleSiteverify-->>PublicRequestService: Verification result
PublicAPI->>PublicRequestService: Create public request
PublicRequestService->>Database: Check conflicts and persist PublicRequest
Database-->>PublicRequestService: Saved request
PublicRequestService-->>PublicAPI: PublicRequestResponse
PublicAPI-->>Client: Request response
sequenceDiagram
participant Admin
participant AdminAPI
participant ReviewService
participant Database
Admin->>AdminAPI: Submit request review
AdminAPI->>ReviewService: Review PublicRequest
ReviewService->>Database: Create requested entity when approved
ReviewService->>Database: Save review metadata
Database-->>ReviewService: Reviewed PublicRequest
ReviewService-->>AdminAPI: PublicRequestAdminResponse
AdminAPI-->>Admin: Review result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (4)
app/services/public_request/ensure_language_available.py-13-19 (1)
13-19: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winNormalize language codes before comparing The current writers lowercase these fields, but the schema/migrations don’t enforce that invariant, and
20260226_0003_languages_table.pycopiesprojects.codeverbatim. CompareLanguage.codeand the pending-request codes withfunc.lower(...), or add a DB-level lowercase constraint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/ensure_language_available.py` around lines 13 - 19, Update the language lookup around existing_stmt to normalize codes during comparison: apply func.lower to Language.code and to the lowered_code value, and make the corresponding comparison for pending-request codes case-insensitive. Preserve the existing language-name matching behavior.app/services/public_request/create_project_request.py-26-42 (1)
26-42: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMake language-request de-duplication atomic
ensure_language_available()only does a pre-insert read; without a unique constraint or transaction-level lock, two concurrent requests can still create duplicate pending requests for the same normalized name/code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/create_project_request.py` around lines 26 - 42, Make de-duplication atomic in the create-project flow around ensure_language_available() and PublicRequest creation: enforce a database-level uniqueness rule for pending language requests using the normalized name/code, then handle concurrent insert conflicts by rolling back and returning the existing validation behavior. Do not rely on the pre-insert read alone.app/models/public_request.py-24-32 (1)
24-32: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject mixed project-language input.
PublicProjectRequestCreateaccepts bothlanguage_idandnew_language_*, andcreate_project_request()silently preferslanguage_id, dropping the proposed language data. Make the two modes mutually exclusive in the model, and keep the service-side guard for direct callers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/public_request.py` around lines 24 - 32, Make PublicProjectRequestCreate reject requests that provide language_id together with either new_language_name or new_language_code, using model-level validation while preserving valid existing-language and new-language modes. Also retain the service-side guard in create_project_request() for direct callers, updating both app/models/public_request.py:24-32 and app/services/public_request/create_project_request.py:20-28.app/services/public_request/verify_recaptcha.py-9-31 (1)
9-31: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat malformed siteverify replies as validation failures.
A malformed JSON body or a non-object 2xx reply can still raise
JSONDecodeError,UnicodeDecodeError, orAttributeErrorhere; the current handler only catcheshttpx.HTTPError, so this can surface as a 500 instead ofValidationError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/verify_recaptcha.py` around lines 9 - 31, Update _siteverify and verify_recaptcha so malformed siteverify responses—including JSONDecodeError, UnicodeDecodeError, or non-object payloads causing AttributeError—are converted into ValidationError rather than escaping as server errors. Preserve the existing unavailable-message handling for HTTP failures and the failed-verification behavior for valid responses with success false.
🧹 Nitpick comments (2)
tests/test_public_request_service.py (1)
196-218: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover reCAPTCHA transport failures.
Add a test where
_siteverifyraiseshttpx.HTTPErrorand assert thatverify_recaptcharaisesValidationErrorwith the unavailable/retry message. This behavior is explicitly part of the PR contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_public_request_service.py` around lines 196 - 218, Extend the reCAPTCHA verification tests alongside test_verify_recaptcha_rejects_failed_verification and test_verify_recaptcha_accepts_valid_token with a case whose _siteverify raises httpx.HTTPError. Assert that public_request_service.verify_recaptcha raises ValidationError and that its message indicates the service is unavailable and should be retried.app/services/public_request/create_project_request.py (1)
9-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the exported service functions. Both functions are re-exported as public service APIs but lack concise docstrings.
app/services/public_request/create_project_request.py#L9-L18: add a concise docstring describing the pending-project-request workflow.app/services/public_request/verify_recaptcha.py#L20-L31: add a concise docstring describing conditional token verification.As per coding guidelines,
app/services/**/*.pyrequires concise docstrings on public service functions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/create_project_request.py` around lines 9 - 18, Add concise docstrings to the exported service functions create_project_request in app/services/public_request/create_project_request.py (lines 9-18) and verify_recaptcha in app/services/public_request/verify_recaptcha.py (lines 20-31). Describe the pending-project-request workflow for create_project_request and conditional token verification for verify_recaptcha; no other behavior changes are needed.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@alembic/versions/20260710_0001_add_public_requests.py`:
- Around line 21-51: Enforce atomic uniqueness for pending proposed languages by
adding normalized, partial unique database constraints or indexes for proposed
language names and codes in
alembic/versions/20260710_0001_add_public_requests.py (lines 21-51), scoped to
pending requests and excluding null values. In
app/services/public_request/create_language_request.py (lines 15-25), catch the
resulting integrity/constraint violation, roll back the transaction, and raise
ConflictError instead of allowing the database error to escape.
In `@app/services/public_request/ensure_language_available.py`:
- Line 9: Add concise docstrings to the public service functions
ensure_language_available in
app/services/public_request/ensure_language_available.py (lines 9-9) describing
its conflict-validation behavior, and create_language_request in
app/services/public_request/create_language_request.py (lines 7-13) describing
its creation contract.
---
Other comments:
In `@app/models/public_request.py`:
- Around line 24-32: Make PublicProjectRequestCreate reject requests that
provide language_id together with either new_language_name or new_language_code,
using model-level validation while preserving valid existing-language and
new-language modes. Also retain the service-side guard in
create_project_request() for direct callers, updating both
app/models/public_request.py:24-32 and
app/services/public_request/create_project_request.py:20-28.
In `@app/services/public_request/create_project_request.py`:
- Around line 26-42: Make de-duplication atomic in the create-project flow
around ensure_language_available() and PublicRequest creation: enforce a
database-level uniqueness rule for pending language requests using the
normalized name/code, then handle concurrent insert conflicts by rolling back
and returning the existing validation behavior. Do not rely on the pre-insert
read alone.
In `@app/services/public_request/ensure_language_available.py`:
- Around line 13-19: Update the language lookup around existing_stmt to
normalize codes during comparison: apply func.lower to Language.code and to the
lowered_code value, and make the corresponding comparison for pending-request
codes case-insensitive. Preserve the existing language-name matching behavior.
In `@app/services/public_request/verify_recaptcha.py`:
- Around line 9-31: Update _siteverify and verify_recaptcha so malformed
siteverify responses—including JSONDecodeError, UnicodeDecodeError, or
non-object payloads causing AttributeError—are converted into ValidationError
rather than escaping as server errors. Preserve the existing unavailable-message
handling for HTTP failures and the failed-verification behavior for valid
responses with success false.
---
Nitpick comments:
In `@app/services/public_request/create_project_request.py`:
- Around line 9-18: Add concise docstrings to the exported service functions
create_project_request in app/services/public_request/create_project_request.py
(lines 9-18) and verify_recaptcha in
app/services/public_request/verify_recaptcha.py (lines 20-31). Describe the
pending-project-request workflow for create_project_request and conditional
token verification for verify_recaptcha; no other behavior changes are needed.
In `@tests/test_public_request_service.py`:
- Around line 196-218: Extend the reCAPTCHA verification tests alongside
test_verify_recaptcha_rejects_failed_verification and
test_verify_recaptcha_accepts_valid_token with a case whose _siteverify raises
httpx.HTTPError. Assert that public_request_service.verify_recaptcha raises
ValidationError and that its message indicates the service is unavailable and
should be retried.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 0d4b087f-454f-4204-860d-8fdc069c0347
📒 Files selected for processing (14)
alembic/versions/20260710_0001_add_public_requests.pyapp/api/public.pyapp/core/config.pyapp/db/models/__init__.pyapp/db/models/public_request.pyapp/main.pyapp/models/public_request.pyapp/services/__init__.pyapp/services/public_request/__init__.pyapp/services/public_request/create_language_request.pyapp/services/public_request/create_project_request.pyapp/services/public_request/ensure_language_available.pyapp/services/public_request/verify_recaptcha.pytests/test_public_request_service.py
| op.create_table( | ||
| "public_requests", | ||
| sa.Column("id", sa.String(length=36), nullable=False), | ||
| sa.Column("kind", sa.String(length=30), nullable=False), | ||
| sa.Column("status", sa.String(length=20), nullable=False, server_default="pending"), | ||
| sa.Column("requester_name", sa.String(length=200), nullable=False), | ||
| sa.Column("requester_email", sa.String(length=320), nullable=False), | ||
| sa.Column("name", sa.String(length=200), nullable=False), | ||
| sa.Column("code", sa.String(length=3), nullable=True), | ||
| sa.Column("description", sa.Text(), nullable=True), | ||
| sa.Column("language_id", sa.String(length=36), nullable=True), | ||
| sa.Column("new_language_name", sa.String(length=200), nullable=True), | ||
| sa.Column("new_language_code", sa.String(length=3), nullable=True), | ||
| sa.Column("reviewed_by", sa.String(length=36), nullable=True), | ||
| sa.Column("reviewed_at", sa.DateTime(timezone=True), nullable=True), | ||
| sa.Column("review_reason", sa.Text(), nullable=True), | ||
| sa.Column("created_entity_id", sa.String(length=36), nullable=True), | ||
| sa.Column( | ||
| "requested_at", | ||
| sa.DateTime(timezone=True), | ||
| server_default=sa.text("now()"), | ||
| nullable=False, | ||
| ), | ||
| sa.ForeignKeyConstraint(["language_id"], ["languages.id"], ondelete="SET NULL"), | ||
| sa.ForeignKeyConstraint(["reviewed_by"], ["users.id"], ondelete="SET NULL"), | ||
| sa.PrimaryKeyConstraint("id"), | ||
| ) | ||
| op.create_index("ix_public_requests_kind", "public_requests", ["kind"]) | ||
| op.create_index("ix_public_requests_status", "public_requests", ["status"]) | ||
| op.create_index("ix_public_requests_requester_email", "public_requests", ["requester_email"]) | ||
| op.create_index("ix_public_requests_code", "public_requests", ["code"]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Enforce pending-language uniqueness atomically.
The SELECT-based availability check cannot prevent concurrent conflicting submissions.
alembic/versions/20260710_0001_add_public_requests.py#L21-L51: add database-enforced normalized, partial uniqueness for pending proposed language names and codes.app/services/public_request/create_language_request.py#L15-L25: catch the resulting constraint violation, roll back, and raiseConflictError.
📍 Affects 2 files
alembic/versions/20260710_0001_add_public_requests.py#L21-L51(this comment)app/services/public_request/create_language_request.py#L15-L25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@alembic/versions/20260710_0001_add_public_requests.py` around lines 21 - 51,
Enforce atomic uniqueness for pending proposed languages by adding normalized,
partial unique database constraints or indexes for proposed language names and
codes in alembic/versions/20260710_0001_add_public_requests.py (lines 21-51),
scoped to pending requests and excluding null values. In
app/services/public_request/create_language_request.py (lines 15-25), catch the
resulting integrity/constraint violation, roll back the transaction, and raise
ConflictError instead of allowing the database error to escape.
| from app.db.models.public_request import PublicRequest | ||
|
|
||
|
|
||
| async def ensure_language_available(db: AsyncSession, name: str, code: str) -> None: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document public service functions.
app/services/public_request/ensure_language_available.py#L9-L9: add a concise conflict-validation docstring.app/services/public_request/create_language_request.py#L7-L13: add a concise creation-contract docstring.
As per coding guidelines, “Keep service functions function-oriented and composable, with concise docstrings on public service functions.” <coding_guidelines>
📍 Affects 2 files
app/services/public_request/ensure_language_available.py#L9-L9(this comment)app/services/public_request/create_language_request.py#L7-L13
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/public_request/ensure_language_available.py` at line 9, Add
concise docstrings to the public service functions ensure_language_available in
app/services/public_request/ensure_language_available.py (lines 9-9) describing
its conflict-validation behavior, and create_language_request in
app/services/public_request/create_language_request.py (lines 7-13) describing
its creation contract.
Source: Coding guidelines
…nguage Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…esponse Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…guage rejection Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
@coderabbitai — decisions on this review:
Other findings: atomic pending-language uniqueness (A/D) — skipped (a duplicate pending row in a human-moderated queue is benign, and a per-column unique index can't cover the cross-kind name/code invariant with the current schema). Case-insensitive code (E) — already handled (codes are normalized to lowercase on both write and read). Docstring nitpicks — declined (docstrings are intentionally omitted). |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd public_request.py app/models
echo "---model---"
cat -n app/models/public_request.py 2>/dev/null || true
echo "---verify_recaptcha---"
cat -n app/services/public_request/verify_recaptcha.py 2>/dev/null || true
echo "---tests---"
rg -n "unavailable|ValueError|HTTPError|model_validator" tests/test_public_request_service.py app/models/public_request.py app/services/public_request/verify_recaptcha.pyLength of output: 5155
Your rationale for skipping the atomic pending-language uniqueness (A/D) and declining docstrings is reasonable given the human-moderated queue and the schema constraints — no pushback from me there. Good to go on my end. (‑‑ᴗ‑‑) 🐰 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/public_request/verify_recaptcha.py (1)
1-19: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winRequire a boolean
successvalue.bool(payload.get("success"))accepts values like"false"or1as a pass, so a malformed response can slip through. Reject non-dict payloads and only treatsuccess is Trueas verified; thenAttributeErrorno longer needs to be caught here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/verify_recaptcha.py` around lines 1 - 19, Update _siteverify to reject non-dictionary JSON payloads and verify only when payload["success"] is exactly True, rather than coercing arbitrary values with bool(). Remove any now-unnecessary AttributeError handling in this verification flow while preserving the existing HTTP error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/services/public_request/verify_recaptcha.py`:
- Around line 1-19: Update _siteverify to reject non-dictionary JSON payloads
and verify only when payload["success"] is exactly True, rather than coercing
arbitrary values with bool(). Remove any now-unnecessary AttributeError handling
in this verification flow while preserving the existing HTTP error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: b8965faf-0753-43f8-8221-002e082f2e84
📒 Files selected for processing (3)
app/models/public_request.pyapp/services/public_request/verify_recaptcha.pytests/test_public_request_service.py
…admin review Public language requests now accept an optional description, matching the project request shape. Platform admins can list public requests filtered by kind/status and approve or reject them; approving creates the language or the project (with its proposed language) and records the reviewer, mirroring the change-request review flow. Public requesters have no account, so created entities carry no creator and no project access is granted. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/models/public_request.py (1)
48-63: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExpose persisted language descriptions in the response DTO.
create_language_requestnow storesdescription, andapp/api/public.pypasses it through, butPublicRequestResponsedoes not declare adescriptionfield in this response contract. Pydantic will omit it from submission and admin listing/review responses, preventing reviewers from seeing the requester’s description.Add
description: str | None = NonetoPublicRequestResponse.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/models/public_request.py` around lines 48 - 63, Add the optional description field with a None default to the PublicRequestResponse DTO, ensuring persisted language-request descriptions are included in submission and admin listing/review responses.app/services/public_request/create_language_request.py (1)
15-24: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject whitespace-only language names before persisting.
name.strip()can collapse inputs like" "into"", and the currentmin_length=1check still allows that. Reject empty normalized names before adding the request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/public_request/create_language_request.py` around lines 15 - 24, Validate the normalized language name in the create-language request flow before calling ensure_language_available or constructing PublicRequest. Reject names that become empty after trimming, while preserving the trimmed value for requester persistence instead of allowing name.strip() to produce an empty name.
🧹 Nitpick comments (2)
app/api/public_requests.py (2)
15-16: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
Literaltypes forkind/statusquery params.Unlike
PublicRequestReview.status(Literal["approved", "rejected"]), these accept arbitrary strings. Constraining them toLiteral["create_language", "create_project"]andLiteral["pending", "approved", "rejected"]would improve the OpenAPI contract and reject typos with a 422 instead of silently returning an empty list.♻️ Proposed refactor
- kind: str | None = Query(default=None), - status: str | None = Query(default=None), + kind: Literal["create_language", "create_project"] | None = Query(default=None), + status: Literal["pending", "approved", "rejected"] | None = Query(default=None),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/public_requests.py` around lines 15 - 16, Update the `kind` and `status` query parameter annotations in the public requests endpoint to use `Literal` with the allowed values: `kind` must be `"create_language"` or `"create_project"`, and `status` must be `"pending"`, `"approved"`, or `"rejected"`. Preserve their optional defaults so invalid values are rejected by validation with a 422 response.
13-21: 🚀 Performance & Scalability | 🔵 TrivialNo pagination on the admin listing endpoint.
list_public_requestsreturns every matching row with nolimit/offset. Fine at current volume, but as the queue grows this becomes an unbounded query/response. Worth adding pagination before this endpoint sees production-scale traffic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/api/public_requests.py` around lines 13 - 21, Extend the list_public_requests endpoint and its service call to accept pagination parameters such as limit and offset, apply them to the database query, and return only the requested page. Preserve the existing kind and status filters and admin authorization while enforcing sensible bounds on pagination inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/main.py`:
- Around line 131-136: Restrict ProxyHeadersMiddleware to the known trusted
proxy IP addresses or networks instead of allowing all hosts via
trusted_hosts="*". Update the middleware configuration associated with the
public and public_requests_router routes, or sanitize forwarded headers before
they reach the app, while preserving correct client-IP handling for trusted
upstream proxies.
In `@app/services/public_request/review_public_request.py`:
- Around line 49-57: Update the create_language branch to use await db.flush()
instead of await db.commit(), matching _create_requested_language, while
preserving the refresh and return flow so language creation commits atomically
with review_public_request’s request-status update.
- Around line 38-47: Update the create_project branch of _apply so a missing
request.language_id with no usable replacement language data is detected before
or within _create_requested_language and converted into the established
NotFoundError or ConflictError path. Preserve the existing language creation
flow when replacement data is available and continue passing the resolved
language_id to create_project.
---
Outside diff comments:
In `@app/models/public_request.py`:
- Around line 48-63: Add the optional description field with a None default to
the PublicRequestResponse DTO, ensuring persisted language-request descriptions
are included in submission and admin listing/review responses.
In `@app/services/public_request/create_language_request.py`:
- Around line 15-24: Validate the normalized language name in the
create-language request flow before calling ensure_language_available or
constructing PublicRequest. Reject names that become empty after trimming, while
preserving the trimmed value for requester persistence instead of allowing
name.strip() to produce an empty name.
---
Nitpick comments:
In `@app/api/public_requests.py`:
- Around line 15-16: Update the `kind` and `status` query parameter annotations
in the public requests endpoint to use `Literal` with the allowed values: `kind`
must be `"create_language"` or `"create_project"`, and `status` must be
`"pending"`, `"approved"`, or `"rejected"`. Preserve their optional defaults so
invalid values are rejected by validation with a 422 response.
- Around line 13-21: Extend the list_public_requests endpoint and its service
call to accept pagination parameters such as limit and offset, apply them to the
database query, and return only the requested page. Preserve the existing kind
and status filters and admin authorization while enforcing sensible bounds on
pagination inputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: c96c1ab6-e669-4727-8a17-4e59d268578b
📒 Files selected for processing (9)
app/api/public.pyapp/api/public_requests.pyapp/main.pyapp/models/public_request.pyapp/services/public_request/__init__.pyapp/services/public_request/create_language_request.pyapp/services/public_request/list_public_requests.pyapp/services/public_request/review_public_request.pytests/test_public_request_service.py
| app.include_router(public_router, prefix="/api/public", tags=["public"]) | ||
| app.include_router( | ||
| public_requests_router, | ||
| prefix="/api/public-requests", | ||
| tags=["public-requests"], | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant app entrypoint and surrounding middleware/router setup.
git ls-files app/main.py app | sed -n '1,120p'
echo '--- app/main.py outline ---'
ast-grep outline app/main.py --view expanded || true
echo '--- app/main.py lines 1-220 ---'
cat -n app/main.py | sed -n '1,220p'
echo '--- search for ProxyHeadersMiddleware and rate limiting ---'
rg -n "ProxyHeadersMiddleware|trusted_hosts|X-Forwarded-For|rate limit|limiter|slowapi|public_requests|public-router|public_requests_router" appRepository: shemaobt/tripod-api
Length of output: 15426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- app/api/public.py ---'
cat -n app/api/public.py | sed -n '1,220p'
echo '--- app/api/public_requests.py ---'
cat -n app/api/public_requests.py | sed -n '1,220p'
echo '--- app/core/rate_limit.py ---'
cat -n app/core/rate_limit.py | sed -n '1,120p'
echo '--- search for dependencies/auth on these routers ---'
rg -n "Depends\\(|require_|oauth2|Authorization|jwt|current_user|admin|auth" app/api/public.py app/api/public_requests.py app/services/public_request app/core/rate_limit.pyRepository: shemaobt/tripod-api
Length of output: 5730
🌐 Web query:
uvicorn ProxyHeadersMiddleware trusted_hosts "*" X-Forwarded-For get_remote_address SlowAPI behavior
💡 Result:
Uvicorn's ProxyHeadersMiddleware is designed to process forwarded headers—specifically X-Forwarded-For and X-Forwarded-Proto—to update the request scope's client and scheme information [1][2]. By modifying the scope's client address, this middleware allows subsequent application components (like rate limiters) to see the actual original client IP rather than the proxy's IP [3][4]. Key aspects of its behavior include: Trusted Hosts Configuration: The middleware only processes these headers if the connecting IP (the immediate upstream proxy) is present in the trusted_hosts list [1][5]. When configured via --forwarded-allow-ips (or the ProxyHeadersMiddleware constructor), this list can include IP addresses, IP networks, or literal paths [6][2]. Behavior of trusted_hosts = "": Setting this to "" instructs Uvicorn to trust every incoming connection as a proxy [6][2]. If enabled, the middleware will automatically parse and use the X-Forwarded-For headers from any client [2]. This poses a significant security risk, as any client can then spoof their IP address by injecting arbitrary X-Forwarded-For headers, which the middleware will then trust and propagate [2][3][7]. It should only be used when the infrastructure guarantees that all incoming headers are stripped or sanitized by an external, trusted gateway before reaching Uvicorn [2]. X-Forwarded-For Parsing Logic: When a trusted host is detected, the middleware parses the X-Forwarded-For header [1][8]. It searches for the correct client IP, typically by iterating through the list of IPs in the header and returning the first one that is not considered a proxy (or the correct client address based on the configuration) [9][10]. If no trusted proxy is found, or if the header is missing, the request scope's client address remains unchanged [1][5]. Interaction with SlowAPI: SlowAPI's get_ipaddr utility is intended for extracting the client's IP address [4]. Its documentation and maintainers explicitly recommend against relying on it for robust IP identification in production, as it can be easily spoofed [4][7]. They instead recommend using Uvicorn's ProxyHeadersMiddleware to correctly populate the request's client host before the request reaches the rate-limiting logic [4][7]. Once ProxyHeadersMiddleware has correctly updated scope["client"], SlowAPI can reliably read the trusted IP from request.client.host [7]. In summary, for secure production deployments, you should explicitly define your trusted proxy IP/network instead of using "*" [2][10]. Once correctly configured, Uvicorn updates the request's client address, allowing tools like SlowAPI to function accurately and securely [4][7].
Citations:
- 1: https://github.com/encode/uvicorn/blob/master/uvicorn/middleware/proxy_headers.py
- 2: https://uvicorn.dev/deployment/
- 3: https://fastapi.tiangolo.com/advanced/behind-a-proxy/
- 4: https://slowapi.readthedocs.io/en/latest/api/
- 5: https://github.com/encode/uvicorn/blob/main/uvicorn/middleware/proxy_headers.py
- 6: https://uvicorn.dev/settings/
- 7: XFF Standard
X-Forwarded-Forheader laurentS/slowapi#126 - 8: https://github.com/Kludex/uvicorn/blob/main/uvicorn/middleware/proxy_headers.py
- 9: Add search for a trusted host in ProxyHeadersMiddleware Kludex/uvicorn#591
- 10: Improve ProxyHeadersMiddleware Kludex/uvicorn#2231
Restrict ProxyHeadersMiddleware to trusted proxies
trusted_hosts="*" lets any client supply X-Forwarded-For, so the public IP-based limits on these routes can be bypassed unless an upstream gateway strips and rewrites forwarded headers. Use the actual proxy IPs/networks here, or enforce header sanitization before the app.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/main.py` around lines 131 - 136, Restrict ProxyHeadersMiddleware to the
known trusted proxy IP addresses or networks instead of allowing all hosts via
trusted_hosts="*". Update the middleware configuration associated with the
public and public_requests_router routes, or sanitize forwarded headers before
they reach the app, while preserving correct client-IP handling for trusted
upstream proxies.
| async def _apply(db: AsyncSession, request: PublicRequest) -> str: | ||
| if request.kind == "create_project": | ||
| language_id = request.language_id or await _create_requested_language(db, request) | ||
| project = await create_project( | ||
| db, | ||
| name=request.name, | ||
| language_id=language_id, | ||
| description=request.description, | ||
| ) | ||
| return project.id |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== review_public_request.py ==\n'
sed -n '1,220p' app/services/public_request/review_public_request.py
printf '\n== public_request model ==\n'
sed -n '1,260p' app/db/models/public_request.py
printf '\n== core exceptions ==\n'
sed -n '1,220p' app/core/exceptions.pyRepository: shemaobt/tripod-api
Length of output: 9542
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== locate symbols ==\n'
rg -n "_create_requested_language|class PublicRequest|language_id|new_language_name|new_language_code|NotFoundError|ConflictError" app/services/public_request app/db/models app/core -S
printf '\n== review_public_request.py (numbered) ==\n'
cat -n app/services/public_request/review_public_request.py
printf '\n== public_request.py (numbered) ==\n'
cat -n app/db/models/public_request.py
printf '\n== exceptions.py (numbered) ==\n'
cat -n app/core/exceptions.pyRepository: shemaobt/tripod-api
Length of output: 16906
Handle missing language references in create_project requests
If a pending create_project request loses its language_id, _create_requested_language can still reach the assert with no replacement language data and raise an uncaught AssertionError. Raise NotFoundError (or ConflictError) explicitly instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/public_request/review_public_request.py` around lines 38 - 47,
Update the create_project branch of _apply so a missing request.language_id with
no usable replacement language data is detected before or within
_create_requested_language and converted into the established NotFoundError or
ConflictError path. Preserve the existing language creation flow when
replacement data is available and continue passing the resolved language_id to
create_project.
| code = request.code | ||
| assert code is not None | ||
| if await get_language_by_code(db, code): | ||
| raise ConflictError("Language code already exists") | ||
| language = Language(name=request.name, code=code) | ||
| db.add(language) | ||
| await db.commit() | ||
| await db.refresh(language) | ||
| return language.id |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Inconsistent commit breaks review atomicity.
The create_language branch calls await db.commit() (line 55) independently of the final commit in review_public_request (line 33), while the sibling _create_requested_language (lines 60-69) only flush()es. If the outer commit (line 33) fails after this inner commit succeeds, the Language is permanently persisted but request.status stays "pending". A retry then re-enters _apply, finds the language already exists via get_language_by_code, and raises ConflictError, permanently blocking approval of that request.
Use flush() here too so the language creation and the request status update commit atomically as one transaction.
🔧 Proposed fix
language = Language(name=request.name, code=code)
db.add(language)
- await db.commit()
- await db.refresh(language)
+ await db.flush()
return language.id📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| code = request.code | |
| assert code is not None | |
| if await get_language_by_code(db, code): | |
| raise ConflictError("Language code already exists") | |
| language = Language(name=request.name, code=code) | |
| db.add(language) | |
| await db.commit() | |
| await db.refresh(language) | |
| return language.id | |
| code = request.code | |
| assert code is not None | |
| if await get_language_by_code(db, code): | |
| raise ConflictError("Language code already exists") | |
| language = Language(name=request.name, code=code) | |
| db.add(language) | |
| await db.flush() | |
| return language.id |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/public_request/review_public_request.py` around lines 49 - 57,
Update the create_language branch to use await db.flush() instead of await
db.commit(), matching _create_requested_language, while preserving the refresh
and return flow so language creation commits atomically with
review_public_request’s request-status update.
| from alembic import op | ||
|
|
||
| revision: str = "20260710_0001" | ||
| down_revision: str | None = "20260609_0001" |
There was a problem hiding this comment.
20260709_0001 already revises 20260609_0001, and main is at 20260714_0002 now. alembic upgrade head will fail with multiple heads on deploy, and the git rebase will not catch it because the files don't conflict. Could you re-point the down_revision to the current head and verify the chain?
| raise ConflictError("Language code already exists") | ||
| language = Language(name=request.name, code=code) | ||
| db.add(language) | ||
| await db.commit() |
There was a problem hiding this comment.
flushed, not committed, so a mid-approval failure leaves no orphan language
Not really — this line commits, and on the project branch create_project commits internally too, so the language is persisted before the status is stamped in both paths. Flushing here fixes this branch; the project one I think not, it would need create_project to change and that is not for this PR. Could you adapt the line and the description? @levigtri
| @router.get("", response_model=list[PublicRequestAdminResponse]) | ||
| async def list_public_requests( | ||
| kind: str | None = Query(default=None), | ||
| status: str | None = Query(default=None), |
There was a problem hiding this comment.
?status=bogus returns an empty list here instead of a 422. You already have Literal on PublicRequestReview.status — maybe the same closed set on these filters and on the model/service, a StrEnum on app/core/enums.py is preferred by me for a status machine. Just for the new code, the existent ones would be a separate task.
| assert code is not None | ||
| if await get_language_by_code(db, code): | ||
| raise ConflictError("Language code already exists") | ||
| language = Language(name=request.name, code=code) |
There was a problem hiding this comment.
Just make sure here — the description you just added on the language request is dropped on approval, Language has no column for it. If it is only context for the admin reviewing the request that's ok, could you confirm?
…-requests-languageproject-with # Conflicts: # app/db/models/__init__.py
2 participants
[US-15.1 / OBT-250] Public creation requests for languages and projects
Summary
Visitors without an account can now request the creation of a Language or a Project by providing only their name and email plus the same fields used by the authenticated creation flows. Submissions are stored as pending rows in a new
public_requeststable — a cheap insert with no side effects — and a platform admin can then list and review them, mirroring thechange_requestsreview shape. Abuse protection is layered: per-IP rate limiting (existing slowapi limiter) on every public endpoint, plus optional server-side reCAPTCHA v2 verification — whenRECAPTCHA_SECRET_KEYis unset the app runs with rate limiting alone, so the feature can ship before the reCAPTCHA keys are provisioned. A public, unauthenticated languages listing feeds the project form's language combobox. A project request may either reference an existing language or propose a new one (name + code), mirroring the authenticated flow. Language uniqueness is enforced on both name and code, case-insensitively ("English/eng" ≡ "english/ENG"), against thelanguagestable and every pending public request — including new languages proposed inside project requests.Changes
Model + migration
app/db/models/public_request.py— newPublicRequestmodel (public_requeststable):kind(create_language|create_project),status(defaultpending), requester contact columns (requester_name,requester_email) instead of a user FK, entity fields (name,code,description,language_id), review columns (reviewed_by,reviewed_at,review_reason,created_entity_id) andrequested_at.new_language_name+new_language_codefor project requests that propose a language.alembic/versions/20260710_0001_add_public_requests.py— creates the table with FKs (languages.id,users.id, bothSET NULL) and indexes onkind,status,requester_email,code. Down revision:20260609_0001. No new migration was needed for thedescriptionaddition — the column already existed on the table for project requests.Schemas
app/models/public_request.py—PublicLanguageOption(slimid/name/codefor the public combobox, avoids exposing internal fields),PublicLanguageRequestCreate(requester name/email viaEmailStr, language name, code, optionaldescription, optionalrecaptcha_token),PublicProjectRequestCreate(requester name/email, project name, optional description,language_idORnew_language_name+new_language_code, optionalrecaptcha_token),PublicRequestResponse.PublicRequestReview(status:approved|rejected, optionalreason) andPublicRequestAdminResponse(extendsPublicRequestResponsewithreviewed_by,reviewed_at,review_reason,created_entity_id). The admin response is kept separate from the public one so the review columns are never serialized back to an anonymous requester.^[A-Za-z]{3}$(ISO 639-3, ASCII letters) so lowercasing can never expand past theVARCHAR(3)column (Unicode edge cases likeİwould otherwise 500 on insert).Service
app/services/public_request/ensure_language_available.py— shared uniqueness check:ConflictError(409) when the name (case-insensitive) or code matches an existing language OR any pending public request (language kind, or a new language proposed in a project request).app/services/public_request/create_language_request.py— normalizes the code to lowercase, stores the optional description, and delegates to the shared check.app/services/public_request/create_project_request.py— acceptslanguage_id(validated viaget_language_or_404, 404 for unknown ids) OR anew_language_name+new_language_codepair (validated via the shared check); neither →ValidationError(400).app/services/public_request/list_public_requests.py(new) — newest first, optionalkind/statusfilters.app/services/public_request/review_public_request.py(new) — guards onstatus != "pending"(ConflictError409), applies the request when approved, then stampsstatus/reviewed_by/reviewed_at/review_reason._applycreates the language, or the project plus its proposed language (flushed, not committed, so a mid-approval failure leaves no orphan language — same shape asreview_change_request). A public requester has no account, so created entities carry nocreated_by/creator_user_idand no project access is granted — this is the one place the flow deliberately diverges from the change-request path.app/services/public_request/verify_recaptcha.py— POSTs tohttps://www.google.com/recaptcha/api/siteverifywithhttpx; raisesValidationError(400) when the token is missing or rejected by Google, and also when the siteverify call itself fails (network/HTTP errors no longer bubble up as 500). EmptyRECAPTCHA_SECRET_KEYdisables verification entirely (rate limiting remains), sorecaptcha_tokenis optional in the schemas.app/core/config.py— newrecaptcha_secret_keysetting (envRECAPTCHA_SECRET_KEY).app/services/__init__.py— registerspublic_request_service.Routers
app/api/public.py— unauthenticated router:GET /api/public/languages,POST /api/public/requests/language(201),POST /api/public/requests/project(201). No database access in the router; reCAPTCHA verified before any write.5/minute;20/hour, languages listing at30/minute(429 beyond that, handler already registered inmain.py).app/api/public_requests.py(new) — admin router, every routerequire_platform_admin:GET /api/public-requests?kind=&status=,PATCH /api/public-requests/{id}/review. Mounted separately from/api/publicprecisely because that prefix is unauthenticated.app/main.py— registers the public router at/api/publicwithout auth dependencies, and the admin router at/api/public-requests.Type of Change
Testing
uv run ruff check .anduv run ruff format --check .pass.uv run --group dev python -m pytest tests/test_public_request_service.py— 25 passed: language request happy path (code lowercased, status pending), conflicts by code and by name (case-insensitive) against existing languages and pending requests (including new languages inside pending project requests), project request with existing language, with a proposed new language, with conflicting new language, without any language info (400), unknown language 404, and reCAPTCHA skip/missing-token/reject/accept paths — plus, new: description stored / description optional, list filtered by kind and by status, approve creates the language, approve creates the project with its proposed language, reject creates nothing and records the reason, re-review → 409, review of an unknown id → 404.Run each test in its own process (
rm -f test.db && pytest tests/test_public_request_service.py::<name>) — the suite's file-sqlite fixture races when a whole file shares one process./api/public/requests/languagewithin a minute from the same IP return 429. Verified:[201, 201, 201, 201, 201, 429, 429, 429]. Note the limit is bypassable by forgingX-Forwarded-For— pre-existing, already onmain, tracked separately in OBT-260; not introduced here.Summary by CodeRabbit