feat(api-gateway): Expose usedPreAggregations on data responses - #11591
feat(api-gateway): Expose usedPreAggregations on data responses#11591MazterQyou wants to merge 1 commit into
usedPreAggregations on data responses#11591Conversation
Signed-off-by: Alex Qyoun-ae <4062971+MazterQyou@users.noreply.github.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
github-actions
Bot
added
client:core
|
Claude finished @MazterQyou's task in 5m 9s —— View job Review: solid, well-documented change — 1 design question (public
|
| # | Severity | Where | Issue |
|---|---|---|---|
| 1 | Design question | gateway.ts:147 |
targetTableName (schema name + build version hashes) is now returned to every consumer, including public embedded tokens. preAggregationId + lastUpdatedAt alone satisfy the stated use case; consider keeping the table name dev/playground-only, or gating the projection behind an env flag so operators can restore the old behavior. |
| 2 | Low | gateway.ts:1989 |
The dev/playground spread re-assigns response.usedPreAggregations verbatim, so dev responses carry usedPreAggregations: {} for queries that hit no pre-aggregation while production omits the key. Not a regression, but now that the key is public contract, 'usedPreAggregations' in res differs across modes. Rust already normalizes this. |
| 3 | Low | node_export.rs:440-449 |
Schema-metadata fallback doesn't apply the empty/null normalization build_response_schema does (from_str("null") → Some(Value::Null)), and .ok() swallows parse errors where the equivalent parse in load_data logs a warn!. Defensive only today, since build_response_schema is the sole writer. |
| 4 | Low (docs) | rest-api/reference.mdx:45 |
rollupJoin/rollupLambda expand to their referenced rollups in preAggregationDescriptionsFor, so clients never see the lambda/join pre-aggregation itself in usedPreAggregations — worth documenting. Also, the adjacent external bullet claims "present only when the query hit a pre-aggregation", but /load always emits it (this PR's own test asserts false). |
Non-blocking observations
PreAggregationTypeis now exported fromPreAggregations.tsas'rollup' \| 'originalSql', whilecubejs-client-core's same-named type is'rollup' \| 'rollupJoin' \| 'rollupLambda' \| 'originalSql'. The narrow one looks correct at runtime (join/lambda descriptions expand to their referenced rollups), so the client-core union is the stale one — but two exported types with the same name and different unions across packages is a trap for the next reader. Worth a comment on one of them.- The
request.streamingpushdown branch ofsqlApiLoaddoesn't attachusedPreAggregations, same aslastRefreshTime. Thenode_export.rscomment documents the gap honestly; the docs say the header "optionally" carries it, which technically covers it, but a reader may expect it in stream mode. - No test exercises the GraphQL
res.extensions.usedPreAggregationspath (which passes through the unredacted object in dev mode). Low value, just noting it's uncovered.
Verification
I read the diff and traced the data path across all three layers but did not execute the test suites — running the api-gateway/native tests needs a compiled cubejs-backend-native binary, and the cd && npx jest invocation was blocked by the sandbox permission prompt, so cargo test / yarn test results here are unverified by me. Everything above is from static reading.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| * data the caller cannot otherwise reach. The full object, including them, is | ||
| * still returned in dev mode and to the Playground. | ||
| */ | ||
| function publicUsedPreAggregations( |
There was a problem hiding this comment.
Design/security posture — worth an explicit decision from reviewers.
The docblock explains well why refreshKeyValues stays out, but the flip side isn't argued: targetTableName is now returned to every consumer, and it embeds the pre-aggregation schema name plus the content/structure version hashes of a specific build (e.g. stb_pre_aggs.foo_foo_main20240101_abc_def_1712). For a public embedded-analytics deployment that's internal storage layout leaking to untrusted browsers, and it's exactly what the test this PR rewrote was guarding.
Two things that would de-risk the reversal:
preAggregationId+lastUpdatedAtalone already satisfy the stated use case ("match a result to the build behind it" / "join a chart to the build it is watching").targetTableNameis only needed if the client wants the physical table, which a data-API consumer can't use anyway. Consider dropping it from the public whitelist and keeping it dev/playground-only.- If it must stay, consider gating the whole public projection behind an env flag (defaulting to on or off, your call) so operators who relied on the previous behavior can restore it without pinning a version.
Either way, the security-relevant part of this change (what ordinary consumers can now see) deserves a line in the docs, not just the usedPreAggregations field description.
| // Identity of the pre-aggregations behind this result, so a client can | ||
| // join it to the build it is waiting on. The dev-mode block below | ||
| // replaces it with the unredacted object. | ||
| usedPreAggregations: publicUsedPreAggregations(response.usedPreAggregations), |
There was a problem hiding this comment.
Minor consistency gap: publicUsedPreAggregations deliberately collapses {} to undefined ("a query that hit no pre-aggregation reports nothing rather than an empty object"), but the dev/playground spread just below re-assigns response.usedPreAggregations verbatim. Since the orchestrator builds that object with R.fromPairs over the used-tables list, it's {} for every query that hit no pre-aggregation — so in dev mode the response carries usedPreAggregations: {} while in production the key is absent.
Not a regression (dev mode did this before too), but now that the key is part of the public contract, a client written against 'usedPreAggregations' in res behaves differently in dev vs prod. Cheap to align:
usedPreAggregations: Object.keys(response.usedPreAggregations || {}).length
? response.usedPreAggregations
: undefined,The Rust side already has the same normalization (is_empty_used_pre_aggregations), so this would make all three layers agree.
| let used_pre_aggregations = span_used_pre_aggregations.or_else(|| { | ||
| stream | ||
| .schema() | ||
| .metadata() | ||
| .get("usedPreAggregations") | ||
| .and_then(|v| serde_json::from_str(v).ok()) | ||
| }); | ||
| if let Some(used_pre_aggregations) = used_pre_aggregations { | ||
| schema_response.insert("usedPreAggregations".into(), used_pre_aggregations); | ||
| } |
There was a problem hiding this comment.
Two small robustness gaps in the schema-metadata fallback:
-
It doesn't apply the empty/null normalization that
build_response_schemadoes.serde_json::from_str("null")→Some(Value::Null)and"{}"→Some({}), either of which would put"usedPreAggregations": null/{}on the JSONL header — the exact thingis_empty_used_pre_aggregationswas added to prevent. Today onlybuild_response_schemawrites this key and it pre-filters, so this is defensive, but the invariant lives in a different crate from the reader. Exportingis_empty_used_pre_aggregations(or aparse_used_pre_aggregations(&str) -> Option<Value>helper next to it inscan.rs) and calling it here would keep the normalization in one place. -
.ok()drops parse errors silently, while the same parse inload_datalogswarn!("Unable to parse usedPreAggregations of a load response: {}", e). A malformed blob here would just make the field vanish with no trace — worth a matchingwarn!.
| - `external` - A boolean indicating whether the query was served from a | ||
| pre-aggregation in Cube Store. Present only when the query hit a pre-aggregation. | ||
| - `usedPreAggregations` - The pre-aggregations this result was served from, keyed | ||
| by pre-aggregation table name. Each entry carries `preAggregationId` (the name in |
There was a problem hiding this comment.
Two doc notes while this section is being touched:
-
For a
rollupJoin/rollupLambdapre-aggregation,preAggregationDescriptionsForin the schema compiler expands to the referenced rollups, so the entries a client sees are the underlyingrollups (their ids andtype: "rollup"), never the lambda/join pre-aggregation itself. Worth one sentence — otherwise a user who definedOrders.lambdaand looks for it inusedPreAggregationswill conclude it's a bug. -
The
externalbullet two lines above claims "Present only when the query hit a pre-aggregation", but/loadalways emits it (the test in this PR assertsexternal === falsefor a non-pre-agg query). Only the SQL API JSONL header omits it when false. Pre-existing inaccuracy, but adjacent and cheap to fix in the same pass.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11591 +/- ##
==========================================
- Coverage 84.06% 79.54% -4.52%
==========================================
Files 257 484 +227
Lines 81582 99864 +18282
Branches 0 3673 +3673
==========================================
+ Hits 68581 79441 +10860
- Misses 13001 19875 +6874
- Partials 0 548 +548
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Check List
Description of Changes Made
This PR exposes pre-aggregation identity as
usedPreAggregationson the REST, GraphQL and SQL API data responses, without the refresh key values, so clients can match a result to the pre-aggregation build behind it. Related tests are included.Two behavior changes worth noting:
queriesWithPreAggregationstelemetry counter was structurally always 0 in production because it read a dev-only field, and now counts real usage