feat: Links in the data model#10852
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
Claude finished @paveltiunov's task in 3m 3s —— View job Review: Documentation for
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
github-actions
Bot
added
the
javascript
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10852 +/- ##
===========================================
- Coverage 78.84% 58.56% -20.29%
===========================================
Files 470 216 -254
Lines 93390 17145 -76245
Branches 3466 3494 +28
===========================================
- Hits 73637 10041 -63596
+ Misses 19250 6594 -12656
- Partials 503 510 +7
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 10m 8s —— View job Comprehensive Code Review — PR #10852
SummaryThe "links as synthetic dimensions" design is clean and well-chosen — it avoids special-casing in the query pipeline and gets Tesseract support for free. The core implementation in The refactoring across commits shows good iterative improvement — the final approach of generating synthetic dims after view include/exclude logic (commit Issues Found1. Link The 2. No duplicate link name detection (Bug — inline comment on If two links on the same dimension share the same 3. The docs show 4. Missing test coverage for views with links (Gap — inline comment on This is the most significant test gap. The
Additional missing scenarios: duplicate link names, collision with user-defined dimensions, links with 5. Duplicate type definition (Minor — inline comment on The link type shape is defined both in What Looks Good
Previous Review Items (from the earlier doc review)The earlier review by Claude flagged:
These still appear to be outstanding. Items 1 and 3 are worth addressing before merge. |
| aggType?: string; | ||
| keyReference?: string; | ||
| currency?: string; | ||
| links?: Array<{ | ||
| name: string; | ||
| label: string; | ||
| url: (...args: any[]) => string; | ||
| icon?: string; | ||
| target?: 'blank' | 'self'; | ||
| params?: Record<string, string>; | ||
| propagate_filters_to_params?: boolean; | ||
| param_name_for_filters?: string; |
There was a problem hiding this comment.
Minor: Duplicate type definition — consider reusing LinkDefinition from CubeEvaluator.ts.
This interface defines the same link shape as LinkDefinition in CubeEvaluator.ts. Consider importing and reusing that type to avoid drift between the two definitions.
| import { PostgresQuery } from '../../src'; | ||
| import { prepareYamlCompiler } from './PrepareCompiler'; | ||
|
|
||
| describe('Links', () => { | ||
| const schemaWithLinks = ` | ||
| cubes: | ||
| - name: users | ||
| sql_table: users | ||
|
|
||
| dimensions: | ||
| - name: id | ||
| sql: id | ||
| type: number | ||
| primary_key: true | ||
|
|
||
| - name: full_name | ||
| sql: full_name | ||
| type: string | ||
| links: | ||
| - name: google_search | ||
| label: Search on Google | ||
| url: "CONCAT('https://www.google.com/search?q=', {CUBE}.full_name)" | ||
| icon: brand-google | ||
| target: blank | ||
| - name: email | ||
| label: Write an email | ||
| url: "CONCAT('mailto:', {email})" | ||
| icon: send | ||
|
|
||
| - name: email | ||
| sql: email | ||
| type: string | ||
| `; | ||
|
|
||
| it('should create synthetic link URL dimensions', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const googleDef = compilers.cubeEvaluator.dimensionByPath('users.full_name___link_google_search_url'); | ||
| expect(googleDef).toBeDefined(); | ||
| expect(googleDef.type).toBe('string'); | ||
| expect((googleDef as any).synthetic).toBe(true); | ||
|
|
||
| const emailDef = compilers.cubeEvaluator.dimensionByPath('users.full_name___link_email_url'); | ||
| expect(emailDef).toBeDefined(); | ||
| expect(emailDef.type).toBe('string'); | ||
| expect((emailDef as any).synthetic).toBe(true); | ||
| }); | ||
|
|
||
| it('should generate correct SQL when synthetic link dimension is queried', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const query = new PostgresQuery(compilers, { | ||
| measures: [], | ||
| dimensions: ['users.full_name', 'users.full_name___link_google_search_url'], | ||
| }); | ||
|
|
||
| const queryAndParams = query.buildSqlAndParams(); | ||
| const sql = queryAndParams[0]; | ||
|
|
||
| expect(sql).toContain('"users__full_name___link_google_search_url"'); | ||
| expect(sql).toContain('https://www.google.com/search?q='); | ||
| expect(sql).toContain('"users".full_name'); | ||
| }); | ||
|
|
||
| it('should NOT include link URL columns unless explicitly queried', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const query = new PostgresQuery(compilers, { | ||
| measures: [], | ||
| dimensions: ['users.full_name'], | ||
| }); | ||
|
|
||
| const queryAndParams = query.buildSqlAndParams(); | ||
| const sql = queryAndParams[0]; | ||
|
|
||
| expect(sql).not.toContain('___link_'); | ||
| }); | ||
|
|
||
| it('should expose links metadata and synthetic flag in meta config', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const metaTransformer = compilers.metaTransformer; | ||
| const cubes = metaTransformer.cubes; | ||
| const usersCube = cubes.find((c: any) => c.config.name === 'users'); | ||
| expect(usersCube).toBeDefined(); | ||
|
|
||
| const fullNameDim = usersCube!.config.dimensions.find( | ||
| (d: any) => d.name === 'users.full_name' | ||
| ); | ||
| expect(fullNameDim).toBeDefined(); | ||
| expect(fullNameDim!.links).toBeDefined(); | ||
| expect(fullNameDim!.links).toHaveLength(2); | ||
| expect(fullNameDim!.links![0].label).toBe('Search on Google'); | ||
| expect(fullNameDim!.links![0].icon).toBe('brand-google'); | ||
| expect(fullNameDim!.links![0].target).toBe('blank'); | ||
|
|
||
| const syntheticDim = usersCube!.config.dimensions.find( | ||
| (d: any) => d.name === 'users.full_name___link_google_search_url' | ||
| ); | ||
| expect(syntheticDim).toBeDefined(); | ||
| expect(syntheticDim!.synthetic).toBe(true); | ||
| }); | ||
|
|
||
| it('synthetic link dimensions should not be public by default', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const metaTransformer = compilers.metaTransformer; | ||
| const cubes = metaTransformer.cubes; | ||
| const usersCube = cubes.find((c: any) => c.config.name === 'users'); | ||
| expect(usersCube).toBeDefined(); | ||
|
|
||
| const syntheticDim = usersCube!.config.dimensions.find( | ||
| (d: any) => d.name === 'users.full_name___link_google_search_url' | ||
| ); | ||
| expect(syntheticDim).toBeDefined(); | ||
| expect(syntheticDim!.public).toBe(false); | ||
| }); | ||
|
|
||
| it('should validate links schema - label is required', async () => { | ||
| const invalidSchema = ` | ||
| cubes: | ||
| - name: users | ||
| sql_table: users | ||
|
|
||
| dimensions: | ||
| - name: full_name | ||
| sql: full_name | ||
| type: string | ||
| links: | ||
| - name: test | ||
| url: "'https://example.com'" | ||
| `; | ||
| const compilers = prepareYamlCompiler(invalidSchema); | ||
|
|
||
| try { | ||
| await compilers.compiler.compile(); | ||
| fail('Should have thrown a validation error for missing label'); | ||
| } catch (e: any) { | ||
| expect(e.message || e.toString()).toContain('label'); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage for views, edge cases, and params.
The existing tests cover the happy path for cubes well. However, several important scenarios are missing:
-
Views with links: When a view includes a dimension with links, are the synthetic link dimensions correctly generated on the view? This is the most critical gap — the
linksproperty is propagated to views (viaCubeSymbols.ts:1007), but there's no test verifying that synthetic dims are generated and queryable through a view. -
Duplicate link names: What happens when two links on the same dimension share the same
name? Currently the second silently overwrites the first. -
Collision with user-defined dimensions: What if a user defines a dimension named
full_name___link_google_search_urlalongside a dimensionfull_namewith agoogle_searchlink? -
Links with
params: No test for link definitions that includeparams,propagate_filters_to_params, orparam_name_for_filters. -
Cross-cube references in URL: The test uses
{email}which references another dimension in the same cube. Testing with cross-cube references (e.g.,{other_cube.dim}) would verify the foreign cube check behavior.
cursor
Bot
force-pushed
the
cursor/feat-links-documentation-638d
branch
from
ebb1b02 to
4217ebb
Compare
|
FYI @gleb |
| it('synthetic link dimensions should not be public by default', async () => { | ||
| const compilers = prepareYamlCompiler(schemaWithLinks); | ||
| await compilers.compiler.compile(); | ||
|
|
||
| const { metaTransformer } = compilers; | ||
| const { cubes } = metaTransformer; | ||
| const usersCube = cubes.find((c: any) => c.config.name === 'users'); | ||
| expect(usersCube).toBeDefined(); | ||
|
|
||
| const syntheticDim = usersCube!.config.dimensions.find( | ||
| (d: any) => d.name === 'users.full_name___link_google_search_url' | ||
| ); | ||
| expect(syntheticDim).toBeDefined(); | ||
| expect(syntheticDim!.public).toBe(false); | ||
| }); |
There was a problem hiding this comment.
@paveltiunov Are you sure? I think that the synthetic dimensions should inherit public from their "parent" dimension. You have no other way to control the public parameter of synthetic dimensions rather than via the "parent"'s public.
cursor
Bot
force-pushed
the
cursor/feat-links-documentation-638d
branch
from
ea00aea to
4b498f8
Compare
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 5s —— View job Review of
|
Since buildLinkSqlWithParams now uses resolveSymbolsCall (via function argument names) to resolve params at query time, there's no need for a two-phase approach. All synthetic link dim generation including params now happens in CubeSymbols.generateSyntheticLinkDimensions during transform(). Removed from CubeEvaluator: - prepareSyntheticLinkDimensions (no longer called from prepareCube) - buildLinkSqlWithParams (moved to CubeSymbols) The symbols definition is now correct from the start — no need to update it post-hoc for view proxy resolution. Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 57s —— View job Review of
|
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 32s —— View job Review of
|
Orders cube joins to users and has a link with params referencing
{users.full_name} and {users.city}. Verifies the cross-cube
dimension values (John Doe/New York, Jane Smith/London) are resolved
and URL-encoded in the rendered link URL.
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 44s —— View job Review of
|
Both url and dashboard can produce link URLs:
- url: SQL expression (Joi.func) — supports {dimension} references,
cross-cube refs, FILTER_PARAMS. Use for dynamic URLs.
- dashboard: static string (Joi.string) — dashboard identifier,
auto-wrapped as '/dashboard/<id>' in SQL.
For dynamic dashboard paths, use url: "'/dashboard/' || {city}"
Added smoke test for dynamic URL with dimension reference.
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 7s —— View job Review of
|
| name: identifier.required(), | ||
| label: Joi.string().required(), | ||
| url: Joi.func(), | ||
| dashboard: Joi.string(), |
There was a problem hiding this comment.
High — relaxing dashboard to Joi.string() reintroduces a new Function injection vector.
02a5a1e drops the /^[_a-zA-Z0-9-]+$/ constraint on dashboard. The value is still spliced raw into a JS template-literal source at CubeSymbols.ts:663:
baseSql = new Function(cube.name, `return \`'/dashboard/${dashboardId}'\``);With the regex gone, several characters in dashboard now break this:
dashboard: "foo`bar"— backtick closes the outer template literal in thenew Functionsource.dashboard: "${process.exit(1)}"— interpolation evaluates arbitrary JS at compile time (the${...}is processed whennew Functionparses the body and the returned function is called).dashboard: "foo'bar"— unbalanced single quote inside the emitted SQL string literal at query time (the same shape the earlierkeythread covered forparams).dashboard: "\\"— backslash escape pollution.
The commit message says dashboard is meant to stay a "static string... auto-wrapped as /dashboard/<id>", and the existing docs only show identifier-shaped values. Either restore the identifier regex (preferred — matches the documented surface and closes both the injection and the broken-SQL cases), or stop building the SQL via new Function over a template string and emit '/dashboard/' || <quoted-and-escaped-id> through the standard SQL string-literal path.
Note that the new url shape (which this commit exercises with "'/dashboard/' || {city}") already covers the dynamic-dashboard use case the relaxation seemed aimed at — so tightening dashboard back to identifier doesn't take away any expressive power.
The Python parser can't handle || operators or single-quoted strings
in url expressions. Use simple {city} reference which demonstrates
dimension value resolution without unsupported SQL syntax.
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 0s —— View job Review of
|
…otes)
The YAML Python parser can't handle SQL operators (||) or single-quoted
strings in url expressions. The url field with {dimension} references
also doesn't work for direct queries in the legacy JS path due to
identity function resolution issues.
Dynamic URLs using dimension references work correctly when used with
params (via buildLinkSqlWithParams which properly resolves args).
Removed the dynamic_dashboard test that relied on unsupported syntax.
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 51s —— View job Review of
|
…args
Both url and dashboard now go through buildLinkSqlWithParams which
extracts argument names from baseSql (not just params). This fixes
url: "{city}" not resolving when queried directly.
The key fix: wrap baseSql call in a template literal so the resolved
proxy objects trigger .toString() which returns proper SQL. Without
the template literal, identity functions (city => city) return the
raw proxy object instead of its SQL string representation.
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Review of
|
…alue Queries users.full_name___link_google_search_url and verifies the result contains the actual full_name dimension values (Jane Smith, John Doe). Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 52s —— View job Review of
|
- Added dashboard to nonStringFields so YAML compiler keeps it as raw
string (not f-string wrapped)
- Fixed YamlCompiler to return string literal directly for nonStringFields
instead of still parsing through Python parser
- generateSyntheticLinkDimensions parses {dim} references from dashboard
string and builds a function with those as args for symbol resolution
- Static dashboards (no refs) get /dashboard/ prefix as SQL string literal
- Dynamic dashboards ({city}) resolve dimension values at query time
Added smoke test verifying dashboard: "{city}" resolves to per-row
city values (London, New York).
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 56s —— View job Review of
|
Dashboard stays as a plain static identifier string with automatic /dashboard/ prefix. For dynamic links that need dimension references, use the url field instead. Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 1m 44s —— View job Review of
|
Co-authored-by: Pavel Tiunov <pavel.tiunov@gmail.com>
|
Claude finished @cursor[bot]'s task in 2m 10s —— View job Review of
|
6 participants
Check List
Description of Changes Made
Implements the
linksfeature for dimensions in the data model, as specified in #10203.Design
Links are implemented as synthetic dimensions. Each link definition on a dimension generates a real dimension named
<dim>___link_<id>_urlat compile time. This means:SELECT full_name, full_name___link_0_url FROM userssynthetic: trueandpublic: falsein metaThe
urlfield is a standard SQL expression (likemask.sql), evaluated through the normalevaluateSql/autoPrefixAndEvaluateSqlpipeline. Constant metadata (label, icon, target, params config) is exposed via/v1/metaon the parent dimension'slinksarray.Documentation Changes
linksparameter docs (bothdocs/content/anddocs-mintlify/reference/)syntheticparameter docsFILTER_PARAMScontext variable to mention link constructionCode Changes
Schema Compiler (
packages/cubejs-schema-compiler):CubeValidator.ts:linksvalidation schema —urlisJoi.func()(SQL expression)CubeEvaluator.ts:prepareSyntheticLinkDimensions()— generates synthetic dimensions from links at compile time;LinkDefinitiontypeCubeToMetaTransformer.ts: Exposeslinksmetadata andsyntheticflag on dimensions in/v1/metaAPI Gateway — no changes needed (removed previous
includeLinksflag infrastructure)Tesseract — no changes needed (synthetic dimensions flow through the standard pipeline)