diff --git a/src/qodev_apollo_api/client.py b/src/qodev_apollo_api/client.py index a295a66..934dc2c 100644 --- a/src/qodev_apollo_api/client.py +++ b/src/qodev_apollo_api/client.py @@ -46,10 +46,22 @@ logger = logging.getLogger(__name__) -# Filters accepted by POST /accounts/search (besides page/per_page, which are -# passed explicitly). Apollo *silently drops* unrecognised keys and returns an -# unfiltered default page, so we validate against this allowlist and raise -# instead of returning wrong data. See ``ApolloClient.search_accounts``. +# --------------------------------------------------------------------------- +# Search-filter allowlists +# +# Apollo's /search endpoints *silently drop* unrecognised filter keys and return +# an unfiltered default page that looks like a real match (e.g. a typo'd +# ``query=`` on accounts returned ~28k rows, "Google" first). To stop that, each +# search method validates its ``**filters`` against the relevant allowlist below. +# +# Endpoints with a documented, stable flat-filter vocabulary are validated +# *strictly* (raise on unknown). The activity endpoints below have no published +# filter docs, so an over-tight allowlist would reject valid filters — those are +# validated *leniently* (log a warning, still send the request). See +# ``_validate_search_filters``. +# --------------------------------------------------------------------------- + +# Strict (raise on unknown) — documented flat-filter endpoints. ACCOUNT_SEARCH_FILTERS = frozenset( { "q_organization_name", @@ -59,6 +71,87 @@ "sort_ascending", } ) +CONTACT_SEARCH_FILTERS = frozenset( + { + "q_keywords", + "contact_stage_ids", + "contact_label_ids", + "linkedin_url", + "sort_by_field", + "sort_ascending", + } +) +DEAL_SEARCH_FILTERS = frozenset( + { + "q_keywords", + "opportunity_stage_ids", + "sort_by_field", + "sort_ascending", + } +) +# search_people passes *everything* (incl. page/per_page) through **filters, so +# those are part of the allowlist here (unlike the methods with explicit args). +PEOPLE_SEARCH_FILTERS = frozenset( + { + "q_keywords", + "person_titles", + "include_similar_titles", + "person_seniorities", + "person_locations", + "organization_locations", + "organization_ids", + "organization_num_employees_ranges", + "q_organization_domains_list", + "revenue_range", + "currently_using_all_of_technology_uids", + "currently_using_any_of_technology_uids", + "currently_not_using_any_of_technology_uids", + "q_organization_job_titles", + "organization_job_locations", + "organization_num_jobs_range", + "organization_job_posted_at_range", + "contact_email_status", + "page", + "per_page", + } +) + +# Lenient (warn on unknown) — undocumented activity endpoints. Seeded from known +# usage; incompleteness only costs a log line, never a broken call. +NOTE_SEARCH_FILTERS = frozenset({"contact_ids", "account_ids", "opportunity_ids", "q_keywords"}) +CALL_SEARCH_FILTERS = frozenset({"contact_ids", "account_ids", "user_ids", "q_keywords"}) +TASK_SEARCH_FILTERS = frozenset({"contact_ids", "account_ids", "opportunity_ids", "q_keywords"}) +EMAIL_SEARCH_FILTERS = frozenset({"contact_ids", "emailer_campaign_ids", "q_keywords"}) +CONVERSATION_SEARCH_FILTERS = frozenset({"q_keywords"}) +CALENDAR_EVENT_SEARCH_FILTERS = frozenset({"contact_ids", "user_ids", "q_keywords"}) + + +def _validate_search_filters( + filters: dict, allowed: frozenset[str], resource: str, *, strict: bool +) -> None: + """Guard against Apollo silently dropping unknown ``**filters`` keys. + + Apollo ignores unrecognised keys on its /search endpoints and returns an + unfiltered default page that looks like a real match. When ``strict``, raise + ``ValueError`` on any unknown key (documented endpoints); otherwise log a + warning and let the request through (undocumented endpoints, where the full + valid set isn't published and a hard allowlist would reject valid filters). + """ + unknown = set(filters) - allowed + if not unknown: + return + # Strict allowlists are authoritative ("Supported filters"); lenient ones are + # seeded from known usage and may be incomplete ("Known filters"), so the + # wording doesn't imply the warned-about key is definitely invalid. + label = "Supported filters" if strict else "Known filters" + msg = ( + f"Unknown {resource} search filter(s): {', '.join(sorted(unknown))}. " + f"Apollo silently ignores unrecognised keys and returns an unfiltered " + f"default page. {label}: {', '.join(sorted(allowed))}." + ) + if strict: + raise ValueError(msg) + logger.warning(msg) class ApolloClient: @@ -202,6 +295,7 @@ async def search_contacts( Returns: Paginated response with Contact items """ + _validate_search_filters(filters, CONTACT_SEARCH_FILTERS, "contact", strict=True) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/contacts/search", data) @@ -369,17 +463,7 @@ async def search_accounts( wrong accounts. Note ``query=`` is **not** a valid filter — use ``q_organization_name=`` to search by name. """ - unknown = set(filters) - ACCOUNT_SEARCH_FILTERS - if unknown: - raise ValueError( - "Unknown account search filter(s): " - + ", ".join(sorted(unknown)) - + ". Apollo silently ignores unrecognised keys and returns an unfiltered " - "default page. Supported filters: " - + ", ".join(sorted(ACCOUNT_SEARCH_FILTERS)) - + " (to search by name use q_organization_name=)." - ) - + _validate_search_filters(filters, ACCOUNT_SEARCH_FILTERS, "account", strict=True) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/accounts/search", data) @@ -421,6 +505,7 @@ async def search_deals( Returns: Paginated response with Deal items """ + _validate_search_filters(filters, DEAL_SEARCH_FILTERS, "deal", strict=True) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/opportunities/search", data) @@ -676,6 +761,7 @@ async def search_people(self, **filters) -> dict: Returns: Raw Apollo response dict: ``people`` (list) and ``total_entries`` (int). """ + _validate_search_filters(filters, PEOPLE_SEARCH_FILTERS, "people", strict=True) return await self._post("/mixed_people/api_search", filters) # ======================================================================== @@ -695,6 +781,7 @@ async def search_notes( Returns: Paginated response with Note items (content converted to Markdown) """ + _validate_search_filters(filters, NOTE_SEARCH_FILTERS, "note", strict=False) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/notes/search", data) @@ -780,6 +867,7 @@ async def search_calls( Returns: Paginated response with Call items """ + _validate_search_filters(filters, CALL_SEARCH_FILTERS, "call", strict=False) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/phone_calls/search", data) @@ -819,6 +907,7 @@ async def search_tasks( Returns: Paginated response with specific Task subclass items """ + _validate_search_filters(filters, TASK_SEARCH_FILTERS, "task", strict=False) data: dict[str, Any] = {"page": page, "per_page": min(limit, 100), **filters} if task_type_cds is not None: data["task_type_cds"] = task_type_cds @@ -862,6 +951,7 @@ async def search_emails( Returns: Paginated response with Email items """ + _validate_search_filters(filters, EMAIL_SEARCH_FILTERS, "email", strict=False) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/emailer_messages/search", data) @@ -1208,6 +1298,9 @@ async def search_calendar_events( Returns: Paginated response with CalendarEvent items """ + _validate_search_filters( + filters, CALENDAR_EVENT_SEARCH_FILTERS, "calendar event", strict=False + ) data = {"page": page, "per_page": min(limit, 100), **filters} result = await self._post("/calendar_events/search", data) @@ -1237,6 +1330,7 @@ async def search_conversations( Returns: Paginated response with Conversation items """ + _validate_search_filters(filters, CONVERSATION_SEARCH_FILTERS, "conversation", strict=False) data = {"page": page, "per_page": min(limit, 25), **filters} result = await self._post("/conversations/search", data) diff --git a/tests/test_client.py b/tests/test_client.py index 9e608bf..4e49653 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -290,6 +290,68 @@ async def test_search_accounts_allows_documented_filters(client: ApolloClient): assert body["sort_by_field"] == "account_created_at" +# --- Generalized search-filter validation (strict raise vs lenient warn) ------ + + +@pytest.mark.parametrize( + "method,good_filter", + [ + ("search_contacts", {"q_keywords": "x"}), + ("search_deals", {"opportunity_stage_ids": ["s1"]}), + ("search_people", {"person_titles": ["CEO"]}), + ], +) +async def test_strict_search_methods_reject_unknown_filter(client, method, good_filter): + """contacts/deals/people raise on an unknown key (like accounts) and never call Apollo.""" + client._client.request.return_value = _make_response({}) + with pytest.raises(ValueError, match=r"Unknown .* search filter"): + await getattr(client, method)(query="typo") + client._client.request.assert_not_called() + + # A documented filter passes validation and reaches Apollo. + await getattr(client, method)(**good_filter) + assert client._client.request.called + + +async def test_search_people_allows_page_and_per_page(client: ApolloClient): + """search_people has no explicit page/limit, so page/per_page are valid filters.""" + client._client.request.return_value = _make_response({"people": [], "contacts": []}) + await client.search_people(q_keywords="x", page=2, per_page=50) + assert client._client.request.call_args[1]["json"]["per_page"] == 50 + + +@pytest.mark.parametrize( + "method,endpoint_key", + [ + ("search_notes", "notes"), + ("search_calls", "phone_calls"), + ("search_tasks", "tasks"), + ("search_emails", "emailer_messages"), + ("search_conversations", "conversations"), + ("search_calendar_events", "calendar_events"), + ], +) +async def test_lenient_search_methods_warn_but_still_send(client, method, endpoint_key, caplog): + """Activity endpoints log a warning on an unknown key but still send the request.""" + client._client.request.return_value = _make_response({endpoint_key: [], "pagination": {}}) + + with caplog.at_level("WARNING", logger="qodev_apollo_api.client"): + await getattr(client, method)(bogus_filter="x") + + assert any("Unknown" in r.message and "search filter" in r.message for r in caplog.records) + # Lenient: the request is still sent (unknown key forwarded, not blocked). + assert client._client.request.called + assert client._client.request.call_args[1]["json"]["bogus_filter"] == "x" + + +async def test_lenient_search_method_no_warn_on_known_filter(client, caplog): + """A known activity filter passes without a warning.""" + client._client.request.return_value = _make_response({"notes": [], "pagination": {}}) + with caplog.at_level("WARNING", logger="qodev_apollo_api.client"): + await client.search_notes(contact_ids=["c1"]) + assert not any("Unknown" in r.message for r in caplog.records) + + async def test_search_deals(client: ApolloClient): """Test POST /opportunities/search returns PaginatedResponse[Deal].""" client._client.request.return_value = _make_response(