fix: AnalyticsQuerySchema cube field separation of concerns#626
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… wrapper The AnalyticsQuerySchema had cube as required, but it should be optional at the query level since the cube context can be provided externally (e.g., at the API request wrapper level). - Made cube optional in AnalyticsQuerySchema (data layer) - Added cube as required field in AnalyticsQueryRequestSchema (API layer) - Added missing AnalyticsQueryRequest type export - All 184 test files (4860 tests) pass Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the analytics protocol schema to separate “what to query” (query definition) from “which cube to target” (API request wrapper), addressing test failures caused by cube being required in the wrong layer.
Changes:
- Made
cubeoptional inAnalyticsQuerySchema(data-level query definition). - Added required top-level
cubetoAnalyticsQueryRequestSchemaand exported the missingAnalyticsQueryRequesttype. - Updated
pnpm-lock.yaml(includes removal/pruning of various workspace/importer and transitive dependency entries).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Regenerated lockfile; removes/prunes workspace importer entries and older transitive toolchain versions. |
| packages/spec/src/data/analytics.zod.ts | Makes AnalyticsQuerySchema.cube optional with updated description. |
| packages/spec/src/api/analytics.zod.ts | Requires cube at the API request wrapper level and exports AnalyticsQueryRequest type. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
pnpm-lock.yaml:335
- This lockfile diff contains substantial unrelated churn (e.g. importer removal for example workspaces and pruning of older transitive toolchain versions). If this isn’t intentional, it will increase merge conflicts and makes the PR harder to review. Consider splitting lockfile cleanup into a separate PR/commit or describing why the lockfile regeneration is required for the analytics schema change.
| * The request format for the Analytics API. | ||
| */ | ||
| export const AnalyticsQuerySchema = z.object({ | ||
| cube: z.string().describe('Target cube name'), | ||
| cube: z.string().optional().describe('Target cube name (optional when provided externally, e.g. in API request wrapper)'), |
There was a problem hiding this comment.
Making AnalyticsQuerySchema.cube optional changes the exported AnalyticsQuery type to cube?: string, but downstream code treats cube as required (e.g. analytics services use it to look up the cube). Consider introducing a separate schema/type for the API-level “query definition” (no cube) and keep the service/engine AnalyticsQuery requiring cube, or update all consumers/contracts accordingly so type-checking under strict doesn’t break.
| */ | ||
| export const AnalyticsQuerySchema = z.object({ | ||
| cube: z.string().describe('Target cube name'), | ||
| cube: z.string().optional().describe('Target cube name (optional when provided externally, e.g. in API request wrapper)'), |
There was a problem hiding this comment.
cube is a machine identifier and CubeSchema.name already enforces snake_case via regex. To keep validation consistent, consider applying the same regex to AnalyticsQuerySchema.cube (and other request-level cube fields) instead of accepting any string.
| cube: z.string().optional().describe('Target cube name (optional when provided externally, e.g. in API request wrapper)'), | |
| cube: z.string().regex(/^[a-z_][a-z0-9_]*$/).optional().describe('Target cube name (optional when provided externally, e.g. in API request wrapper)'), |
| export const AnalyticsQueryRequestSchema = z.object({ | ||
| query: AnalyticsQuerySchema.describe('The analytic query definition'), | ||
| cube: z.string().describe('Target cube name'), | ||
| format: z.enum(['json', 'csv', 'xlsx']).default('json').describe('Response format'), |
There was a problem hiding this comment.
AnalyticsQueryRequestSchema now requires a top-level cube, but the nested query schema still permits query.cube. That allows conflicting values and weakens the intended separation of concerns. Consider using a query schema that omits/disallows cube (or adding a refinement that rejects requests when both cube fields are present).
| format: z.enum(['json', 'csv', 'xlsx']).default('json').describe('Response format'), | |
| format: z.enum(['json', 'csv', 'xlsx']).default('json').describe('Response format'), | |
| }).superRefine((value, ctx) => { | |
| const queryCube = (value.query as any)?.cube; | |
| if (queryCube !== undefined) { | |
| ctx.addIssue({ | |
| code: z.ZodIssueCode.custom, | |
| path: ['query', 'cube'], | |
| message: 'query.cube is not allowed when a top-level cube is provided. Remove cube from query and use the top-level cube instead.', | |
| }); | |
| } |
| cube: z.string().describe('Target cube name'), | ||
| format: z.enum(['json', 'csv', 'xlsx']).default('json').describe('Response format'), |
There was a problem hiding this comment.
For consistency with CubeSchema.name (snake_case constraint), consider validating AnalyticsQueryRequestSchema.cube with the same regex instead of z.string() so invalid cube names are rejected at the API boundary.
AnalyticsQuerySchema.cubewas required, causing 8 test failures. The cube targeting belongs at the API request wrapper level, not inside the query definition itself.Changes
data/analytics.zod.ts: Madecubeoptional inAnalyticsQuerySchema— the query defines what to analyze (measures, dimensions, filters), not which cubeapi/analytics.zod.ts: Added requiredcubefield toAnalyticsQueryRequestSchemaand added missingAnalyticsQueryRequesttype exportAll 184 test files (4860 tests) pass.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.