From 671a1a634c564c711ee053c52c4ec988e6ec3bdd Mon Sep 17 00:00:00 2001 From: Aditya Hegde Date: Thu, 19 Dec 2024 11:06:18 +0530 Subject: [PATCH] fix: human readable params issues like special chars in filters and url history issues (#6301) * Allow special chars in column names in filters * Use time controls store in url params calcs * Remove session store after navigating away from dashboard * Fix bookmarks not being applied if the store was initialised * PR comments --- web-admin/src/features/bookmarks/selectors.ts | 1 + .../dashboards/query-mappers/utils.ts | 8 ++ .../dashboards/stores/dashboard-stores.ts | 2 - .../dashboards/stores/test-data/data.ts | 1 + .../time-controls/time-control-store.ts | 72 ++++++++--- .../url-state/DashboardURLStateSync.svelte | 115 ++++++++++++++---- .../url-state/convertExploreStateToPreset.ts | 46 +++---- .../convertExploreStateToURLSearchParams.ts | 102 ++++++++++------ .../url-state/convertPresetToExploreState.ts | 5 + .../url-state/explore-web-view-store.spec.ts | 10 +- .../url-state/explore-web-view-store.ts | 20 ++- .../url-state/filters/converters.ts | 29 ++++- .../url-state/filters/expression.cjs | 2 +- .../url-state/filters/expression.ne | 2 +- .../url-state/filters/expression.spec.ts | 44 +++++-- .../url-state/getUpdatedUrlForExploreState.ts | 39 ------ .../url-state/url-state-variations.spec.ts | 7 ++ 17 files changed, 342 insertions(+), 163 deletions(-) delete mode 100644 web-common/src/features/dashboards/url-state/getUpdatedUrlForExploreState.ts diff --git a/web-admin/src/features/bookmarks/selectors.ts b/web-admin/src/features/bookmarks/selectors.ts index d870486ad62..026c7627edc 100644 --- a/web-admin/src/features/bookmarks/selectors.ts +++ b/web-admin/src/features/bookmarks/selectors.ts @@ -156,6 +156,7 @@ export function convertBookmarkToUrlSearchParams( ...exploreStateFromBookmark, } as MetricsExplorerEntity, exploreSpec, + undefined, // TODO defaultExplorePreset, ); } diff --git a/web-admin/src/features/dashboards/query-mappers/utils.ts b/web-admin/src/features/dashboards/query-mappers/utils.ts index 9168f2bb090..f2c14056076 100644 --- a/web-admin/src/features/dashboards/query-mappers/utils.ts +++ b/web-admin/src/features/dashboards/query-mappers/utils.ts @@ -1,5 +1,6 @@ import { createInExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; +import { getTimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { PreviousCompleteRangeMap } from "@rilldata/web-common/features/dashboards/time-controls/time-range-mappers"; import { convertExploreStateToURLSearchParams } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams"; import { getDefaultExplorePreset } from "@rilldata/web-common/features/dashboards/url-state/getDefaultExplorePreset"; @@ -182,6 +183,7 @@ export async function getExplorePageUrl( const url = new URL(`${curPageUrl.protocol}//${curPageUrl.host}`); url.pathname = `/${organization}/${project}/explore/${exploreName}`; + const metricsViewSpec = metricsView?.metricsView?.state?.validSpec ?? {}; const exploreSpec = explore?.explore?.state?.validSpec ?? {}; const metricsViewName = exploreSpec.metricsView; @@ -206,6 +208,12 @@ export async function getExplorePageUrl( url.search = convertExploreStateToURLSearchParams( exploreState, exploreSpec, + getTimeControlState( + metricsViewSpec, + exploreSpec, + fullTimeRange?.timeRangeSummary, + exploreState, + ), getDefaultExplorePreset(exploreSpec, fullTimeRange), ); return url.toString(); diff --git a/web-common/src/features/dashboards/stores/dashboard-stores.ts b/web-common/src/features/dashboards/stores/dashboard-stores.ts index 57a4b78f06b..51ef6a4168b 100644 --- a/web-common/src/features/dashboards/stores/dashboard-stores.ts +++ b/web-common/src/features/dashboards/stores/dashboard-stores.ts @@ -168,8 +168,6 @@ function syncDimensions( const metricsViewReducers = { init(name: string, initState: Partial = {}) { update((state) => { - if (state.entities[name]) return state; - state.entities[name] = getFullInitExploreState(name, initState); updateMetricsExplorerProto(state.entities[name]); diff --git a/web-common/src/features/dashboards/stores/test-data/data.ts b/web-common/src/features/dashboards/stores/test-data/data.ts index 18a707952ed..79a329a1d99 100644 --- a/web-common/src/features/dashboards/stores/test-data/data.ts +++ b/web-common/src/features/dashboards/stores/test-data/data.ts @@ -193,6 +193,7 @@ export const AD_BIDS_METRICS_3_MEASURES_DIMENSIONS: V1MetricsViewSpec = { table: AD_BIDS_SOURCE_NAME, measures: AD_BIDS_THREE_MEASURES, dimensions: AD_BIDS_THREE_DIMENSIONS, + timeDimension: AD_BIDS_TIMESTAMP_DIMENSION, }; export const AD_BIDS_EXPLORE_INIT: V1ExploreSpec = { diff --git a/web-common/src/features/dashboards/time-controls/time-control-store.ts b/web-common/src/features/dashboards/time-controls/time-control-store.ts index e2c63267f52..ed8859ef762 100644 --- a/web-common/src/features/dashboards/time-controls/time-control-store.ts +++ b/web-common/src/features/dashboards/time-controls/time-control-store.ts @@ -30,6 +30,7 @@ import { type V1MetricsViewSpec, type V1MetricsViewTimeRangeResponse, V1TimeGrain, + type V1TimeRangeSummary, } from "@rilldata/web-common/runtime-client"; import type { QueryObserverResult } from "@tanstack/svelte-query"; import type { Readable } from "svelte/store"; @@ -68,6 +69,12 @@ export type TimeControlState = { ComparisonTimeRangeState; export type TimeControlStore = Readable; +/** + * Returns a TimeControlState. Calls getTimeControlState internally. + * + * Consumers of this will have a QueryObserverResult for time range summary. + * They will need `isFetching` to wait for this response. + */ export const timeControlStateSelector = ([ metricsView, explore, @@ -80,15 +87,13 @@ export const timeControlStateSelector = ([ MetricsExplorerEntity, ]): TimeControlState => { const hasTimeSeries = Boolean(metricsView?.timeDimension); - const timeDimension = metricsView?.timeDimension; if ( !metricsView || !explore || !metricsExplorer || !timeRangeResponse || !timeRangeResponse.isSuccess || - !timeRangeResponse.data.timeRangeSummary?.min || - !timeRangeResponse.data.timeRangeSummary?.max + !hasTimeSeries ) { return { isFetching: timeRangeResponse.isRefetching, @@ -96,54 +101,85 @@ export const timeControlStateSelector = ([ } as TimeControlState; } + const state = getTimeControlState( + metricsView, + explore, + timeRangeResponse.data?.timeRangeSummary, + metricsExplorer, + ); + if (!state) { + return { + ready: false, + isFetching: false, + }; + } + + return { + ...state, + isFetching: false, + ready: true, + } as TimeControlState; +}; + +/** + * Generates TimeControlState + * + * Consumers of this will already have a V1TimeRangeSummary. + */ +export function getTimeControlState( + metricsViewSpec: V1MetricsViewSpec, + exploreSpec: V1ExploreSpec, + timeRangeSummary: V1TimeRangeSummary | undefined, + exploreState: MetricsExplorerEntity, +) { + const hasTimeSeries = Boolean(metricsViewSpec.timeDimension); + const timeDimension = metricsViewSpec.timeDimension; + if (!hasTimeSeries || !timeRangeSummary?.max || !timeRangeSummary?.min) + return undefined; + const allTimeRange = { name: TimeRangePreset.ALL_TIME, - start: new Date(timeRangeResponse.data.timeRangeSummary.min), - end: new Date(timeRangeResponse.data.timeRangeSummary.max), + start: new Date(timeRangeSummary.min), + end: new Date(timeRangeSummary.max), }; const minTimeGrain = - (metricsView.smallestTimeGrain as V1TimeGrain) || + (metricsViewSpec.smallestTimeGrain as V1TimeGrain) || V1TimeGrain.TIME_GRAIN_UNSPECIFIED; const defaultTimeRange = isoDurationToFullTimeRange( - explore?.defaultPreset?.timeRange, + exploreSpec.defaultPreset?.timeRange, allTimeRange.start, allTimeRange.end, - metricsExplorer.selectedTimezone, + exploreState.selectedTimezone, ); const timeRangeState = calculateTimeRangePartial( - metricsExplorer, + exploreState, allTimeRange, defaultTimeRange, minTimeGrain, ); if (!timeRangeState) { - return { - ready: false, - isFetching: false, - }; + return undefined; } const comparisonTimeRangeState = calculateComparisonTimeRangePartial( - explore, - metricsExplorer, + exploreSpec, + exploreState, allTimeRange, timeRangeState, ); return { - isFetching: false, minTimeGrain, allTimeRange, defaultTimeRange, timeDimension, - ready: true, ...timeRangeState, ...comparisonTimeRangeState, } as TimeControlState; -}; +} export function createTimeControlStore(ctx: StateManagers) { return derived( diff --git a/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte b/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte index 66a5ef45b64..4bbca009f62 100644 --- a/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte +++ b/web-common/src/features/dashboards/url-state/DashboardURLStateSync.svelte @@ -1,13 +1,23 @@ {#if schemaError} diff --git a/web-common/src/features/dashboards/url-state/convertExploreStateToPreset.ts b/web-common/src/features/dashboards/url-state/convertExploreStateToPreset.ts index 450fe04a0d4..742d8eac425 100644 --- a/web-common/src/features/dashboards/url-state/convertExploreStateToPreset.ts +++ b/web-common/src/features/dashboards/url-state/convertExploreStateToPreset.ts @@ -5,6 +5,7 @@ import { } from "@rilldata/web-common/features/dashboards/pivot/types"; import { createAndExpression } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; +import type { TimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { toTimeRangeParam } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams"; import { FromLegacySortTypeMap } from "@rilldata/web-common/features/dashboards/url-state/legacyMappers"; import { @@ -13,7 +14,6 @@ import { ToURLParamTimeDimensionMap, ToURLParamTimeGrainMapMap, } from "@rilldata/web-common/features/dashboards/url-state/mappers"; -import { inferCompareTimeRange } from "@rilldata/web-common/lib/time/comparisons"; import { DashboardState_LeaderboardSortDirection } from "@rilldata/web-common/proto/gen/rill/ui/v1/dashboard_pb"; import { V1ExploreComparisonMode, @@ -24,6 +24,7 @@ import { export function convertExploreStateToPreset( exploreState: Partial, exploreSpec: V1ExploreSpec, + timeControlsState: TimeControlState | undefined, ) { const preset: V1ExplorePreset = {}; @@ -38,7 +39,9 @@ export function convertExploreStateToPreset( ); } - Object.assign(preset, getTimeRangeFields(exploreState, exploreSpec)); + if (timeControlsState) { + Object.assign(preset, getTimeRangeFields(exploreState, timeControlsState)); + } Object.assign(preset, getExploreFields(exploreState, exploreSpec)); @@ -51,40 +54,27 @@ export function convertExploreStateToPreset( function getTimeRangeFields( exploreState: Partial, - exploreSpec: V1ExploreSpec, + timeControlsState: TimeControlState, ) { const preset: V1ExplorePreset = {}; - if (exploreState.selectedTimeRange?.name) { + if (timeControlsState.selectedTimeRange?.name) { preset.timeRange = toTimeRangeParam(exploreState.selectedTimeRange); } - if (exploreState.selectedTimeRange?.interval) { + if (timeControlsState.selectedTimeRange?.interval) { preset.timeGrain = - ToURLParamTimeGrainMapMap[exploreState.selectedTimeRange.interval]; + ToURLParamTimeGrainMapMap[timeControlsState.selectedTimeRange.interval]; } - if (exploreState.showTimeComparison) { - if (exploreState.selectedComparisonTimeRange?.name) { - preset.compareTimeRange = toTimeRangeParam( - exploreState.selectedComparisonTimeRange, - ); - preset.comparisonMode = - V1ExploreComparisonMode.EXPLORE_COMPARISON_MODE_TIME; - } else if ( - !exploreState.selectedComparisonTimeRange && - exploreState.selectedTimeRange?.name - ) { - // we infer compare time range if the user has not explicitly selected one but has enabled comparison - const inferredCompareTimeRange = inferCompareTimeRange( - exploreSpec.timeRanges, - exploreState.selectedTimeRange.name, - ); - if (inferredCompareTimeRange) { - preset.compareTimeRange = inferredCompareTimeRange; - preset.comparisonMode = - V1ExploreComparisonMode.EXPLORE_COMPARISON_MODE_TIME; - } - } + if ( + exploreState.showTimeComparison && + timeControlsState.selectedComparisonTimeRange?.name + ) { + preset.compareTimeRange = toTimeRangeParam( + exploreState.selectedComparisonTimeRange, + ); + preset.comparisonMode = + V1ExploreComparisonMode.EXPLORE_COMPARISON_MODE_TIME; } if (exploreState.selectedComparisonDimension !== undefined) { diff --git a/web-common/src/features/dashboards/url-state/convertExploreStateToURLSearchParams.ts b/web-common/src/features/dashboards/url-state/convertExploreStateToURLSearchParams.ts index 8a94f16bf77..5334190dde8 100644 --- a/web-common/src/features/dashboards/url-state/convertExploreStateToURLSearchParams.ts +++ b/web-common/src/features/dashboards/url-state/convertExploreStateToURLSearchParams.ts @@ -5,10 +5,12 @@ import { } from "@rilldata/web-common/features/dashboards/pivot/types"; import { SortDirection } from "@rilldata/web-common/features/dashboards/proto-state/derived-types"; import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; +import type { TimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { convertExpressionToFilterParam } from "@rilldata/web-common/features/dashboards/url-state/filters/converters"; import { FromLegacySortTypeMap } from "@rilldata/web-common/features/dashboards/url-state/legacyMappers"; import { FromActivePageMap, + FromURLParamViewMap, ToURLParamSortTypeMap, ToURLParamTDDChartMap, ToURLParamTimeDimensionMap, @@ -20,7 +22,6 @@ import { arrayOrderedEquals, arrayUnorderedEquals, } from "@rilldata/web-common/lib/arrayUtils"; -import { inferCompareTimeRange } from "@rilldata/web-common/lib/time/comparisons"; import { TimeComparisonOption, type TimeRange, @@ -33,9 +34,46 @@ import { type V1ExploreSpec, } from "@rilldata/web-common/runtime-client"; +/** + * Sometimes data is loaded from sources other than the url. + * In that case update the URL to make sure the state matches the current url. + */ +export function getUpdatedUrlForExploreState( + exploreSpec: V1ExploreSpec, + timeControlsState: TimeControlState | undefined, + defaultExplorePreset: V1ExplorePreset, + partialExploreState: Partial, + curSearchParams: URLSearchParams, +) { + const newUrlSearchParams = new URLSearchParams( + convertExploreStateToURLSearchParams( + partialExploreState as MetricsExplorerEntity, + exploreSpec, + timeControlsState, + defaultExplorePreset, + ), + ); + curSearchParams.forEach((value, key) => { + if ( + key === ExploreStateURLParams.WebView && + FromURLParamViewMap[value] === defaultExplorePreset.view + ) { + // ignore default view. + // since we do not add params equal to default this will append to the end of the URL breaking the param order. + return; + } + newUrlSearchParams.set(key, value); + }); + return newUrlSearchParams.toString(); +} + export function convertExploreStateToURLSearchParams( exploreState: MetricsExplorerEntity, exploreSpec: V1ExploreSpec, + // We have quite a bit of logic in TimeControlState to validate selections and update them + // Eg: if a selected grain is not applicable for the current grain then we change it + // But it is only available in TimeControlState and not MetricsExplorerEntity + timeControlsState: TimeControlState | undefined, preset: V1ExplorePreset, ) { const searchParams = new URLSearchParams(); @@ -50,10 +88,13 @@ export function convertExploreStateToURLSearchParams( ); } - mergeSearchParams( - toTimeRangesUrl(exploreState, exploreSpec, preset), - searchParams, - ); + // timeControlsState will be undefined for dashboards without timeseries + if (timeControlsState) { + mergeSearchParams( + toTimeRangesUrl(exploreState, exploreSpec, timeControlsState, preset), + searchParams, + ); + } const expr = mergeMeasureFilters(exploreState); if (expr && expr?.cond?.exprs?.length) { @@ -91,14 +132,17 @@ export function convertExploreStateToURLSearchParams( function toTimeRangesUrl( exploreState: MetricsExplorerEntity, exploreSpec: V1ExploreSpec, + timeControlsState: TimeControlState, preset: V1ExplorePreset, ) { const searchParams = new URLSearchParams(); - if (shouldSetParam(preset.timeRange, exploreState.selectedTimeRange?.name)) { + if ( + shouldSetParam(preset.timeRange, timeControlsState.selectedTimeRange?.name) + ) { searchParams.set( ExploreStateURLParams.TimeRange, - toTimeRangeParam(exploreState.selectedTimeRange), + toTimeRangeParam(timeControlsState.selectedTimeRange), ); } @@ -112,38 +156,24 @@ function toTimeRangesUrl( ); } - if (exploreState.showTimeComparison) { - if ( - (preset.compareTimeRange !== undefined && - exploreState.selectedComparisonTimeRange !== undefined && - exploreState.selectedComparisonTimeRange.name !== - preset.compareTimeRange) || - preset.compareTimeRange === undefined - ) { - searchParams.set( - ExploreStateURLParams.ComparisonTimeRange, - toTimeRangeParam(exploreState.selectedComparisonTimeRange), - ); - } else if ( - !exploreState.selectedComparisonTimeRange?.name && - exploreState.selectedTimeRange?.name - ) { - // we infer compare time range if the user has not explicitly selected one but has enabled comparison - const inferredCompareTimeRange = inferCompareTimeRange( - exploreSpec.timeRanges, - exploreState.selectedTimeRange.name, - ); - if (inferredCompareTimeRange) - searchParams.set( - ExploreStateURLParams.ComparisonTimeRange, - inferredCompareTimeRange, - ); - } + if ( + exploreState.showTimeComparison && + ((preset.compareTimeRange !== undefined && + timeControlsState.selectedComparisonTimeRange !== undefined && + timeControlsState.selectedComparisonTimeRange.name !== + preset.compareTimeRange) || + preset.compareTimeRange === undefined) + ) { + searchParams.set( + ExploreStateURLParams.ComparisonTimeRange, + toTimeRangeParam(timeControlsState.selectedComparisonTimeRange), + ); } const mappedTimeGrain = - ToURLParamTimeGrainMapMap[exploreState.selectedTimeRange?.interval ?? ""] ?? - ""; + ToURLParamTimeGrainMapMap[ + timeControlsState.selectedTimeRange?.interval ?? "" + ] ?? ""; if (mappedTimeGrain && shouldSetParam(preset.timeGrain, mappedTimeGrain)) { searchParams.set(ExploreStateURLParams.TimeGrain, mappedTimeGrain); } diff --git a/web-common/src/features/dashboards/url-state/convertPresetToExploreState.ts b/web-common/src/features/dashboards/url-state/convertPresetToExploreState.ts index c9c79f98ad4..9f89482dd36 100644 --- a/web-common/src/features/dashboards/url-state/convertPresetToExploreState.ts +++ b/web-common/src/features/dashboards/url-state/convertPresetToExploreState.ts @@ -158,6 +158,11 @@ function fromTimeRangesParams( // unset compare dimension partialExploreState.selectedComparisonDimension = ""; } + } else if ( + preset.comparisonMode === + V1ExploreComparisonMode.EXPLORE_COMPARISON_MODE_TIME + ) { + partialExploreState.showTimeComparison = true; } if (preset.comparisonDimension) { diff --git a/web-common/src/features/dashboards/url-state/explore-web-view-store.spec.ts b/web-common/src/features/dashboards/url-state/explore-web-view-store.spec.ts index f7282ab131d..cf7c36a1bd7 100644 --- a/web-common/src/features/dashboards/url-state/explore-web-view-store.spec.ts +++ b/web-common/src/features/dashboards/url-state/explore-web-view-store.spec.ts @@ -28,6 +28,7 @@ import { applyMutationsToDashboard, type TestDashboardMutation, } from "@rilldata/web-common/features/dashboards/stores/test-data/store-mutations"; +import { getTimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { convertExploreStateToURLSearchParams } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams"; import { convertPresetToExploreState } from "@rilldata/web-common/features/dashboards/url-state/convertPresetToExploreState"; import { @@ -326,10 +327,17 @@ function getUrlForWebView( explorePresetFromSessionStorage, ); + const exploreState = partialExploreState as MetricsExplorerEntity; newUrl.search = convertExploreStateToURLSearchParams( - partialExploreState as MetricsExplorerEntity, + exploreState, AD_BIDS_EXPLORE_INIT, + getTimeControlState( + AD_BIDS_METRICS_3_MEASURES_DIMENSIONS, + AD_BIDS_EXPLORE_INIT, + AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary, + exploreState, + ), defaultExplorePreset, ) + (additionalParams ?? ""); return newUrl; diff --git a/web-common/src/features/dashboards/url-state/explore-web-view-store.ts b/web-common/src/features/dashboards/url-state/explore-web-view-store.ts index dbc55cffc06..1067e480b31 100644 --- a/web-common/src/features/dashboards/url-state/explore-web-view-store.ts +++ b/web-common/src/features/dashboards/url-state/explore-web-view-store.ts @@ -1,4 +1,5 @@ import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; +import type { TimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { convertExploreStateToPreset } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToPreset"; import { FromActivePageMap } from "@rilldata/web-common/features/dashboards/url-state/mappers"; import { @@ -93,6 +94,7 @@ export function updateExploreSessionStore( prefix: string | undefined, exploreState: MetricsExplorerEntity, exploreSpec: V1ExploreSpec, + timeControlsState: TimeControlState | undefined, ) { const view = FromActivePageMap[exploreState.activePage]; const key = getKeyForSessionStore(exploreName, prefix, view); @@ -102,7 +104,11 @@ export function updateExploreSessionStore( SharedStateStoreKey, ); - const preset = convertExploreStateToPreset(exploreState, exploreSpec); + const preset = convertExploreStateToPreset( + exploreState, + exploreSpec, + timeControlsState, + ); const storedPreset: V1ExplorePreset = {}; const sharedPreset: V1ExplorePreset = { ...preset, @@ -184,3 +190,15 @@ export function getExplorePresetForWebView( return undefined; } } + +export function hasSessionStorageData( + exploreName: string, + prefix: string | undefined, +) { + const sharedKey = getKeyForSessionStore( + exploreName, + prefix, + SharedStateStoreKey, + ); + return !!sessionStorage.getItem(sharedKey); +} diff --git a/web-common/src/features/dashboards/url-state/filters/converters.ts b/web-common/src/features/dashboards/url-state/filters/converters.ts index b1d892170c4..42c684f876d 100644 --- a/web-common/src/features/dashboards/url-state/filters/converters.ts +++ b/web-common/src/features/dashboards/url-state/filters/converters.ts @@ -13,6 +13,7 @@ export function convertFilterParamToExpression(filter: string) { return parser.results[0] as V1Expression; } +const NonStandardName = /^[a-zA-Z][a-zA-Z0-9_]*$/; export function convertExpressionToFilterParam( expr: V1Expression, depth = 0, @@ -21,12 +22,12 @@ export function convertExpressionToFilterParam( if ("val" in expr) { if (typeof expr.val === "string") { - return `'${expr.val}'`; + return escapeValue(expr.val); } return expr.val + ""; } - if (expr.ident) return expr.ident; + if (expr.ident) return escapeColumnName(expr.ident); switch (expr.cond?.op) { case V1Operation.OPERATION_AND: @@ -73,6 +74,7 @@ function convertInExpressionToFilterParam(expr: V1Expression, depth: number) { const column = expr.cond.exprs[0]?.ident; if (!column) return ""; + const safeColumn = escapeColumnName(column); if (expr.cond.exprs[1]?.subquery?.having) { // TODO: support `NIN ` @@ -80,14 +82,14 @@ function convertInExpressionToFilterParam(expr: V1Expression, depth: number) { expr.cond.exprs[1]?.subquery?.having, 0, ); - if (having) return `${column} having (${having})`; + if (having) return `${safeColumn} having (${having})`; } if (expr.cond.exprs.length > 1) { const vals = expr.cond.exprs .slice(1) .map((e) => convertExpressionToFilterParam(e, depth + 1)); - return `${column} ${joiner} (${vals.join(",")})`; + return `${safeColumn} ${joiner} (${vals.join(",")})`; } return ""; @@ -107,3 +109,22 @@ function convertBinaryExpressionToFilterParam( return `${left} ${op} ${right}`; } + +function escapeColumnName(columnName: string) { + // if name doesnt have any special chars do not surround it by quotes. + // this makes the url more readable + if (NonStandardName.test(columnName)) return columnName; + const escapedColumnName = columnName + .replace(/\\/g, "\\\\") + .replace(/"/g, '\\"'); + return `"${escapedColumnName}"`; +} + +function escapeValue(value: string) { + const escapedValue = value + // TODO: this was a CodeQL suggestion. could this cause conflicts in values? + .replace(/\\/g, "\\\\") + .replace(/'/g, "\\'") + .replace(/\n/g, "\\n"); + return `'${escapedValue}'`; +} diff --git a/web-common/src/features/dashboards/url-state/filters/expression.cjs b/web-common/src/features/dashboards/url-state/filters/expression.cjs index 59db01f74bb..16a30709747 100644 --- a/web-common/src/features/dashboards/url-state/filters/expression.cjs +++ b/web-common/src/features/dashboards/url-state/filters/expression.cjs @@ -184,7 +184,7 @@ let ParserRules = [ {"name": "compare_operator", "symbols": ["compare_operator$subexpression$5"], "postprocess": id}, {"name": "compare_operator$subexpression$6", "symbols": [/[lL]/, /[tT]/, /[eE]/], "postprocess": function(d) {return d.join(""); }}, {"name": "compare_operator", "symbols": ["compare_operator$subexpression$6"], "postprocess": id}, - {"name": "column", "symbols": ["sqstring"], "postprocess": id}, + {"name": "column", "symbols": ["dqstring"], "postprocess": id}, {"name": "column$ebnf$1", "symbols": []}, {"name": "column$ebnf$1", "symbols": ["column$ebnf$1", /[a-zA-Z0-9_]/], "postprocess": function arrpush(d) {return d[0].concat([d[1]]);}}, {"name": "column", "symbols": [/[a-zA-Z]/, "column$ebnf$1"], "postprocess": ([fst, rest]) => [fst, ...rest].join("")}, diff --git a/web-common/src/features/dashboards/url-state/filters/expression.ne b/web-common/src/features/dashboards/url-state/filters/expression.ne index edcee420c70..b07cf571921 100644 --- a/web-common/src/features/dashboards/url-state/filters/expression.ne +++ b/web-common/src/features/dashboards/url-state/filters/expression.ne @@ -46,7 +46,7 @@ compare_operator => "eq"i {% id %} | "lt"i {% id %} | "lte"i {% id %} -column => sqstring {% id %} +column => dqstring {% id %} | [a-zA-Z] [a-zA-Z0-9_]:* {% ([fst, rest]) => [fst, ...rest].join("") %} value => sqstring {% id %} | int {% id %} diff --git a/web-common/src/features/dashboards/url-state/filters/expression.spec.ts b/web-common/src/features/dashboards/url-state/filters/expression.spec.ts index 831fb0a725f..3910c13ee6d 100644 --- a/web-common/src/features/dashboards/url-state/filters/expression.spec.ts +++ b/web-common/src/features/dashboards/url-state/filters/expression.spec.ts @@ -6,6 +6,7 @@ import { createSubQueryExpression, } from "@rilldata/web-common/features/dashboards/stores/filter-utils"; import { + convertExpressionToFilterParam, convertFilterParamToExpression, stripParserError, } from "@rilldata/web-common/features/dashboards/url-state/filters/converters"; @@ -19,18 +20,22 @@ describe("expression", () => { const Cases = [ { expr: "country IN ('US','IN')", - expected_expression: createInExpression("country", ["US", "IN"]), + expectedExprString: "country IN ('US','IN')", + expectedExprObject: createInExpression("country", ["US", "IN"]), }, { expr: "country IN ('US','IN') and state eq 'ABC'", - expected_expression: createAndExpression([ + expectedExprString: "country IN ('US','IN') AND state eq 'ABC'", + expectedExprObject: createAndExpression([ createInExpression("country", ["US", "IN"]), createBinaryExpression("state", V1Operation.OPERATION_EQ, "ABC"), ]), }, { expr: "country IN ('US','IN') and state eq 'ABC' and lat gte 12.56", - expected_expression: createAndExpression([ + expectedExprString: + "country IN ('US','IN') AND state eq 'ABC' AND lat gte 12.56", + expectedExprObject: createAndExpression([ createInExpression("country", ["US", "IN"]), createBinaryExpression("state", V1Operation.OPERATION_EQ, "ABC"), createBinaryExpression("lat", V1Operation.OPERATION_GTE, 12.56), @@ -38,7 +43,9 @@ describe("expression", () => { }, { expr: "country IN ('US','IN') AND state eq 'ABC' OR lat gte 12.56", - expected_expression: createAndExpression([ + expectedExprString: + "country IN ('US','IN') AND (state eq 'ABC' OR lat gte 12.56)", + expectedExprObject: createAndExpression([ createInExpression("country", ["US", "IN"]), createOrExpression([ createBinaryExpression("state", V1Operation.OPERATION_EQ, "ABC"), @@ -48,7 +55,9 @@ describe("expression", () => { }, { expr: "country not in ('US','IN') and (state eq 'ABC' or lat gte 12.56)", - expected_expression: createAndExpression([ + expectedExprString: + "country NIN ('US','IN') AND (state eq 'ABC' OR lat gte 12.56)", + expectedExprObject: createAndExpression([ createInExpression("country", ["US", "IN"], true), createOrExpression([ createBinaryExpression("state", V1Operation.OPERATION_EQ, "ABC"), @@ -58,7 +67,9 @@ describe("expression", () => { }, { expr: "country NIN ('US','IN') and state having (lat gte 12.56)", - expected_expression: createAndExpression([ + expectedExprString: + "country NIN ('US','IN') AND state having (lat gte 12.56)", + expectedExprObject: createAndExpression([ createInExpression("country", ["US", "IN"], true), createSubQueryExpression( "state", @@ -67,18 +78,33 @@ describe("expression", () => { ), ]), }, + { + expr: `"coun tr.y" IN ('U\\'S','I\\nN') and "st ate" having ("la t" gte 12.56)`, + expectedExprString: `"coun tr.y" IN ('U\\'S','I\\nN') AND "st ate" having ("la t" gte 12.56)`, + expectedExprObject: createAndExpression([ + // values converted to V1Expression do not have escaped chars + createInExpression("coun tr.y", ["U'S", "I\nN"]), + createSubQueryExpression( + "st ate", + ["la t"], + createBinaryExpression("la t", V1Operation.OPERATION_GTE, 12.56), + ), + ]), + }, ]; const compiledGrammar = nearley.Grammar.fromCompiled(grammar); - for (const { expr, expected_expression } of Cases) { + for (const { expr, expectedExprString, expectedExprObject } of Cases) { it(expr, () => { const parser = new nearley.Parser(compiledGrammar); parser.feed(expr); // assert that there is only match. this ensures unambiguous grammar. expect(parser.results).length(1); - expect(convertFilterParamToExpression(expr)).to.deep.eq( - expected_expression, + const exprObject = convertFilterParamToExpression(expr); + expect(exprObject).to.deep.eq(expectedExprObject); + expect(convertExpressionToFilterParam(exprObject)).toEqual( + expectedExprString, ); }); } diff --git a/web-common/src/features/dashboards/url-state/getUpdatedUrlForExploreState.ts b/web-common/src/features/dashboards/url-state/getUpdatedUrlForExploreState.ts deleted file mode 100644 index 69e512e008c..00000000000 --- a/web-common/src/features/dashboards/url-state/getUpdatedUrlForExploreState.ts +++ /dev/null @@ -1,39 +0,0 @@ -import type { MetricsExplorerEntity } from "@rilldata/web-common/features/dashboards/stores/metrics-explorer-entity"; -import { convertExploreStateToURLSearchParams } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams"; -import { FromURLParamViewMap } from "@rilldata/web-common/features/dashboards/url-state/mappers"; -import { ExploreStateURLParams } from "@rilldata/web-common/features/dashboards/url-state/url-params"; -import type { - V1ExplorePreset, - V1ExploreSpec, -} from "@rilldata/web-common/runtime-client"; - -/** - * Sometimes data is loaded from sources other than the url. - * In that case update the URL to make sure the state matches the current url. - */ -export function getUpdatedUrlForExploreState( - exploreSpec: V1ExploreSpec, - defaultExplorePreset: V1ExplorePreset, - partialExploreState: Partial, - curSearchParams: URLSearchParams, -) { - const newUrlSearchParams = new URLSearchParams( - convertExploreStateToURLSearchParams( - partialExploreState as MetricsExplorerEntity, - exploreSpec, - defaultExplorePreset, - ), - ); - curSearchParams.forEach((value, key) => { - if ( - key === ExploreStateURLParams.WebView && - FromURLParamViewMap[value] === defaultExplorePreset.view - ) { - // ignore default view. - // since we do not add params equal to default this will append to the end of the URL breaking the param order. - return; - } - newUrlSearchParams.set(key, value); - }); - return newUrlSearchParams.toString(); -} diff --git a/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts b/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts index 5bbf80f892c..745e5e70f5c 100644 --- a/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts +++ b/web-common/src/features/dashboards/url-state/url-state-variations.spec.ts @@ -42,6 +42,7 @@ import { applyMutationsToDashboard, type TestDashboardMutation, } from "@rilldata/web-common/features/dashboards/stores/test-data/store-mutations"; +import { getTimeControlState } from "@rilldata/web-common/features/dashboards/time-controls/time-control-store"; import { convertExploreStateToURLSearchParams } from "@rilldata/web-common/features/dashboards/url-state/convertExploreStateToURLSearchParams"; import { convertURLToExploreState } from "@rilldata/web-common/features/dashboards/url-state/convertPresetToExploreState"; import { getDefaultExplorePreset } from "@rilldata/web-common/features/dashboards/url-state/getDefaultExplorePreset"; @@ -363,6 +364,12 @@ describe("Human readable URL state variations", () => { url.search = convertExploreStateToURLSearchParams( get(metricsExplorerStore).entities[AD_BIDS_EXPLORE_NAME], explore, + getTimeControlState( + AD_BIDS_METRICS_3_MEASURES_DIMENSIONS, + explore, + AD_BIDS_TIME_RANGE_SUMMARY.timeRangeSummary, + get(metricsExplorerStore).entities[AD_BIDS_EXPLORE_NAME], + ), defaultExplorePreset, ); expect(url.toString()).to.eq(expectedUrl);