jhoang304 / jhoang304/Whelp

Enforce one cover photo per restaurant in the schema

Open
#59 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
TypeScript
Stars
0
Forks
0
Avg merge
2h 19m
Merged PRs (30d)
30

Description

Raised by @greptile during review of #54, and flagged in that PR's description
as a deliberate follow-up. **Depends on #54 merging** — the helper named below
arrives with it.

## The problem

"At most one `preview=True` row per restaurant" is enforced only in
application code. `app/api/utils.py:clear_other_previews` reads the current
previews, demotes them, and the caller commits alongside the new cover. #54
added `SELECT ... FOR UPDATE` on the restaurant row, which serialises the two
write paths against each other on Postgres, but:

- the rule still has no backing in the schema, so anything that writes
`preview` without going through that helper can break it (a seeder, a shell,
a future route);
- **rows already in production may violate it**, written by the pre-#54 code
that set `preview` from the request body without clearing the old cover.

When two rows do have `preview=True`, `GET /api/restaurants` picks whichever
iterates last (`restaurant_routes.py`, the preview loop), so a restaurant's
card can change cover between requests for no visible reason.

## Suggested fix

A partial unique index — supported by both Postgres and SQLite, so the test
suite is covered too:

```sql
CREATE UNIQUE INDEX uq_restaurant_images_one_preview
ON restaurant_images (restaurant_id)
WHERE preview;
```

The migration has to clean up before it can add the index, or it will fail on
any restaurant that already has duplicates:

1. For each `restaurant_id` with more than one `preview=True` row, keep one
(lowest `id` is the stable choice, and matches the promotion order #54 uses
when a cover is deleted) and set the rest to `false`.
2. Then create the index.
3. `downgrade()` drops the index; the demoted rows stay demoted, which is
correct — they were never meant to be covers.

Alembic head is currently `b4d7f1a6e93c` (`zipcode_as_text`). Production uses
a schema prefix (`add_prefix_for_prod`), so the migration needs the same
treatment the existing ones use.

## Why it is worth doing

With the index in place a racing or buggy write fails loudly with an
`IntegrityError` instead of silently leaving two covers, and the listing's
choice of cover becomes deterministic by construction rather than by
convention.

Contributor guide

No contributing guide indexed for this repository

Research direction

After #54 merges, read app/api/utils.py:clear_other_previews and the preview loop in restaurant_routes.py, then inspect the Alembic migrations after head b4d7f1a6e93c and their add_prefix_for_prod handling. The migration is done when duplicate previews are cleaned deterministically, the partial unique index is added and dropped by downgrade(), and production schema prefixes are supported.

Written by the indexing model from the issue text.

Assessment

Tech stack
postgresql, sql, sqlite
Domain
backend, databases
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.