Skip to content

[superlog] Skip unsupported filter fields per query type instead of throwing#477

Open
superlog-app[bot] wants to merge 1 commit into
stagingfrom
superlog/skip-unsupported-batch-filters
Open

[superlog] Skip unsupported filter fields per query type instead of throwing#477
superlog-app[bot] wants to merge 1 commit into
stagingfrom
superlog/skip-unsupported-batch-filters

Conversation

@superlog-app

@superlog-app superlog-app Bot commented Jun 14, 2026

Copy link
Copy Markdown

Summary

When a user applies an href filter on the outbound links or outbound domains view, the batch query endpoint sends all 6 dashboard queries — including country, top_pages, browsers, etc. — with that same filter. Standard analytics queries don't declare href in their allowedFilters, so buildFilter throws inside buildWhereClauseFromFilters. This crashes the entire union batch group, logs a noisy ERROR, and leaves those widgets empty.

The outbound_links and outbound_domains builders correctly declare allowedFilters: ["client_id", "anonymous_id", "session_id", "href", "text"], so they succeed individually after the fallback. But other query types (like country) have no allowedFilters and reject href with an error.

The fix changes buildWhereClauseFromFilters to silently skip filters whose field is not permitted by the current query config, rather than throwing. This is the correct semantic: a dimension-specific filter (href, id) that doesn't apply to a given query type should simply be omitted from that query's WHERE clause. The security behavior is unchanged — unknown/injection field names are still never emitted into SQL, they're just silently skipped.

An alternative approach would be to strip unsupported filters in the batch route before calling executeBatch, but that would require per-query filter pre-processing at the API layer and duplicates the allowedFilters knowledge that already lives in the builder config.

Tests updated: the three existing tests that expected compile() to throw on unsupported fields now assert the field is absent from the compiled SQL instead.

Incident on Superlog


Was this PR helpful? Leave feedback — goes straight to the Superlog team.


Summary by cubic

Skip unsupported filter fields per query type instead of throwing, so batch queries no longer fail when a dimension-specific filter (e.g. href) is applied. This prevents empty widgets and noisy errors; unrelated queries now compile and run.

  • Bug Fixes
    • Updated buildWhereClauseFromFilters to ignore filters not in allowedFilters for the current query config while preserving SQL injection safety.
    • Batch views with href filters (outbound links/domains) no longer break other queries like country, top_pages, or browsers.
    • Adjusted tests to assert unsupported or injection-like fields are omitted from SQL instead of causing throws.

Written for commit 59b6f12. Summary will update on new commits.

Review in cubic

@vercel

vercel Bot commented Jun 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
databuddy-status Ready Ready Preview, Comment Jun 14, 2026 12:42am
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
dashboard Skipped Skipped Jun 14, 2026 12:42am
documentation Skipped Skipped Jun 14, 2026 12:42am

@unkey-deploy

unkey-deploy Bot commented Jun 14, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Unkey Deploy

Name Status Preview Inspect Updated (UTC)
api (preview) Ready Visit Preview Inspect Jun 14, 2026 12:42am

@greptile-apps

greptile-apps Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR changes buildWhereClauseFromFilters to silently skip filters whose field is not in the query's allowed set, rather than throwing, so that a batch containing an href filter (valid for outbound_links) no longer crashes unrelated queries like country or top_pages.

  • The overall approach — skip unsupported filters before reaching buildFilter — is correct, and the tests clearly capture the intended semantics.
  • The new guard (lines 1055-1064) calls GLOBAL_ALLOWED_FILTERS.includes(...), but GLOBAL_ALLOWED_FILTERS is a Set. Set has .has(), not .includes(). At runtime this throws TypeError: GLOBAL_ALLOWED_FILTERS.includes is not a function on every request that carries filters, which is a regression worse than the original error. The pre-existing isFilterFieldAllowed(this.config, filter.field) helper (line 61) already performs the same check correctly and should be used here instead.

Confidence Score: 3/5

The fix is conceptually right but the implementation is broken — every query carrying any filter will throw a TypeError instead of running.

The only changed code path — buildWhereClauseFromFilters — now calls .includes() on a Set, which doesn't have that method. This would crash all filtered queries in production, turning a narrow incident (batch group error on unsupported filter fields) into a broader one (all analytics queries with any filter). The test changes are well-structured and would catch this if the tests can actually run to completion, but they will also fail due to the same TypeError.

packages/ai/src/query/simple-builder.ts — the new guard block at lines 1055-1064

Important Files Changed

Filename Overview
packages/ai/src/query/simple-builder.ts Introduces a skip-instead-of-throw guard in buildWhereClauseFromFilters, but calls .includes() on a Set (should be .has()), causing a TypeError on every filtered query.
packages/ai/src/query/simple-builder.test.ts Updates three existing tests to assert absence-from-SQL rather than throws, and adds a focused regression test for the outbound filter in a mixed batch; test logic is sound but tests will fail due to the runtime bug in the implementation.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[buildWhereClauseFromFilters called] --> B{filter.target or filter.having?}
    B -- yes --> C[continue / skip]
    B -- no --> D{isGloballyAllowed?\n⚠️ calls Set.includes — TypeError}
    D -- TypeError --> E[💥 Crashes — regression]
    D -- allowed --> F{config.allowedFilters includes field?}
    F -- no --> C
    F -- yes --> G[buildFilter]
    G -- allowed --> H[append WHERE clause]
    G -- not allowed --> I[throw 'not permitted']
    H --> J[return whereClause]

    style E fill:#ff4444,color:#fff
    style D fill:#ffaaaa
Loading

Reviews (1): Last reviewed commit: "[superlog] Skip unsupported filter field..." | Re-trigger Greptile

Comment on lines +1055 to +1064
const isGloballyAllowed = GLOBAL_ALLOWED_FILTERS.includes(
filter.field as (typeof GLOBAL_ALLOWED_FILTERS)[number]
);
if (
!(
isGloballyAllowed ||
this.config.allowedFilters?.includes(filter.field)
)
) {
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Set.prototype.includes does not exist — TypeError on every request with filters

GLOBAL_ALLOWED_FILTERS is declared as new Set([...]) (line 44). Set has .has(), not .includes(). Calling .includes() on a Set throws TypeError: GLOBAL_ALLOWED_FILTERS.includes is not a function at runtime, which would crash every query that carries filters — a regression worse than the original throw. The pre-existing isFilterFieldAllowed helper (line 61-68) already performs this exact check correctly using .has(). The entire new block should be replaced with a single call to that helper:

if (!isFilterFieldAllowed(this.config, filter.field)) {
    continue;
}

The type assertion as (typeof GLOBAL_ALLOWED_FILTERS)[number] is also unsound for a SetSet<string>[number] resolves to undefined — but it is superseded by the runtime crash anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants