fix(schema-compiler): reject multiple joins to the same cube - #11809
fix(schema-compiler): reject multiple joins to the same cube#11809waralexrom wants to merge 5 commits into
Conversation
A cube declaring more than one join to the same cube compiles today and all but one of those joins is dropped without a word, so the model silently resolves through a join path the author did not write. Covers the duplicate declarations that must be rejected together with the shapes that must keep compiling: joins to different cubes, the same pair joined from both sides, a transitive path, and a child cube overriding a join it inherited through extends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The join graph is keyed by the pair of cubes a join connects, so a cube declaring several joins to the same cube kept only the last of them. Nothing was reported: the model compiled, queries ran, and they resolved through a join path the author never wrote, which is impossible to notice from the outside. Report it as a model compilation error instead, naming the file, the cube, every conflicting declaration and the way out. The cube is also marked invalid, so that with compile errors omitted it drops out of the join graph rather than keeping the collapsed path. Only the joins a cube declares itself are checked. `extends` appends a child's joins to the inherited ones, and a child redeclaring an inherited join replaces it, which stays valid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 30s —— View job Review: sound fix, 3 comments — 1 medium (blast radius of
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Medium | CubeValidator.ts:1522 |
valid = false drops the entire cube from the join graph, meta API and cube evaluator under omitErrors: true — every measure and dimension vanishes, and other cubes lose their edges to it, over one extra joins entry. buildJoinEdges handles its analogous soft failure (Cube X doesn't exist) by filtering just that edge. The omitErrors test also only asserts toThrow(/orders/), which any unrelated failure would satisfy. |
| 2 | Low | CubeValidator.ts:1486-1494 |
9-line comment; the first paragraph restates the method name and the error message. Repo guidance is 3 lines — keep the extends rationale, and consider a named helper so the reach into cubeSymbols.cubeDefinitions carries its own justification. |
| 3 | Low | CubeValidator.ts:1519 |
"the rest are ignored" describes the pre-change behaviour; after this change nothing is ignored because the model does not compile. State the rule instead. |
Docs gap (not posted inline — the file isn't in the diff)
docs-mintlify/reference/data-modeling/joins.mdx never states the one-join-per-cube-pair rule, so an operator hit by this at deploy time has no reference to point at. Given this is a deliberate breaking change, a two-line <Note> in that page (and, if the release warrants it, a migration mention) would carry more weight than the error string alone. Per the repo's CLAUDE.md, /docs-mintlify is the right place; leave /docs alone.
On the warning-vs-error question
The PR pre-empts this and I don't think it needs re-litigating in code review — it is a release call, and the reasoning given (a warning still leaves a model silently using a join path nobody wrote) is sound. Worth noting though that finding #1 changes the shape of that call: as written, the error is not just fatal under strict compilation, it also degrades the omitErrors path from "one wrong join" to "cube gone". Scoping that down makes the strict-error choice easier to defend, because the soft path then fails proportionally.
Not verified
I could not run the test suite — node_modules is not installed in this checkout and a full monorepo yarn install was out of scope for the review. My assessment of the tests is from reading them plus tracing the code they exercise; the PR's own report of 960/962 with 2 pre-existing error-reporter ANSI snapshot failures is consistent with what CI should show.
No existing review threads on this PR, so nothing to resolve or deduplicate against.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11809 +/- ##
==========================================
+ Coverage 60.16% 60.19% +0.03%
==========================================
Files 239 239
Lines 19203 19223 +20
Branches 3886 3891 +5
==========================================
+ Hits 11554 11572 +18
- Misses 7099 7101 +2
Partials 550 550
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:
|
…self Marking the cube invalid took the whole cube out of the join graph, the meta API and the cube evaluator. Every other cube joining to it then reported `Cube X doesn't exist`, which is false and unrelated to what went wrong. Keep the cube valid and drop only the conflicting joins from the graph, the way a join to a cube that does not exist is already handled. Strict compilation still fails on the reported error; with errors omitted the cube keeps serving its own members and only the ambiguous join is gone. Also state the rule in the error instead of describing the behavior it replaces, and document it in the joins troubleshooting page. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
|
Claude finished @waralexrom's task in 2m 23s —— View job Re-review after
|
| # | Prior finding | Status |
|---|---|---|
| 1 | valid = false dropped the whole cube under omitErrors: true |
Fixed properly. validateUniqueJoins no longer returns a validity flag; it records the offending join targets in duplicateJoins and JoinGraph.buildJoinEdges filters just those edges (JoinGraph.ts:129-134), which is exactly the shape the neighbouring Cube X doesn't exist case uses. The cube keeps all its members, other cubes keep their edges to it, and only the ambiguous path is gone. |
| 2 | 9-line comment | Fixed. Down to the two load-bearing lines (raw definition, not cube.joins, plus the map-form early return). Both earn their place — a refactor to cube.joins would silently break the extends override, and the Array.isArray guard is non-obvious without the map-form note. |
| 3 | "the rest are ignored" described the removed behaviour | Fixed. Now Only one join per pair of cubes is supported, and that string is also the docs heading, so the error is greppable straight to docs/data-modeling/joins.mdx#L537. |
Plus the docs gap I raised outside the diff is closed: the new Troubleshooting section sits alongside Can't find join path and Primary key is required when join is defined, and the extends workaround it shows is the same one the error message names. [ref-extends] resolves (reference/data-modeling/cube.mdx:134).
Things I checked and found sound
- Ordering. The new
isDuplicateJoinread happens incubeCompilers, whileCubeValidatorruns inpreTranspileCubeCompilers(PrepareCompiler.ts:96,103) — a strictly earlier phase over all cubes, so the map is fully populated before any edge is built. No per-cube interleaving hazard. - Default when validation never ran.
isDuplicateJoinreturnsfalsevia?? false, so split views (valid without being validated,isCubeValidfalls through tocube.isSplitView) and views behave as before. - Recompilation.
validateUniqueJoinswrites a freshSetfor the cube on every call, so a re-validation can't leave a stale duplicate marked. - Tests. The
omitErrorstest is materially better than before: it now asserts the cube still serves on its own (orders_tblpresent fororders.countalone) and that only the join path is gone (Can't find join path to join), rather than the previoustoThrow(/orders/)which any failure satisfied. The newdoes not make cubes joining to it report that it does not existtest pins down the regression that finding ReferenceError: self is not defined - Issue with whatwg-fetch when using the core lib with Node.js #1 was about, with a negative assertion — good choice.
New finding
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | JoinGraph.ts:133 |
The duplicate set is keyed by the cube that declares the duplicates, but cube.joins at that point is the merged list. A cube that extends the offender is not filtered, so under omitErrors: true it still resolves the join through the last duplicate declaration — the exact collapse this PR removes. Invisible under strict compilation (the parent's error throws first), so it only affects the path 111648a was written for. Not a one-liner: JoinGraph can't distinguish an inherited duplicate from a legitimate child override, since the override relies on the same last-wins collapse. Suggested narrow fix is to union the ancestor's duplicate set into the child's in CubeValidator while keeping error attribution on the declaring cube. |
On the warning-vs-error question
Still a release call, not a code one, and I don't think it needs re-litigating — but the scoping in 111648a does strengthen the case for keeping it fatal. The soft path now degrades to "this one join path is unavailable" instead of "cube gone", which is a proportionate failure that an operator can read off the error and fix.
Not verified
node_modules is absent in this checkout and a full monorepo yarn install was out of scope, so I did not run the suite — the test assessment is from reading them and tracing the code they exercise. Finding #1 above is likewise derived from reading CubeSymbols.ts:427-451 and the new filter, not from an executed repro.
Resolved my three earlier threads (all superseded by this commit). No human review threads on the PR, so nothing else to leave alone.
· branch cube-duplicate-cube-joins-error ·
| protected readonly validCubes: Map<string, boolean> = new Map(); | ||
|
|
||
| // cube name -> names of the cubes it declares more than one join to | ||
| protected readonly duplicateJoins: Map<string, Set<string>> = new Map(); |
There was a problem hiding this comment.
@claude I guess we should clean it after validation, do we have a place/method to do it?
There was a problem hiding this comment.
Claude finished @ovr's task in 8m 56s —— View job
Yes — CubeSymbols.compile, before the joins getter merges anything. Written and committed, but I could not push (403)
Short answer to @ovr: cleaning it after validation doesn't work, but cleaning it before does, and that place also closes the inherited-duplicates gap and lets the JoinGraph change be reverted entirely.
- Trace where the merged
joinsarray is built and memoized (CubeSymbols.createCube) - Check the compiler phase ordering to see what state survives validation
- Implement the cleanup at the merge site, revert the
JoinGraphfilter, drop the validator state - Cover the inherited-duplicate case in tests
-
Run the suite— nonode_modulesin this checkout andyarn installis not permitted for this job -
Push—git pushreturned 403 (Permission to cube-js/cube.git denied to github-actions[bot]), so the patch is below rather than on the branch
Why "after validation" has no place to hook into
Two things get in the way, and both are worth knowing before picking a spot:
- The model is re-executed per phase.
DataSchemaCompilerrunscleanup()and re-compiles the files for each phase (DataSchemaCompiler.ts:491-521), andcube()pushes a fresh plain object each time (DataSchemaCompiler.ts:414).CubeValidatorruns in phase 1 (preTranspileCubeCompilers);JoinGraphreadscubeEvaluator.cubeListin phase 3, andCubeEvaluator.compilecallssuper.compile(cubes, …)— i.e. it builds its own cube definitions from phase-3 objects. So anything the validator mutates in phase 1 is thrown away before the join graph is built. Only name-keyed state crosses phases (validCubes, and henceisCubeValid) — which is exactly why the current commit neededisDuplicateJoin. - By validation time the merged array is already memoized.
transform()touchescube.joinsviacamelCaseTypesduringCubeSymbols.compile, and thejoinssetter is a no-op, so a post-validation prune would have to splice in place — and still wouldn't reach a child cube, because each child builds[...parentJoins, ...localJoins], its own copy.
The place that satisfies both
The start of CubeSymbols.compile, over the raw definitions, before this.cubeList builds any cube object:
- it runs in every phase and in every
CubeSymbolsinstance —cubeSymbols(phase 1),viewCompiler(phase 2) andcubeEvaluator(phase 3) — so the cleaned model is what the validator, the evaluated cubes, views and the join graph all read; - the raw defs are plain objects, so
cube.joins = …actually assigns (no no-op setter); - it precedes the
extendsmerge, so a cube extending the offender inherits the cleaned list — this closes the finding I left open onJoinGraph.ts:133, without needing to propagate anything through the extends chain; CubeSymbolsrecords the removed declarations by position, andCubeValidatorreports from that record — so the error text, thejoins[0], joins[1]positions, the file name and the "blame the declaring cube" behaviour are unchanged;JoinGraphgoes back tomaster, andCubeValidatorloses its map and its publicisDuplicateJoin.
Bonus: the dropped joins no longer reach CubeEvaluator.prepareJoins either, so they are absent from the evaluated model that views and the native planner read — the previous version filtered them only inside the join graph.
The patch
src — 47 added in CubeSymbols.ts, 47→18 in CubeValidator.ts, JoinGraph.ts reverted
--- a/packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts
+++ b/packages/cubejs-schema-compiler/src/compiler/CubeSymbols.ts
@@ -289,6 +289,10 @@ export class CubeSymbols implements TranspilerSymbolResolver, CompilerInterface
public cubeList: CubeDefinitionExtended[];
+ // Removed by `dropDuplicateJoins`, reported by the CubeValidator:
+ // cube name -> joined cube name -> positions it was declared at
+ public readonly duplicateJoins: Map<string, Map<string, number[]>> = new Map();
+
private readonly evaluateViews: boolean;
@@ -308,6 +312,7 @@ export class CubeSymbols implements TranspilerSymbolResolver, CompilerInterface
this.cubeList = [];
+ this.duplicateJoins.clear();
this.resolveSymbolsCallContext = undefined;
}
@@ -316,6 +321,8 @@ export class CubeSymbols implements TranspilerSymbolResolver, CompilerInterface
);
+ this.dropDuplicateJoins(cubes);
+
this.cubeList = cubes.map(c => (c.name ? this.getCubeDefinition(c.name) : this.createCube(c)));
@@ -340,6 +347,46 @@ export class CubeSymbols implements TranspilerSymbolResolver, CompilerInterface
+ /**
+ * The join graph is keyed by the pair of cubes a join connects, so only one
+ * join per joined cube survives into it: any extra declaration is dropped
+ * there and the model resolves through a path its author did not write. Take
+ * them out of the definition instead, so nothing downstream builds on a join
+ * that was not chosen, and let the CubeValidator report the conflict.
+ *
+ * Over the raw definitions, before the `joins` getter merges the parent's
+ * joins in: a cube extending this one then inherits the cleaned list, while a
+ * child redeclaring an inherited join keeps overriding it as before.
+ */
+ private dropDuplicateJoins(cubes: CubeDefinition[]) {
+ this.duplicateJoins.clear();
+
+ for (const cube of cubes) {
+ // The map form is keyed by the joined cube name and can not hold duplicates
+ if (!Array.isArray(cube.joins)) {
+ continue;
+ }
+
+ const declaredAt = new Map<string, number[]>();
+ cube.joins.forEach((join, index) => {
+ if (join?.name) {
+ declaredAt.set(join.name, [...(declaredAt.get(join.name) || []), index]);
+ }
+ });
+
+ const duplicates = new Map(
+ [...declaredAt].filter(([, indexes]) => indexes.length > 1)
+ );
+
+ if (duplicates.size > 0) {
+ this.duplicateJoins.set(cube.name, duplicates);
+ // Which of the conflicting declarations was meant is unknowable, so none
+ // of them is kept
+ cube.joins = cube.joins.filter(join => !join?.name || !duplicates.has(join.name));
+ }
+ }
+ }
+
public getCubeDefinition(cubeName: string): CubeDefinitionExtended {
--- a/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts
+++ b/packages/cubejs-schema-compiler/src/compiler/CubeValidator.ts
@@ -1402,9 +1402,6 @@ export class CubeValidator implements CompilerInterface {
protected readonly validCubes: Map<string, boolean> = new Map();
- // cube name -> names of the cubes it declares more than one join to
- protected readonly duplicateJoins: Map<string, Set<string>> = new Map();
-
public constructor(
@@ -1484,38 +1481,22 @@ export class CubeValidator implements CompilerInterface {
+ // Detected over the cube's own declarations and taken out of the definition by
+ // `CubeSymbols.dropDuplicateJoins`, which runs before `extends` merges the
+ // parent's joins in, so a cube is only blamed for what it declares itself
private validateUniqueJoins(cube, errorReporter: ErrorReporter) {
- // The raw definition, not `cube.joins`: … (detection removed)
- const ownJoins = this.cubeSymbols.cubeDefinitions[cube.name]?.joins;
- …
+ const duplicates = this.cubeSymbols.duplicateJoins.get(cube.name);
+
+ if (!duplicates) {
+ return;
+ }
+
+ for (const [joinedCube, indexes] of duplicates.entries()) {
+ const declarations = indexes.map(index => `joins[${index}]`).join(', ');
+ errorReporter.error(
+ `Cube '${cube.name}' declares ${indexes.length} joins to '${joinedCube}' (${declarations}). Only one join per pair of cubes is supported. Keep a single join to '${joinedCube}', or use extends to create a child cube of '${joinedCube}' and join that instead`,
+ cube.fileName
+ );
}
}
@@ -1571,8 +1552,4 @@ export class CubeValidator implements CompilerInterface {
public isCubeValid(cube: CubeDefinition): boolean {
return this.validCubes.get(cube.name) ?? cube.isSplitView ?? false;
}
-
- public isDuplicateJoin(cubeName: string, joinName: string): boolean {
- return this.duplicateJoins.get(cubeName)?.has(joinName) ?? false;
- }
}
--- a/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
+++ b/packages/cubejs-schema-compiler/src/compiler/JoinGraph.ts
@@ -127,12 +127,6 @@ export class JoinGraph implements CompilerInterface {
return cube.joins
.filter(join => {
- // Already reported when the cube was validated. Which of the conflicting
- // declarations was meant is unknowable, so none of them is used
- if (this.cubeValidator.isDuplicateJoin(cube.name, join.name)) {
- return false;
- }
-
if (!this.cubeEvaluator.cubeExists(join.name)) {Two tests added to test/unit/duplicate-cube-joins.test.ts (both under omitErrors: true, since the strict path throws on the parent):
drops the conflicting joins for a cube that inherits them— the case my earlier finding described:orders extends orders_base, the duplicates are gone from the child too (Can't find join path to join).keeps a child join that replaces the conflicting inherited ones— only the conflicting declarations are dropped, so a child's own redeclaration still wins ("orders".approver_id = "users".id, and neither parentONclause).
The seven existing tests should be unaffected — the error text, positions and attribution are unchanged, and the two behavioural ones (… when compile errors are omitted, does not make cubes joining to it report that it does not exist) assert exactly what the definition-level prune produces.
What I could not verify
No node_modules in this checkout and yarn install is not in this job's allowlist, so nothing here has been type-checked or run — the reasoning is from reading DataSchemaCompiler.compile, CubeSymbols.compile/createCube, CubeEvaluator.compile and JoinGraph.compile. Worth running yarn test test/unit/duplicate-cube-joins.test.ts plus the views/joins unit tests locally, since the prune now also affects prepareIncludes/joinMap for views.
@waralexrom — since the push failed, this needs applying by hand or:
Apply this on the branch →
· branch cube-duplicate-cube-joins-error
| // in, and a child redeclaring one of them is a supported override | ||
| const ownJoins = this.cubeSymbols.cubeDefinitions[cube.name]?.joins; | ||
| const duplicates = new Set<string>(); | ||
| this.duplicateJoins.set(cube.name, duplicates); |
There was a problem hiding this comment.
@claude is it possible to rework the PR without storing duplicateJoins in global state behind CubeValidator?
There was a problem hiding this comment.
Claude finished @ovr's task in 7m 49s —— View job
Yes — the state can go away entirely. Reworked so the rule lives only in JoinGraph; ⚠️ push was denied in this run, patch below
- Re-read the current implementation (
CubeValidator,JoinGraph,CubeSymbols.joins) - Confirm
JoinGraphcan derive the duplicate set itself - Revert
CubeValidator(noduplicateJoinsmap, noisDuplicateJoin) — the file is now byte-identical tomaster - Detect + report + filter in
JoinGraph.buildJoinEdges - Re-check every test in the file against the new phase
- Add a test covering two cubes each declaring duplicates
-
Run the suite—yarn installis not permitted in this run (see Not verified) -
Push—git pushreturned 403 forgithub-actions[bot], so commit62afa94is local only
Why the state existed, and why it doesn't need to
Detection has to read the cube's own joins (cubeDefinitions[name].joins), not the merged cube.joins, because extends appends and a child redeclaring an inherited join is a supported override. Filtering has to happen in JoinGraph. Splitting those two across components is what forced the result to be carried in a map on the validator — state that outlives the compile it was computed for, and, as you point out, needs invalidating.
It only had to travel because I put detection in the wrong place. JoinGraph already holds a CubeEvaluator, and CubeEvaluator extends CubeSymbols, so this.cubeEvaluator.cubeDefinitions[cube.name]?.joins is right there — the same raw definition CubeValidator was reaching for. cubeEvaluator runs immediately before joinGraph in the same cubeCompilers phase (PrepareCompiler.ts:103) and compileObjects hands each compiler the full cube list, so the map is fully populated.
So detection, reporting and filtering are now one function, called per cube while its edges are built:
const duplicateJoins = this.duplicateJoinTargets(cube, errorReporter);
return cube.joins
.filter(join => {
// Which of the conflicting declarations was meant is unknowable, so none
// of them is used
if (duplicateJoins.has(join.name)) {
return false;
}
if (!this.cubeEvaluator.cubeExists(join.name)) { … }This also puts the error next to the join errors buildJoinEdges already reports (Cube X doesn't exist, primary key for X is required), which is arguably where it belonged: the constraint is a property of the graph — one edge per pair of cubes — not of a cube's schema shape, which is what CubeValidator checks. Nothing is stored, so there is nothing to clean between compiles (which answers your other question on duplicateJoins: the cleanup point disappears with the field). The error still reaches the user identically — errorReporter.inContext('orders cube') in JoinGraph.compile, cube.fileName passed through, and throwIfAnyErrors runs after all phases, so strict compilation still fails.
Alternatives I rejected
- Keep detection in
CubeValidator, recompute inJoinGraph. Removes the state but leaves the same rule written twice, which is worse than the state was. - Collapse overrides in
CubeSymbols'joinsgetter (drop inherited joins the child redeclares, instead of appending), so any duplicate left in the merged list is genuine and both sides can just look atcube.joins. This would additionally close the inherited-duplicate gap from my previous review, but it changes an inheritance path everyextendsmodel goes through — edge insertion order, and grandchild-override semantics — which is too much to ride along on this PR. Worth its own change if you want that gap closed; as written it is unchanged and noted in the code comment.
The patch (commit 62afa94, local)
CubeValidator.ts — full revert of the previous commit's changes (field, validateUniqueJoins, isDuplicateJoin, and the else if restored). JoinGraph.ts:
@@ -165,6 +167,48 @@ export class JoinGraph implements CompilerInterface {
});
}
+ /**
+ * The cubes a cube declares more than one join to. Only one edge per pair of
+ * cubes fits into the graph, so the extra declarations would be dropped and
+ * the model would resolve through a path its author did not write.
+ */
+ protected duplicateJoinTargets(cube: CubeDefinition, errorReporter: ErrorReporter): Set<string> {
+ const duplicates = new Set<string>();
+ // The raw definition, not `cube.joins`: `extends` merges the parent's joins
+ // in, and a child redeclaring one of them is a supported override. Duplicates
+ // a parent declares are reported and dropped on the parent itself
+ const ownJoins = this.cubeEvaluator.cubeDefinitions[cube.name]?.joins;
+
+ // The map form is keyed by the joined cube name and can not hold duplicates
+ if (!Array.isArray(ownJoins)) {
+ return duplicates;
+ }
+
+ const declarationsByCube = new Map<string, number[]>();
+
+ ownJoins.forEach((join, index) => {
+ if (!join?.name) {
+ return;
+ }
+ const declarations = declarationsByCube.get(join.name) ?? [];
+ declarations.push(index);
+ declarationsByCube.set(join.name, declarations);
+ });
+
+ for (const [joinedCube, indexes] of declarationsByCube.entries()) {
+ if (indexes.length > 1) {
+ duplicates.add(joinedCube);
+ const declarations = indexes.map(index => `joins[${index}]`).join(', ');
+ errorReporter.error(
+ `Cube '${cube.name}' declares ${indexes.length} joins to '${joinedCube}' (${declarations}). Only one join per pair of cubes is supported. Keep a single join to '${joinedCube}', or use extends to create a child cube of '${joinedCube}' and join that instead`,
+ cube.fileName
+ );
+ }
+ }
+
+ return duplicates;
+ }
+
protected buildJoinNode(cube: CubeDefinition): Record<string, 1> {Plus one new test, reports every cube that declares duplicates (two cubes, each with its own duplicate target, both named in the error) — it pins down that the set is derived per cube rather than shared.
Effect on the existing tests
All seven keep their meaning; only the phase the error comes from changes.
| Test | Under the rework |
|---|---|
rejects two joins to the same cube, reports every conflicting declaration |
Same message, now raised in the join-graph phase; throwIfAnyErrors runs after all phases, so it still throws |
lets a child cube override a join inherited through extends |
orders has own joins, one entry per target → no duplicate; unchanged |
blames the cube that declares the duplicates |
cubeDefinitions['orders'].joins is undefined (extends only) → orders reports nothing, orders_base does |
drops the conflicting joins … when compile errors are omitted |
Filtering is unchanged, so the cube still serves orders.count and only the join path is gone |
does not make cubes joining to it report that it does not exist |
The cube is never invalidated (validator untouched now), so this holds by construction |
Not verified
node_modules is absent in this checkout and yarn install is not in this run's allowed tools, so I could not compile or run the suite — the analysis is from reading the code and tracing the phases. And git push returned 403 Permission to cube-js/cube.git denied to github-actions[bot], so nothing landed on the branch: either re-trigger me on a run with contents write, or apply the patch above (it is three files: the CubeValidator.ts revert, the JoinGraph.ts diff, the added test).
· branch cube-duplicate-cube-joins-error
…ges are built Reporting the conflict in the validator and dropping the edges in the join graph split one rule across two components with nothing keeping them in sync. `buildJoinEdges` already owns this shape of failure — it reports a join to a cube that does not exist and leaves that join out — so the check belongs there, next to the collapse it guards against. Behavior is unchanged except that a cube failing validation for an unrelated reason no longer reports its duplicate joins in the same pass, which is how every other join-level error in that function already behaves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 3m 23s —— View job Re-review of
|
| Check | Result |
|---|---|
| Is the raw definition reachable there? | Yes — CubeEvaluator extends CubeSymbols, and cubeDefinitions is populated by CubeEvaluator.compile, which runs immediately before joinGraph in the same cubeCompilers phase (PrepareCompiler.ts:103). compileObjects hands each compiler the full cube list, so the map is complete. |
| Does strict compilation still fail? | Yes — throwIfAnyErrors runs once after doCompile() (DataSchemaCompiler.ts:549-551), so a phase-3 error throws exactly like a phase-1 one. |
| Double reporting? | No — buildJoinEdges has a single caller (JoinGraph.ts:74), once per cube per compile. |
| Is the edge really gone for both planners? | Yes — every join resolution goes through joinGraph.buildJoin (BaseQuery.js:340,391,441,461,…), so filtering at edge construction is the effective point for the legacy planner and Tesseract alike. |
| Split views / views without a raw definition? | cubeDefinitions[cube.name]?.joins → !Array.isArray(undefined) → empty set. Unchanged behaviour. |
Map form (joins: { users: {…} }) |
Correctly skipped: it is keyed by the joined cube and can't hold duplicates. The dynamic-model map form is normalised only inside the joins getter, which this deliberately doesn't read. |
Placing the error next to Cube X doesn't exist and primary key for X is required also reads better than it did in CubeValidator: the constraint is a property of the graph — one edge per pair of cubes — not of a cube's schema shape.
One thing the move gives up, and I think acceptably: buildJoinEdges runs only for cubes passing isCubeValid, so a cube that is invalid for an unrelated reason no longer gets its duplicate reported. Strict compilation still fails on the other error, and under omitErrors such a cube is out of the graph entirely, so nothing silently resolves through an unwritten path.
Findings
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Low | JoinGraph.ts:177-179 |
The comment's third line — "Duplicates a parent declares are reported and dropped on the parent itself" — reads as if the extends case were covered. It is only covered for the parent's own edges; a child that only extends has no own joins, so duplicates is empty for it while its merged cube.joins still holds both parent declarations and one becomes an edge. Either state the scope or drop the line. Posted inline. |
| 2 | Nit | JoinGraph.ts:175 |
duplicateJoinTargets is a pure-sounding query that reports errors; neither the name nor the call site at line 128 shows the side effect. Folded into the same inline comment. |
Finding 1 is the same underlying gap as my earlier thread on JoinGraph.ts:135, which still applies and I've left unresolved rather than re-posting. I don't think it needs closing in this PR — the honest fix is to make the joins getter collapse overrides instead of appending, which touches an inheritance path every extends model goes through.
Tests and docs
The seven tests keep their meaning under the move — the message, the joins[i] positions, the file name and the attribution are all unchanged, and the two behavioural ones still assert what edge-level filtering produces:
drops the conflicting joins instead of picking one when compile errors are omittedasserts both halves — the cube still servesorders.countofforders_tbl, and only the join path is gone (Can't find join path to join).does not make cubes joining to it report that it does not existnow holds by construction, since the validator is untouched and the cube is never invalidated.blames the cube that declares the duplicatesstill pins attribution, andlets a child cube override a join inherited through extendsstill protects the override pattern.
Coverage gap, matching finding 1: nothing exercises a cube that inherits duplicates under omitErrors: true. A test there would fail today.
Docs read well and sit with the sibling troubleshooting sections; the heading is the error string verbatim, so an operator can grep from the error straight to the page, and [ref-extends] resolves.
Not verified
node_modules is absent in this checkout and a monorepo yarn install is out of scope for this job, so I could not compile or run test/unit/duplicate-cube-joins.test.ts — the assessment is from reading the code and tracing the compile phases. CI is the check that matters here.
Nothing to resolve: my three pre-rework threads were already resolved, the one open thread of mine still applies, and @ovr's two threads are a human reviewer's to close.
`duplicateJoinTargets` read as a pure query while it also reported the errors, and its comment claimed the parent's duplicates are dropped everywhere. They are dropped on the parent's own edges only — a cube extending it still resolves through one of them, which the comment now says instead of implying coverage that is not there. Also cover two cubes each declaring their own duplicate, which pins the result down as per-cube. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @waralexrom's task in 1m 11s —— View job Re-review of
|
| # | Prior finding | Status |
|---|---|---|
| 1 | Comment claimed the extends case was covered |
Fixed. JoinGraph.ts:176-179 now says "dropped on the parent's own edges; a cube extending it still resolves through one of them" — that is exactly what the code does, and the next reader is no longer told the gap is closed. |
| 2 | duplicateJoinTargets named as a pure query while it reports errors |
Fixed. Renamed reportDuplicateJoinTargets, so the side effect is visible at the call site (JoinGraph.ts:128) as well as at the definition. The JSDoc also lost its first line, which only restated the old name; what remains states the constraint (one edge per pair of cubes), which is the load-bearing part. |
Both were low, and both are now closed the way I'd have written them.
The new test
reports every cube that declares duplicates (duplicate-cube-joins.test.ts:100) is the right addition for this commit: with detection derived per cube inside buildJoinEdges, "the set is not shared across cubes" is the property that could plausibly regress, and two cubes with different duplicate targets (orders→users, tickets→managers) pins it down rather than just re-asserting one message. compileError returns the aggregated e.message, so both toContain assertions read from the same throw — correct, since throwIfAnyErrors collects across cubes.
Checked the fixture holds up on its own: tickets declares a primary key and no measures, managers has none either, so neither trips the joinRequired path and the two assertions can only be satisfied by the duplicate rule.
Ten tests now, and the split still looks right — three rejection shapes, four must-keep-compiling shapes, three behavioural ones under omitErrors.
Still open, deliberately
My thread on JoinGraph.ts:135 — a cube that only extends an offender has no own joins, so duplicates is empty for it while its merged cube.joins still holds both parent declarations, and one becomes an edge under omitErrors: true. Left unresolved because the defect is real, but I don't think it should be closed here: the honest fix is to make the joins getter collapse overrides instead of appending, which touches an inheritance path every extends model goes through. ab57772 does the right thing for this PR by documenting the boundary rather than implying coverage.
Resolved my comment-length/naming thread as superseded. @ovr's two threads are outdated against the current diff (CubeValidator.ts is no longer touched at all) but are a human reviewer's to close.
Not verified
node_modules is absent in this checkout and a monorepo yarn install is out of scope for this job, so I could not run test/unit/duplicate-cube-joins.test.ts — the assessment is from reading the code and tracing the compile phases. CI is the check that matters.
2 participants
Problem
A cube may declare several joins to the same cube today, and the model still
compiles. All but one of those joins are then dropped without a word: queries
run, results look plausible, and they are produced through a join path the
author never wrote. There is no way to notice this from the outside — no error,
no warning, no difference in the API.
Cause
JoinGraphkeys its edges by the pair of cubes a join connects(
${cube.name}-${join.name}) and builds the edge map withR.fromPairs, soseveral declarations for the same pair collapse into one entry and the last one
wins. The dropped declarations leave no trace.
What was done
JoinGraph.buildJoinEdgesnow reports this as a model compilation error:The message names the file, the cube, every conflicting declaration by its
position in the list, and the way out.
The check sits in the function that builds the edges, next to the collapse it
guards against and alongside the two failures already handled there the same way
(a join to a cube that does not exist, a missing primary key): report it, and
leave the join out. So the failure is scoped to the join rather than the cube —
the cube stays valid.
The strict path fails on the reported error. An embedder compiling with
omitErrors: truekeeps a cube that serves its own members and has lost only theambiguous join, and none of the conflicting declarations is used, so a query
needing that join fails loudly with
Can't find join pathinstead of quietlypicking one. Invalidating the cube instead would have made every other cube
joining to it report
Cube orders doesn't exist, which is both false andunrelated to the defect.
One corner is left open: a cube that inherits an ambiguous
joinsblock throughextendswithout declaring any joins of its own still resolves through thecollapsed edge under
omitErrors: true. The error is still reported, on the cubethat declares the duplicates; deciding it for the child means walking the
extendschain to find which level last declared the target, which is moremachinery than the case is worth.
The rule is documented in the joins troubleshooting page, together with the
extendsrecipe for joining the same table through two different keys.Only the joins a cube declares itself are checked.
extendsappends a child'sjoins to the inherited ones, and a child redeclaring an inherited join replaces
it — that stays valid, and duplicates a parent declares are reported once, on
the parent. The join map form (
joins: { users: { … } }) is keyed by the joinedcube name and cannot hold duplicates in the first place.
The check deliberately stops at "one join per joined cube". Allowing several
aliased joins to the same cube needs join aliases, which do not exist yet;
relaxing the condition then means loosening a single
ifin one place.Compatibility
This is a behavior change: models that compile today start failing. The
shape that breaks is exactly one — a single cube listing the same joined cube
more than once in its own
joinslist, which is only expressible in the YAML /array form. Every such model is already broken in a worse way: it runs through a
join path its author did not write, and the two forms are indistinguishable from
the API. Failing at model compilation, with the conflicting declarations named,
is strictly more informative than losing a join in silence.
Blast radius in this repository: zero. A scan of all 255 list-form
joinsblocks across packages, examples and docs — 51 of which declare more than one
join — found no duplicate target outside the new test, and the schema-compiler
unit suite is unchanged (960 passing; the 2
remaining
error-reportersnapshot failures are pre-existing ANSI-colourmismatches, present on
mastertoo). Scaffolding is unaffected: it reducesgenerated joins into a map keyed by the joined cube before rendering, so two
foreign keys to the same table already produce a single join.
Deployments that hit this will need a data model change — dropping the extra
join, or using
extendsto create a second cube over the same table.How it was verified
duplicate-cube-joins.test.ts: 3 rejection cases and 4 shapesthat must keep compiling (joins to different cubes, the same pair joined from
both sides, a transitive path, an
extendsoverride). Without the fix the 3rejection cases fail, the other 4 pass.
CUBEJS_TESSERACT_SQL_PLANNER=trueand
=false(native addon built). The error is raised while the data model iscompiled, before any query planning, so it does not depend on the planner.
packages/cubejs-schema-compilerunit suite: 960/962, the 2 failurespre-existing on
master.the JS array form.
extendsrecipe the docs now recommend: verified it compiles and emitstwo distinct
ONclauses against the same table.Risks
The error fires at model compilation, so a deployment carrying this shape stops
serving until its data model is fixed, rather than serving quietly wrong join
paths. That is the intended trade-off, but it is a breaking change for anyone
who has the shape in production.
A review of this diff suggested softening it to
errorReporter.warningfor onerelease, or gating it behind an env flag, so operators would see it in logs
before it turns fatal. That is deliberately not done here: a warning is still a
model that silently uses a join path nobody wrote, which is the thing this
change exists to end. Whether the rollout deserves a grace period is a release
decision rather than a code one, and the warning path stays a one-line change if
that call is made.
🤖 Generated with Claude Code