fix: generalize search-filter validation across all search_* methods

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)
 

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(