Feat/acronym search 03 bulk upload endpoint - #6
Open
borkarsaish65 wants to merge 7 commits into
Open
Conversation
Adds Alembic with a no-op baseline revision for the pre-existing unmanaged translations table, plus a migration that creates acronym_mapping (id PK, expansions as JSONB array, description, composite index on acronym+is_active) and seeds it from data/acronyms.csv, now the real 599-acronym dataset.
get_expansion() checks Redis first, falls back to Postgres on miss, writes through to cache; warm_cache() pre-populates every active acronym at startup. Redis pieces (cache_client.py, redis_client.py) stay acronym-agnostic; only acronym_service.py knows about acronyms, JSON-encoding the expansions array for the string-only cache layer.
get_expansion() only handled a cache miss (Redis says "not found"), not a cache error (Redis unreachable/timing out) — the latter raised straight out of the function before Postgres was ever tried, so a Redis outage broke every acronym-detected search instead of just degrading it. Now both the cache read and the write-through are guarded: any Redis error logs a warning and falls back to Postgres, matching the fallback behavior main.py's startup path already assumes exists.
POST /api/acronyms/bulk, gated behind a shared-secret X-Internal-Token
header (INTERNAL_API_TOKEN env var, no default). Upserts a batch of
{acronym, expansion, is_active} rows in a single transaction — a row
that's blank after trimming, or an in-batch duplicate acronym (Postgres
can't affect the same row twice in one ON CONFLICT statement), is
collected as a per-row error instead of failing the whole batch, with
last-value-wins for duplicates. Invalidates the Redis cache for every
acronym actually upserted so the next lookup re-fetches the new value.
Third slice of the acronym-search feature (see ACRONYM_SEARCH_PLAN.md).
# Conflicts:
# app/services/acronym_service.py
Was blank, which read as easy to miss when setting up a new environment. Placeholder, not a real token — each env still needs its own generated value per the comment above it.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Three issues found in review:
- invalidate_cache() (the bulk-upload endpoint's fallback when the
post-commit cache refresh fails) had no error handling of its own,
so the same Redis outage that triggered the fallback made the
fallback raise too — turning a fully successful, already-committed
upload into an opaque 500 for the caller. Now swallows and logs.
- Bulk upload decoded the file as strict utf-8, which leaves a
spreadsheet-exported BOM attached to the first header name
("acronym"), silently failing every row's acronym validation.
Switched to utf-8-sig, which strips a BOM if present and is
otherwise identical to utf-8.
- The shared Redis connection never forwarded REDIS_PASSWORD, so any
environment with a password-protected Redis would have every
acronym cache operation fail with AuthenticationError. Also
normalizes a blank password to None rather than "", since redis-py
treats an empty string as a real credential to AUTH with.
Adds tests/test_acronym_bulk_upload.py (BOM handling, cache-outage
500 regression) and commits tests/test_acronym_service.py, which was
still sitting uncommitted from PR2's review.
…loop Two more issues found in review: - bulk_upsert() only checked non-empty acronym/expansions before the single multi-row INSERT. An acronym longer than the acronym_mapping column's 32-char limit reached Postgres, raised StringDataRightTruncation, and rolled back the WHOLE batch — including every otherwise-valid row — contradicting the endpoint's own promise that one bad row doesn't fail the batch. Now validated per-row (length read off the model, not hardcoded) before the insert, alongside the existing non-empty check. - bulk_upload_acronyms is async def, but bulk_upsert() and warm_cache() are both fully synchronous, blocking I/O (sync SQLAlchemy Session, sync Redis client) with no actual awaits — warm_cache() alone does up to ~600 sequential blocking Redis writes. Called directly, this stalls the single-threaded event loop for its full duration, so every other in-flight request (search, health checks) queues up behind one acronym upload. warm_cache() converted from async def to a plain (honestly synchronous) function; both it and bulk_upsert() are now run via starlette.concurrency.run_in_threadpool from the endpoint and from app/main.py's startup lifespan. Verified live: 3 concurrent health checks each returned in ~20-30ms while a real upload (with its full cache refresh) was in flight, with no stacking delay. Tests added for both; two existing startup-lifespan tests updated from AsyncMock to Mock since warm_cache is no longer a coroutine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.