feat: support nested view groups via includes#10999
Conversation
github-actions
Bot
added
client:core
|
Claude finished @igorlukanin's task in 3m 0s —— View job Code reviewVerdict: Looks solid overall — feature works end-to-end, validation/transpile/evaluation/meta paths line up, and the test matrix is unusually thorough (string/bare/mixed refs, deep nesting, sibling + parent/child membership, mutual exclusion, unknown view, dup nested, malformed). Posted 0 high, 2 medium, 3 low inline. Details (click to expand)Checklist
FindingsMedium
Low Nits / praise
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| private isNestedGroup(item: any): item is ViewGroupIncludeNested { | ||
| return typeof item === 'object' && item !== null && typeof item.name === 'string' && 'includes' in item; | ||
| } |
There was a problem hiding this comment.
[medium] isNestedGroup keys off 'includes' in item — but the recursive Joi viewGroupSchema (via Joi.link('#viewGroupSchema')) also accepts { name, views: [...] } for a nested entry (since the schema only enforces oxor('views', 'includes'), not xor). A nested group authored with the legacy views: field would pass validation, then isNestedGroup returns false, then resolveViewReference is called on the object and returns [] — the nested group is silently dropped with no error.
Either tighten the schema to require includes on nested entries, or detect "is this an object with name" here and surface an error (or honor views). Right now there's a quiet hole between validator and evaluator.
Same hole exists in the YAML path: viewGroupBodyAst emits both views and includes for nested groups, so a YAML nested group using views: makes it through transpile but gets dropped at evaluation.
| @@ -63,86 +122,207 @@ export class ViewGroupEvaluator implements CompilerInterface { | |||
| title: viewGroup.title, | |||
| description: viewGroup.description, | |||
| views, | |||
| includes: views.slice(), | |||
| }; | |||
| } | |||
|
|
|||
| /** | |||
| * Recursively compiles a view group's `includes` into the group's own direct | |||
| * view references plus a recursive `includes` representation (strings for | |||
| * views, nested CompiledViewGroup objects for nested groups). | |||
| */ | |||
| private compileIncludes( | |||
| rawIncludes: ViewGroupInclude[] | (() => any[]), | |||
| seenNames: Set<string>, | |||
| errorReporter?: ErrorReporter, | |||
| ): { views: string[]; includes: (string | CompiledViewGroup)[] } { | |||
| let items: any[] = []; | |||
| if (typeof rawIncludes === 'function') { | |||
| const evaluated = rawIncludes(); | |||
| items = Array.isArray(evaluated) ? evaluated : [evaluated]; | |||
| } else if (Array.isArray(rawIncludes)) { | |||
| items = rawIncludes; | |||
| } | |||
|
|
|||
| const views: string[] = []; | |||
| const includes: (string | CompiledViewGroup)[] = []; | |||
|
|
|||
| for (const item of items) { | |||
| if (this.isNestedGroup(item)) { | |||
| if (errorReporter && seenNames.has(item.name)) { | |||
| errorReporter.error(`View group "${item.name}" already exists!`); | |||
| // eslint-disable-next-line no-continue | |||
| continue; | |||
| } | |||
| seenNames.add(item.name); | |||
There was a problem hiding this comment.
[medium] Nested group name uniqueness is scoped per-top-level tree (seenNames is reset each compileViewGroup call), so two unrelated top-level groups can each define a nested ent_sales, and a nested group can even share its name with an unrelated top-level group — no collision error.
Two concrete consequences:
viewToGroups(used for the per-viewviewGroupstag in meta) maps view → group name. If a top-levelfooand a nestedfooboth exist, the tag becomes ambiguous and downstream lookups (viewGroupMap.get(name)) resolve only the top-level entry.- The duplicate-name test on line 1268 of the test file passes only because both
dups sit in the same parent tree. Move one to a sibling top-level group and the error disappears.
Consider keeping a global seenNames set populated with all top-level names up-front, and threading it through compileIncludes recursion across all groups — or at minimum, after compiling, fail if any nested name clashes with another nested or top-level name.
| for (const groupName of this.viewGroupNamesForCube(cube)) { | ||
| const group = viewGroupMap.get(groupName); | ||
| if (!group) { | ||
| if (errorReporter) { | ||
| errorReporter.error(`View "${cube.name}" references view group "${groupName}" which is not defined. Define it using view_group('${groupName}', { ... }).`); | ||
| } | ||
| } else if (!group.views.includes(cube.name)) { | ||
| group.views.push(cube.name); | ||
| group.includes.push(cube.name); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[low] viewGroupNamesForCube only looks up top-level entries in viewGroupMap, so a view declaring viewGroup: 'ent_sales' (a nested-group name) compiles with an error saying the group is "not defined" — even though it visibly exists in the nested hierarchy. Worth either (a) allowing cubes to auto-attach to nested groups by walking the resolved tree, or (b) documenting that the viewGroup/viewGroups cube properties only accept top-level group names. Either is fine; right now the behavior is surprising.
| const viewGroupSchema = Joi.object().keys({ | ||
| name: Joi.string().required(), | ||
| title: Joi.string(), | ||
| description: Joi.string(), | ||
| // Legacy way of including views into a group, kept for backward compatibility. | ||
| views: Joi.alternatives([Joi.array().items(Joi.string().required()), Joi.func()]), | ||
| // Preferred way of including views (and nested view groups) into a group. | ||
| includes: Joi.alternatives([ | ||
| Joi.func(), | ||
| Joi.array().items( | ||
| Joi.alternatives([ | ||
| Joi.string().required(), | ||
| Joi.func(), | ||
| Joi.link('#viewGroupSchema'), // Can contain nested view groups | ||
| ]), | ||
| ), | ||
| ]), | ||
| fileName: Joi.string(), | ||
| }) | ||
| .oxor('views', 'includes') | ||
| .messages({ | ||
| 'object.oxor': 'View group must use either "views" or "includes", but not both' | ||
| }) | ||
| .id('viewGroupSchema'); |
There was a problem hiding this comment.
[low] Because nested entries are validated through the same #viewGroupSchema link, a nested entry inherits name: required plus optional views | includes plus fileName — but a nested entry with neither views nor includes (e.g. { name: 'empty' }) passes validation and produces a zero-view compiled group that still gets a viewToGroups entry for no one. Consider adding .or('views', 'includes') so a nested entry must include something, or — better — defining a separate schema for nested entries that omits the legacy views path entirely.
Side note: fileName is also accepted for nested entries from this same link, which is meaningless. A separate nested schema would tidy that up too.
| * nor any of its nested groups contains a visible view, preventing leaks of | ||
| * restricted view names. | ||
| */ | ||
| private filterVisibleViewGroup(group: any, visibleCubeNames: Set<string>): any | null { | ||
| const views = (group.views || []).filter((v: string) => visibleCubeNames.has(v)); | ||
| const includes = (group.includes || []) | ||
| .map((include: any) => { | ||
| if (typeof include === 'string') { | ||
| return visibleCubeNames.has(include) ? include : null; | ||
| } | ||
| return this.filterVisibleViewGroup(include, visibleCubeNames); | ||
| }) | ||
| .filter((include: any) => include !== null); | ||
|
|
||
| if (views.length === 0 && includes.length === 0) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
[low] { ...group, views, includes } copies every property from the compiler-side group object into the meta response — including anything future code adds to CompiledViewGroup (e.g. internal fields). This is mostly fine today but is an easy place to leak future internal state. Consider an explicit projection: { name, title, description, views, includes }.
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
1 participant
Summary
includesparameter to view groups as the way to add views to a group, mirroring how folders useincludes. It accepts view references and nested view group definitions ({ name, title, description, includes }), supporting an arbitrarily deep structure.includesis the encouraged form; the legacyviewsparameter is still accepted for backward compatibility but is no longer documented.viewsandincludesare mutually exclusive on a single view group (enforced via Joioxor)./v1/metaresponse exposes a recursiveincludesarray on each view group (alongside the existingviewsarray of direct members). A view'sviewGroupstag reports the most-specific group(s) it directly belongs to; a view may belong to multiple groups (e.g. two sibling groups, or a parent and its nested child). Nested groups are pruned from meta when none of their views are visible to the requester.viewsandincludes).Test plan
/v1/metareturns the nested structure; invalid both-fields model fails to compile