From b3c4d4181fa50818bcfbf08a5aed04223fadcff5 Mon Sep 17 00:00:00 2001 From: Robbie Date: Fri, 20 Oct 2023 15:00:30 +0100 Subject: [PATCH] feat(insights): Add dashboard filters to hogql insights (#18076) * WIP * Kinda sorta working * Add dashboard filters to cache key * Make mypy happy * Add support for properties * Use get_query_runner * Add tests for dashboard_query * Add tests for merging of filters * Add test for hash when dashboard filters are applied * Silently accept having no valid query runner * Remove index type from DashboardFilter * Fix type of dashboard_query * Fix test typing --- frontend/src/queries/schema.json | 25 ++++ frontend/src/queries/schema.ts | 6 + .../scenes/dashboard/dashboardLogic.test.ts | 4 +- frontend/src/types.ts | 12 +- posthog/api/insight.py | 1 + .../hogql_queries/apply_dashboard_filters.py | 22 ++++ posthog/hogql_queries/hogql_query_runner.py | 15 ++- posthog/hogql_queries/query_runner.py | 4 + posthog/models/insight.py | 22 +++- posthog/models/test/test_insight_model.py | 115 ++++++++++++++++++ posthog/schema.py | 24 ++++ 11 files changed, 240 insertions(+), 10 deletions(-) create mode 100644 posthog/hogql_queries/apply_dashboard_filters.py diff --git a/frontend/src/queries/schema.json b/frontend/src/queries/schema.json index ff07a99907292..2dd7b987625e0 100644 --- a/frontend/src/queries/schema.json +++ b/frontend/src/queries/schema.json @@ -323,6 +323,31 @@ ], "type": "string" }, + "DashboardFilter": { + "additionalProperties": false, + "properties": { + "date_from": { + "type": ["string", "null"] + }, + "date_to": { + "type": ["string", "null"] + }, + "properties": { + "anyOf": [ + { + "items": { + "$ref": "#/definitions/AnyPropertyFilter" + }, + "type": "array" + }, + { + "type": "null" + } + ] + } + }, + "type": "object" + }, "DataNode": { "additionalProperties": false, "properties": { diff --git a/frontend/src/queries/schema.ts b/frontend/src/queries/schema.ts index f4b3a91b7b282..5a9dbe9fcd98e 100644 --- a/frontend/src/queries/schema.ts +++ b/frontend/src/queries/schema.ts @@ -699,3 +699,9 @@ export interface BreakdownFilter { breakdown_group_type_index?: number | null breakdown_histogram_bin_count?: number // trends breakdown histogram bin count } + +export interface DashboardFilter { + date_from?: string | null + date_to?: string | null + properties?: AnyPropertyFilter[] | null +} diff --git a/frontend/src/scenes/dashboard/dashboardLogic.test.ts b/frontend/src/scenes/dashboard/dashboardLogic.test.ts index fb61307850f3d..eb04138096c8f 100644 --- a/frontend/src/scenes/dashboard/dashboardLogic.test.ts +++ b/frontend/src/scenes/dashboard/dashboardLogic.test.ts @@ -10,7 +10,6 @@ import { eventUsageLogic } from 'lib/utils/eventUsageLogic' import { DashboardTile, DashboardType, - FilterType, InsightColor, InsightModel, InsightShortId, @@ -24,6 +23,7 @@ import { dayjs, now } from 'lib/dayjs' import { teamLogic } from 'scenes/teamLogic' import { MOCK_TEAM_ID } from 'lib/api.mock' import api from 'lib/api' +import { DashboardFilter } from '~/queries/schema' const dashboardJson = _dashboardJson as any as DashboardType @@ -63,7 +63,7 @@ export const tileFromInsight = (insight: InsightModel, id: number = tileId++): D export const dashboardResult = ( dashboardId: number, tiles: DashboardTile[], - filters: Partial> = {} + filters: Partial = {} ): DashboardType => { return { ...dashboardJson, diff --git a/frontend/src/types.ts b/frontend/src/types.ts index dfd0636122d0d..39444841413ef 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -26,7 +26,13 @@ import { BehavioralFilterKey, BehavioralFilterType } from 'scenes/cohorts/Cohort import { LogicWrapper } from 'kea' import { AggregationAxisFormat } from 'scenes/insights/aggregationAxisFormat' import { Layout } from 'react-grid-layout' -import { DatabaseSchemaQueryResponseField, HogQLQuery, InsightVizNode, Node } from './queries/schema' +import type { + DashboardFilter, + DatabaseSchemaQueryResponseField, + HogQLQuery, + InsightVizNode, + Node, +} from './queries/schema' import { QueryContext } from '~/queries/types' import { JSONContent } from 'scenes/notebooks/Notebook/utils' @@ -1345,7 +1351,7 @@ export type DashboardTemplateScope = 'team' | 'global' | 'feature_flag' export interface DashboardType extends DashboardBasicType { tiles: DashboardTile[] - filters: Record + filters: DashboardFilter } export interface DashboardTemplateType { @@ -1354,7 +1360,7 @@ export interface DashboardTemplateType { created_at?: string template_name: string dashboard_description?: string - dashboard_filters?: Record + dashboard_filters?: DashboardFilter tiles: DashboardTile[] variables?: DashboardTemplateVariableType[] tags?: string[] diff --git a/posthog/api/insight.py b/posthog/api/insight.py index 7eb4010cdbc0f..9412e744b4e89 100644 --- a/posthog/api/insight.py +++ b/posthog/api/insight.py @@ -489,6 +489,7 @@ def to_representation(self, instance: Insight): dashboard: Optional[Dashboard] = self.context.get("dashboard") representation["filters"] = instance.dashboard_filters(dashboard=dashboard) + representation["query"] = instance.dashboard_query(dashboard=dashboard) if "insight" not in representation["filters"] and not representation["query"]: representation["filters"]["insight"] = "TRENDS" diff --git a/posthog/hogql_queries/apply_dashboard_filters.py b/posthog/hogql_queries/apply_dashboard_filters.py new file mode 100644 index 0000000000000..709ef4d7835ca --- /dev/null +++ b/posthog/hogql_queries/apply_dashboard_filters.py @@ -0,0 +1,22 @@ +from posthog.hogql_queries.query_runner import get_query_runner +from posthog.models import Team +from posthog.schema import DashboardFilter + + +# Apply the filters from the django-style Dashboard object +def apply_dashboard_filters(query: dict, filters: dict, team: Team) -> dict: + kind = query.get("kind", None) + + if kind == "DataTableNode": + source = apply_dashboard_filters(query["source"], filters, team) + return {**query, "source": source} + + try: + query_runner = get_query_runner(query, team) + except ValueError: + return query + try: + return query_runner.apply_dashboard_filters(DashboardFilter(**filters)).dict() + except NotImplementedError: + # TODO when we implement apply_dashboard_filters on more query runners, we can remove the try/catch + return query diff --git a/posthog/hogql_queries/hogql_query_runner.py b/posthog/hogql_queries/hogql_query_runner.py index f8dfa43bdd428..815822ce894c6 100644 --- a/posthog/hogql_queries/hogql_query_runner.py +++ b/posthog/hogql_queries/hogql_query_runner.py @@ -10,7 +10,7 @@ from posthog.hogql.timings import HogQLTimings from posthog.hogql_queries.query_runner import QueryRunner from posthog.models import Team -from posthog.schema import HogQLQuery, HogQLQueryResponse +from posthog.schema import HogQLQuery, HogQLQueryResponse, DashboardFilter, HogQLFilters, DateRange class HogQLQueryRunner(QueryRunner): @@ -66,3 +66,16 @@ def _is_stale(self, cached_result_package): def _refresh_frequency(self): return timedelta(minutes=1) + + def apply_dashboard_filters(self, dashboard_filter: DashboardFilter) -> HogQLQuery: + self.query.filters = self.query.filters or HogQLFilters() + self.query.filters.dateRange = self.query.filters.dateRange or DateRange() + + if dashboard_filter.date_to or dashboard_filter.date_from: + self.query.filters.dateRange.date_to = dashboard_filter.date_to + self.query.filters.dateRange.date_from = dashboard_filter.date_from + + if dashboard_filter.properties: + self.query.filters.properties = (self.query.filters.properties or []) + dashboard_filter.properties + + return self.query diff --git a/posthog/hogql_queries/query_runner.py b/posthog/hogql_queries/query_runner.py index d39ed7ed91454..afaa7534c655b 100644 --- a/posthog/hogql_queries/query_runner.py +++ b/posthog/hogql_queries/query_runner.py @@ -25,6 +25,7 @@ EventsQuery, WebStatsTableQuery, HogQLQuery, + DashboardFilter, ) from posthog.utils import generate_cache_key, get_safe_cache @@ -240,3 +241,6 @@ def _is_stale(self, cached_result_package): @abstractmethod def _refresh_frequency(self): raise NotImplementedError() + + def apply_dashboard_filters(self, dashboard_filter: DashboardFilter) -> RunnableQueryNode: + raise NotImplementedError() diff --git a/posthog/models/insight.py b/posthog/models/insight.py index 044ff82f5d514..1c5d168ed7b5b 100644 --- a/posthog/models/insight.py +++ b/posthog/models/insight.py @@ -105,7 +105,7 @@ class Meta: unique_together = ("team", "short_id") def dashboard_filters(self, dashboard: Optional[Dashboard] = None): - # TODO dashboard filtering needs to know how to override query date ranges😱 + # query date range is set in a different function, see dashboard_query if dashboard and not self.query: dashboard_filters = {**dashboard.filters} dashboard_properties = dashboard_filters.pop("properties") if dashboard_filters.get("properties") else None @@ -154,6 +154,13 @@ def dashboard_filters(self, dashboard: Optional[Dashboard] = None): else: return self.filters + def dashboard_query(self, dashboard: Optional[Dashboard]) -> Optional[dict]: + if not dashboard or not self.query: + return self.query + from posthog.hogql_queries.apply_dashboard_filters import apply_dashboard_filters + + return apply_dashboard_filters(self.query, dashboard.filters, self.team) + @property def url(self): return absolute_uri(f"/insights/{self.short_id}") @@ -174,12 +181,19 @@ class Meta: def generate_insight_cache_key(insight: Insight, dashboard: Optional[Dashboard]) -> str: try: if insight.query is not None: - # TODO: dashboard filtering needs to know how to override queries and date ranges 😱 - q = insight.query + dashboard_filters = dashboard.filters if dashboard else None + + if dashboard_filters: + from posthog.hogql_queries.apply_dashboard_filters import apply_dashboard_filters + + q = apply_dashboard_filters(insight.query, dashboard_filters, insight.team) + else: + q = insight.query + if q.get("source"): q = q["source"] - return generate_cache_key("{}_{}".format(q, insight.team_id)) + return generate_cache_key("{}_{}_{}".format(q, dashboard_filters, insight.team_id)) dashboard_insight_filter = get_filter(data=insight.dashboard_filters(dashboard=dashboard), team=insight.team) candidate_filters_hash = generate_cache_key("{}_{}".format(dashboard_insight_filter.toJSON(), insight.team_id)) diff --git a/posthog/models/test/test_insight_model.py b/posthog/models/test/test_insight_model.py index 20a5a77685f14..08d82d0a416ac 100644 --- a/posthog/models/test/test_insight_model.py +++ b/posthog/models/test/test_insight_model.py @@ -118,3 +118,118 @@ def test_query_hash_varies_with_query_content(self) -> None: filters_hash_two = generate_insight_cache_key(insight_two, None) assert filters_hash_one != filters_hash_two + + def test_dashboard_with_query_insight_and_filters(self) -> None: + browser_equals_firefox = { + "key": "$browser", + "label": None, + "operator": "exact", + "type": "event", + "value": ["Firefox"], + } + browser_equals_chrome = { + "key": "$browser", + "label": None, + "operator": "exact", + "type": "event", + "value": ["Chrome"], + } + + # use test cases and a for loop because django TestCase doesn't have parametrization + test_cases = [ + ( + # test that query filters are equal when there are no dashboard filters + {"dateRange": {"date_from": "-14d", "date_to": "-7d"}}, + {}, + {"dateRange": {"date_from": "-14d", "date_to": "-7d"}, "properties": None}, + ), + ( + # test that dashboard filters are used when there are no query filters + {}, + {"date_from": "-14d", "date_to": "-7d"}, + {"dateRange": {"date_from": "-14d", "date_to": "-7d"}, "properties": None}, + ), + ( + # test that dashboard filters take priority + {"dateRange": {"date_from": "-2d", "date_to": "-1d"}}, + {"date_from": "-4d", "date_to": "-3d"}, + {"dateRange": {"date_from": "-4d", "date_to": "-3d"}, "properties": None}, + ), + ( + # test that dashboard filters take priority, even if only one value is set, the other is set to None + {"dateRange": {"date_from": "-14d", "date_to": "-7d"}}, + {"date_from": "all"}, + {"dateRange": {"date_from": "all", "date_to": None}, "properties": None}, + ), + ( + # test that if no filters are set then none are outputted + {}, + {}, + {"dateRange": {"date_from": None, "date_to": None}, "properties": None}, + ), + ( + # test that properties from the query are used when there are no dashboard properties + {"properties": [browser_equals_firefox]}, + {}, + {"dateRange": {"date_from": None, "date_to": None}, "properties": [browser_equals_firefox]}, + ), + ( + # test that properties from the dashboard are used when there are no query properties + {}, + {"properties": [browser_equals_chrome]}, + {"dateRange": {"date_from": None, "date_to": None}, "properties": [browser_equals_chrome]}, + ), + ( + # test that properties are merged when set in both query and dashboard + {"properties": [browser_equals_firefox]}, + {"properties": [browser_equals_chrome]}, + { + "dateRange": {"date_from": None, "date_to": None}, + "properties": [browser_equals_firefox, browser_equals_chrome], + }, + ), + ] + + for query_filters, dashboard_filters, expected_filters in test_cases: + query_insight = Insight.objects.create( + team=self.team, + query={ + "kind": "DataTableNode", + "source": { + "filters": query_filters, + "kind": "HogQLQuery", + "modifiers": None, + "query": "select * from events where {filters}", + "response": None, + "values": None, + }, + }, + ) + dashboard = Dashboard.objects.create(team=self.team, filters=dashboard_filters) + + data = query_insight.dashboard_query(dashboard) + assert data + actual = data["source"]["filters"] + assert expected_filters == actual + + def test_query_hash_varies_with_dashboard_filters(self) -> None: + query = { + "kind": "DataTableNode", + "source": { + "filters": {"dateRange": {"date_from": "-14d", "date_to": "-7d"}}, + "kind": "HogQLQuery", + "modifiers": None, + "query": "select * from events where {filters}", + "response": None, + "values": None, + }, + } + dashboard_filters = {"date_from": "-4d", "date_to": "-3d"} + + query_insight = Insight.objects.create(team=self.team, query=query) + dashboard = Dashboard.objects.create(team=self.team, filters=dashboard_filters) + + hash_sans_dashboard = generate_insight_cache_key(query_insight, None) + hash_with_dashboard = generate_insight_cache_key(query_insight, dashboard) + + assert hash_sans_dashboard != hash_with_dashboard diff --git a/posthog/schema.py b/posthog/schema.py index 4370ee5a9c7a9..10d2e5e62372c 100644 --- a/posthog/schema.py +++ b/posthog/schema.py @@ -947,6 +947,30 @@ class AnyResponseType(RootModel): ] +class DashboardFilter(BaseModel): + model_config = ConfigDict( + extra="forbid", + ) + date_from: Optional[str] = None + date_to: Optional[str] = None + properties: Optional[ + List[ + Union[ + EventPropertyFilter, + PersonPropertyFilter, + ElementPropertyFilter, + SessionPropertyFilter, + CohortPropertyFilter, + RecordingDurationFilter, + GroupPropertyFilter, + FeaturePropertyFilter, + HogQLPropertyFilter, + EmptyPropertyFilter, + ] + ] + ] = None + + class DatabaseSchemaQuery(BaseModel): model_config = ConfigDict( extra="forbid",