Enforced model security, semi-additive measures, concurrent/cached serving, and explore-UI features#252
Enforced model security, semi-additive measures, concurrent/cached serving, and explore-UI features#252nicosuave wants to merge 34 commits into
Conversation
…sors Query handling was serialized: the HTTP API wrapped every handler in app.state.lock, and the PG wire server's executor threads shared one DuckDB connection, so effective query concurrency was ~1. - db/base.py: add BaseDatabaseAdapter.cursor() with a lock-guarded fallback wrapper (_SerializedCursor) that preserves exactly the old single-connection serialization for adapters without concurrent-handle support. Lazy lock creation is double-checked under a global guard. - db/duckdb.py: override cursor() with conn.cursor(), duckdb-python's sanctioned independent handle for multithreaded reads over the same database. - api_server.py: classify handlers. All current endpoints are read-only (metadata reads of the in-memory graph, pure-CPU compile/rewrite, and SELECT-only query execution); none mutate layer state, so none take the lock anymore. /query, /sql, and /raw execute on a fresh adapter.cursor() per request. app.state.lock is retained, documented as reserved for future mutation endpoints (model registration, config reload). - server/connection.py: each executor-submitted query runs on its own adapter.cursor(); the Arrow streaming path (fetch_record_batch / send_reader) is unchanged since the cursor exposes the same API. - tests/server/test_concurrency.py: proof test with threads=1 DuckDB so each query is single-threaded; 4 concurrent slow queries complete in ~1.2x single-request wall time (bound 2.5x), and a control-plane read stays responsive while queries hammer the server. Library path (SemanticLayer.query / adapter.execute) is unchanged for single-threaded users. All adapters except DuckDB stay on the lock-guarded fallback.
Raise UnsupportedMetricError for metrics declaring non_additive_dimension (previously silently over-aggregated) unless allow_non_additive_unsafe=True. Memoize sqlglot fragment parsing in the generator (parse-once + copy), cutting representative 2-model join compile from ~26ms to ~10ms median (no coverage). Adds a coverage-robust compile-latency benchmark.
Define SecurityPolicy (access expression + row_filters) attachable via Model.security, plus SecurityError and a StrictUndefined render_row_filter helper. Round-trips through the native YAML adapter; regenerates JSON schema. Enforcement is a separate follow-up; field names/semantics are load-bearing.
Per-dimension filters gain mode (include/exclude/contains) with a searchable value list and a '+ Filter' affordance. filterExprs emits NOT IN for exclude and ESCAPE-safe ILIKE for contains; NULL handling mirrors the mode. URL state stays backward-compatible (bare value lists deserialize as include). 60 unit tests pass; Playwright e2e specs included (require the Python backend to run).
Move sqlglot, SemanticLayer, and dependency-analyzer imports off the module load path (into the functions that use them). import sidemantic drops from ~1170ms to ~290ms; sqlglot and duckdb no longer load on 'from sidemantic import Model'.
…askara-e8f7c3 # Conflicts: # sidemantic/core/semantic_layer.py
E2: global context-column setting (none | % of total | Δ | Δ%) toggled from each leaderboard header. % of total reuses the focused metric's ungrouped scorecard total threaded from ExplorerView (no extra query); Δ/Δ% issue one comparison-window query per panel joined by dimension value client-side, with a missing previous value rendering an em dash. E3: comparison-range picker (off | previous period | previous year | custom) in DateRangeControl. Adds previousYearRange (clamps Feb 29 to Feb 28). The chosen window is resolved once in ExplorerView and threaded to the chart and strip instead of each component calling previousRange; off disables the overlay and all deltas. Both settings are URL-serialized backward-compatibly (defaults omitted so old links decode to none/previous).
Add ResultCache: LRU-by-bytes Arrow cache with injectable-clock TTL and per-key singleflight so concurrent identical queries run compute once; an in-flight compute failure propagates to all waiters of that generation. Add a monotonic generation counter to SemanticLayer (bumped in add_model and add_metric, alongside the existing _sql_rewrite_cache clears) and a build_result_key helper keyed on compiled SQL, dialect + connection fingerprint, generation (layer counter + graph version), and sorted user_attributes (None today; A2 will populate them) so security-scoped results never collide across users. Wire an opt-in cache into the HTTP API server (--result-cache-mb, default 0; --result-cache-ttl, default 60) covering the /query, /sql, and /raw read-only handlers. The library SemanticLayer.query() is not cached by default. The PG wire server is left uncached because its riffq send_reader path streams single-use RecordBatchReaders.
The PG connection handler now runs each query on adapter.cursor() (C1). The mock adapters in TestServerConnectionAdapterIntegration only implemented execute(); give them a cursor() that delegates to self so the handler's DML, system-query, and pg_namespace paths resolve.
# Conflicts: # sidemantic/api_server.py
# Conflicts: # webapp/src/components/LeaderboardPanel.tsx # webapp/src/lib/format.test.ts # webapp/src/state/explorerState.test.ts # webapp/src/state/url.test.ts # webapp/src/state/url.ts
Add model-level security: access gates plus row-level filters, enforced once per compile before SQL assembly. - New sidemantic/core/security.py: SecurityPolicy(access, row_filters), render_row_filter (StrictUndefined + SQL-quote escaping so a hostile attribute value stays a single quoted literal), evaluate_access. - Model.security field; SecurityError/UnsupportedMetricError in semantic_layer; thread user_attributes through query()/compile()/ generate()/generate_view(). - SQLGenerator._enforce_security runs over the full participating-model set (base + joined + intermediate, via _participating_models). Access gate + deny-by-default (policy present, user_attributes is None) + row filters rendered, parsed via memoized _parse_fragment, qualified to the model and pushed into that model's own CTE so rows are scoped before joins/aggregation. user_attributes is part of the generate cache key. - Disable pre-aggregation routing when a participating model has active row filters; route secured-model queries to the Python generator. - enforce_visibility flag: reject non-public field requests during compile and omit non-public fields from catalog/introspection listings. - Regenerate JSON schema for the new field. - Tests in tests/core/test_security_enforcement.py incl. DuckDB execution proving scoped queries return fewer rows.
C2 added result_cache_mb/result_cache_ttl to start_api_server; the fake_start_api_server mock in test_api_serve_calls_start_server needed the matching optional params.
Add a compact timezone control to the explorer toolbar and thread the
selected IANA zone end-to-end:
- Reducer: new timezone state (default UTC) + setTimezone action.
- URL: backward-compatible tz param; missing => UTC, omitted at default.
- HTTP: optional timezone field on the api_server structured-query request
model, passed into layer.compile; sent from the webapp query payload
(elided when UTC) for both explorer and pivot queries.
- Labels: bucket axis ticks + tooltip render in-zone via Intl.DateTimeFormat
(no date library), including DST transitions.
- TimezoneSelect: curated common zones + typeahead over
Intl.supportedValuesOf('timeZone').
Date-range window bounds remain UTC day boundaries (documented in
timeFilters); full in-zone boundary reinterpretation deferred.
HTTP (api_server.py): resolve per-request user attributes from a trusted JSON header (default X-Sidemantic-User) and thread them into the compile/query path for /compile, /query, /sql, /raw plus the result-cache key so cached results never leak across users. Add create_app / start_api_server params require_user_attrs, enforce_visibility, user_header and matching api-serve CLI flags. Missing/malformed header with require_user_attrs -> 400; SecurityError from a secured model -> 403. PG (server/connection.py): map the connecting Postgres username to a user_attributes dict via a startup --user-attrs-file mapping (riffq does not cleanly expose per-session startup params). Enforce coarse-grained access gates on the SQL-first path; row-level filtering over the wire path is a documented limitation of the rewriter. MCP (mcp_server.py): accept optional static user_attributes at initialize_layer and thread into run_query/create_chart compile calls (no per-session identity here; defaults to None). Add tests under tests/server and tests/test_mcp_security.py.
# Conflicts: # sidemantic/api_server.py
…antics
A measure with non_additive_dimension is now computed correctly instead of
raising: within the owning model's CTE, rows are restricted to the last (or
first) value of the time dimension per group via a QUALIFY clause, so an
account-balance snapshot is summed across accounts but not double-counted
across days.
- generator: _plan_semi_additive plans the rewrite and raises only for
unimplemented cases (non-QUALIFY dialects; the semi-additive + fan-out
symmetric-aggregate combination, which cannot compose safely). _build_model_cte
emits QUALIFY <time> = MAX/MIN(<time>) OVER (PARTITION BY <other grouping dims>).
Grouping by the non-additive dim itself is a no-op (each bucket is additive).
- metric: add non_additive_window ("min"|"max", default "max"); regenerate schema.
- metricflow adapter: import/export window_choice.
- allow_non_additive_unsafe now means "skip the rewrite, aggregate naively".
- tests: value-level DuckDB correctness (semi 253 vs naive 413; by-region
220/33; min window; grouped-by-time additive), escape hatch reverts to naive,
non-QUALIFY dialect raises, semi-additive + symmetric-aggregate raises.
Map Cube access_policy row_level.filters (member/operator/values) to an enforced SecurityPolicy on the model, warning only for constructs that can't translate (roles, conditions, member_level). Map Rill metrics-view security: blocks (access + row_filter), translating Go-template .user.* refs to the user.* Jinja namespace. Add Dimension.uri (metadata-only rendering hint).
Cube access_policy and Rill security: blocks now import as enforced SecurityPolicy, so fixtures carrying them trip deny-by-default when the parse/graph/execution contract tests run queries without user attributes. These tests validate query structure, not access control (which is covered by tests/adapters/test_security_import.py and tests/core/test_security_enforcement.py), so strip security from parsed graphs before exercising them.
P0-1: row filters rendered user attributes as raw text, so an unquoted template
(tenant_id = {{ user.x }}) let a string value inject SQL. Render every attribute
as a type-correct SQL literal (strings always quoted+escaped) and strip author
quotes hugging a placeholder, closing injection in both template forms.
P0-2: semi-additive planning ignored granularity, so grouping by a coarser grain
of the non-additive dimension (day__month) dropped the QUALIFY and summed every
snapshot. Keep the QUALIFY for coarser grains and partition by the truncated bucket.
P0-3: /sql and /raw HTTP endpoints could not apply row filters yet still executed;
they now return 403 whenever any model declares a security policy.
P1-1: enforce_visibility now also rejects non-public fields referenced only in
filters/order_by/segments (previously an information-disclosure oracle).
Adds regression tests and a docs/security.md page; updates Cube/native-format docs
to reflect enforced access_policy import and the new fields.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea6dd0e9f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if getattr(self, "user_attrs_map", None): | ||
| self._enforce_pg_access(rendered_sql, user_attributes) |
There was a problem hiding this comment.
When sidemantic serve is started without --user-attrs-file, this condition is false, so _enforce_pg_access() is skipped entirely and SQL against a model with security runs directly via cursor.execute(rendered_sql). That bypasses the deny-by-default behavior used by SemanticLayer.compile() and lets PG clients read secured models with no user attributes; the check should run whenever the query touches a secured model, not only when a map is configured.
Useful? React with 👍 / 👎.
| # NOTE: QueryRewriter does not accept user_attributes, so this SQL-first path | ||
| # does not bake in per-user row filters. Security enforcement via user | ||
| # attributes applies to the structured run_query/create_chart tools. |
There was a problem hiding this comment.
For MCP clients, run_query now passes user_attributes, but this SQL-first tool still rewrites and executes semantic SQL without access checks or row filters. If a secured model is loaded, an MCP client can call run_sql("SELECT ... FROM orders") and retrieve unscoped rows even though the structured tool would deny or filter the same model; mirror the HTTP /sql behavior by refusing this tool when any model declares security, or route it through a security-aware compile path.
Useful? React with 👍 / 👎.
| "dimensions": [d.name for d in model.dimensions], | ||
| "metrics": [m.name for m in model.metrics], |
There was a problem hiding this comment.
With create_app(..., enforce_visibility=True), /query rejects hidden fields and /describe uses the filtered layer metadata, but /models still serializes every dimension and metric. Any authenticated API client can enumerate public=False field names through this catalog endpoint despite visibility enforcement; filter these lists when current_layer.enforce_visibility is set (and apply the same treatment to /graph).
Useful? React with 👍 / 👎.
| # Rendering hint (metadata only; no SQL generation impact) | ||
| uri: bool = Field(False, description="Whether this dimension's values should be rendered as links in UIs") |
There was a problem hiding this comment.
uri in native dimension YAML
Adding Dimension.uri makes the generated schema and docs advertise uri: true, but the native adapter still rejects it because DIMENSION_FIELDS in sidemantic/adapters/sidemantic.py does not include uri before calling reject_unknown_fields(). CLI-first users following the documented native format will get an unknown-field error instead of loading link dimensions; add uri to the allow-list and parse/export it.
Useful? React with 👍 / 👎.
| non_additive_window: Literal["min", "max"] = Field( | ||
| "max", |
There was a problem hiding this comment.
non_additive_window in native models
The new non_additive_window value is documented and included in the schema, but the native adapter's metric allow-list/export path only handles non_additive_dimension. A native YAML metric with non_additive_window: min is rejected as an unknown field, and any in-memory metric using min would round-trip back to the default max, changing first-snapshot metrics into last-snapshot metrics.
Useful? React with 👍 / 👎.
- PG wire server: enforce the access gate (deny-by-default) for secured models even when no user-attrs map is configured, not only when one is. - MCP run_sql: refuse when any model declares security (mirrors HTTP /sql), since the SQL-first path cannot apply row filters. - HTTP /models and /graph: omit public=false fields when enforce_visibility is on, so the catalog endpoints cannot enumerate hidden field names. - Native adapter: accept and round-trip Dimension.uri and Metric.non_additive_window (were rejected as unknown fields / lost on export).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c05a645cf6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const instant = new Date(normalizeBucketLabel(label)); | ||
| if (Number.isNaN(instant.getTime())) return label; | ||
| const parts = bucketFormatter(timeZone, grain).formatToParts(instant); |
There was a problem hiding this comment.
When a non-UTC timezone is selected, the backend already truncates to that timezone and returns local wall-clock bucket labels (for example the New York day bucket is returned as 2024-01-01). This code parses that label as a UTC instant and formats it again in the selected timezone, so negative-offset zones display the previous date (2024-01-01 becomes 2023-12-31 in America/New_York) and chart labels/tooltips become misleading. Treat timezone-bucketed labels as local strings, or have the backend return real UTC instants before applying Intl here.
Useful? React with 👍 / 👎.
| if non_additive and isinstance(non_additive, dict): | ||
| non_additive_dimension = non_additive.get("name") | ||
| window_choice = non_additive.get("window_choice") | ||
| if window_choice in ("min", "max"): | ||
| non_additive_window = window_choice |
There was a problem hiding this comment.
MetricFlow measures can specify non_additive_dimension.window_groupings (the checked-in accounts fixture has current_account_balance_by_user grouped by user), but this parser keeps only the dimension name/window choice and drops those groupings. The new semi-additive generator then partitions only by the query's selected dimensions, so totals or coarser groupings use a global max/min snapshot instead of the required per-user window and silently misstate balances. Preserve the groupings in the model, or reject these measures instead of importing them as executable semi-additive metrics.
Useful? React with 👍 / 👎.
| window = getattr(metric, "non_additive_window", "max") or "max" | ||
| plan[owning_model] = (metric.non_additive_dimension, window) |
There was a problem hiding this comment.
If a query asks for two semi-additive metrics from the same model that need different windows or time dimensions (for example opening balance with min and closing balance with max), this assignment overwrites the first metric's plan with the last one. Only one QUALIFY is then applied to the model CTE, so one of the metrics is aggregated over the wrong snapshot. Detect conflicting (non_additive_dimension, window) requirements or build separate CTEs instead of silently reusing one plan.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| def get_catalog_metadata(graph: SemanticGraph, schema: str = "public") -> dict: | ||
| def get_catalog_metadata(graph: SemanticGraph, schema: str = "public", enforce_visibility: bool = False) -> dict: |
There was a problem hiding this comment.
The helper now accepts enforce_visibility, but the public SemanticLayer.get_catalog_metadata() path still calls it without passing self.enforce_visibility, and MCP's semantic://catalog resource uses that method. As a result, a layer with enforce_visibility=True still exposes public=False dimensions and metrics through catalog metadata even though /describe, /models, and /graph hide them. Thread the layer flag through this catalog path as well.
Useful? React with 👍 / 👎.
…els, catalog visibility - MetricFlow window_groupings: preserve on Metric (non_additive_window_groupings) and partition the semi-additive QUALIFY by them regardless of query grouping, so balance-per-user totals aren't taken over a global snapshot. Native adapter round-trips the field. - Reject conflicting semi-additive plans: two metrics on one model needing different (dimension, window) snapshots now raise instead of silently applying only the last plan. - webapp timezone labels: the backend already returns local wall-clock bucket labels, so formatBucketLabel no longer re-zones them through Intl (that double-shifted negative-offset dates to the previous day). - get_catalog_metadata now threads enforce_visibility, so the MCP semantic://catalog resource also hides non-public fields. - Make an order-sensitive visibility test order-independent (CI flake).
…a-e8f7c3 # Conflicts: # webapp/src/state/explorerState.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53b142e71b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| security_forces_python = user_attributes is not None or self._query_touches_secured_model( | ||
| metrics, dimensions, filters |
There was a problem hiding this comment.
This guard only applies to the structured compile()/query() path. The SQL-first path used by SemanticLayer.sql() and the CLI sidemantic query command still rewrites with QueryRewriter and executes without user_attributes or a secured-model denial, so a CLI user can run SELECT ... FROM secured_model and get unfiltered rows even though the structured API would deny or apply row filters. Gate sql()/CLI when model security exists, or make the rewriter security-aware.
Useful? React with 👍 / 👎.
| def execute(self, sql: str) -> Any: | ||
| """Execute SQL under the shared-connection lock and return the result.""" | ||
| with self._lock: | ||
| return self._adapter.execute(sql) |
There was a problem hiding this comment.
For adapters that use this fallback cursor, the lock is released as soon as adapter.execute() returns, but the returned result is fetched later by callers such as HTTP _execute_to_table() and the PG wire handler. Non-DuckDB adapters like PostgreSQLAdapter do not override cursor(), so concurrent requests can fetch from results backed by the same shared connection outside the lock, which does not preserve the old serialized single-connection behavior and can interleave/corrupt driver state. Hold the lock through result materialization or return a result wrapper that locks during fetch.
Useful? React with 👍 / 👎.
| if (present.length === 1) { | ||
| parts.push(`${dimRef} ${negate ? "!=" : "="} ${filterLiteral(present[0], type)}`); | ||
| } else if (present.length > 1) { | ||
| const list = present.map((v) => filterLiteral(v, type)).join(", "); | ||
| parts.push(`${dimRef} ${negate ? "NOT IN" : "IN"} (${list})`); |
There was a problem hiding this comment.
When an exclude filter contains only non-null values, this emits dim != value or dim NOT IN (...); SQL evaluates those predicates to UNKNOWN for NULL rows, so NULLs are dropped even though the user did not select the null token for exclusion. For a nullable dimension like country, excluding US should keep unknown countries; add an OR dim IS NULL branch for exclude filters unless NULL_TOKEN is explicitly selected.
Useful? React with 👍 / 👎.
| if getattr(measure, "non_additive_window", "max") == "min": | ||
| non_additive_def["window_choice"] = "min" | ||
| measure_def["non_additive_dimension"] = non_additive_def |
There was a problem hiding this comment.
Fresh evidence after the prior parser fix: the current export path still writes only the non-additive dimension name and optional window_choice, dropping non_additive_window_groupings that were parsed onto the metric and used by the generator. A MetricFlow measure such as balance grouped by user round-trips to a model with no window_groupings, so a later import computes the snapshot globally at coarser grains and changes results; include window_groupings when the metric has them.
Useful? React with 👍 / 👎.
…locking, NULL exclude, MF export - SemanticLayer.sql() (SQL-first path, used by CLI 'query') now refuses when any model declares a security policy; it cannot apply row filters, so it would otherwise return unfiltered rows. Mirrors the HTTP /sql and MCP run_sql gates. - Fallback _SerializedCursor now materializes the full result to an Arrow table while the shared-connection lock is held, so non-DuckDB adapters can't interleave a later fetch on the shared connection (which could corrupt driver state). Downstream reads from memory. - Exclude filters preserve NULL rows: 'dim != v' / 'NOT IN (...)' is UNKNOWN for NULLs and dropped them; now emit 'OR dim IS NULL' unless the null token is explicitly excluded. - MetricFlow export writes non_additive_window_groupings so balance-per-user round-trips. Tests added for each; existing exclude-mode tests updated to the NULL-preserving form.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d99af6c48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if isinstance(value, bool): | ||
| return _SqlLiteral("TRUE" if value else "FALSE") |
There was a problem hiding this comment.
When a row filter uses a Jinja conditional such as {% if user.is_admin %}, false boolean attributes have already been wrapped in _SqlLiteral("FALSE"); because _SqlLiteral does not define __bool__, Jinja treats that object as truthy. In policies that emit TRUE or skip the tenant predicate for admins, non-admin users with is_admin: false render the admin branch and bypass row filtering. Keep raw values available for control flow, or make the literal wrapper preserve the underlying value's truthiness while still rendering as a SQL literal.
Useful? React with 👍 / 👎.
| groupings = non_additive.get("window_groupings") | ||
| if groupings: | ||
| non_additive_window_groupings = list(groupings) |
There was a problem hiding this comment.
Fresh evidence after the prior window_groupings preservation: MetricFlow groupings can name entities, not dimensions (user in the checked-in accounts fixture is backed by the user_id foreign entity), but this stores the raw name. The semi-additive CTE only projects matching dimensions and then partitions by that alias, so such imports produce PARTITION BY user without a projected user column, or partition on the wrong field if a dimension happens to share the name. Map entity groupings to their expr/foreign key or reject them during import.
Useful? React with 👍 / 👎.
…entity groupings
- Row-filter rendering no longer wraps attribute values (which made a false boolean look
truthy to Jinja, so '{% if user.is_admin %}' bypassed filtering for non-admins). Instead
a dedicated environment uses a finalize callback that quotes each {{ }} OUTPUT into a SQL
literal while {% if %}/comparisons see raw values -- fixing truthiness and enabling
equality checks, with injection still neutralized on output.
- MetricFlow import resolves semi-additive window_groupings that name an entity to the
entity's backing column (e.g. user -> user_id), so the generator partitions by a real
projectable column instead of a non-existent one.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d4b894335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self._enforce_pg_access(rendered_sql, user_attributes) | ||
|
|
||
| # Execute the query | ||
| result = self.layer.adapter.execute(rendered_sql) | ||
| result = cursor.execute(rendered_sql) |
There was a problem hiding this comment.
When the PG wire server is started with a user-attrs map and a secured model has row_filters, this path only calls _enforce_pg_access() (which evaluates the access gate) and then executes the rewritten SQL unchanged. In that context a user whose access expression passes can SELECT the secured model and receive all rows, because the row-filter predicates are never injected; either apply the filters here or deny PG SQL for models with row filters as the other SQL-first paths do.
Useful? React with 👍 / 👎.
| for reference in metrics: | ||
| metric = self._resolve_metric_obj(reference) | ||
| if metric is not None and getattr(metric, "non_additive_dimension", None): | ||
| semi_additive.append((reference, metric)) |
There was a problem hiding this comment.
When the requested metric is a graph-level/simple metric that wraps a model measure with non_additive_dimension (the shape MetricFlow imports commonly produce), this loop inspects only the outer metric object, finds no non_additive_dimension, and leaves semi_additive_plan empty. The generated model CTE then has no QUALIFY, so balances or snapshots behind the graph metric are summed across every snapshot instead of first/last per group; traverse metric dependencies and plan for any dependent non-additive measures.
Useful? React with 👍 / 👎.
| if (debouncedSearch.trim()) { | ||
| base.push(`${dim.ref} ILIKE ${sqlLiteral(`%${likeEscape(debouncedSearch.trim())}%`)} ESCAPE '\\'`); |
There was a problem hiding this comment.
When the filter editor is opened for a numeric or boolean dimension and the user types in the value search box, this emits a distinct-values query with dim ILIKE '%...%'. DuckDB and PostgreSQL do not accept ILIKE on INTEGER/BOOLEAN operands, so the value list turns into a backend error for those dimensions; cast the dimension to text for search or restrict this search predicate to textual dimensions.
Useful? React with 👍 / 👎.
…dditive, text-cast search - PG wire server denies a touched secured model that declares row_filters (the SQL-first path evaluates the access gate but cannot inject row filters, so it would return all rows to any user passing the gate). Mirrors /sql, MCP run_sql, and .sql(). - Semi-additive planning traverses metric dependencies, so a graph-level/derived metric that wraps a model measure with non_additive_dimension still emits the QUALIFY instead of summing every snapshot. - Filter-editor value search casts the dimension to VARCHAR before ILIKE, so searching a numeric/boolean dimension no longer errors on DuckDB/Postgres.
There was a problem hiding this comment.
💡 Codex Review
sidemantic/sidemantic/sql/generator.py
Line 447 in f3a4299
Do not timezone-shift DATE buckets
When a time dimension is backed by a DATE/local-day column and the UI sends a non-UTC timezone, this path applies AT TIME ZONE before DATE_TRUNC; in DuckDB, DATE '2024-01-01' with America/New_York becomes 2023-12-31 19:00, so daily/monthly buckets move to the previous day/month. Only localize timestamp expressions, or leave DATE dimensions unchanged.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Strip a leading Cube self-reference (``{CUBE}.col`` / ``{cube_name}.col``) and any prefix. | ||
| col = re.sub(r"^\{[^}]*\}\.", "", member or "") | ||
| col = col.split(".")[-1] if "." in col else col | ||
| vals = values or [] |
There was a problem hiding this comment.
When a Cube access_policy uses dynamic values such as values: security_context.auth.userAttributes.allowedCities (present in tests/fixtures/cube/rbac_customers.yaml), YAML gives this helper a string, but this line keeps it as an iterable. The generated enforced filter becomes city IN ('s', 'e', 'c', ...) instead of being preserved/unmapped or templated, so imported Cube RBAC models return incorrectly scoped rows, often none. Guard for list literals before translating.
Useful? React with 👍 / 👎.
| continue | ||
| # Multiple filters within one policy combine per filters_type (default AND). | ||
| joiner = " OR " if combine == "or" else " AND " | ||
| row_filters.append(joiner.join(fragments) if len(fragments) > 1 else fragments[0]) |
There was a problem hiding this comment.
When filters_type: or creates a row filter like A OR B and the query also has another filter or row policy, _build_model_cte() later concatenates filter strings with AND. Without wrapping the OR group here, SQL precedence turns (A OR B) AND C into A OR (B AND C), so rows matching A bypass the additional predicate. Wrap multi-fragment OR policies before appending.
Useful? React with 👍 / 👎.
| if (parts.length === 1) out.push(parts[0]); | ||
| else if (parts.length > 1) out.push(`(${parts.join(" OR ")})`); | ||
| if (filter.mode === "contains") { | ||
| out.push(`${dimRef} ILIKE ${sqlLiteral(`%${likeEscape(filter.pattern ?? "")}%`)} ESCAPE '\\'`); |
There was a problem hiding this comment.
For contains-mode filters on numeric or boolean dimensions, this still emits orders.id ILIKE '%12%' even though type is available above; DuckDB/Postgres reject ILIKE on INTEGER/BOOLEAN operands, so applying Contains to those dimensions turns every dashboard query into a backend error. Mirror the distinct-value search path by casting the dimension to text, or limit contains mode to text dimensions.
Useful? React with 👍 / 👎.
| for raw in [*(filters or []), *(order_by or []), *(segments or [])]: | ||
| for model_name, field_name in ref_pattern.findall(raw): | ||
| field_name = field_name.rsplit("__", 1)[0] if "__" in field_name else field_name | ||
| if not self._field_is_public(model_name, field_name): | ||
| raise SecurityError(f"Field '{model_name}.{field_name}' is not public") |
There was a problem hiding this comment.
When enforce_visibility=True and a request passes segments: ['orders.internal'], this loop scans it as a generic model.field token and _field_is_public() treats unknown fields as public because it only checks dimensions/metrics. _resolve_segments() later resolves and applies Segment(public=False), so hidden segments remain invokable through /query and /compile; explicitly look up model.get_segment() and reject non-public segments.
Useful? React with 👍 / 👎.
…nt visibility, contains cast - Cube access_policy: OR-combined row filters are parenthesized so later AND-ing keeps precedence (A OR B AND C previously let A-matching rows bypass the other predicates). - Cube access_policy: dynamic (non-list) values references (e.g. security_context.*) are treated as unmapped instead of iterated character by character into a bogus IN list. - enforce_visibility now rejects a public=false segment referenced via segments=[...] (segments are named filters, not dimensions/metrics, so they needed an explicit check). - Webapp contains-mode filters cast the dimension to VARCHAR before ILIKE, matching the value-search path, so Contains on numeric/boolean dimensions no longer errors. - Document that query-timezone localization assumes TIMESTAMP-backed time dimensions; it cannot detect DATE columns at compile time and must not regress the tested timestamp day-shift, so the DATE case is a documented limitation rather than a silent grain gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59848a942c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.username is None and self.password is None: | ||
| # No auth required | ||
| callback(True) |
There was a problem hiding this comment.
When --user-attrs-file is configured but --username/--password are not, this branch still accepts any startup username and stores it in session_user; _user_attributes() then trusts that client-supplied name to select the security attributes. In a PG server exposed beyond a fully trusted localhost, a client can connect as any key in the attrs file (for example an admin user) and satisfy access gates, so require authentication or refuse user-attrs maps without auth.
Useful? React with 👍 / 👎.
| @@ -63,8 +108,17 @@ def _handle_query(self, sql, callback, **kwargs): | |||
| # Use non-strict mode to pass through system queries (SHOW, SET, etc.) | |||
| rendered_sql = rewriter.rewrite(sql, strict=False) | |||
There was a problem hiding this comment.
With a valid user-attrs map and an access-only policy, authorized PG clients are still denied because QueryRewriter.rewrite() calls the SQL generator without user_attributes, so the generator's deny-by-default security check can raise before _enforce_pg_access(rendered_sql, user_attributes) gets a chance to evaluate the mapped user. This affects sidemantic serve queries against secured models that have no row filters; thread the attrs into the rewrite/compile step or perform the coarse access decision before invoking the rewriter.
Useful? React with 👍 / 👎.
| security_forces_python = user_attributes is not None or self._query_touches_secured_model( | ||
| metrics, dimensions, filters |
There was a problem hiding this comment.
When a secured model is referenced only through segments and the Rust generator is enabled, this check still sees only metrics/dimensions/filters, so security_forces_python can be false with user_attributes=None. The code then enters the Rust path described above as not enforcing security, letting a segment-only secured model bypass deny-by-default/access checks; resolve segments or include them in _query_touches_secured_model() before allowing Rust.
Useful? React with 👍 / 👎.
| : null, | ||
| [wantsDelta, comparisonRange, timeRef, state.filters, dim.ref, types], | ||
| ); | ||
| const prev = useQueryResult(backend, prevFilters ? dimensionLeaderboard(rankMetric.ref, dim.ref, prevFilters, limit) : null); |
There was a problem hiding this comment.
When the delta column is enabled, this previous-period query is also limited/sorted as the previous period's top N. Any current top-N value that was not also in the previous top N is absent from prevByValue, so the UI renders — instead of the real prior value/delta; fetch previous values for the current row set (or remove/increase this limit) rather than ranking the prior period independently.
Useful? React with 👍 / 👎.
| for column in parsed.find_all(exp.Column): | ||
| if not column.table: | ||
| column.set("table", model_name) |
There was a problem hiding this comment.
Security row filters that contain a subquery with its own unqualified columns (for example id IN (SELECT id FROM allowed_ids)) will have those inner columns rewritten to model.id, producing invalid or wrong SQL. Segment qualification already avoids walking into subqueries; apply the same scoping here so only outer filter columns are qualified.
Useful? React with 👍 / 👎.
…ement, subquery filters, delta rows - PG server refuses to start with a user-attrs map but no --username/--password auth: the attrs are keyed off the client-supplied startup username, which would otherwise let a client spoof any user (e.g. admin) to satisfy access gates. - QueryRewriter.rewrite accepts user_attributes and threads them into generation, so the SQL-first PG path evaluates the access gate against the caller instead of always hitting deny-by-default; the PG handler now runs the coarse access check (incl. row-filter denial) before rewriting and passes the attributes through. Per-user rewrites bypass the cache. - enforce_visibility / Rust-bypass security checks now include segment references, so a model referenced only through segments is still enforced (no Rust bypass, deny-by-default holds). - Security row filters no longer descend into subqueries when qualifying columns, so 'id IN (SELECT id FROM allowed)' keeps the inner id bound to the subquery. - Leaderboard delta column constrains the comparison-period query to the current row set's dimension values, so a current row missing from the prior top-N shows its real prior value.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Adds enforced model security, correct semi-additive measures, concurrent/cached serving, and several explore-UI features. Grouped into five themes.
Security (new, enforced)
Previously
publicvisibility flags and imported access policies were stored but never enforced. This adds real enforcement at query-compile time.SecurityPolicyonModel(accessgate +row_filters). Row filters are injected into each model's own CTE so rows are scoped before joins/aggregation (fan-out-safe). Attribute values render as type-correct SQL literals (strings always quoted+escaped), so a row filter cannot be injected regardless of template form.user_attributesraises rather than running unscoped. Passuser_attributes={}for an authenticated user with no attributes.SemanticLayer(enforce_visibility=True)rejectspublic=falsefields whether projected, filtered on, or ordered by (so a hidden field can't be used as an oracle).X-Sidemantic-User, JSON); flags--require-user-attrs,--enforce-visibility,--user-header.SecurityErrormaps to 403 and the result cache is keyed per user. The free-form/sqland/rawendpoints return 403 when any model is secured (they cannot apply row filters).docs/security.md).SecurityPolicyrow filters where they translate mechanically; untranslatable constructs are preserved inmetawith a warning.See the new
docs/security.mdfor the full model.Correctness: semi-additive measures
Metric.non_additive_dimensionwas accepted but ignored by SQL generation, silently over-aggregating (e.g. summing every daily balance snapshot). It now computes correct last/first-value semantics with aQUALIFYon capable engines (DuckDB, Snowflake, BigQuery, Databricks, Spark, ClickHouse), including at coarse time grains (month-end-balance style queries partition by the truncated bucket). Unsupported dialects and fan-out combinations raise a clear error instead of returning wrong numbers. Addsnon_additive_window(min/max).Performance
import sidemanticdrops from ~1170ms to ~290ms, andsidemantic --helpcold start improves accordingly.Web UI (
webapp/)All new UI state is URL-serialized and backward-compatible with existing links.
Ergonomics
Dimension.urirendering hint (metadata only; UIs may render values as links).Validation
tsc --noEmitandvite buildare clean.Known limitations
run_sqlenforce access gates but not row-level filters (the SQL-string paths do not thread user attributes).