fix: use tablesSchemaV2 for Generate Data Model so PKs are detected (Playground + CLI) - #11806
Open
haechangcho wants to merge 2 commits into
Open
haechangcho wants to merge 2 commits into
haechangcho wants to merge 2 commits into
Conversation
…re detected
/playground/db-schema and /playground/generate-schema both called
driver.tablesSchema(), which only queries information_schema.columns and
never merges primary/foreign key metadata. ScaffoldingSchema only marks a
dimension as a primary key via column.attributes.includes('primaryKey'),
which tablesSchema() never sets — so Generate Data Model never emits
primary_key: true for any table whose PK column isn't literally named `id`.
Any generated cube that also has a join then fails to compile with
"primary key for 'X' is required when join is defined".
driver.tablesSchemaV2() (BaseDriver) already exists for exactly this: it
calls tablesSchema() and merges in primaryKeys()/foreignKeys(), returning
the same DatabaseStructure shape with attributes/foreign_keys added where
applicable. Swapping both call sites to it is a backward-compatible
superset with no other behavior change.
Fixes cube-js#10517
github-actions
Bot
added
javascript
3 tasks
Author
|
@paveltiunov — since you've already looked closely at the manufacturing-KPI schema issue we filed (#11680), flagging this one too: this is a small, self-contained fix (2 lines) for #10517. Would appreciate a workflow approval so CI can actually run, and a look when you have a moment. Happy to make any changes requested. |
The `cubejs generate -t ...` CLI command has the same bug as the Playground endpoints fixed in the previous commit: it calls driver.tablesSchema() and feeds the result into ScaffoldingTemplate, which never sets a column's primaryKey attribute, so generated cubes never get `primary_key: true` unless the PK column is literally named `id`. @paveltiunov already diagnosed this exact call site (and this exact fix) in cube-js#11270. Fixes cube-js#11270
haechangcho
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Check List
Issue Reference this PR resolves
Fixes #10517, Fixes #11270
Description of Changes Made (if issue reference is not provided)
Three call sites feed
ScaffoldingSchema/ScaffoldingTemplatewithdriver.tablesSchema(), which only queriesinformation_schema.columnsand never setsattributes.ScaffoldingSchema.dimensions()decidesisPrimaryKeypurely fromcolumn.attributes?.includes('primaryKey'), so Generate Data Model never emittedprimary_key: trueunless a table's PK column happened to be namedid. Any generated cube that also had ajointhen failed to compile withprimary key for 'X' is required when join is defined.BaseDriver.tablesSchemaV2()already exists for exactly this (added in #8115) — it callstablesSchema()and merges inprimaryKeys()/foreignKeys(), returning the same shape withattributes/foreign_keysadded. It was defined but never actually called from any production code path; only a unit test exercised the PK-consuming logic directly with hand-builtattributesdata, so the missing wiring went unnoticed.@paveltiunov independently reproduced and diagnosed this exact bug in #11270, tracing it down to
informationColumnsSchemaReducer()always producingattributes: []and confirmingtablesSchemaV2()as the fix — this PR is that fix, plus a third call site the analysis there flagged but that wasn't covered yet:packages/cubejs-server-core/src/core/DevServer.ts—/playground/db-schemaand/playground/generate-schema(first commit)packages/cubejs-cli/src/command/generate.ts— thecubejs generate -t ...CLI command (second commit)All three swapped to
tablesSchemaV2()— a backward-compatible superset, no other behavior change.Notes
driveris typed asBaseDriverin all three call sites, which declarestablesSchemaV2()publicly, so no capability guard is needed.DevServer.ts's or the CLI command's wiring directly; the PK-detection logic itself (attributes.includes('primaryKey')→isPrimaryKey) is already covered byscaffolding-schema.test.ts/scaffolding-template.test.ts.