From 2d896851f43a40f898a8a387076d4880de72c252 Mon Sep 17 00:00:00 2001 From: Michael Matloka Date: Mon, 9 Sep 2024 16:20:43 +0200 Subject: [PATCH] fix(dashboards): Make sure loading dashboard items does not `POST /query/` (#24808) --- cypress/e2e/dashboard-shared.cy.ts | 3 +- .../queries/nodes/DataNode/dataNodeLogic.ts | 11 +++- .../nodes/DataTable/dataTableLogic.test.ts | 3 +- frontend/src/queries/query.ts | 61 +++++++++++++------ frontend/src/queries/utils.ts | 7 +++ frontend/src/scenes/funnels/funnelUtils.ts | 2 +- posthog/models/person_overrides/sql.py | 2 +- 7 files changed, 61 insertions(+), 28 deletions(-) diff --git a/cypress/e2e/dashboard-shared.cy.ts b/cypress/e2e/dashboard-shared.cy.ts index ba24a6a558aac..4e46554160424 100644 --- a/cypress/e2e/dashboard-shared.cy.ts +++ b/cypress/e2e/dashboard-shared.cy.ts @@ -62,7 +62,6 @@ describe('Shared dashboard', () => { cy.get('.InsightCard').should('have.length', 6) // Make sure no element with text "There are no matching events for this query" exists - // TODO this was failing, it shouldn't be but YOLO - // cy.get('.insight-empty-state').should('not.exist') + cy.get('.insight-empty-state').should('not.exist') }) }) diff --git a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts index c5de284dea2c6..99c5cb6af687b 100644 --- a/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts +++ b/frontend/src/queries/nodes/DataNode/dataNodeLogic.ts @@ -122,7 +122,11 @@ export const dataNodeLogic = kea([ } }), actions({ - loadData: (refresh = false, queryId?: string) => ({ refresh, queryId: queryId || uuid() }), + loadData: (refresh = false, alreadyRunningQueryId?: string) => ({ + refresh, + queryId: alreadyRunningQueryId || uuid(), + pollOnly: !!alreadyRunningQueryId, + }), abortAnyRunningQuery: true, abortQuery: (payload: { queryId: string }) => payload, cancelQuery: true, @@ -142,7 +146,7 @@ export const dataNodeLogic = kea([ { setResponse: (response) => response, clearResponse: () => null, - loadData: async ({ refresh: refreshArg, queryId }, breakpoint) => { + loadData: async ({ refresh: refreshArg, queryId, pollOnly }, breakpoint) => { const refresh = props.alwaysRefresh || refreshArg if (props.doNotLoad) { return props.cachedResults @@ -200,7 +204,8 @@ export const dataNodeLogic = kea([ methodOptions, refresh, queryId, - actions.setPollResponse + actions.setPollResponse, + pollOnly )) ?? null const duration = performance.now() - now return { data, duration } diff --git a/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts b/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts index f1798c8e8149f..3dcfc6d5bc82c 100644 --- a/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts +++ b/frontend/src/queries/nodes/DataTable/dataTableLogic.test.ts @@ -65,7 +65,8 @@ describe('dataTableLogic', () => { expect.anything(), false, expect.any(String), - expect.any(Function) + expect.any(Function), + false ) expect(performQuery).toHaveBeenCalledTimes(1) }) diff --git a/frontend/src/queries/query.ts b/frontend/src/queries/query.ts index 8d3942f495823..3c534486efce0 100644 --- a/frontend/src/queries/query.ts +++ b/frontend/src/queries/query.ts @@ -7,7 +7,14 @@ import posthog from 'posthog-js' import { OnlineExportContext, QueryExportContext } from '~/types' import { DataNode, HogQLQuery, HogQLQueryResponse, NodeKind, PersonsNode, QueryStatus } from './schema' -import { isDataTableNode, isDataVisualizationNode, isHogQLQuery, isInsightVizNode, isPersonsNode } from './utils' +import { + isAsyncResponse, + isDataTableNode, + isDataVisualizationNode, + isHogQLQuery, + isInsightVizNode, + isPersonsNode, +} from './utils' const QUERY_ASYNC_MAX_INTERVAL_SECONDS = 3 const QUERY_ASYNC_TOTAL_POLL_SECONDS = 10 * 60 + 6 // keep in sync with backend-side timeout (currently 10min) + a small buffer @@ -29,7 +36,6 @@ export function queryExportContext( const SYNC_ONLY_QUERY_KINDS = [ 'HogQuery', 'HogQLMetadata', - 'EventsQuery', 'HogQLAutocomplete', 'DatabaseSchemaQuery', 'ErrorTrackingQuery', @@ -39,7 +45,7 @@ export async function pollForResults( queryId: string, showProgress: boolean, methodOptions?: ApiMethodOptions, - callback?: (response: QueryStatus) => void + onPoll?: (response: QueryStatus) => void ): Promise { const pollStart = performance.now() let currentDelay = 300 // start low, because all queries will take at minimum this @@ -50,12 +56,11 @@ export async function pollForResults( try { const statusResponse = (await api.queryStatus.get(queryId, showProgress)).query_status - if (statusResponse.complete) { return statusResponse } - if (callback) { - callback(statusResponse) + if (onPoll) { + onPoll(statusResponse) } } catch (e: any) { e.detail = e.data?.query_status?.error_message @@ -73,27 +78,42 @@ async function executeQuery( methodOptions?: ApiMethodOptions, refresh?: boolean, queryId?: string, - setPollResponse?: (response: QueryStatus) => void + setPollResponse?: (response: QueryStatus) => void, + /** + * Whether to limit the function to just polling the provided query ID. + * This is important in shared contexts, where we cannot create arbitrary queries via POST – we can only GET. + */ + pollOnly = false ): Promise> { const isAsyncQuery = methodOptions?.async !== false && !SYNC_ONLY_QUERY_KINDS.includes(queryNode.kind) && !!featureFlagLogic.findMounted()?.values.featureFlags?.[FEATURE_FLAGS.QUERY_ASYNC] - const showProgress = !!featureFlagLogic.findMounted()?.values.featureFlags?.[FEATURE_FLAGS.INSIGHT_LOADING_BAR] - const response = await api.query(queryNode, methodOptions, queryId, refresh, isAsyncQuery) + if (!pollOnly) { + const response = await api.query(queryNode, methodOptions, queryId, refresh, isAsyncQuery) - if (!response.query_status?.query_async) { - // Executed query synchronously or from cache - return response - } - if (response.query_status?.complete) { - // Async query returned immediately - return response.results - } + if (!isAsyncResponse(response)) { + // Executed query synchronously or from cache + return response + } - const statusResponse = await pollForResults(response.query_status.id, showProgress, methodOptions, setPollResponse) + if (response.query_status.complete) { + // Async query returned immediately + return response.results + } + + queryId = response.query_status.id + } else { + if (!isAsyncQuery) { + throw new Error('pollOnly is only supported for async queries') + } + if (!queryId) { + throw new Error('pollOnly requires a queryId') + } + } + const statusResponse = await pollForResults(queryId, showProgress, methodOptions, setPollResponse) return statusResponse.results } @@ -103,7 +123,8 @@ export async function performQuery( methodOptions?: ApiMethodOptions, refresh?: boolean, queryId?: string, - setPollResponse?: (status: QueryStatus) => void + setPollResponse?: (status: QueryStatus) => void, + pollOnly = false ): Promise> { let response: NonNullable const logParams: Record = {} @@ -113,7 +134,7 @@ export async function performQuery( if (isPersonsNode(queryNode)) { response = await api.get(getPersonsEndpoint(queryNode), methodOptions) } else { - response = await executeQuery(queryNode, methodOptions, refresh, queryId, setPollResponse) + response = await executeQuery(queryNode, methodOptions, refresh, queryId, setPollResponse, pollOnly) if (isHogQLQuery(queryNode) && response && typeof response === 'object') { logParams.clickhouse_sql = (response as HogQLQueryResponse)?.clickhouse } diff --git a/frontend/src/queries/utils.ts b/frontend/src/queries/utils.ts index 2d140f843a037..ed9cfc8d2fcf1 100644 --- a/frontend/src/queries/utils.ts +++ b/frontend/src/queries/utils.ts @@ -30,6 +30,8 @@ import { NodeKind, PathsQuery, PersonsNode, + QuerySchema, + QueryStatusResponse, RetentionQuery, SavedInsightNode, SessionAttributionExplorerQuery, @@ -78,6 +80,7 @@ export function isDataWarehouseNode(node?: Record | null): node is return node?.kind === NodeKind.DataWarehouseNode } +/** @deprecated `ActorsQuery` is now used instead of `PersonsNode`. */ export function isPersonsNode(node?: Record | null): node is PersonsNode { return node?.kind === NodeKind.PersonsNode } @@ -204,6 +207,10 @@ export function isQueryForGroup(query: PersonsNode | ActorsQuery): boolean { ) } +export function isAsyncResponse(response: NonNullable): response is QueryStatusResponse { + return 'query_status' in response && response.query_status +} + export function isInsightQueryWithSeries( node?: Node ): node is TrendsQuery | FunnelsQuery | StickinessQuery | LifecycleQuery { diff --git a/frontend/src/scenes/funnels/funnelUtils.ts b/frontend/src/scenes/funnels/funnelUtils.ts index da23089d9aad5..931935415cc3a 100644 --- a/frontend/src/scenes/funnels/funnelUtils.ts +++ b/frontend/src/scenes/funnels/funnelUtils.ts @@ -389,7 +389,7 @@ export function flattenedStepsByBreakdown( breakdown_value: 'Baseline', })), conversionRates: { - total: (lastStep?.count ?? 0) / (baseStep?.count ?? 1), + total: (lastStep?.count || 0) / (baseStep?.count || 1), }, }) } diff --git a/posthog/models/person_overrides/sql.py b/posthog/models/person_overrides/sql.py index c518db6de0e11..5c7a82c9469c1 100644 --- a/posthog/models/person_overrides/sql.py +++ b/posthog/models/person_overrides/sql.py @@ -69,7 +69,7 @@ ENGINE = ReplicatedReplacingMergeTree( -- NOTE: for testing we use a uuid to ensure that we don't get conflicts -- when the tests tear down and recreate the table. - '/clickhouse/tables/{'{uuid}' if settings.TEST else ''}noshard/{CLICKHOUSE_DATABASE}.person_overrides', + '/clickhouse/tables/{'{uuid}' if settings.TEST or settings.E2E_TESTING else ''}noshard/{CLICKHOUSE_DATABASE}.person_overrides', '{{replica}}-{{shard}}', version )