From 2d4504b0f79bec3c23f09af24c46d0bf02d4614e Mon Sep 17 00:00:00 2001 From: Fei Xu Date: Thu, 22 Oct 2026 00:28:13 +0000 Subject: [PATCH 1/2] feat(mdcode): deploy the deployments that --deployment names, after every check --- .../mdcode/docs/semantic-model/profiles.md | 7 +- .../mdcode/docs/semantic-model/reference.md | 20 +- toolbox/mdcode/src/libts/gcp/spanner.ts | 12 +- .../src/libts/semantic/deploy_bigquery.ts | 2 +- .../src/libts/semantic/deployment_target.ts | 4 +- .../src/libts/semantic/runtime/runtime.ts | 4 +- .../src/libts/semantic/runtime/store.ts | 11 +- toolbox/mdcode/src/libts/semantic/skills.ts | 4 +- toolbox/mdcode/src/libts/semantic/validate.ts | 2 +- toolbox/mdcode/src/tool/commands.ts | 449 ++++++++++++++---- toolbox/mdcode/src/tool/main.ts | 8 +- .../tests/tool/two_phase/deploy.test.ts | 396 ++++++++++++++- .../mdcode/tests/tool/two_phase/harness.ts | 71 ++- .../tool/two_phase/two_phase_path.test.ts | 82 +++- 14 files changed, 941 insertions(+), 131 deletions(-) diff --git a/toolbox/mdcode/docs/semantic-model/profiles.md b/toolbox/mdcode/docs/semantic-model/profiles.md index d2266bb44..1f2766530 100644 --- a/toolbox/mdcode/docs/semantic-model/profiles.md +++ b/toolbox/mdcode/docs/semantic-model/profiles.md @@ -247,9 +247,10 @@ catalog/EntryGroups/commerce_eg/ The `` in the file name must match the `name:` inside the file, and `default` is reserved for the inline bindings. A profile file carries `name`, `entities`, `relationships`, `metrics_exclude` and `actions`, and names no -deployment target. For now `kcmd push` refuses a deployment that names a -profile, except under `--no-deployment`, and refuses to deploy a graph for a -profile file. It says so before it deploys anything. +deployment target. `kcmd push --deployment` deploys each deployment it names +with the profile that deployment names. A push without `--deployment` or +`--no-deployment` refuses a deployment that names a profile, and refuses to +deploy a graph for a profile file. It says so before it deploys anything. `kcmd profiles --print-store` and `kcmd skills-generate` take a profile file's store from the deployments that name the profile in `profile`. When none does, they report that there is no database to run it diff --git a/toolbox/mdcode/docs/semantic-model/reference.md b/toolbox/mdcode/docs/semantic-model/reference.md index d84703a97..8af95c2f1 100644 --- a/toolbox/mdcode/docs/semantic-model/reference.md +++ b/toolbox/mdcode/docs/semantic-model/reference.md @@ -40,15 +40,21 @@ whether the catalog leg runs. | `--all-profiles` | Deploy the graph for every defined binding profile (plus the inline bindings when the document itself declares a target), instead of a single one. The Knowledge Catalog leg still records one canonical view — the default binding. Mutually exclusive with `--profile` and `--no-profile`. | | `--no-profile` | Deploy the graph for no binding profile: publish only the logical model to Knowledge Catalog, leaving any deployed graph untouched. | | `--no-kc` | Skip the Knowledge Catalog metadata push and deploy only the graph the selected profile targets. Knowledge Catalog is pushed by default. A push that deploys no graph (a logical model, or `--no-profile`) can only reach Knowledge Catalog, so `--no-kc` on it is an error, as is `--no-profile --no-kc` (nothing left to deploy). | -| `--no-deployment` | Publish the model to Knowledge Catalog and deploy nothing, after every check of the two-phase path below. It publishes the model file as written, whereas `--no-profile` publishes the binding `default_profile` names. | +| `--deployment ` | Deploy only these entries of the model's `deployments:` list, comma-separated or repeated, in the order given. Push still publishes the model to Knowledge Catalog first. | +| `--no-deployment` | Publish the model to Knowledge Catalog and deploy nothing, after every check of the two-phase path below. It publishes the model file as written, whereas `--no-profile` publishes the binding `default_profile` names. Push refuses it with `--deployment`, in either order. | | `--validate-only` | Run every validation check and report pass/fail, but write nothing. | | `--print` | Print each destination's generated artifact (BigQuery or Spanner Graph SQL DDL, Knowledge Catalog entry plan). Combine with `--validate-only` to preview without deploying. | | `--force-remove` | Delete models in the entry group that this push no longer includes (see [Updating and removing models](README.md#updating-and-removing-models)). | | `--emit-expressions` | Also write the SQL-expression fields (per-field `schema.semantics` and `semantic-metric.expression`) to Knowledge Catalog. Off by default: the published system-type templates do not carry them yet. Knowledge Catalog push only. | -A push with `--no-deployment` runs in two phases. The first runs every check -for Knowledge Catalog and writes nothing. The second publishes the model to -Knowledge Catalog. On this path `--no-kc`, `--profile`, +A push with `--deployment` or `--no-deployment` runs in two phases. The first +runs every check, for Knowledge Catalog and for each deployment, and writes +nothing. The second publishes the model to Knowledge Catalog, then deploys each +deployment in order with the profile it names. An AlloyDB deployment deploys no +graph. The first phase dry-runs each BigQuery graph's DDL. Spanner cannot check +a graph's DDL without running it, so a Spanner deployment whose DDL Spanner +rejects, for example one that reads a missing column, fails in the second phase, +after Knowledge Catalog is written. On this path `--no-kc`, `--profile`, `--all-profiles` and `--no-profile` are errors, and catalog.yaml's `default_profile` chooses no binding: Knowledge Catalog receives the model file as written. @@ -407,9 +413,11 @@ differs from BigQuery Graph in five ways: - **Bare table and graph names.** A Spanner property graph lives inside one database and names tables in that same database, so the generator emits bare names — no `project.dataset.` qualifier on either the tables or the graph. -- **No `MEASURE`.** Metrics are dropped (warned per metric), never errored. The +- **No `MEASURE`.** Metrics are dropped (warned per metric). The BigQuery-only rule that a metric must resolve to a single entity therefore does - not apply to a Spanner target (see [Validation](#validation)). + not apply to a Spanner target (see [Validation](#validation)). Push refuses a + Spanner deployment that names no profile in a model with metrics; name a + profile with `metrics_exclude: "*"`. This holds for a push with `--deployment`. - **No per-element `OPTIONS`.** `description` / `synonyms` are not emitted into the Spanner DDL; they ride into Knowledge Catalog instead — mirroring how BigQuery's graph-level `OPTIONS` is dropped. See diff --git a/toolbox/mdcode/src/libts/gcp/spanner.ts b/toolbox/mdcode/src/libts/gcp/spanner.ts index 54f8fe36d..fb6ff9428 100644 --- a/toolbox/mdcode/src/libts/gcp/spanner.ts +++ b/toolbox/mdcode/src/libts/gcp/spanner.ts @@ -1,6 +1,7 @@ // API client for Cloud Spanner, covering two surfaces the semantic-model push -// leg needs: Database Admin (updateDatabaseDdl and getOperation) to deploy -// Spanner Graph DDL, and the data plane (sessions and executeSql) to PLAN an +// leg needs: Database Admin (updateDatabaseDdl, getDatabase and getOperation) +// to check the database and deploy Spanner Graph DDL, and the data plane +// (sessions and executeSql) to PLAN an // action's DML against the live database before publishing it. import * as api from './api'; @@ -62,6 +63,13 @@ export class SpannerClient extends api.ApiClient { return await this._patch(name, {statements}); } + // Fetches a database's metadata. + async getDatabase(project: string, instance: string, database: string): + Promise> { + return await this._get( + `projects/${project}/instances/${instance}/databases/${database}`); + } + // Fetches a long-running operation by its resource name (as returned in // Operation.name, e.g. // `projects/.../instances/.../databases/.../operations/...`). diff --git a/toolbox/mdcode/src/libts/semantic/deploy_bigquery.ts b/toolbox/mdcode/src/libts/semantic/deploy_bigquery.ts index 5ae0316ca..3e143750a 100644 --- a/toolbox/mdcode/src/libts/semantic/deploy_bigquery.ts +++ b/toolbox/mdcode/src/libts/semantic/deploy_bigquery.ts @@ -151,7 +151,7 @@ async function awaitQueryDone( // location, but record a warning so the degraded path is visible -- otherwise a // silent fallback can route a non-US CREATE to the wrong region and fail later // with an opaque location error. -async function datasetLocation( +export async function datasetLocation( bigQuery: BigQueryClient, target: BigQueryGraphTarget, warnings: string[]): Promise { const res = await bigQuery.getDataset(target.project, target.dataset); diff --git a/toolbox/mdcode/src/libts/semantic/deployment_target.ts b/toolbox/mdcode/src/libts/semantic/deployment_target.ts index 0d362a48e..d498beb77 100644 --- a/toolbox/mdcode/src/libts/semantic/deployment_target.ts +++ b/toolbox/mdcode/src/libts/semantic/deployment_target.ts @@ -307,10 +307,10 @@ export function targetError(at: string, target: string): string { /** * The deployment names a `--deployment` flag selects, or undefined when the * flag is absent or negated. The flag is repeated or comma-separated. It - * arrives as a string, as an array when it is repeated, or as a boolean when + * arrives as a string, as an array of them, or as a boolean when * it is absent or negated. Pieces are trimmed, empty ones dropped, and a * repeated name kept once, at its first position. Throws when any occurrence - * of the flag names no deployment. A bare occurrence among repeated ones + * of the flag names no deployment. A bare occurrence in an array * arrives as `true` and names none. */ export function parseDeploymentSelection( diff --git a/toolbox/mdcode/src/libts/semantic/runtime/runtime.ts b/toolbox/mdcode/src/libts/semantic/runtime/runtime.ts index 17bd17213..87997268a 100644 --- a/toolbox/mdcode/src/libts/semantic/runtime/runtime.ts +++ b/toolbox/mdcode/src/libts/semantic/runtime/runtime.ts @@ -57,8 +57,8 @@ export interface SemanticRuntime { */ storeFailed?: boolean; /** - * Set with `storeFailed` when push rejects the model's deployments too, so - * the model is wrong whatever it is used for. One such case is a target in + * Set with `storeFailed` when the model's deployments give no store + * whatever the model is used for. One such case is a target in * no accepted form in any deployment. Another is a GOOGLE block that is not * valid JSON. A third is several targets for the profile with no single * store. A table bound outside the store and a failed AlloyDB lookup are diff --git a/toolbox/mdcode/src/libts/semantic/runtime/store.ts b/toolbox/mdcode/src/libts/semantic/runtime/store.ts index 76b1caf6c..29c9eec9c 100644 --- a/toolbox/mdcode/src/libts/semantic/runtime/store.ts +++ b/toolbox/mdcode/src/libts/semantic/runtime/store.ts @@ -186,12 +186,11 @@ function strayBindings(model: SemanticModel, store: ChosenStore): string[] { * * `named` is set when the model's deployments name a target and still no * store results, so a caller that runs against the store fails. `rejected` is - * set with it when push rejects those deployments too, so the model is wrong - * whatever it is used for. Push rejects a target in no accepted form in any - * deployment, whichever profile the deployment is for, and a target listed - * twice. It also rejects a GOOGLE block whose data is not valid JSON, and - * several targets for the profile that leave the store ambiguous. `named` - * alone means the model binds a table outside its Spanner or AlloyDB store. + * set with it when the deployments give no store whatever the model is used + * for: a target in no accepted form in any deployment, a target listed twice, + * a GOOGLE block whose data is not valid JSON, or several targets for the + * profile. `named` alone means the model binds a table outside its Spanner or + * AlloyDB store. * * Neither is set when the profile names no store, unless a target is in no * accepted form or listed twice, or a GOOGLE block is not valid JSON. Neither diff --git a/toolbox/mdcode/src/libts/semantic/skills.ts b/toolbox/mdcode/src/libts/semantic/skills.ts index b2dba6315..4ad8860fd 100644 --- a/toolbox/mdcode/src/libts/semantic/skills.ts +++ b/toolbox/mdcode/src/libts/semantic/skills.ts @@ -86,7 +86,7 @@ export interface GenerateSkillOptions { * The skill for one model, as files a caller writes out. * * Returns an error for a name the Agent Skills spec would reject. It also - * returns the store error when push rejects the model's deployments, and + * returns the store error when the model's deployments are rejected, and * when the profile names a store that cannot be resolved for a model with * actions, whose skill names the store. A model with no actions is not an * error -- it is a skill that says the model declares nothing to act on, which @@ -106,7 +106,7 @@ export function generateSkill(opts: GenerateSkillOptions): SkillPackage|{ const {actions, instruction} = modelTools({runtime}); // The skill names the store only in what it says about actions. So a store // the profile names and that cannot be resolved stops only a skill with - // actions. Deployments push rejects stop any skill. + // actions. Deployments the runtime marks rejected stop any skill. if (runtime.deploymentsRejected || (runtime.storeFailed && actions.length)) { return {error: runtimeStoreError(runtime) ?? ''}; } diff --git a/toolbox/mdcode/src/libts/semantic/validate.ts b/toolbox/mdcode/src/libts/semantic/validate.ts index 273aaa810..cbffd801e 100644 --- a/toolbox/mdcode/src/libts/semantic/validate.ts +++ b/toolbox/mdcode/src/libts/semantic/validate.ts @@ -1952,7 +1952,7 @@ function firstLine(message: string): string { // as `Unexpected \"$1\"`, where the detail says `Unexpected "$1"`. BigQuery // sends no such detail, so the fallback is the normal path there, not an // unusual one. -function storeMessage(body: string|undefined, status: number): string { +export function storeMessage(body: string|undefined, status: number): string { const raw = (body ?? '').trim(); if (!raw) return `HTTP ${status}.`; try { diff --git a/toolbox/mdcode/src/tool/commands.ts b/toolbox/mdcode/src/tool/commands.ts index 7b9238b31..191f26569 100644 --- a/toolbox/mdcode/src/tool/commands.ts +++ b/toolbox/mdcode/src/tool/commands.ts @@ -17,19 +17,19 @@ import * as deploy from '../libts/semantic/deploy_bigquery'; import * as kc from '../libts/semantic/deploy_knowledge_catalog'; import * as deploySpannerLeg from '../libts/semantic/deploy_spanner'; import {cloudSqlDialects} from '../libts/semantic/cloudsql_dialect'; -import {googleBlockTargets, googleDeploymentTargets, resolveModelDeployments, unnamedDeployments} from '../libts/semantic/deployment_target'; +import {googleBlockTargets, googleDeploymentTargets, parseDeploymentSelection, ResolvedDeployment, resolveModelDeployments, unnamedDeployments} from '../libts/semantic/deployment_target'; import {ProfileSpec, SemanticModel} from '../libts/semantic/ir'; import {FILE_COPY_TYPE_ID, FileCopy, provisionCustomTypes} from '../libts/semantic/kc_custom_types'; import {deploymentProfileError, isGoogleVendor, LoadedModel, loadSemanticModels, SemanticModelError, setsAKeyTwice, writtenSource} from '../libts/semantic/loader'; import {serializeModel} from '../libts/semantic/osi_converter'; import * as pullKc from '../libts/semantic/pull_kc'; -import {applyProfileExclusions, AvailabilityReport, CloudSqlLoad, DeclaredProfile, declaredProfiles, DEFAULT_PROFILE, DocumentProfile, expressionSources, inheritanceErrorOf, isLegacyWrapper, isProfileFileForm, loadProfileFile, modelFileAloneMessage, ProfileExclusion, profileErrorMessage, profileForDocument, profileSourceMessage, pruneUnavailable, reservedProfileNameError, sourcesDialect, unknownProfileMessage, validateProfileCompleteness, validateProfileConsistency, withoutExclusions,} from '../libts/semantic/resolve_profiles'; +import {applyProfile, applyProfileExclusions, AvailabilityReport, CloudSqlLoad, DeclaredProfile, declaredProfiles, DEFAULT_PROFILE, DocumentProfile, expressionSources, inheritanceErrorOf, isLegacyWrapper, isProfileFileForm, loadProfileFile, modelFileAloneMessage, ProfileExclusion, profileErrorMessage, profileForDocument, profileSourceMessage, pruneUnavailable, reservedProfileNameError, sourcesDialect, unknownProfileMessage, validateProfileCompleteness, validateProfileConsistency, withoutExclusions,} from '../libts/semantic/resolve_profiles'; import {createSemanticRuntimes, undeclaredProfileError} from '../libts/semantic/runtime/runtime'; import {profileTargets, storeLine} from '../libts/semantic/runtime/store'; import {generateSkill, SkillPackage} from '../libts/semantic/skills'; import {loadSqlEngine} from '../libts/semantic/sql_parser'; import {transpileModels} from '../libts/semantic/transpile'; -import {inlineBindingWarnings, validateBigQueryActionStatements, validateBigQueryDataSources, validateDeployments, validateExpressionReferences, validateInheritance, validateInheritedColumns, validateInlineDialects, validatePushRequirements, validateSpannerActionStatements} from '../libts/semantic/validate'; +import {inlineBindingWarnings, storeMessage, validateBigQueryActionStatements, validateBigQueryDataSources, validateDeployments, validateExpressionReferences, validateInheritance, validateInheritedColumns, validateInlineDialects, validatePushRequirements, validateSpannerActionStatements} from '../libts/semantic/validate'; import {YAML_OPTIONS} from '../libts/semantic/yaml_options'; import {Sources} from '../libts/source'; import {SemanticModelSource} from '../libts/sources/semantic-model'; @@ -117,9 +117,10 @@ export interface PushOptions { // Mutually exclusive with profile and with --no-profile. Semantic-model push // only. allProfiles?: boolean; - // False for `--no-deployment`, or true when it is absent. Semantic-model - // push only. - deployment?: boolean; + // The `--deployment` values as typed, true for a bare `--deployment` and + // false for `--no-deployment`, in order. Outside a list, false is + // `--no-deployment` alone and true is neither flag. Semantic-model push only. + deployment?: string|boolean|Array; } @@ -448,7 +449,7 @@ function fieldExclusionsOf(model: SemanticModel): ProfileExclusion[] { // Why a graph push of `profileName` cannot go ahead because the profile is a // named profile, or undefined when it can. A named profile is a profile file // or a profile in a vanilla model's GOOGLE block. It names no deployment -// target, and a deployment cannot name a profile yet, so a graph push of one +// target, so a graph push of one // would deploy into the model file's own target, whatever system the profile // binds. export function profileFileGraphError( @@ -456,8 +457,8 @@ export function profileFileGraphError( const fileForm = docs.find(d => d.applied); if (!fileForm) return undefined; return `[${fileForm.name}] binding profile '${profileName}' is a named ` + - `profile, which names no deployment target, and a deployment cannot ` + - `name a profile yet; a graph push would deploy it into the model ` + + `profile, which names no deployment target; a graph push would deploy ` + + `it into the model ` + `file's own target. Run 'kcmd push --profile default' to deploy the ` + `model file's own bindings, or 'kcmd push --no-profile' to publish to ` + `Knowledge Catalog without deploying a graph. Both need the model file's ` + @@ -634,19 +635,21 @@ const PUSH_REPORT: PrepareReport = { // on its own for completeness. An error about a profile file names that file, // and one about a block profile names the model document. // A model's profiles are checked against its model file as loaded, whichever -// binding `profileName` selects. `checkedByDoc` +// binding push selects. `checkedByDoc` // holds each document as push checks it: with the profile applied, or loaded -// with no profile applied. Push's own checks cover part of this for the -// binding it selects, so those rules leave that binding out. A +// with no profile applied. Push's own checks cover part of this for each +// profile `applied` says push applies, so those rules leave that profile out. A // legacy `semantic_model:` wrapper, and a file under the reserved name // 'default', which no push deploys, are not read here. With `transpile`, push // fills in BigQuery SQL for a text written only in a vendor dialect its // translator reads. Push also checks the keys and join columns of the model -// file and of each named profile. The warnings go to `warnings`. +// file and of each named profile. Push's own key check covers the join of +// `pushedProfile`, so this check leaves it out. The warnings go to `warnings`. function namedProfileErrors( docs: ProfiledDoc[], checkedByDoc: Map, - profileName: string, profileDocs: ProfileDocs, declared: DeclaredProfiles, - warnings: string[], transpile: boolean, load: CloudSqlLoad): string[] { + applied: (doc: ProfiledDoc, profile: string) => boolean, + profileDocs: ProfileDocs, declared: DeclaredProfiles, warnings: string[], + transpile: boolean, pushedProfile: string, load: CloudSqlLoad): string[] { const errors: string[] = []; for (const d of docs) { // Each named profile, with the label its errors carry and a function that @@ -727,14 +730,14 @@ function namedProfileErrors( const profileWarnings: string[] = []; errors.push( ...validateProfileCompleteness(logical, spec, profileWarnings, { - applied: !!d.applied && p.name === profileName, + applied: applied(d, p.name), ownSqlOnly, transpiled: transpile, ...load, }).map(at)); warnings.push(...profileWarnings.map(at)); } - errors.push(...validateProfileConsistency(logical, specs, profileName) + errors.push(...validateProfileConsistency(logical, specs, pushedProfile) .map(e => `[${d.name}] ${e}`)); } return errors; @@ -870,8 +873,9 @@ async function prepareModels( const unprunedWarnings: string[] = []; const profileErrors = [ ...namedProfileErrors( - docs, checkedByDoc, profileName, opts.profileDocs, opts.declared, - unprunedWarnings, !!opts.transpile, opts.load), + docs, checkedByDoc, (d, name) => !!d.applied && name === profileName, + opts.profileDocs, opts.declared, unprunedWarnings, !!opts.transpile, + profileName, opts.load), ...modelFileBindingErrors( docs, checkedByDoc, opts.declared, unprunedWarnings, !!opts.transpile), ]; @@ -905,25 +909,9 @@ async function prepareModels( availability.push(r); return {document, model: pruned}; }); - // Pruning never removes an entity or a relationship: their key and join - // columns are physical columns, not fields. So the note counts what - // pruning can make unavailable: fields, metrics and actions. for (const r of availability) { - const dropped = r.droppedMetrics.length + r.droppedActions.length; - if (r.unboundFields.length || r.droppedFields.length || dropped) { - report.note( - `profile '${r.profile}' leaves ${ - r.unboundFields.length} field(s) unbound` + - (r.droppedFields.length ? - `; ${r.droppedFields.length} field(s) that read them ` + - `unavailable` : - '') + - (dropped ? - `; ${r.droppedMetrics.length} metric(s) and ${ - r.droppedActions.length} action(s) unavailable` : - '') + - '.'); - } + const note = pruningNote(r); + if (note) report.note(note); } } const pushWarnings: string[] = []; @@ -959,6 +947,25 @@ async function prepareModels( return {models, bqModels, spannerModels}; } +// What a pruned profile leaves unavailable, or undefined when it leaves +// nothing. It counts fields, metrics and actions, since pruning never removes +// an entity or a relationship. +function pruningNote(r: AvailabilityReport): string|undefined { + const dropped = r.droppedMetrics.length + r.droppedActions.length; + if (!r.unboundFields.length && !r.droppedFields.length && !dropped) { + return undefined; + } + return `profile '${r.profile}' leaves ${ + r.unboundFields.length} field(s) unbound` + + (r.droppedFields.length ? + `; ${r.droppedFields.length} field(s) that read them unavailable` : + '') + + (dropped ? `; ${r.droppedMetrics.length} metric(s) and ${ + r.droppedActions.length} action(s) unavailable` : + '') + + '.'; +} + // Warns about each model whose model file is not named after it, when push // stores file copies. Pull writes a model to `.yaml`, so it returns the // copy of a model file only under that name. @@ -1026,14 +1033,8 @@ async function pushErrors( ] as const; for (const [models, deployLeg] of legs) { if (!models.length) continue; - try { - const result = await deployLeg(models, ctx, {validateOnly: true}); - if (!result.success) { - errors.push(result.details ?? 'the graph could not be generated'); - } - } catch (err: any) { - errors.push(err.message || String(err)); - } + const result = await generateOffline(deployLeg, models, ctx); + if (typeof result === 'string') errors.push(result); } return errors; } @@ -1054,9 +1055,24 @@ function catalogAspects( }); } +// Generates a graph leg's DDL without calling a service, or gives why it +// cannot be generated. +async function generateOffline( + deployLeg: typeof deploy.deployBigQuery, models: LoadedModel[], + ctx: context.ApiContext): Promise { + try { + const result = await deployLeg(models, ctx, {validateOnly: true}); + if (result.success) return result; + return result.details ?? 'the graph could not be generated'; + } catch (err: any) { + return err.message || String(err); + } +} + export async function push(options: PushOptions): Promise { const ctx = context.ApiContext.default(); const snapshot = await kcmd.CatalogSnapshot.fromPath('.', ctx); + const {named, negated} = deploymentFlags(options.deployment); if (snapshot.manifest.source.type === Sources.SEMANTIC_MODEL) { // The semantic-model source always resolves to the SemanticModel layout @@ -1068,7 +1084,7 @@ export async function push(options: PushOptions): Promise { // Reading a GOOGLE block profile checks its field bindings for SQL, which // needs the SQL parser. if (!await sqlParserReady()) return 1; - if (options.deployment === false) { + if (named || negated) { return pushInTwoPhases(options, { ctx, layout, @@ -1525,7 +1541,8 @@ export async function push(options: PushOptions): Promise { [!!options.print, '--print'], [!!options.emitExpressions, '--emit-expressions'], [!!options.forceRemove, '--force-remove'], - [options.deployment === false, '--no-deployment'], + [named, '--deployment'], + [negated, '--no-deployment'], ]; for (const [set, flag] of semanticOnlyFlags) { if (set) { @@ -1554,22 +1571,57 @@ export async function push(options: PushOptions): Promise { } +// Which of `--deployment` and `--no-deployment` the push options carry. +function deploymentFlags(deployment: PushOptions['deployment']): + {named: boolean; negated: boolean} { + if (deployment === undefined || deployment === true) { + return {named: false, negated: false}; + } + const given = [deployment].flat(); + return {named: given.some(v => v !== false), negated: given.includes(false)}; +} + +// The `deployment` option as typed before `--` in `args`, since cac turns a +// value that looks like a number into a number and drops `--deployment` +// before `--no-deployment`. It is false for `--no-deployment` alone and true +// for neither flag. +export function typedDeployment(args: string[]): PushOptions['deployment'] { + const typed: Array = []; + for (let i = 0; i < args.length && args[i] !== '--'; i++) { + // cac reads `-no-deployment` as `--no-deployment` too. + if (/^-+no-deployment$/.test(args[i])) typed.push(false); + if (args[i] === '--deployment') { + const next = args[i + 1] ?? '-'; + typed.push(next.startsWith('-') ? true : next); + } else if (args[i].startsWith('--deployment=')) { + typed.push(args[i].slice('--deployment='.length)); + } + } + return deploymentFlags(typed).named ? typed : !typed.length; +} + // The error for a flag the two-phase path does not take, or undefined. function twoPhaseFlagError(options: PushOptions): string|undefined { if (options.kc === false) { return `'--no-kc' is not supported; every push publishes the model to ` + `Knowledge Catalog.`; } + const {named, negated} = deploymentFlags(options.deployment); + if (named && negated) { + return 'Cannot combine --no-deployment with --deployment.'; + } const ownProfile = `does not apply to a push that uses deployments, since ` + `each deployment names its own profile.`; // cac passes a numeric profile name as a number, so any value but a boolean // counts. if (options.profile !== undefined && options.profile !== true && options.profile !== false) { - return `'--profile' ${ownProfile}`; + return `'--profile' ${ownProfile} Use '--deployment ' to deploy ` + + `only some deployments.`; } if (options.allProfiles) { - return `'--all-profiles' ${ownProfile}`; + return `'--all-profiles' ${ownProfile} A push deploys every deployment ` + + `unless '--deployment' names some.`; } if (options.profile === false) { return `'--no-profile' does not apply to a push that uses deployments. ` + @@ -1590,8 +1642,18 @@ interface TwoPhaseScope { load: CloudSqlLoad; } +// One selected deployment, its message prefix and the model it deploys. A +// BigQuery or Spanner deployment also holds its generated DDL and warnings. +interface DeploymentPlan { + deployment: ResolvedDeployment; + at: string; + model: LoadedModel; + generated?: {ddl: string[]; warnings: string[]}; +} + // Runs checkBeforeWrites and the checks that call a service, then publishes to -// Knowledge Catalog when every check passes. Returns a process exit code. +// Knowledge Catalog and deploys each selected deployment in order with its +// own model. Returns a process exit code. async function pushInTwoPhases( options: PushOptions, scope: Omit): Promise { const {ctx, source} = scope; @@ -1605,7 +1667,7 @@ async function pushInTwoPhases( if (!load) return 1; const checked = await checkBeforeWrites(options, {...scope, load}); if (!checked) return 1; - const {kcModels, fileCopies, overviews} = checked; + const {plans, kcModels, fileCopies, notes, overviews} = checked; if (fileCopies.length) { const problem = await kc.checkCustomTypes( new dataplex.CatalogClient(ctx), source, [FILE_COPY_TYPE_ID]); @@ -1622,36 +1684,93 @@ async function pushInTwoPhases( if (preflight.result && !preflight.result.success) { return reportCatalogResult(preflight.result, options); } + // The local checks and the catalog preflight have passed, so what pruning + // leaves out is worth reading now. + for (const n of new Set(notes)) console.warn(`Note: ${n}`); + const locationWarnings: string[] = []; + let errors: string[] = []; + for (const plan of plans) { + errors = await liveErrors( + plan, ctx, source.project ?? ctx.project, locationWarnings); + if (errors.length) break; + } + // A push that deploys reads each dataset's location again and prints the + // same warning with the graph. + if (errors.length || options.validateOnly) { + for (const w of new Set(locationWarnings)) console.warn(`Warning: ${w}`); + } + if (errors.length) return fail(...errors); + const header = (d: ResolvedDeployment) => { + if (plans.length > 1) console.log(`\n-- Deployment '${d.name}' --`); + }; if (options.validateOnly) { + for (const {deployment, generated} of plans) { + if (!generated) continue; + header(deployment); + printGraph( + deployment.engine === 'bigquery' ? 'BigQuery' : 'Spanner', generated, + options); + } return reportCatalogResult(preflight.result!, options); } console.log('Pushing semantic model (Knowledge Catalog)...'); const published = await kc.applyKnowledgeCatalog(preflight.prepared!, ctx, kcOptions); - return reportCatalogResult(published, options); + const publishedCode = reportCatalogResult(published, options); + if (publishedCode !== 0) return publishedCode; + for (const {deployment, model} of plans) { + header(deployment); + if (deployment.engine === 'alloydb') { + console.warn( + `Note: deployment '${deployment.name}' targets an AlloyDB ` + + `database, which has no property graph, so push deploys no graph ` + + `there. The model runs against that database as published to ` + + `Knowledge Catalog.`); + continue; + } + const code = deployment.engine === 'bigquery' ? + await pushBigQuery([model], ctx, options) : + await pushSpanner([model], ctx, options); + if (code !== 0) return code; + } + return 0; } // The local checks of a two-phase push, which call no service: it stops at -// the first check that fails, and returns the models the catalog receives, the -// file copies it stores and the overview sidecars it writes, or undefined -// after printing why push stops. -async function checkBeforeWrites(options: PushOptions, scope: TwoPhaseScope): - Promise<{ - kcModels: LoadedModel[]; fileCopies: FileCopy[]; - overviews?: Map; - }|undefined> { +// the first check that fails, and returns each selected deployment's plan, the +// models the catalog receives, the file copies it stores, the overview sidecars +// it writes and what pruning leaves out, or undefined after printing why push +// stops. +async function checkBeforeWrites( + options: PushOptions, scope: TwoPhaseScope): Promise<{ + plans: DeploymentPlan[]; kcModels: LoadedModel[]; fileCopies: FileCopy[]; + notes: string[]; + overviews?: Map; +}|undefined> { const {ctx, layout, source, docs, profileDocs, load} = scope; const fail = (...errors: string[]) => { for (const error of new Set(errors)) console.error(`Error: ${error}`); return undefined; }; - // Prints a warning, except the loader's 'needs transpilation' warnings - // under --transpile. - const warn = (w: string) => { + // Prints each warning once, and none of the loader's 'needs + // transpilation' warnings under --transpile. A deployment's warning is + // skipped when the catalog's model printed it as `asCatalog`. + const printed = new Set(); + const warn = (w: string, asCatalog = w) => { if (options.transpile && w.includes('needs transpilation')) return; + if (printed.has(w) || printed.has(asCatalog)) return; + printed.add(w); console.warn(`Warning: ${w}`); }; + let selected: string[]|undefined; + try { + selected = parseDeploymentSelection(options.deployment); + } catch (err: any) { + return fail(err.message || String(err)); + } + const noDeployment = deploymentFlags(options.deployment).negated; + const defaultProject = source.project ?? ctx.project; // A model file may leave every source to its profile files, so a source is // required only once a deployment's profile is applied. @@ -1659,6 +1778,11 @@ async function checkBeforeWrites(options: PushOptions, scope: TwoPhaseScope): docs, {defaultProject, bindingOptional: true, ...load}); if (loaded.error) return fail(loaded.error); for (const w of loaded.warnings) warn(w); + if (selected && !loaded.models.length) { + return fail( + `--deployment names ${selected.map(n => `'${n}'`).join(', ')}, but ` + + `the scope has no model.`); + } if (loaded.models.length > 1) { return fail(kc.oneModelPerEntryGroupMessage( source.entryGroup, loaded.models.length)); @@ -1691,13 +1815,18 @@ async function checkBeforeWrites(options: PushOptions, scope: TwoPhaseScope): } // Every deployment's target must be in an accepted form, and its profile one - // the model declares. + // the model declares, whether or not this push selects the deployment. + const selectedByDoc = new Map(); for (const {document, model} of models) { const every = resolveModelDeployments(model); if ('error' in every) return fail(every.error); + const resolved = noDeployment ? {deployments: []} : + resolveModelDeployments(model, selected); + if ('error' in resolved) return fail(resolved.error); const undeclared = undeclaredProfileError( document, model, declaredNames(document)); if (undeclared) return fail(undeclared); + selectedByDoc.set(document, resolved.deployments); } const byDefault = defaultProfileCheck(scope, models, declaredBy); if ('error' in byDefault) return fail(byDefault.error); @@ -1709,8 +1838,9 @@ async function checkBeforeWrites(options: PushOptions, scope: TwoPhaseScope): const profileWarnings: string[] = []; const profileErrors = namedProfileErrors( docs, new Map(loaded.models.map(m => [m.document, m.model])), - DEFAULT_PROFILE, profileDocs, declaredBy, profileWarnings, - !!options.transpile, load); + (d, name) => !!selectedByDoc.get(d.name)?.some(s => s.profile === name), + profileDocs, declaredBy, profileWarnings, !!options.transpile, + DEFAULT_PROFILE, load); for (const {document, model} of models) { profileErrors.push( ...validateInlineDialects( @@ -1743,7 +1873,20 @@ async function checkBeforeWrites(options: PushOptions, scope: TwoPhaseScope): return fail(...profileErrors, ...kcErrors); } - return {kcModels, fileCopies, overviews}; + const plans: DeploymentPlan[] = []; + const notes: string[] = []; + for (const base of models) { + for (const deployment of selectedByDoc.get(base.document) ?? []) { + const plan = await deploymentPlan( + base, profileDocs(base.document), deployment, ctx, options, warn, + notes); + if ('errors' in plan) { + return fail(...plan.errors); + } + plans.push(plan); + } + } + return {plans, kcModels, fileCopies, notes, overviews}; } // Adds the profile files of the model in `base` to `model.profiles`, without @@ -1794,6 +1937,145 @@ function defaultProfileCheck( return {}; } +// Builds and checks the model `deployment` deploys from `base`, reading its +// profile file in `files` again with its actions, and generates its DDL +// offline. Returns the plan, or the errors that stop the push. +async function deploymentPlan( + base: LoadedModel, files: ReturnType, + deployment: ResolvedDeployment, ctx: context.ApiContext, + options: PushOptions, warn: (w: string, asCatalog?: string) => void, + notes: string[]): Promise { + const {document, model} = base; + const at = `[${document}] deployment '${deployment.name}'`; + const fail = (...errors: string[]) => ({errors}); + if (deployment.engine === 'spanner' && + deployment.profile === DEFAULT_PROFILE && model.metrics?.length) { + return fail( + `${at} targets a Spanner graph and uses the model file's own ` + + `bindings, but the model declares metrics, which a Spanner graph ` + + `cannot hold; name a profile with metrics_exclude: "*".`); + } + let applied = model; + if (deployment.profile !== DEFAULT_PROFILE) { + // A declared profile that `model.profiles` lacks is a legacy wrapper. + let spec = model.profiles?.find(p => p.name === deployment.profile); + if (!spec) { + return fail( + `${at} names profile '${deployment.profile}', which is in the ` + + `legacy 'semantic_model:' form; a deployment can use only a ` + + `profile file.`); + } + const file = files.find(p => p.name === deployment.profile); + const warnings: string[] = []; + try { + // A profile in a GOOGLE block has no file and applies as loaded. + if (file) spec = loadProfileFile(file.text, file.name); + } catch (err) { + if (!(err instanceof SemanticModelError)) throw err; + return fail(`[${file!.fileName}] ${err.message}`); + } + try { + applied = applyProfile(model, spec, {warnings}); + } catch (err) { + if (!(err instanceof SemanticModelError)) throw err; + return fail(`${at}: ${err.message}`); + } + for (const w of warnings) warn(`[${document}] ${w}`); + } + // Messages name the deployment. A warning the catalog's model already gave + // under the document's own name is not repeated. + const label = `${document}, deployment '${deployment.name}'`; + const warnDeployment = (w: string) => warn(w, w.replaceAll(label, document)); + let checked: LoadedModel = {document: label, model: applied}; + if (options.transpile) { + const transpiled = await transpileModels([checked]); + checked = transpiled.models[0]; + for (const w of transpiled.warnings) warnDeployment(w); + } + // Pruning can remove the field these rules are about, so they run first. + const unprunedWarnings: string[] = []; + const unprunedErrors = [ + ...validateInheritance([checked]), + ...validateExpressionReferences([checked], false, unprunedWarnings), + ]; + const {model: pruned, report} = + pruneUnavailable(checked.model, deployment.profile); + const note = pruningNote(report); + if (note) notes.push(note); + // Keeps only this deployment, so every reader sees its target alone. + const narrowed: LoadedModel = { + document: checked.document, + model: { + ...pruned, + deployments: + (model.deployments ?? []).filter(d => d.name === deployment.name), + }, + }; + // An AlloyDB database holds no graph. + const errors = [ + ...unprunedErrors, + ...validatePushRequirements([narrowed], { + targetOptional: deployment.engine === 'alloydb', + fieldsPruned: true, + v2Aspects: kcV2Aspects(), + }), + ]; + for (const w of unprunedWarnings) warnDeployment(w); + if (errors.length) return {errors}; + if (deployment.engine === 'alloydb') { + return {deployment, at, model: narrowed}; + } + const generated = await generateOffline( + deployment.engine === 'bigquery' ? deploy.deployBigQuery : + deploySpannerLeg.deploySpanner, + [narrowed], ctx); + if (typeof generated === 'string') return fail(`${at}: ${generated}`); + return {deployment, at, model: narrowed, generated}; +} + +// The errors of one deployment's live checks, which change nothing. They read +// the dataset or database, probe each BigQuery source, dry-run the BigQuery DDL +// in the dataset's location and check each action's DML. A dataset whose +// location cannot be read adds a warning to `warnings`. +async function liveErrors( + plan: DeploymentPlan, ctx: context.ApiContext, defaultProject: string, + warnings: string[]): Promise { + const {deployment, at, model} = plan; + if (deployment.engine === 'spanner') { + const spanner = new SpannerClient(ctx); + const {project, instance, database} = deployment.coordinates; + const found = await spanner.getDatabase(project, instance, database); + const name = `${project}/${instance}/${database}`; + if (found.status === 404) return [`${at}: database ${name} not found`]; + if (found.status !== 200) { + return [`${at}: database ${name} could not be read: ${ + storeMessage(found.message, found.status)}`]; + } + return validateSpannerActionStatements([model], spanner); + } + if (deployment.engine !== 'bigquery') return []; + const bq = new BigQueryClient(ctx); + const {project} = deployment.coordinates; + let location: string|undefined; + try { + location = + await deploy.datasetLocation(bq, deployment.coordinates, warnings); + } catch (err: any) { + return [`${at}: ${err.message}`]; + } + const accessErrors = + await validateBigQueryDataSources([model], bq, defaultProject); + if (accessErrors.length) return accessErrors; + for (const ddl of plan.generated?.ddl ?? []) { + const res = await bq.query(project, ddl, location, true); + if (res.status !== 200) { + return [`${at}: the graph's DDL fails a BigQuery dry run: ${ + storeMessage(res.message, res.status)}`]; + } + } + return validateBigQueryActionStatements([model], bq, defaultProject); +} + export interface ProfilesOptions { // `string|boolean` for the same reason push's is: cac yields `true` for a @@ -2151,6 +2433,17 @@ async function sqlParserError(): Promise { } +// Prints a graph leg's warnings, and under --print its DDL. +function printGraph( + engine: 'BigQuery'|'Spanner', result: {warnings: string[]; ddl: string[]}, + options: PushOptions): void { + for (const w of result.warnings) console.warn(`Warning: ${w}`); + if (!options.print) return; + console.log(`-- ${engine} Graph --`); + for (const block of result.ddl) console.log(`${block}\n`); +} + + // Deploys the semantic model's BigQuery Graph leg (over the pre-loaded models) // and prints the result. Returns a process exit code (0 on success). async function pushBigQuery( @@ -2160,16 +2453,7 @@ async function pushBigQuery( options.validateOnly ? 'Validating semantic model for BigQuery Graph...' : 'Pushing semantic model (BigQuery Graph)...'); const result = await deploy.deployBigQuery(models, ctx, options); - - for (const w of result.warnings) { - console.warn(`Warning: ${w}`); - } - if (options.print) { - console.log('-- BigQuery Graph --'); - for (const block of result.ddl) { - console.log(`${block}\n`); - } - } + printGraph('BigQuery', result, options); if (!result.success) { console.error('Error pushing semantic model to BigQuery:', result.details); @@ -2192,16 +2476,7 @@ async function pushSpanner( options.validateOnly ? 'Validating semantic model for Spanner Graph...' : 'Pushing semantic model (Spanner Graph)...'); const result = await deploySpannerLeg.deploySpanner(models, ctx, options); - - for (const w of result.warnings) { - console.warn(`Warning: ${w}`); - } - if (options.print) { - console.log('-- Spanner Graph --'); - for (const block of result.ddl) { - console.log(`${block}\n`); - } - } + printGraph('Spanner', result, options); if (!result.success) { console.error('Error pushing semantic model to Spanner:', result.details); diff --git a/toolbox/mdcode/src/tool/main.ts b/toolbox/mdcode/src/tool/main.ts index 2573a09f8..a62a269db 100644 --- a/toolbox/mdcode/src/tool/main.ts +++ b/toolbox/mdcode/src/tool/main.ts @@ -77,16 +77,20 @@ cli.command('push', 'Push catalog entries') 'Rewrite vendor-dialect (e.g. Snowflake/Databricks) expressions to GoogleSQL before deploying, filling target expressions the loader left unset; semantic-model push only') .option( '--profile [name]', - 'Deploy the graph for one binding profile (reads .profiles/.yaml; a sibling .profile..yaml, or a profile in a 0.2.0.dev0 model\'s GOOGLE block, cannot be deployed as a graph yet); its deployment target selects the graph backend; defaults to default_profile, else the inline bindings; mutually exclusive with --all-profiles and --no-profile; semantic-model push only') + 'Deploy the graph for one binding profile (reads .profiles/.yaml; a sibling .profile..yaml, or a profile in a 0.2.0.dev0 model\'s GOOGLE block, deploys only through a deployment that names it); its deployment target selects the graph backend; defaults to default_profile, else the inline bindings; mutually exclusive with --all-profiles and --no-profile; semantic-model push only') .option( '--all-profiles', 'Deploy the graph for every defined binding profile (plus the inline bindings when the document declares a target); Knowledge Catalog still records the default binding; mutually exclusive with --profile and --no-profile; semantic-model push only') + // The value is optional because --no-deployment defaults this option to + // true, which cac reports as a missing value when the value is required. + .option('--deployment [names...]', 'Deploy only these deployments, by name') .option( '--no-deployment', 'Publish to Knowledge Catalog only; deploy nothing') .action(async (options) => { let exitCode = 1; try { - exitCode = await commands.push(options); + const deployment = commands.typedDeployment(process.argv.slice(2)); + exitCode = await commands.push({...options, deployment}); } catch (err: any) { console.error('Error:', err.message || err); exitCode = 1; diff --git a/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts b/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts index 394598abc..267fe2d71 100644 --- a/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts +++ b/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts @@ -1,7 +1,13 @@ -// Tests that push checks every destination before it writes -// (src/tool/commands.ts, push). +// Tests that push checks every destination before it writes, then writes the +// catalog and deploys each deployment in order, with its own target and +// profile, and tests what push prints for each deployment +// (src/tool/commands.ts, checkBeforeWrites, liveErrors and pushInTwoPhases). -import {bqGraph, err, has, modelFile, refused, scenarios, Scope} from './harness'; +import {expect} from 'bun:test'; + +import {ALLOYDB, at, boundAt, boundTo, bqGraph, bqTable, Case, CUSTOMER, deploymentNames, err, has, kcResult, modelFile, msg, PREPARED, profileFile, PushRun, REAL, refused, scenarios, Scope, SNOWFLAKE_METRIC, SPANNER_CUSTOMER, spannerGraph, TRANSPILE_WARNING, when} from './harness'; + +const header = (d: string) => `\n-- Deployment '${d}' --`; const sales = (s: Scope) => modelFile(s).replace('name: commerce', 'name: sales'); @@ -12,6 +18,20 @@ const NO_TARGET = has("model 'sales' (sales) declares 0 deployments"); const ONE_MODEL = err( "entry group 'commerce_eg' would receive 2 models, but only one model " + 'per entry group is supported; split them into separate entry groups.'); +const spannerAnswers = + (what: string, status: number, body: string, error: string): Case => ({ + name: `a Spanner deployment whose database ${what} writes nothing`, + deployments: [{name: 'sp', target: spannerGraph('g'), profile: 'ops'}], + profiles: {ops: boundAt(SPANNER_CUSTOMER)}, + services: {getDatabase: {status, message: body}}, + errors: [err(`${at('sp')}: database test-project/i/db ${error}`)], + called: {spannerDml: 0}, + }); +const DISABLED = {code: 403, message: 'Cloud Spanner API is disabled.'}; +const DENIED = {getDataset: {status: 403, message: 'Access Denied'}}; +const NO_LOCATION = (project: string) => + `Warning: could not read location of dataset ${project}.d (Access ` + + 'Denied); letting BigQuery infer the query location'; scenarios('push writes nothing when a check fails', [ { @@ -35,7 +55,377 @@ scenarios('push writes nothing when a check fails', [ stops(ONE_MODEL, {deployment: false}), ], }, + { + name: 'a live source check that fails on the second deployment stops the push before the first graph deploys', + services: { + sources: when( + models => deploymentNames(models) === 'g2', + ["table 'test-project.d.customer' is not reachable from g2"]), + }, + errors: [err("table 'test-project.d.customer' is not reachable from g2")], + calls: ['generate g1', 'generate g2', 'sources g1', 'sources g2'], + called: {preflight: 1}, + }, + { + name: 'a deployment whose BigQuery dataset does not exist writes nothing', + deployments: ['g1', {name: 'g2', target: bqGraph('g2', 'missing')}], + services: { + getDataset: when( + (_project, dataset) => dataset === 'missing', + {status: 404, message: 'Not found'}), + }, + errors: [err(`${at('g2')}: dataset test-project.missing not found`)], + }, + { + name: 'a deployment whose profile does not apply with its actions writes nothing', + deployments: [{name: 'g', profile: 'p'}], + profiles: {p: {...boundTo('p'), actions: [{name: 'refund', bogus: 1}]}}, + errors: [err( + "[commerce.profile.p.yaml] profile 'p': action 'refund' sets " + + "'bogus', which a profile may not set; a profile carries only " + + 'physical bindings, and the logical model owns everything else')], + called: {preflight: 0}, + then: [{ + profiles: {p: boundTo('p')}, + actions: [{ + name: 'refund', + executor: {sql: {statements: ['UPDATE t SET a = 1 WHERE TRUE']}}, + }], + code: 1, + nothingWritten: true, + errors: [err( + `${at('g')}: profile 'p': action 'refund' in model 'commerce' ` + + "declares a 'sql' executor. A statement names one database's " + + 'own tables and columns, so it belongs in the profile that ' + + 'binds them, not in the model. Move the executor into each ' + + 'profile that performs this write as DML.')], + }], + }, + spannerAnswers('does not exist', 404, 'Not found', 'not found'), + spannerAnswers( + 'answers 403', 403, JSON.stringify({error: DISABLED}), + `could not be read: ${DISABLED.message}`), + { + name: 'a graph DDL that fails a BigQuery dry run stops the push before the catalog write', + deployments: [{ + name: 'g', + target: bqGraph('g').replace('test-project', 'graph-project'), + }], + services: { + query: when( + (_project, sql) => sql.includes('PROPERTY GRAPH'), + {status: 400, message: "Column 'c_key' not found"}), + }, + errors: [err( + `${at('g')}: the graph's DDL fails a BigQuery dry run: Column ` + + "'c_key' not found.")], + // The dry run runs in the dataset's project and location. + check: r => { + expect(r.fake.getDataset.mock.calls).toEqual([['graph-project', 'd']]); + expect(r.fake.query.mock.calls + .filter(c => c[3] === true && c[1].includes('PROPERTY GRAPH')) + .map(c => [c[0], c[2]])) + .toEqual([['graph-project', 'US']]); + }, + // A dataset whose location cannot be read prints that warning too. + then: [{services: DENIED, warnings: [NO_LOCATION('graph-project')]}], + }, ], { + deployments: ['g1', 'g2'], code: 1, nothingWritten: true, }); + +const THREE = { + deployments: [ + {name: 'c', target: bqGraph('gc')}, + {name: 'a', target: spannerGraph('ga'), profile: 'sp'}, + {name: 'b', target: bqGraph('gb'), profile: 'x'}, + ], + profiles: { + sp: boundAt(SPANNER_CUSTOMER), + x: boundTo('x'), + }, +}; +const IN_ORDER = ['preflight', 'apply', 'deploy c', 'deploy a', 'deploy b']; +// Each deployment of THREE, the service that deploys it, its target and the +// source it gives Customer. +const THREE_DEPLOYED = [ + ['c', 'bq', bqGraph('gc'), 'test-project.d.customer'], + ['a', 'spanner', spannerGraph('ga'), `//${SPANNER_CUSTOMER}`], + ['b', 'bq', bqGraph('gb'), 'test-project.x.customer'], +] as const; +const headers = (r: PushRun) => r.log.filter(l => l.includes('-- Deployment')); +// A graph deploy that fails outside --validate-only. +const failedDeploy = when( + (_m: unknown, _c: unknown, o: any) => !o?.validateOnly, + {success: false, details: 'boom', ddl: [], warnings: []}); + +const WITH_LTV = {...CUSTOMER, fields: [...CUSTOMER.fields, {name: 'ltv'}]}; +const NO_LTV = { + entities: { + Customer: { + ...CUSTOMER, + source: bqTable('customer', 'prod'), + fields_exclude: ['ltv'], + }, + }, +}; +const WITH_SCORE = { + ...CUSTOMER, + fields: [...CUSTOMER.fields, {name: 'score', expression: 'score'}], +}; +// Binds field score to `expression`, written in `dialect`. +const scoreSql = (name: string, dialect: string, expression: string) => ({ + entities: { + Customer: { + ...CUSTOMER, + source: bqTable('customer', name), + fields: [ + ...CUSTOMER.fields, + {name: 'score', expression: {dialects: [{dialect, expression}]}}, + ], + }, + }, +}); +// Entity Orders, bound to table orders in dataset d. +const ORDERS = { + name: 'Orders', + source: bqTable('orders'), + primary_key: ['id'], + fields: [{name: 'id', expression: 'o_id'}], +}; +// Relationships of `names` from Orders to Customer, and profile prod, which +// gives each its join columns. +const placedBy = (...names: string[]) => ({ + relationships: names.map(name => ({name, from: 'Orders', to: 'Customer'})), + profiles: { + prod: { + entities: { + Customer: {...CUSTOMER, source: bqTable('customer', 'prod')}, + Orders: {...ORDERS, source: bqTable('orders', 'prod')}, + }, + relationships: names.map( + name => ({name, from_columns: ['customer_key'], to_columns: ['key']})), + }, + }, +}); +const ALLOYDB_NOTE = "Note: deployment 'ad' targets an AlloyDB database, " + + 'which has no property graph, so push deploys no graph there. The model ' + + 'runs against that database as published to Knowledge Catalog.'; + +scenarios('how push deploys each deployment in order, and what it prints', [ + { + name: 'deploys each deployment after the catalog write, in declared order or the order --deployment gives, with only its own deployment entry, its own profile and a header when there are several', + ...THREE, + events: IN_ORDER, + called: {preflight: 1, bqDml: 2, spannerDml: 1}, + check: r => { + expect(r.fake.apply.mock.calls[0][0]).toBe(PREPARED); + expect(headers(r)).toEqual(['c', 'a', 'b'].map(header)); + for (const [name, service, target, source] of THREE_DEPLOYED) { + const model = r.deployed(name, service)!; + expect(model.deployments!.map(d => [d.name, d.target])).toEqual([ + [name, target], + ]); + expect(model.entities[0].dataSource).toBe(source); + expect(model.profiles!.map(p => p.name).sort()).toEqual(['sp', 'x']); + } + const [[models, , opts]] = r.fake.preflight.mock.calls; + expect(models).toHaveLength(1); + const published = r.published; + expect(published.entities[0].dataSource).toBe('test-project.d.customer'); + expect(published.profiles!.map(p => p.name).sort()) + .toEqual(['sp', 'x']); + expect(published.deployments!.map(d => d.name)) + .toEqual(['c', 'a', 'b']); + expect(opts.v2Aspects).toBe(false); + expect(opts.sourceFiles).toHaveLength(3); + expect(Object.fromEntries(opts.sourceFiles.map( + (f: any) => [f.fileName, f.content]))) + .toEqual({ + 'commerce.yaml': modelFile(THREE), + 'commerce.profile.sp.yaml': profileFile('sp', THREE.profiles.sp), + 'commerce.profile.x.yaml': profileFile('x', THREE.profiles.x), + }); + }, + then: [ + { + flags: {deployment: ['b,a']}, + code: 0, + events: ['preflight', 'apply', 'deploy b', 'deploy a'], + }, + { + deployments: [THREE.deployments[0]], + code: 0, + check: r => expect(headers(r)).toEqual([]), + }, + ], + }, + { + name: 'a failed catalog write, or a Spanner or BigQuery deployment that fails to deploy, stops the push', + ...THREE, + services: {spanner: failedDeploy}, + code: 1, + events: ['preflight', 'apply', 'deploy c', 'deploy a'], + errors: ['Error pushing semantic model to Spanner: boom'], + then: [{ + services: {bq: failedDeploy}, + code: 1, + events: ['preflight', 'apply', 'deploy c'], + errors: ['Error pushing semantic model to BigQuery: boom'], + }, { + services: {apply: kcResult({success: false, details: 'x'})}, + code: 1, + events: ['preflight', 'apply'], + }], + }, + { + name: 'a profile that gives the join columns deploys, and with KC_V2_ASPECTS=1 so do relationships whose link IDs collide', + deployments: [{name: 'g', profile: 'prod'}], + entities: [CUSTOMER, ORDERS], + ...placedBy('placed_by'), + events: ['preflight', 'apply', 'deploy g'], + then: [{ + v2Aspects: true, + ...placedBy('placed_by', 'placed__by'), + events: ['preflight', 'apply', 'deploy g'], + }], + }, + { + name: 'an AlloyDB deployment deploys no graph and prints a note naming it, under its own header, and under --validate-only prints nothing', + deployments: [ + {name: 'bq', target: bqGraph('g')}, + {name: 'ad', target: ALLOYDB, profile: 'pg'}, + ], + profiles: {pg: boundAt(`${ALLOYDB}/schemas/public/tables/customer`)}, + events: ['preflight', 'apply', 'deploy bq'], + calls: ['generate bq', 'getDataset', 'bqDml bq', 'deploy bq'], + warnings: [ALLOYDB_NOTE], + check: r => expect(r.printed.filter( + l => l.includes('-- Deployment') || l === ALLOYDB_NOTE)) + .toEqual([header('bq'), header('ad'), ALLOYDB_NOTE]), + then: [{ + flags: {validateOnly: true}, + log: [header('bq'), msg.VALIDATED], + warnings: [], + }], + }, + { + name: 'prints each deployment\'s DDL warnings once, and its DDL only under --print, and one completion line', + flags: {validateOnly: true}, + services: { + bq: async (...a: any[]) => { + const r = await REAL.bq(...a); + return { + ...r, + warnings: [...r.warnings, `ddl warning ${deploymentNames(a[0])}`], + }; + }, + }, + warnings: ['Warning: ddl warning g1', 'Warning: ddl warning g2'], + log: [header('g1'), header('g2'), msg.VALIDATED], + events: ['preflight'], + nothingWritten: true, + then: [ + { + flags: {validateOnly: true, print: true}, + check: r => expect(r.log.slice(0, 6)).toEqual([ + header('g1'), '-- BigQuery Graph --', + expect.stringContaining('PROPERTY GRAPH `test-project.d.g1`'), + header('g2'), '-- BigQuery Graph --', + expect.stringContaining('PROPERTY GRAPH `test-project.d.g2`'), + ]), + }, + // A push that deploys prints them once too. + {warnings: ['Warning: ddl warning g1', 'Warning: ddl warning g2']}, + // A dataset whose location cannot be read prints that warning once, + // and a push that deploys prints it with each graph. + { + flags: {validateOnly: true}, + services: DENIED, + warnings: [ + NO_LOCATION('test-project'), 'Warning: ddl warning g1', + 'Warning: ddl warning g2', + ], + }, + { + services: DENIED, + warnings: [ + NO_LOCATION('test-project'), 'Warning: ddl warning g1', + NO_LOCATION('test-project'), 'Warning: ddl warning g2', + ], + }, + ], + }, + { + name: 'prints once a pruning note that two deployments give, and a warning the catalog\'s model gave', + entities: [WITH_LTV], + metrics: [{name: 'broken', entity: 'Customer', expression: 'SUM((x'}], + warnings: [ + "Warning: metric 'broken' in model 'commerce' (commerce): kcmd could " + + 'not read its SQL, so the fields it reads were not checked.', + "Note: profile 'default' leaves 1 field(s) unbound.", + ], + }, + { + // The model file binds no table, and g1's profile leaves field ltv unbound. + name: 'prints no pruning note when a later deployment fails a local check, and names the profile in the note once every check passes', + deployments: [{name: 'g1', profile: 'prod'}, {name: 'g2', profile: 'p2'}], + entities: [{...WITH_LTV, source: undefined}], + profiles: { + prod: NO_LTV, + // Push reads a profile's actions only when it applies the profile. + p2: {...NO_LTV, actions: [{name: 'refund', bogus: 1}]}, + }, + code: 1, + errors: [has("action 'refund'")], + warnings: [], + nothingWritten: true, + then: [{ + flags: {deployment: ['g1']}, + code: 0, + warnings: ["Note: profile 'prod' leaves 1 field(s) unbound."], + }], + }, + { + name: '--transpile transpiles the catalog\'s model and each deployment\'s model, with the SQL its profile sets, and prints each warning once', + deployments: ['g1', {name: 'g2', profile: 'p'}], + entities: [WITH_SCORE], + profiles: {p: scoreSql('p', 'SNOWFLAKE', 'IFF(score > 0, score, 0)')}, + metrics: [SNOWFLAKE_METRIC], + flags: {validateOnly: true, transpile: true}, + calls: ['generate g1', 'generate g2'], + warnings: [ + TRANSPILE_WARNING, + "Warning: [commerce, deployment 'g2'] field 'Customer.score': " + + "transpiled 'SNOWFLAKE' -> 'BIGQUERY'", + ], + check: r => { + for (const models of [...r.fake.bq.mock.calls.map(c => c[0]), + r.fake.preflight.mock.calls[0][0]]) { + expect(models[0].model.metrics?.[0]?.expression).toBeDefined(); + } + }, + }, + { + name: 'each deployment prints its own warning about the SQL its profile sets', + deployments: [ + {name: 'a', target: bqGraph('ga'), profile: 'pa'}, + {name: 'b', target: bqGraph('gb'), profile: 'pb'}, + ], + entities: [WITH_SCORE], + profiles: { + pa: scoreSql('pa', 'ANSI_SQL', '((pa'), + pb: scoreSql('pb', 'ANSI_SQL', '((pb'), + }, + flags: {validateOnly: true}, + warnings: [ + has("Warning: [commerce] note: no 'BIGQUERY' dialect"), + ...['a', 'b'].map( + d => "Warning: field 'Customer.score' in model 'commerce' " + + `(commerce, deployment '${d}'): kcmd could not read its SQL, ` + + 'so the fields it reads were not checked.'), + ], + }, +], {deployments: ['g1', 'g2'], code: 0}); diff --git a/toolbox/mdcode/tests/tool/two_phase/harness.ts b/toolbox/mdcode/tests/tool/two_phase/harness.ts index 02c88f299..653179d06 100644 --- a/toolbox/mdcode/tests/tool/two_phase/harness.ts +++ b/toolbox/mdcode/tests/tool/two_phase/harness.ts @@ -9,6 +9,7 @@ import * as YAML from 'yaml'; import {BigQueryClient} from '../../../src/libts/gcp/bigquery'; import {ApiContext} from '../../../src/libts/gcp/context'; import {CatalogClient} from '../../../src/libts/gcp/dataplex'; +import {SpannerClient} from '../../../src/libts/gcp/spanner'; import * as bqLeg from '../../../src/libts/semantic/deploy_bigquery'; import * as kc from '../../../src/libts/semantic/deploy_knowledge_catalog'; import * as spannerLeg from '../../../src/libts/semantic/deploy_spanner'; @@ -19,10 +20,17 @@ import * as commands from '../../../src/tool/commands'; export const CTX = new ApiContext('test-project', 'us', 'test-token'); const BQ = 'bigquery.googleapis.com/projects/test-project/datasets'; +export const SPANNER = + 'spanner.googleapis.com/projects/test-project/instances/i/databases/db'; +export const SPANNER_CUSTOMER = `${SPANNER}/tables/Customer`; +export const ALLOYDB = 'alloydb.googleapis.com/projects/test-project/' + + 'locations/us-central1/clusters/c/databases/db'; export const bqGraph = (name: string, dataset = 'd') => `${BQ}/${dataset}/propertyGraphs/${name}`; export const bqTable = (name: string, dataset = 'd') => `${BQ}/${dataset}/tables/${name}`; +export const spannerGraph = (name: string) => + `${SPANNER}/graphs/${name}`; // Entity Customer of the default scope, bound to table customer in dataset d. export const CUSTOMER = { @@ -44,9 +52,12 @@ export const SNOWFLAKE_METRIC = { export const TRANSPILE_WARNING = "Warning: [commerce] metric 'positive_keys': transpiled 'SNOWFLAKE' -> " + "'BIGQUERY'"; -export const boundTo = (dataset: string) => ({ - entities: {Customer: {...CUSTOMER, source: bqTable('customer', dataset)}}, -}); +// A profile that binds Customer to `source`, or to table customer in +// `dataset`. +export const boundAt = (source: string) => + ({entities: {Customer: {...CUSTOMER, source}}}); +export const boundTo = (dataset: string) => + boundAt(bqTable('customer', dataset)); // Customer bound to table customer in dataset prod, keyed by an expression. export const EXPRESSION_KEY = { ...CUSTOMER, source: bqTable('customer', 'prod'), primary_key: ['UPPER(c_key)'], @@ -66,6 +77,7 @@ export interface Scope { entities?: object[]; relationships?: object[]; metrics?: object[]; + actions?: object[]; // A vanilla model file's GOOGLE block data. The keys above are then ignored. google?: object; // Profile files commerce.profile..yaml, by name. @@ -75,6 +87,7 @@ export interface Scope { defaultProfile?: string; // Other files of the entry group by path, such as a second model file. files?: Record; + noModel?: boolean; } export function modelFile(s: Scope = {}): string { @@ -92,6 +105,7 @@ export function modelFile(s: Scope = {}): string { entities: s.entities ?? [CUSTOMER], relationships: s.relationships, metrics: s.metrics, + actions: s.actions, }; return YAML.stringify({ version: s.google ? '0.2.0.dev0' : '0.2.0.dev0/google', @@ -117,7 +131,7 @@ export function writeScope(dir: string, s: Scope): void { fs.rmSync(eg, {recursive: true, force: true}); fs.mkdirSync(eg, {recursive: true}); const files = { - 'commerce.yaml': modelFile(s), + ...(s.noModel ? {} : {'commerce.yaml': modelFile(s)}), ...Object.fromEntries(Object.entries(s.profiles ?? {}).map( ([n, p]) => [`commerce.profile.${n}.yaml`, profileFile(n, p)])), ...Object.fromEntries(Object.entries(s.legacy ?? {}).map( @@ -160,6 +174,8 @@ const SERVICES = { bqDml: [validateMod, 'validateBigQueryActionStatements', none], spannerDml: [validateMod, 'validateSpannerActionStatements', none], query: [BigQueryClient.prototype, 'query', ok({jobComplete: true})], + getDataset: [BigQueryClient.prototype, 'getDataset', ok({location: 'US'})], + getDatabase: [SpannerClient.prototype, 'getDatabase', ok({})], getEntryType: [CatalogClient.prototype, 'getEntryType', ok({})], getAspectType: [CatalogClient.prototype, 'getAspectType', ok({})], fetch: [globalThis, 'fetch', () => { @@ -168,7 +184,7 @@ const SERVICES = { } satisfies Record; type Service = keyof typeof SERVICES; // Read before any spy replaces them. -const REAL = Object.fromEntries(Object.entries(SERVICES).map( +export const REAL = Object.fromEntries(Object.entries(SERVICES).map( ([name, [owner, method]]) => [name, (owner as any)[method] as Function])); // A call's log entry, such as `apply`, `sources g1` or `deploy g1`. @@ -178,6 +194,10 @@ const entry = (service: Service, a: Call) => /^(sources|bqDml|spannerDml)$/.test(service) ? `${service} ${deploymentNames(a[0])}` : service; +// Answers the calls `applies` holds for. Other calls get the default answer. +export const when = (applies: (...a: any[]) => unknown, answer: unknown) => + (...a: any[]) => applies(...a) ? answer : undefined; + // A case's answers by service: a value, or a function of the call's arguments. export type Services = Partial>; // Each service's spy, and the call log, an entry per call in order. @@ -207,6 +227,8 @@ export type PushRun = Awaited>; // outcomes it sets. interface PushStep extends Scope { flags?: commands.PushOptions; + // Sets KC_V2_ASPECTS=1 for this push. + v2Aspects?: boolean; services?: Services; code?: number; // Every line push prints with console.error, exactly. @@ -215,6 +237,8 @@ interface PushStep extends Scope { log?: unknown[]; // The catalog preflight, the catalog write and each graph deploy, in order. events?: string[]; + // The fake's calls of the kinds listed (an entry's first word), in order. + calls?: string[]; called?: Partial>; nothingWritten?: boolean; noNetwork?: boolean; @@ -260,8 +284,14 @@ async function runPush(services: Services, flags: commands.PushOptions) { log: texts(out, 'log'), errors: texts(out, 'error'), warnings: texts(out, 'warn'), + printed: texts(out, 'log', 'warn'), fake, published: fake.preflight.mock.calls.at(-1)?.[0][0].model as SemanticModel, + deployed: (deployment: string, service: 'bq'|'spanner') => + fake[service].mock.calls + .filter(c => !c[2]?.validateOnly) + .flatMap(c => c[0] as LoadedModel[]) + .find(m => deploymentNames([m]) === deployment)?.model, }; } @@ -278,6 +308,8 @@ async function checkPush(r: PushRun, step: PushStep) { same(r.warnings, step.warnings); same(r.log, step.log); same(r.fake.calls.filter(c => EVENTS.has(kindOf(c))), step.events); + const kinds = new Set(step.calls?.map(kindOf)); + same(r.fake.calls.filter(c => kinds.has(kindOf(c))), step.calls); for (const [service, times] of Object.entries(step.called ?? {})) { expect(r.fake[service as Service]).toHaveBeenCalledTimes(times); } @@ -292,18 +324,30 @@ async function checkPush(r: PushRun, step: PushStep) { await step.check?.(r); } +// Selects every deployment of a Google-flavor scope when `flags.deployment` +// is unset, until push takes two phases for a model that lists deployments. +const everyDeployment = (s: Scope, flags: commands.PushOptions) => + flags.deployment !== undefined || s.google || !s.deployments?.length ? + flags : + { + ...flags, + deployment: s.deployments.map(d => typeof d === 'string' ? d : d.name), + }; + async function runCase(c: Case): Promise { const cwd = process.cwd(); const saved = process.env.KC_V2_ASPECTS; const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'kcmd-two-phase-')); process.chdir(dir); - delete process.env.KC_V2_ASPECTS; try { for (const step of [c, ...c.then ?? []]) { const own = step === c ? {} : step; writeScope(dir, {...c, ...own}); + if (step.v2Aspects) process.env.KC_V2_ASPECTS = '1'; + else delete process.env.KC_V2_ASPECTS; const services = {...c.services, ...own.services}; - await checkPush(await runPush(services, step.flags ?? {}), step); + const flags = everyDeployment({...c, ...own}, step.flags ?? {}); + await checkPush(await runPush(services, flags), step); } } finally { mock.restore(); @@ -333,6 +377,7 @@ export function scenarios( } // The two-phase push's messages, without the `Error: ` prefix. +export const at = (d: string) => `[commerce] deployment '${d}'`; const ownProfile = 'does not apply to a push that uses deployments, since ' + 'each deployment names its own profile.'; export const msg = { @@ -341,11 +386,16 @@ export const msg = { flag: { noKc: "'--no-kc' is not supported; every push publishes the model to " + 'Knowledge Catalog.', - profile: `'--profile' ${ownProfile}`, - allProfiles: `'--all-profiles' ${ownProfile}`, + both: 'Cannot combine --no-deployment with --deployment.', + profile: `'--profile' ${ownProfile} Use '--deployment ' to deploy ` + + 'only some deployments.', + allProfiles: `'--all-profiles' ${ownProfile} A push deploys every ` + + "deployment unless '--deployment' names some.", noProfile: "'--no-profile' does not apply to a push that uses " + "deployments. Use '--no-deployment' to publish to Knowledge Catalog " + 'only.', + bare: '--deployment names no deployment; give one or more deployment ' + + 'names, comma-separated.', }, // A deployment that names a profile the model does not declare. unknownProfile: (d: string, profile: string, defined: string[] = []) => @@ -353,4 +403,7 @@ export const msg = { `profile '${profile}'; ` + (defined.length ? `defined profiles: ${defined.join(', ')}.` : 'no profiles are defined for this model.'), + spannerMetrics: (d: string) => `${at(d)} targets a Spanner graph and uses ` + + "the model file's own bindings, but the model declares metrics, which " + + 'a Spanner graph cannot hold; name a profile with metrics_exclude: "*".', }; diff --git a/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts b/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts index 07a8fe6e3..65941cae3 100644 --- a/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts +++ b/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts @@ -1,11 +1,13 @@ -// Tests of which pushes take two phases and the push flags a push in two -// phases refuses (src/tool/commands.ts). +// Tests of which pushes take two phases, the push flags a push in two phases +// refuses, the deployments `--deployment` selects and checks, and the Spanner +// metrics rule (src/tool/commands.ts, src/libts/semantic/deployment_target.ts). -import {expect} from 'bun:test'; +import {expect, test} from 'bun:test'; import {loadSemanticModels} from '../../../src/libts/semantic/loader'; +import {typedDeployment} from '../../../src/tool/commands'; -import {boundTo, err, modelFile, msg, PushRun, refused, scenarios} from './harness'; +import {boundAt, boundTo, CUSTOMER, err, EXPRESSION_KEY, has, modelFile, msg, PushRun, refused, scenarios, SPANNER_CUSTOMER, spannerGraph} from './harness'; const ONE_PHASE = { called: {preflight: 0, apply: 0, deployKc: 1}, @@ -23,7 +25,7 @@ const ONE_PHASE = { scenarios('the two-phase push path', [ { - name: '--no-kc, --profile, --all-profiles and --no-profile each fail before any network call with --no-deployment', + name: '--no-kc, --profile, --all-profiles and --no-profile each fail before any network call with --no-deployment, as do --no-deployment with --deployment in either order, a bare or empty --deployment and a deployment name the model lacks', deployments: ['g1', 'g2'], profiles: {x: boundTo('x')}, ...refused({kc: false, deployment: false}, err(msg.flag.noKc)), @@ -32,8 +34,40 @@ scenarios('the two-phase push path', [ refused({allProfiles: true, deployment: false}, err(msg.flag.allProfiles)), refused({profile: false, deployment: false}, err(msg.flag.noProfile)), + refused({deployment: [false, 'g1']}, err(msg.flag.both)), + refused({deployment: ['g1', false]}, err(msg.flag.both)), + refused({deployment: [true]}, err(msg.flag.bare)), + refused({deployment: ['']}, err(msg.flag.bare)), + refused({deployment: ['nope']}, err( + "Model 'commerce' has no deployment named 'nope' (available: " + + 'g1, g2).')), ], }, + { + name: '--deployment g1 fails before any network call when the unselected ' + + 'g2 has a target in no accepted form, names a profile the model does ' + + 'not declare, or names a profile whose key column is an expression', + deployments: ['g1', {name: 'g2', target: 'not-a-target'}], + profiles: {prod: boundTo('prod')}, + ...refused({deployment: ['g1']}, has( + "Error: Model 'commerce': deployment 'g2' targets 'not-a-target'")), + then: [{ + deployments: ['g1', {name: 'g2', profile: 'nope'}], + ...refused({deployment: ['g1']}, + err(msg.unknownProfile('g2', 'nope', ['prod']))), + }, { + deployments: ['g1', {name: 'g2', profile: 'prod'}], + profiles: {prod: {entities: {Customer: EXPRESSION_KEY}}}, + ...refused({deployment: ['g1']}, has( + "key column 'UPPER(c_key)' is not a physical column name")), + }], + }, + { + name: '--deployment in a scope with no model is refused', + noModel: true, + ...refused({deployment: ['nope']}, + err("--deployment names 'nope', but the scope has no model.")), + }, { // cac gives `true` for an absent flag that has a negated form. name: 'a model with no deployments list and no profile file does not push in two phases, with no flags, deployment: true or profile: true', @@ -43,4 +77,42 @@ scenarios('the two-phase push path', [ {flags: {profile: true}, code: 0, ...ONE_PHASE}, ], }, + { + name: 'a Spanner deployment that names no profile is refused when the model has metrics and deploys when it has none, and one under a profile whose metrics_exclude is "*" deploys', + deployments: [{name: 'sp', target: spannerGraph('g')}], + entities: [{...CUSTOMER, source: SPANNER_CUSTOMER}], + metrics: [{ + name: 'customers', + entity: 'Customer', + expression: 'COUNT(Customer.key)', + }], + code: 1, + errors: [err(msg.spannerMetrics('sp'))], + called: {preflight: 0}, + nothingWritten: true, + then: [{ + metrics: undefined, + code: 0, + events: ['preflight', 'apply', 'deploy sp'], + }, { + deployments: [{name: 'sp', target: spannerGraph('g'), profile: 'ops'}], + entities: [CUSTOMER], + profiles: {ops: {...boundAt(SPANNER_CUSTOMER), metrics_exclude: '*'}}, + code: 0, + events: ['preflight', 'apply', 'deploy sp'], + }], + }, ], {code: 0}); + +// Each row gives the command line and the option push gets. +test.each([ + [['--deployment'], [true]], + [['--deployment', '--validate-only'], [true]], + [['--deployment', ''], ['']], + [['--deployment=a,b'], ['a,b']], + [['--deployment', 'a', '--deployment', 'b'], ['a', 'b']], + [['--', '--deployment', 'a'], true], + [['--deployment', 'g1', '--no-deployment'], ['g1', false]], + [['-no-deployment'], false], +])('typedDeployment(%p)', (args, expected) => + expect(typedDeployment(args)).toEqual(expected)); From 7a295122d1ae4200b4ea34b55527afbf1efe498d Mon Sep 17 00:00:00 2001 From: Fei Xu Date: Sat, 24 Oct 2026 03:19:18 +0000 Subject: [PATCH 2/2] fix(mdcode): report a selected deployment's profile errors with the other errors A push with --deployment left the key, join, expression and hierarchy checks of each selected deployment's profile to that deployment's own checks. Those run only after every other local check passes, and stop at the first deployment that fails. A push that failed an earlier check reported these errors only on the next run. The profile check now gives every error of every profile, as it does under --no-deployment. It leaves to a selected deployment's checks only the warning about SQL in its profile that kcmd cannot read. --- .../src/libts/semantic/resolve_profiles.ts | 8 +++++++- toolbox/mdcode/src/tool/commands.ts | 20 ++++++++++++------- .../libts/semantic/resolve_profiles.test.ts | 6 ++++++ .../tool/two_phase/two_phase_path.test.ts | 16 ++++++++++++++- 4 files changed, 41 insertions(+), 9 deletions(-) diff --git a/toolbox/mdcode/src/libts/semantic/resolve_profiles.ts b/toolbox/mdcode/src/libts/semantic/resolve_profiles.ts index fbd9ad31f..6a7a4a31a 100644 --- a/toolbox/mdcode/src/libts/semantic/resolve_profiles.ts +++ b/toolbox/mdcode/src/libts/semantic/resolve_profiles.ts @@ -1993,6 +1993,10 @@ export interface CompletenessOptions extends CloudSqlLoad { // applied, the caller stops reading what the SQL reads. This check then // reads what the profile's field expressions read, and gives the warning. applied?: boolean; + // The caller warns about the profile's SQL that the parser cannot read. This + // check then gives that warning only when the hierarchy does not resolve + // with the profile applied. + sqlWarned?: boolean; // The caller warns about the model file's SQL that it reads, which it does // only when the hierarchy of the model it checks resolves. It reads an // ANSI_SQL text in `dialect`, or as generic SQL when that is unset, and @@ -2392,7 +2396,9 @@ export function validateProfileCompleteness( return what === fieldLabel(f.slice(0, dot), f.slice(dot + 1)); }); for (const what of unreadable) { - if (applied ? !profiledError : !own.has(what) && callerWarns(what)) { + if (applied ? !profiledError : + opts.sqlWarned && !profiledError || + !own.has(what) && callerWarns(what)) { continue; } warnings.push( diff --git a/toolbox/mdcode/src/tool/commands.ts b/toolbox/mdcode/src/tool/commands.ts index 191f26569..5d21533cd 100644 --- a/toolbox/mdcode/src/tool/commands.ts +++ b/toolbox/mdcode/src/tool/commands.ts @@ -637,8 +637,8 @@ const PUSH_REPORT: PrepareReport = { // A model's profiles are checked against its model file as loaded, whichever // binding push selects. `checkedByDoc` // holds each document as push checks it: with the profile applied, or loaded -// with no profile applied. Push's own checks cover part of this for each -// profile `applied` says push applies, so those rules leave that profile out. A +// with no profile applied. `pushChecks` says what push's own checks cover for +// each profile, so these checks leave that out. A // legacy `semantic_model:` wrapper, and a file under the reserved name // 'default', which no push deploys, are not read here. With `transpile`, push // fills in BigQuery SQL for a text written only in a vendor dialect its @@ -647,7 +647,8 @@ const PUSH_REPORT: PrepareReport = { // `pushedProfile`, so this check leaves it out. The warnings go to `warnings`. function namedProfileErrors( docs: ProfiledDoc[], checkedByDoc: Map, - applied: (doc: ProfiledDoc, profile: string) => boolean, + pushChecks: (doc: ProfiledDoc, profile: string) => + {applied?: boolean; sqlWarned?: boolean}, profileDocs: ProfileDocs, declared: DeclaredProfiles, warnings: string[], transpile: boolean, pushedProfile: string, load: CloudSqlLoad): string[] { const errors: string[] = []; @@ -730,7 +731,7 @@ function namedProfileErrors( const profileWarnings: string[] = []; errors.push( ...validateProfileCompleteness(logical, spec, profileWarnings, { - applied: applied(d, p.name), + ...pushChecks(d, p.name), ownSqlOnly, transpiled: transpile, ...load, @@ -873,7 +874,8 @@ async function prepareModels( const unprunedWarnings: string[] = []; const profileErrors = [ ...namedProfileErrors( - docs, checkedByDoc, (d, name) => !!d.applied && name === profileName, + docs, checkedByDoc, + (d, name) => ({applied: !!d.applied && name === profileName}), opts.profileDocs, opts.declared, unprunedWarnings, !!opts.transpile, profileName, opts.load), ...modelFileBindingErrors( @@ -1834,11 +1836,15 @@ async function checkBeforeWrites( // Every named profile is checked on its own against the model file, and so // are the model file's own bindings and each deployment. The catalog's model // is checked as a model that deploys nowhere, though one with no deployments - // list keeps its GOOGLE block targets. + // list keeps its GOOGLE block targets. A selected deployment's own checks + // run only after these pass, so these give every error. They leave to those + // checks only the warning about its profile's SQL that kcmd cannot read. const profileWarnings: string[] = []; const profileErrors = namedProfileErrors( docs, new Map(loaded.models.map(m => [m.document, m.model])), - (d, name) => !!selectedByDoc.get(d.name)?.some(s => s.profile === name), + (d, name) => ({ + sqlWarned: !!selectedByDoc.get(d.name)?.some(s => s.profile === name), + }), profileDocs, declaredBy, profileWarnings, !!options.transpile, DEFAULT_PROFILE, load); for (const {document, model} of models) { diff --git a/toolbox/mdcode/tests/libts/semantic/resolve_profiles.test.ts b/toolbox/mdcode/tests/libts/semantic/resolve_profiles.test.ts index f5cc483bd..0bb1dbca3 100644 --- a/toolbox/mdcode/tests/libts/semantic/resolve_profiles.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/resolve_profiles.test.ts @@ -2784,6 +2784,12 @@ relationships: [] "profile 'prod': kcmd could not read the SQL of field 'F.f', so the " + 'fields it reads were not checked', ]); + // A --deployment push warns about the model file's SQL when it loads the + // model file, so this check leaves that warning out. + const deferred: string[] = []; + validateProfileCompleteness(model, spec, deferred, + {sqlWarned: true, ownSqlOnly: {dialect: 'BIGQUERY'}}); + expect(deferred).toEqual([]); }); // Push's own check of what a field reads stands down on a broken hierarchy diff --git a/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts b/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts index 65941cae3..64d5409f2 100644 --- a/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts +++ b/toolbox/mdcode/tests/tool/two_phase/two_phase_path.test.ts @@ -46,7 +46,8 @@ scenarios('the two-phase push path', [ { name: '--deployment g1 fails before any network call when the unselected ' + 'g2 has a target in no accepted form, names a profile the model does ' + - 'not declare, or names a profile whose key column is an expression', + 'not declare, or names a profile whose key column is an expression, ' + + "and with g2 selected that error prints beside another profile's error", deployments: ['g1', {name: 'g2', target: 'not-a-target'}], profiles: {prod: boundTo('prod')}, ...refused({deployment: ['g1']}, has( @@ -60,6 +61,19 @@ scenarios('the two-phase push path', [ profiles: {prod: {entities: {Customer: EXPRESSION_KEY}}}, ...refused({deployment: ['g1']}, has( "key column 'UPPER(c_key)' is not a physical column name")), + }, { + deployments: [{name: 'g1', profile: 'p1'}, {name: 'g2', profile: 'prod'}], + profiles: { + p1: {entities: {Customer: {...CUSTOMER, primary_key: undefined}}}, + prod: {entities: {Customer: EXPRESSION_KEY}}, + }, + flags: {deployment: ['g1', 'g2']}, + code: 1, + errors: [ + has("profile 'p1': entity 'Customer' has no primary_key"), + has("profile 'prod': entity 'Customer' key column 'UPPER(c_key)'"), + ], + noNetwork: true, }], }, {