CRUD endpoints for Location, Category, Item, Batch, and barcode lookup

[podmar]

CRUD endpoints for Location, Category, Item, Batch, and barcode lookup

All five routers that make up the core v1 API surface are wired in and passing lint/type checks.

Decisions worth noting

• 404 for wrong-household IDs, never 403. Every ownership check uses obj.household_id != user.household_id and returns 404 — returning 403 would confirm the ID exists.

• Broad IntegrityError catch instead of pre-check queries. A pre-check SELECT is a race condition and an extra round-trip. The catch is safe for create/update on locations and categories because household_id is server-set and name is validated by Pydantic before commit — the only possible violation is the unique constraint on (household_id, name).

• POST /items returns 200 for a duplicate barcode instead of 409. The scan workflow reads a barcode and immediately adds stock — if the item already exists, the client needs it back to create a batch, not an error to handle.

• Batch create and update merge quantities on collision instead of rejecting. If the same (item, location, expiry) already exists, adding the quantity to the existing batch matches user intent ("add stock here") better than a 409 the client has to recover from. Both endpoints return the surviving batch so the client has the correct state without a separate fetch.

• POST /batches/{id}/adjust deletes the batch at quantity zero. Storing a zero-quantity batch would pollute expiry queries; deletion is the correct semantic for "stock exhausted".

• Location delete merges batches on conflict. Moving batches to a target that already holds the same (item, expiry) would violate the unique constraint — quantities are summed instead.

• Default location heuristic for batch create. If the client omits location_id, the server uses the household's most-recently-used location (last created_at), falling back to the first seeded location. This keeps the fast-scan flow one field shorter.

• batches.py router has no URL prefix. Batch routes span two patterns (/items/{id}/batches and /batches/{id}), so a shared prefix doesn't apply.

• Barcode lookup proxied through the backend. Direct browser calls to Open Food Facts are blocked by CORS; proxying also lets us gate the endpoint behind auth.

• class_=AsyncSession added to async_sessionmaker. SQLAlchemy's default AsyncSession doesn't expose .exec() — SQLModel's subclass does. Without class_=, every session would silently fall back to the SQLAlchemy version and Pyright would flag .exec() as unknown.

Note

The /til skill and docs/til.md are included here but belong in a separate PR — they were developed alongside this work and weren't split out in time.

What's next

Integration tests covering household isolation and the edge cases above (batch adjust to zero, batch merge on collision, location delete with merge).

[claude]

Comprehensive CRUD implementation with good patterns overall. Found one critical data isolation bug and several missing IntegrityError handlers that need fixing before merge.

Key findings

Critical (must fix):

• Missing household_id filters in location delete/move flow — could cause cross-household data corruption
• Missing IntegrityError handling for batch unique constraint (POST and PATCH endpoints)

Minor (worth addressing):

• Several queries not using denormalized household_id filter (defense-in-depth)
• Barcode input validation could be stronger
• Error messages could be more specific for debugging

The codebase follows good patterns: proper household isolation in most places, 404 for wrong-household, clear comments explaining IntegrityError catches, PATCH with exclude_unset. The location delete flow is particularly well-designed — the merge logic is clever. Just needs the missing household_id filters added to be safe.

[claude]

CRITICAL: Missing household_id filter in batch queries

These queries are missing household isolation filters, which could cause cross-household data corruption:

# Line 140 - missing household_id filter
batches = (await session.exec(select(Batch).where(Batch.location_id == location_id))).all()

# Line 144 - missing household_id filter  
existing = (await session.exec(
    select(Batch).where(
        Batch.item_id == batch.item_id,
        Batch.location_id == move_to,
        Batch.expiry_date == batch.expiry_date,
    )
)).first()

Why this is critical: Item IDs are not globally unique — two households can both have item_id=5 referring to different products. Without filtering by household_id, the merge logic could add Household A's batch quantity to Household B's batch.

Fix:

batches = (
    await session.exec(
        select(Batch).where(
            Batch.location_id == location_id,
            Batch.household_id == user.household_id,  # Add this
        )
    )
).all()

# And in the existing batch lookup:
existing = (
    await session.exec(
        select(Batch).where(
            Batch.item_id == batch.item_id,
            Batch.location_id == move_to,
            Batch.expiry_date == batch.expiry_date,
            Batch.household_id == user.household_id,  # Add this
        )
    )
).first()

[podmar]

not a critical concern: by the time we reach line 140, location_id has
already been verified to belong to user.household_id (the ownership check at
lines 97–99). Since the FK Batch.location_id → Location.id is enforced by the
DB, every batch with Batch.location_id == location_id is, by definition, in
this household.

Applied regardless for clarity of intent, future bugs and following pattern of the db.

[claude]

Missing IntegrityError handling for unique constraint

The Batch model has a unique constraint on (item_id, location_id, expiry_date). If a client updates a batch's location or expiry_date to match an existing batch, the commit will fail with an unhandled IntegrityError, resulting in a 500 error instead of a clean 409.

Why this matters: Clients need clear feedback when an update would create a duplicate. A 500 error suggests a server bug, not a constraint violation.

Fix: Wrap the commit in a try/except like you did in categories.py and locations.py:

session.add(batch)
try:
    await session.commit()
except IntegrityError:
    await session.rollback()
    raise HTTPException(
        status_code=409, 
        detail="A batch with this item, location, and expiry date already exists"
    ) from None
