Skip a malformed app record instead of 500ing the app search page#8933
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Confidence score: 3/5
- In
backend/routers/apps.py, directapp['id']/app_dict['id']access in both the pre-loop comprehension and the loop can still raise on malformed records, so the search page may return a 500 despite the newValidationErrorhandling; broaden the guard to cover allidreads (or use safe access/skip invalid records) and add a regression test for missing-identries before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Git-on-my-level
left a comment
Git-on-my-level
left a comment
There was a problem hiding this comment.
Nice robustness fix — thanks @ZachL111. One malformed Firestore app document 500'ing the entire /v2/apps/search page for users is a real bug, and the guard matches the existing per-record ValidationError pattern already used by the marketplace listing endpoints (_safe_build_app) and elsewhere in this router. Logging the id + offending field locs (never values) is the right call.
Test looks good — covers the skip-and-continue path with a valid + malformed record pair, and follows the established unit-test conventions (os.environ.setdefault, monkeypatch the db helpers, call the handler directly).
Approving. One small optional nit for later: the marketplace listing loops in this file already route through _safe_build_app in utils/apps.py; if search_apps ever wants to centralize too, it could reuse that helper instead of an inline try/except. Not blocking.
GET /v2/apps/search (search_apps) builds App(**app_dict) in a loop over raw app documents with no per-record guard, so one legacy or malformed app document raised ValidationError and 500'd the entire search result. Skip and log the bad record instead, matching the marketplace-listing guard and the other per-record guards in this router. Adds a unit test that a malformed record is dropped while the valid app still comes through.
cubic flagged that the ValidationError guard was too narrow: a record missing id would KeyError in the app_ids list and the installs/reviews/enabled lookups before the guard ever runs, still 500ing the search page. Drop id-less records up front (logging the count) so the page survives.
ZachL111
force-pushed
the
zach/apps-search-poison-guard
branch
from
ee24589 to
d488623
Compare
kodjima33
left a comment
kodjima33
left a comment
There was a problem hiding this comment.
Backend bug fix with test + green CI: per-record ValidationError guard prevents one malformed record from 500ing the endpoint (conf 5/5).
3 participants
What
GET /v2/apps/search(search_apps) buildsApp(**app_dict)in a loop over raw Firestore app documents with no per-record guard.Apphas required no-default fields (id,name,category,author,description,image,capabilities), so one legacy or malformed app document raisedpydantic.ValidationErrorand 500'd the entire search result for the user.Change
Wrap the per-record build in
try/except ValidationError: skip the malformed record and log its id plus the offending field names (never the field values). This matches the recently merged marketplace-listing guard and the other per-recordValidationErrorguards already in this router. Single-file change inbackend/routers/apps.py.Test
New
backend/tests/unit/test_apps_search_poison_guard.py: a search returning one valid app and one malformed record (missing required fields) returns the valid app and skips the bad one instead of 500ing. Imports the router normally, patches the db helpers withmonkeypatch.setattr, and calls the handler directly.