Conversation
…rity policy When a metrics view security policy excludes a measure or dimension, canvas components now silently drop the excluded fields instead of showing a ComponentError or stuck loading state. - KPI Grid: filters out excluded measures before rendering; shows only the KPIs the current user can access - Table (flat): filters inaccessible columns; only errors if no columns remain - Pivot: filters inaccessible measures/dimensions; only errors if nothing usable remains - Leaderboard: filters inaccessible measures/dimensions consistently - Pivot display now passes loading state to validator to avoid a brief "Metrics view not found" flash on initial load Closes rilldata#8850
247ba9d to
94ed2ee
Compare
nishantmonu51
left a comment
There was a problem hiding this comment.
The branch is three months behind main (merge base c9007517a0, June 11) and git merge-tree reports content conflicts in KPIGrid.svelte, leaderboard/selector.ts and CanvasPivotDisplay.svelte. Two landed changes matter for the rebase. #9674 already fixed the transient "Metrics view not found" flash by giving validateTableSchema/validateLeaderboardSchema a { metricsView, isLoading } argument and gating the pivotState update on !schema.isLoading, which makes the new isLoading parameter and the CanvasPivotRenderer schema type change redundant. More importantly, #9864 added adhoc_measures to the KPI grid, table, pivot and leaderboard specs, and those names are deliberately absent from metricsView.measures; the "keep only names present in metricsView.measures" filters in this PR would silently drop every ad hoc measure once rebased, so they need to union in the adhoc_measures names the way main's validators now do.
A second consequence of validating against validSpec: the frontend cannot distinguish "excluded by policy" from "does not exist", so a mistyped measure or dimension name in a table, pivot or leaderboard YAML now silently disappears in Rill Developer instead of producing the old Invalid measure(s) ... error. The parser does not validate component fields either, so nothing surfaces the typo any more.
| // Build accessible field lists by filtering out any fields not present in the | ||
| // metrics view spec (e.g. excluded by a security policy). | ||
| $: accessibleColumns = | ||
| "columns" in tableSpec | ||
| ? (tableSpec.columns || []).filter((c) => { | ||
| const allMeasures = | ||
| metricsViewSpec?.measures?.map((m) => m.name as string) || []; | ||
| const allDimensions = | ||
| metricsViewSpec?.dimensions?.map( | ||
| (d) => d.name || (d.column as string), | ||
| ) || []; | ||
| return allMeasures.includes(c) || allDimensions.includes(c); | ||
| }) | ||
| : []; | ||
|
|
||
| $: accessibleMeasures = | ||
| !("columns" in tableSpec) | ||
| ? (tableSpec.measures || []).filter((m) => | ||
| metricsViewSpec?.measures?.some((mv) => mv.name === m), | ||
| ) | ||
| : []; | ||
|
|
||
| $: accessibleRowDimensions = | ||
| !("columns" in tableSpec) | ||
| ? (tableSpec.row_dimensions || []).filter( | ||
| (d) => | ||
| metricsViewSpec?.dimensions?.some( | ||
| (mv) => mv.name === d || mv.column === d, | ||
| ) || | ||
| (metricsViewSpec?.timeDimension !== undefined && | ||
| isTimeDimension(d, metricsViewSpec.timeDimension)), | ||
| ) | ||
| : []; | ||
|
|
||
| $: accessibleColDimensions = | ||
| !("columns" in tableSpec) | ||
| ? (tableSpec.col_dimensions || []).filter( | ||
| (d) => | ||
| metricsViewSpec?.dimensions?.some( | ||
| (mv) => mv.name === d || mv.column === d, | ||
| ) || | ||
| (metricsViewSpec?.timeDimension !== undefined && | ||
| isTimeDimension(d, metricsViewSpec.timeDimension)), | ||
| ) | ||
| : []; |
There was a problem hiding this comment.
These lists only reach pivotState, so they filter the header chips and nothing else — the query is still built from the raw spec. createPivotConfig -> processPivot reads $tableSpec.measures/row_dimensions/col_dimensions (dashboards/pivot/util.ts:196-206) and processFlat reads $tableSpec.columns (util.ts:277-296) directly, and createPivotDataStore builds the request from config.measureNames/rowDimensionNames/colDimensionNames (pivot-data-store.ts:219-250), not from config.pivot.columns. With a policy excluding revenue from measures: [revenue, orders], the chips show only orders while the aggregation request still contains revenue, the runtime rejects it at runtime/metricsview/ast.go:720-731 (measure "revenue" not found), and CanvasPivotRenderer shows PivotError — the error this PR is meant to remove. Flat tables fail differently but just as hard: processFlat treats any column not in the (already filtered) measure list as a dimension (util.ts:278), so the excluded measure is sent as a dimension and fails at ast.go:702-713. The filtering has to be applied where measureNames/rowDimensionNames/colDimensionNames are computed, or the config has to be derived from pivotState; a unit test asserting that an excluded field is absent from the query config rather than from the chips would pin this down.
| const accessibleColumns = (tableSpec?.columns || []).filter( | ||
| (c) => allMeasures.includes(c) || allDimensions.includes(c), | ||
| ); |
There was a problem hiding this comment.
This drops legitimate time-grain columns. The table inspector explicitly allows "time" entries in columns (pivot/index.ts:211, allowedTypes: ["time", "dimension", "measure"]); those are named <timeDimension>_rill_TIME_GRAIN_* and are not in metricsView.dimensions, so they match neither allMeasures.includes nor allDimensions.includes. validatePivot below handles this with an isTimeDimension check, but neither validateFlat nor the matching accessibleColumns block in CanvasPivotDisplay.svelte has one. columns: [timestamp_rill_TIME_GRAIN_DAY, revenue] therefore loses its time column from pivotState while processFlat still queries it, and columns: [timestamp_rill_TIME_GRAIN_DAY] alone now fails with "Select at least one measure or dimension for the table".
| $: kpis = (kpiGridProperties.measures || []) | ||
| .filter( | ||
| (measure) => | ||
| $metricsViewQuery.isLoading || accessibleMeasureNames.has(measure), | ||
| ) |
There was a problem hiding this comment.
When this filter empties kpis, the component renders its header over an empty grid rather than an error. validateKPIGridSchema only inspects the raw spec, so once $metricsViewQuery.isLoading is false and the metrics view is missing (query error, wrong name) or every configured measure is excluded, nothing renders and nothing explains why; previously each KPIProvider surfaced "Metrics view X not found" / "Invalid measure". Table, pivot and leaderboard all still error when nothing survives filtering, so the KPI grid is the odd one out.
| const measures = (leaderboardSpec?.measures || []).filter((m) => | ||
| allMeasures.includes(m), | ||
| ); |
There was a problem hiding this comment.
Filtering measures here does not reach the sort measure, so the leaderboard still errors when the first configured measure is excluded. leaderboardState.leaderboardSortByMeasureName is initialised from the raw spec.measures[0] (leaderboard/index.ts:69, and again at :91 and :150 in validateAndResetSortMeasure), and LeaderboardDisplay falls back to the raw leaderboardMeasureNames?.[0] (LeaderboardDisplay.svelte:189-190); the aggregation query then sorts by a measure absent from visibleMeasures and the runtime rejects it in addOrderField with name not present in context (runtime/metricsview/ast.go:1039). This predates the PR, but the change here does not cover it.
When a metrics view security policy
excludes a measure or dimension, canvas components now silently drop those fields instead of showing aComponentErroror a stuck loading/floating UI.isLoadingstate to the schema validator to prevent a brief "Metrics view not found" error on initial loadCloses 8850
Checklist: