-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(api-gateway): Expose usedPreAggregations on data responses
#11591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,6 +132,44 @@ function systemAsyncHandler(handler: (req: Request & { context: ExtendedRequestC | |
|
|
||
| const DEV_TOKEN_SCOPE = 'dev-token'; | ||
|
|
||
| /** | ||
| * Fields of `usedPreAggregations` that are safe to report to any client: the | ||
| * identity of the pre-aggregation a result was served from, so the client can | ||
| * match the result to a build it is watching. | ||
| * | ||
| * `refreshKeyValues` is deliberately left out. Those are raw rows of the | ||
| * refresh key queries - typically aggregates such as `MAX(updated_at)` or | ||
| * `COUNT(*)` - and a `refreshKey.sql` is often written without the security | ||
| * context filtering that the cube itself applies, so the values can describe | ||
| * data the caller cannot otherwise reach. The full object, including them, is | ||
| * still returned in dev mode and to the Playground. | ||
| */ | ||
| function publicUsedPreAggregations( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Design/security posture — worth an explicit decision from reviewers. The docblock explains well why Two things that would de-risk the reversal:
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: Record<string, any> | undefined | ||
| ): Record<string, any> | undefined { | ||
| // A query that hit no pre-aggregation reports nothing rather than an empty | ||
| // object, so the key is simply absent from the response. | ||
| if (!usedPreAggregations || Object.keys(usedPreAggregations).length === 0) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const publicFields = ['preAggregationId', 'targetTableName', 'lastUpdatedAt', 'type']; | ||
|
|
||
| return Object.fromEntries( | ||
| Object.entries(usedPreAggregations).map(([tableName, usage]) => [ | ||
| tableName, | ||
| // Undefined fields are dropped rather than kept: the native result | ||
| // pipeline deserializes a JS `undefined` into a JSON `null`, so leaving | ||
| // them in would put `"preAggregationId": null` on the wire. | ||
| Object.fromEntries( | ||
| publicFields | ||
| .filter((field) => usage?.[field] !== undefined) | ||
| .map((field) => [field, usage[field]]) | ||
| ), | ||
| ]) | ||
| ); | ||
| } | ||
|
|
||
| function hasDevTokenScope(securityContext: unknown): boolean { | ||
| if (typeof securityContext !== 'object' || securityContext === null) { | ||
| return false; | ||
|
|
@@ -1945,6 +1983,10 @@ class ApiGateway { | |
| const resObj = { | ||
| query: normalizedQuery, | ||
| lastRefreshTime: response.lastRefreshTime?.toISOString(), | ||
| // 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), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor consistency gap: 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: Object.keys(response.usedPreAggregations || {}).length
? response.usedPreAggregations
: undefined,The Rust side already has the same normalization ( |
||
| ...( | ||
| getEnv('devMode') || | ||
| context.signedWithPlaygroundAuthSecret | ||
|
|
@@ -2242,6 +2284,11 @@ class ApiGateway { | |
| // otherwise the SQL API reports "unknown" for every query cubesql | ||
| // hands over as pre-generated SQL. | ||
| lastRefreshTime: response.lastRefreshTime?.toISOString(), | ||
| // Same reason as `lastRefreshTime` above: the pre-aggregation | ||
| // identity has to travel with the pushed-down result too, or a | ||
| // cubesql query that goes through pre-generated SQL can never tell | ||
| // the client which pre-aggregation it read. | ||
| usedPreAggregations: publicUsedPreAggregations(response.usedPreAggregations), | ||
| // Always false: this branch builds its sqlQuery with | ||
| // `disableExternalPreAggregations` set (above), which makes | ||
| // `externalPreAggregationQuery()` return false, and the | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.