fix(schema-compiler): Throw error for duplicate cube and view names in JS models#10909
fix(schema-compiler): Throw error for duplicate cube and view names in JS models#10909paveltiunov wants to merge 1 commit into
Conversation
…n JS models Previously, when two cubes or views shared the same name in JS data model files, the later definition would silently overwrite the earlier one. This was already fixed for YAML models but not for JS models. This adds duplicate name detection in CubeDictionary.compile() which runs during stage 0 of compilation, catching: - Duplicate cube names (same name used twice for cubes) - Duplicate view names (same name used twice for views) - Conflicting cube and view names (a cube and view sharing the same name) The check works for both single-file and cross-file duplicates. Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
github-actions
Bot
added
the
javascript
|
Claude finished @paveltiunov's task in 2m 34s —— View job Code Review: PR #10909
Note: Unable to run tests due to missing tool permissions ( SummaryClean, well-scoped fix that adds duplicate cube/view name detection for JS models by placing the check in Detailed ReviewCode Quality — The implementation is concise and correct. The One minor observation: After reporting the duplicate error, the code still executes Error messages — The messages are clear and consistent with the existing YAML duplicate messages. One improvement to consider for a future PR: including file names in the error message (e.g., "Found duplicate cube name 'orders' (defined in orders1.js and orders2.js)") would help users locate the conflict faster, especially in large projects with many model files. Not a blocker. Testing — 5 tests covering:
Test structure uses Potential gaps (non-blocking):
Security — No concerns. This is a compile-time check with no user input flowing to dangerous sinks. Performance — Negligible. Single VerdictLooks good. The placement in |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #10909 +/- ##
===========================================
- Coverage 83.47% 58.41% -25.07%
===========================================
Files 254 216 -38
Lines 75490 16956 -58534
Branches 0 3440 +3440
===========================================
- Hits 63014 9904 -53110
+ Misses 12476 6551 -5925
- Partials 0 501 +501
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:
|
|
Claude finished @paveltiunov's task in 2m 17s —— View job Code Review: PR #10909
SummaryLooks good. Clean, well-scoped fix that adds duplicate cube/view name detection for JS models by placing the check in Key FindingsArchitecture (positive): The Code quality: Implementation is concise — Minor observations (all non-blocking, posted as inline comments):
Security: No concerns. Compile-time check with no user input flowing to dangerous sinks. Performance: Negligible. Single VerdictApprove. The inline comments are all non-blocking suggestions for future improvement. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (errorReporter && this.byId.has(cube.name)) { | ||
| const existing = this.byId.get(cube.name)!; | ||
| const existingType = existing.isView ? 'view' : 'cube'; | ||
| const newType = cube.isView ? 'view' : 'cube'; | ||
| if (existingType === newType) { | ||
| errorReporter.error(`Found duplicate ${newType} name '${cube.name}'.`); | ||
| } else { | ||
| errorReporter.error(`Found conflicting cube and view name '${cube.name}'.`); | ||
| } |
There was a problem hiding this comment.
Nit: After reporting the duplicate error, line 30 still executes this.byId.set(cube.name, cube), silently overwriting the earlier definition with the later one. This is fine in practice because throwIfAnyErrors() will halt compilation. However, if omitErrors is true (used in some contexts), duplicates are swallowed and the last-definition-wins behavior persists — same as before this PR, so no regression, but worth a brief comment or a continue to make the intent explicit.
Also worth noting: the YamlCompiler.checkDuplicateNames() catches within-file YAML duplicates during the transpile phase, while this new check catches cross-file duplicates (and JS duplicates) during stage 0 compilation. For YAML within-file duplicates, both checks will fire, but ErrorReporter.error() deduplicates identical messages (at ErrorReporter.ts:143), so no double-reporting occurs. Good design.
One future improvement to consider: including file names in the error message (e.g., Found duplicate cube name 'orders' (defined in orders1.js and orders2.js)) would help users locate conflicts faster in large projects. The cube.fileName and existing.fileName are already available on the objects. Not a blocker for this PR.
| try { | ||
| await compiler.compile(); | ||
| throw new Error('compile must return an error'); | ||
| } catch (e: any) { | ||
| expect(e.message).toContain("Found duplicate cube name 'orders'"); | ||
| } |
There was a problem hiding this comment.
Minor: The try/catch + throw new Error('compile must return an error') pattern works but is fragile — if the error message changes slightly, the toContain still passes but the test doesn't verify the compile actually failed for the right reason.
Consider using expect(...).rejects.toThrow(...) which is the idiomatic Jest pattern for async error assertions:
| try { | |
| await compiler.compile(); | |
| throw new Error('compile must return an error'); | |
| } catch (e: any) { | |
| expect(e.message).toContain("Found duplicate cube name 'orders'"); | |
| } | |
| await expect(compiler.compile()).rejects.toThrow("Found duplicate cube name 'orders'"); |
This is cleaner and avoids the risk of a false-positive if compile() throws a different error that happens to contain the substring. Same applies to the other four tests in this describe block. Non-blocking.
| expect(e.message).toContain("Found conflicting cube and view name 'orders'"); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage (non-blocking):
-
Duplicate view names across files — only single-file view duplicates are tested (line 1422). A cross-file view duplicate test would round out the matrix:
it('detects duplicate view names across JS files', async () => { ... });
-
Cross-format duplicates (JS cube + YAML cube with same name) — the
CubeDictionarycheck is format-agnostic, so it would catch this, but a test would serve as documentation that this is intentional.
Neither is a blocker since the underlying Map.has() check is identical for all cases.
2 participants
Check List
Description of Changes Made
Previously, when two cubes or views shared the same name in JS data model files, the later definition would silently overwrite the earlier one. This was already fixed for YAML models (in
YamlCompiler.ts) but not for JS models.This PR adds duplicate name detection in
CubeDictionary.compile()which runs during stage 0 of compilation (the first phase that processes all collected cube/view definitions). It catches:The detection is placed in
CubeDictionarybecause it's the first compiler to receive the complete list of cubes/views regardless of source format (JS or YAML). This provides a unified safety net for all model types.Error messages:
Found duplicate cube name '<name>'.Found duplicate view name '<name>'.Found conflicting cube and view name '<name>'.Tests added: 5 new test cases covering single-file duplicates, cross-file duplicates, and cube/view name conflicts.