feat: add view_group support to data model#10768
Conversation
Add view_group as a new first-class object in the Cube data model. View groups can be defined either as standalone view_group() objects or via the viewGroup property on individual views. - Add ViewGroupEvaluator compiler - Add view_group() global function in DataSchemaCompiler - Add viewGroup property to view schema in CubeValidator - Handle view_groups in YamlCompiler for YAML data models - Resolve and merge view groups in CubeToMetaTransformer - Include viewGroups in meta API response - Add comprehensive tests for both JS and YAML definitions Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
- Fix no-continue lint errors in CubeToMetaTransformer - Remove unused CompiledViewGroup import - Fix prefer-const lint errors in gateway - Update all metaConfig consumers to handle new object format - Fix metaTransformer.compileViewGroups() timing via DataSchemaCompiler Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
github-actions
Bot
added
the
javascript
|
Claude finished @paveltiunov's task in 4m 5s —— View job Code Review: feat: add view_group support to data model
Overall AssessmentWell-structured feature addition that follows existing patterns (mirrors Issues Found1. Breaking change in
2. Dead fallback code in gateway (low severity — inline comment on The 3. Looks like a leftover from iterating on the variable rename. See inline comment → 4. Unused
5. No validation of view names in A 6.
7.
Things Done Well
Test Coverage NotesThe test suite covers the happy paths well. Consider adding tests for:
SecurityNo security concerns — |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| const compilerApi = await this.getCompilerApi(context); | ||
| let metaConfigResult = await compilerApi.metaConfig(request.context, { | ||
| const metaConfigResponse2 = await compilerApi.metaConfig(request.context, { |
There was a problem hiding this comment.
Nit: metaConfigResponse2 looks like a leftover from iterating on the rename. Consider a more descriptive name like metaConfigResponse (same as the pattern used at line 1969).
| const metaConfigResponse2 = await compilerApi.metaConfig(request.context, { | |
| const metaConfigResponse = await compilerApi.metaConfig(request.context, { |
| const metaConfigResult = await compilerApi.metaConfig(req.context, { | ||
| requestId: req.context.requestId, | ||
| }); | ||
| const metaConfigCubes = metaConfigResult.cubes || metaConfigResult; |
There was a problem hiding this comment.
The || metaConfigResult fallback is dead code — CompilerApi.metaConfig() now always returns { cubes, viewGroups }, never an array. This fallback gives the false impression that the old array format might still occur, which makes the code harder to reason about. Same applies to lines 304, 1974, and 2072.
Consider removing the fallback:
| const metaConfigCubes = metaConfigResult.cubes || metaConfigResult; | |
| const metaConfigCubes = metaConfigResult.cubes; |
| title?: string; | ||
| description?: string; | ||
| views?: any; | ||
| fileName?: string; |
There was a problem hiding this comment.
Nit: views is typed as any but in practice it's either string[], a function returning string[], or undefined (handled in compileViewGroup). A more specific type would improve readability:
| fileName?: string; | |
| views?: string[] | (() => string[]); |
|
|
||
| public get compiledViewGroups(): CompiledViewGroup[] { | ||
| return Array.from(this.viewGroupDefinitions.values()); | ||
| } | ||
|
|
||
| public viewGroupForView(viewName: string): string | undefined { | ||
| for (const [, viewGroup] of this.viewGroupDefinitions) { | ||
| if (viewGroup.views.includes(viewName)) { | ||
| return viewGroup.name; | ||
| } | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
viewGroupForView is defined but never called — the view-to-group resolution is done inline in CubeToMetaTransformer.resolveViewGroups() instead. Consider removing this dead method, or refactoring resolveViewGroups to use it for consistency.
| const extendedCube = this.cubeSymbols.cubeList.find(c => c.name === cube.config.name) as any; | ||
| const viewGroupName = extendedCube?.viewGroup; |
There was a problem hiding this comment.
This cubeList.find() scan runs inside a loop over all cubes, making it O(n²). Consider building a lookup Map<string, CubeDefinition> before the loop. Similarly, this.cubes.find() at line 249 has the same issue.
For small numbers of views/groups this is fine, but if someone has hundreds of views it could add up.
| return this.compileObjects(compilers.cubeCompilers || [], cubes, errorsReport) | ||
| .then(() => this.compileObjects(compilers.contextCompilers || [], contexts, errorsReport)); | ||
| .then(() => this.compileObjects(compilers.contextCompilers || [], contexts, errorsReport)) | ||
| .then(() => this.compileObjects(compilers.viewGroupCompilers || [], viewGroups, errorsReport)) | ||
| .then(() => this.metaTransformer?.compileViewGroups()); |
There was a problem hiding this comment.
compileCubeFiles is called in every phase (0–3), but viewGroupCompilers is only provided in phase 3. In earlier phases this runs compileObjects([], viewGroups, errorsReport) (no-op) followed by this.metaTransformer?.compileViewGroups() which executes against incomplete data and gets overwritten in phase 3.
Not a bug (phase 3 produces the correct final result), but it means resolveViewGroups() runs 3 extra times with stale data. Consider guarding:
| return this.compileObjects(compilers.cubeCompilers || [], cubes, errorsReport) | |
| .then(() => this.compileObjects(compilers.contextCompilers || [], contexts, errorsReport)); | |
| .then(() => this.compileObjects(compilers.contextCompilers || [], contexts, errorsReport)) | |
| .then(() => this.compileObjects(compilers.viewGroupCompilers || [], viewGroups, errorsReport)) | |
| .then(() => this.metaTransformer?.compileViewGroups()); | |
| return this.compileObjects(compilers.cubeCompilers || [], cubes, errorsReport) | |
| .then(() => this.compileObjects(compilers.contextCompilers || [], contexts, errorsReport)) | |
| .then(() => this.compileObjects(compilers.viewGroupCompilers || [], viewGroups, errorsReport)) | |
| .then(() => (compilers.viewGroupCompilers?.length ? this.metaTransformer?.compileViewGroups() : undefined)); |
| private resolveViewGroups(): ViewGroupConfig[] { | ||
| const viewGroupMap = new Map<string, ViewGroupConfig>(); | ||
|
|
||
| for (const compiled of this.viewGroupEvaluator.compiledViewGroups) { | ||
| viewGroupMap.set(compiled.name, { | ||
| name: compiled.name, | ||
| title: compiled.title, | ||
| description: compiled.description, | ||
| views: [...compiled.views], | ||
| }); | ||
| } | ||
|
|
||
| for (const cube of this.cubes) { | ||
| if (cube.config.type === 'view') { | ||
| const extendedCube = this.cubeSymbols.cubeList.find(c => c.name === cube.config.name) as any; | ||
| const viewGroupName = extendedCube?.viewGroup; | ||
| if (viewGroupName) { | ||
| let group = viewGroupMap.get(viewGroupName); | ||
| if (!group) { | ||
| group = { name: viewGroupName, views: [] }; | ||
| viewGroupMap.set(viewGroupName, group); | ||
| } | ||
| if (!group.views.includes(cube.config.name)) { | ||
| group.views.push(cube.config.name); | ||
| } | ||
| cube.config.viewGroup = viewGroupName; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (const group of viewGroupMap.values()) { | ||
| for (const viewName of group.views) { | ||
| const cube = this.cubes.find(c => c.config.name === viewName); | ||
| if (cube && !cube.config.viewGroup) { | ||
| cube.config.viewGroup = group.name; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return Array.from(viewGroupMap.values()); | ||
| } |
There was a problem hiding this comment.
No validation that views referenced in a view_group() definition actually exist. If someone writes views: ['nonexistent_view'], it silently appears in the meta API output. Consider logging a warning (via an ErrorReporter, similar to how CubeValidator reports invalid cube names) for view names that don't match any compiled view.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10768 +/- ##
==========================================
+ Coverage 58.01% 58.02% +0.01%
==========================================
Files 215 216 +1
Lines 16764 16841 +77
Branches 3383 3405 +22
==========================================
+ Hits 9725 9772 +47
- Misses 6547 6573 +26
- Partials 492 496 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Check List
Description of Changes Made
Adds
view_groupas a new first-class object in the Cube data model. View groups allow organizing views into logical groups for better discoverability and navigation in the meta API response.Definition Methods
View groups can be defined in two ways:
1. Standalone
view_groupdefinition (JS):2. Via
viewGroupproperty on individual views (JS):3. YAML standalone definition:
4. YAML view-level definition:
Both definition methods can be combined — a standalone
view_groupdefines the group metadata (title, description) and lists views, while individual views can also declare their group membership viaview_group. The system merges both sources.Meta API Response
View groups are served as part of the
/v1/metaresponse:{ "cubes": [...], "viewGroups": [ { "name": "sales", "title": "Sales", "description": "Sales related views", "views": ["revenue", "customers"] } ] }Each view cube config also includes a
viewGroupfield indicating which group it belongs to.Implementation Details
Modified packages:
@cubejs-backend/schema-compiler— NewViewGroupEvaluatorcompiler, schema validation, YAML transpilation, meta transformer integration@cubejs-backend/api-gateway— IncludeviewGroupsin meta response@cubejs-backend/server-core— IncludeviewGroupsinCompilerApi.metaConfig()responseKey changes:
ViewGroupEvaluatorclass (similar toContextEvaluator) that compilesview_group()definitionsviewGroupproperty added to view schema inCubeValidatorview_groupstop-level key support in YAML compilerview_group()global function registered inDataSchemaCompilerVM contextCubeToMetaTransformerresolves and merges view groups from both sourcesCompilerApi.metaConfig()now returns{ cubes, viewGroups }object (previously returned cubes array directly whenincludeCompilerIdwas false)