await session.refresh(batch)

Alternatively, you could merge batches (add quantities) like the location delete flow does, but a 409 is simpler and clearer for a direct PATCH.

[podmar]

good catch, batch merge implemented.

[claude]

Minor: Missing defensive household_id filter in cascade delete

While this is safe given the data model invariants (batch.household_id is denormalized from the item, so all batches with this item_id must belong to the same household), adding a household_id filter here would be more defensive:

batches = (
    await session.exec(
        select(Batch).where(
            Batch.item_id == item_id,
            Batch.household_id == user.household_id,  # Add for defense-in-depth
        )
    )
).all()

This protects against edge cases if the denormalization invariant is ever violated (e.g., by direct DB manipulation). Belt-and-braces approach matches the pattern in list_items line 66 where the subquery filters by household_id even though the outer query also does.

[podmar]

decided to skip

[claude]

Missing IntegrityError handling for unique constraint

Similar to the PATCH endpoint, this POST could violate the (item_id, location_id, expiry_date) unique constraint if a batch with these values already exists. Currently would return 500; should return 409.

Fix:

session.add(batch)
try:
    await session.commit()
except IntegrityError:
    await session.rollback()
    raise HTTPException(
        status_code=409,
        detail="A batch with this item, location, and expiry date already exists"
    ) from None
await session.refresh(batch)

Design question: Should this merge quantities instead (upsert pattern)? That would match the location-move behavior. Current behavior requires the client to check first and call adjust if a batch exists, or handle 409 and retry with adjust.

[podmar]

good catch, merge introduced

[claude]

Not using denormalized household_id filter

The batch query only filters by item_id, but Batch has a denormalized household_id column specifically to enable direct filtering. While this is safe (the item verification above guarantees all batches with this item_id belong to the same household), using the denormalized field is more defensive and matches the stated purpose of denormalization.

Suggested:

result = await session.exec(
    select(Batch).where(
        Batch.item_id == item_id,
        Batch.household_id == user.household_id,
    ).order_by(col(Batch.expiry_date))
)

Same pattern applies in other batch queries where the item is pre-verified (lines 82, 166 in this file).

[podmar]

similar situation as in the items.py. Decided to skip for cases where parent is verified and apply if verification happens further in the chain.

[claude]

Minor: Consider barcode format validation

The barcode parameter is interpolated directly into the external URL without validation. While FastAPI path parameters are URL-encoded and Open Food Facts likely handles malformed input gracefully, adding basic validation would be more defensive:

@router.get("/lookup/barcode/{barcode}")
async def lookup_barcode(
    barcode: str = Field(pattern=r'^[0-9A-Za-z-]+$', max_length=50),  # typical barcode formats
    _user: Annotated[User, Depends(current_active_user)],
) -> BarcodeResult:

This prevents any potential URL injection and catches invalid barcodes before making an external request. EAN-13, UPC-A, and similar formats are alphanumeric with possible hyphens.

[podmar]

FastAPI path parameters are
URL-encoded by the framework before they reach the handler — there's no
injection risk into the Open Food Facts URL since we're using an f-string path
segment, not query string concatenation, and httpx handles the URL safely. The
suggested pattern (^[0-9A-Za-z-]+$) would also incorrectly reject
valid barcodes — EAN-13 is digits only, but some internal/retail codes use
spaces or other characters.

The only real value would be catching obviously malformed input early, but Open
Food Facts returns a clean 404 for unknown barcodes anyway, which we already
map to a 404 response.

[claude]

Missing household_id filter (defense-in-depth)

Similar to other endpoints, this query should filter by household_id even though the category was already verified to belong to the household:

in_use = (
    await session.exec(
        select(Item.id).where(
            Item.category_id == category_id,
            Item.household_id == user.household_id,  # Add for consistency
        ).limit(1)
    )
).first()

This protects against edge cases where the FK relationship might be violated (e.g., by direct DB manipulation) and maintains consistency with the "always filter by household_id" principle.

[podmar]

as above, parent verified.

[claude]

Both error paths return the same message

Both httpx.HTTPError (network/timeout error) and non-200 status (server response) return "Product lookup service unavailable". This makes debugging harder — was it a network timeout or did Open Food Facts return 503?

Consider:

try:
    async with httpx.AsyncClient(timeout=5.0) as client:
        response = await client.get(url)
except httpx.HTTPError as exc:
    raise HTTPException(
        status_code=502, detail="Failed to reach product lookup service"
    ) from exc

if response.status_code != 200:
    raise HTTPException(
        status_code=502, 
        detail=f"Product lookup service returned {response.status_code}"
    )

Or keep the same message if you prefer not exposing error details — just noting the current behavior loses debugging signal.

[claude]

PR Review Summary

Critical (must fix before merge):

• Data isolation bug in locations.py:140,144 — batch queries missing household_id filter in the move/merge flow. Could corrupt data across households.
• Missing IntegrityError handling — POST /items/{id}/batches and PATCH /batches/{id} don't catch violations of the (item_id, location_id, expiry_date) unique constraint. Would return 500 instead of clean 409.

Minor (worth addressing):

• Several batch queries don't use the denormalized household_id column for filtering (defense-in-depth opportunity)
• Barcode lookup could validate input format before making external request
• Error messages in lookup endpoint could be more specific for debugging

Cost: $0.25