diff --git a/.env.example b/.env.example index f59caa5..4b80011 100644 --- a/.env.example +++ b/.env.example @@ -119,17 +119,68 @@ AUTH_LOCKOUT_SECONDS=300 AUTH_ALLOW_ANY_LOGIN=true # Machine consumers of api. - scripts, partner integrations, your own -# backends. Format: name:role:secret, comma-separated, role is admin or user. +# backends. Format: name:role:secret, comma-separated. Role is one of: +# +# admin everything, including system/init and model training +# user catalog write access (add products, upload inventory) +# uploader ONE verb: POST /api/uploads/catalog. Nothing else - it cannot +# read the catalog, cannot see another caller's submissions, and +# cannot cancel or resume anything. This is the role to issue to +# an outside party who needs to send you spreadsheets. +# # Callers send the secret as an X-API-Key header. # # One entry per consumer, always: a shared key cannot be revoked for one caller # without breaking every other. Mint them with: # python scripts/make_auth_secrets.py --api-key partner-x:user +# python scripts/make_auth_secrets.py --api-key catalog-drop:uploader +# +# NAME THE KEY FOR ITS FUNCTION, NOT THE PERSON HOLDING IT. /api/health is +# public and reports {name, role, fingerprint} for every configured key. The +# secret is never exposed, but the NAME is - so `catalog-drop:uploader:...` is +# right and `priya-laptop:uploader:...` publishes a colleague's name to anyone +# who curls the health endpoint. # # Leave empty if only the web app calls the API - it signs in through # /api/auth/login instead, and a key nobody needs is only risk. API_KEYS= +# --------------------------------------------------------------------------- +# Catalog batch ingestion +# --------------------------------------------------------------------------- +# Used by both spreadsheet ingestion routes - the admin one +# (POST /api/admin/catalog-batch/ingest) and the API-client one +# (POST /api/uploads/catalog). Every ceiling is enforced in the application, +# not at the proxy: in production the caller reaches mcp.nearle.ai.in directly, +# so neither nginx's client_max_body_size nor Caddy's request_body cap is in +# front of these endpoints. +# +# Per-file limits are fixed in code at 10MB / 2000 rows, matching the +# single-file upload path. These bound the BATCH on top of that. +BATCH_MAX_FILES=20 +BATCH_MAX_TOTAL_BYTES=52428800 +BATCH_MAX_TOTAL_ROWS=20000 + +# Where staged uploads live. Under DATA_DIR because that path is already a +# declared volume, which is what lets a batch survive a container restart. +# BATCH_UPLOAD_DIR=/app/data/batch_uploads + +# Batches allowed to wait behind the one running. One worker thread runs a +# single batch at a time; past this depth the endpoints answer 429 rather than +# accepting work they have no intention of starting soon. This queue is the +# only thing bounding what an uploader key can cost in CPU - raise it with care +# on a one-vCPU host. +BATCH_QUEUE_MAX=4 + +# Staged files are deleted this many days after the batch was created. +BATCH_RETENTION_DAYS=7 + +# Deliberately false. A batch a restart cut short is marked "interrupted" and +# waits for someone to press Resume. Auto-resuming means a container stuck in a +# restart loop re-runs the heaviest work in the app on every boot, which is how +# a slow start becomes an unrecoverable spiral. +BATCH_AUTO_RESUME=false + USE_OLLAMA=true OLLAMA_BASE_URL=http://localhost:11434 OLLAMA_MODEL_NAME=qwen2.5:1.5b diff --git a/.gitignore b/.gitignore index 08dfb06..a727e9a 100644 --- a/.gitignore +++ b/.gitignore @@ -51,15 +51,10 @@ orchestration/.dagster_home/.telemetry/ # .env.orchestration IS committed - it holds no secrets, only the local # database pin that keeps orchestrated writes off production. -# Spreadsheets uploaded through Admin -> Batch Catalog Ingestion, plus their +# Spreadsheets sent to the catalog ingestion endpoints, plus their # manifests. This is operator data, not source: it is whatever a colleague # happened to send, it can contain a store's real pricing, and in a container # it lives on the /app/data volume rather than in the image. Committing it # would put customer files in the repository permanently. # BATCH_UPLOAD_DIR overrides the location; this covers the default. data/batch_uploads/ - -# Spreadsheets a colleague dropped through POST /api/uploads/catalog, waiting -# for review. Same reasoning as above, and more so: these arrive from outside -# the team and nobody here has looked at them yet. -data/inbox/ diff --git a/README.md b/README.md index 34124b5..5dd7ac5 100644 --- a/README.md +++ b/README.md @@ -117,10 +117,52 @@ curl -X POST localhost:8000/api/auth/login \ Send it as `Authorization: Bearer `, or use an `X-API-Key` from the `API_KEYS` setting for server-to-server callers. `admin` passes every -permission check; `user` holds the product/store/inventory permissions. +permission check; `user` holds the product/store/inventory permissions; +`uploader` holds exactly one, `upload_catalog` (see below). `AUTH_ENABLED=false` disables all of it for local work — never in a deployment. See the Authentication section of `../DEPLOYMENT.md` for the full endpoint map. +## Catalog ingestion for API clients + +An outside party can send spreadsheets straight into the catalog pipeline +without an admin in the loop. Issue them an `uploader` key: + +```bash +python scripts/make_auth_secrets.py --api-key catalog-drop:uploader +# -> API_KEYS=catalog-drop:uploader: (add to .env, redeploy) +``` + +Name the key for its function, not its holder: `/api/health` publicly reports +every key's name, role and fingerprint (never the secret). + +They then POST files and poll the batch: + +```bash +curl -X POST https://mcp.nearle.ai.in/api/uploads/catalog \ + -H 'X-API-Key: ' \ + -F 'files=@store-catalog.xlsx' -F 'files=@second-store.xlsx' +# -> 202 {"batch_id": "...", "status": "queued", "files": [...], "message": "..."} + +curl https://mcp.nearle.ai.in/api/uploads/catalog/ -H 'X-API-Key: ' +# -> {"status": "running", "files_done": 1, "files": [{"stage_name": "...", ...}]} +``` + +`.xlsx`, `.xls` and `.csv` are accepted, up to 10MB / 2000 rows per file and +`BATCH_MAX_FILES` files per request. Each file runs the same 11 stages as the +admin route (`app/core/store_catalog_pipeline.py`). A sheet that cannot be +parsed — or that has no product-name column — is rejected during the request +with a 400 naming the problem, so the sender finds out while they can still fix +it; a bad file alongside good ones comes back in `files` as `status: "failed"` +while the rest still run. + +Two things this credential cannot do. It cannot see anything but its own +submissions — every read is filtered by `submitted_by`, so it reaches neither +the catalog nor another caller's batches — and it cannot multiply the work: +all ingestion, from every source, goes through one worker thread behind a queue +of `BATCH_QUEUE_MAX`, past which the endpoint answers 429. Cancel, resume and +the full batch list stay on the admin router +(`/api/admin/catalog-batch/...`, `require_admin`). + ## MCP server The catalog is exposed to AI clients over the Model Context Protocol at `/mcp`, diff --git a/app/api/batch_common.py b/app/api/batch_common.py new file mode 100644 index 0000000..15b1885 --- /dev/null +++ b/app/api/batch_common.py @@ -0,0 +1,268 @@ +"""Shared machinery for the two routes that start a catalog batch. + + app/api/routers/batch_catalog.py POST /api/admin/catalog-batch/ingest + app/api/routers/uploads.py POST /api/uploads/catalog + +Both accept spreadsheets, both run the same 11 stages over them, and both hand +back a batch id to poll. What differs is only who may call them and what the +caller is allowed to see afterwards - so everything between "read the upload" +and "queue the batch" lives here instead of being written twice and drifting. + +WHY THE LIMITS ARE ARGUMENTS RATHER THAN IMPORTS +------------------------------------------------ +`read_uploads` and `parse_all` take an `UploadLimits` instead of reading the +settings themselves. Each router builds one from ITS OWN module globals, at +call time, which is what keeps + + monkeypatch.setattr(batch_catalog, "BATCH_MAX_FILES", 2) + +working - the idiom the existing suite is written in. Had this module read the +settings directly, those patches would become silently inert and a limit test +that no longer exercises its limit would still pass. +""" +from __future__ import annotations + +import logging +import queue +from dataclasses import dataclass +from typing import List, Optional, Tuple + +from fastapi import HTTPException, UploadFile +from pydantic import BaseModel + +from app.api.batch_job_store import batch_job_store +from app.core import batch_ingest, batch_worker +from app.core import store_catalog_pipeline as pipeline + +logger = logging.getLogger(__name__) + +# The worker cannot import the API layer without a cycle, so the wiring is done +# once, here, at import. This module is imported by every router that can start +# a batch, which is why it is the right place: whichever of them loads first, +# the worker is configured before anything can be submitted to it. +batch_worker.configure( + on_change=batch_job_store.put, + should_cancel=batch_job_store.is_cancelled, +) + + +@dataclass(frozen=True) +class UploadLimits: + """Ceilings for one submission. Per-file first, then per-batch.""" + + max_files: int + max_file_bytes: int + max_file_rows: int + max_total_bytes: int + max_total_rows: int + + +# --------------------------------------------------------------------------- +# Reading and parsing +# --------------------------------------------------------------------------- +async def read_uploads( + files: List[UploadFile], limits: UploadLimits +) -> List[Tuple[str, bytes]]: + """Read every upload into memory, enforcing the count and size ceilings. + + Read here rather than in the worker for the reason `store_catalog.py` gives: + `UploadFile` is backed by a temporary file tied to the request, and it is + gone before a background thread would reach it. + """ + if not files: + raise HTTPException(status_code=400, detail="No files were uploaded.") + if len(files) > limits.max_files: + raise HTTPException( + status_code=413, + detail=( + f"{len(files)} files exceeds the {limits.max_files}-file limit for one " + f"batch. Split the drop and send it in two." + ), + ) + + read: List[Tuple[str, bytes]] = [] + total = 0 + for upload in files: + contents = await upload.read() + name = upload.filename or "upload.xlsx" + if not contents: + # Recorded rather than raised - an empty file among nine good ones + # is a fact about that file, not a reason to reject the drop. + read.append((name, b"")) + continue + if len(contents) > limits.max_file_bytes: + raise HTTPException( + status_code=413, + detail=( + f"'{name}' is larger than the " + f"{limits.max_file_bytes // (1024 * 1024)}MB per-file limit." + ), + ) + total += len(contents) + if total > limits.max_total_bytes: + raise HTTPException( + status_code=413, + detail=( + f"The batch is larger than the " + f"{limits.max_total_bytes // (1024 * 1024)}MB total limit." + ), + ) + read.append((name, contents)) + return read + + +def parse_all(read: List[Tuple[str, bytes]], limits: UploadLimits): + """Split the uploads into (valid, invalid) by trying to parse each one. + + Failing here is what stops a batch transitioning straight to "failed" a + second after it started - the same reasoning as `store_catalog.py`, applied + per file so one bad sheet does not condemn the others. + """ + valid: List[Tuple[str, bytes, int]] = [] + invalid: List[Tuple[str, str]] = [] + rows_total = 0 + + for name, contents in read: + if not contents: + invalid.append((name, "The file is empty.")) + continue + try: + df, mapping = pipeline.parse_spreadsheet(name, contents) + except HTTPException as exc: + # read_products_dataframe raises HTTPException for an unsupported + # extension or a missing Excel reader; its message already names the + # file and says what to do about it. + invalid.append((name, str(exc.detail))) + continue + except Exception as exc: # noqa: BLE001 - an unreadable sheet is caller error + invalid.append((name, f"Could not parse the file: {exc}")) + continue + + if df.empty: + invalid.append((name, "The file has no data rows.")) + continue + + # A sheet whose headers carry no product name is not a catalog, and + # without this it is accepted with a 202 and then ingests nothing - the + # worst possible answer, because it looks like success from every angle + # the caller can see. `user_products.py` has always made this check on + # its own upload path; the catalog paths did not, and an API client + # sending the wrong export is the likeliest mistake there is. + if "product_name" not in mapping.columns: + recognised = ", ".join(sorted(mapping.columns)) or "none" + invalid.append(( + name, + f"No product name column was found. Headers read: " + f"{', '.join(str(c) for c in df.columns)}. Recognised fields: " + f"{recognised}.", + )) + continue + + if len(df) > limits.max_file_rows: + invalid.append(( + name, + f"{len(df)} rows exceeds the {limits.max_file_rows}-row per-file limit.", + )) + continue + + rows_total += int(len(df)) + if rows_total > limits.max_total_rows: + raise HTTPException( + status_code=413, + detail=( + f"The batch totals more than {limits.max_total_rows} rows. " + f"Split it and send it in two." + ), + ) + valid.append((name, contents, int(len(df)))) + + return valid, invalid, rows_total + + +# --------------------------------------------------------------------------- +# Response shape +# --------------------------------------------------------------------------- +class BatchFileOut(BaseModel): + index: int + filename: str + status: str + detail: Optional[str] = None + stage_index: int = 0 + stage_name: str = "" + total_stages: int = pipeline.TOTAL_STAGES + rows_done: int = 0 + rows_total: int = 0 + size_bytes: int = 0 + result: Optional[dict] = None + + +class BatchOut(BaseModel): + batch_id: str + status: str + detail: Optional[str] = None + submitted_by: Optional[str] = None + created_at: float + updated_at: float + files_total: int + files_done: int + files_failed: int + current_file: Optional[str] = None + use_llm: bool + fetch_images: bool + totals: dict + brands: List[str] + files: List[BatchFileOut] + + +def to_out(manifest: batch_ingest.BatchManifest) -> BatchOut: + body = manifest.to_dict() + body["files"] = [BatchFileOut(**{ + key: entry[key] for key in BatchFileOut.model_fields if key in entry + }) for entry in body["files"]] + return BatchOut(**{k: v for k, v in body.items() if k in BatchOut.model_fields}) + + +# --------------------------------------------------------------------------- +# Staging and queueing +# --------------------------------------------------------------------------- +def stage_and_queue( + valid: List[Tuple[str, bytes, int]], + invalid: List[Tuple[str, str]], + *, + use_llm: bool, + fetch_images: bool, + submitted_by: Optional[str] = None, +) -> Tuple[batch_ingest.BatchManifest, bool]: + """Write the files down, publish the batch, and try to start it. + + Returns `(manifest, started)`. `started` is False only when the worker queue + was full: the batch is staged and durable either way, and the caller decides + what to say about it - an admin has a Resume button, an API client does not, + and the two deserve different words for the same 429. + """ + manifest = batch_ingest.stage_batch( + [(name, contents) for name, contents, _n in valid], + use_llm=use_llm, + fetch_images=fetch_images, + invalid=invalid, + submitted_by=submitted_by, + ) + # stage_batch records size but not row counts; it never parsed the files. + for entry, (_name, _contents, rows) in zip(manifest.files, valid): + entry.rows_total = rows + batch_ingest.write_manifest(manifest) + batch_job_store.put(manifest) + + try: + batch_worker.submit(manifest.batch_id) + except queue.Full: + manifest.status = batch_ingest.QUEUED + manifest.detail = ( + "The ingestion queue was full when this batch arrived. It is staged " + "and can be started with Resume once the running batches finish." + ) + batch_ingest.write_manifest(manifest) + batch_job_store.put(manifest) + return manifest, False + + return manifest, True diff --git a/app/api/routers/admin_train.py b/app/api/routers/admin_train.py index dd505b0..987dd1d 100644 --- a/app/api/routers/admin_train.py +++ b/app/api/routers/admin_train.py @@ -3,7 +3,7 @@ from __future__ import annotations import io import logging -from typing import Any, Dict, List, Optional +from typing import List, Optional import pandas as pd from pydantic import BaseModel, Field from fastapi import APIRouter, Depends, File, HTTPException, UploadFile @@ -12,7 +12,6 @@ from app.api.deps import require_permission from app.infrastructure.settings import S3_BUCKET from app.services.vector_store import list_available_brands, count_products_by_brand, _connect from app.services.s3_service import s3_service -from app.services import store_db logger = logging.getLogger(__name__) router = APIRouter(prefix="/admin/training", tags=["admin_train"]) diff --git a/app/api/routers/batch_catalog.py b/app/api/routers/batch_catalog.py index dac52fb..3780b5d 100644 --- a/app/api/routers/batch_catalog.py +++ b/app/api/routers/batch_catalog.py @@ -21,19 +21,26 @@ image stage, a Playwright subprocess that can burn three minutes on its own - on a single-vCPU container that is also serving the API. So a batch opts IN to those stages; it does not opt out. `USE_OLLAMA` is false in production anyway, which makes `use_llm` a no-op there and the honest default obvious. + +THE OTHER WAY INTO THE SAME PIPELINE +------------------------------------ +`app/api/routers/uploads.py` exposes ingestion to outside API clients under +`upload_catalog` rather than `require_admin`. It stages and queues through the +identical helpers (`app/api/batch_common.py`) and produces ordinary batches, +so everything here - the list, resume, cancel - applies to those too. The only +difference is that its reads are filtered to the caller's own submissions. """ from __future__ import annotations -import logging import queue -from typing import List, Optional +from typing import List from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, status -from pydantic import BaseModel +from app.api import batch_common from app.api.batch_job_store import batch_job_store from app.api.deps import require_admin -from app.core import batch_ingest, batch_worker, inbox +from app.core import batch_ingest, batch_worker from app.core import store_catalog_pipeline as pipeline from app.infrastructure.settings import ( BATCH_MAX_FILES, @@ -41,7 +48,6 @@ from app.infrastructure.settings import ( BATCH_MAX_TOTAL_ROWS, ) -logger = logging.getLogger(__name__) router = APIRouter(prefix="/admin/catalog-batch", tags=["admin", "catalog"]) # Per-file ceilings match store_catalog.py exactly. A file that is too big for @@ -51,152 +57,23 @@ MAX_UPLOAD_BYTES = 10 * 1024 * 1024 MAX_UPLOAD_ROWS = 2000 PREVIEW_ROWS = 10 -# The worker cannot import the API layer without a cycle, so the wiring is done -# here, at import, once. -batch_worker.configure( - on_change=batch_job_store.put, - should_cancel=batch_job_store.is_cancelled, -) +# The response shape, the upload readers and the staging helper are shared with +# app/api/routers/uploads.py - see app/api/batch_common.py, which also wires the +# worker to the job store at import. +BatchFileOut = batch_common.BatchFileOut +BatchOut = batch_common.BatchOut +_to_out = batch_common.to_out -class BatchFileOut(BaseModel): - index: int - filename: str - status: str - detail: Optional[str] = None - stage_index: int = 0 - stage_name: str = "" - total_stages: int = pipeline.TOTAL_STAGES - rows_done: int = 0 - rows_total: int = 0 - size_bytes: int = 0 - result: Optional[dict] = None - - -class BatchOut(BaseModel): - batch_id: str - status: str - detail: Optional[str] = None - submitted_by: Optional[str] = None - created_at: float - updated_at: float - files_total: int - files_done: int - files_failed: int - current_file: Optional[str] = None - use_llm: bool - fetch_images: bool - totals: dict - brands: List[str] - files: List[BatchFileOut] - - -def _to_out(manifest: batch_ingest.BatchManifest) -> BatchOut: - body = manifest.to_dict() - body["files"] = [BatchFileOut(**{ - key: entry[key] for key in BatchFileOut.model_fields if key in entry - }) for entry in body["files"]] - return BatchOut(**{k: v for k, v in body.items() if k in BatchOut.model_fields}) - - -async def _read_uploads(files: List[UploadFile]) -> List[tuple]: - """Read every upload into memory, enforcing the count and size ceilings. - - Read here rather than in the worker for the same reason `store_catalog.py` - gives: `UploadFile` is backed by a temporary file tied to the request, and - it is gone before a background thread would reach it. - """ - if not files: - raise HTTPException(status_code=400, detail="No files were uploaded.") - if len(files) > BATCH_MAX_FILES: - raise HTTPException( - status_code=413, - detail=( - f"{len(files)} files exceeds the {BATCH_MAX_FILES}-file limit for one " - f"batch. Split the drop and upload it in two batches." - ), - ) - - read: List[tuple] = [] - total = 0 - for upload in files: - contents = await upload.read() - name = upload.filename or "upload.xlsx" - if not contents: - # Recorded rather than raised - an empty file among nine good ones - # is a fact about that file, not a reason to reject the drop. - read.append((name, b"")) - continue - if len(contents) > MAX_UPLOAD_BYTES: - raise HTTPException( - status_code=413, - detail=( - f"'{name}' is larger than the " - f"{MAX_UPLOAD_BYTES // (1024 * 1024)}MB per-file limit." - ), - ) - total += len(contents) - if total > BATCH_MAX_TOTAL_BYTES: - raise HTTPException( - status_code=413, - detail=( - f"The batch is larger than the " - f"{BATCH_MAX_TOTAL_BYTES // (1024 * 1024)}MB total limit." - ), - ) - read.append((name, contents)) - return read - - -def _parse_all(read: List[tuple]): - """Split the uploads into (valid, invalid) by trying to parse each one. - - Failing fast here is what stops a batch transitioning straight to "failed" - a second after it started - the same reasoning as `store_catalog.py:147`, - applied per file so that one bad sheet does not condemn the others. - """ - valid: List[tuple] = [] - invalid: List[tuple] = [] - rows_total = 0 - - for name, contents in read: - if not contents: - invalid.append((name, "The file is empty.")) - continue - try: - df, _mapping = pipeline.parse_spreadsheet(name, contents) - except HTTPException as exc: - # read_products_dataframe raises HTTPException for an unsupported - # extension or a missing Excel reader; its message already names - # the file and what to do about it. - invalid.append((name, str(exc.detail))) - continue - except Exception as exc: # noqa: BLE001 - an unreadable sheet is user error - invalid.append((name, f"Could not parse the file: {exc}")) - continue - - if df.empty: - invalid.append((name, "The file has no data rows.")) - continue - if len(df) > MAX_UPLOAD_ROWS: - invalid.append(( - name, - f"{len(df)} rows exceeds the {MAX_UPLOAD_ROWS}-row per-file limit.", - )) - continue - - rows_total += int(len(df)) - if rows_total > BATCH_MAX_TOTAL_ROWS: - raise HTTPException( - status_code=413, - detail=( - f"The batch totals more than {BATCH_MAX_TOTAL_ROWS} rows. " - f"Split it and upload in two batches." - ), - ) - valid.append((name, contents, int(len(df)))) - - return valid, invalid, rows_total +def _limits() -> batch_common.UploadLimits: + """Read at call time, from THIS module's globals - see batch_common.""" + return batch_common.UploadLimits( + max_files=BATCH_MAX_FILES, + max_file_bytes=MAX_UPLOAD_BYTES, + max_file_rows=MAX_UPLOAD_ROWS, + max_total_bytes=BATCH_MAX_TOTAL_BYTES, + max_total_rows=BATCH_MAX_TOTAL_ROWS, + ) @router.post("/preview", dependencies=[Depends(require_admin)]) @@ -207,7 +84,7 @@ async def preview_catalog_batch(files: List[UploadFile] = File(...)) -> dict: headers onto catalog fields is a guess, and finding out that "Item" was read as the description after twenty files have been scraped is expensive. """ - read = await _read_uploads(files) + read = await batch_common.read_uploads(files, _limits()) out = [] for name, contents in read: if not contents: @@ -267,8 +144,9 @@ async def ingest_catalog_batch( on a different host to the frontend, so anything that sat on the request path would be racing an idle timeout nobody here controls. """ - read = await _read_uploads(files) - valid, invalid, _rows = _parse_all(read) + limits = _limits() + read = await batch_common.read_uploads(files, limits) + valid, invalid, _rows = batch_common.parse_all(read, limits) if not valid: detail = "; ".join(f"{name}: {reason}" for name, reason in invalid) @@ -277,27 +155,10 @@ async def ingest_catalog_batch( detail=f"None of the uploaded files could be ingested. {detail}", ) - manifest = batch_ingest.stage_batch( - [(name, contents) for name, contents, _n in valid], - use_llm=use_llm, - fetch_images=fetch_images, - invalid=invalid, + manifest, started = batch_common.stage_and_queue( + valid, invalid, use_llm=use_llm, fetch_images=fetch_images, ) - for entry, (_name, _contents, rows) in zip(manifest.files, valid): - entry.rows_total = rows - batch_ingest.write_manifest(manifest) - batch_job_store.put(manifest) - - try: - batch_worker.submit(manifest.batch_id) - except queue.Full: - manifest.status = batch_ingest.QUEUED - manifest.detail = ( - "The ingestion queue is full. This batch is staged and can be started " - "with Resume once the running batches finish." - ) - batch_ingest.write_manifest(manifest) - batch_job_store.put(manifest) + if not started: raise HTTPException( status_code=429, detail=( @@ -309,139 +170,6 @@ async def ingest_catalog_batch( return _to_out(manifest) -# --------------------------------------------------------------------------- -# The review inbox: files a colleague dropped, waiting for a decision -# --------------------------------------------------------------------------- -# These are the admin half of the two-actor flow. The uploader half lives in -# app/api/routers/uploads.py and can reach none of this. - - -class InboxSelection(BaseModel): - file_ids: List[str] - use_llm: bool = False - fetch_images: bool = False - - -class InboxDismissal(BaseModel): - file_ids: List[str] - - -@router.get("/inbox", dependencies=[Depends(require_admin)]) -def list_inbox() -> dict: - """Files awaiting review, grouped by the drop they arrived in. - - `pending_count` is what the badge renders, and it is computed here rather - than by summing the response client-side so the two can never disagree. - """ - submissions = inbox.list_pending() - return { - "pending_count": sum(len(s.pending_files) for s in submissions), - "submissions": [ - { - "submission_id": s.submission_id, - "submitted_by": s.submitted_by, - "created_at": s.created_at, - "files": [ - { - "file_id": f.file_id, - "filename": f.filename, - "rows_total": f.rows_total, - "size_bytes": f.size_bytes, - } - for f in s.pending_files - ], - } - for s in submissions - ], - } - - -@router.post("/from-inbox", status_code=status.HTTP_202_ACCEPTED, - dependencies=[Depends(require_admin)]) -def start_batch_from_inbox(selection: InboxSelection) -> BatchOut: - """Compose a batch out of the selected inbox files and start it. - - The files may come from different drops on different days; that is the - point of selecting per file rather than per submission. From here on this - is an ordinary batch and every existing path - progress, resume, cancel - - applies unchanged. - """ - if not selection.file_ids: - raise HTTPException(status_code=400, detail="No files were selected.") - - try: - uploads, submitters = inbox.collect_for_batch(selection.file_ids) - except KeyError as exc: - raise HTTPException( - status_code=404, detail=f"No such file in the inbox: {exc.args[0]}" - ) from exc - except ValueError as exc: - # Two admin tabs open on the same inbox. Tell the second one what - # happened rather than silently running the file a second time. - raise HTTPException(status_code=409, detail=str(exc)) from exc - - # Re-parse rather than trusting the row counts recorded at upload: the - # ceilings are a property of the batch about to run, not of the drops it - # was assembled from, and a selection can span any number of drops. - read = [(name, contents) for name, contents in uploads] - valid, invalid, _rows = _parse_all(read) - if not valid: - detail = "; ".join(f"{name}: {reason}" for name, reason in invalid) - raise HTTPException( - status_code=400, - detail=f"None of the selected files could be ingested. {detail}", - ) - - manifest = batch_ingest.stage_batch( - [(name, contents) for name, contents, _n in valid], - use_llm=selection.use_llm, - fetch_images=selection.fetch_images, - invalid=invalid, - ) - for entry, (_name, _contents, rows) in zip(manifest.files, valid): - entry.rows_total = rows - manifest.submitted_by = ", ".join(submitters) or None - batch_ingest.write_manifest(manifest) - batch_job_store.put(manifest) - - try: - batch_worker.submit(manifest.batch_id) - except queue.Full: - manifest.detail = ( - "The ingestion queue is full. This batch is staged and can be started " - "with Resume once the running batches finish." - ) - batch_ingest.write_manifest(manifest) - batch_job_store.put(manifest) - raise HTTPException( - status_code=429, - detail=( - "Too many batches are already queued. This selection has been staged - " - "press Resume on it once the current batch finishes." - ), - ) - - # Only now, once the batch exists AND is queued. Marking first would drop - # the files out of the inbox with nothing left to retry from if staging - # had then failed. - inbox.mark_consumed(selection.file_ids, manifest.batch_id) - return _to_out(manifest) - - -@router.post("/inbox/dismiss", dependencies=[Depends(require_admin)]) -def dismiss_inbox_files(dismissal: InboxDismissal) -> dict: - """Mark files as never-to-run, so the badge can reach zero.""" - if not dismissal.file_ids: - raise HTTPException(status_code=400, detail="No files were selected.") - changed = inbox.dismiss(dismissal.file_ids) - if not changed: - raise HTTPException( - status_code=409, - detail="None of those files were still awaiting review.", - ) - return {"dismissed": changed, "pending_count": inbox.pending_count()} - - @router.get("/batches", dependencies=[Depends(require_admin)]) def list_catalog_batches(limit: int = 20) -> dict: limit = max(1, min(limit, 100)) diff --git a/app/api/routers/system.py b/app/api/routers/system.py index 323db7e..227dba6 100644 --- a/app/api/routers/system.py +++ b/app/api/routers/system.py @@ -3,7 +3,6 @@ from __future__ import annotations import logging import os import subprocess -import threading from pathlib import Path from typing import Any, Dict diff --git a/app/api/routers/upload.py b/app/api/routers/upload.py index 0721212..d5a9364 100644 --- a/app/api/routers/upload.py +++ b/app/api/routers/upload.py @@ -1,7 +1,43 @@ +""" +Operator spreadsheet uploads: store inventory, sales history, nutrition facts. + + POST /api/upload/stores (+ /stores/upload) + POST /api/upload/analytics (+ /analytics/upload) + POST /api/upload/nutrition (+ /nutrition/upload) + GET /api/upload/template/{tab_type} + +These are the endpoints the Admin UI's upload tabs call, through +`/api/upload/${tabType}`. + +WHY NOTHING HERE INVENTS A VALUE +-------------------------------- +Every reader in this module returns `None` for an absent column, and every +writer either stores NULL or skips the row. That is a deliberate correction of +how this file used to work: it filled a missing column with a plausible +constant, so a spreadsheet whose headers did not match silently produced rows +attributed to brand "amul" in store "store_mumbai_1" at MRP 100 - and, worse, +nutrition rows carrying invented calories and `allergens = ['None']` stamped +`data_status = 'verified'`. + +That last one is the reason this rule is absolute rather than a preference. +`app/services/nutrition_db.py` states the contract these tables are built on: + + Every numeric column in `nutrition_facts` is nullable and stays NULL + unless a value was actually returned by a trusted source. Nothing in + this module ever writes an estimated, interpolated, or LLM-guessed + number into these columns. + +A missing allergens column means "we were not told", never "this product is +allergen-free" - and the two are indistinguishable once a default has been +written. So a row that lacks the fields identifying it is REPORTED BACK to the +uploader, per row, rather than repaired into something importable. + +Arithmetic on values that were supplied is not invention: `line_total` may be +derived from `quantity * unit_price`, because both were given. +""" from __future__ import annotations import io -import json import uuid import logging import pandas as pd @@ -18,6 +54,10 @@ from app.services import store_db, nutrition_db logger = logging.getLogger(__name__) router = APIRouter(prefix="/upload", tags=["upload"]) +# A caller who sent a 2000-row sheet with the wrong headers does not need 2000 +# identical messages to understand what went wrong. +MAX_REPORTED_ERRORS = 50 + def _normalize_col(col: str) -> str: """Normalize dataframe column names (lower, strip, replace spaces/hyphens with underscore).""" @@ -36,39 +76,114 @@ def read_df_from_upload(filename: str, contents: bytes) -> pd.DataFrame: df = pd.read_csv(io.BytesIO(contents)) except Exception: df = pd.read_csv(io.BytesIO(contents), sep=None, engine='python') - + # Rename columns to normalized format df.columns = [_normalize_col(c) for c in df.columns] return df -def _get_str(row: dict, keys: List[str], default: str = "") -> str: +# --------------------------------------------------------------------------- +# Cell readers +# --------------------------------------------------------------------------- +# All three return None for "this row did not carry the value", which every +# caller below is required to handle explicitly. There is deliberately no +# `default=` parameter: that parameter is what made a missing column look like +# data, and adding it back would reintroduce the bug this module documents. +def _opt_str(row: dict, keys: List[str]) -> Optional[str]: for k in keys: if k in row and pd.notna(row[k]): val = str(row[k]).strip() if val: return val - return default + return None -def _get_float(row: dict, keys: List[str], default: float = 0.0) -> float: +def _opt_float(row: dict, keys: List[str]) -> Optional[float]: for k in keys: if k in row and pd.notna(row[k]): try: return float(row[k]) except (ValueError, TypeError): pass - return default + return None -def _get_int(row: dict, keys: List[str], default: int = 0) -> int: +def _opt_int(row: dict, keys: List[str]) -> Optional[int]: for k in keys: if k in row and pd.notna(row[k]): try: return int(float(row[k])) except (ValueError, TypeError): pass - return default + return None + + +def _slug_id(brand: str, product_name: str) -> str: + """A deterministic image_id for a row that did not carry one. + + Derived entirely from values the sheet supplied, so it is a formatting + decision rather than an invented fact, and the same product re-uploaded + lands on the same key instead of duplicating. + """ + return f"{brand}_{product_name.lower().replace(' ', '_')}" + + +def _record(errors: List[Dict[str, Any]], index: Any, message: str) -> None: + """Note why a row was skipped. Row numbers are as the uploader sees them: + 1-based, counting the header, which is what their spreadsheet shows.""" + if len(errors) < MAX_REPORTED_ERRORS: + try: + row_no = int(index) + 2 + except (TypeError, ValueError): + row_no = -1 + errors.append({"row": row_no, "error": message}) + + +def _finish( + filename: Optional[str], + rows_total: int, + imported: int, + errors: List[Dict[str, Any]], + skipped: int, + noun: str, + **extra: Any, +) -> Dict[str, Any]: + """Build the response, or refuse the upload if nothing at all landed. + + Nothing imported is a 422 rather than a 200 with `rows_imported: 0`. The + old shape reported success for a file that stored not one row, which is how + a header mismatch went unnoticed for as long as it did. + """ + body = { + "status": "success" if skipped == 0 else "partial", + "filename": filename, + "rows_total": rows_total, + "rows_imported": imported, + "rows_skipped": skipped, + "errors": errors, + **extra, + } + if imported == 0: + raise HTTPException( + status_code=422, + detail={ + "message": ( + f"No {noun} could be imported from '{filename}'. " + f"Check the column headers against the sample template " + f"(GET /api/upload/template/...)." + ), + "rows_total": rows_total, + "errors": errors, + }, + ) + if skipped: + body["message"] = ( + f"Imported {imported} {noun}; skipped {skipped} row(s) that were " + f"missing required fields - see 'errors'." + ) + else: + body["message"] = f"Successfully imported {imported} {noun}." + return body # --------------------------------------------------------------------------- @@ -79,69 +194,106 @@ def _get_int(row: dict, keys: List[str], default: int = 0) -> int: @router.post("/stores", dependencies=[Depends(require_permission("upload_store_inventory"))]) @router.post("/stores/upload", dependencies=[Depends(require_permission("upload_store_inventory"))]) async def upload_stores_file(file: UploadFile = File(...)) -> Dict[str, Any]: + """Import store inventory, and prices where the sheet carries them. + + `store_id`, `brand` and `product_name` identify the row and are required; + a row without them is skipped and reported rather than filed under a + default store and brand. + + Stock levels fall back to 0 - the column default the schema itself + declares - because `store_inventory` cannot hold NULL there. Prices are + different: `store_prices` requires all three of mrp/cost_price/selling_price + NOT NULL, and cost price cannot be derived from anything else on the row, + so a sheet without it gets its inventory imported and its prices left + alone, counted in `prices_skipped`. That is the honest outcome; the + alternative is a margin computed from a cost nobody supplied. + """ if not file.filename: raise HTTPException(status_code=400, detail="No file uploaded") - + contents = await file.read() try: df = read_df_from_upload(file.filename, contents) except Exception as e: raise HTTPException(status_code=400, detail=f"Could not parse Excel/CSV file: {e}") - + if df.empty: raise HTTPException(status_code=400, detail="Uploaded file contains no data rows") - + conn = _connect() if not conn: raise HTTPException(status_code=500, detail="Database connection failed") - + imported_count = 0 + prices_written = 0 + prices_skipped = 0 stores_created = set() - + errors: List[Dict[str, Any]] = [] + skipped = 0 + try: with conn, conn.cursor() as cur: # Ensure tables exist store_db.ensure_store_intelligence_schema() - - for _, r in df.iterrows(): + + for index, r in df.iterrows(): row = r.to_dict() - store_id = _get_str(row, ['store_id', 'store'], 'store_mumbai_1') - brand = _get_str(row, ['brand', 'brand_name'], 'amul').lower() - product_name = _get_str(row, ['product_name', 'title', 'name', 'item'], 'Product Item') - image_id = _get_str(row, ['image_id', 'sku', 'product_sku', 'item_id'], '') - if not image_id: - image_id = f"{brand}_{product_name.lower().replace(' ', '_')}" - - category = _get_str(row, ['category', 'cat'], 'Dairy') - avail_stock = _get_int(row, ['available_stock', 'stock', 'qty', 'quantity'], 50) - reserved_stock = _get_int(row, ['reserved_stock', 'reserved'], 0) - reorder_lvl = _get_int(row, ['reorder_level', 'reorder'], 15) - safety_stk = _get_int(row, ['safety_stock', 'safety'], 10) - - mrp = _get_float(row, ['mrp', 'price'], 100.0) - cost_price = _get_float(row, ['cost_price', 'cost'], 70.0) - selling_price = _get_float(row, ['selling_price', 'sell_price'], mrp * 0.9 if mrp else 90.0) - - # 1. Ensure store exists + store_id = _opt_str(row, ['store_id', 'store']) + brand = _opt_str(row, ['brand', 'brand_name']) + product_name = _opt_str(row, ['product_name', 'title', 'name', 'item']) + + missing = [ + label for label, value in ( + ("store_id", store_id), ("brand", brand), ("product_name", product_name), + ) if not value + ] + if missing: + skipped += 1 + _record(errors, index, f"missing required field(s): {', '.join(missing)}") + continue + + brand = brand.lower() + image_id = _opt_str(row, ['image_id', 'sku', 'product_sku', 'item_id']) \ + or _slug_id(brand, product_name) + + category = _opt_str(row, ['category', 'cat']) + city = _opt_str(row, ['city', 'store_city']) + store_name = _opt_str(row, ['store_name']) or store_id.replace('_', ' ').title() + + # NOT NULL with a schema default of 0. Absent means "not + # counted", which 0 represents as faithfully as anything can. + avail_stock = _opt_int(row, ['available_stock', 'stock', 'qty', 'quantity']) or 0 + reserved_stock = _opt_int(row, ['reserved_stock', 'reserved']) or 0 + reorder_lvl = _opt_int(row, ['reorder_level', 'reorder']) or 0 + safety_stk = _opt_int(row, ['safety_stock', 'safety']) or 0 + + mrp = _opt_float(row, ['mrp', 'price']) + cost_price = _opt_float(row, ['cost_price', 'cost']) + selling_price = _opt_float(row, ['selling_price', 'sell_price']) + + # 1. Ensure store exists. Only the id and a display name are + # asserted; city/tier/footfall stay at their schema defaults + # unless the sheet said otherwise. cur.execute( """ - INSERT INTO stores (store_id, store_name, city, tier, footfall_index) - VALUES (%s, %s, %s, %s, %s) - ON CONFLICT (store_id) DO NOTHING + INSERT INTO stores (store_id, store_name, city) + VALUES (%s, %s, %s) + ON CONFLICT (store_id) DO UPDATE SET + city = COALESCE(EXCLUDED.city, stores.city) """, - (store_id, store_id.replace('_', ' ').title(), 'Mumbai', 'standard', 25.0) + (store_id, store_name, city) ) stores_created.add(store_id) - + # 2. Upsert store_inventory cur.execute( """ - INSERT INTO store_inventory + INSERT INTO store_inventory (store_id, brand, image_id, title, category, available_stock, reserved_stock, reorder_level, safety_stock) VALUES (%s, %s, %s, %s, %s, %s, %s, %s, %s) ON CONFLICT (store_id, brand, image_id) DO UPDATE SET title = EXCLUDED.title, - category = EXCLUDED.category, + category = COALESCE(EXCLUDED.category, store_inventory.category), available_stock = EXCLUDED.available_stock, reserved_stock = EXCLUDED.reserved_stock, reorder_level = EXCLUDED.reorder_level, @@ -150,36 +302,41 @@ async def upload_stores_file(file: UploadFile = File(...)) -> Dict[str, Any]: """, (store_id, brand, image_id, product_name, category, avail_stock, reserved_stock, reorder_lvl, safety_stk) ) - - # 3. Upsert store_prices - cur.execute( - """ - INSERT INTO store_prices (store_id, brand, image_id, mrp, cost_price, selling_price) - VALUES (%s, %s, %s, %s, %s, %s) - ON CONFLICT (store_id, brand, image_id) DO UPDATE SET - mrp = EXCLUDED.mrp, - cost_price = EXCLUDED.cost_price, - selling_price = EXCLUDED.selling_price, - updated_at = CURRENT_TIMESTAMP - """, - (store_id, brand, image_id, mrp, cost_price, selling_price) - ) + + # 3. Upsert store_prices - only with a complete price triple. + if mrp is not None and cost_price is not None and selling_price is not None: + cur.execute( + """ + INSERT INTO store_prices (store_id, brand, image_id, mrp, cost_price, selling_price) + VALUES (%s, %s, %s, %s, %s, %s) + ON CONFLICT (store_id, brand, image_id) DO UPDATE SET + mrp = EXCLUDED.mrp, + cost_price = EXCLUDED.cost_price, + selling_price = EXCLUDED.selling_price, + updated_at = CURRENT_TIMESTAMP + """, + (store_id, brand, image_id, mrp, cost_price, selling_price) + ) + prices_written += 1 + else: + prices_skipped += 1 + imported_count += 1 - + + except HTTPException: + raise except Exception as e: logger.error("Stores upload failed: %s", e) raise HTTPException(status_code=500, detail=f"Database import failed: {e}") finally: conn.close() - - return { - "status": "success", - "filename": file.filename, - "rows_total": len(df), - "rows_imported": imported_count, - "stores_affected": list(stores_created), - "message": f"Successfully imported {imported_count} store inventory items across {len(stores_created)} store(s)." - } + + return _finish( + file.filename, len(df), imported_count, errors, skipped, "store inventory items", + stores_affected=sorted(stores_created), + prices_written=prices_written, + prices_skipped=prices_skipped, + ) # --------------------------------------------------------------------------- @@ -188,244 +345,407 @@ async def upload_stores_file(file: UploadFile = File(...)) -> Dict[str, Any]: @router.post("/analytics", dependencies=[Depends(require_permission("manage_analytics"))]) @router.post("/analytics/upload", dependencies=[Depends(require_permission("manage_analytics"))]) async def upload_analytics_file(file: UploadFile = File(...)) -> Dict[str, Any]: + """Import sales transactions into `orders` / `order_items`. + + THE COLUMN NAMES HERE WERE WRONG AND THE ENDPOINT NEVER WORKED. + The insert named `total_price`, which is not a column on `order_items` + (it is `line_total` - see store_db.py, which writes the same table), and it + omitted `store_id`, which is NOT NULL. Every call therefore raised, was + swallowed by the except below, and came back as a flat + 500 "Database import failed". The input column may still be spelled + `total_price` in a customer's sheet - that alias is kept - but it is stored + in `line_total`, which is the column every analytics reader actually sums + (`intelligence/analytics.py`, `trending_model.py`, `features.py`). + + Re-uploading the same file appends its lines again: `order_items` has a + surrogate key and no natural uniqueness to conflict on. Import each file + once, or give the rows stable `order_id`s and clear them first. + + `customer_id` is required rather than defaulted. It used to fall back to a + single shared 'cust_imported', which silently merges every buyer in the + file into one customer and corrupts exactly the per-customer models - + purchase propensity, engagement - that this table exists to feed. + """ if not file.filename: raise HTTPException(status_code=400, detail="No file uploaded") - + contents = await file.read() try: df = read_df_from_upload(file.filename, contents) except Exception as e: raise HTTPException(status_code=400, detail=f"Could not parse Excel/CSV file: {e}") - + if df.empty: raise HTTPException(status_code=400, detail="Uploaded file contains no data rows") - + conn = _connect() if not conn: raise HTTPException(status_code=500, detail="Database connection failed") - + imported_orders = 0 total_revenue = 0.0 - + errors: List[Dict[str, Any]] = [] + skipped = 0 + dates_defaulted = 0 + touched_orders: List[str] = [] + try: with conn, conn.cursor() as cur: store_db.ensure_store_intelligence_schema() - - for _, r in df.iterrows(): + + for index, r in df.iterrows(): row = r.to_dict() - store_id = _get_str(row, ['store_id', 'store'], 'store_mumbai_1') - brand = _get_str(row, ['brand', 'brand_name'], 'amul').lower() - image_id = _get_str(row, ['image_id', 'sku', 'product_sku'], '') - product_name = _get_str(row, ['product_name', 'title', 'item'], 'Analytics Item') - if not image_id: - image_id = f"{brand}_{product_name.lower().replace(' ', '_')}" - - order_id = _get_str(row, ['order_id', 'transaction_id'], f"ord_up_{uuid.uuid4().hex[:8]}") - customer_id = _get_str(row, ['customer_id', 'user_id', 'customer'], 'cust_imported') - - raw_date = _get_str(row, ['order_date', 'date', 'timestamp'], '') - order_date = datetime.now() + store_id = _opt_str(row, ['store_id', 'store']) + brand = _opt_str(row, ['brand', 'brand_name']) + product_name = _opt_str(row, ['product_name', 'title', 'item']) + image_id = _opt_str(row, ['image_id', 'sku', 'product_sku']) + customer_id = _opt_str(row, ['customer_id', 'user_id', 'customer']) + qty = _opt_int(row, ['quantity', 'units_sold', 'qty', 'count']) + unit_price = _opt_float(row, ['unit_price', 'selling_price', 'price']) + + missing = [ + label for label, value in ( + ("store_id", store_id), + ("brand", brand), + ("customer_id", customer_id), + ("quantity", qty), + ("unit_price", unit_price), + ) if value is None or value == "" + ] + if not image_id and not product_name: + missing.append("image_id or product_name") + if missing: + skipped += 1 + _record(errors, index, f"missing required field(s): {', '.join(missing)}") + continue + + brand = brand.lower() + image_id = image_id or _slug_id(brand, product_name) + + # A surrogate key, not a fact about the sale: a sheet without + # order ids is one row per order, which is what this generates. + order_id = _opt_str(row, ['order_id', 'transaction_id']) \ + or f"ord_up_{uuid.uuid4().hex[:8]}" + + raw_date = _opt_str(row, ['order_date', 'date', 'timestamp']) + order_date = None if raw_date: try: order_date = pd.to_datetime(raw_date).to_pydatetime() except Exception: - pass - - qty = _get_int(row, ['quantity', 'units_sold', 'qty', 'count'], 1) - unit_price = _get_float(row, ['unit_price', 'selling_price', 'price'], 100.0) - tot_price = _get_float(row, ['total_price', 'revenue', 'total'], qty * unit_price) - - # Ensure store exists + order_date = None + if order_date is None: + # orders.order_date is NOT NULL. Import time is the only + # defensible stand-in, and it is counted so the response can + # say how much of the file is not really dated. + order_date = datetime.now() + dates_defaulted += 1 + + # Arithmetic over supplied values, not invention. + line_total = _opt_float(row, ['total_price', 'line_total', 'revenue', 'total']) + if line_total is None: + line_total = qty * unit_price + + payment_method = _opt_str(row, ['payment_method', 'payment']) + delivery_status = _opt_str(row, ['delivery_status', 'status']) + cur.execute( - "INSERT INTO stores (store_id, store_name, city, tier, footfall_index) VALUES (%s, %s, %s, %s, %s) ON CONFLICT (store_id) DO NOTHING", - (store_id, store_id.replace('_', ' ').title(), 'Mumbai', 'standard', 25.0) + """ + INSERT INTO stores (store_id, store_name) + VALUES (%s, %s) + ON CONFLICT (store_id) DO NOTHING + """, + (store_id, store_id.replace('_', ' ').title()) ) - - # Insert order header + cur.execute( """ INSERT INTO orders (order_id, customer_id, store_id, order_date, payment_method, order_value, delivery_status) VALUES (%s, %s, %s, %s, %s, %s, %s) - ON CONFLICT (order_id) DO UPDATE SET order_value = EXCLUDED.order_value + ON CONFLICT (order_id) DO NOTHING """, - (order_id, customer_id, store_id, order_date, 'upi', tot_price, 'delivered') + (order_id, customer_id, store_id, order_date, payment_method, 0, delivery_status) ) - - # Insert order item + cur.execute( """ - INSERT INTO order_items (order_id, brand, image_id, quantity, unit_price, total_price) - VALUES (%s, %s, %s, %s, %s, %s) + INSERT INTO order_items + (order_id, store_id, brand, image_id, quantity, unit_price, line_total) + VALUES (%s, %s, %s, %s, %s, %s, %s) """, - (order_id, brand, image_id, qty, unit_price, tot_price) + (order_id, store_id, brand, image_id, qty, unit_price, line_total) ) - + + touched_orders.append(order_id) imported_orders += 1 - total_revenue += tot_price - + total_revenue += line_total + + # order_value is the sum of the order's lines, so it is recomputed + # from order_items rather than accumulated per row. A multi-line + # order previously ended up carrying only its last line's value. + if touched_orders: + cur.execute( + """ + UPDATE orders o + SET order_value = s.total + FROM ( + SELECT order_id, SUM(line_total) AS total + FROM order_items + WHERE order_id = ANY(%s) + GROUP BY order_id + ) s + WHERE o.order_id = s.order_id + """, + (list(set(touched_orders)),) + ) + + except HTTPException: + raise except Exception as e: logger.error("Analytics upload failed: %s", e) raise HTTPException(status_code=500, detail=f"Database import failed: {e}") finally: conn.close() - - return { - "status": "success", - "filename": file.filename, - "rows_total": len(df), - "rows_imported": imported_orders, - "total_revenue": round(total_revenue, 2), - "message": f"Successfully imported {imported_orders} sales transactions (Total Revenue: ₹{total_revenue:,.2f})." - } + + return _finish( + file.filename, len(df), imported_orders, errors, skipped, "sales transactions", + total_revenue=round(total_revenue, 2), + dates_defaulted_to_now=dates_defaulted, + ) # --------------------------------------------------------------------------- # Nutrition Intelligence Excel / CSV Upload # --------------------------------------------------------------------------- +# The per-100g fields the "high protein" / "low sugar" endpoints sort on. Which +# of these a row carries decides its data_status, so the list is named once +# rather than repeated in the status logic. +_CORE_NUTRIENTS = ( + "calories_kcal", "protein_g", "carbohydrates_g", "total_sugar_g", + "dietary_fiber_g", "total_fat_g", "sodium_mg", +) + + @router.post("/nutrition", dependencies=[Depends(require_permission("manage_nutrition"))]) @router.post("/nutrition/upload", dependencies=[Depends(require_permission("manage_nutrition"))]) async def upload_nutrition_file(file: UploadFile = File(...)) -> Dict[str, Any]: + """Import nutrition facts, storing NULL for everything the sheet omitted. + + THIS IS THE ENDPOINT THE DATA-INTEGRITY RULE EXISTS FOR. + It used to substitute a constant for every absent column - 150 kcal, 5g + protein, health_score 78, `diet_tags = ['High Protein', 'Gluten Free']`, + `allergens = ['None']` - and write the result with + `data_status = 'verified'`. A sheet with no allergens column therefore + asserted, verified, that every product in it was allergen-free, and the + public /api/nutrition endpoints served that. + + Now: absent means NULL, `data_status` reflects what the row actually + carried ('verified' only with the full core set, else 'partial', else + 'unavailable'), and `allergen_source` records 'upload' or 'unavailable' so + a reader can tell "no allergens" from "not told". The upserts COALESCE, so + a later sheet that omits a column can never blank a value an earlier + trusted source established, and a row already 'verified' is never + downgraded by a thinner upload. + """ if not file.filename: raise HTTPException(status_code=400, detail="No file uploaded") - + contents = await file.read() try: df = read_df_from_upload(file.filename, contents) except Exception as e: raise HTTPException(status_code=400, detail=f"Could not parse Excel/CSV file: {e}") - + if df.empty: raise HTTPException(status_code=400, detail="Uploaded file contains no data rows") - + conn = _connect() if not conn: raise HTTPException(status_code=500, detail="Database connection failed") - + imported_count = 0 - + errors: List[Dict[str, Any]] = [] + skipped = 0 + status_counts = {"verified": 0, "partial": 0, "unavailable": 0} + try: with conn, conn.cursor() as cur: nutrition_db.ensure_nutrition_schema() - - for _, r in df.iterrows(): + + for index, r in df.iterrows(): row = r.to_dict() - brand = _get_str(row, ['brand', 'brand_name'], 'amul').lower() - product_name = _get_str(row, ['product_name', 'title', 'item', 'name'], 'Nutrition Item') - image_id = _get_str(row, ['image_id', 'sku', 'id'], '') - if not image_id: - image_id = f"{brand}_{product_name.lower().replace(' ', '_')}" - - category = _get_str(row, ['category', 'cat'], 'Food') - - calories = _get_float(row, ['calories', 'calories_kcal', 'energy'], 150.0) - protein = _get_float(row, ['protein', 'protein_g'], 5.0) - carbs = _get_float(row, ['carbohydrates', 'carbs', 'carbohydrates_g'], 20.0) - sugar = _get_float(row, ['sugar', 'total_sugar_g', 'sugars'], 4.0) - fiber = _get_float(row, ['fiber', 'dietary_fiber_g'], 2.0) - fat = _get_float(row, ['fat', 'total_fat_g'], 6.0) - sodium = _get_float(row, ['sodium', 'sodium_mg'], 120.0) - calcium = _get_float(row, ['calcium', 'calcium_mg'], 80.0) - iron = _get_float(row, ['iron', 'iron_mg'], 1.5) - vitamin_c = _get_float(row, ['vitamin_c', 'vitamin_c_mg'], 5.0) - - health_score = _get_float(row, ['health_score', 'nutrition_score', 'score'], 78.0) - diet_tags_raw = _get_str(row, ['diet_tags', 'tags', 'diet'], 'High Protein, Gluten Free') - allergens_raw = _get_str(row, ['allergens', 'allergen'], 'None') - - diet_tags = [t.strip() for t in diet_tags_raw.split(',') if t.strip()] - allergens = [a.strip() for a in allergens_raw.split(',') if a.strip()] - - # 1. Upsert nutrition_facts + brand = _opt_str(row, ['brand', 'brand_name']) + product_name = _opt_str(row, ['product_name', 'title', 'item', 'name']) + image_id = _opt_str(row, ['image_id', 'sku', 'id']) + + missing = [ + label for label, value in (("brand", brand),) if not value + ] + if not image_id and not product_name: + missing.append("image_id or product_name") + if missing: + skipped += 1 + _record(errors, index, f"missing required field(s): {', '.join(missing)}") + continue + + brand = brand.lower() + image_id = image_id or _slug_id(brand, product_name) + category = _opt_str(row, ['category', 'cat']) + + values = { + "calories_kcal": _opt_float(row, ['calories', 'calories_kcal', 'energy']), + "protein_g": _opt_float(row, ['protein', 'protein_g']), + "carbohydrates_g": _opt_float(row, ['carbohydrates', 'carbs', 'carbohydrates_g']), + "total_sugar_g": _opt_float(row, ['sugar', 'total_sugar_g', 'sugars']), + "dietary_fiber_g": _opt_float(row, ['fiber', 'dietary_fiber_g']), + "total_fat_g": _opt_float(row, ['fat', 'total_fat_g']), + "sodium_mg": _opt_float(row, ['sodium', 'sodium_mg']), + "calcium_mg": _opt_float(row, ['calcium', 'calcium_mg']), + "iron_mg": _opt_float(row, ['iron', 'iron_mg']), + "vitamin_c_mg": _opt_float(row, ['vitamin_c', 'vitamin_c_mg']), + } + + present_core = [k for k in _CORE_NUTRIENTS if values.get(k) is not None] + if len(present_core) == len(_CORE_NUTRIENTS): + data_status = "verified" + elif present_core: + data_status = "partial" + else: + data_status = "unavailable" + status_counts[data_status] += 1 + cur.execute( """ INSERT INTO nutrition_facts (brand, image_id, product_name, category, data_status, data_source, calories_kcal, protein_g, carbohydrates_g, total_sugar_g, dietary_fiber_g, - total_fat_g, sodium_mg, calcium_mg, iron_mg, vitamin_c_mg) - VALUES (%s, %s, %s, %s, 'verified', 'excel_upload', %s, %s, %s, %s, %s, %s, %s, %s, %s, %s) + total_fat_g, sodium_mg, calcium_mg, iron_mg, vitamin_c_mg, updated_at) + VALUES (%s, %s, %s, %s, %s, 'excel_upload', + %s, %s, %s, %s, %s, %s, %s, %s, %s, %s, CURRENT_TIMESTAMP) ON CONFLICT (brand, image_id) DO UPDATE SET - product_name = EXCLUDED.product_name, - category = EXCLUDED.category, - data_status = 'verified', - calories_kcal = EXCLUDED.calories_kcal, - protein_g = EXCLUDED.protein_g, - carbohydrates_g = EXCLUDED.carbohydrates_g, - total_sugar_g = EXCLUDED.total_sugar_g, - dietary_fiber_g = EXCLUDED.dietary_fiber_g, - total_fat_g = EXCLUDED.total_fat_g, - sodium_mg = EXCLUDED.sodium_mg, - calcium_mg = EXCLUDED.calcium_mg, - iron_mg = EXCLUDED.iron_mg, - vitamin_c_mg = EXCLUDED.vitamin_c_mg + product_name = COALESCE(EXCLUDED.product_name, nutrition_facts.product_name), + category = COALESCE(EXCLUDED.category, nutrition_facts.category), + data_source = 'excel_upload', + -- Never downgrade a row a trusted source already verified. + data_status = CASE WHEN nutrition_facts.data_status = 'verified' + THEN 'verified' ELSE EXCLUDED.data_status END, + calories_kcal = COALESCE(EXCLUDED.calories_kcal, nutrition_facts.calories_kcal), + protein_g = COALESCE(EXCLUDED.protein_g, nutrition_facts.protein_g), + carbohydrates_g = COALESCE(EXCLUDED.carbohydrates_g, nutrition_facts.carbohydrates_g), + total_sugar_g = COALESCE(EXCLUDED.total_sugar_g, nutrition_facts.total_sugar_g), + dietary_fiber_g = COALESCE(EXCLUDED.dietary_fiber_g, nutrition_facts.dietary_fiber_g), + total_fat_g = COALESCE(EXCLUDED.total_fat_g, nutrition_facts.total_fat_g), + sodium_mg = COALESCE(EXCLUDED.sodium_mg, nutrition_facts.sodium_mg), + calcium_mg = COALESCE(EXCLUDED.calcium_mg, nutrition_facts.calcium_mg), + iron_mg = COALESCE(EXCLUDED.iron_mg, nutrition_facts.iron_mg), + vitamin_c_mg = COALESCE(EXCLUDED.vitamin_c_mg, nutrition_facts.vitamin_c_mg), + updated_at = CURRENT_TIMESTAMP """, - (brand, image_id, product_name, category, calories, protein, carbs, sugar, fiber, fat, sodium, calcium, iron, vitamin_c) + (brand, image_id, product_name, category, data_status, + values["calories_kcal"], values["protein_g"], values["carbohydrates_g"], + values["total_sugar_g"], values["dietary_fiber_g"], values["total_fat_g"], + values["sodium_mg"], values["calcium_mg"], values["iron_mg"], + values["vitamin_c_mg"]) ) - - # 2. Upsert nutrition_insights - insights_json = json.dumps({ - "brand": brand, - "image_id": image_id, - "data_status": "verified", - "nutrition_score": health_score, - "health_score": health_score, - "positive_insights": [f"Contains {protein}g protein per 100g", f"Provides {fiber}g dietary fiber"], - "nutritional_cautions": [f"{sugar}g sugar per 100g"], - "diet_tags": diet_tags, - "allergens": allergens - }) - - cur.execute( - """ - INSERT INTO nutrition_insights - (brand, image_id, data_status, nutrition_score, health_score, score_breakdown, - positive_insights, nutritional_cautions, diet_tags, allergens) - VALUES (%s, %s, 'verified', %s, %s, %s, %s, %s, %s, %s) - ON CONFLICT (brand, image_id) DO UPDATE SET - data_status = 'verified', - nutrition_score = EXCLUDED.nutrition_score, - health_score = EXCLUDED.health_score, - positive_insights = EXCLUDED.positive_insights, - nutritional_cautions = EXCLUDED.nutritional_cautions, - diet_tags = EXCLUDED.diet_tags, - allergens = EXCLUDED.allergens - """, - (brand, image_id, health_score, health_score, json.dumps({"protein": 85, "fiber": 80}), - [f"Contains {protein}g protein per 100g"], [f"{sugar}g sugar per 100g"], diet_tags, allergens) - ) - + + # -- insights ------------------------------------------------- + # Only ever built from values this row actually carried. A row + # that carried none produces no insight row at all, rather than + # a confident-looking one full of defaults. + health_score = _opt_float(row, ['health_score', 'nutrition_score', 'score']) + diet_tags_raw = _opt_str(row, ['diet_tags', 'tags', 'diet']) + allergens_raw = _opt_str(row, ['allergens', 'allergen']) + + diet_tags = [t.strip() for t in diet_tags_raw.split(',') if t.strip()] \ + if diet_tags_raw is not None else None + allergens = [a.strip() for a in allergens_raw.split(',') if a.strip()] \ + if allergens_raw is not None else None + # 'unavailable' is the schema's own vocabulary for "we were not + # told", and it is what stops an empty list reading as "none". + allergen_source = "upload" if allergens is not None else "unavailable" + + positives = [] + if values["protein_g"] is not None: + positives.append(f"Contains {values['protein_g']}g protein per 100g") + if values["dietary_fiber_g"] is not None: + positives.append(f"Provides {values['dietary_fiber_g']}g dietary fiber") + cautions = [] + if values["total_sugar_g"] is not None: + cautions.append(f"{values['total_sugar_g']}g sugar per 100g") + + if health_score is not None or diet_tags or allergens or positives or cautions: + cur.execute( + """ + INSERT INTO nutrition_insights + (brand, image_id, data_status, nutrition_score, health_score, + scoring_version, positive_insights, nutritional_cautions, + diet_tags, allergens, allergen_source, generated_at) + VALUES (%s, %s, %s, %s, %s, 'excel_upload', %s, %s, %s, %s, %s, CURRENT_TIMESTAMP) + ON CONFLICT (brand, image_id) DO UPDATE SET + data_status = CASE WHEN nutrition_insights.data_status = 'verified' + THEN 'verified' ELSE EXCLUDED.data_status END, + nutrition_score = COALESCE(EXCLUDED.nutrition_score, nutrition_insights.nutrition_score), + health_score = COALESCE(EXCLUDED.health_score, nutrition_insights.health_score), + scoring_version = 'excel_upload', + positive_insights = COALESCE(EXCLUDED.positive_insights, nutrition_insights.positive_insights), + nutritional_cautions = COALESCE(EXCLUDED.nutritional_cautions, nutrition_insights.nutritional_cautions), + diet_tags = COALESCE(EXCLUDED.diet_tags, nutrition_insights.diet_tags), + allergens = COALESCE(EXCLUDED.allergens, nutrition_insights.allergens), + allergen_source = CASE WHEN EXCLUDED.allergens IS NULL + THEN nutrition_insights.allergen_source + ELSE EXCLUDED.allergen_source END, + generated_at = CURRENT_TIMESTAMP + """, + (brand, image_id, data_status, health_score, health_score, + positives or None, cautions or None, + diet_tags, allergens, allergen_source) + ) + imported_count += 1 - + + except HTTPException: + raise except Exception as e: logger.error("Nutrition upload failed: %s", e) raise HTTPException(status_code=500, detail=f"Database import failed: {e}") finally: conn.close() - - return { - "status": "success", - "filename": file.filename, - "rows_total": len(df), - "rows_imported": imported_count, - "message": f"Successfully imported {imported_count} nutritional intelligence items." - } + + return _finish( + file.filename, len(df), imported_count, errors, skipped, + "nutritional intelligence items", + data_status_counts=status_counts, + ) # --------------------------------------------------------------------------- # Template Downloads # --------------------------------------------------------------------------- +# The required columns below are the ones the handlers refuse a row without. +# Everything else is genuinely optional and is stored as NULL when omitted - +# these endpoints no longer fill a gap with a plausible-looking constant, so a +# template that omits a column now produces an honest blank rather than a +# confident wrong number. @router.get("/template/{tab_type}") def get_sample_template(tab_type: str) -> Response: tab_type = tab_type.lower() - + if tab_type == 'stores': + # Required: store_id, brand, product_name. + # Prices are written only when mrp, cost_price and selling_price are + # all present - store_prices declares all three NOT NULL. content = ( - "store_id,brand,image_id,product_name,category,available_stock,reserved_stock,mrp,cost_price,selling_price,reorder_level,safety_stock\n" - "store_mumbai_1,amul,amul_amul_butter_500ml,Amul Butter 500ml,Dairy,120,5,250.00,200.00,235.00,20,10\n" - "store_mumbai_1,amul,amul_amul_ghee_1l,Amul Ghee 1L,Dairy,85,2,650.00,520.00,610.00,15,5\n" - "store_delhi_2,nestle,nestle_everyday_1kg,Everyday Milk Powder 1kg,Dairy,45,0,420.00,340.00,399.00,10,5\n" + "store_id,brand,image_id,product_name,category,available_stock,reserved_stock,mrp,cost_price,selling_price,reorder_level,safety_stock,city\n" + "store_mumbai_1,amul,amul_amul_butter_500ml,Amul Butter 500ml,Dairy,120,5,250.00,200.00,235.00,20,10,Mumbai\n" + "store_mumbai_1,amul,amul_amul_ghee_1l,Amul Ghee 1L,Dairy,85,2,650.00,520.00,610.00,15,5,Mumbai\n" + "store_delhi_2,nestle,nestle_everyday_1kg,Everyday Milk Powder 1kg,Dairy,45,0,420.00,340.00,399.00,10,5,Delhi\n" ) filename = "sample_stores_inventory_template.csv" elif tab_type == 'analytics': + # Required: store_id, brand, customer_id, quantity, unit_price, and one + # of image_id / product_name. `total_price` is optional - it is derived + # from quantity * unit_price when absent - and is stored in the + # `line_total` column every analytics reader sums. content = ( "order_id,store_id,brand,image_id,product_name,order_date,customer_id,quantity,unit_price,total_price\n" "ORD_9001,store_mumbai_1,amul,amul_amul_butter_500ml,Amul Butter 500ml,2026-08-01 10:30:00,cust_101,2,235.00,470.00\n" @@ -434,16 +754,21 @@ def get_sample_template(tab_type: str) -> Response: ) filename = "sample_analytics_sales_template.csv" elif tab_type == 'nutrition': + # Required: brand, and one of image_id / product_name. Every nutrient + # column is optional and stays NULL when omitted; a row is marked + # 'verified' only when the full core set is present. Leave `allergens` + # out entirely rather than writing "None" - an empty cell records "not + # told", which is not the same claim as "contains no allergens". content = ( "brand,image_id,product_name,category,calories_kcal,protein_g,carbohydrates_g,total_sugar_g,dietary_fiber_g,total_fat_g,sodium_mg,health_score,diet_tags,allergens\n" - "amul,amul_amul_butter_500ml,Amul Butter 500ml,Dairy,717,0.8,0.1,0.0,0.0,81.0,650,75,Vegetarian,Dairy\n" - "amul,amul_amul_ghee_1l,Amul Ghee 1L,Dairy,898,0.0,0.0,0.0,0.0,99.8,0,82,Vegetarian,Keto Friendly\n" - "nestle,nestle_everyday_1kg,Everyday Milk Powder 1kg,Dairy,496,25.5,38.0,38.0,0.0,27.0,350,88,High Protein,Dairy\n" + "amul,amul_amul_butter_500ml,Amul Butter 500ml,Dairy,717,0.8,0.1,0.0,0.0,81.0,650,75,Vegetarian,Milk\n" + "amul,amul_amul_ghee_1l,Amul Ghee 1L,Dairy,898,0.0,0.0,0.0,0.0,99.8,0,82,Vegetarian,Milk\n" + "nestle,nestle_everyday_1kg,Everyday Milk Powder 1kg,Dairy,496,25.5,38.0,38.0,0.0,27.0,350,88,High Protein,Milk\n" ) filename = "sample_nutrition_intelligence_template.csv" else: raise HTTPException(status_code=400, detail=f"Unknown template type '{tab_type}'. Use stores, analytics, or nutrition.") - + return PlainTextResponse( content=content, media_type="text/csv", diff --git a/app/api/routers/uploads.py b/app/api/routers/uploads.py index 7a79ef3..8c3e19a 100644 --- a/app/api/routers/uploads.py +++ b/app/api/routers/uploads.py @@ -1,6 +1,8 @@ -"""The one endpoint an outside contributor may call. +"""The catalog ingestion API given to outside API users. - POST /api/uploads/catalog - drop spreadsheets into the review inbox + POST /api/uploads/catalog - send spreadsheets, the pipeline runs + GET /api/uploads/catalog - the batches this caller has sent + GET /api/uploads/catalog/{batch_id} - progress and result of one of them This is deliberately its own router, with its own prefix and its own guard, so that the difference between it and everything else in the app is visible in one @@ -9,42 +11,55 @@ module. Everything else that touches catalog data is `require_admin`; this is `require_permission("upload_catalog")`, and that single line is the whole security boundary of the feature. -WHAT THIS ENDPOINT CANNOT DO ----------------------------- -Start work. Files land in the inbox and wait for an admin to select them -(`app/core/inbox.py`). No worker is touched, no queue is entered, no thread is -started. That is what makes it safe to hand a credential to someone outside the -team: the worst a leaked uploader key costs is bounded disk, never CPU on a -one-vCPU host that is also serving the API. +WHAT HAPPENS WHEN A FILE ARRIVES +-------------------------------- +It is parsed during the request - while the caller is still on the phone - so +an unusable sheet comes back as a 400 naming the problem rather than as a job +that fails a minute later into a void. Then the bytes are staged to disk, a +batch is queued, and the same 11-stage pipeline the admin routes use runs over +them: `app/core/batch_ingest.py` -> `store_catalog_pipeline.run_pipeline`. -It also cannot read anything. There is no GET here on purpose - the decision was -that this is a one-way drop, so the credential grants no visibility into the -catalog, into other submissions, or even into the submitter's own past uploads. +The response is a `batch_id`. Ingestion is far too slow to finish inside a +request - it is thousands of rows through eleven stages - so the caller polls +GET /api/uploads/catalog/{batch_id} until `status` leaves `queued`/`running`. -WHY A BAD SHEET IS REJECTED HERE AND NOT LATER ----------------------------------------------- -The file is parsed during the request, while the colleague is still watching. -Telling them "row 1 has no product name column" in the 202 is worth far more -than discovering it days later in an admin panel they cannot see, with no way to -ask them for a corrected file except out of band. +THIS CREDENTIAL NOW COSTS CPU, AND THAT IS THE POINT +---------------------------------------------------- +An earlier version of this endpoint parked files in a review inbox and started +nothing, so that a leaked key could cost only disk. That is not the product: +an API user sends a file in order for it to be ingested, and a queue that needs +an admin to press a button is not an API. + +So the bound is no longer "this role cannot start work" but "all work, from +every source, goes through one worker". `batch_worker` runs a single batch at a +time behind a queue of `BATCH_QUEUE_MAX`; past that this endpoint answers 429. +An uploader key can therefore occupy the ingestion worker, which is what it is +for - it cannot multiply it, which is what matters on a one-vCPU host that is +also serving the API and its healthcheck. + +WHAT THIS ENDPOINT STILL CANNOT DO +---------------------------------- +See anyone else's data. Every read here is filtered by `submitted_by`, so a key +sees the batches it sent and nothing else - not the catalog, not other callers' +submissions, not the admin batch list. Nothing here can cancel, resume, or +delete; those stay on the admin router. """ from __future__ import annotations import logging -from typing import List, Optional +from typing import List from fastapi import APIRouter, Depends, File, HTTPException, UploadFile, status -from pydantic import BaseModel +from app.api import batch_common +from app.api.batch_job_store import batch_job_store from app.api.deps import require_permission -from app.core import inbox -from app.core import store_catalog_pipeline as pipeline +from app.core import batch_ingest from app.infrastructure.security import Principal from app.infrastructure.settings import ( BATCH_MAX_FILES, BATCH_MAX_TOTAL_BYTES, BATCH_MAX_TOTAL_ROWS, - INBOX_MAX_PENDING_FILES, ) logger = logging.getLogger(__name__) @@ -56,157 +71,138 @@ MAX_UPLOAD_BYTES = 10 * 1024 * 1024 MAX_UPLOAD_ROWS = 2000 -class SubmittedFileOut(BaseModel): - filename: str - accepted: bool - rows_total: int = 0 - error: Optional[str] = None +def _limits() -> batch_common.UploadLimits: + """Read at call time, from THIS module's globals - see batch_common.""" + return batch_common.UploadLimits( + max_files=BATCH_MAX_FILES, + max_file_bytes=MAX_UPLOAD_BYTES, + max_file_rows=MAX_UPLOAD_ROWS, + max_total_bytes=BATCH_MAX_TOTAL_BYTES, + max_total_rows=BATCH_MAX_TOTAL_ROWS, + ) -class SubmissionOut(BaseModel): - submission_id: Optional[str] - submitted_by: str - files_accepted: int - files_rejected: int - files: List[SubmittedFileOut] +class CatalogUploadOut(batch_common.BatchOut): + """A started batch, plus a sentence a human can read without a schema.""" + message: str -@router.post("/catalog", status_code=status.HTTP_202_ACCEPTED) -async def submit_catalog_files( - files: List[UploadFile] = File(...), - principal: Principal = Depends(require_permission("upload_catalog")), -) -> SubmissionOut: - """Accept spreadsheets into the review inbox. Starts nothing. +def _owns(manifest: batch_ingest.BatchManifest, principal: Principal) -> bool: + """May this caller see this batch? - Returns 202 with a per-file verdict. A drop where some files parse and some - do not is a partial success, not a failure: the good ones are kept and the - caller is told precisely which sheet to fix. + An admin sees everything - they already have the whole batch router. Anyone + else sees only what their own credential sent, matched on the credential + NAME, which is what `principal.username` is for an API key (see + security.principal_for_api_key). """ - if not files: - raise HTTPException(status_code=400, detail="No files were uploaded.") - if len(files) > BATCH_MAX_FILES: + if principal.role == "admin": + return True + return bool(manifest.submitted_by) and manifest.submitted_by == principal.username + + +@router.post("/catalog", status_code=status.HTTP_202_ACCEPTED) +async def ingest_catalog_files( + files: List[UploadFile] = File(...), + use_llm: bool = False, + fetch_images: bool = False, + principal: Principal = Depends(require_permission("upload_catalog")), +) -> CatalogUploadOut: + """Accept spreadsheets and run the catalog pipeline over them. + + Returns 202 and a `batch_id` to poll. A drop where some files parse and some + do not is a partial success, not a failure: the good ones are ingested and + the bad ones come back in `files` as `status: "failed"` with the reason, so + the caller knows exactly which sheet to fix and resend. + + `use_llm` and `fetch_images` default OFF, the opposite of the single-file + admin route. Both are network stages, and image search in particular spawns + a Playwright subprocess that can spend minutes per batch on a host with one + vCPU. An API client that genuinely wants them can ask; an API client that + does not think about it gets the cheap, predictable path. + """ + limits = _limits() + read = await batch_common.read_uploads(files, limits) + valid, invalid, _rows = batch_common.parse_all(read, limits) + + if not valid: + detail = "; ".join(f"{name}: {reason}" for name, reason in invalid) raise HTTPException( - status_code=413, - detail=( - f"{len(files)} files exceeds the {BATCH_MAX_FILES}-file limit for one " - f"upload. Send them in smaller drops." - ), + status_code=400, + detail=f"None of the uploaded files could be ingested. {detail}", ) - # Refuse before reading a byte if the inbox is already backed up. This is - # the ceiling that stops an unattended key filling the disk one perfectly - # valid file at a time. - already_waiting = inbox.pending_count() - if already_waiting + len(files) > INBOX_MAX_PENDING_FILES: + manifest, started = batch_common.stage_and_queue( + valid, + invalid, + use_llm=use_llm, + fetch_images=fetch_images, + # The key's NAME, never its secret. Principal.username is the name half + # of the API_KEYS entry (see principal_for_api_key). + submitted_by=principal.username, + ) + + if not started: + # The batch is staged and durable, but this caller has no Resume button + # - that lives on the admin router - so the honest instruction is to + # send it again shortly. The id is included so an admin can find and + # resume this one instead if the caller reports it. raise HTTPException( status_code=429, detail=( - f"The review inbox already holds {already_waiting} file(s) awaiting " - f"review, and the limit is {INBOX_MAX_PENDING_FILES}. Please wait until " - f"some have been processed." + f"Too many batches are already queued. Batch {manifest.batch_id} has " + f"been saved but not started; retry this upload shortly." ), ) - accepted: list = [] - rejected: list = [] - total_bytes = 0 - total_rows = 0 - - for upload in files: - name = upload.filename or "upload.xlsx" - contents = await upload.read() - - if not contents: - rejected.append((name, "The file is empty.")) - continue - if len(contents) > MAX_UPLOAD_BYTES: - rejected.append(( - name, - f"Larger than the {MAX_UPLOAD_BYTES // (1024 * 1024)}MB per-file limit.", - )) - continue - - total_bytes += len(contents) - if total_bytes > BATCH_MAX_TOTAL_BYTES: - raise HTTPException( - status_code=413, - detail=( - f"This drop is larger than the " - f"{BATCH_MAX_TOTAL_BYTES // (1024 * 1024)}MB total limit." - ), - ) - - # Parse now, while the sender is still here to be told. - try: - frame, _mapping = pipeline.parse_spreadsheet(name, contents) - except HTTPException as exc: - # read_products_dataframe raises this for an unsupported extension - # or a missing Excel reader; its message already names the file and - # says what to do. - rejected.append((name, str(exc.detail))) - continue - except Exception as exc: # noqa: BLE001 - an unreadable sheet is caller error - rejected.append((name, f"Could not parse the file: {exc}")) - continue - - if frame.empty: - rejected.append((name, "The file has no data rows.")) - continue - rows = int(len(frame)) - if rows > MAX_UPLOAD_ROWS: - rejected.append(( - name, f"{rows} rows exceeds the {MAX_UPLOAD_ROWS}-row per-file limit." - )) - continue - - total_rows += rows - if total_rows > BATCH_MAX_TOTAL_ROWS: - raise HTTPException( - status_code=413, - detail=( - f"This drop totals more than {BATCH_MAX_TOTAL_ROWS} rows. " - f"Send it in two smaller drops." - ), - ) - - accepted.append((name, contents, rows)) - - submission = None - if accepted: - submission = inbox.stage_submission( - accepted, - # The key's NAME, never its secret. Principal.username is the name - # half of the API_KEYS entry (see principal_for_api_key). - submitted_by=principal.username, - ) - logger.info( - "Inbox: %d file(s) from %s awaiting review (submission %s)", - len(accepted), principal.username, submission.submission_id, - ) - - out = [ - SubmittedFileOut(filename=n, accepted=True, rows_total=r) - for n, _c, r in accepted - ] + [ - SubmittedFileOut(filename=n, accepted=False, error=e) for n, e in rejected - ] - - if accepted and rejected: - message = ( - f"{len(accepted)} file(s) received and awaiting review. " - f"{len(rejected)} could not be read - see the errors below and resend those." - ) - elif accepted: - message = f"{len(accepted)} file(s) received and awaiting review." - else: - message = "No file could be read. Nothing was received - see the errors below." - - return SubmissionOut( - submission_id=submission.submission_id if submission else None, - submitted_by=principal.username, - files_accepted=len(accepted), - files_rejected=len(rejected), - files=out, - message=message, + logger.info( + "Catalog batch %s queued: %d file(s), %d rejected, from %s", + manifest.batch_id, len(valid), len(invalid), principal.username, ) + + body = batch_common.to_out(manifest).model_dump() + if invalid: + message = ( + f"{len(valid)} file(s) accepted and queued for ingestion. " + f"{len(invalid)} could not be read - see 'files' for the reason on each, " + f"and resend those." + ) + else: + message = ( + f"{len(valid)} file(s) accepted and queued for ingestion. " + f"Poll GET /api/uploads/catalog/{manifest.batch_id} for progress." + ) + return CatalogUploadOut(**body, message=message) + + +@router.get("/catalog") +def list_my_catalog_batches( + limit: int = 20, + principal: Principal = Depends(require_permission("upload_catalog")), +) -> dict: + """The batches this credential has sent, newest first.""" + limit = max(1, min(limit, 100)) + # Over-fetch before filtering: `recent` orders by creation across every + # caller, so taking `limit` first would return fewer than `limit` of this + # caller's own - or none at all while another key is busy. + mine = [ + m for m in batch_job_store.recent(limit * 10) if _owns(m, principal) + ][:limit] + return {"batches": [batch_common.to_out(m) for m in mine]} + + +@router.get("/catalog/{batch_id}") +def get_my_catalog_batch( + batch_id: str, + principal: Principal = Depends(require_permission("upload_catalog")), +) -> batch_common.BatchOut: + """Progress and result for one batch this credential sent. + + A batch belonging to someone else is a 404, not a 403: whether a given id + exists is not this caller's business, and the two answers must therefore be + indistinguishable. + """ + manifest = batch_job_store.get(batch_id) + if not manifest or not _owns(manifest, principal): + raise HTTPException(status_code=404, detail="Batch not found") + return batch_common.to_out(manifest) diff --git a/app/core/batch_ingest.py b/app/core/batch_ingest.py index 89e5401..0e59a04 100644 --- a/app/core/batch_ingest.py +++ b/app/core/batch_ingest.py @@ -293,6 +293,7 @@ def stage_batch( use_llm: bool = False, fetch_images: bool = False, invalid: Optional[List[Tuple[str, str]]] = None, + submitted_by: Optional[str] = None, ) -> BatchManifest: """Write the uploads to disk and return the manifest describing them. @@ -300,12 +301,23 @@ def stage_batch( They are recorded as failed members of the batch rather than dropped: an operator who selected six files and sees five must be told what happened to the sixth, and the batch page is the only place they will look. + + `submitted_by` is the credential name the files arrived under. It is what + scopes an API client's view to its own batches, so it is set at staging + time rather than patched on afterwards - a batch that existed for even a + moment without an owner is a batch the ownership filter would hide from + the only person entitled to see it. """ batch_id = uuid.uuid4().hex directory = batch_dir(batch_id) directory.mkdir(parents=True, exist_ok=True) - manifest = BatchManifest(batch_id=batch_id, use_llm=use_llm, fetch_images=fetch_images) + manifest = BatchManifest( + batch_id=batch_id, + use_llm=use_llm, + fetch_images=fetch_images, + submitted_by=submitted_by, + ) position = 0 for filename, content in uploads: diff --git a/app/core/catalog_engine.py b/app/core/catalog_engine.py index eaf8928..ded0310 100644 --- a/app/core/catalog_engine.py +++ b/app/core/catalog_engine.py @@ -7,11 +7,10 @@ resort). The Node.js/Crawlee scraping layer that used to live here has been removed - see app/services/image_search.py and app/services/playwright_image_fallback.py for details. """ -import asyncio import json import sys from pathlib import Path -from typing import Dict, List, Optional, Any +from typing import Dict, List, Any import logging # Add app directory to path @@ -41,10 +40,8 @@ def generate_product_highlights(product: Dict[str, Any], brand: str) -> List[str product = {} highlights = [] - title = str(product.get('title', '')).strip() description = str(product.get('description', '')).strip() category = str(product.get('category', '')).strip() - price_range = str(product.get('price_range', '')).strip() size_variants = product.get('size_variants', []) # Ensure size_variants is a list @@ -820,7 +817,7 @@ class ProductCatalogEngine: price_str = variant.split(' - ₹')[1] price_num = float(price_str) prices.append(price_num) - except: + except (ValueError, TypeError): pass if prices: min_price = min(prices) @@ -918,7 +915,7 @@ class ProductCatalogEngine: 'products': enhanced_products } - logger.info(f"🎉 Catalog generation complete!") + logger.info("🎉 Catalog generation complete!") logger.info(f"📊 Products: {catalog['total_products']}") logger.info(f"🖼️ Total images: {catalog['total_images']}") diff --git a/app/core/inbox.py b/app/core/inbox.py deleted file mode 100644 index e6e2987..0000000 --- a/app/core/inbox.py +++ /dev/null @@ -1,348 +0,0 @@ -""" -A review inbox: a third party drops spreadsheets, an admin decides what runs. - -WHY THIS IS NOT PART OF `batch_ingest` --------------------------------------- -A batch is a RUN. It has a worker, a state machine -(`queued -> running -> done | failed | partial | cancelled | interrupted`), a -resume path, and thirty-five tests built around exactly that. A submission is -not a run and never becomes one: it is a pile of files sitting still, with no -worker and no run state, waiting for a human. - -Folding "awaiting review" into `BatchManifest` would have put review logic -inside the thing that executes pipelines, and every one of those tests would -have needed re-reasoning to answer "can this state reach the worker?". Two -small state machines are easier to be sure about than one that means two -things. - -So the flow is: - - colleague admin batch_ingest - --------- ----- ------------ - POST /api/uploads/catalog - | - v - SUBMISSION (inert) ---> select file ids ---> stage_batch() ---> worker - -`claim_files()` is the seam. It copies the chosen files into a fresh batch -directory and hands back what `stage_batch` needs, so everything downstream is -the machinery that already exists and is already tested. - -WHY COPY RATHER THAN MOVE -------------------------- -A batch directory has to be self-contained: that is what lets a batch be -resumed after a restart without caring whether anything else still exists. If a -batch referenced files living in the inbox, dismissing or purging an inbox entry -would break the resume of a run that had already started. The cost is a -duplicate of a file that is at most 10MB, until retention clears the inbox copy. - -NOTHING HERE EXECUTES ANYTHING ------------------------------- -No import of `batch_worker`, no thread, no queue. That is the security property -the uploader role depends on: a credential that can reach only this module can -consume bounded disk and can never consume CPU. -""" -from __future__ import annotations - -import json -import logging -import os -import re -import shutil -import time -import uuid -from dataclasses import asdict, dataclass, field -from pathlib import Path -from typing import Any, Dict, List, Optional, Tuple - -from app.core.batch_ingest import _safe_name -from app.infrastructure.settings import ( - INBOX_RETENTION_DAYS, - INBOX_UPLOAD_DIR, -) - -logger = logging.getLogger(__name__) - -RECORD_NAME = "submission.json" - -# File states inside the inbox. Deliberately disjoint from the batch states - -# an inbox file is never "running"; the COPY of it inside a batch is. -PENDING = "pending" # waiting for an admin to look at it -CONSUMED = "consumed" # copied into a batch and started -DISMISSED = "dismissed" # an admin decided it will never run - -CLOSED_STATES = {CONSUMED, DISMISSED} - - -@dataclass -class InboxFile: - """One spreadsheet in a drop.""" - - file_id: str - filename: str # what the colleague called it - stored_name: str # what it is called on disk - size_bytes: int = 0 - rows_total: int = 0 - status: str = PENDING - detail: Optional[str] = None - batch_id: Optional[str] = None # set when consumed, so the trail is followable - decided_at: Optional[float] = None - - -@dataclass -class Submission: - """One POST from one colleague. Serialised to submission.json verbatim.""" - - submission_id: str - submitted_by: str # the API key's NAME, never its secret - created_at: float = field(default_factory=time.time) - updated_at: float = field(default_factory=time.time) - files: List[InboxFile] = field(default_factory=list) - - @property - def pending_files(self) -> List[InboxFile]: - return [f for f in self.files if f.status == PENDING] - - def to_dict(self) -> Dict[str, Any]: - return { - "submission_id": self.submission_id, - "submitted_by": self.submitted_by, - "created_at": self.created_at, - "updated_at": self.updated_at, - "files": [asdict(f) for f in self.files], - } - - @classmethod - def from_dict(cls, raw: Dict[str, Any]) -> "Submission": - allowed = set(InboxFile.__dataclass_fields__) - return cls( - submission_id=raw["submission_id"], - submitted_by=raw.get("submitted_by") or "unknown", - created_at=float(raw.get("created_at") or time.time()), - updated_at=float(raw.get("updated_at") or time.time()), - files=[ - InboxFile(**{k: v for k, v in entry.items() if k in allowed}) - for entry in (raw.get("files") or []) - ], - ) - - -# --------------------------------------------------------------------------- -# Disk layout -# --------------------------------------------------------------------------- -def inbox_root() -> Path: - """Read at call time, not import time, so tests can repoint the directory.""" - return Path(INBOX_UPLOAD_DIR) - - -def submission_dir(submission_id: str) -> Path: - # Ids are generated here (uuid4 hex), never taken from a request body, but - # this is the function that turns one into a path - so it validates anyway. - if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", submission_id or ""): - raise ValueError("Invalid submission id: {!r}".format(submission_id)) - return inbox_root() / submission_id - - -def record_path(submission_id: str) -> Path: - return submission_dir(submission_id) / RECORD_NAME - - -def write_record(submission: Submission) -> None: - """Temp file then os.replace, for the same reason batch_ingest does it: - a half-written record is indistinguishable from a corrupt one, and the - listing path reads every record it finds.""" - target = record_path(submission.submission_id) - target.parent.mkdir(parents=True, exist_ok=True) - tmp = target.with_name(RECORD_NAME + ".tmp") - tmp.write_text(json.dumps(submission.to_dict(), indent=2), encoding="utf-8") - os.replace(tmp, target) - - -def read_record(submission_id: str) -> Optional[Submission]: - try: - path = record_path(submission_id) - except ValueError: - return None - if not path.exists(): - return None - try: - return Submission.from_dict(json.loads(path.read_text(encoding="utf-8"))) - except Exception as exc: # noqa: BLE001 - one bad record must not break the inbox - logger.warning("Ignoring unreadable submission record %s: %s", path, exc) - return None - - -def list_submissions() -> List[Submission]: - """Every readable submission on disk, newest first.""" - root = inbox_root() - if not root.exists(): - return [] - found = [] - for child in sorted(root.iterdir()): - if not child.is_dir(): - continue - record = read_record(child.name) - if record: - found.append(record) - return sorted(found, key=lambda s: s.created_at, reverse=True) - - -def list_pending() -> List[Submission]: - """Submissions that still have at least one file awaiting a decision.""" - return [s for s in list_submissions() if s.pending_files] - - -def pending_count() -> int: - """What the badge shows.""" - return sum(len(s.pending_files) for s in list_submissions()) - - -# --------------------------------------------------------------------------- -# Intake -# --------------------------------------------------------------------------- -def stage_submission( - uploads: List[Tuple[str, bytes, int]], - *, - submitted_by: str, - rejected: Optional[List[Tuple[str, str]]] = None, -) -> Submission: - """Write a colleague's drop to disk. Starts nothing. - - `uploads` is (filename, content, rows_total) for files that already parsed - cleanly. `rejected` carries the ones that did not; they are recorded so the - colleague's 202 can tell them which sheet to fix, but they are NOT written - to disk and never appear in the inbox - there is nothing an admin could - usefully do with a file that cannot be read. - """ - submission_id = uuid.uuid4().hex - directory = submission_dir(submission_id) - directory.mkdir(parents=True, exist_ok=True) - - submission = Submission(submission_id=submission_id, submitted_by=submitted_by) - - for position, (filename, content, rows) in enumerate(uploads): - stored = "{:02d}_{}".format(position, _safe_name(filename)) - (directory / stored).write_bytes(content) - submission.files.append( - InboxFile( - file_id=uuid.uuid4().hex, - filename=filename or stored, - stored_name=stored, - size_bytes=len(content), - rows_total=rows, - ) - ) - - write_record(submission) - return submission - - -# --------------------------------------------------------------------------- -# The seam into batch_ingest -# --------------------------------------------------------------------------- -def find_file(file_id: str) -> Optional[Tuple[Submission, InboxFile]]: - for submission in list_submissions(): - for entry in submission.files: - if entry.file_id == file_id: - return submission, entry - return None - - -def collect_for_batch(file_ids: List[str]) -> Tuple[List[Tuple[str, bytes]], List[str]]: - """Read the chosen pending files into memory, in the order given. - - Returns (uploads, submitters). Raises KeyError naming the first id that is - unknown, and ValueError naming the first that is no longer pending - two - admin tabs open on the same inbox must not both start the same file, and - the second one gets told why rather than silently double-running it. - """ - uploads: List[Tuple[str, bytes]] = [] - submitters: List[str] = [] - - for file_id in file_ids: - found = find_file(file_id) - if found is None: - raise KeyError(file_id) - submission, entry = found - if entry.status != PENDING: - raise ValueError( - "'{}' was already {} and cannot be started again.".format( - entry.filename, entry.status - ) - ) - path = submission_dir(submission.submission_id) / entry.stored_name - uploads.append((entry.filename, path.read_bytes())) - if submission.submitted_by not in submitters: - submitters.append(submission.submitted_by) - - return uploads, submitters - - -def mark_consumed(file_ids: List[str], batch_id: str) -> None: - """Called only after the batch has actually been created and queued. - - Ordering matters: marking first and staging second would lose the files - from the inbox if staging then failed, with nothing left to retry from. - """ - _decide(file_ids, CONSUMED, batch_id=batch_id, - detail="Started as part of batch {}.".format(batch_id)) - - -def dismiss(file_ids: List[str]) -> int: - """An admin has decided these will never run. Returns how many changed. - - This exists so the badge can reach zero. Without it a file nobody intends - to process sits in the inbox forever, and a count that never clears is a - count people stop reading. - """ - return _decide(file_ids, DISMISSED, detail="Dismissed without running.") - - -def _decide(file_ids: List[str], status: str, *, batch_id: Optional[str] = None, - detail: Optional[str] = None) -> int: - wanted = set(file_ids) - changed = 0 - for submission in list_submissions(): - touched = False - for entry in submission.files: - if entry.file_id in wanted and entry.status == PENDING: - entry.status = status - entry.detail = detail - entry.batch_id = batch_id - entry.decided_at = time.time() - touched = True - changed += 1 - if touched: - submission.updated_at = time.time() - write_record(submission) - return changed - - -# --------------------------------------------------------------------------- -# Retention -# --------------------------------------------------------------------------- -def purge_expired(now: Optional[float] = None) -> List[str]: - """Delete submissions whose files are all decided and past retention. - - A submission with anything still PENDING is never purged, however old. - Deleting a file nobody has looked at yet would lose a colleague's work - silently, which is worse than the disk it occupies. - """ - if INBOX_RETENTION_DAYS <= 0: - return [] - cutoff = (now if now is not None else time.time()) - INBOX_RETENTION_DAYS * 86400 - removed: List[str] = [] - for submission in list_submissions(): - if submission.created_at >= cutoff: - continue - if any(f.status == PENDING for f in submission.files): - continue - try: - shutil.rmtree(submission_dir(submission.submission_id)) - removed.append(submission.submission_id) - except OSError as exc: - logger.warning("Could not purge submission %s: %s", - submission.submission_id, exc) - if removed: - logger.info("Purged %d expired inbox submission(s).", len(removed)) - return removed diff --git a/app/core/store_catalog_pipeline.py b/app/core/store_catalog_pipeline.py index 99e4959..4777285 100644 --- a/app/core/store_catalog_pipeline.py +++ b/app/core/store_catalog_pipeline.py @@ -53,7 +53,6 @@ from app.infrastructure.settings import ( ENABLE_BARCODE_LOOKUP, ENABLE_HSN_GST_ENRICHMENT, ENABLE_PRODUCT_VALIDATION, - ENABLE_SKU_WEB_LOOKUP, MAX_VARIANTS_PER_PRODUCT, USE_EMBEDDINGS, ) @@ -64,7 +63,6 @@ from app.infrastructure.settings import ( # and tests/test_user_products_upload.py monkeypatches these names on that # module object. from app.api.routers.user_products import ( - AddProductRequest, _slugify, _text, map_spreadsheet_columns, diff --git a/app/infrastructure/security.py b/app/infrastructure/security.py index f8df7e9..dd72e50 100644 --- a/app/infrastructure/security.py +++ b/app/infrastructure/security.py @@ -72,8 +72,8 @@ ROLE_PERMISSIONS: Dict[str, List[str]] = { "view_nutrition_insights", "optimize_profits", ], - # A third party who may drop spreadsheets into the review inbox and do - # NOTHING else. One permission, deliberately. + # An outside API client that may send spreadsheets for catalog ingestion and + # do NOTHING else. One permission, deliberately. # # This role exists because API keys carry no per-key scoping: # principal_for_api_key() derives permissions entirely from the role, so @@ -82,9 +82,17 @@ ROLE_PERMISSIONS: Dict[str, List[str]] = { # add_product, upload_batch_products and upload_store_inventory - real # write access to the catalog - to solve a problem that needed one verb. # - # Nothing this role can do starts work: an uploaded file waits in the inbox - # until an admin selects it. So the worst an leaked uploader key costs is - # bounded disk (INBOX_MAX_PENDING_FILES), never CPU on a one-vCPU host. + # WHAT A LEAKED UPLOADER KEY COSTS. Real CPU: this permission starts the + # 11-stage pipeline, which is the point of the endpoint. The bound is not + # "this role cannot work" but "all ingestion, from every source, shares one + # worker" - batch_worker runs a single batch at a time behind a queue of + # BATCH_QUEUE_MAX, past which POST /api/uploads/catalog answers 429. So a + # key can occupy the ingestion worker; it cannot multiply it, and it cannot + # touch the request path the healthcheck reads. + # + # What it still cannot do: read the catalog, read another caller's + # submissions (every read on that router is filtered by submitted_by), or + # cancel, resume or delete anything. "uploader": [ "upload_catalog", ], diff --git a/app/infrastructure/settings.py b/app/infrastructure/settings.py index 945d312..b915a1e 100644 --- a/app/infrastructure/settings.py +++ b/app/infrastructure/settings.py @@ -169,24 +169,6 @@ BATCH_RETENTION_DAYS = int(os.getenv("BATCH_RETENTION_DAYS", "7")) # which is precisely how a slow start turns into an unrecoverable spiral. BATCH_AUTO_RESUME = _bool("BATCH_AUTO_RESUME", "false") -# --- Review inbox (a third party drops files; an admin decides) ------------- -# Files arrive here from POST /api/uploads/catalog and WAIT. Nothing in this -# directory is ever executed until an admin selects it, which is the whole -# security property of the feature: an uploader credential can consume bounded -# disk but can never consume CPU on a one-vCPU host. -INBOX_UPLOAD_DIR = _dir("INBOX_UPLOAD_DIR", DATA_DIR / "inbox") - -# Per-file ceilings are the batch ones (10MB / 2000 rows). This bounds the -# QUEUE: how many unreviewed files may accumulate before uploads are refused -# with 429. Without it an unattended key fills the disk one valid file at a -# time, and every one of them looks legitimate. -INBOX_MAX_PENDING_FILES = int(os.getenv("INBOX_MAX_PENDING_FILES", "200")) - -# Consumed and dismissed files are deleted this many days after upload. -# Pending files are never purged - deleting something nobody has looked at yet -# would lose a colleague's work silently. -INBOX_RETENTION_DAYS = int(os.getenv("INBOX_RETENTION_DAYS", "7")) - # Pristine copies of the bundled seed catalogs and pre-trained models, placed # here by the Dockerfile at a path that is never itself mounted over. # diff --git a/app/intelligence/discount_model.py b/app/intelligence/discount_model.py index 63ad211..22549ff 100644 --- a/app/intelligence/discount_model.py +++ b/app/intelligence/discount_model.py @@ -17,7 +17,7 @@ business-rule signal. from __future__ import annotations from dataclasses import dataclass -from typing import Dict, List +from typing import Dict import numpy as np import pandas as pd diff --git a/app/intelligence/features.py b/app/intelligence/features.py index a4f09cd..937b174 100644 --- a/app/intelligence/features.py +++ b/app/intelligence/features.py @@ -9,10 +9,9 @@ ML feature logic without a live Postgres instance. from __future__ import annotations import math -from datetime import date, datetime -from typing import Dict, Iterable, List, Optional +from datetime import date +from typing import Dict, Iterable, Optional -import numpy as np import pandas as pd # --------------------------------------------------------------------------- diff --git a/app/intelligence/forecasting.py b/app/intelligence/forecasting.py index f1ab52e..96a7d61 100644 --- a/app/intelligence/forecasting.py +++ b/app/intelligence/forecasting.py @@ -26,7 +26,7 @@ individually; only the training data is pooled). """ from __future__ import annotations -from datetime import date, timedelta +from datetime import date from typing import List import numpy as np diff --git a/app/intelligence/nutrition_clustering.py b/app/intelligence/nutrition_clustering.py index 78a2bc0..9ee7a9e 100644 --- a/app/intelligence/nutrition_clustering.py +++ b/app/intelligence/nutrition_clustering.py @@ -20,7 +20,6 @@ from __future__ import annotations import logging from typing import Any, Dict, List, Tuple -import numpy as np import pandas as pd from app.intelligence.model_utils import ModelBundle, load_bundle, save_bundle diff --git a/app/intelligence/nutrition_similarity.py b/app/intelligence/nutrition_similarity.py index b35dfad..9fde488 100644 --- a/app/intelligence/nutrition_similarity.py +++ b/app/intelligence/nutrition_similarity.py @@ -22,7 +22,6 @@ from __future__ import annotations import logging from typing import Any, Dict, List -import numpy as np import pandas as pd from app.intelligence.model_utils import ModelBundle, load_bundle, save_bundle diff --git a/app/intelligence/order_simulation.py b/app/intelligence/order_simulation.py index 9b62215..5428e7d 100644 --- a/app/intelligence/order_simulation.py +++ b/app/intelligence/order_simulation.py @@ -22,9 +22,9 @@ repeatable ML training and for demos. """ from __future__ import annotations -from dataclasses import dataclass, field +from dataclasses import dataclass from datetime import date, timedelta -from typing import Dict, List, Optional, Sequence +from typing import Dict, List, Sequence import hashlib import numpy as np diff --git a/app/intelligence/purchase_propensity_model.py b/app/intelligence/purchase_propensity_model.py index 44ee1eb..1e070fb 100644 --- a/app/intelligence/purchase_propensity_model.py +++ b/app/intelligence/purchase_propensity_model.py @@ -18,10 +18,9 @@ propensity score in the analytics UI). """ from __future__ import annotations -from datetime import date, timedelta +from datetime import date from typing import List -import numpy as np import pandas as pd from app.intelligence import features as F diff --git a/app/intelligence/store_performance_model.py b/app/intelligence/store_performance_model.py index f545016..fda93d9 100644 --- a/app/intelligence/store_performance_model.py +++ b/app/intelligence/store_performance_model.py @@ -15,7 +15,6 @@ heavy tuning. """ from __future__ import annotations -from typing import List import numpy as np import pandas as pd diff --git a/app/intelligence/trending_model.py b/app/intelligence/trending_model.py index 2f0c5e6..87eefc3 100644 --- a/app/intelligence/trending_model.py +++ b/app/intelligence/trending_model.py @@ -17,8 +17,8 @@ is `predicted_score.sort_values(ascending=False)` on live order data. """ from __future__ import annotations -from datetime import date, timedelta -from typing import Dict, List, Literal, Optional +from datetime import date +from typing import Dict, List, Literal import numpy as np import pandas as pd diff --git a/app/services/analytics_service.py b/app/services/analytics_service.py index 935ab0d..df96eda 100644 --- a/app/services/analytics_service.py +++ b/app/services/analytics_service.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Dict, List, Optional +from typing import Dict, List import pandas as pd @@ -50,7 +50,6 @@ def product_dashboard(brand: str, image_id: str) -> Dict: popularity = None if engagement_row: - from app.intelligence import features as F feat_row = pd.DataFrame([{ "views_norm": min(engagement_row["views"] / 10, 100), "wishlist_norm": min(engagement_row["wishlist_count"] / 2, 100), diff --git a/app/services/nutrition_alternatives_service.py b/app/services/nutrition_alternatives_service.py index 25ccc91..2491612 100644 --- a/app/services/nutrition_alternatives_service.py +++ b/app/services/nutrition_alternatives_service.py @@ -10,7 +10,7 @@ ranked by health_score improvement rather than by distance. """ from __future__ import annotations -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List from app.services import nutrition_db diff --git a/app/services/nutrition_db.py b/app/services/nutrition_db.py index 0878618..21c8623 100644 --- a/app/services/nutrition_db.py +++ b/app/services/nutrition_db.py @@ -39,7 +39,6 @@ treating NULL as "zero". """ from __future__ import annotations -import json import logging from typing import Any, Dict, List, Optional diff --git a/app/services/product_validator.py b/app/services/product_validator.py index 9cdda23..2ab5a6e 100644 --- a/app/services/product_validator.py +++ b/app/services/product_validator.py @@ -46,6 +46,11 @@ from typing import Any, Optional from pydantic import BaseModel, Field +from app.infrastructure.settings import ( + VALIDATION_REJECT_THRESHOLD, + VALIDATION_REVIEW_THRESHOLD, +) + from app.services.title_validator import find_category_conflicts from app.services import price_estimator from app.services import category_units as cu @@ -119,10 +124,20 @@ class ValidationIssue(BaseModel): class ValidationConfig(BaseModel): """Tunable thresholds - see docs/VALIDATION_PIPELINE.md for defaults - and how to change them via settings.py without editing this file.""" + and how to change them via settings.py without editing this file. - reject_below: float = 0.35 - review_below: float = 0.70 + The two settings really are read now. They were declared in settings.py + with these same numbers as their defaults, and nothing ever imported them: + the values below were hardcoded copies, so setting VALIDATION_REJECT_THRESHOLD + in the environment did exactly nothing while appearing to work. + + `default_factory` rather than a plain default so the setting is read when a + config is constructed, not once at import - which is what lets a test move + the threshold without reloading the module. + """ + + reject_below: float = Field(default_factory=lambda: VALIDATION_REJECT_THRESHOLD) + review_below: float = Field(default_factory=lambda: VALIDATION_REVIEW_THRESHOLD) class ValidationReport(BaseModel): diff --git a/app/services/recommendation_service.py b/app/services/recommendation_service.py index 18000c4..008ab3d 100644 --- a/app/services/recommendation_service.py +++ b/app/services/recommendation_service.py @@ -6,13 +6,11 @@ functions need, then persists/reads the result cache via `store_db.py`. from __future__ import annotations import logging -from typing import Dict, List, Optional +from typing import Dict, List import numpy as np -import pandas as pd from app.intelligence import recommendation_engine as RE -from app.intelligence.popularity_model import popularity_scorer from app.services import store_db logger = logging.getLogger(__name__) diff --git a/app/services/store_db.py b/app/services/store_db.py index 3bea1c3..773ceee 100644 --- a/app/services/store_db.py +++ b/app/services/store_db.py @@ -34,7 +34,7 @@ from __future__ import annotations import json import logging -from datetime import date, datetime +from datetime import date from typing import Any, Dict, List, Optional import pandas as pd diff --git a/app/services/title_validator.py b/app/services/title_validator.py index 063f0fc..fd525e6 100644 --- a/app/services/title_validator.py +++ b/app/services/title_validator.py @@ -39,7 +39,7 @@ from __future__ import annotations import re import logging -from typing import Iterable, Optional +from typing import Optional logger = logging.getLogger(__name__) diff --git a/app/services/vector_store.py b/app/services/vector_store.py index 42bab06..4b3028d 100644 --- a/app/services/vector_store.py +++ b/app/services/vector_store.py @@ -1,8 +1,7 @@ from __future__ import annotations -from typing import List, Optional, Dict, Any +from typing import List, Optional, Dict, Any, Tuple -import json import logging import os import re @@ -10,7 +9,7 @@ import time import psycopg from app.infrastructure.settings import ( - DATABASE_URL, USE_PGVECTOR, DB_HOST, DB_PORT, DB_NAME, DB_USER, DB_PASSWORD, + USE_PGVECTOR, DB_HOST, DB_PORT, DB_NAME, DB_USER, DB_PASSWORD, DB_CONNECT_TIMEOUT_SECONDS, ) from app.services.brand_registry import BRAND_ALIASES, resolve_parent_brand @@ -161,8 +160,8 @@ def _ensure_columns(cur, table_name: str) -> None: "embedding": "vector(384)", } cur.execute( - f"SELECT column_name, is_nullable, column_default FROM information_schema.columns " - f"WHERE table_schema = 'public' AND table_name = %s", + "SELECT column_name, is_nullable, column_default FROM information_schema.columns " + "WHERE table_schema = 'public' AND table_name = %s", (table_name,), ) col_info = cur.fetchall() diff --git a/orchestration/README.md b/orchestration/README.md index 3af744c..98d5c35 100644 --- a/orchestration/README.md +++ b/orchestration/README.md @@ -251,9 +251,10 @@ allocates the whole 8GB VPS (ollama 3G, backend 2560M, postgres 1G), so there is no headroom for a webserver plus daemon. Orchestration is a development concern here. -**This includes `batch_ingestion_job`.** The Batch Catalog Ingestion screen on -the site does not talk to Dagster and does not need it running. It stages the -uploaded files, then executes the same functions this job wraps +**This includes `batch_ingestion_job`.** The catalog ingestion endpoints +(`POST /api/admin/catalog-batch/ingest` and `POST /api/uploads/catalog`) do not +talk to Dagster and do not need it running. They stage the uploaded files, then +execute the same functions this job wraps (`app/core/batch_ingest.py`) on a single bounded worker thread inside the API process - see `app/core/batch_worker.py` for why one worker rather than a thread per upload. The job here exists so the graph is inspectable and a batch diff --git a/orchestration/assets/batch_catalog.py b/orchestration/assets/batch_catalog.py index 55d63a3..4734f88 100644 --- a/orchestration/assets/batch_catalog.py +++ b/orchestration/assets/batch_catalog.py @@ -73,9 +73,10 @@ def _pick_batch_id(config: BatchConfig) -> str: raise Failure( description=( "No batch_id given and no staged batch is waiting to run.\n\n" - "Upload one through Admin -> Batch Catalog Ingestion on the site, " - "or pass {\"batch_id\": \"...\"} in the Launchpad. Staged batches " - "live under BATCH_UPLOAD_DIR ({}).".format(batch_ingest.batch_root()) + "Stage one through POST /api/admin/catalog-batch/ingest (or " + "POST /api/uploads/catalog), or pass {\"batch_id\": \"...\"} in " + "the Launchpad. Staged batches live under BATCH_UPLOAD_DIR " + f"({batch_ingest.batch_root()})." ), metadata={"staged_batches": len(batch_ingest.list_manifests())}, ) diff --git a/scripts/export_seed_data.py b/scripts/export_seed_data.py index 56dd45e..e6184bc 100644 --- a/scripts/export_seed_data.py +++ b/scripts/export_seed_data.py @@ -22,7 +22,7 @@ from typing import Dict, List, Any sys.path.insert(0, str(Path(__file__).resolve().parents[1])) -from app.services.vector_store import _connect, _sanitize_name +from app.services.vector_store import _connect from app.infrastructure.settings import DB_HOST, DB_PORT, DB_NAME logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s") diff --git a/scripts/fast_test.py b/scripts/fast_test.py index 350b3b5..8e082b6 100644 --- a/scripts/fast_test.py +++ b/scripts/fast_test.py @@ -1,7 +1,6 @@ import sys -import os sys.path.insert(0, ".") -from app.services.rag_service import retrieve, answer_query +from app.services.rag_service import retrieve from app.services.query_intent import extract_max_price, extract_brand_mention, is_count_query, detected_category print("=== Q1: How many products are there in Cadbury? ===") diff --git a/scripts/make_auth_secrets.py b/scripts/make_auth_secrets.py index d60a387..25d2e49 100644 --- a/scripts/make_auth_secrets.py +++ b/scripts/make_auth_secrets.py @@ -192,7 +192,12 @@ def main() -> None: metavar="NAME:ROLE", action="append", default=[], - help="Also mint an API_KEYS entry, e.g. --api-key partner-x:user (repeatable)", + help=( + "Also mint an API_KEYS entry, e.g. --api-key partner-x:user " + "(repeatable). Role is admin, user, or uploader - uploader grants " + "only POST /api/uploads/catalog, which is the one to issue to an " + "outside party sending you spreadsheets." + ), ) parser.add_argument( "--fingerprint", @@ -229,11 +234,11 @@ def main() -> None: user_password = args.user_password or generate_password() print("# --- Paste into backend/.env -------------------------------------") - print(f"AUTH_ENABLED=true") + print("AUTH_ENABLED=true") print(f"AUTH_SECRET_KEY={secrets.token_urlsafe(48)}") - print(f"AUTH_ADMIN_USERNAME=admin") + print("AUTH_ADMIN_USERNAME=admin") print(f"AUTH_ADMIN_PASSWORD_HASH={hash_password(admin_password)}") - print(f"AUTH_USER_USERNAME=user") + print("AUTH_USER_USERNAME=user") print(f"AUTH_USER_PASSWORD_HASH={hash_password(user_password)}") if args.api_key: @@ -244,8 +249,13 @@ def main() -> None: name, role = spec.split(":", 1) except ValueError: parser.error(f"--api-key expects NAME:ROLE, got {spec!r}") - if role not in {"admin", "user"}: - parser.error(f"--api-key role must be 'admin' or 'user', got {role!r}") + # Must stay in step with ROLE_PERMISSIONS in + # app/infrastructure/security.py and the same list in + # settings._parse_api_keys, which rejects an unknown role at boot. + if role not in {"admin", "user", "uploader"}: + parser.error( + f"--api-key role must be 'admin', 'user' or 'uploader', got {role!r}" + ) key = secrets.token_urlsafe(32) entries.append(f"{name}:{role}:{key}") secrets_shown.append((name, key)) diff --git a/scripts/test_rag.py b/scripts/test_rag.py index 5e379f1..5f71cc1 100644 --- a/scripts/test_rag.py +++ b/scripts/test_rag.py @@ -1,4 +1,3 @@ -import sys from app.services.rag_service import retrieve, answer_query from app.services.query_intent import extract_max_price, extract_brand_mention, is_count_query diff --git a/tests/test_inbox_upload.py b/tests/test_inbox_upload.py deleted file mode 100644 index 1aca704..0000000 --- a/tests/test_inbox_upload.py +++ /dev/null @@ -1,390 +0,0 @@ -"""Two-actor flow: a colleague drops files, an admin decides what runs. - -Follows test_batch_catalog_ingest.py: point the directories at tmp_path, -monkeypatch the storage/embedding boundary, and stub the background worker by -default. Nothing here loads sentence-transformers or torch, nothing reaches the -network, and nothing writes into the repository's data directory. - -The most important tests in this file are the ones asserting what an `uploader` -credential CANNOT do. That role exists specifically so an outside contributor -does not get the `user` role's catalog write access, and an assertion is the -only thing that keeps it true as endpoints are added. -""" -from __future__ import annotations - -import io - -import pytest - -from app.core import batch_ingest, inbox -from app.core import store_catalog_pipeline as pipeline - -openpyxl = pytest.importorskip("openpyxl") - -HEADERS = ["Product Name", "Category", "Brand"] -ROWS = [["Amul Butter 100g", "Butter", "Amul"]] - -UPLOAD_KEY = "k" * 43 -UPLOAD = "/api/uploads/catalog" -INBOX = "/api/admin/catalog-batch/inbox" -FROM_INBOX = "/api/admin/catalog-batch/from-inbox" -DISMISS = "/api/admin/catalog-batch/inbox/dismiss" - - -def _csv(rows=ROWS) -> bytes: - return ("\n".join([",".join(HEADERS)] + [",".join(r) for r in rows])).encode() - - -def _sheet(rows=ROWS) -> bytes: - wb = openpyxl.Workbook() - ws = wb.active - ws.append(HEADERS) - for row in rows: - ws.append(row) - buf = io.BytesIO() - wb.save(buf) - return buf.getvalue() - - -def _files(*pairs): - return [("files", (n, io.BytesIO(c), "application/octet-stream")) - for n, c in pairs] - - -@pytest.fixture(autouse=True) -def _isolate_sku_counter(tmp_path, monkeypatch): - from app.services import sku_service - monkeypatch.setattr(sku_service, "_data_dir", tmp_path / "sku_sequences") - - -@pytest.fixture(autouse=True) -def dirs(tmp_path, monkeypatch): - """Both staging directories under tmp_path. Read at call time by design.""" - monkeypatch.setattr(inbox, "INBOX_UPLOAD_DIR", tmp_path / "inbox") - monkeypatch.setattr(batch_ingest, "BATCH_UPLOAD_DIR", tmp_path / "batch_uploads") - return tmp_path - - -@pytest.fixture(autouse=True) -def no_background_worker(monkeypatch): - """Same trap as in test_batch_catalog_ingest: a worker still running after - teardown resolves the directory setting again and writes into the real - data/ directory. Stub it; the tests here are about routing and state.""" - from app.core import batch_worker - - submitted: list = [] - monkeypatch.setattr(batch_worker, "submit", submitted.append) - return submitted - - -@pytest.fixture(autouse=True) -def upload_key(monkeypatch): - """One uploader API key. - - Patched on `security`, not on `settings`: security.py does - `from ...settings import API_KEYS` at import, binding the dict object, so - rebinding the name in `settings` leaves `principal_for_api_key` still - looking at the original and every request comes back 401. - """ - from app.infrastructure import security - - monkeypatch.setattr( - security, "API_KEYS", {UPLOAD_KEY: ("catalog-drop", "uploader")} - ) - return {"X-API-Key": UPLOAD_KEY} - - -@pytest.fixture -def store(monkeypatch): - table: dict = {} - - def fake_upsert(brand, rows, cleanup=False): - assert cleanup is False - for row in rows: - table[row["image_id"]] = dict(row) - return len(rows) - - monkeypatch.setattr(pipeline, "upsert_brand_products", fake_upsert) - monkeypatch.setattr(pipeline, "get_products_by_brand", lambda b, **kw: list(table.values())) - monkeypatch.setattr(pipeline, "embed_texts", lambda texts: [[0.0] * 384 for _ in texts]) - return table - - -def _submit(client, upload_key, *pairs): - return client.post(UPLOAD, files=_files(*pairs), headers=upload_key) - - -# --------------------------------------------------------------------------- -# The security boundary - the reason the `uploader` role exists -# --------------------------------------------------------------------------- -def test_uploader_key_can_submit(client, upload_key): - r = _submit(client, upload_key, ("a.csv", _csv())) - assert r.status_code == 202 - body = r.json() - assert body["files_accepted"] == 1 - assert body["submitted_by"] == "catalog-drop" - - -@pytest.mark.parametrize("method,path", [ - ("get", "/api/admin/catalog-batch/inbox"), - ("post", "/api/admin/catalog-batch/from-inbox"), - ("post", "/api/admin/catalog-batch/inbox/dismiss"), - ("post", "/api/admin/catalog-batch/preview"), - ("post", "/api/admin/catalog-batch/ingest"), - ("get", "/api/admin/catalog-batch/batches"), - ("get", "/api/admin/catalog-batch/batches/abc"), - ("post", "/api/admin/catalog-batch/batches/abc/resume"), - ("post", "/api/admin/catalog-batch/batches/abc/cancel"), - ("post", "/api/admin/store-catalog/preview"), - ("post", "/api/admin/store-catalog/ingest"), - ("get", "/api/admin/store-catalog/jobs/abc"), -]) -def test_uploader_key_is_refused_on_every_admin_route(client, upload_key, method, path): - """An uploader credential must reach nothing but its own endpoint.""" - r = getattr(client, method)(path, headers=upload_key) - assert r.status_code == 403, f"{method.upper()} {path} returned {r.status_code}" - - -@pytest.mark.parametrize("path", [ - "/api/user/products/add", - "/api/user/products/upload-file", - "/api/upload/stores", -]) -def test_uploader_key_has_none_of_the_user_roles_write_access(client, upload_key, path): - """This is the specific over-grant avoided by NOT reusing the `user` role. - - A `user` key would carry add_product / upload_batch_products / - upload_store_inventory - real catalog writes - to solve a problem that - needed one verb. - """ - r = client.post(path, headers=upload_key) - assert r.status_code == 403, f"{path} returned {r.status_code}" - - -def test_upload_endpoint_rejects_anonymous(client): - r = client.post(UPLOAD, files=_files(("a.csv", _csv()))) - assert r.status_code == 401 - - -def test_admin_can_still_use_the_upload_endpoint(client, admin_headers): - """admin bypasses every permission check (Principal.has_permission).""" - r = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=admin_headers) - assert r.status_code == 202 - - -def test_role_allow_lists_stay_in_step(client): - """settings._parse_api_keys duplicates the role names from security.py - because importing back would be a cycle. Pin the duplication.""" - from app.infrastructure import settings - from app.infrastructure.security import VALID_ROLES - - for role in VALID_ROLES: - parsed = settings._parse_api_keys(f"n:{role}:{'x' * 43}") - assert parsed[("x" * 43)] == ("n", role) - - with pytest.raises(RuntimeError): - settings._parse_api_keys(f"n:wizard:{'x' * 43}") - - -# --------------------------------------------------------------------------- -# Intake -# --------------------------------------------------------------------------- -def test_a_broken_sheet_is_refused_at_upload_and_never_enters_the_inbox(client, upload_key): - r = _submit(client, upload_key, ("good.csv", _csv()), ("broken.pdf", b"%PDF-1.4")) - assert r.status_code == 202 - body = r.json() - assert body["files_accepted"] == 1 - assert body["files_rejected"] == 1 - bad = [f for f in body["files"] if f["filename"] == "broken.pdf"][0] - assert bad["accepted"] is False and bad["error"] - # Only the good one is waiting. - assert inbox.pending_count() == 1 - - -def test_a_drop_where_nothing_parses_stages_no_submission(client, upload_key): - r = _submit(client, upload_key, ("broken.pdf", b"%PDF-1.4")) - assert r.status_code == 202 - assert r.json()["submission_id"] is None - assert inbox.pending_count() == 0 - assert inbox.list_submissions() == [] - - -def test_uploaded_filenames_cannot_escape_the_inbox(client, upload_key, dirs): - r = _submit(client, upload_key, ("../../evil.csv", _csv())) - assert r.status_code == 202 - submission = inbox.list_submissions()[0] - directory = inbox.submission_dir(submission.submission_id) - for entry in submission.files: - written = directory / entry.stored_name - assert written.resolve().parent == directory.resolve() - assert not (dirs / "evil.csv").exists() - - -def test_a_full_inbox_refuses_further_uploads(client, upload_key, monkeypatch): - from app.api.routers import uploads - - monkeypatch.setattr(uploads, "INBOX_MAX_PENDING_FILES", 1) - assert _submit(client, upload_key, ("a.csv", _csv())).status_code == 202 - r = _submit(client, upload_key, ("b.csv", _csv())) - assert r.status_code == 429 - assert "awaiting review" in r.json()["detail"] - - -def test_too_many_files_in_one_drop_is_413(client, upload_key, monkeypatch): - from app.api.routers import uploads - - monkeypatch.setattr(uploads, "BATCH_MAX_FILES", 2) - r = _submit(client, upload_key, - ("a.csv", _csv()), ("b.csv", _csv()), ("c.csv", _csv())) - assert r.status_code == 413 - - -def test_uploading_starts_no_work(client, upload_key, no_background_worker): - """The whole safety property: an uploader can spend disk, never CPU.""" - _submit(client, upload_key, ("a.csv", _csv())) - assert no_background_worker == [] - - -# --------------------------------------------------------------------------- -# The admin side -# --------------------------------------------------------------------------- -def test_inbox_lists_pending_grouped_by_drop(client, upload_key, admin_headers): - _submit(client, upload_key, ("a.csv", _csv()), ("b.csv", _csv())) - _submit(client, upload_key, ("c.csv", _csv())) - - r = client.get(INBOX, headers=admin_headers) - assert r.status_code == 200 - body = r.json() - assert body["pending_count"] == 3 - assert len(body["submissions"]) == 2 - assert all(s["submitted_by"] == "catalog-drop" for s in body["submissions"]) - assert body["submissions"][0]["files"][0]["rows_total"] == 1 - - -def test_selecting_files_across_two_drops_makes_one_batch( - client, upload_key, admin_headers, store, no_background_worker -): - """The requirement that ruled out reusing batch-resume: files from - different drops, composed into a single run.""" - _submit(client, upload_key, ("a.csv", _csv()), ("b.csv", _csv())) - _submit(client, upload_key, ("c.csv", _csv())) - - listing = client.get(INBOX, headers=admin_headers).json() - first = listing["submissions"][0]["files"][0]["file_id"] - second = listing["submissions"][1]["files"][0]["file_id"] - - r = client.post(FROM_INBOX, json={"file_ids": [first, second]}, headers=admin_headers) - assert r.status_code == 202 - batch = r.json() - assert batch["files_total"] == 2 - assert batch["submitted_by"] == "catalog-drop" - assert no_background_worker == [batch["batch_id"]] - - # The unselected file is untouched. - after = client.get(INBOX, headers=admin_headers).json() - assert after["pending_count"] == 1 - - -def test_a_started_file_cannot_be_started_twice(client, upload_key, admin_headers, store): - """Two admin tabs on the same inbox must not double-run a file.""" - _submit(client, upload_key, ("a.csv", _csv())) - file_id = client.get(INBOX, headers=admin_headers).json()["submissions"][0]["files"][0]["file_id"] - - assert client.post(FROM_INBOX, json={"file_ids": [file_id]}, headers=admin_headers).status_code == 202 - again = client.post(FROM_INBOX, json={"file_ids": [file_id]}, headers=admin_headers) - assert again.status_code == 409 - assert "already consumed" in again.json()["detail"] - - -def test_selecting_an_unknown_file_is_404(client, admin_headers): - r = client.post(FROM_INBOX, json={"file_ids": ["nope"]}, headers=admin_headers) - assert r.status_code == 404 - - -def test_selecting_nothing_is_400(client, admin_headers): - assert client.post(FROM_INBOX, json={"file_ids": []}, headers=admin_headers).status_code == 400 - - -def test_dismiss_clears_the_badge_without_running_anything( - client, upload_key, admin_headers, no_background_worker -): - _submit(client, upload_key, ("a.csv", _csv())) - file_id = client.get(INBOX, headers=admin_headers).json()["submissions"][0]["files"][0]["file_id"] - - r = client.post(DISMISS, json={"file_ids": [file_id]}, headers=admin_headers) - assert r.status_code == 200 - assert r.json() == {"dismissed": 1, "pending_count": 0} - assert no_background_worker == [], "dismiss must not start work" - assert client.get(INBOX, headers=admin_headers).json()["pending_count"] == 0 - - -def test_dismissing_an_already_decided_file_is_409(client, upload_key, admin_headers): - _submit(client, upload_key, ("a.csv", _csv())) - file_id = client.get(INBOX, headers=admin_headers).json()["submissions"][0]["files"][0]["file_id"] - client.post(DISMISS, json={"file_ids": [file_id]}, headers=admin_headers) - again = client.post(DISMISS, json={"file_ids": [file_id]}, headers=admin_headers) - assert again.status_code == 409 - - -# --------------------------------------------------------------------------- -# End to end, and retention -# --------------------------------------------------------------------------- -def test_selected_files_actually_reach_the_catalog(client, upload_key, admin_headers, store): - """Run the composed batch for real (worker stubbed, pipeline not).""" - _submit(client, upload_key, ("a.csv", _csv()), ("b.xlsx", _sheet())) - listing = client.get(INBOX, headers=admin_headers).json() - ids = [f["file_id"] for f in listing["submissions"][0]["files"]] - - started = client.post(FROM_INBOX, json={"file_ids": ids}, headers=admin_headers).json() - result = batch_ingest.run_batch(started["batch_id"]) - - assert result.status == "done" - assert result.files_done == 2 - assert result.submitted_by == "catalog-drop" - assert store, "no rows reached the fake brand table" - - -def test_purge_removes_decided_submissions_and_spares_pending(client, upload_key, admin_headers, monkeypatch): - import time - - monkeypatch.setattr(inbox, "INBOX_RETENTION_DAYS", 7) - - _submit(client, upload_key, ("old.csv", _csv())) - old = inbox.list_submissions()[0] - file_id = old.files[0].file_id - client.post(DISMISS, json={"file_ids": [file_id]}, headers=admin_headers) - aged = inbox.read_record(old.submission_id) - aged.created_at = time.time() - 30 * 86400 - inbox.write_record(aged) - - _submit(client, upload_key, ("pending.csv", _csv())) - still_pending = [s for s in inbox.list_submissions() if s.pending_files][0] - stale_but_unreviewed = inbox.read_record(still_pending.submission_id) - stale_but_unreviewed.created_at = time.time() - 30 * 86400 - inbox.write_record(stale_but_unreviewed) - - removed = inbox.purge_expired() - - assert removed == [old.submission_id] - assert not inbox.submission_dir(old.submission_id).exists() - assert inbox.submission_dir(still_pending.submission_id).exists(), \ - "a file nobody has reviewed must never be purged" - - -def test_purge_is_disabled_when_retention_is_zero(client, upload_key, monkeypatch): - monkeypatch.setattr(inbox, "INBOX_RETENTION_DAYS", 0) - _submit(client, upload_key, ("a.csv", _csv())) - assert inbox.purge_expired() == [] - - -def test_unreadable_record_is_skipped_not_raised(client, upload_key): - _submit(client, upload_key, ("a.csv", _csv())) - sid = inbox.list_submissions()[0].submission_id - inbox.record_path(sid).write_text("{ not json", encoding="utf-8") - assert inbox.read_record(sid) is None - assert inbox.list_submissions() == [] - - -def test_submission_dir_rejects_a_traversing_id(): - with pytest.raises(ValueError): - inbox.submission_dir("../escape") diff --git a/tests/test_upload_data_integrity.py b/tests/test_upload_data_integrity.py new file mode 100644 index 0000000..6e991f9 --- /dev/null +++ b/tests/test_upload_data_integrity.py @@ -0,0 +1,402 @@ +"""The operator upload endpoints must never invent a value. + + POST /api/upload/stores + POST /api/upload/analytics + POST /api/upload/nutrition + +These three used to substitute a plausible constant for every column a sheet +did not carry, so a header mismatch produced rows filed under brand "amul" in +store "store_mumbai_1" - and nutrition rows with invented calories and +`allergens = ['None']` written as `data_status = 'verified'`. + +Every test here pins the opposite behaviour: a missing identifying column +SKIPS the row and reports it, and a missing value is stored as NULL. The +allergen tests are the ones that matter most - "we were not told" and "this +product is allergen-free" must never become the same database row. + +No database is involved: `_connect` is replaced with a recorder, so the +assertions are about the exact SQL and parameters the handlers would send. +""" +from __future__ import annotations + +import io + +import pytest + +from app.api.routers import upload as upload_router + +openpyxl = pytest.importorskip("openpyxl") + + +class FakeCursor: + def __init__(self, calls): + self.calls = calls + + def execute(self, sql, params=None): + self.calls.append((" ".join(str(sql).split()), params)) + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + +class FakeConn: + def __init__(self): + self.calls = [] + self.closed = False + + def cursor(self): + return FakeCursor(self.calls) + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + def close(self): + self.closed = True + + +@pytest.fixture +def db(monkeypatch): + """Swap the connection and the two schema bootstraps for recorders.""" + conn = FakeConn() + monkeypatch.setattr(upload_router, "_connect", lambda: conn) + monkeypatch.setattr( + upload_router.store_db, "ensure_store_intelligence_schema", lambda: None + ) + monkeypatch.setattr( + upload_router.nutrition_db, "ensure_nutrition_schema", lambda: None + ) + return conn + + +def _csv(text: str): + return {"file": ("sheet.csv", io.BytesIO(text.encode()), "text/csv")} + + +def _find(conn, needle): + """Every recorded statement containing `needle`, with its parameters.""" + return [(sql, params) for sql, params in conn.calls if needle in sql] + + +# --------------------------------------------------------------------------- +# Stores +# --------------------------------------------------------------------------- +STORES_OK = ( + "store_id,brand,product_name,category,available_stock,mrp,cost_price,selling_price\n" + "store_mumbai_1,Amul,Amul Butter 500ml,Dairy,120,250,200,235\n" +) + + +def test_a_store_row_without_brand_is_skipped_not_filed_under_amul(client, admin_headers, db): + body = "store_id,product_name,available_stock\nstore_1,Butter,10\n" + response = client.post("/api/upload/stores", files=_csv(body), headers=admin_headers) + + assert response.status_code == 422 + detail = response.json()["detail"] + assert "brand" in detail["errors"][0]["error"] + assert detail["errors"][0]["row"] == 2 # header is row 1 + # The old code would have inserted with brand 'amul'. + assert _find(db, "INSERT INTO store_inventory") == [] + assert "amul" not in response.text.lower() + + +def test_a_store_row_without_a_store_id_is_skipped(client, admin_headers, db): + body = "brand,product_name\namul,Butter\n" + response = client.post("/api/upload/stores", files=_csv(body), headers=admin_headers) + assert response.status_code == 422 + assert "store_id" in response.json()["detail"]["errors"][0]["error"] + + +def test_a_complete_store_row_is_imported_with_its_own_values(client, admin_headers, db): + response = client.post("/api/upload/stores", files=_csv(STORES_OK), headers=admin_headers) + assert response.status_code == 200, response.text + body = response.json() + assert body["rows_imported"] == 1 + assert body["rows_skipped"] == 0 + assert body["stores_affected"] == ["store_mumbai_1"] + + _sql, params = _find(db, "INSERT INTO store_inventory")[0] + assert params[0] == "store_mumbai_1" + assert params[1] == "amul" + assert params[3] == "Amul Butter 500ml" + assert params[4] == "Dairy" + assert params[5] == 120 + + +def test_absent_category_and_city_are_stored_as_null(client, admin_headers, db): + body = "store_id,brand,product_name\nstore_1,amul,Butter\n" + assert client.post("/api/upload/stores", files=_csv(body), + headers=admin_headers).status_code == 200 + + _sql, inv = _find(db, "INSERT INTO store_inventory")[0] + assert inv[4] is None, "category was invented" + _sql, store = _find(db, "INSERT INTO stores")[0] + assert store[2] is None, "city was invented ('Mumbai')" + + +def test_absent_stock_is_zero_not_fifty(client, admin_headers, db): + body = "store_id,brand,product_name\nstore_1,amul,Butter\n" + client.post("/api/upload/stores", files=_csv(body), headers=admin_headers) + + _sql, inv = _find(db, "INSERT INTO store_inventory")[0] + assert inv[5] == 0, "available_stock was invented" + assert inv[6] == 0 and inv[7] == 0 and inv[8] == 0 + + +def test_prices_are_written_only_when_the_whole_triple_is_present(client, admin_headers, db): + response = client.post("/api/upload/stores", files=_csv(STORES_OK), headers=admin_headers) + assert response.json()["prices_written"] == 1 + _sql, params = _find(db, "INSERT INTO store_prices")[0] + assert params[3] == 250.0 and params[4] == 200.0 and params[5] == 235.0 + + +def test_a_missing_cost_price_skips_prices_but_keeps_the_inventory(client, admin_headers, db): + """The old code invented cost = mrp * 0.7 and a selling price to match.""" + body = "store_id,brand,product_name,mrp,selling_price\nstore_1,amul,Butter,250,235\n" + response = client.post("/api/upload/stores", files=_csv(body), headers=admin_headers) + + assert response.status_code == 200 + assert response.json()["prices_written"] == 0 + assert response.json()["prices_skipped"] == 1 + assert _find(db, "INSERT INTO store_prices") == [] + assert len(_find(db, "INSERT INTO store_inventory")) == 1 + + +def test_a_partial_file_imports_the_good_rows_and_reports_the_rest(client, admin_headers, db): + body = ( + "store_id,brand,product_name\n" + "store_1,amul,Butter\n" + ",,\n" + "store_1,nestle,Milk Powder\n" + ) + response = client.post("/api/upload/stores", files=_csv(body), headers=admin_headers) + assert response.status_code == 200 + assert response.json()["status"] == "partial" + assert response.json()["rows_imported"] == 2 + assert response.json()["rows_skipped"] == 1 + + +# --------------------------------------------------------------------------- +# Analytics - the endpoint that never worked +# --------------------------------------------------------------------------- +ANALYTICS_OK = ( + "order_id,store_id,brand,image_id,order_date,customer_id,quantity,unit_price,total_price\n" + "ORD_1,store_1,amul,amul_butter,2026-08-01 10:30:00,cust_101,2,235.00,470.00\n" +) + + +def test_order_items_uses_the_columns_the_table_actually_has(client, admin_headers, db): + """REGRESSION: the insert named `total_price` (not a column) and omitted + `store_id` (NOT NULL), so every analytics upload raised and returned 500.""" + response = client.post("/api/upload/analytics", files=_csv(ANALYTICS_OK), + headers=admin_headers) + assert response.status_code == 200, response.text + + sql, params = _find(db, "INSERT INTO order_items")[0] + assert "line_total" in sql + assert "total_price" not in sql + assert "store_id" in sql + # (order_id, store_id, brand, image_id, quantity, unit_price, line_total) + assert params == ("ORD_1", "store_1", "amul", "amul_butter", 2, 235.0, 470.0) + + +def test_line_total_is_derived_when_the_sheet_omits_it(client, admin_headers, db): + body = ("store_id,brand,image_id,customer_id,quantity,unit_price\n" + "store_1,amul,amul_butter,cust_1,3,100.00\n") + client.post("/api/upload/analytics", files=_csv(body), headers=admin_headers) + + _sql, params = _find(db, "INSERT INTO order_items")[0] + assert params[6] == 300.0, "quantity * unit_price is arithmetic, not invention" + + +def test_a_sale_without_a_customer_is_skipped_not_merged_into_one_buyer(client, admin_headers, db): + """'cust_imported' collapsed every buyer in a file into a single customer.""" + body = "store_id,brand,image_id,quantity,unit_price\nstore_1,amul,x,1,10\n" + response = client.post("/api/upload/analytics", files=_csv(body), headers=admin_headers) + + assert response.status_code == 422 + assert "customer_id" in response.json()["detail"]["errors"][0]["error"] + assert _find(db, "INSERT INTO order_items") == [] + + +def test_a_sale_without_quantity_or_price_is_skipped(client, admin_headers, db): + body = "store_id,brand,image_id,customer_id\nstore_1,amul,x,cust_1\n" + response = client.post("/api/upload/analytics", files=_csv(body), headers=admin_headers) + assert response.status_code == 422 + err = response.json()["detail"]["errors"][0]["error"] + assert "quantity" in err and "unit_price" in err + + +def test_order_value_is_recomputed_from_its_lines(client, admin_headers, db): + """A two-line order must not end up carrying only its last line's value.""" + body = ( + "order_id,store_id,brand,image_id,customer_id,quantity,unit_price\n" + "ORD_1,store_1,amul,a,cust_1,1,100\n" + "ORD_1,store_1,amul,b,cust_1,1,50\n" + ) + response = client.post("/api/upload/analytics", files=_csv(body), headers=admin_headers) + assert response.status_code == 200 + + updates = _find(db, "UPDATE orders") + assert len(updates) == 1 + assert "SUM(line_total)" in updates[0][0] + assert updates[0][1] == (["ORD_1"],) + + +def test_an_undated_row_is_counted_as_such(client, admin_headers, db): + body = ("store_id,brand,image_id,customer_id,quantity,unit_price\n" + "store_1,amul,x,cust_1,1,10\n") + response = client.post("/api/upload/analytics", files=_csv(body), headers=admin_headers) + assert response.json()["dates_defaulted_to_now"] == 1 + + +# --------------------------------------------------------------------------- +# Nutrition - the data-integrity rule this module exists for +# --------------------------------------------------------------------------- +NUTRITION_FULL = ( + "brand,image_id,product_name,calories_kcal,protein_g,carbohydrates_g," + "total_sugar_g,dietary_fiber_g,total_fat_g,sodium_mg,allergens\n" + "amul,amul_butter,Amul Butter,717,0.8,0.1,0.0,0.0,81.0,650,Milk\n" +) + + +def _facts(db): + return _find(db, "INSERT INTO nutrition_facts")[0][1] + + +def _insights(db): + hits = _find(db, "INSERT INTO nutrition_insights") + return hits[0][1] if hits else None + + +def test_an_absent_allergen_column_is_null_never_none(client, admin_headers, db): + """THE test. 'We were not told' must not become 'contains no allergens'.""" + body = "brand,image_id,product_name,protein_g\namul,x,Butter,5.0\n" + assert client.post("/api/upload/nutrition", files=_csv(body), + headers=admin_headers).status_code == 200 + + params = _insights(db) + assert params is not None + allergens, allergen_source = params[8], params[9] + assert allergens is None, "allergens were invented" + assert allergen_source == "unavailable" + + +def test_an_absent_diet_tag_column_is_null(client, admin_headers, db): + """It used to default to ['High Protein', 'Gluten Free'] for everything.""" + body = "brand,image_id,product_name,protein_g\namul,x,Butter,5.0\n" + client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + assert _insights(db)[7] is None + + +def test_absent_nutrients_are_null_not_plausible_numbers(client, admin_headers, db): + body = "brand,image_id,product_name\namul,x,Butter\n" + client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + + params = _facts(db) + # (brand, image_id, product_name, category, data_status, then 10 nutrients) + assert params[3] is None # category + assert all(v is None for v in params[5:]), "a nutrient was invented" + + +def test_a_row_with_no_nutrients_is_unavailable_not_verified(client, admin_headers, db): + body = "brand,image_id,product_name\namul,x,Butter\n" + response = client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + + assert _facts(db)[4] == "unavailable" + assert response.json()["data_status_counts"]["unavailable"] == 1 + + +def test_a_row_with_some_nutrients_is_partial(client, admin_headers, db): + body = "brand,image_id,product_name,protein_g,total_sugar_g\namul,x,Butter,5.0,2.0\n" + response = client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + + assert _facts(db)[4] == "partial" + assert response.json()["data_status_counts"]["partial"] == 1 + + +def test_only_a_complete_core_set_is_verified(client, admin_headers, db): + response = client.post("/api/upload/nutrition", files=_csv(NUTRITION_FULL), + headers=admin_headers) + assert _facts(db)[4] == "verified" + assert response.json()["data_status_counts"]["verified"] == 1 + + params = _insights(db) + assert params[8] == ["Milk"] + assert params[9] == "upload" + + +def test_insights_are_built_only_from_supplied_values(client, admin_headers, db): + body = "brand,image_id,product_name,protein_g\namul,x,Butter,5.0\n" + client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + + params = _insights(db) + positives, cautions = params[5], params[6] + assert positives == ["Contains 5.0g protein per 100g"] + assert cautions is None, "a sugar caution was invented from no sugar value" + + +def test_no_insight_row_at_all_when_nothing_was_supplied(client, admin_headers, db): + body = "brand,image_id,product_name\namul,x,Butter\n" + client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + assert _find(db, "INSERT INTO nutrition_insights") == [] + + +def test_the_score_breakdown_is_no_longer_a_hardcoded_constant(client, admin_headers, db): + client.post("/api/upload/nutrition", files=_csv(NUTRITION_FULL), headers=admin_headers) + sql, _params = _find(db, "INSERT INTO nutrition_insights")[0] + assert "score_breakdown" not in sql + for _sql, params in db.calls: + assert '"protein": 85' not in str(params) + + +def test_a_nutrition_row_without_a_brand_is_skipped(client, admin_headers, db): + body = "image_id,product_name,protein_g\nx,Butter,5.0\n" + response = client.post("/api/upload/nutrition", files=_csv(body), headers=admin_headers) + + assert response.status_code == 422 + assert "brand" in response.json()["detail"]["errors"][0]["error"] + assert _find(db, "INSERT INTO nutrition_facts") == [] + + +def test_upserts_never_blank_a_value_an_earlier_source_established(client, admin_headers, db): + """A thinner sheet must not overwrite verified data with NULL.""" + client.post("/api/upload/nutrition", files=_csv(NUTRITION_FULL), headers=admin_headers) + sql, _ = _find(db, "INSERT INTO nutrition_facts")[0] + + for column in ("protein_g", "total_sugar_g", "sodium_mg", "calories_kcal"): + assert f"{column} = COALESCE(EXCLUDED.{column}, nutrition_facts.{column})" in sql + assert "WHEN nutrition_facts.data_status = 'verified' THEN 'verified'" in sql + + +# --------------------------------------------------------------------------- +# Shared behaviour +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("path", ["/api/upload/stores", "/api/upload/analytics", + "/api/upload/nutrition"]) +def test_an_empty_sheet_is_refused(client, admin_headers, db, path): + assert client.post(path, files=_csv("brand,product_name\n"), + headers=admin_headers).status_code == 400 + + +@pytest.mark.parametrize("path", ["/api/upload/stores", "/api/upload/analytics", + "/api/upload/nutrition"]) +def test_uploads_require_a_credential(client, path): + assert client.post(path, files=_csv("a,b\n1,2\n")).status_code == 401 + + +@pytest.mark.parametrize("tab", ["stores", "analytics", "nutrition"]) +def test_the_template_matches_what_the_handler_requires(client, tab): + response = client.get(f"/api/upload/template/{tab}") + assert response.status_code == 200 + header = response.text.splitlines()[0] + assert "brand" in header + # The nutrition template must not teach people to write "None" for + # allergens - that is the exact claim the endpoint must never store. + if tab == "nutrition": + assert ",None" not in response.text diff --git a/tests/test_uploads_api.py b/tests/test_uploads_api.py new file mode 100644 index 0000000..a75b871 --- /dev/null +++ b/tests/test_uploads_api.py @@ -0,0 +1,470 @@ +"""The catalog ingestion API given to outside API clients. + + POST /api/uploads/catalog - send spreadsheets, the pipeline runs + GET /api/uploads/catalog - the batches this caller has sent + GET /api/uploads/catalog/{batch_id} - progress and result of one of them + +Follows test_batch_catalog_ingest.py: point BATCH_UPLOAD_DIR at tmp_path, +monkeypatch the storage/embedding boundary, and stub the background worker by +default. Nothing here loads sentence-transformers or torch, nothing reaches the +network, and nothing writes into the repository's data directory. + +Two groups of tests matter more than the rest: + +* the ones asserting an upload is actually QUEUED. The endpoint previously + accepted files and started nothing, which read as a success to every client + and to every test that only checked the status code. `submitted` is + asserted on directly for that reason. +* the ones asserting what an `uploader` credential CANNOT do. That role + exists so an outside contributor does not get the `user` role's catalog + write access, and an assertion is the only thing that keeps it true as + endpoints are added. +""" +from __future__ import annotations + +import io + +import pytest + +from app.api.batch_job_store import batch_job_store +from app.core import batch_ingest +from app.core import store_catalog_pipeline as pipeline + +openpyxl = pytest.importorskip("openpyxl") + +HEADERS = ["Product Name", "Category", "Brand"] +ROWS = [["Amul Butter 100g", "Butter", "Amul"]] + +UPLOAD_KEY = "k" * 43 +OTHER_KEY = "z" * 43 +UPLOAD = "/api/uploads/catalog" + + +def _csv(rows=ROWS) -> bytes: + return ("\n".join([",".join(HEADERS)] + [",".join(r) for r in rows])).encode() + + +def _sheet(rows=ROWS) -> bytes: + """A real .xlsx, which is the format this endpoint exists to accept.""" + wb = openpyxl.Workbook() + ws = wb.active + ws.append(HEADERS) + for row in rows: + ws.append(row) + buf = io.BytesIO() + wb.save(buf) + return buf.getvalue() + + +def _files(*pairs): + return [("files", (n, io.BytesIO(c), "application/octet-stream")) + for n, c in pairs] + + +@pytest.fixture(autouse=True) +def _isolate_sku_counter(tmp_path, monkeypatch): + from app.services import sku_service + monkeypatch.setattr(sku_service, "_data_dir", tmp_path / "sku_sequences") + + +@pytest.fixture(autouse=True) +def batch_root(tmp_path, monkeypatch): + """BATCH_UPLOAD_DIR under tmp_path. Read at call time by design.""" + root = tmp_path / "batch_uploads" + monkeypatch.setattr(batch_ingest, "BATCH_UPLOAD_DIR", root) + return root + + +@pytest.fixture(autouse=True) +def submitted(monkeypatch): + """Stub the worker and record what was handed to it. + + Same trap as in test_batch_catalog_ingest: a worker still running after + teardown resolves BATCH_UPLOAD_DIR again and writes into the real data/ + directory. Stubbing also turns "was this queued?" into something a test can + assert on rather than infer. + """ + from app.core import batch_worker + + seen: list = [] + monkeypatch.setattr(batch_worker, "submit", seen.append) + return seen + + +@pytest.fixture(autouse=True) +def _clean_job_store(): + """Empty the process-global batch cache around every test. + + batch_job_store outlives a test - it is a module singleton - so without + this a batch staged by one test is still in the cache for the next one, and + the ownership assertions below ("this caller sees only their own") would + pass or fail depending on test order. + """ + batch_job_store._batches.clear() + batch_job_store._cancelled.clear() + yield + batch_job_store._batches.clear() + batch_job_store._cancelled.clear() + + +@pytest.fixture(autouse=True) +def keys(monkeypatch): + """Two uploader keys, so "only my own batches" is testable. + + Patched on `security`, not on `settings`: security.py does + `from ...settings import API_KEYS` at import, binding the dict object, so + rebinding the name in `settings` leaves `principal_for_api_key` still + looking at the original and every request comes back 401. + """ + from app.infrastructure import security + + monkeypatch.setattr(security, "API_KEYS", { + UPLOAD_KEY: ("catalog-drop", "uploader"), + OTHER_KEY: ("other-drop", "uploader"), + }) + + +@pytest.fixture +def upload_headers() -> dict: + return {"X-API-Key": UPLOAD_KEY} + + +@pytest.fixture +def other_headers() -> dict: + return {"X-API-Key": OTHER_KEY} + + +@pytest.fixture +def store(monkeypatch): + table: dict = {} + + def fake_upsert(brand, rows, cleanup=False): + assert cleanup is False + for row in rows: + table[row["image_id"]] = dict(row) + return len(rows) + + monkeypatch.setattr(pipeline, "upsert_brand_products", fake_upsert) + monkeypatch.setattr(pipeline, "get_products_by_brand", lambda b, **kw: list(table.values())) + monkeypatch.setattr(pipeline, "embed_texts", lambda texts: [[0.0] * 384 for _ in texts]) + return table + + +# --------------------------------------------------------------------------- +# The requirement: a file sent here is ingested +# --------------------------------------------------------------------------- +def test_an_uploaded_spreadsheet_is_queued_for_ingestion(client, upload_headers, submitted): + """THE regression test for this endpoint. + + An earlier version parked the file and started nothing, which is + indistinguishable from working if you only assert on the status code. + """ + response = client.post(UPLOAD, files=_files(("catalog.xlsx", _sheet())), + headers=upload_headers) + + assert response.status_code == 202, response.text + body = response.json() + assert body["batch_id"] + assert body["status"] == "queued" + assert body["files_total"] == 1 + # The batch reached the worker. This is the assertion that was missing. + assert submitted == [body["batch_id"]] + + +def test_an_excel_file_reaches_the_catalog(client, upload_headers, store): + """End to end: POST a real .xlsx, run the batch, find the row stored.""" + response = client.post(UPLOAD, files=_files(("catalog.xlsx", _sheet())), + headers=upload_headers) + assert response.status_code == 202, response.text + batch_id = response.json()["batch_id"] + + manifest = batch_ingest.run_batch(batch_id) + + assert manifest.status == "done" + assert manifest.files[0].status == "done" + assert manifest.totals()["rows_total"] == 1 + assert manifest.totals()["products_built"] >= 1 + assert "Amul" in manifest.brands() + assert store, "the pipeline stored nothing" + + +def test_the_batch_records_which_credential_sent_it(client, upload_headers): + response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers) + batch_id = response.json()["batch_id"] + + # The key's NAME, never its secret. + assert response.json()["submitted_by"] == "catalog-drop" + assert batch_ingest.read_manifest(batch_id).submitted_by == "catalog-drop" + assert UPLOAD_KEY not in response.text + + +def test_the_response_says_where_to_poll(client, upload_headers): + response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers) + body = response.json() + assert body["batch_id"] in body["message"] + + +def test_network_stages_are_off_unless_asked_for(client, upload_headers): + """Image search spawns a Playwright subprocess; it is opt-in on this path.""" + response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers) + assert response.json()["fetch_images"] is False + assert response.json()["use_llm"] is False + + +def test_network_stages_can_be_opted_into(client, upload_headers): + response = client.post(UPLOAD + "?fetch_images=true", files=_files(("a.csv", _csv())), + headers=upload_headers) + assert response.json()["fetch_images"] is True + + +# --------------------------------------------------------------------------- +# Who may call it +# --------------------------------------------------------------------------- +def test_upload_endpoint_rejects_anonymous(client): + assert client.post(UPLOAD, files=_files(("a.csv", _csv()))).status_code == 401 + + +def test_admin_can_also_send_files(client, admin_headers, submitted): + """`admin` passes every permission check, so this path stays open to them.""" + response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=admin_headers) + assert response.status_code == 202, response.text + assert submitted == [response.json()["batch_id"]] + + +def test_uploader_key_is_refused_on_every_admin_route(client, upload_headers): + admin_routes = [ + ("post", "/api/admin/catalog-batch/preview"), + ("post", "/api/admin/catalog-batch/ingest"), + ("get", "/api/admin/catalog-batch/batches"), + ("get", "/api/admin/catalog-batch/batches/anything"), + ("post", "/api/admin/catalog-batch/batches/anything/resume"), + ("post", "/api/admin/catalog-batch/batches/anything/cancel"), + ("post", "/api/admin/store-catalog/ingest"), + ("post", "/api/system/init"), + ] + for method, path in admin_routes: + response = getattr(client, method)(path, headers=upload_headers) + assert response.status_code == 403, f"{method.upper()} {path} -> {response.status_code}" + + +def test_uploader_key_has_none_of_the_user_roles_write_access(client, upload_headers): + """The reason this is its own role rather than a reuse of `user`.""" + blocked = [ + ("post", "/api/user/products/add"), + ("post", "/api/user/products/batch-add"), + ("post", "/api/user/products/upload-file"), + ("post", "/api/upload/stores"), + ] + for method, path in blocked: + response = getattr(client, method)(path, headers=upload_headers) + assert response.status_code == 403, f"{method.upper()} {path} -> {response.status_code}" + + +def test_uploader_role_grants_exactly_one_permission(): + """A permission added here is a permission granted to an outside party.""" + from app.infrastructure.security import ROLE_PERMISSIONS + + assert ROLE_PERMISSIONS["uploader"] == ["upload_catalog"] + + +def test_api_keys_accept_the_uploader_role(): + from app.infrastructure.settings import _parse_api_keys + + parsed = _parse_api_keys(f"catalog-drop:uploader:{UPLOAD_KEY}") + assert parsed[UPLOAD_KEY] == ("catalog-drop", "uploader") + + +# --------------------------------------------------------------------------- +# Bad input is refused while the sender is still here to be told +# --------------------------------------------------------------------------- +def test_a_broken_sheet_is_refused_and_nothing_is_queued(client, upload_headers, submitted): + response = client.post(UPLOAD, files=_files(("notes.txt", b"this is not a spreadsheet")), + headers=upload_headers) + assert response.status_code == 400 + assert submitted == [] + + +def test_a_sheet_without_the_required_columns_is_refused(client, upload_headers, submitted): + response = client.post(UPLOAD, files=_files(("wrong.csv", b"colour,size\nred,10\n")), + headers=upload_headers) + assert response.status_code == 400 + assert submitted == [] + + +def test_a_bad_file_among_good_ones_is_reported_not_fatal(client, upload_headers, submitted): + """A partial drop is a partial success: the good files still run.""" + response = client.post( + UPLOAD, + files=_files(("good.xlsx", _sheet()), ("bad.txt", b"nope")), + headers=upload_headers, + ) + assert response.status_code == 202, response.text + body = response.json() + assert submitted == [body["batch_id"]] + + by_name = {f["filename"]: f for f in body["files"]} + assert by_name["good.xlsx"]["status"] == "queued" + assert by_name["bad.txt"]["status"] == "failed" + assert by_name["bad.txt"]["detail"] + assert "bad.txt" not in body["message"] or "1 could not be read" in body["message"] + + +def test_no_files_is_refused(client, upload_headers): + # An empty multipart body fails FastAPI's own validation before the handler. + assert client.post(UPLOAD, headers=upload_headers).status_code == 422 + + +def test_an_empty_file_is_refused(client, upload_headers, submitted): + assert client.post(UPLOAD, files=_files(("empty.csv", b"")), + headers=upload_headers).status_code == 400 + assert submitted == [] + + +def test_too_many_files_in_one_drop_is_refused(client, upload_headers, monkeypatch, submitted): + from app.api.routers import uploads + + monkeypatch.setattr(uploads, "BATCH_MAX_FILES", 2) + response = client.post( + UPLOAD, + files=_files(("a.csv", _csv()), ("b.csv", _csv()), ("c.csv", _csv())), + headers=upload_headers, + ) + assert response.status_code == 413 + assert submitted == [] + + +def test_a_file_over_the_size_limit_is_refused(client, upload_headers, monkeypatch, submitted): + from app.api.routers import uploads + + monkeypatch.setattr(uploads, "MAX_UPLOAD_BYTES", 64) + response = client.post(UPLOAD, files=_files(("big.xlsx", _sheet())), + headers=upload_headers) + assert response.status_code == 413 + assert submitted == [] + + +def test_too_many_rows_across_the_drop_is_refused(client, upload_headers, monkeypatch, submitted): + from app.api.routers import uploads + + monkeypatch.setattr(uploads, "BATCH_MAX_TOTAL_ROWS", 3) + rows = [["Amul Butter %d" % i, "Butter", "Amul"] for i in range(4)] + response = client.post(UPLOAD, files=_files(("many.csv", _csv(rows))), + headers=upload_headers) + assert response.status_code == 413 + assert submitted == [] + + +def test_uploaded_filenames_cannot_escape_the_batch_directory(client, upload_headers, batch_root): + """UploadFile.filename is caller-controlled and is used to build a path.""" + response = client.post(UPLOAD, files=_files(("../../evil.csv", _csv())), + headers=upload_headers) + assert response.status_code == 202, response.text + + directory = batch_root / response.json()["batch_id"] + for entry in batch_ingest.read_manifest(response.json()["batch_id"]).files: + assert ".." not in entry.stored_name + assert (directory / entry.stored_name).resolve().parent == directory.resolve() + assert not (batch_root.parent / "evil.csv").exists() + + +def test_a_full_queue_is_refused_but_the_batch_is_saved(client, upload_headers, monkeypatch): + """The caller has no Resume button, so they are told to retry - but the + files are on disk and an admin can start them.""" + import queue + + from app.core import batch_worker + + def full(_batch_id): + raise queue.Full() + + monkeypatch.setattr(batch_worker, "submit", full) + response = client.post(UPLOAD, files=_files(("a.csv", _csv())), headers=upload_headers) + + assert response.status_code == 429 + detail = response.json()["detail"] + assert "retry" in detail.lower() + # The id is in the message precisely so an admin can find it. + staged = batch_ingest.list_manifests() + assert len(staged) == 1 + assert staged[0].batch_id in detail + assert staged[0].status == "queued" + + +# --------------------------------------------------------------------------- +# Polling, and seeing only your own +# --------------------------------------------------------------------------- +def test_the_sender_can_poll_their_own_batch(client, upload_headers): + batch_id = client.post(UPLOAD, files=_files(("a.csv", _csv())), + headers=upload_headers).json()["batch_id"] + + response = client.get(f"{UPLOAD}/{batch_id}", headers=upload_headers) + assert response.status_code == 200 + assert response.json()["batch_id"] == batch_id + assert response.json()["files"][0]["filename"] == "a.csv" + + +def test_progress_is_visible_after_the_batch_runs(client, upload_headers, store): + batch_id = client.post(UPLOAD, files=_files(("a.csv", _csv())), + headers=upload_headers).json()["batch_id"] + # The worker passes on_change=batch_job_store.put; calling run_batch + # directly must do the same, or the cache in front of the manifest is stale + # and the poll answers with the state as it was at staging time. + batch_ingest.run_batch(batch_id, on_change=batch_job_store.put) + + body = client.get(f"{UPLOAD}/{batch_id}", headers=upload_headers).json() + assert body["status"] == "done" + assert body["files_done"] == 1 + assert body["files"][0]["result"] is not None + + +def test_another_callers_batch_is_not_found(client, upload_headers, other_headers): + """404 rather than 403 - whether an id exists is not their business.""" + batch_id = client.post(UPLOAD, files=_files(("a.csv", _csv())), + headers=upload_headers).json()["batch_id"] + + assert client.get(f"{UPLOAD}/{batch_id}", headers=other_headers).status_code == 404 + + +def test_a_batch_an_admin_uploaded_is_not_visible_to_an_api_client( + client, admin_headers, upload_headers +): + """Admin batches carry no submitted_by, and an empty owner matches nobody.""" + batch_id = client.post("/api/admin/catalog-batch/ingest", + files=_files(("a.csv", _csv())), + headers=admin_headers).json()["batch_id"] + + assert batch_ingest.read_manifest(batch_id).submitted_by is None + assert client.get(f"{UPLOAD}/{batch_id}", headers=upload_headers).status_code == 404 + + +def test_listing_shows_only_this_callers_batches(client, upload_headers, other_headers): + mine = client.post(UPLOAD, files=_files(("mine.csv", _csv())), + headers=upload_headers).json()["batch_id"] + theirs = client.post(UPLOAD, files=_files(("theirs.csv", _csv())), + headers=other_headers).json()["batch_id"] + + listed = client.get(UPLOAD, headers=upload_headers).json()["batches"] + ids = [b["batch_id"] for b in listed] + assert ids == [mine] + assert theirs not in ids + + +def test_an_admin_may_see_any_batch(client, admin_headers, upload_headers): + batch_id = client.post(UPLOAD, files=_files(("a.csv", _csv())), + headers=upload_headers).json()["batch_id"] + + assert client.get(f"{UPLOAD}/{batch_id}", headers=admin_headers).status_code == 200 + + +def test_an_unknown_batch_is_not_found(client, upload_headers): + assert client.get(f"{UPLOAD}/nosuchbatch", headers=upload_headers).status_code == 404 + + +def test_a_traversing_batch_id_is_not_found(client, upload_headers): + """batch_dir() validates the id; the route must surface that as a 404.""" + assert client.get(f"{UPLOAD}/..%2F..%2Fetc", headers=upload_headers).status_code == 404 + + +def test_reads_require_a_credential(client): + assert client.get(UPLOAD).status_code == 401 + assert client.get(f"{UPLOAD}/anything").status_code == 401