upload: add the Upload model, migration and real crud (swap the stub) (#198)

* upload: add the Upload model, migration and real crud (swap the stub)

Replaces the no-op crud_upload with the real persistence layer: the Upload
model (one row per accepted upload; sha256 indexed but not unique), a named-FK
migration, and real create/exists_with_sha/get_multi_by_sha/all_shas. find_orphans
is now typed against models.Upload. Restores the deferred tests -- re-upload of
identical bytes is free, a second row is written, find_orphans/detection work.

* upload: close the purpose set, count avatars as usages, check config up front (#199)

Four review findings on the image-upload work, none of which change the shape
of the design.

purpose was a free-form string. The karma gate tests for "figure" and the
read-time misuse detection tests for "avatar", so any third value was an upload
that was neither gated nor detected: karma 0 with purpose="banana" returned 200.
It is now upload_purpose_T, a closed Literal beside editor_T, so anything else
is a 422 at the boundary.

find_usages scanned bodies only, but a purpose="avatar" upload lives in
User.avatar_url and is embedded in no body at all, so every avatar ever
uploaded read as an orphan -- and orphans are the input to any future garbage
collection. Question.description had the same gap. Both avatar columns and both
question description columns join the scan.

is_configured() checked only the endpoint URL while the write path also needs
the bucket (a NOT NULL column) and the public URL base, so a half-configured
deployment 500'd *after* put_image had already stored the object. It now checks
all three, which makes it the 503 it was meant to be, before anything is
written.

s15 shared one PNG fixture across checks, so the "new figure costs 2 coins"
point was uploading bytes an earlier check had already sent -- correctly free,
and reported as XFAIL against a backend that was right. Each check now builds
its own bytes, and every point is a hard assertion rather than a note, so a
regression fails the suite instead of printing a known-failure line.


Claude-Session: https://claude.ai/code/session_01CqbhVfwzFUYGkgJWnWwSk1

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: chai-inu

alembic/versions/3c2f1a8b9d4e_add_upload_table.py

@@ -0,0 +1,50 @@
+"""Add upload table
+
+One row per accepted image upload; the object store holds one object per sha256.
+
+Revision ID: 3c2f1a8b9d4e
+Revises: 7a670908f3fa
+Create Date: 2026-08-14 12:00:00.000000
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+# revision identifiers, used by Alembic.
+revision = '3c2f1a8b9d4e'
+down_revision = '7a670908f3fa'
+branch_labels = None
+depends_on = None
+
+# Named, rather than the ``None`` autogenerate emits: an unnamed constraint
+# leaves the downgrade with no name to drop.
+FK_NAME = "upload_uploader_id_fkey"
+
+
+def upgrade():
+    op.create_table(
+        'upload',
+        sa.Column('id', sa.Integer(), nullable=False),
+        sa.Column('uploader_id', sa.Integer(), nullable=False),
+        sa.Column('sha256', sa.String(), nullable=False),
+        sa.Column('content_type', sa.String(), nullable=False),
+        sa.Column('size_bytes', sa.Integer(), nullable=False),
+        sa.Column('original_filename', sa.String(), nullable=True),
+        sa.Column('purpose', sa.String(), nullable=False),
+        sa.Column('storage_bucket', sa.String(), nullable=False),
+        sa.Column('created_at', sa.DateTime(timezone=True), nullable=False),
+        sa.ForeignKeyConstraint(['uploader_id'], ['user.id'], name=FK_NAME),
+        sa.PrimaryKeyConstraint('id'),
+    )
+    op.create_index(op.f('ix_upload_id'), 'upload', ['id'], unique=False)
+    op.create_index(op.f('ix_upload_uploader_id'), 'upload', ['uploader_id'], unique=False)
+    op.create_index(op.f('ix_upload_sha256'), 'upload', ['sha256'], unique=False)
+
+
+def downgrade():
+    op.drop_index(op.f('ix_upload_sha256'), table_name='upload')
+    op.drop_index(op.f('ix_upload_uploader_id'), table_name='upload')
+    op.drop_index(op.f('ix_upload_id'), table_name='upload')
+    op.drop_constraint(FK_NAME, 'upload', type_='foreignkey')
+    op.drop_table('upload')

chafan_core/app/api/api_v1/endpoints/upload.py

@@ -9,6 +9,7 @@
 from chafan_core.app.limiter import limiter
 from chafan_core.app.services import uploads as uploads_service
 from chafan_core.utils.base import HTTPException_
+from chafan_core.utils.constants import upload_purpose_T
 
 router = APIRouter()
 
@@ -22,7 +23,11 @@ def upload_image(
     ctx: RequestContext = Depends(deps.get_request_context_logged_in),
     file: UploadFile = File(...),
     file_size: int = Depends(valid_content_length),
-    purpose: str = Form("figure"),
+    # A closed set, not a free-form string: anything outside it would skip the
+    # karma gate (which tests for "figure") and be invisible to the read-time
+    # misuse detection (which tests for "avatar"). The default is the gated
+    # value, so an old client cannot slip past the karma check either.
+    purpose: upload_purpose_T = Form("figure"),
 ) -> Any:
     if not object_storage.is_configured():
         raise HTTPException_(

chafan_core/app/crud/crud_upload.py

@@ -1,19 +1,10 @@
-"""Stub persistence for image uploads.
-
-TODO: replace with the real ``Upload`` model + migration. Until then the
-endpoint works against the object store but leaves no database record, and
-every upload looks new -- so re-uploading identical bytes is charged again and
-``find_orphans`` / ``misdeclared_avatars`` see nothing. The real crud, model
-and migration land together in the final "SQL" PR; this module is the seam that
-lets the rest of the code merge first.
-"""
-
-from __future__ import annotations
-
+import datetime
 from typing import Any, List, Optional
 
 from sqlalchemy.orm import Session
 
+from chafan_core.app.models.upload import Upload
+
 
 def create(
     db: Session,
@@ -25,21 +16,34 @@ def create(
     purpose: str,
     storage_bucket: str,
     original_filename: Optional[str] = None,
-) -> Any:
-    # TODO: write an Upload row.
-    return None
+) -> Upload:
+    upload = Upload(
+        uploader_id=uploader_id,
+        sha256=sha256,
+        content_type=content_type,
+        size_bytes=size_bytes,
+        original_filename=original_filename,
+        purpose=purpose,
+        storage_bucket=storage_bucket,
+        created_at=datetime.datetime.now(tz=datetime.timezone.utc),
+    )
+    db.add(upload)
+    db.flush()
+    db.refresh(upload)
+    return upload
 
 
 def exists_with_sha(db: Session, *, sha: str) -> bool:
-    # TODO: query the Upload table. Until then every upload looks new.
-    return False
+    return db.query(Upload).filter(Upload.sha256 == sha).first() is not None
 
 
-def get_multi_by_sha(db: Session, *, sha: str) -> List[Any]:
-    # TODO: return the Upload rows for `sha`.
-    return []
+def get_multi_by_sha(db: Session, *, sha: str) -> List[Upload]:
+    return db.query(Upload).filter(Upload.sha256 == sha).all()
 
 
 def all_shas(db: Session) -> List[str]:
-    # TODO: return every distinct sha in the Upload table.
-    return []
+    return [row[0] for row in db.query(Upload.sha256).distinct().all()]
+
+
+def get(db: Session, id: Any) -> Optional[Upload]:
+    return db.query(Upload).filter(Upload.id == id).first()

chafan_core/app/models/__init__.py

@@ -32,6 +32,7 @@
 from .submission_suggestion import SubmissionSuggestion
 from .task import Task
 from .topic import Topic
+from .upload import Upload
 from .user import User
 from .webhook import Webhook
 from .viewcount import ViewCountArticle, ViewCountAnswer, ViewCountQuestion, ViewCountSubmission

chafan_core/app/models/upload.py

@@ -0,0 +1,34 @@
+from typing import TYPE_CHECKING, Optional
+
+from sqlalchemy import Column, DateTime, ForeignKey, Integer, String
+from sqlalchemy.orm import relationship
+
+from chafan_core.db.base_class import Base
+
+if TYPE_CHECKING:
+    from . import *  # noqa: F401, F403
+
+
+class Upload(Base):
+    """One row per *accepted* upload; the bucket holds one object per sha256.
+
+    The two non-obvious columns:
+
+      * ``sha256`` is deliberately NOT unique. The bucket stores one object per
+        sha; this table records one row per accepted upload, so a free re-upload
+        still leaves an audit trail and the second person to upload the same
+        bytes is not erased.
+      * ``storage_bucket`` records which bucket holds those bytes, so a future
+        vendor migration is a verifiable list rather than a guess.
+    """
+
+    id = Column(Integer, primary_key=True, index=True)
+    uploader_id = Column(Integer, ForeignKey("user.id"), nullable=False, index=True)
+    uploader: Optional["User"] = relationship("User", back_populates="uploads")  # type: ignore
+    sha256 = Column(String, nullable=False, index=True)
+    content_type = Column(String, nullable=False)
+    size_bytes = Column(Integer, nullable=False)  # after sanitizing
+    original_filename = Column(String)
+    purpose = Column(String, nullable=False)  # "figure" | "avatar"
+    storage_bucket = Column(String, nullable=False)
+    created_at = Column(DateTime(timezone=True), nullable=False)

chafan_core/app/models/user.py

@@ -25,6 +25,7 @@
 from chafan_core.app.models.submission import Submission
 from chafan_core.app.models.submission_suggestion import SubmissionSuggestion
 from chafan_core.app.models.task import Task
+from chafan_core.app.models.upload import Upload
 from chafan_core.db.base_class import Base
 from chafan_core.utils.base import UUID_LENGTH
 from chafan_core.utils.validators import StrippedNonEmptyBasicStr
@@ -247,6 +248,11 @@ class User(Base):
         back_populates="user",
         order_by="AuditLog.created_at.desc()",
     )
+    uploads: List["Upload"] = relationship(  # type: ignore
+        "Upload",
+        back_populates="uploader",
+        order_by="Upload.created_at.desc()",
+    )
     initiated_tasks: List["Task"] = relationship(  # type: ignore
         "Task",
         back_populates="initiator",

chafan_core/app/object_storage.py

@@ -35,7 +35,21 @@
 
 
 def is_configured() -> bool:
-    return settings.UPLOADS_S3_ENDPOINT_URL is not None
+    """True when every setting the write path needs is present.
+
+    All three, not just the endpoint: the bucket is a NOT NULL column on the
+    upload row and the public base is what the response URL is built from, so
+    a half-configured deployment used to fail with a 500 *after* the object had
+    already been stored. Checked up front, that becomes a 503 and nothing is
+    written anywhere.
+    """
+    return all(
+        (
+            settings.UPLOADS_S3_ENDPOINT_URL,
+            settings.UPLOADS_S3_BUCKET,
+            settings.UPLOADS_PUBLIC_URL_BASE,
+        )
+    )
 
 
 @lru_cache(maxsize=1)

chafan_core/app/services/uploads.py

@@ -21,6 +21,7 @@
 from chafan_core.app.common import MAX_UPLOAD_BYTES, report_msg
 from chafan_core.app.config import settings
 from chafan_core.utils.base import HTTPException_
+from chafan_core.utils.constants import upload_purpose_T
 
 logger = logging.getLogger(__name__)
 
@@ -44,7 +45,9 @@ def _read_bytes(file: Any) -> bytes:
     return bytes(buf)
 
 
-def upload_image(ctx, *, file, file_size: int, purpose: str) -> schemas.UploadedImage:
+def upload_image(
+    ctx, *, file, file_size: int, purpose: upload_purpose_T
+) -> schemas.UploadedImage:
     current_user = ctx.get_current_active_user()
     db = ctx.get_db()
 
@@ -98,39 +101,44 @@ def upload_image(ctx, *, file, file_size: int, purpose: str) -> schemas.Uploaded
 # the correct answer with no join table to maintain. Nothing here deletes.
 # ---------------------------------------------------------------------------
 
-_BODY_COLUMNS = [
+# Every column that can hold the URL of an uploaded image. Bodies are the
+# obvious ones. The two avatar columns matter just as much: a purpose="avatar"
+# upload is never embedded in anyone's body, so without them every avatar ever
+# uploaded reads as an orphan -- and orphans are the input to any future
+# garbage collection.
+_REFERENCE_COLUMNS = [
     (models.Article, "body", "article"),
     (models.Article, "body_draft", "article"),
     (models.ArticleArchive, "body", "article_archive"),
     (models.Answer, "body", "answer"),
     (models.Answer, "body_draft", "answer"),
     (models.Archive, "body", "answer_archive"),
     (models.Comment, "body", "comment"),
+    (models.Question, "description", "question"),
+    (models.QuestionArchive, "description", "question_archive"),
     (models.Submission, "description", "submission"),
     (models.SubmissionArchive, "description", "submission_archive"),
+    (models.User, "avatar_url", "user_avatar"),
+    (models.User, "gif_avatar_url", "user_gif_avatar"),
 ]
 
 
 def find_usages(db: Session, *, sha: str) -> List[str]:
-    """Locations (``table:id``) whose body text embeds ``sha``.
+    """Locations (``table:id``) that reference ``sha``.
 
     The public URL embeds the sha, so a ``LIKE`` hit is exact.
     """
     usages: List[str] = []
-    for model, column, label in _BODY_COLUMNS:
+    for model, column, label in _REFERENCE_COLUMNS:
         col = getattr(model, column)
         for (id_,) in db.query(model.id).filter(col.like(f"%{sha}%")).all():
             usages.append(f"{label}:{id_}")
     return usages
 
 
-def find_orphans(db: Session) -> List[Any]:
-    """Uploads whose sha appears in no body or archive text. Report only.
-
-    Returns nothing until the Upload table lands (``crud.upload`` is stubbed);
-    typed ``Any`` so the real model can arrive later without touching this.
-    """
-    orphans: List[Any] = []
+def find_orphans(db: Session) -> List[models.Upload]:
+    """Uploads whose sha appears in no body or archive text. Report only."""
+    orphans: List[models.Upload] = []
     for sha in crud.upload.all_shas(db):
         if not find_usages(db, sha=sha):
             orphans.extend(crud.upload.get_multi_by_sha(db, sha=sha))