fix: generalize search-filter validation across all search_* methods - #18
Conversation
Generalizes the search_accounts filter-key guard to every search method. Apollo silently drops unknown filter keys and returns an unfiltered default page that looks like a real match. - Strict (raise on unknown), documented flat-filter endpoints: contacts, deals, people (+ accounts, refactored onto the shared helper). - Lenient (logging.warning, non-breaking), undocumented activity endpoints: notes, calls, tasks, emails, conversations, calendar_events — where the full valid filter set isn't published and a hard allowlist could reject valid filters. Live-verified: valid filters pass; unknown keys raise (strict) or warn (lenient) while still returning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![peqy[bot]](https://avatars.githubusercontent.com/in/1930020?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall this is a solid hardening change: strict validation where the filter vocab is stable prevents Apollo’s “silent drop → unfiltered default page” footgun, and lenient mode for activity endpoints is a good trade-off.
Found 2 issues (0 critical, 1 improvement, 1 question).
| unknown = set(filters) - allowed | ||
| if not unknown: | ||
| return | ||
| msg = ( |
There was a problem hiding this comment.
Improvement: In lenient mode the allowlists are explicitly incomplete, but the warning text says “Supported filters: …”, which reads like a definitive list.
Fix: consider wording like “Known filters” (or omit the list entirely for
strict=False), and maybe keep the full supported-list only for strictValueErrorto avoid very noisy log/error messages (esp.PEOPLE_SEARCH_FILTERS).
There was a problem hiding this comment.
Yep — the lenient-mode wording (“Known filters”) reads correctly now. Thanks for addressing this.
There was a problem hiding this comment.
Confirmed fixed in the latest commit: lenient mode now says “Known filters” (strict still “Supported filters”). This thread can be resolved.
There was a problem hiding this comment.
Still looks good on my side — thanks again for the “Known filters” wording tweak. Can resolve this thread.
| Returns: | ||
| Search results dictionary | ||
| """ | ||
| _validate_search_filters(filters, PEOPLE_SEARCH_FILTERS, "people", strict=True) |
There was a problem hiding this comment.
Question: search_people validates against PEOPLE_SEARCH_FILTERS, but still posts to /mixed_people/search (not the new /mixed_people/api_search mentioned in the description).
Fix: is it worth adding a small TODO / docstring note here that the endpoint is deprecated (or switching in this PR if it’s low-risk)? Mainly to avoid readers assuming the PR already addressed the deprecation.
There was a problem hiding this comment.
Still seeing /mixed_people/search here. If you’re intentionally keeping the endpoint switch for the follow-up MR, I’d consider a small TODO/comment (or link to the follow-up issue) so readers don’t assume this PR fixed the deprecation. Non-blocking for this change set.
There was a problem hiding this comment.
This is now posting to /mixed_people/api_search (instead of /mixed_people/search), matching the deprecation note in the MR description. 👍 This thread can be resolved.
There was a problem hiding this comment.
Re-checking: search_people is now using /mixed_people/api_search, so the deprecation concern is addressed. Can resolve this thread.
peqy: lenient allowlists are intentionally incomplete, so 'Supported filters' over-claimed. Strict mode still says 'Supported filters'; lenient (warn) mode now says 'Known filters' so the message doesn't imply the key is definitely invalid. (The search_people endpoint deprecation peqy asked about is handled by the separate people-api-search PR.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Looks good — the shared filter validation + strict-vs-lenient split is clear and the tests cover the new behavior well. Question: |
…idation # Conflicts: # src/qodev_apollo_api/client.py
|
Looks good — the generalized The two items I previously called out are now addressed:
Ship it. |
1 participant
What
Extends the
search_accountsfilter-key guard (shipped in 0.4.0) to everysearch_*method. Apollo silently drops unrecognised filter keys and returns an unfiltered default page that looks like a real match (the bug that once attached the wrong company to a deal).A shared
_validate_search_filters(filters, allowed, resource, *, strict)helper backs all of them (accounts refactored onto it too).Strict — raise on unknown (documented, stable flat-filter vocab):
search_contacts,search_deals,search_people(+search_accounts).Lenient —
logging.warning, still sends (undocumented activity endpoints, where an over-tight allowlist would reject valid filters):search_notes,search_calls,search_tasks,search_emails,search_conversations,search_calendar_events.Per-endpoint allowlists come from Apollo's public API docs + verified usage.
Why strict-vs-lenient
The docs are incomplete (e.g.
opportunities/searchdocs omitq_keywords, which works) and the 6 activity endpoints have no filter docs at all. Raising everywhere would trade silent-wrong for loud-but-wrong on those. So: raise where the vocab is known, warn where it isn't.Testing
Live testing surfaced that
search_people's endpoint/mixed_people/searchis now deprecated for API callers (422 → use/mixed_people/api_search). That's an unrelated pre-existing bug; filed to fix in a follow-up MR. This PR's people validation layer is correct for the new endpoint's params.No version bump (left for a release PR).
🤖 Generated with Claude Code