From 2759284d991fff74301b7e10a47907d6c3c8bc3d Mon Sep 17 00:00:00 2001 From: Fei Xu Date: Thu, 22 Oct 2026 03:08:33 +0000 Subject: [PATCH] feat(mdcode): push every model that lists deployments or has a sibling profile file in two phases, and report its errors --- .../mdcode/docs/semantic-model/fidelity.md | 3 +- .../mdcode/docs/semantic-model/profiles.md | 10 +- .../mdcode/docs/semantic-model/reference.md | 67 ++- toolbox/mdcode/src/libts/semantic/loader.ts | 16 - toolbox/mdcode/src/libts/semantic/validate.ts | 6 +- toolbox/mdcode/src/tool/commands.ts | 293 +++++++++--- toolbox/mdcode/src/tool/main.ts | 8 +- toolbox/mdcode/tests/libts/mocks.ts | 11 + .../tests/libts/semantic/push_plan.test.ts | 166 ++----- .../tests/libts/semantic/validate.test.ts | 13 +- toolbox/mdcode/tests/tool/commands.test.ts | 451 ++++++++---------- toolbox/mdcode/tests/tool/profiles.test.ts | 234 ++++----- .../mdcode/tests/tool/store_agreement.test.ts | 109 +++-- .../tests/tool/two_phase/catalog_only.test.ts | 54 ++- .../tests/tool/two_phase/deploy.test.ts | 4 +- .../mdcode/tests/tool/two_phase/harness.ts | 101 +++- .../tests/tool/two_phase/report.test.ts | 197 ++++++++ .../tool/two_phase/two_phase_path.test.ts | 90 +++- 18 files changed, 1082 insertions(+), 751 deletions(-) create mode 100644 toolbox/mdcode/tests/tool/two_phase/report.test.ts diff --git a/toolbox/mdcode/docs/semantic-model/fidelity.md b/toolbox/mdcode/docs/semantic-model/fidelity.md index 341839da..1687ffea 100644 --- a/toolbox/mdcode/docs/semantic-model/fidelity.md +++ b/toolbox/mdcode/docs/semantic-model/fidelity.md @@ -169,7 +169,8 @@ Spanner, say — and `kcmd push --all-profiles` deploys a graph for each. The catalog leg does not fan out with it: it runs once, against the profile named by `--profile`, otherwise the default binding. So a bare `kcmd push` and a `kcmd push --all-profiles` write the same entries and differ only in how many -graphs they deploy. +graphs they deploy. A [push in two phases](reference.md#push) records the model +file as written instead. Nor do successive pushes accumulate. Entry ids are derived from logical names alone — model, entity, metric — with no profile component, so pushing a second diff --git a/toolbox/mdcode/docs/semantic-model/profiles.md b/toolbox/mdcode/docs/semantic-model/profiles.md index 1f276653..e01d6036 100644 --- a/toolbox/mdcode/docs/semantic-model/profiles.md +++ b/toolbox/mdcode/docs/semantic-model/profiles.md @@ -247,11 +247,8 @@ 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. `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` +deployment target. `kcmd push` deploys each deployment with the profile it +names. `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 against. @@ -477,7 +474,8 @@ once for every defined profile — for example the analytical (BigQuery) and operational (Spanner) graphs in a single run. Knowledge Catalog still records one canonical view of the logical model — the default binding — so a bare `kcmd push` and a `kcmd push --all-profiles` write the same catalog entries and differ only in -how many graphs deploy. +how many graphs deploy. A [push in two phases](reference.md#push) records the +model file as written, whatever `default_profile` names. There is always a **default binding profile**: whatever `default_profile` names, or — if it is unset — the model's inline bindings (the implicit `default` diff --git a/toolbox/mdcode/docs/semantic-model/reference.md b/toolbox/mdcode/docs/semantic-model/reference.md index 87f4986c..6be0ddea 100644 --- a/toolbox/mdcode/docs/semantic-model/reference.md +++ b/toolbox/mdcode/docs/semantic-model/reference.md @@ -23,20 +23,21 @@ existing group is fine) and creates its local directory, kcmd push ``` -With no flags, deploys to every destination — the model's graph backend and -Knowledge Catalog — the graph first. You do not choose the graph backend: each +With no flags, deploys to every destination, the model's graph backends and +Knowledge Catalog, in the order below. You do not choose the graph backend: each model deploys to whichever its deployment target names (BigQuery Graph or Spanner Graph). The binding profile selects which physical binding feeds the graph, and that binding's deployment target selects the backend. A push has two axes, both defaulted so a bare `kcmd push` deploys the graph for -the default binding and records to Knowledge Catalog. The **binding-profile axis** +the default binding and records to Knowledge Catalog, unless it takes the +two-phase path below. The **binding-profile axis** sets how many profiles the graph deploys for; the **Knowledge Catalog axis** is whether the catalog leg runs. | Flag | Effect | |------|--------| -| `--profile ` | Deploy the graph for one binding profile (`.profiles/.yaml`); its deployment target selects the backend. Defaults to `default_profile` from `catalog.yaml`, else the model's inline bindings. Mutually exclusive with `--all-profiles` and `--no-profile`. See [Binding profiles](profiles.md). | +| `--profile ` | Deploy the graph for one binding profile (`.profiles/.yaml`); its deployment target selects the backend. Defaults to `default_profile` from `catalog.yaml`, else the model's inline bindings, except on the two-phase path below. Mutually exclusive with `--all-profiles` and `--no-profile`. See [Binding profiles](profiles.md). | | `--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). | @@ -47,7 +48,9 @@ whether the catalog leg runs. | `--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 `--deployment` or `--no-deployment` runs in two phases. The first +A model that lists `deployments:`, and a push with `--deployment` or +`--no-deployment`, run in two phases. So does a push of a model with a sibling +profile file, without `--profile`, `--all-profiles` or `--no-profile`. 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 @@ -57,7 +60,9 @@ 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. +as written. The runtime still reads `default_profile`, so it must name a +profile the model declares, and that profile must apply as the runtime +applies it. Any other push prepares the catalog's model, checks that it publishes one model and checks the file copies before the first graph deploys. Its Knowledge @@ -89,14 +94,15 @@ kcmd profiles Reports each binding profile the model declares: its deployment target, the source each entity binds to, and what the profile cannot answer or cannot run. -Under each profile it then prints every error `kcmd push --profile ` +Under each profile it then prints every error push would give for it, after a `push rejects:` heading. The inline `default` binding is checked too. Beside declared profiles it is listed only when push would reject it. Some errors fail every push of the scope, such as a second model in the entry group. When push would stop at such an error, `kcmd profiles` prints it once above the profiles and still checks each -profile. A Cloud SQL table whose entry cannot be read stops the command before -the report. When `default_profile` names a profile that a model does not declare, +profile, unless a model lists deployments. A Cloud SQL table whose entry +cannot be read stops the command before the report. When +`default_profile` names a profile that a model does not declare, a push that names no profile fails. That error prints above the model's profiles. When push warns that the model file's own bindings are incomplete, the report prints that warning under `profile 'default'`, unless `--profile` @@ -107,11 +113,22 @@ graph as `push --validate-only` does, but deploys nothing, so you can compare coverage before choosing one. Like push, it reads the Knowledge Catalog entry of each Cloud SQL table written as its resource name, for the table's dialect. The other checks that need a service run only at push time: the BigQuery source table probe, the dry runs of action -statements and, on a push without `--validate-only`, the Knowledge Catalog +statements and of each BigQuery graph's DDL, the check that a deployment's +BigQuery dataset or Spanner database exists and, on a push without +`--validate-only`, the Knowledge Catalog checks on the entry group. Push's lookup of the file-copy type also runs only at push time, because it calls a service. It exits non-zero when push would -reject a binding. Until push accepts a deployment that names a profile, the -report refuses one, while `--print-store` takes that profile's store from it. +reject a binding. When no model in the scope lists deployments, the report +checks each profile as `kcmd push --profile ` does. When a model lists +deployments, the report gives the errors of a plain `kcmd push` and stops where +that push stops. Under each profile it prints the errors of that profile's own +checks and of each deployment that uses it. The errors of a deployment that +names no profile print under `default`. With `--profile `, the other +profiles' errors print above the profiles too, since any of them stops the +push. Push applies only the profiles the deployments use and the one +`default_profile` names, so it can accept a profile that does not apply. When +push gives no error and a profile does not apply, the report prints why on a +`not shown:` line under that profile. The report's `target:` line lists the targets of the deployments the profile uses, the same ones `--print-store` reads. For an AlloyDB target named by its cluster, `--print-store` calls the AlloyDB API to find the cluster's primary @@ -149,10 +166,13 @@ action" section. The command fails without writing anything when push rejects the deployments. That happens when the target of any deployment is in no accepted form, whichever profile the deployment is for. It also happens when several targets leave the store ambiguous, or when a deployment names a profile -the model does not declare. Until push accepts a deployment that names a -profile, the command accepts one that names `default` or a declared profile, -which push refuses. It also accepts a profile file that no deployment uses, -which every graph push refuses, and writes a skill whose store line says there +the model does not declare. It accepts a deployment that names `default` or a +declared profile, as push does, but also one that names a legacy +`semantic_model:` profile, which push refuses. It accepts a Spanner deployment +that names no profile in a model with metrics, which push refuses. It fails on +two BigQuery deployments that use one profile, since they leave no single +store, while push deploys both. It also accepts a profile file that no +deployment uses, as push does, and writes a skill whose store line says there is none. A vanilla model lists its deployments or targets in a `GOOGLE` block. The command fails the same way when a target there is in no accepted form or listed twice, or when the block's data is not valid JSON. Like push, it reads @@ -415,9 +435,9 @@ differs from BigQuery Graph in five ways: names — no `project.dataset.` qualifier on either the tables or the graph. - **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)). 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`. + 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: "*"`. - **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 @@ -592,8 +612,10 @@ defined in [model spec §1](model_spec.md#1-status-and-baseline), and [§4.1](model_spec.md#41-narrowings-stricter-than-ossie). * **A graph push declares exactly one deployment target per model, and it must - be a valid BigQuery Graph or Spanner Graph resource name.** A model with more - than one is rejected, and so is a single target that matches neither + be a valid BigQuery Graph or Spanner Graph resource name.** A model that lists + `deployments:` pushes in two phases and holds each BigQuery and Spanner + deployment to this rule on its own. Otherwise a model with more than one is + rejected, and so is a single target that matches neither `bigquery.googleapis.com/projects/

/datasets//propertyGraphs/` nor `spanner.googleapis.com/projects/

/instances//databases//graphs/` (for example a `propertyGraph` typo, a Spanner graph under `propertyGraphs/`, @@ -624,8 +646,7 @@ and [§4.1](model_spec.md#41-narrowings-stricter-than-ossie). measure, so each metric must be `SUM`, `AVG`, `COUNT`, `MIN` or `MAX` over one operand, and the entity it belongs to needs a primary key or a unique key. The measure rule skips a metric that has only a vendor text. - A push of a profile in the older `.profiles/` layout deploys that - profile's bindings instead, so it skips this check. *(static)* + *(static)* * **Each field the model file binds has a text its entity's database runs.** The field needs an entry in the dialect of its entity's source, an `ANSI_SQL` entry, or the string form. The model file's own bindings may span diff --git a/toolbox/mdcode/src/libts/semantic/loader.ts b/toolbox/mdcode/src/libts/semantic/loader.ts index 975bf392..912c9c10 100644 --- a/toolbox/mdcode/src/libts/semantic/loader.ts +++ b/toolbox/mdcode/src/libts/semantic/loader.ts @@ -3573,19 +3573,3 @@ export function loadSemanticModels( } return {models, warnings}; } - -// Why a loaded model cannot be used yet because one of its deployments names a -// binding profile, or undefined when none does. -export function deploymentProfileError(models: LoadedModel[]): string|undefined { - for (const {document, model} of models) { - for (const d of model.deployments ?? []) { - if (d.profile !== undefined) { - return `[${document}] model '${model.name}': deployment '${ - d.name}' names profile '${d.profile}'; remove 'profile' from ` + - `the deployment entry so it uses the model file's own bindings.`; - } - } - } - return undefined; -} - diff --git a/toolbox/mdcode/src/libts/semantic/validate.ts b/toolbox/mdcode/src/libts/semantic/validate.ts index 5c15ab4b..4e8e31d8 100644 --- a/toolbox/mdcode/src/libts/semantic/validate.ts +++ b/toolbox/mdcode/src/libts/semantic/validate.ts @@ -133,8 +133,8 @@ export function validatePushRequirements( `model runs against rather than deploys to; AlloyDB has no ` + `property graph for a push to publish. Declare a BigQuery or ` + `Spanner Graph target (${ - where ?? DEPLOYMENTS_VERSION_HINT}; push uses exactly one ` + - `deployment for now), or run 'kcmd push --no-profile' to publish ` + + where ?? DEPLOYMENTS_VERSION_HINT}), or ` + + `run 'kcmd push --no-profile' to publish ` + `to Knowledge Catalog only.`); } } @@ -482,7 +482,7 @@ function metricFieldCollisionError( // entry that names nothing real leaves a reader believing the blast radius is // described when it is not, and a consumer routing on it would route on a // concept that does not exist. -function validateActions( +export function validateActions( model: SemanticModel, document: string, fieldsPruned: boolean, checkDescriptions: boolean): string[] { const errors: string[] = []; diff --git a/toolbox/mdcode/src/tool/commands.ts b/toolbox/mdcode/src/tool/commands.ts index fd0e0b13..62a6b004 100644 --- a/toolbox/mdcode/src/tool/commands.ts +++ b/toolbox/mdcode/src/tool/commands.ts @@ -20,7 +20,7 @@ import {cloudSqlDialects} from '../libts/semantic/cloudsql_dialect'; 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, OSSIE_VERSION, SemanticModelError, setsAKeyTwice, writtenSource} from '../libts/semantic/loader'; +import {isGoogleVendor, LoadedModel, loadSemanticModels, OSSIE_VERSION, SemanticModelError, setsAKeyTwice, writtenSource} from '../libts/semantic/loader'; import {serializeModel, serializeModelFiles} from '../libts/semantic/osi_converter'; import * as pullKc from '../libts/semantic/pull_kc'; 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'; @@ -29,7 +29,7 @@ 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, storeMessage, validateBigQueryActionStatements, validateBigQueryDataSources, validateDeployments, validateExpressionReferences, validateInheritance, validateInheritedColumns, validateInlineDialects, validatePushRequirements, validateSpannerActionStatements} from '../libts/semantic/validate'; +import {inlineBindingWarnings, storeMessage, validateActions, 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'; @@ -630,6 +630,16 @@ const PUSH_REPORT: PrepareReport = { error: m => console.error(`Error: ${m}`), }; +// An error of a two-phase push's checks, and what it belongs to: the whole +// push when `document` is unset, the model in `document` when `profile` is +// unset, and that profile of the model otherwise. A deployment's errors belong +// to the profile it uses, which is 'default' when it names none. +interface LocalCheckError { + error: string; + document?: string; + profile?: string; +} + // The errors the named profiles of each model in `docs` give: each profile // file, and each profile a vanilla model carries in its GOOGLE block, checked // on its own for completeness. An error about a profile file names that file, @@ -650,8 +660,9 @@ function namedProfileErrors( 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[] = []; + transpile: boolean, pushedProfile: string, + load: CloudSqlLoad): LocalCheckError[] { + const errors: LocalCheckError[] = []; for (const d of docs) { // Each named profile, with the label its errors carry and a function that // reads it. declared() lists the profile files too, so only its block @@ -677,7 +688,7 @@ function namedProfileErrors( } catch (err: any) { // A profile file that breaks one of the layout's file rules, or block // profiles a load rejects. - errors.push(`${err.message || err}`); + errors.push({error: `${err.message || err}`, document: d.name}); continue; } // The model file as loaded on its own, which is the model push checks @@ -697,7 +708,7 @@ function namedProfileErrors( if (!(err instanceof SemanticModelError)) throw err; error = `[${p.label}] ${err.message}`; } - errors.push(error); + errors.push({error, document: d.name, profile: p.name}); } } continue; @@ -719,12 +730,14 @@ function namedProfileErrors( const specs: ProfileSpec[] = []; for (const p of named) { const at = (m: string) => `[${p.label}] ${m}`; + const tag = (m: string) => + ({error: at(m), document: d.name, profile: p.name}); let spec: ProfileSpec; try { spec = p.read(); } catch (err) { if (!(err instanceof SemanticModelError)) throw err; - errors.push(at(err.message)); + errors.push(tag(err.message)); continue; } specs.push(spec); @@ -735,11 +748,11 @@ function namedProfileErrors( ownSqlOnly, transpiled: transpile, ...load, - }).map(at)); + }).map(tag)); warnings.push(...profileWarnings.map(at)); } errors.push(...validateProfileConsistency(logical, specs, pushedProfile) - .map(e => `[${d.name}] ${e}`)); + .map(e => ({error: `[${d.name}] ${e}`, document: d.name}))); } return errors; } @@ -796,7 +809,7 @@ function reboundFields(base: SemanticModel, pushed: SemanticModel): string[] { // one IR across the push legs. `prune` drops each unbound field, and whatever // depends on it, so a deployed graph presents only what its binding answers; a // catalog-only push leaves it off to publish the whole logical model. Once -// the documents load and no deployment names a profile, every named profile +// the documents load, every named profile // the models declare is checked too: the profile files `profileDocs` reads, // and the GOOGLE block profiles `declared` adds. So is each deployment. Runs // local checks only. Returns the models and the target partition the graph @@ -857,11 +870,6 @@ async function prepareModels( {document: d.name, model: prune ? d.applied : withoutExclusions(d.applied)} : loadedByDoc.get(d.name)!); - const depProfileErr = deploymentProfileError(models); - if (depProfileErr) { - report.error(depProfileErr); - return null; - } // The fields and metrics a legacy wrapper excludes, by document. const excludedByDoc = new Map(docs.map(d => [d.name, d.excluded ?? []] as const)); @@ -877,7 +885,8 @@ async function prepareModels( docs, checkedByDoc, (d, name) => ({applied: !!d.applied && name === profileName}), opts.profileDocs, opts.declared, unprunedWarnings, !!opts.transpile, - profileName, opts.load), + profileName, opts.load) + .map(e => e.error), ...modelFileBindingErrors( docs, checkedByDoc, opts.declared, unprunedWarnings, !!opts.transpile), ]; @@ -1074,7 +1083,6 @@ async function generateOffline( 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 @@ -1086,7 +1094,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 (named || negated) { + if (takesTwoPhasePath(options, layout, layoutDocs)) { return pushInTwoPhases(options, { ctx, layout, @@ -1534,6 +1542,7 @@ export async function push(options: PushOptions): Promise { // These flags only take effect on a semantic-model push; on a regular // catalog snapshot they are inert. Warn rather than silently ignore them, so // a user who expected (say) --transpile to run isn't misled by a clean exit. + const {named, negated} = deploymentFlags(options.deployment); const semanticOnlyFlags: Array<[boolean, string]> = [ [typeof options.profile === 'string', '--profile'], [!!options.allProfiles, '--all-profiles'], @@ -1573,6 +1582,34 @@ export async function push(options: PushOptions): Promise { } +// Whether a model document's text gives a model a non-empty `deployments` +// list, directly or in a GOOGLE block's data. YAML or JSON that does not +// parse lists none. +function listsDeployments(text: string): boolean { + let doc: any; + try { + doc = yaml.parse(text, YAML_OPTIONS); + } catch { + return false; + } + const nonEmpty = (v: unknown) => Array.isArray(v) && v.length > 0; + const models = Array.isArray(doc?.semantic_model) ? doc.semantic_model : []; + return models.some((m: any) => { + if (nonEmpty(m?.deployments)) return true; + const exts = Array.isArray(m?.custom_extensions) ? m.custom_extensions : []; + return exts.some((ext: any) => { + if (!isGoogleVendor(ext?.vendor_name) || typeof ext?.data !== 'string') { + return false; + } + try { + return nonEmpty(JSON.parse(ext.data)?.deployments); + } catch { + return false; + } + }); + }); +} + // Which of `--deployment` and `--no-deployment` the push options carry. function deploymentFlags(deployment: PushOptions['deployment']): {named: boolean; negated: boolean} { @@ -1602,6 +1639,25 @@ export function typedDeployment(args: string[]): PushOptions['deployment'] { return deploymentFlags(typed).named ? typed : !typed.length; } +// Whether push checks everything before it writes anything (see +// pushInTwoPhases). cac gives an absent `--profile` the value true, so only +// another defined value counts as passing `--profile` or `--no-profile`. +function takesTwoPhasePath( + options: PushOptions, layout: SemanticModelLayout, + docs: {name: string; text: string}[]): boolean { + const {named, negated} = deploymentFlags(options.deployment); + if (named || negated) return true; + if (docs.some(d => listsDeployments(d.text))) return true; + if ((options.profile !== undefined && options.profile !== true) || + options.allProfiles) { + return false; + } + // localProfileFiles gives a profile name only to a sibling profile file. + return docs.some( + d => layout.localProfileFiles(d.name).some( + f => f.profileName !== undefined)); +} + // The error for a flag the two-phase path does not take, or undefined. function twoPhaseFlagError(options: PushOptions): string|undefined { if (options.kc === false) { @@ -1660,7 +1716,7 @@ async function pushInTwoPhases( options: PushOptions, scope: Omit): Promise { const {ctx, source} = scope; const fail = (...errors: string[]) => { - for (const e of errors) console.error(`Error: ${e}`); + for (const e of new Set(errors)) console.error(`Error: ${e}`); return 1; }; const flagError = twoPhaseFlagError(options); @@ -1668,7 +1724,7 @@ async function pushInTwoPhases( const load = await cloudSqlLoad(ctx, scope.docs, scope.profileDocs); if (!load) return 1; const checked = await checkBeforeWrites(options, {...scope, load}); - if (!checked) return 1; + if ('errors' in checked) return fail(...checked.errors.map(e => e.error)); const {plans, kcModels, fileCopies, notes, overviews} = checked; if (fileCopies.length) { const problem = await kc.checkCustomTypes( @@ -1741,25 +1797,27 @@ async function pushInTwoPhases( // The local checks of a two-phase push, which call no service: it stops at // 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. +// it writes and what pruning leaves out, or the errors that stop it. With +// `quiet`, for the `kcmd profiles` report, it prints no warning. async function checkBeforeWrites( - options: PushOptions, scope: TwoPhaseScope): Promise<{ + options: PushOptions, scope: TwoPhaseScope, quiet = false): Promise<{ plans: DeploymentPlan[]; kcModels: LoadedModel[]; fileCopies: FileCopy[]; notes: string[]; overviews?: Map; -}|undefined> { +}|{errors: LocalCheckError[]}> { 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; - }; + const stop = (errors: LocalCheckError[]) => ({errors}); + const fail = (...errors: string[]) => stop(errors.map(error => ({error}))); + const ofModel = (document: string, ...errors: string[]) => + errors.map(error => ({error, document})); + const ofProfile = (document: string, profile: string, ...errors: string[]) => + errors.map(error => ({error, document, profile})); // 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 (quiet || options.transpile && w.includes('needs transpilation')) return; if (printed.has(w) || printed.has(asCatalog)) return; printed.add(w); console.warn(`Warning: ${w}`); @@ -1792,7 +1850,7 @@ async function checkBeforeWrites( const models: LoadedModel[] = []; for (const base of loaded.models) { const model = withProfileFiles(base, profileDocs); - if ('error' in model) return fail(model.error); + if ('errors' in model) return stop(model.errors); models.push(model); } // Every profile each model declares, legacy wrappers included. @@ -1821,17 +1879,20 @@ async function checkBeforeWrites( const selectedByDoc = new Map(); for (const {document, model} of models) { const every = resolveModelDeployments(model); - if ('error' in every) return fail(every.error); + if ('error' in every) return stop(ofModel(document, every.error)); const resolved = noDeployment ? {deployments: []} : resolveModelDeployments(model, selected); - if ('error' in resolved) return fail(resolved.error); + if ('error' in resolved) return stop(ofModel(document, resolved.error)); const undeclared = undeclaredProfileError( document, model, declaredNames(document)); - if (undeclared) return fail(undeclared); + if (undeclared) return stop(ofModel(document, undeclared)); selectedByDoc.set(document, resolved.deployments); } - const byDefault = defaultProfileCheck(scope, models, declaredBy); - if ('error' in byDefault) return fail(byDefault.error); + const byDefault = defaultProfileCheck( + scope, models, declaredBy, [...selectedByDoc.values()].flat(), + defaultProject); + if ('errors' in byDefault) return stop(byDefault.errors); + if (byDefault.warning) warn(byDefault.warning); // 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 @@ -1848,11 +1909,12 @@ async function checkBeforeWrites( profileDocs, declaredBy, profileWarnings, !!options.transpile, DEFAULT_PROFILE, load); for (const {document, model} of models) { - profileErrors.push( + profileErrors.push(...ofProfile( + document, DEFAULT_PROFILE, ...validateInlineDialects( model, document, {transpiled: !!options.transpile}), ...validateInheritedColumns(model, document), - ...validateDeployments(model, document)); + ...validateDeployments(model, document))); const named = declaredNames(document).some(n => n.toLowerCase() !== DEFAULT_PROFILE); profileWarnings.push(...inlineBindingWarnings(model, document, named)); @@ -1865,7 +1927,7 @@ async function checkBeforeWrites( } const kcWarnings: string[] = []; const kcErrors = kcModels.flatMap( - m => validatePushRequirements( + m => ofModel(m.document, ...validatePushRequirements( [m.model.deployments === undefined ? m : {...m, model: {...m.model, deployments: []}}], @@ -1873,10 +1935,10 @@ async function checkBeforeWrites( targetOptional: true, warnings: kcWarnings, v2Aspects: kcV2Aspects(), - })); + }))); for (const w of [...profileWarnings, ...kcWarnings]) warn(w); if (profileErrors.length || kcErrors.length) { - return fail(...profileErrors, ...kcErrors); + return stop([...profileErrors, ...kcErrors]); } const plans: DeploymentPlan[] = []; @@ -1887,7 +1949,8 @@ async function checkBeforeWrites( base, profileDocs(base.document), deployment, ctx, options, warn, notes); if ('errors' in plan) { - return fail(...plan.errors); + return stop( + ofProfile(base.document, deployment.profile, ...plan.errors)); } plans.push(plan); } @@ -1900,17 +1963,17 @@ async function checkBeforeWrites( // first profile file that breaks a file rule. A profile file that does not load // is left out, for namedProfileErrors to report. function withProfileFiles(base: LoadedModel, profileDocs: ProfileDocs): - LoadedModel|{error: string} { + LoadedModel|{errors: LocalCheckError[]} { const {document, model} = base; const profiles = [...(model.profiles ?? [])]; let files: ReturnType; try { files = profileDocs(document); } catch (err: any) { - return {error: err.message || String(err)}; + return {errors: [{error: err.message || String(err), document}]}; } const reserved = reservedProfileNameError(document, files); - if (reserved) return {error: reserved}; + if (reserved) return {errors: [{error: reserved, document}]}; for (const p of files) { if (isLegacyWrapper(p.text)) continue; try { @@ -1922,25 +1985,64 @@ function withProfileFiles(base: LoadedModel, profileDocs: ProfileDocs): return {document, model: profiles.length ? {...model, profiles} : model}; } -// Checks that each model declares catalog.yaml's default_profile, since the -// runtime refuses a scope whose default_profile a model lacks. +// Checks a default_profile other than 'default': each model declares it, and +// it applies with its actions, as the runtime applies it. Gives a warning when +// no deployment uses it or a selected deployment names no profile. function defaultProfileCheck( - scope: TwoPhaseScope, models: LoadedModel[], declaredBy: DeclaredProfiles): - {error: string}|{} { - const {defaultProfile, docs, load} = scope; - if (defaultProfile === undefined || defaultProfile === DEFAULT_PROFILE) { + scope: TwoPhaseScope, models: LoadedModel[], declaredBy: DeclaredProfiles, + selected: ResolvedDeployment[], + defaultProject: string): {errors: LocalCheckError[]}|{warning?: string} { + const {defaultProfile, docs, profileDocs, load} = scope; + if (defaultProfile === undefined || defaultProfile === DEFAULT_PROFILE || + !models.length) { return {}; } for (const {document} of models) { + const fails = (errors: string[], profile?: string) => + ({errors: errors.map(error => ({error, document, profile}))}); const doc = docs.find(d => d.name === document)!; const res = profileForDocument(doc, declaredBy(doc), defaultProfile, load); if (res.kind === 'missing') { - return { - error: unknownProfileMessage(defaultProfile, document, res.declared), - }; - } - } - return {}; + return fails( + [unknownProfileMessage(defaultProfile, document, res.declared)]); + } + if (res.kind === 'error') { + const error = profileErrorMessage( + document, res, defaultProfile, profileDocs(document)); + return fails([error], defaultProfile); + } + let applied = res.kind === 'file' ? res.model : undefined; + if (res.kind === 'wrapper') { + const merged = loadSemanticModels( + [{name: document, text: res.text, allowLegacyBareSource: true}], + {defaultProject, bindingOptional: true, ...load}); + if (merged.error) return fails([merged.error], defaultProfile); + applied = merged.models[0].model; + } + // Push checks the actions of the profile the runtime applies. + if (applied) { + const errors = validateActions( + applied, `${document}, profile '${defaultProfile}'`, false, true); + if (errors.length) return fails(errors, defaultProfile); + } + } + const used = models.some( + ({model}) => + (model.deployments ?? []).some(d => d.profile === defaultProfile)); + const unnamed = selected.filter(d => d.profile === DEFAULT_PROFILE) + .map(d => `'${d.name}'`); + const n = unnamed.length; + const ownBindings = n ? + ` ${plural(n, 'Deployment', 'Deployments')} ${unnamed.join(', ')} ` + + `${plural(n, 'uses', 'use')} the model file's own bindings.` : + ''; + if (used && !ownBindings) return {}; + return { + warning: `catalog.yaml sets default_profile '${defaultProfile}', which ` + + `chooses no binding on this push: Knowledge Catalog receives the ` + + `model file as written, and each deployment uses the profile it ` + + `names.${ownBindings}`, + }; } // Builds and checks the model `deployment` deploys from `base`, reading its @@ -2193,7 +2295,8 @@ function printProfileReport( // it applies and prunes each profile and runs push's local checks the way push // does, but deploys nothing, so a user can see coverage and whether push // accepts a profile before choosing one. Its one service call is push's Cloud -// SQL dialect lookup. Returns a process exit code (0 on success). +// SQL dialect lookup. When a model in the scope lists deployments, it gives the +// errors of a plain push instead. Returns a process exit code (0 on success). export async function profiles(options: ProfilesOptions = {}): Promise { if (options.printStore) return await printStore(options); @@ -2246,10 +2349,6 @@ export async function profiles(options: ProfilesOptions = {}): Promise { // Knowledge Catalog entry, looked up once, as push does. const load = await cloudSqlLoad(ctx, docs, profileDocs); if (!load) return 1; - const errorsFor = (doc: ProfiledDoc, name: string) => pushErrors( - doc, name, defaultProject, ctx, profileDocs, declaredBy, load, source, - docs.length === 1, overviews, - w => console.warn(` profile '${name}': warning: ${w}`)); // The inline bindings' header. 'default' is not a profile document and // never can be -- a push rejects a file by that name. It names the inline // bindings, which every model has. @@ -2275,21 +2374,51 @@ export async function profiles(options: ProfilesOptions = {}): Promise { for (const doc of docs) { if (!readDeclaredProfiles(declaredBy, doc)) return 1; } + // The errors of a plain push's checks, when a model lists deployments so + // that push runs in two phases. They replace the checks below. + let twoPhase: LocalCheckError[]|undefined; + if (docs.some(d => listsDeployments(d.text))) { + const checked = await checkBeforeWrites( + {}, {ctx, layout, source, docs, profileDocs, defaultProfile, load}, + true); + twoPhase = 'errors' in checked ? checked.errors : []; + // With KC_V2_ASPECTS on, push prints the emitter's warnings and checks + // every aspect's size once the local checks pass, before any write. + if (kcV2Aspects() && 'kcModels' in checked) { + const {error, warnings} = + catalogAspects(checked.kcModels, source, checked.overviews); + for (const w of warnings) console.warn(`Warning: ${w}`); + if (error) twoPhase.push({error, document: checked.kcModels[0].document}); + } + } + const twoPhaseErrors = (belongs: (e: LocalCheckError) => boolean) => + [...new Set((twoPhase ?? []).filter(belongs).map(e => e.error))]; + // The two-phase errors that belong to `document` and `profile`. With + // neither, those of the whole push. + const twoPhaseErrorsOf = (document?: string, profile?: string) => + twoPhaseErrors(e => e.document === document && e.profile === profile); + const errorsFor = async (doc: ProfiledDoc, name: string) => twoPhase ? + twoPhaseErrorsOf(doc.name, name) : + pushErrors( + doc, name, defaultProject, ctx, profileDocs, declaredBy, load, source, + docs.length === 1, overviews, + w => console.warn(` profile '${name}': warning: ${w}`)); + printRejects('', twoPhaseErrorsOf()); // Push publishes to Knowledge Catalog one model per entry group. - if (docs.length > 1) { + if (!twoPhase && docs.length > 1) { printRejects( '', [kc.oneModelPerEntryGroupMessage(source.entryGroup, docs.length)]); } // With KC_V2_ASPECTS off, push stores a copy of the model file and of each // profile file, and checks the copies before any graph deploys. Its lookup // of the type that holds them calls a service, so it runs only at push time. - if (!kcV2Aspects() && docs.length === 1) { + if (!twoPhase && !kcV2Aspects() && docs.length === 1) { const {error} = checkedFileCopies(layout, docs, profileDocs); if (error) printRejects('', [error]); } // With it on, push reads the overview sidecars before any graph deploys. let overviews: ReadonlyMap = new Map(); - if (kcV2Aspects() && docs.length === 1) { + if (!twoPhase && kcV2Aspects() && docs.length === 1) { try { overviews = layout.overviewSidecars(); } catch (err: any) { @@ -2300,17 +2429,24 @@ export async function profiles(options: ProfilesOptions = {}): Promise { for (const doc of docs) { console.log(`Model '${doc.name}' (${source.entryGroup}):`); const modelProfiles = declaredBy(doc); - const modelErrors: string[] = []; + // The errors printed above this model's profiles. The two-phase checks + // give those of the model and, in a report narrowed to one profile, those + // of the other profiles, since any of them stops the push. + const modelErrors = twoPhaseErrors( + e => e.document === doc.name && + (e.profile === undefined || + (only !== undefined && e.profile !== only))); // Push refuses every graph push while a model declares a profile named // 'default', whichever binding it deploys. - const reserved = reservedProfileNameError(doc.name, modelProfiles); + const reserved = + !twoPhase && reservedProfileNameError(doc.name, modelProfiles); if (reserved) modelErrors.push(reserved); // Push deploys no profile under that name, so it is neither merged nor // listed. const declared = modelProfiles.filter(p => p.name.toLowerCase() !== DEFAULT_PROFILE); // Push fails when default_profile names a profile this model lacks. - if (!only && defaultProfile !== undefined && + if (!twoPhase && !only && defaultProfile !== undefined && defaultProfile !== DEFAULT_PROFILE && !modelProfiles.some(p => p.name === defaultProfile)) { modelErrors.push(unknownProfileMessage( @@ -2358,16 +2494,21 @@ export async function profiles(options: ProfilesOptions = {}): Promise { if (res.kind === 'missing') continue; // Push applies the profile to every model in the scope, so a model that // does not declare it fails the push. + // A two-phase push applies it only for the deployments that use it. const errors = docs.filter(d => d.name !== doc.name).flatMap(d => { const names = declaredBy(d).map(p => p.name); - return names.includes(name) ? + return twoPhase || names.includes(name) ? [] : [unknownProfileMessage(name, d.name, names)]; }); let model: SemanticModel|undefined; + // Why the profile does not apply, when it does not. + let applyError: string|undefined; if (res.kind === 'error') { - errors.push(profileErrorMessage( - doc.name, res, name, profileDocs(doc.name))); + applyError = + profileErrorMessage(doc.name, res, name, profileDocs(doc.name)); + errors.push( + ...(twoPhase ? twoPhaseErrorsOf(doc.name, name) : [applyError])); } else { for (const w of res.warnings) { console.warn(` profile '${name}': warning: ${w}`); @@ -2396,13 +2537,23 @@ export async function profiles(options: ProfilesOptions = {}): Promise { if (!loaded.error) { model = applyProfileExclusions(loaded.models[0].model, res.excluded); + } else { + applyError = loaded.error; } } errors.push(...await errorsFor(profiledDoc(doc, res, load), name)); } const marker = name === defaultProfile ? ' (default)' : ''; console.log(` profile '${name}'${marker}`); - if (model) printProfileReport(model, name, res.kind === 'file'); + if (model) { + printProfileReport(model, name, res.kind === 'file'); + } else if (twoPhase && applyError !== undefined && !twoPhase.length) { + // Push accepts a profile it does not apply, such as one no deployment + // uses. When push gives no error, say why nothing is shown. + const [first, ...rest] = applyError.split('\n'); + console.log(` not shown: ${first}`); + for (const line of rest) console.log(` ${line}`); + } printRejects(' ', errors); } // `kcmd push --profile default`, and a push that names no profile when no diff --git a/toolbox/mdcode/src/tool/main.ts b/toolbox/mdcode/src/tool/main.ts index a62a269d..b8280dd0 100644 --- a/toolbox/mdcode/src/tool/main.ts +++ b/toolbox/mdcode/src/tool/main.ts @@ -65,10 +65,10 @@ cli.command('push', 'Push catalog entries') .option('--validate-only', 'Only validate changes without applying') .option( '--no-profile', - 'Deploy the graph for no binding profile: publish only the logical model to Knowledge Catalog, leaving any deployed graph untouched; the graph is deployed by default for the default binding profile; semantic-model push only') + 'Deploy the graph for no binding profile: publish only the logical model to Knowledge Catalog, leaving any deployed graph untouched; the graph is deployed by default for the default binding profile; not accepted for a model that lists deployments; semantic-model push only') .option( '--no-kc', - 'Skip the Knowledge Catalog metadata push and deploy only the graph; Knowledge Catalog is pushed by default; semantic-model push only') + 'Skip the Knowledge Catalog metadata push and deploy only the graph; Knowledge Catalog is pushed by default; not accepted for a model that lists deployments or has a sibling profile file; semantic-model push only') .option( '--print', 'Print each pushed destination\'s generated artifact in its native format (BigQuery/Spanner Graph SQL DDL, Knowledge Catalog entry plan); semantic-model push only') @@ -77,10 +77,10 @@ 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, 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') + '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, for a model that lists no deployments and has no sibling profile file; mutually exclusive with --all-profiles and --no-profile; not accepted for a model that lists deployments; 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') + '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; not accepted for a model that lists deployments; 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') diff --git a/toolbox/mdcode/tests/libts/mocks.ts b/toolbox/mdcode/tests/libts/mocks.ts index 913c1d85..8f461918 100644 --- a/toolbox/mdcode/tests/libts/mocks.ts +++ b/toolbox/mdcode/tests/libts/mocks.ts @@ -16,6 +16,17 @@ export function stubCloudSqlEntry(status: number, type?: string) { {status, message: 'Permission denied (or it may not exist).'}) as any); } +// Makes push find each graph's dataset or database, and pass each BigQuery dry +// run. +export function passGraphChecks(): void { + const ok = (result: object) => ({status: 200, result}) as any; + spyOn(bigquery.BigQueryClient.prototype, 'getDataset') + .mockResolvedValue(ok({location: 'US'})); + spyOn(bigquery.BigQueryClient.prototype, 'query').mockResolvedValue(ok({})); + spyOn(spanner.SpannerClient.prototype, 'getDatabase') + .mockResolvedValue(ok({})); +} + export class CatalogClientMock extends gcp.CatalogClient { public mockEntries: gcp.Entry[] = []; diff --git a/toolbox/mdcode/tests/libts/semantic/push_plan.test.ts b/toolbox/mdcode/tests/libts/semantic/push_plan.test.ts index 2819f55b..d8fd073d 100644 --- a/toolbox/mdcode/tests/libts/semantic/push_plan.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/push_plan.test.ts @@ -303,6 +303,9 @@ describe('push, legacy profiles, and pull fallback (b/567743508)', () => { const eg = path.join(dir, 'catalog', 'EntryGroups', 'sales_eg'); fs.mkdirSync(eg, {recursive: true}); spyOn(ApiContext, 'default').mockReturnValue(CTX); + spyOn(globalThis, 'fetch').mockImplementation((() => { + throw new Error('unexpected network call'); + }) as any); spyOn(console, 'log').mockImplementation(() => {}); return eg; } @@ -315,42 +318,6 @@ describe('push, legacy profiles, and pull fallback (b/567743508)', () => { mock.restore(); }); - // A legacy wrapper is merged into the model file's text and may set its own - // target, so a deployment-profile error is reported under each wrapper, and - // under the inline bindings. - test('kcmd profiles reports a deployment-profile error under each legacy wrapper', async () => { - const eg = setupWorkspace(); - const errSpy = spyOn(console, 'error').mockImplementation(() => {}); - fs.writeFileSync(path.join(eg, 'sales.yaml'), `version: "0.2.0.dev0/google" -semantic_model: - - name: sales - deployments: - - name: g - target: bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g - profile: prod - datasets: - - name: orders - source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders - primary_key: [id] - fields: - - { name: id, expression: id } -`); - fs.mkdirSync(path.join(eg, 'sales.profiles')); - for (const name of ['prod', 'qa']) { - fs.writeFileSync( - path.join(eg, 'sales.profiles', `${name}.yaml`), - `semantic_model:\n - name: sales\n datasets:\n - name: orders\n source: bigquery.googleapis.com/projects/p/datasets/d/tables/${name}_orders\n`); - } - - expect(await profiles()).toBe(1); - const errors = errSpy.mock.calls.map(c => c.join(' ')); - expect(errors.filter( - l => l === " [sales] model 'sales': deployment 'g' names " + - "profile 'prod'; remove 'profile' from the deployment " + - "entry so it uses the model file's own bindings.")) - .toHaveLength(3); - }); - test('a model file the loader rejects is reported, not thrown', async () => { const eg = setupWorkspace(); const errSpy = spyOn(console, 'error').mockImplementation(() => {}); @@ -393,7 +360,8 @@ semantic_model: "'deployment_target' is no longer supported"), }); - // `kcmd profiles` reports the bad file and still lists the good one. + // `kcmd profiles` reports the bad file as push does, and still lists the + // good one. errSpy.mockClear(); logSpy.mockClear(); expect(await profiles()).toBe(1); @@ -402,98 +370,20 @@ semantic_model: "'deployment_target' is no longer supported; migrate to the named " + "'deployments' list"); expect(errSpy.mock.calls.map(c => c.join(' ')).join('\n')) - .toContain( - "(under profile 'x', the model file must load on its own before " + - 'the profile applies)'); + .not.toContain('the model file must load on its own'); expect(logSpy.mock.calls.map(c => c.join(' ')).join('\n')) .toContain("Model 'b'"); }); - test('a push with a deployment naming a profile fails before anything is written', async () => { - const eg = setupWorkspace(); - const errSpy = spyOn(console, 'error').mockImplementation(() => {}); - const logSpy = spyOn(console, 'log').mockImplementation(() => {}); - const bqSpy = spyOn(bqDeploy, 'deployBigQuery'); - const kcSpy = spyOn(kcDeploy, 'deployKnowledgeCatalog'); - - const modelYaml = `version: "0.2.0.dev0/google" -semantic_model: - - name: sales - deployments: - - name: staging - target: bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g - profile: spanner_binding - datasets: - - name: orders - source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders - primary_key: [id] - fields: - - { name: id, expression: id } -`; - fs.writeFileSync(path.join(eg, 'sales.yaml'), modelYaml); - fs.writeFileSync( - path.join(eg, 'sales.profile.spanner_binding.yaml'), - 'name: spanner_binding\nentities:\n - name: orders\n source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders\n'); - fs.writeFileSync( - path.join(eg, 'sales.profile.other.yaml'), - 'name: other\nentities:\n - name: orders\n source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders2\n'); - - const code = await push({}); - expect(code).toBe(1); - expect(bqSpy).not.toHaveBeenCalled(); - expect(kcSpy).not.toHaveBeenCalled(); - const errors = errSpy.mock.calls.map(c => c.join(' ')).join('\n'); - expect(errors).toContain("deployment 'staging' names profile 'spanner_binding'"); - expect(errors).toContain("remove 'profile' from the deployment entry so it uses the model file's own bindings"); - expect(errors).not.toContain('--deployment'); - - // The runtime takes the store of the deployment that uses the profile, - // so the default profile has none. - const rt = await createSemanticRuntimes({ctx: CTX}); - if ('error' in rt) throw new Error(rt.error); - expect(rt[0].store).toBeUndefined(); - expect(rt[0].storeError).toContain( - "declares no deployment without a 'profile' in 'deployments'"); - - // The runtime applies the profile file to the model file, and the applied - // model keeps the model file's deployments, so it has the store. - const rtProfile = - await createSemanticRuntimes({ctx: CTX, profile: 'spanner_binding'}); - if ('error' in rtProfile) throw new Error(rtProfile.error); - expect(rtProfile[0].store?.name).toBe('projects/p/datasets/d'); - - // And `kcmd profiles` reports what push reports, and exits non-zero. A - // push of the inline bindings gives the deployment's error. A push of - // either profile file refuses it, since the model file names a graph. - errSpy.mockClear(); - logSpy.mockClear(); - expect(await profiles()).toBe(1); - const profErrors = errSpy.mock.calls.map(c => c.join(' ')).join('\n'); - expect(profErrors).toContain( - " [sales] model 'sales': deployment 'staging' names profile 'spanner_binding'"); - expect(profErrors.split('names profile').length - 1).toBe(1); - for (const name of ['other', 'spanner_binding']) { - expect(profErrors).toContain( - `[sales] binding profile '${name}' is a named profile, which names ` + - 'no deployment target'); - } - }); - // The error names the model and the profile on each side. test('push --all-profiles refuses two profiles that deploy one graph', async () => { const eg = setupWorkspace(); - spyOn(globalThis, 'fetch').mockImplementation((() => { - throw new Error('unexpected network call'); - }) as any); const errSpy = spyOn(console, 'error').mockImplementation(() => {}); spyOn(console, 'warn').mockImplementation(() => {}); const bqSpy = spyOn(bqDeploy, 'deployBigQuery'); fs.writeFileSync(path.join(eg, 'sales.yaml'), `version: "0.2.0.dev0/google" semantic_model: - name: sales - deployments: - - name: prod - target: bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g datasets: - name: orders source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders @@ -509,39 +399,44 @@ semantic_model: - name: orders source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders_staging `); + fs.copyFileSync(path.join(eg, 'sales.profiles', 'staging.yaml'), + path.join(eg, 'sales.profiles', 'qa.yaml')); expect(await push({allProfiles: true, kc: false, validateOnly: true})) .toBe(1); expect(bqSpy).not.toHaveBeenCalled(); expect(errSpy.mock.calls.map(c => c.join(' ')).join('\n')) .toContain( - "model 'sales' under binding profile 'staging' and model 'sales' " + - "under binding profile 'default' both deploy to the same graph " + + "model 'sales' under binding profile 'qa' and model 'sales' " + + "under binding profile 'staging' both deploy to the same graph " + "'bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g'"); }); - // Both models are under one profile, so only their names tell them apart. - test('push refuses two model files that deploy one graph', async () => { - const eg = setupWorkspace(); - spyOn(globalThis, 'fetch').mockImplementation((() => { - throw new Error('unexpected network call'); - }) as any); - const errSpy = spyOn(console, 'error').mockImplementation(() => {}); - spyOn(console, 'warn').mockImplementation(() => {}); - const bqSpy = spyOn(bqDeploy, 'deployBigQuery'); - const modelFile = (name: string) => `version: "0.2.0.dev0/google" + // A vanilla model file for model `name`, whose GOOGLE block lists `target`. + const modelFile = ( + name: string, + target = 'bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g') => + `version: "0.2.0.dev0" semantic_model: - name: ${name} - deployments: - - name: prod - target: bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g + custom_extensions: + - vendor_name: GOOGLE + data: '{"deploymentTargets": ["${target}"]}' datasets: - name: orders source: bigquery.googleapis.com/projects/p/datasets/d/tables/orders primary_key: [id] fields: - - { name: id, expression: id } + - name: id + expression: {dialects: [{dialect: ANSI_SQL, expression: id}]} `; + + // Both models are under one profile, so only their names tell them apart. + test('push refuses two model files that deploy one graph', async () => { + const eg = setupWorkspace(); + const errSpy = spyOn(console, 'error').mockImplementation(() => {}); + spyOn(console, 'warn').mockImplementation(() => {}); + const bqSpy = spyOn(bqDeploy, 'deployBigQuery'); fs.writeFileSync(path.join(eg, 'a.yaml'), modelFile('a')); fs.writeFileSync(path.join(eg, 'b.yaml'), modelFile('b')); @@ -554,8 +449,7 @@ semantic_model: "'bigquery.googleapis.com/projects/p/datasets/d/propertyGraphs/g'"); }); - // A resource name and its URI name the same graph. - test('push refuses a deployment naming a profile when it sits in the second of two model files', async () => { + test('push refuses two model files that list deployments before it checks a deployment\'s profile', async () => { const eg = setupWorkspace(); const errSpy = spyOn(console, 'error').mockImplementation(() => {}); spyOn(console, 'log').mockImplementation(() => {}); @@ -596,7 +490,9 @@ semantic_model: expect(kcSpy).not.toHaveBeenCalled(); const errors = errSpy.mock.calls.map(c => c.join(' ')).join('\n'); expect(errors).toContain( - "model 'second_model': deployment 'staging' names profile 'spanner_binding'"); + "entry group 'sales_eg' would receive 2 models, but only one model " + + 'per entry group is supported'); + expect(errors).not.toContain('spanner_binding'); }); test('a legacy profile file carrying deployment_target and a bare source still merges and loads', async () => { diff --git a/toolbox/mdcode/tests/libts/semantic/validate.test.ts b/toolbox/mdcode/tests/libts/semantic/validate.test.ts index 99034f55..866b2569 100644 --- a/toolbox/mdcode/tests/libts/semantic/validate.test.ts +++ b/toolbox/mdcode/tests/libts/semantic/validate.test.ts @@ -100,8 +100,7 @@ describe('validatePushRequirements', () => { }); expect(validatePushRequirements([loaded(alloy)])[0]).toContain( "Declare a BigQuery or Spanner Graph target (under 'deployments' in " + - 'its GOOGLE custom_extensions block; push uses exactly one ' + - 'deployment for now)'); + 'its GOOGLE custom_extensions block), or run'); }); test( @@ -154,9 +153,8 @@ describe('validatePushRequirements', () => { expect(errs[0]).toContain(`target '${ALLOYDB_TARGET}' is an AlloyDB database`); expect(errs[0]).toContain('runs against rather than deploys to'); expect(errs[0]).toContain( - "Declare a BigQuery or Spanner Graph target (under 'deployments'; " + - "push uses exactly one deployment for now), or run 'kcmd push " + - "--no-profile' to publish to Knowledge Catalog only."); + "Declare a BigQuery or Spanner Graph target (under 'deployments'), " + + "or run 'kcmd push --no-profile' to publish to Knowledge Catalog only."); expect(errs[0]).not.toContain('deploymentTarget'); expect(errs[0]).not.toContain('is not a valid'); @@ -169,9 +167,8 @@ describe('validatePushRequirements', () => { expect(vanillaErrs[0]).toContain( "Declare a BigQuery or Spanner Graph target (list 'deployments' in " + "a GOOGLE custom_extensions block on the model, or set version: " + - "'0.2.0.dev0/google' and use 'deployments:'; push uses exactly one " + - "deployment for now), or run 'kcmd push --no-profile' to publish to " + - "Knowledge Catalog only."); + "'0.2.0.dev0/google' and use 'deployments:'), or run 'kcmd push " + + "--no-profile' to publish to Knowledge Catalog only."); expect(vanillaErrs[0]).not.toContain("under 'deployments'"); }); diff --git a/toolbox/mdcode/tests/tool/commands.test.ts b/toolbox/mdcode/tests/tool/commands.test.ts index d19be9e6..880b8a92 100644 --- a/toolbox/mdcode/tests/tool/commands.test.ts +++ b/toolbox/mdcode/tests/tool/commands.test.ts @@ -22,7 +22,8 @@ import * as pullKc from '../../src/libts/semantic/pull_kc'; import {createSemanticRuntimes} from '../../src/libts/semantic/runtime/runtime'; import * as sqlParser from '../../src/libts/semantic/sql_parser'; import * as validateMod from '../../src/libts/semantic/validate'; -import {kcV2Aspects, profiles, pull, push, PushOptions, skillsGenerate} from '../../src/tool/commands'; +import {kcV2Aspects, profiles, pull, push, skillsGenerate} from '../../src/tool/commands'; +import {passGraphChecks} from '../libts/mocks'; const CTX = new ApiContext('test-project', 'us', 'test-token'); @@ -58,6 +59,11 @@ function writeWorkspace(): void { fs.writeFileSync(path.join(eg, 'commerce.yaml'), MODEL_YAML); } +// A model file's text without its deployments list. Push takes --no-kc, +// --profile and --no-profile only for a model that lists no deployments. +const withoutDeployments = (model: string) => + model.replace(/ {4}deployments:\n.*\n.*\n/, ''); + beforeEach(() => { cwd = process.cwd(); dir = fs.mkdtempSync(path.join(os.tmpdir(), 'kcmd-commands-')); @@ -77,6 +83,7 @@ beforeEach(() => { .mockResolvedValue({status: 200, result: {}} as any); spyOn(CatalogClient.prototype, 'getAspectType') .mockResolvedValue({status: 200, result: {}} as any); + passGraphChecks(); }); afterEach(() => { @@ -218,89 +225,11 @@ describe('v2Aspects forwarding', () => { }); -describe('a push with a sibling profile file', () => { - test('a graph push of the profile file is refused before anything deploys', async () => { - fs.writeFileSync( - path.join(dir, 'catalog.yaml'), - 'scope: semantic-model.test-project.us.commerce_eg\n'); - const eg = path.join(dir, 'catalog', 'EntryGroups', 'commerce_eg'); - fs.mkdirSync(eg, {recursive: true}); - fs.writeFileSync(path.join(eg, 'commerce.yaml'), `version: "0.2.0.dev0/google" -semantic_model: - - name: commerce - deployments: - - name: prod - target: bigquery.googleapis.com/projects/test-project/datasets/d/propertyGraphs/g - entities: - - name: Customer - source: bigquery.googleapis.com/projects/test-project/datasets/d/tables/customer - primary_key: [key] - fields: - - { name: key, expression: c_key } -`); - fs.writeFileSync(path.join(eg, 'commerce.profile.prod.yaml'), PROD_PROFILE); - const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - - const code = await push({validateOnly: true, kc: false, profile: 'prod'}); - - expect(code).toBe(1); - expect(errorSpy.mock.calls.map(c => String(c[0])).join('\n')) - .toContain("binding profile 'prod' is a named profile, which names no deployment target"); - }); - - test('when default_profile names the profile file, the refusal points at --profile default', async () => { - fs.writeFileSync( - path.join(dir, 'catalog.yaml'), - 'scope: semantic-model.test-project.us.commerce_eg\ndefault_profile: prod\n'); - const eg = path.join(dir, 'catalog', 'EntryGroups', 'commerce_eg'); - fs.mkdirSync(eg, {recursive: true}); - fs.writeFileSync(path.join(eg, 'commerce.yaml'), `version: "0.2.0.dev0/google" -semantic_model: - - name: commerce - deployments: - - name: prod - target: bigquery.googleapis.com/projects/test-project/datasets/d/propertyGraphs/g - entities: - - name: Customer - source: bigquery.googleapis.com/projects/test-project/datasets/d/tables/customer - primary_key: [key] - fields: - - { name: key, expression: c_key } -`); - fs.writeFileSync(path.join(eg, 'commerce.profile.prod.yaml'), PROD_PROFILE); - const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - spyOn(console, 'log').mockImplementation(() => {}); - spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); - spyOn(validateMod, 'validateBigQueryActionStatements').mockResolvedValue([]); - const bqSpy = spyOn(bqLeg, 'deployBigQuery') - .mockResolvedValue({deployed: 0, ddl: [], warnings: []} as any); - - // Without --profile, push takes default_profile, the profile file. - expect(await push({validateOnly: true, kc: false})).toBe(1); - const refusal = errorSpy.mock.calls.map(c => String(c[0])).join('\n'); - expect(refusal).toContain( - "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 own " + - 'bindings to be complete.'); - expect(bqSpy).not.toHaveBeenCalled(); - - // The command it names deploys the model file's own bindings. - errorSpy.mockClear(); - await push({validateOnly: true, kc: false, profile: 'default'}); - expect(errorSpy.mock.calls.map(c => String(c[0])).join('\n')) - .not.toContain('is a named profile'); - expect(bqSpy).toHaveBeenCalledTimes(1); - expect(bqSpy.mock.calls[0][0][0].model.entities?.[0].dataSource) - .toBe('test-project.d.customer'); - }); -}); - - describe('push preflights under a legacy wrapper', () => { // Each live preflight gets documents labeled with the profile, so its errors - // name the profile that may have set the value they are about. - test('every preflight names the profile', async () => { + // name the profile that may have set the value they are about. Field ltv is + // unbound, so push prints a pruning note before a source check that fails. + test('every preflight names the profile, and a push whose source check fails still prints the pruning note', async () => { fs.writeFileSync( path.join(dir, 'catalog.yaml'), 'scope: semantic-model.test-project.us.commerce_eg\n'); @@ -315,6 +244,8 @@ semantic_model: primary_key: [key] fields: - { name: key, expression: c_key } + - { name: ltv } + - { name: odd, expression: "odd +* (" } `); const wrapper = (target: string, source: string) => `semantic_model: - name: commerce @@ -349,6 +280,17 @@ semantic_model: bqDml: ["commerce, profile 'bq'"], spDml: ["commerce, profile 'sp'"], }); + spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue(['gone']); + const warn = spyOn(console, 'warn').mockClear(); + const error = spyOn(console, 'error').mockClear(); + expect(await push({validateOnly: true, kc: false, profile: 'bq'})).toBe(1); + expect(messages(error)).toBe('Error: gone'); + expect(messages(warn).split('\n')).toEqual([ + "Warning: field 'Customer.odd' in model 'commerce' (commerce, profile " + + "'bq'): kcmd could not read its SQL, so the fields it reads were " + + 'not checked.', + "Note: profile 'bq' leaves 1 field(s) unbound.", + ]); }); }); @@ -358,7 +300,7 @@ describe('a catalog-only push with a sibling profile file', () => { success: true, created: 0, updated: 0, deleted: 0, linked: 0, unlinked: 0, warnings: [], plan: [], details: '', } as any; - const setup = (deployment: string, entities: string) => { + const setup = (entities: string) => { fs.writeFileSync( path.join(dir, 'catalog.yaml'), 'scope: semantic-model.test-project.us.commerce_eg\ndefault_profile: prod\n'); @@ -367,7 +309,7 @@ describe('a catalog-only push with a sibling profile file', () => { fs.writeFileSync(path.join(eg, 'commerce.yaml'), `version: "0.2.0.dev0/google" semantic_model: - name: commerce -${deployment} entities: + entities: ${entities}`); fs.writeFileSync(path.join(eg, 'commerce.profile.prod.yaml'), PROD_PROFILE); }; @@ -378,28 +320,11 @@ ${entities}`); - { name: key, expression: c_key } `; - // `--no-profile` publishes the default binding, here the profile file, and - // the model it gives keeps the model file's deployments. - test('the deployment-profile check covers the model a profile file gives', async () => { - setup(` deployments: - - name: g - target: bigquery.googleapis.com/projects/test-project/datasets/d/propertyGraphs/g - profile: prod -`, CUSTOMER); - const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - const deploySpy = spyOn(kc, 'deployKnowledgeCatalog').mockResolvedValue(KC_OK); - - expect(await push({validateOnly: true, profile: false})).toBe(1); - expect(deploySpy).not.toHaveBeenCalled(); - expect(errorSpy.mock.calls.map(c => String(c[0])).join('\n')) - .toContain("deployment 'g' names profile 'prod'"); - }); - // A named profile binds every concrete entity, whether or not a push // deploys it, so an entity the profile file leaves without a source stops a // catalog-only push too. test('an entity left without a source stops a catalog-only push', async () => { - setup('', CUSTOMER + ` - name: Orders + setup(CUSTOMER + ` - name: Orders primary_key: [id] fields: - { name: id, expression: o_id } @@ -719,9 +644,6 @@ describe('a model with a Spanner graph target is pruned in the Spanner dialect', fs.writeFileSync(path.join(eg, 'commerce.yaml'), `version: "0.2.0.dev0/google" semantic_model: - name: commerce - deployments: - - { name: sp, target: "${spGraph}" } - - { name: bq, target: "${bqGraph}", profile: bqprof } entities: - name: orders source: ${table} @@ -741,9 +663,6 @@ semantic_model: - { dialect: SPANNER, expression: "orders.gross" } - { dialect: ANSI_SQL, expression: "orders.gross - orders.tax" } `); - // Push refuses a deployment that names a profile. spprof's - // deployment_target replaces the model file's deployments, so a push of - // spprof never reads the one that names bqprof. fs.writeFileSync(path.join(eg, 'commerce.profiles', 'spprof.yaml'), `semantic_model: - name: commerce deployment_target: ${spGraph} @@ -753,6 +672,7 @@ semantic_model: `); fs.writeFileSync(path.join(eg, 'commerce.profiles', 'bqprof.yaml'), `semantic_model: - name: commerce + deployment_target: ${bqGraph} entities: - name: orders source: ${table} @@ -796,9 +716,6 @@ describe('a pruned push checks the sources a profile file leaves out', () => { fs.writeFileSync(path.join(eg, 'a.yaml'), `version: "0.2.0.dev0/google" semantic_model: - name: a - deployments: - - name: g - target: spanner.googleapis.com/${SPANNER}/graphs/g entities: - name: X source: spanner.googleapis.com/${SPANNER}/tables/X @@ -808,6 +725,7 @@ semantic_model: `); fs.writeFileSync(path.join(eg, 'a.profiles', 'prod.yaml'), `semantic_model: - name: a + deployment_target: spanner.googleapis.com/${SPANNER}/graphs/g entities: - name: X source: spanner.googleapis.com/${SPANNER}/tables/X2 @@ -880,7 +798,7 @@ semantic_model: fs.writeFileSync(path.join(eg, 'commerce.yaml'), GRAPH_MODEL); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - const code = await push({validateOnly: true, kc: false}); + const code = await push({validateOnly: true}); expect(code).toBe(1); expect(errorSpy.mock.calls.map(c => String(c[0])).join('\n')) @@ -893,10 +811,12 @@ semantic_model: 'scope: semantic-model.test-project.us.commerce_eg\n'); const eg = path.join(dir, 'catalog', 'EntryGroups', 'commerce_eg'); fs.mkdirSync(path.join(eg, 'commerce.profiles'), {recursive: true}); - fs.writeFileSync(path.join(eg, 'commerce.yaml'), GRAPH_MODEL); + fs.writeFileSync(path.join(eg, 'commerce.yaml'), + withoutDeployments(GRAPH_MODEL)); fs.writeFileSync( path.join(eg, 'commerce.profiles', 'legacy.yaml'), `semantic_model: - name: commerce + deployment_target: bigquery.googleapis.com/projects/test-project/datasets/d/propertyGraphs/g entities: - name: customer source: bigquery.googleapis.com/projects/test-project/datasets/d2/tables/customer @@ -940,7 +860,7 @@ semantic_model: `); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - const code = await push({validateOnly: true, kc: false}); + const code = await push({validateOnly: true}); expect(code).toBe(1); expect(errorSpy.mock.calls.map(c => String(c[0])).join('\n')) @@ -991,7 +911,7 @@ semantic_model: return origPushReqs(...args); }); - expect(await push({validateOnly: true, kc: false})).toBe(0); + expect(await push({validateOnly: true})).toBe(0); expect(warnSpy.mock.calls.map(c => String(c[0])).filter(m => m.includes('could not read its SQL'))) .toEqual([ "Warning: metric 'broken_metric' in model 'commerce' (commerce): kcmd " + @@ -1003,6 +923,9 @@ semantic_model: 'loadSqlEngine', 'loadSemanticModels', 'validateExpressionReferences', + // The catalog's model, then the deployment's. + 'validatePushRequirements', + 'validateExpressionReferences', 'validateExpressionReferences', 'validatePushRequirements', 'validateExpressionReferences', @@ -1125,6 +1048,15 @@ function storedCopies(entries: Map): Record { return out; } +// Mocks the BigQuery leg's live checks and deploy, and returns the deploy's +// mock. +function mockBigQuery() { + spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); + spyOn(validateMod, 'validateBigQueryActionStatements').mockResolvedValue([]); + return spyOn(bqLeg, 'deployBigQuery').mockResolvedValue( + {success: true, deployed: 1, ddl: [], warnings: []} as any); +} + function messages(spy: any): string { return spy.mock.calls.map((c: any[]) => c.map(String).join(' ')).join('\n'); } @@ -1134,12 +1066,7 @@ describe('the file copy round trip', () => { async () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); - spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); - spyOn(validateMod, 'validateBigQueryActionStatements') - .mockResolvedValue([]); - spyOn(bqLeg, 'deployBigQuery') - .mockResolvedValue( - {success: true, deployed: 1, ddl: [], warnings: []} as any); + mockBigQuery(); expect(await push({})).toBe(0); expect(Object.keys(storedCopies(entries)).sort()).toEqual([ @@ -1163,11 +1090,13 @@ describe('the file copy round trip', () => { test('a push with --no-profile still stores the profile copies', async () => { writeRoundTripWorkspace(); + const model = withoutDeployments(ROUND_TRIP_MODEL); + fs.writeFileSync(path.join(entryGroupDir(), 'commerce.yaml'), model); const {entries} = fakeCatalog(); expect(await push({profile: false})).toBe(0); expect(storedCopies(entries)).toEqual({ - 'commerce.yaml': ROUND_TRIP_MODEL, + 'commerce.yaml': model, 'commerce.profile.prod.yaml': ROUND_TRIP_PROFILE, }); }); @@ -1200,7 +1129,7 @@ describe('the file copy round trip', () => { for (const validateOnly of [true, false]) { const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false, validateOnly})).toBe(1); + expect(await push({deployment: false, validateOnly})).toBe(1); expect(messages(errorSpy)).toContain('bogus'); expect(messages(errorSpy)).not.toContain('not found in project'); } @@ -1216,7 +1145,7 @@ describe('the file copy round trip', () => { const {create, list} = fakeCatalog(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain( 'file \'commerce.profile.prod.yaml\' is too large to store ' + @@ -1244,7 +1173,7 @@ semantic_model: fs.writeFileSync(legacyPath, legacy); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(Object.keys(storedCopies(entries)).sort()) .toEqual(['commerce.profiles/prod.yaml', 'commerce.yaml']); @@ -1257,13 +1186,13 @@ semantic_model: async () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); const changed = ROUND_TRIP_MODEL.replace( 'Orders and customers', 'Orders and their customers'); fs.writeFileSync(path.join(entryGroupDir(), 'commerce.yaml'), changed); fs.rmSync(path.join(entryGroupDir(), 'commerce.profile.prod.yaml')); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(storedCopies(entries)).toEqual({'commerce.yaml': changed}); }); @@ -1280,7 +1209,7 @@ semantic_model: fs.writeFileSync(shop, ROUND_TRIP_MODEL); const {entries} = fakeCatalog(); const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(Object.keys(storedCopies(entries))).toEqual(['shop.yaml']); fs.rmSync(shop); @@ -1295,7 +1224,8 @@ semantic_model: // Push still reads each profile file, to check it. Both push paths write // each overview sidecar into its entry's overview aspect, and a malformed - // sidecar stops them before they read the catalog. + // sidecar stops them before they read the catalog. Today's path takes the + // model with no deployments list. test('with KC_V2_ASPECTS on, push stores no copy and writes the overview ' + 'sidecars, with --no-deployment too; a malformed sidecar or ' + 'profile file fails it', @@ -1310,8 +1240,13 @@ semantic_model: write('commerce.overview.md', '---\ncontentType: MARKDOWN\n---\n# Commerce\n'); const {entries, create, list} = fakeCatalog(); - const pushes = [{profile: false}, {deployment: false}]; - for (const options of pushes) { + const pushes = [ + {options: {profile: false}, + model: withoutDeployments(ROUND_TRIP_MODEL)}, + {options: {deployment: false}, model: ROUND_TRIP_MODEL}, + ]; + for (const {options, model} of pushes) { + write('commerce.yaml', model); expect(await push(options)).toBe(0); expect(storedCopies(entries)).toEqual({}); expect(entries @@ -1324,7 +1259,8 @@ semantic_model: write('commerce/metrics/revenue.overview.md', '---\ncontentType: TEXT\n---\nNet.\n'); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - for (const options of pushes) { + for (const {options, model} of pushes) { + write('commerce.yaml', model); list.mockClear(); create.mockClear(); errorSpy.mockClear(); @@ -1339,6 +1275,10 @@ semantic_model: writeRoundTripWorkspace('name: [unterminated\n'); errorSpy.mockClear(); + expect(await push({deployment: false})).toBe(1); + expect(messages(errorSpy)).toContain('commerce.profile.prod.yaml'); + write('commerce.yaml', withoutDeployments(ROUND_TRIP_MODEL)); + errorSpy.mockClear(); expect(await push({profile: false})).toBe(1); expect(messages(errorSpy)).toContain('commerce.profile.prod.yaml'); }); @@ -1802,12 +1742,7 @@ describe('the file-copy type is checked before anything is written', () => { test('a graph push into a project without it deploys no graph', async () => { writeRoundTripWorkspace(); const {create} = fakeCatalog({missingType: 'semantic-model-file'}); - spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); - spyOn(validateMod, 'validateBigQueryActionStatements').mockResolvedValue([]); - const deployed = - spyOn(bqLeg, 'deployBigQuery') - .mockResolvedValue( - {success: true, deployed: 1, ddl: [], warnings: []} as any); + const deployed = mockBigQuery(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); expect(await push({})).toBe(1); @@ -1815,7 +1750,8 @@ describe('the file-copy type is checked before anything is written', () => { .toContain( 'Error: Custom type \'semantic-model-file\' not found in project ' + '\'test-project\'; run \'kcmd init --semantic-model\' first.'); - expect(deployed).not.toHaveBeenCalled(); + // Push generates the graph's DDL first, which deploys nothing. + expect(deployed.mock.calls.map(c => c[2]?.validateOnly)).toEqual([true]); expect(create).not.toHaveBeenCalled(); }); @@ -1824,7 +1760,7 @@ describe('the file-copy type is checked before anything is written', () => { fakeCatalog({missingType: 'semantic-model-file'}); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({validateOnly: true, profile: false})).toBe(1); + expect(await push({validateOnly: true, deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain( 'Custom type \'semantic-model-file\' not found in project ' + @@ -1840,13 +1776,13 @@ describe('the file-copy type is checked before anything is written', () => { spy => spy.mock.calls.filter(c => c[2] === 'semantic-model-file') .length); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(Object.keys(storedCopies(entries))).toHaveLength(2); expect(lookups()).toEqual([1, 1]); entryType.mockClear(); aspectType.mockClear(); - expect(await push({validateOnly: true, profile: false})).toBe(0); + expect(await push({validateOnly: true, deployment: false})).toBe(0); expect(lookups()).toEqual([1, 1]); }); @@ -1863,19 +1799,6 @@ describe('the file-copy type is checked before anything is written', () => { describe('push checks each file before anything is stored', () => { - test('a --no-profile push applies the profile file rules', async () => { - writeRoundTripWorkspace('name: staging\n'); - const {create} = fakeCatalog(); - const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - - expect(await push({profile: false})).toBe(1); - expect(messages(errorSpy)) - .toContain( - 'Error: Profile file \'commerce.profile.prod.yaml\' declares name ' + - '\'staging\', which does not match filename suffix \'prod\'.'); - expect(create).not.toHaveBeenCalled(); - }); - test('a profile file that is not valid UTF-8 fails the push, naming it', async () => { writeRoundTripWorkspace(); @@ -1908,7 +1831,7 @@ describe('push checks each file before anything is stored', () => { fakeCatalog(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain('Error: File \'commerce.yaml\' is not valid UTF-8'); }); @@ -1918,7 +1841,7 @@ describe('push checks each file before anything is stored', () => { writeRoundTripWorkspace(profile); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(storedCopies(entries)['commerce.profile.prod.yaml']).toBe(profile); }); @@ -1949,13 +1872,7 @@ describe('push checks each file before anything is stored', () => { writeRoundTripWorkspace(); setup(); const {create} = fakeCatalog(); - spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); - spyOn(validateMod, 'validateBigQueryActionStatements') - .mockResolvedValue([]); - const deployed = - spyOn(bqLeg, 'deployBigQuery') - .mockResolvedValue( - {success: true, deployed: 1, ddl: [], warnings: []} as any); + const deployed = mockBigQuery(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); expect(await push({})).toBe(1); @@ -1972,7 +1889,7 @@ describe('push checks each file before anything is stored', () => { fakeCatalog(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({validateOnly: true, profile: false})).toBe(1); + expect(await push({validateOnly: true, deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain( 'file \'commerce.profile.prod.yaml\' is too large to store ' + @@ -1990,7 +1907,7 @@ describe('push checks each file before anything is stored', () => { const {create} = fakeCatalog(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain( 'Error: files \'commerce.profiles/a b.yaml\' and ' + @@ -2013,7 +1930,7 @@ describe('push checks each file before anything is stored', () => { fakeCatalog(); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); const errors = messages(errorSpy); expect(errors).toContain( 'entry group \'commerce_eg\' would receive 2 models'); @@ -2042,7 +1959,7 @@ describe('a model file not named after its model', () => { const {entries} = fakeCatalog(); const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(warnSpy.mock.calls.map(c => String(c[0])) .filter(w => w.includes('is not named'))) .toEqual([NOTE]); @@ -2055,12 +1972,12 @@ describe('a model file not named after its model', () => { writeRoundTripWorkspace(); fakeCatalog(); const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); fs.rmSync(entryGroupDir(), {recursive: true}); writeShop(); process.env.KC_V2_ASPECTS = '1'; - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); expect(messages(warnSpy)).not.toContain('is not named'); }); }); @@ -2119,7 +2036,7 @@ describe('the push id on every file copy', () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); failCopyWrites(entries); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(storedCopies(entries)).toEqual({}); // A teammate's workspace, with another model file that @@ -2143,13 +2060,13 @@ describe('the push id on every file copy', () => { fs.writeFileSync( shop, ROUND_TRIP_MODEL.replace('name: commerce', 'name: shop')); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); fs.rmSync(shop); fs.writeFileSync( path.join(entryGroupDir(), 'commerce.yaml'), ROUND_TRIP_MODEL); failCopyWrites(entries); - expect(await push({profile: false, forceRemove: true})).toBe(1); + expect(await push({deployment: false, forceRemove: true})).toBe(1); expect(storedCopies(entries)).toEqual({}); const before = localFiles(); @@ -2166,7 +2083,7 @@ describe('the push id on every file copy', () => { writeRoundTripWorkspace(); fs.rmSync(path.join(entryGroupDir(), 'commerce.profile.prod.yaml')); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); fs.writeFileSync( path.join(entryGroupDir(), 'commerce.yaml'), @@ -2175,7 +2092,7 @@ describe('the push id on every file copy', () => { path.join(entryGroupDir(), 'commerce.profile.prod.yaml'), ROUND_TRIP_PROFILE); failCopyWrites(entries); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(Object.keys(storedCopies(entries))).toEqual(['commerce.yaml']); const before = localFiles(); @@ -2200,7 +2117,7 @@ describe('the push id on every file copy', () => { fs.writeFileSync( shop, ROUND_TRIP_MODEL.replace('name: commerce', 'name: shop')); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); fs.rmSync(shop); fs.writeFileSync( @@ -2215,7 +2132,7 @@ describe('the push id on every file copy', () => { } return {status: 200}; }); - expect(await push({profile: false, forceRemove: true})).toBe(1); + expect(await push({deployment: false, forceRemove: true})).toBe(1); expect(entries.size).toBe(0); const before = localFiles(); @@ -2234,7 +2151,7 @@ describe('the push id on every file copy', () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); failCopyWrites(entries, 'commerce.profile.prod.yaml'); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(Object.keys(storedCopies(entries))).toEqual(['commerce.yaml']); const before = localFiles(); @@ -2248,13 +2165,13 @@ describe('the push id on every file copy', () => { writeRoundTripWorkspace(); fs.rmSync(path.join(entryGroupDir(), 'commerce.profile.prod.yaml')); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); fs.writeFileSync( path.join(entryGroupDir(), 'commerce.profile.prod.yaml'), ROUND_TRIP_PROFILE); failCopyWrites(entries, 'commerce.profile.prod.yaml'); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); const before = localFiles(); await expect(pull({forceRemove: true})).rejects.toThrow(PARTIAL); @@ -2267,7 +2184,7 @@ describe('the push id on every file copy', () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); const first = pushIds(entries); expect(Object.keys(first).sort()).toEqual([ 'commerce.profile.prod.yaml', @@ -2283,7 +2200,7 @@ describe('the push id on every file copy', () => { .toBe(2); } - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); const second = pushIds(entries); expect(second['commerce.profile.prod.yaml']) .toBe(second['commerce.yaml']); @@ -2295,7 +2212,7 @@ describe('the push id on every file copy', () => { async () => { writeRoundTripWorkspace(); const {entries} = fakeCatalog(); - expect(await push({profile: false})).toBe(0); + expect(await push({deployment: false})).toBe(0); // The next push edits both files, and the profile copy's write fails. fs.writeFileSync( @@ -2314,7 +2231,7 @@ describe('the push id on every file copy', () => { return {status: 200, result: entry}; }); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); - expect(await push({profile: false})).toBe(1); + expect(await push({deployment: false})).toBe(1); expect(messages(errorSpy)) .toContain( 'The catalog may now lack some or all of this push\'s file ' + @@ -2523,8 +2440,8 @@ describe('push applies, loads and checks a profile', () => { } // A model whose field 'customer.ltv' has no expression, with a BigQuery - // deployment for each name in `deployments`, and then the `rest` lines. - const ltvUnbound = (deployments: string[], rest = '') => + // deployment for each name in `deployments`. + const ltvUnbound = (deployments: string[]) => `version: "0.2.0.dev0/google" semantic_model: - name: commerce @@ -2537,74 +2454,54 @@ ${deployments.map(d => ` - { name: ${d}, target: "${BQ}/propertyGraphs/${d} fields: - { name: c_id, expression: c_id } - { name: ltv } -${rest}`; - - // Push prints the pruning note once the checks before the deploy loop - // pass, after their warnings. A push that then fails a source check still - // prints it. - const NOTE = "Note: profile 'default' leaves 1 field(s) unbound."; - const validateNoKc = {validateOnly: true, kc: false}; - const pruned: Array<[ - outcome: string, deployments: string[], rest: string, options: PushOptions, - errors: string[], warnings: string[], sources?: string[], - v2Aspects?: boolean]> = [ - ['stopped by two deployments', ['a', 'b'], '', {validateOnly: true}, - ["Error: model 'commerce' (commerce) declares 2 deployments; push uses " + - 'exactly one deployment for now, with a BigQuery Graph or Spanner ' + - "Graph target under 'deployments'."], []], - // Under --all-profiles a legacy profile that passes comes first. The - // default binding then fails the rule on the fields a metric reads. - ['stopped by a later binding under --all-profiles', ['g'], - ' metrics:\n' + - ' - { name: total_ltv, expression: "SUM(customer.ltv)" }\n', - {...validateNoKc, allProfiles: true}, - ["Error: model 'commerce' (commerce) deployment 'g' uses the model " + - "file's own bindings, under which metric 'total_ltv' reads field " + - "'customer.ltv', which has no expression; a deployment needs every " + - 'field its metrics read bound.'], []], - ['that passes', ['a'], '', validateNoKc, [], [NOTE]], - ['that warns', ['g'], ' - { name: odd, expression: "odd +* (" }\n', - validateNoKc, [], - ["Warning: field 'customer.odd' in model 'commerce' (commerce): kcmd " + - 'could not read its SQL, so the fields it reads were not checked.', NOTE]], - ['that fails a source check', ['g'], '', validateNoKc, - ['Error: the table is gone'], [NOTE], ['the table is gone']], - ['that passes with KC_V2_ASPECTS on', ['a'], '', {validateOnly: true}, [], - [NOTE], [], true], - ['stopped by an aspect too large with KC_V2_ASPECTS on', ['a'], - ` ai_context:\n instructions: ${ - 'x'.repeat(kc.MAX_ASPECT_DATA_BYTES)}\n`, - {validateOnly: true}, - [expect.stringContaining("Error: aspect 'dataplex-types.global." + - "guidelines' of entry 'commerce' is too large")], - [], [], true], - ]; - for (const [outcome, deployments, rest, options, errors, warnings, - sources = [], v2Aspects = false] of pruned) { - const note = warnings.includes(NOTE) ? 'its' : 'no'; - test(`a pruned push ${outcome} gives ${note} pruning note`, async () => { - if (v2Aspects) process.env.KC_V2_ASPECTS = '1'; - const eg = writeModel(ltvUnbound(deployments, rest)); - if (options.allProfiles) { - fs.mkdirSync(path.join(eg, 'commerce.profiles')); - fs.writeFileSync( - path.join(eg, 'commerce.profiles', 'staging.yaml'), - `semantic_model: +`; + + test('a pruned push that fails a source check gives its pruning note', async () => { + writeModel(ltvUnbound(['g'])); + spyOn(validateMod, 'validateBigQueryDataSources') + .mockResolvedValue(['the table is gone']); + const bqSpy = spyOn(bqLeg, 'deployBigQuery').mockResolvedValue( + {success: true, deployed: 0, ddl: [], warnings: []} as any); + expect(await push({validateOnly: true})).toBe(1); + expect(prints(console.error)).toEqual(['Error: the table is gone']); + expect(prints(console.warn)) + .toEqual(["Note: profile 'default' leaves 1 field(s) unbound."]); + expect(bqSpy).toHaveBeenCalledTimes(1); + }); + + // With KC_V2_ASPECTS on, today's path checks every aspect's size before any + // graph deploys. A legacy wrapper's deployment_target keeps a graph push + // there. + test("a graph push on today's path with KC_V2_ASPECTS on stops at an " + + 'aspect too large before the graph deploys', + async () => { + process.env.KC_V2_ASPECTS = '1'; + const model = ltvUnbound(['g']).replace(/ {4}deployments:\n.*\n/, ''); + const eg = writeModel(model); + fs.mkdirSync(path.join(eg, 'commerce.profiles')); + fs.writeFileSync( + path.join(eg, 'commerce.profiles', 'staging.yaml'), + `semantic_model: - name: commerce deployment_target: ${BQ}/propertyGraphs/staging entities: [{ name: customer, source: "${BQ}/tables/customer_staging" }] `); - } - spyOn(validateMod, 'validateBigQueryDataSources') - .mockResolvedValue(sources); - const bqSpy = spyOn(bqLeg, 'deployBigQuery').mockResolvedValue( - {success: true, deployed: 0, ddl: [], warnings: []} as any); - expect(await push(options)).toBe(errors.length ? 1 : 0); - expect(prints(console.error)).toEqual(errors); - expect(prints(console.warn)).toEqual(warnings); - expect(bqSpy).toHaveBeenCalledTimes(errors.length ? 0 : 1); - }); - } + spyOn(validateMod, 'validateBigQueryDataSources') + .mockResolvedValue([]); + const bqSpy = spyOn(bqLeg, 'deployBigQuery').mockResolvedValue( + {success: true, deployed: 0, ddl: [], warnings: []} as any); + const options = {validateOnly: true, profile: 'staging'}; + expect(await push(options)).toBe(0); + writeModel(`${model} ai_context:\n instructions: ${ + 'x'.repeat(kc.MAX_ASPECT_DATA_BYTES)}\n`); + expect(await push(options)).toBe(1); + expect(prints(console.error)).toEqual([expect.stringContaining( + "Error: aspect 'dataplex-types.global.guidelines' of entry " + + "'commerce' is too large")]); + expect(prints(console.warn)) + .toEqual(["Note: profile 'staging' leaves 1 field(s) unbound."]); + expect(bqSpy).toHaveBeenCalledTimes(1); + }); // Unlike a field with no expression, a field with a blank one stops the push // when its model file loads. @@ -2700,7 +2597,7 @@ semantic_model: dialects: - { dialect: SNOWFLAKE, expression: "SUM(IFF(purchase.amount > 0, purchase.amount, 0))" } `); - const kcSpy = mockKc(); + const kcSpy = spyOn(kc, 'preflightKnowledgeCatalog'); spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); spyOn(validateMod, 'validateBigQueryActionStatements').mockResolvedValue([]); spyOn(bqLeg, 'deployBigQuery').mockResolvedValue( @@ -2734,7 +2631,6 @@ semantic_model: expect(metric().stringForm).toBe(false); }); - // Two deployments stop the push after pruning, before any live check. test('a pruned push warns about a metric whose SQL it cannot read', async () => { writeModel(`version: "0.2.0.dev0/google" semantic_model: @@ -2751,7 +2647,7 @@ semantic_model: metrics: - { name: broken, expression: "SUM((customer.c_id" } `); - expect(await push({validateOnly: true})).toBe(1); + expect(await push({validateOnly: true})).toBe(0); expect(prints(console.warn)) .toContain( "Warning: metric 'broken' in model 'commerce' (commerce): kcmd " + @@ -3185,18 +3081,24 @@ semantic_model: for (const [what, write, problem] of otherKeys) { test(`a GOOGLE block with ${what} does not hide one`, async () => { write(); + const listsDeployments = problem.startsWith("'deployments'"); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); const logSpy = spyOn(console, 'log').mockImplementation(() => {}); const alone = "(under profile 'prod', the model file must load on its " + 'own before the profile applies)'; - expect(await push({validateOnly: true, profile: 'prod'})).toBe(1); + // A push of a model that lists deployments takes no --profile. + expect(await push({validateOnly: true, + ...(listsDeployments ? {} : {profile: 'prod'})})) + .toBe(1); expect(messages(errorSpy)).toContain(problem); - expect(messages(errorSpy)).toContain(alone); + if (!listsDeployments) expect(messages(errorSpy)).toContain(alone); errorSpy.mockClear(); expect(await profiles({profile: 'prod'})).toBe(1); expect(messages(logSpy)).toContain(" profile 'prod'"); - expect(messages(errorSpy)).toContain(alone); + // For a model with deployments, the report gives a plain push's error. + expect(messages(errorSpy)).toContain(listsDeployments ? problem : alone); + if (listsDeployments) expect(messages(errorSpy)).not.toContain(alone); errorSpy.mockClear(); expect(await profiles({printStore: true, profile: 'prod'})).toBe(1); expect(messages(errorSpy)).toContain(problem); @@ -3340,18 +3242,45 @@ semantic_model: .toBeLessThan(report.indexOf(" profile 'prod'")); }); - test('a graph push of one is refused as a named profile, alone or under --all-profiles', async () => { - writeModel(JSON.stringify({ - deployments: [{name: 'main', target: `${BQ}/raw/propertyGraphs/g`}], - profiles: [{name: 'prod', entities: [{name: 'Customer', source: PROD_SOURCE}]}], - })); + // Profile prod beside a graph target, outside any deployments list. + test('a graph push of one is refused as a named profile, alone, under --all-profiles, in the report or as default_profile, where the refusal points at --profile default', async () => { + const targeted = JSON.stringify({ + deploymentTargets: [`${BQ}/raw/propertyGraphs/g`], + profiles: [binding('prod', PROD_SOURCE)], + }); + writeModel(targeted); const errorSpy = spyOn(console, 'error').mockImplementation(() => {}); + spyOn(console, 'log').mockImplementation(() => {}); + const bqSpy = mockBigQuery(); expect(await push({validateOnly: true, profile: 'prod'})).toBe(1); expect(messages(errorSpy)).toContain(`[commerce] ${NO_TARGET}`); errorSpy.mockClear(); expect(await push({validateOnly: true, allProfiles: true})).toBe(1); expect(messages(errorSpy)).toContain(`[commerce] ${NO_TARGET}`); + errorSpy.mockClear(); + expect(await profiles({profile: 'prod'})).toBe(1); + expect(messages(errorSpy)).toContain(`[commerce] ${NO_TARGET}`); + + // Without --profile, push takes default_profile. + writeModel(targeted, 'default_profile: prod\n'); + errorSpy.mockClear(); + expect(await push({validateOnly: true, kc: false})).toBe(1); + expect(messages(errorSpy)).toContain( + "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 own " + + 'bindings to be complete.'); + expect(bqSpy).not.toHaveBeenCalled(); + + // The command it names deploys the model file's own bindings. + errorSpy.mockClear(); + expect(await push({validateOnly: true, kc: false, profile: 'default'})) + .toBe(0); + expect(messages(errorSpy)).not.toContain('is a named profile'); + expect(bqSpy).toHaveBeenCalledTimes(1); + expect(bqSpy.mock.calls[0][0][0].model.entities?.[0].dataSource) + .toBe('test-project.raw.customer'); }); test('default_profile naming one applies it to a push with no --profile', async () => { diff --git a/toolbox/mdcode/tests/tool/profiles.test.ts b/toolbox/mdcode/tests/tool/profiles.test.ts index 8fa05cad..0e38271e 100644 --- a/toolbox/mdcode/tests/tool/profiles.test.ts +++ b/toolbox/mdcode/tests/tool/profiles.test.ts @@ -18,7 +18,9 @@ import * as bigQueryDeploy from '../../src/libts/semantic/deploy_bigquery'; import {createSemanticRuntimes} from '../../src/libts/semantic/runtime/runtime'; import * as sqlParser from '../../src/libts/semantic/sql_parser'; import {profiles, push} from '../../src/tool/commands'; -import {stubCloudSqlEntry} from '../libts/mocks'; +import {passGraphChecks, stubCloudSqlEntry} from '../libts/mocks'; + +import {modelFile} from './two_phase/harness'; const CTX = new ApiContext('test-project', 'us', 'test-token'); @@ -122,6 +124,7 @@ beforeEach(() => { spyOn(globalThis, 'fetch').mockImplementation((() => { throw new Error('a test made a network call'); }) as any); + passGraphChecks(); spyOn(ApiContext, 'default').mockReturnValue(CTX); spyOn(console, 'log').mockImplementation((...a: any[]) => { logs.push(a.join(' ')); @@ -684,11 +687,10 @@ describe('kcmd profiles reports what push rejects', () => { "inline 'default' binding."; // Purchases join customers on the customer's key, c_id. `targets` is how - // many graph deployments the model declares, of which a graph push takes - // exactly one. `joined: false` leaves the join without columns, which push - // rejects for a model with a graph target. + // many graph deployments the model declares. `joined: false` leaves the join + // without columns, which push rejects for a model with a graph target. function commerce({targets = 0, joined = true} = {}): string { - const deployments = ['prod', 'qa'].slice(0, targets).map( + const deployments = ['prod'].slice(0, targets).map( d => ` - name: ${d}\n target: ${GRAPH}_${d}\n`); return `version: "0.2.0.dev0/google" semantic_model: @@ -1063,21 +1065,6 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; ' - name: customer\n bogus_key: x\n', }, ], - [ - 'a profile file sits beside a model file with a graph deployment', - { - 'commerce.yaml': commerce({targets: 1}), - 'commerce.profile.prod.yaml': profileFile('prod'), - }, - ], - [ - 'the model file\'s deployment names the profile', - { - 'commerce.yaml': commerce({targets: 1}) - .replace(/(target: \S+)\n/, '$1\n profile: prod\n'), - 'commerce.profile.prod.yaml': profileFile('prod'), - }, - ], [ 'the model file does not load on its own', { @@ -1106,23 +1093,22 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; }); } - // Push refuses a deployment that names a profile. The report has no check - // of its own for it: it shows push's refusal under each binding push - // refuses. - test('shows push\'s refusal of a deployment that names a profile under each binding', async () => { + // Push applies the profile a deployment names, but it only stores a legacy + // profile, so a deployment cannot use one. The report gives push's error + // under that profile, and none under the model file's own bindings. + test('push refuses a deployment that names a legacy profile, and the report gives the error under that profile', async () => { writeScope({ 'commerce.yaml': commerce({targets: 1}) .replace(/(target: \S+)\n/, '$1\n profile: prod\n'), 'commerce.profiles/prod.yaml': legacyProfile(false), }); - const refusal = "[commerce] model 'commerce': deployment 'prod' names " + - "profile 'prod'; remove 'profile' from the deployment entry so it " + - "uses the model file's own bindings."; - expect(await pushResult('prod')).toEqual({code: 1, errors: [refusal]}); - expect(await pushResult()).toEqual({code: 1, errors: [refusal]}); + const legacy = "[commerce] deployment 'prod' names profile 'prod', which " + + "is in the legacy 'semantic_model:' form; a deployment can use only " + + 'a profile file.'; + expect(await pushResult()).toEqual({code: 1, errors: [legacy]}); expect(await profiles()).toBe(1); - expect(rejects(profileSection('prod'))).toEqual([refusal]); - expect(rejects(section(INLINE))).toEqual([refusal]); + expect(rejects(profileSection('prod'))).toEqual([legacy]); + expect(section(INLINE)).toEqual([]); }); test('prints push\'s errors at the profile\'s indent, under a heading', async () => { @@ -1194,31 +1180,9 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; expect(rejects(profileSection('b'))).toEqual(b); }); - // Two deployments fail a graph push only, so this also shows the inline - // bindings get the checks of the push that deploys them. - test('prints push\'s errors for the inline bindings', async () => { - writeScope({'commerce.yaml': commerce({targets: 2})}); - const pushed = await pushResult(); - expect(pushed.errors).toEqual( - [expect.stringContaining('declares 2 deployments')]); - expect(await profiles()).toBe(1); - expect(logs).toEqual([ - "Model 'commerce' (commerce_eg):", - NO_PROFILES, - ' push rejects:', - ` ${pushed.errors[0]}`, - ]); - logs.length = 0; - expect(await profiles({profile: 'default'})).toBe(1); - expect(logs).toEqual([ - "Model 'commerce' (commerce_eg):", - INLINE, - ' push rejects:', - ` ${pushed.errors[0]}`, - ]); - }); - - // Push generates each graph's DDL offline, and a generator can fail. + // Push generates each graph's DDL offline, and a generator can fail. Its + // error names the deployment. + const DEPLOYMENT_LABEL = "[commerce] deployment 'prod': "; const ungenerated: Array<[string, string]> = [ ['BigQuery', uniqueKeysOnly(commerce({targets: 1}))], [ @@ -1232,8 +1196,8 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; test(`prints the error generating a ${system} graph gives`, async () => { writeScope({'commerce.yaml': model}); const pushed = await pushResult(); - expect(pushed.errors).toEqual( - [expect.stringContaining('cannot generate a property graph')]); + expect(pushed.errors).toEqual([expect.stringContaining( + `${DEPLOYMENT_LABEL}cannot generate a property graph`)]); expect(await profiles()).toBe(1); expect(rejects(section(NO_PROFILES))).toEqual(pushed.errors); }); @@ -1251,13 +1215,16 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; deployed: 0, }); const pushed = await pushResult(); - expect(pushed).toEqual({code: 1, errors: ['simulated generator failure']}); + expect(pushed).toEqual({ + code: 1, + errors: [`${DEPLOYMENT_LABEL}simulated generator failure`], + }); expect(await profiles()).toBe(1); expect(rejects(section(NO_PROFILES))).toEqual(pushed.errors); }); - // `kcmd push --profile default`, and a bare push when no `default_profile` - // is set, deploy the inline bindings beside the profile files. + // `kcmd push --profile default` deploys the inline bindings beside the + // profile files, whatever `default_profile` names. for (const defaultProfile of [undefined, 'prod']) { test(`checks the inline bindings beside a profile file${ defaultProfile ? ' when default_profile is set' : ''}`, @@ -1282,12 +1249,15 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; expect(rejects(section(` profile 'prod'${marker}`))) .toEqual(prod.errors); expect(rejects(section(INLINE))).toEqual(pushed.errors); + logs.length = 0; + expect(await profiles({profile: 'default'})).toBe(1); + expect(rejects(section(INLINE))).toEqual(pushed.errors); }); } - // A push that names no profile deploys `default_profile`, so it fails when - // the model does not declare that profile. A push that names a profile does - // not read `default_profile`. + // A push that names no profile refuses a `default_profile` the model does + // not declare, and a push that names a profile does not read it. The report + // prints the error above the profiles. test('prints push\'s error when default_profile names a profile the model does not declare', async () => { writeScope( { @@ -1409,11 +1379,12 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; // With KC_V2_ASPECTS on, push reads the overview sidecars and checks every // aspect the catalog leg writes, before any graph deploys. A malformed - // sidecar is reported once, for the scope. + // sidecar is reported once, for the scope. For a model that lists + // deployments, an aspect too large belongs to the model. for (const [label, files, error, profile, indent = ' '] of [ ['an aspect over the size limit in a graph push', {'commerce.yaml': bigBlock(commerce({targets: 1}))}, - 'too large to store in Knowledge Catalog'], + 'too large to store in Knowledge Catalog', undefined, ' '], ['an aspect over the size limit for a named profile', { 'commerce.yaml': bigBlock(commerce()), 'commerce.profile.prod.yaml': profileFile('prod'), @@ -1467,9 +1438,19 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; expect(await profiles()).toBe(0); expect(warned.splice(0)).toEqual( [warning, sidecar].map(w => ` profile 'default': warning: ${w}`)); - delete process.env.KC_V2_ASPECTS; - writeScope({'commerce.yaml': model}); + // A model that lists deployments: the report warns once, as push does. + const deployed = commerce({targets: 1}).replace( + ' - name: commerce\n', + ' - name: commerce\n ai_context: {custom: {cap: .inf}}\n'); + writeScope({'commerce.yaml': deployed}); expect(await profiles()).toBe(0); + expect(warned.splice(0)) + .toEqual([warning, sidecar].map(w => `Warning: ${w}`)); + delete process.env.KC_V2_ASPECTS; + for (const m of [model, deployed]) { + writeScope({'commerce.yaml': m}); + expect(await profiles()).toBe(0); + } expect(warned).toEqual([]); }); @@ -1662,6 +1643,7 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; "'from_columns' and 'to_columns' itself", ]; + // The report gives a profile's errors under that profile. test('when the push deploys another binding', async () => { writeScope({ 'commerce.yaml': commerce({targets: 1}), @@ -1670,7 +1652,8 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; const errors = incomplete('commerce.profile.staging.yaml'); expect(await pushResult()).toEqual({code: 1, errors}); expect(await profiles()).toBe(1); - expect(rejects(section(INLINE))).toEqual(errors); + expect(rejects(profileSection('staging'))).toEqual(errors); + expect(section(INLINE)).toEqual([]); }); // The completeness check also requires each field a profile binds to hold @@ -1705,6 +1688,13 @@ ${exclude ? ' fields_exclude: [notes]\n' : ''}`; "for this entity's table."; await expectPushes({default: [inherited], prod: [inherited]}); expect(await pushResult({deployment: false})).toEqual({code: 1, errors: [inherited]}); + // With deployments listed, the report gives it under default only. + writeScope({'commerce.yaml': unrestated.replace( + ' entities:\n', + ` deployments: [{name: g, target: ${GRAPH}_g}]\n entities:\n`)}); + expect(await profiles()).toBe(1); + expect(rejects(section(INLINE))).toEqual([inherited]); + expect(rejects(profileSection('prod'))).toEqual([]); writeScope({'commerce.yaml': unrestated.replace('[party]', '[party, ghost]')}); expect(await pushResult()).toEqual({code: 1, errors: [expect.stringContaining( "entity 'customer' extends unknown entity 'ghost'")]}); @@ -2012,7 +2002,7 @@ semantic_model: // reports, and what push's key check may misread. It compares no join // until the hierarchy resolves. test('and reports each key and join problem at most once', async () => { - const model = (targets = 0) => joinOnEmail(commerce({targets})); + const model = () => joinOnEmail(commerce()); const unknown = (binding: string) => `model 'commerce' (${binding}): entity 'customer' extends unknown ` + "entity 'nope'; it is not defined in the model. Checks that need " + @@ -2032,8 +2022,8 @@ semantic_model: ], }], ['a legacy wrapper that excludes the key field', { - 'commerce.yaml': model(1), - 'commerce.profiles/legacy.yaml': legacyProfile(false).replace( + 'commerce.yaml': model(), + 'commerce.profiles/legacy.yaml': legacyProfile(true).replace( /source: .*/, 'fields_exclude: [c_id]'), 'commerce.profiles/staging.yaml': profileFile('staging'), }, {legacy: [own("commerce, profile 'legacy'", 'c_email')]}], @@ -2253,12 +2243,14 @@ semantic_model: // graph deploys, so its profile errors leave every graph as it was. test('before any graph deploys, under --no-kc with two models', async () => { writeScope({ - 'commerce.yaml': commerce({targets: 1}), + 'commerce.yaml': + modelFile({google: {deploymentTargets: [`${GRAPH}_prod`]}}), 'inventory.yaml': commerce().replace('name: commerce', 'name: inventory'), 'inventory.profile.staging.yaml': STAGING, }); const deploy = spyOn(bigQueryDeploy, 'deployBigQuery'); - expect(await push({validateOnly: true, kc: false})).toBe(1); + expect(await push({validateOnly: true, kc: false, profile: 'default'})) + .toBe(1); expect(deploy).not.toHaveBeenCalled(); expect(logs).toEqual(expect.arrayContaining( incomplete('inventory.profile.staging.yaml').map(e => `Error: ${e}`))); @@ -2352,27 +2344,6 @@ ${shopExtends} source: ${BQ('dev', 'shop')} /^Model document 'commerce': [^]*\(under profile 'staging', the model file must load on its own before the profile applies\)$/)]); }); - // A deployment that names a profile stops the push before the profile - // checks, so an incomplete profile file is reported on the next push. - test('after a deployment that names a profile is fixed', async () => { - writeScope({ - 'commerce.yaml': commerce({targets: 1}).replace( - /(target: \S+_prod\n)/, '$1 profile: staging\n'), - 'commerce.profile.staging.yaml': STAGING, - }); - expect(await pushResult()).toEqual({ - code: 1, - errors: [ - "[commerce] model 'commerce': deployment 'prod' names profile " + - "'staging'; remove 'profile' from the deployment entry so it " + - "uses the model file's own bindings.", - ], - }); - writeScope({'commerce.yaml': commerce({targets: 1})}); - expect(await pushResult()).toEqual( - {code: 1, errors: incomplete('commerce.profile.staging.yaml')}); - }); - // --all-profiles --no-kc deploys each profile's graph and publishes // nothing, so a model that deploys no graph and does not declare the // default_profile is neither deployed nor checked. @@ -2860,12 +2831,13 @@ semantic_model: ]; for (const [file, block, expected] of cases) { writeScope({ - 'commerce.yaml': commerce({targets: 1}), + 'commerce.yaml': commerce(), 'commerce.profile.staging.yaml': file, 'shop.yaml': vanilla([block], 'shop'), }); logs.length = 0; - expect(await push({validateOnly: true, kc: false})).toBe(1); + expect(await push({validateOnly: true, kc: false, profile: 'default'})) + .toBe(1); expect(logs.filter(l => l.startsWith('Error: '))) .toEqual(expected.map(e => `Error: ${e}`)); } @@ -2873,13 +2845,11 @@ semantic_model: }); }); - // Push judges each deployment on the model file as loaded, and `kcmd - // profiles` reports what push rejects. Each deployment uses the model - // file's own bindings, since push refuses one that names a profile. + // Push judges each deployment that names no profile on the model file as + // loaded, and `kcmd profiles` reports what push rejects. describe('push holds each deployment to the rules of its engine', () => { const OWN = "model 'commerce' (commerce) deployment 'prod' uses the " + "model file's own bindings, "; - const noProfile = {profile: false}; const noDeployment = {deployment: false}; // `model`, a commerce() text, with `fields` added to purchase after p_id. const withFields = (model: string, ...fields: string[]) => model.replace( @@ -2890,45 +2860,28 @@ semantic_model: `${withFields(commerce({targets: 1}), ...fields)} metrics:\n` + ` - ${metric}\n`; - // Each unbound entity or relationship is reported once, by push's own - // checks where they see it and by this check otherwise. No warning prints. - test('rejects a deployment that uses incomplete inline bindings, on every push but a legacy wrapper push', async () => { + // Each unbound entity or relationship is reported once. No warning prints. + test('rejects a deployment that uses incomplete inline bindings, on every push', async () => { const unbound = (left = "entity 'customer' without a source") => `${OWN}which leave ${left}; a deployment needs every entity and ` + 'relationship bound.'; writeScope({'commerce.yaml': customerUnbound(commerce({targets: 1}))}); - expect(await pushResult()).toEqual({ - code: 1, - errors: [expect.stringContaining( - "dataset 'customer': a non-abstract dataset requires a source")], - }); - for (const opts of [noProfile, noDeployment]) { - expect(await pushResult(opts)).toEqual({code: 1, errors: [unbound()]}); - } - // A legacy wrapper push deploys the wrapper's bindings. - writeScope({'commerce.profiles/legacy.yaml': legacyProfile(false)}); - expect(await pushResult('legacy')).toEqual({code: 0, errors: []}); - fs.rmSync(path.join(dir, 'catalog'), {recursive: true, force: true}); + expect(await pushResult()).toEqual({code: 1, errors: [unbound()]}); writeScope({'commerce.yaml': commerce({targets: 1, joined: false})}); const unjoined = await pushResult(); expect(unjoined).toEqual({ code: 1, - errors: [expect.stringContaining( - "relationship 'placed_by' in model 'commerce' (commerce) " + - 'targets a graph but has no join columns')], + errors: [unbound("relationship 'placed_by' without join columns")], }); - expect(await pushResult(noProfile)).toEqual(unjoined); - expect((await pushResult(noDeployment)).errors) - .toEqual([unbound("relationship 'placed_by' without join columns")]); expect(await profiles()).toBe(1); expect(rejects(section(NO_PROFILES))).toEqual(unjoined.errors); writeScope({ 'commerce.yaml': customerUnbound(commerce({targets: 1, joined: false})), 'commerce.profile.prod.yaml': profileFile('prod'), - }, 'prod'); - expect(await pushResult(noProfile)).toEqual({ + }); + expect(await pushResult(noDeployment)).toEqual({ code: 1, errors: [unbound("entity 'customer' without a source and " + "relationship 'placed_by' without join columns")], @@ -2984,9 +2937,9 @@ semantic_model: .replace(`${GRAPH}_prod`, db) .replaceAll(BQ('dev', ''), `${db}/schemas/public/tables/`); writeScope({'commerce.yaml': alloyDb('note', 'p_id')}); - expect(await pushResult(noProfile)).toEqual({code: 0, errors: []}); + expect(await pushResult(noDeployment)).toEqual({code: 0, errors: []}); writeScope({'commerce.yaml': alloyDb('p_id', 'note')}); - expect(await pushResult(noProfile)) + expect(await pushResult(noDeployment)) .toEqual({code: 1, errors: [reads('m', 'purchase.note')]}); }); @@ -3002,7 +2955,7 @@ semantic_model: '[{ dialect: BIGQUERY, expression: p_id }] } }'), }); const spanner = `${modelNoText('purchase.p_id', 'SPANNER')}.`; - for (const opts of [{}, noProfile, noDeployment]) { + for (const opts of [{}, noDeployment]) { expect({opts, ...await pushResult(opts)}) .toEqual({opts, code: 1, errors: [spanner]}); } @@ -3021,15 +2974,11 @@ semantic_model: withFields(commerce({targets}), amount(dialect, text)), }); const label = {targets, dialect, text}; - for (const opts of [{}, noDeployment]) { - expect({...label, ...await pushResult(opts)}) - .toEqual({...label, code: 1, errors}); - } - for (const opts of [noProfile, noDeployment]) { - expect({...label, ...await pushResult({...opts, transpile: true})}) - .toEqual({...label, code: transpiledErrors.length ? 1 : 0, - errors: transpiledErrors}); - } + expect({...label, ...await pushResult()}) + .toEqual({...label, code: 1, errors}); + expect({...label, ...await pushResult({...noDeployment, transpile: true})}) + .toEqual({...label, code: transpiledErrors.length ? 1 : 0, + errors: transpiledErrors}); } } }); @@ -3043,10 +2992,11 @@ semantic_model: 'commerce.yaml': withMetric('{ name: half, expression: "SUM(purchase.p_id) / 2" }'), }); - await expectPushes({ - default: [`${MEASURE}'half' cannot be a measure: it is not SUM, AVG, ` + - 'COUNT, MIN or MAX over one operand.'], - }); + const half = `${MEASURE}'half' cannot be a measure: it is not SUM, ` + + 'AVG, COUNT, MIN or MAX over one operand.'; + expect(await pushResult()).toEqual({code: 1, errors: [half]}); + expect(await profiles()).toBe(1); + expect(rejects(section(NO_PROFILES))).toEqual([half]); const block = (data: object) => `custom_extensions: [{vendor_name: GOOGLE, data: '${ JSON.stringify(data)}'}]`; @@ -3119,8 +3069,8 @@ semantic_model: 'commerce.yaml': customerUnbound(commerce({targets: 1})) .replace(/(target: \S+)\n/, '$1\n profile: prod\n'), }); - expect((await pushResult(noProfile)).code).toBe(1); - expect(await profiles()).toBe(1); + expect(await pushResult()).toEqual({code: 0, errors: []}); + expect(await profiles()).toBe(0); expect(newWarnings()).toEqual([]); }); diff --git a/toolbox/mdcode/tests/tool/store_agreement.test.ts b/toolbox/mdcode/tests/tool/store_agreement.test.ts index cf0e276c..c3d30ec2 100644 --- a/toolbox/mdcode/tests/tool/store_agreement.test.ts +++ b/toolbox/mdcode/tests/tool/store_agreement.test.ts @@ -4,9 +4,11 @@ // `kcmd profiles --print-store` and `kcmd skills-generate` over it. On // deployments and profile files, no runtime command accepts a model push // rejects, and none rejects a model push accepts, apart from a few cases. The -// docs describe a deployment that names a profile, `default` included, a -// profile file that no deployment uses, an AlloyDB target and several -// targets. Push also accepts a Spanner model that binds a table outside its +// docs describe a profile file that no deployment uses. Push refuses a Spanner +// deployment that names no profile in a model with metrics, which the runtime +// accepts. The `kcmd profiles` report gives the errors a plain push gives, so +// it accepts `--profile` on a model that lists deployments, which push +// refuses. Push also accepts a Spanner model that binds a table outside its // database today. The runtime refuses it on purpose: a write would land in // the target database's table of that name, so --print-store gives no store // and skills-generate fails for a model with actions. @@ -41,6 +43,9 @@ import {CatalogClient} from '../../src/libts/gcp/dataplex'; import * as kc from '../../src/libts/semantic/deploy_knowledge_catalog'; import * as validateMod from '../../src/libts/semantic/validate'; import {profiles, push, skillsGenerate} from '../../src/tool/commands'; +import {passGraphChecks} from '../libts/mocks'; + +import {msg} from './two_phase/harness'; const CTX = new ApiContext('test-project', 'us', 'test-token'); @@ -125,13 +130,15 @@ beforeEach(() => { throw new Error('a test made a network call'); }) as any); // Push's live checks and its Knowledge Catalog leg. Their local checks run - // before them, in prepareModels. + // before them: in checkBeforeWrites for a model that lists deployments, and + // in prepareModels otherwise. spyOn(CatalogClient.prototype, 'getEntryType') .mockResolvedValue({status: 200, result: {}} as any); spyOn(CatalogClient.prototype, 'getAspectType') .mockResolvedValue({status: 200, result: {}} as any); spyOn(validateMod, 'validateBigQueryDataSources').mockResolvedValue([]); spyOn(validateMod, 'validateBigQueryActionStatements').mockResolvedValue([]); + passGraphChecks(); spyOn(validateMod, 'validateSpannerActionStatements').mockResolvedValue([]); spyOn(kc, 'deployKnowledgeCatalog').mockResolvedValue({ success: true, @@ -254,9 +261,8 @@ semantic_model: }); -// A deployment whose `profile` names no profile the model declares. Push -// refuses any deployment that names a profile, and the runtime refuses one -// that names an undeclared profile. +// A deployment whose `profile` names no profile the model declares. Push and +// the runtime refuse it. describe('a deployment that names an undeclared profile', () => { test('push rejects it, so skills-generate does too', async () => { writeScope({'commerce.yaml': `version: "0.2.0.dev0/google" @@ -303,8 +309,9 @@ entities: source: ${SPANNER_DB}/tables/Orders `, }); - const r = await runAll('prod'); + const r = await runAll(); expect(r.push.code).toBe(1); + expect(r.push.out).toContain("names unknown binding profile 'prdo'"); expect(r.skills.code).toBe(1); }); }); @@ -561,7 +568,7 @@ ${entities}`}); // The differences the docs describe, and cases where every command agrees. describe('documented divergences', () => { - test('a deployment that names a declared profile: push refuses, the runtime uses it', + test('a Spanner deployment that names no profile, in a model with metrics: push and the report refuse, the runtime accepts it', async () => { writeScope({ 'commerce.yaml': `version: "0.2.0.dev0/google" @@ -570,26 +577,25 @@ semantic_model: deployments: - name: prod target: ${SPANNER_DB}/graphs/commerce - profile: prod entities: - name: Order source: ${SPANNER_DB}/tables/Orders primary_key: [key] fields: - { name: key, expression: OrderId } -`, - 'commerce.profile.prod.yaml': `name: prod -entities: - - name: Order - source: ${SPANNER_DB}/tables/Orders + metrics: + - name: orders + entity: Order + expression: COUNT(Order.key) `, }); - const r = await runAll('prod'); + const r = await runAll(); expect(codes(r)).toEqual({push: 1, report: 1, printStore: 0, skills: 0}); - expect(r.printStore.out).toBe('acme-ops/prod/commerce'); + expect(r.push.out).toContain(msg.spannerMetrics('prod')); + expect(r.report.out).toContain(msg.spannerMetrics('prod')); }); - test('Spanner beside BigQuery: push refuses two, the runtime takes Spanner', + test('Spanner beside BigQuery: push\'s local checks pass, the runtime takes Spanner', async () => { writeScope({'commerce.yaml': `version: "0.2.0.dev0/google" semantic_model: @@ -607,7 +613,8 @@ semantic_model: - { name: key, expression: OrderId } `}); const r = await runAll(); - expect(codes(r)).toEqual({push: 1, report: 1, printStore: 0, skills: 0}); + expect(codes(r)).toEqual({push: 0, report: 0, printStore: 0, skills: 0}); + expect(r.printStore.out).toBe('acme-ops/prod/commerce'); }); test('a vanilla GOOGLE block with two Spanner targets: every command refuses', @@ -735,16 +742,6 @@ entities: expect(r.skills.out).toContain("deployment 'prod' targets"); }); - // Push refuses a deployment that names a profile, whatever its target. The - // runtime uses dev, as the docs say. - test('a valid deployment for another declared profile: push refuses, the runtime uses dev', - async () => { - writeScope(devAndProd(`${SPANNER_DB}2/graphs/commerce`)); - const r = await runAll(); - expect(codes(r)).toEqual({push: 1, report: 1, printStore: 0, skills: 0}); - expect(r.printStore.out).toBe('acme-ops/prod/commerce'); - }); - test('a malformed deployment for default under a profile file: every command refuses', async () => { writeScope({ @@ -769,6 +766,8 @@ entities: }); const r = await runAll('prod'); expect(codes(r)).toEqual({push: 1, report: 1, printStore: 1, skills: 1}); + expect(r.push.out).toContain(msg.flag.profile); + expect(r.report.out).toContain("deployment 'dev' targets"); expect(r.printStore.out).toContain("deployment 'dev' targets"); expect(r.skills.out).toContain("deployment 'dev' targets"); }); @@ -809,9 +808,9 @@ semantic_model: } }); - // Push and the report read every model's profile files first. A file whose - // `name:` does not match its file name throws there, and the CLI prints the - // error and exits 1. + // The report throws on a profile file whose `name:` does not match its file + // name, and the CLI prints the error and exits 1. Push prints the same error + // and exits 1. test('a profile file whose name does not match its file name: every command refuses', async () => { writeScope({ @@ -838,11 +837,12 @@ entities: "name 'prd', which does not match filename suffix 'prod'."; const r = await runAll(); expect(codes(r)).toEqual({ - push: `threw: ${mismatch}`, + push: 1, report: `threw: ${mismatch}`, printStore: 1, skills: 1, }); + expect(r.push.out).toBe(`Error: ${mismatch}`); expect(r.printStore.out).toBe(`Error: ${mismatch}`); expect(r.skills.out).toBe(`Error: ${mismatch}`); }); @@ -910,10 +910,13 @@ entities: const mismatch = "[commerce.profiles/prod.yaml] profile 'prod' " + "declares name 'prd', which does not match its file's profile " + "name 'prod'"; - for (const r of [await runAll(), await runAll('prod')]) { + for (const [r, pushError] of [ + [await runAll(), mismatch], + [await runAll('prod'), msg.flag.profile], + ] as const) { expect(codes(r)) .toEqual({push: 1, report: 1, printStore: 1, skills: 1}); - expect(r.push.out).toContain(`Error: ${mismatch}`); + expect(r.push.out).toContain(`Error: ${pushError}`); expect(r.printStore.out).toBe(`Error: ${mismatch}`); expect(r.skills.out).toContain(`Error: ${mismatch}`); } @@ -1007,7 +1010,7 @@ semantic_model: expect(r.push.out).toContain('is not valid UTF-8'); }); - test('a deployment that names profile default: push refuses, the runtime uses it', + test('a deployment that names profile default: push and the runtime use it', async () => { writeScope({'commerce.yaml': `version: "0.2.0.dev0/google" semantic_model: @@ -1024,11 +1027,11 @@ semantic_model: - { name: key, expression: OrderId } `}); const r = await runAll(); - expect(codes(r)).toEqual({push: 1, report: 1, printStore: 0, skills: 0}); + expect(codes(r)).toEqual({push: 0, report: 0, printStore: 0, skills: 0}); expect(r.printStore.out).toBe('acme-ops/prod/commerce'); }); - test('a profile file no deployment uses: push refuses, skills-generate writes a skill with no store', + test('a profile file no deployment uses: push accepts it without --profile, --print-store gives no store, skills-generate accepts it', async () => { writeScope({ 'commerce.yaml': `version: "0.2.0.dev0/google" @@ -1044,15 +1047,12 @@ semantic_model: fields: - { name: key, expression: OrderId } `, - 'commerce.profile.prod.yaml': `name: prod -entities: - - name: Order - source: ${SPANNER_DB}/tables/Orders -`, + 'commerce.profile.prod.yaml': PROD_PROFILE, }); const r = await runAll('prod'); - expect(codes(r)).toEqual({push: 1, report: 1, printStore: 1, skills: 0}); - expect(r.push.out).toContain("binding profile 'prod' is a named profile"); + expect(codes(r)).toEqual({push: 1, report: 0, printStore: 1, skills: 0}); + expect(r.push.out).toContain(msg.flag.profile); + expect(await push({validateOnly: true})).toBe(0); }); test('a logical model with no deployments: push and skills-generate accept it', @@ -1174,6 +1174,7 @@ ${extendsList ? ` extends: [${extendsList.join(', ')}]\n` : ''}\ }; } + // The model's block is named `google`, which kcmd reads as a GOOGLE block. function vanillaScope(deployments: Deployment[], extendsList?: string[]): Record { const model = JSON.stringify({ @@ -1191,7 +1192,7 @@ ${extendsList ? ` extends: [${extendsList.join(', ')}]\n` : ''}\ semantic_model: - name: commerce custom_extensions: - - vendor_name: GOOGLE + - vendor_name: google data: '${model}' datasets: - name: Order @@ -1219,26 +1220,30 @@ ${dataset} fields: const dev = {name: 'dev', target: `${SPANNER_DB}/graphs/commerce`}; const prod = (target: string) => ({name: 'prod', target, profile: 'prod'}); - // The runtime checks that each deployment's profile is declared. A vanilla - // model declares prod in its GOOGLE block. - test('a deployment for a declared profile, under default: the runtime uses dev', + // Push accepts dev with the model file's bindings and prod with profile + // prod, and the runtime uses dev. A vanilla model declares prod in its + // GOOGLE block. + test('a deployment for a declared profile, under default: push accepts both, the runtime uses dev', async () => { const {google, vanilla} = await bothFlavors( [dev, prod(`${SPANNER_DB}2/graphs/commerce`)]); expect(codes(google)) - .toEqual({push: 1, report: 1, printStore: 0, skills: 0}); + .toEqual({push: 0, report: 0, printStore: 0, skills: 0}); expect(codes(vanilla)).toEqual(codes(google)); // The vanilla expression form adds a dialect warning on its own line. expect(vanilla.printStore.out.split('\n').pop()) .toBe(google.printStore.out); + expect(google.printStore.out).toBe('acme-ops/prod/commerce'); }); - test('a deployment for a declared profile, under it: the runtime uses it', + test('a deployment for a declared profile, under it: push takes no --profile, the runtime uses it', async () => { const {google, vanilla} = await bothFlavors( [prod(`${SPANNER_DB}/graphs/commerce`)], 'prod'); expect(codes(google)) - .toEqual({push: 1, report: 1, printStore: 0, skills: 0}); + .toEqual({push: 1, report: 0, printStore: 0, skills: 0}); + expect(google.push.out).toContain(msg.flag.profile); + expect(google.printStore.out).toBe('acme-ops/prod/commerce'); expect(codes(vanilla)).toEqual(codes(google)); expect(vanilla.printStore.out.split('\n').pop()) .toBe('acme-ops/prod/commerce'); diff --git a/toolbox/mdcode/tests/tool/two_phase/catalog_only.test.ts b/toolbox/mdcode/tests/tool/two_phase/catalog_only.test.ts index 07223ba8..3651ead7 100644 --- a/toolbox/mdcode/tests/tool/two_phase/catalog_only.test.ts +++ b/toolbox/mdcode/tests/tool/two_phase/catalog_only.test.ts @@ -10,11 +10,11 @@ import * as YAML from 'yaml'; import {createSemanticRuntimes} from '../../../src/libts/semantic/runtime/runtime'; -import {boundTo, bqGraph, bqTable, Case, CTX, CUSTOMER, err, EXPRESSION_KEY, has, kcResult, msg, PREPARED, scenarios, Scope, SNOWFLAKE_METRIC, TRANSPILE_WARNING, writeScope} from './harness'; +import {boundTo, bqGraph, bqTable, Case, CTX, CUSTOMER, err, EXPRESSION_KEY, has, kcResult, msg, PREPARED, pushed, scenarios, Scope, SNOWFLAKE_METRIC, TRANSPILE_WARNING, writeScope} from './harness'; const PROD = boundTo('prod'); -const legacy = (model: string, entities?: object[]) => - YAML.stringify({semantic_model: [{name: model, entities}]}); +const legacy = (model: string, entities?: object[], actions?: object[]) => + YAML.stringify({semantic_model: [{name: model, entities, actions}]}); const PLAN = ['-- Knowledge Catalog --', 'PLAN LINE']; // A catalog preflight that plans one line and gives `result`. @@ -189,4 +189,52 @@ scenarios('push --no-deployment refuses a scope before any network call', [ 'profiles: prod.')], then: ['prod', 'default'].map(p => ({defaultProfile: p, code: 0})), }), + sameAsRuntime({ + name: '--no-deployment refuses a default_profile the runtime refuses: the profile file has an action that does not load', + profiles: { + prod: {...PROD, actions: [{name: 'refund', executor: {bogus: {}}}]}, + }, + defaultProfile: 'prod', + errors: [has("action 'refund'")], + }), + { + name: '--no-deployment refuses a default_profile whose action push refuses', + deployments: ['g'], + actions: [{name: 'refund', executor: {mcp: {server: 's', tool: 't'}}}], + profiles: {prod: {...PROD, actions: [ + {name: 'refund', executor: {mcp: {server: 's', tool: ' '}}}, + ]}}, + defaultProfile: 'prod', + errors: [err( + "action 'refund' in model 'commerce' (commerce, profile 'prod') has " + + "an mcp executor whose 'tool' is missing or blank.")], + then: [{ + profiles: {}, + legacy: {prod: legacy('commerce', undefined, [ + {name: 'refund', executor: {mcp: {server: 's', tool: ' '}}}, + ])}, + code: 1, rejects: [pushed()], + }], + report: {code: 1, rejects: {'prod (default)': [pushed()]}}, + }, + sameAsRuntime({ + name: '--no-deployment refuses a default_profile the runtime refuses: the profile is a legacy wrapper that does not merge', + legacy: { + prod: legacy( + 'shop', [{name: 'Customer', source: bqTable('customer', 'prod')}]), + }, + defaultProfile: 'prod', + errors: [has("model 'shop' is not in the logical model")], + }), + sameAsRuntime({ + name: '--no-deployment refuses a default_profile the runtime refuses: the profile is a legacy wrapper that merges but does not load', + deployments: ['g'], + legacy: { + prod: legacy( + 'commerce', [{name: 'Customer', source: 'not a table at all'}]), + }, + defaultProfile: 'prod', + errors: [has("source 'not a table at all' looks like a SQL query")], + report: {code: 1, rejects: {'prod (default)': [pushed()]}}, + }), ], {flags: {deployment: false}, code: 1, noNetwork: true}); diff --git a/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts b/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts index 689c11a5..a0c708aa 100644 --- a/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts +++ b/toolbox/mdcode/tests/tool/two_phase/deploy.test.ts @@ -216,7 +216,7 @@ const ALLOYDB_NOTE = "Note: deployment 'ad' targets an AlloyDB database, " + 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', + name: 'deploys each deployment after the catalog write, also under a bare --profile, 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}, @@ -250,6 +250,8 @@ scenarios('how push deploys each deployment in order, and what it prints', [ }); }, then: [ + // cac gives `true` for a bare --profile and for no --profile alike. + {flags: {profile: true}, code: 0, events: IN_ORDER}, { flags: {deployment: ['b,a']}, code: 0, diff --git a/toolbox/mdcode/tests/tool/two_phase/harness.ts b/toolbox/mdcode/tests/tool/two_phase/harness.ts index 653179d0..f7f21826 100644 --- a/toolbox/mdcode/tests/tool/two_phase/harness.ts +++ b/toolbox/mdcode/tests/tool/two_phase/harness.ts @@ -233,6 +233,8 @@ interface PushStep extends Scope { code?: number; // Every line push prints with console.error, exactly. errors?: unknown[]; + // The `Error: ` lines without that prefix, as the report prints them. + rejects?: unknown[]; warnings?: unknown[]; log?: unknown[]; // The catalog preflight, the catalog write and each graph deploy, in order. @@ -249,12 +251,32 @@ interface PushStep extends Scope { export const refused = (flags: commands.PushOptions, error: unknown) => ({flags, code: 1, errors: [error], noNetwork: true}); -// A push, then each push in `then` with what it changes. +// A `kcmd profiles` run and its outcome. A section's key is its profile's name, +// with ' (default)' for default_profile's, or 'default' for the inline +// bindings, 'no profiles', 'model' above a model's profiles or '' above the +// first model, and a second model's keys start with its name and a colon. +export interface ReportStep { + flags?: commands.ProfilesOptions; + code: number; + // Each printed section's rejects. 'model' and '' appear only with errors. + rejects?: Record; + // Gets the log and error lines in order, and the error lines alone. + check?: (lines: string[], errors: string[]) => void; +} + +// A push, then each push in `then` with what it changes, then the report. export interface Case extends PushStep { name: string; then?: PushStep[]; + report?: ReportStep; } +// Stands for the errors push `i` of a case rejects, in a list of rejects. +const PUSHED = Symbol('pushed'); +export const pushed = (i = 0) => ({[PUSHED]: i}); +const expand = (list: unknown[], runs: PushRun[]) => list.flatMap( + (e: any) => e?.[PUSHED] !== undefined ? runs[e[PUSHED]].rejects : [e]); + export const has = (text: string) => expect.stringContaining(text); export const err = (message: string) => `Error: ${message}`; @@ -285,8 +307,11 @@ async function runPush(services: Services, flags: commands.PushOptions) { errors: texts(out, 'error'), warnings: texts(out, 'warn'), printed: texts(out, 'log', 'warn'), + rejects: texts(out, 'error') + .filter(l => l.startsWith('Error: ')) + .map(l => l.slice(7)), fake, - published: fake.preflight.mock.calls.at(-1)?.[0][0].model as SemanticModel, + 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) @@ -299,12 +324,13 @@ const kindOf = (call: string) => call.split(' ')[0]; // The kinds of call the `events` outcome lists. const EVENTS = new Set(['preflight', 'apply', 'deployKc', 'deploy']); -async function checkPush(r: PushRun, step: PushStep) { +async function checkPush(r: PushRun, step: PushStep, runs: PushRun[]) { const same = (actual: unknown, expected?: unknown[]) => { - if (expected) expect(actual).toEqual(expected); + if (expected) expect(actual).toEqual(expand(expected, runs)); }; if (step.code !== undefined) expect(r.code).toBe(step.code); same(r.errors, step.errors); + same(r.rejects, step.rejects); same(r.warnings, step.warnings); same(r.log, step.log); same(r.fake.calls.filter(c => EVENTS.has(kindOf(c))), step.events); @@ -324,15 +350,53 @@ 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), - }; +// The key of a report line that starts a profile section, or undefined. +function sectionKey(line: string): string|undefined { + if (line === ' no binding profiles; the model document is its own ' + + "inline 'default' binding.") { + return 'no profiles'; + } + const m = /^ profile '([^']+)'( \(default\))?(:.*)?$/.exec(line); + return m ? m[1] + (m[2] ?? '') : undefined; +} + +// Checks a report against what `step` expects. `out` is what it printed. +function checkReport(out: Line[], step: ReportStep, runs: PushRun[]): void { + const lines = texts(out, 'log', 'error'); + // Each section's errors, rebuilt from each first line and its indented lines. + const rejects: Record = {}; + let key = '', indent = '', prefix = '', models = 0; + let found: string[]|undefined; + for (const {stream, text} of out.filter(l => l.stream !== 'warn')) { + const model = /^Model '([^']+)'/.exec(text)?.[1]; + if (model && models++) prefix = `${model}: `; + const section = model ? 'model' : sectionKey(text); + if (section !== undefined) { + key = prefix + section; + indent = model ? ' ' : ' '; + found = undefined; + if (!model) rejects[key] = []; + } else if (text === `${indent}push rejects:`) { + expect(stream).toBe('error'); + found = rejects[key] = []; + } else if (found && text.startsWith(`${indent} `)) { + expect(stream).toBe('error'); + const error = text.slice(indent.length + 2); + if (!error.startsWith(' ')) found.push(error); + else found[found.length - 1] += `\n${error.slice(2)}`; + } else { + found = undefined; + } + } + if (step.rejects) { + expect(rejects).toEqual(Object.fromEntries( + Object.entries(step.rejects).map(([k, v]) => [k, expand(v, runs)]))); + } + expect(lines.filter(l => l.trim() === 'push rejects:')) + .toHaveLength(Object.values(rejects).filter(r => r.length).length); + expect(texts(out, 'warn').filter(w => w.startsWith('Warning: '))).toEqual([]); + step.check?.(lines, texts(out, 'error')); +} async function runCase(c: Case): Promise { const cwd = process.cwd(); @@ -340,14 +404,21 @@ async function runCase(c: Case): Promise { const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'kcmd-two-phase-')); process.chdir(dir); try { + const runs: PushRun[] = []; 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}; - const flags = everyDeployment({...c, ...own}, step.flags ?? {}); - await checkPush(await runPush(services, flags), step); + runs.push(await runPush(services, step.flags ?? {})); + await checkPush(runs.at(-1)!, step, runs); + } + if (c.report) { + writeScope(dir, c); + const {out} = installRun(c.services ?? {}); + expect(await commands.profiles(c.report.flags)).toBe(c.report.code); + checkReport(out, c.report, runs); } } finally { mock.restore(); diff --git a/toolbox/mdcode/tests/tool/two_phase/report.test.ts b/toolbox/mdcode/tests/tool/two_phase/report.test.ts new file mode 100644 index 00000000..bcf363e5 --- /dev/null +++ b/toolbox/mdcode/tests/tool/two_phase/report.test.ts @@ -0,0 +1,197 @@ +// Tests of what `kcmd profiles` prints for a model that lists deployments +// (src/tool/commands.ts, profiles). Each case runs `kcmd push --validate-only` +// and then the report. + +import {expect} from 'bun:test'; + +import {bqGraph, bqTable, Case, has, modelFile, pushed, ReportStep, Scope, scenarios, SPANNER, spannerGraph} from './harness'; + +const graph = (name: string, profile?: string) => + ({name, target: bqGraph(`commerce_${name}`, 'prod'), profile}); +const entities = (at = (e: string) => bqTable(e, 'dev')) => + ['customer', 'purchase'].map(e => ({ + name: e, + source: at(e), + primary_key: [`${e[0]}_id`], + fields: [{name: `${e[0]}_id`, expression: `${e[0]}_id`}], + })); +const COLUMNS = {from_columns: ['p_customer'], to_columns: ['c_id']}; +const MODEL: Scope = { + entities: entities(), + relationships: + [{name: 'placed_by', from: 'purchase', to: 'customer', ...COLUMNS}], +}; +// customer and purchase bound to their tables in `dataset`, by name. +const boundIn = (dataset: string) => Object.fromEntries( + entities(e => bqTable(e, dataset)).map(e => [e.name, e])); +const PROD = { + entities: boundIn('prod'), + relationships: [{name: 'placed_by', ...COLUMNS}], +}; +const UNKNOWN_ACTION = + [{name: 'nothing', executor: {sql: {statements: ['SELECT 1']}}}]; +const legacy = (entity = 'customer', more = '') => + `semantic_model:\n - name: commerce\n entities:\n - name: ` + + `${entity}\n${more} source: ${bqTable(entity, 'prod')}\n`; + +const SHARED = {...MODEL, deployments: [graph('prod')], code: 1, + flags: {validateOnly: true}}; + +scenarios('kcmd profiles reports what push rejects', [ + { + // Deployment qa deploys the model file's Spanner tables, which fails + // since the model declares a metric, but push stops at prod first. + name: "prints a deployment's error under the profile it names, only for the first deployment that fails", + deployments: [ + graph('prod', 'prod'), {name: 'qa', target: spannerGraph('commerce')}, + ], + entities: entities(e => `${SPANNER}/tables/${e}`), + metrics: [{name: 'customers', expression: 'COUNT(customer.c_id)'}], + profiles: {prod: {...PROD, actions: UNKNOWN_ACTION}}, + rejects: [has("action 'nothing' is not in the logical model")], + report: {flags: {profile: 'prod'}, code: 1, rejects: {prod: [pushed()]}}, + }, + ...([ + ['a profile file overrides an action the model lacks', + {profiles: {prod: {...PROD, actions: UNKNOWN_ACTION}}}, + ["[commerce.profile.prod.yaml] profile 'prod': action 'nothing' is not " + + 'in the logical model']], + ['a legacy wrapper gives a model that does not load', + {legacy: {prod: legacy('customer', ' primary_key: 5\n')}}, + ["Model document 'commerce': Semantic model load error: [", ' {']], + ] as Array<[string, Scope, string[]]>).map(([label, scope, reason]): Case => ({ + name: `says why it shows nothing for a profile push stores when ${label}`, + ...scope, + code: 0, + rejects: [], + report: { + code: 0, + check: (lines, errors) => { + expect(errors).toEqual([]); + const [, header, ...shown] = lines; + expect([header, ...shown.slice(0, reason.length)]).toEqual([ + " profile 'prod'", ` not shown: ${reason[0]}`, + ...reason.slice(1).map(line => ` ${line}`), + ]); + for (const line of shown.slice(1)) expect(line).toStartWith(' '); + }, + }, + })), + { + // An error in qa's entry skips prod's checks. + name: 'prints no not-shown line when push stops before the deployment that uses the profile', + deployments: + [graph('prod', 'prod'), {name: 'qa', target: 'bigquery:acme.qa'}], + profiles: {prod: {...PROD, actions: UNKNOWN_ACTION}}, + rejects: [has("deployment 'qa' targets 'bigquery:acme.qa'")], + then: [{ + deployments: [graph('prod', 'prod'), graph('qa')], + code: 1, + rejects: [has("action 'nothing' is not in the logical model")], + }], + report: { + code: 1, + rejects: {model: [pushed(0)], prod: []}, + check: lines => expect(lines.slice(lines.indexOf(" profile 'prod'"))) + .toEqual([" profile 'prod'"]), + }, + }, + { + // The legacy wrapper merged into the model file gives no second error. + name: 'prints a model file that does not load once, above the models', + files: { + 'commerce.yaml': modelFile({...MODEL, deployments: [graph('prod')]}) + .replace(' entities:\n', ' bogus_key: 1\n entities:\n'), + }, + legacy: {staging: legacy()}, + rejects: [has('bogus_key')], + report: { + code: 1, + rejects: {'': [pushed()], staging: []}, + check: lines => { + expect(lines.filter(l => l.includes('load error'))).toHaveLength(1); + const model = "Model 'commerce' (commerce_eg):"; + expect(lines.slice(lines.indexOf(model))) + .toEqual([model, " profile 'staging'"]); + }, + }, + }, + { + name: "prints none of push's warnings", + profiles: {prod: PROD}, + defaultProfile: 'prod', + code: 0, + check: r => expect(r.warnings).toContainEqual( + has("catalog.yaml sets default_profile 'prod'")), + report: {code: 0, rejects: {'prod (default)': []}}, + }, + ...([ + ['a default_profile the model does not declare', {defaultProfile: 'nope'}, + "unknown binding profile 'nope'", {model: [pushed()], 'no profiles': []}], + ['a profile file named default', {legacy: {Default: legacy()}}, + "may not be named 'default'", {model: [pushed()]}], + ['a deployment profile the model does not declare', + {deployments: [graph('prod', 'nope')]}, "names unknown binding profile", + {model: [pushed()], 'no profiles': []}], + ['a join that covers no key', { + relationships: [{name: 'placed_by', from: 'purchase', to: 'customer', + from_columns: ['p_c'], to_columns: ['c_x']}], + }, 'placed_by', {model: [pushed()], 'no profiles': []}], + ['a default_profile that does not apply', + {defaultProfile: 'prod', legacy: {prod: legacy('ghost')}}, + "profile 'prod': entity 'ghost' is not in the logical model", + {'prod (default)': [pushed()]}], + ['a file too large to store', + {files: {'commerce.profile.big.yaml': `#${'x'.repeat(125e3)}\nname: big`}}, + 'too large to store', {'': [pushed()], big: []}], + ['a malformed overview sidecar', {v2Aspects: true, files: { + 'commerce.overview.md': '---\ncontentType: markdown\n---\nOrders.\n', + }}, "not 'markdown'", {'': [pushed()], 'no profiles': []}], + ['a repeated metrics_exclude name the model lacks', + {profiles: {prod: {...PROD, metrics_exclude: ['nope', 'nope']}}}, + "metric 'nope' in 'metrics_exclude'", {prod: [pushed()]}], + ] as Array<[string, Scope, string, ReportStep['rejects']]>).map( + ([label, scope, part, rejects]): Case => ({ + name: `prints ${label} once`, + ...scope, + rejects: [has(part)], + report: {code: 1, rejects}, + })), + { + name: 'prints only push\'s error for two models when one lists deployments', + deployments: [graph('prod', 'prod')], + profiles: {prod: PROD}, + files: { + 'shop.yaml': modelFile(MODEL).replace('name: commerce', 'name: shop'), + }, + rejects: [has('would receive 2 models')], + report: {code: 1, + rejects: {'': [pushed()], prod: [], 'shop: no profiles': []}}, + }, + { + name: 'fails under --profile when another profile leaves an entity without a source', + deployments: [graph('prod', 'prod')], + profiles: { + prod: PROD, + staging: {entities: {customer: boundIn('staging').customer}}, + }, + rejects: [has("profile 'staging': entity"), + has("profile 'staging': relationship")], + report: {flags: {profile: 'prod'}, code: 1, + rejects: {model: [pushed()], prod: []}}, + }, + { + name: 'prints a profile file that does not load once, under the profile', + deployments: [graph('prod', 'prod'), graph('qa')], + profiles: {prod: {entities: {customer: {bogus: 1}}}}, + rejects: [has('[commerce.profile.prod.yaml]')], + report: { + code: 1, + rejects: {prod: [pushed()]}, + check: (_, errors) => { + expect(errors.filter(l => l.includes('bogus'))).toHaveLength(1); + expect(errors.filter(l => l.includes('prod'))).toHaveLength(1); + }, + }, + }, +], SHARED); 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 64d5409f..9282e5c7 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,14 +1,44 @@ // 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). +// refuses, the deployments `--deployment` selects and checks, the +// default_profile warning and the Spanner metrics rule (src/tool/commands.ts, +// src/libts/semantic/deployment_target.ts). import {expect, test} from 'bun:test'; import {loadSemanticModels} from '../../../src/libts/semantic/loader'; -import {typedDeployment} from '../../../src/tool/commands'; +import {PushOptions, typedDeployment} from '../../../src/tool/commands'; -import {boundAt, boundTo, CUSTOMER, err, EXPRESSION_KEY, has, modelFile, msg, PushRun, refused, scenarios, SPANNER_CUSTOMER, spannerGraph} from './harness'; +import {boundAt, boundTo, Case, CUSTOMER, Deployment, err, EXPRESSION_KEY, has, modelFile, msg, PushRun, refused, scenarios, SPANNER_CUSTOMER, spannerGraph} from './harness'; +const ownBinding = (r: PushRun, model = r.published) => + expect(model.entities[0].dataSource).toBe('test-project.d.customer'); + +const CHOOSES_NO_BINDING = "Warning: catalog.yaml sets default_profile " + + "'prod', which chooses no binding on this push: Knowledge Catalog " + + 'receives the model file as written, and each deployment uses the ' + + 'profile it names.'; + +// A push of a model with sibling profile file prod, under default_profile +// prod, that prints `warnings` and publishes the model file's own binding. +const underDefault = + (name: string, deployments: Deployment[], flags: PushOptions, + warnings: string[]): Case => ({ + name: `default_profile ${name}`, + deployments, + defaultProfile: 'prod', + profiles: {prod: boundTo('prod')}, + flags, + warnings, + check: ownBinding, + }); + +const PUBLISHED = { + events: ['preflight', 'apply'], + check: (r: PushRun) => { + ownBinding(r); + expect(r.published.profiles!.map(p => p.name)).toEqual(['prod']); + }, +}; const ONE_PHASE = { called: {preflight: 0, apply: 0, deployKc: 1}, check: (r: PushRun) => { @@ -25,15 +55,14 @@ 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, as do --no-deployment with --deployment in either order, a bare or empty --deployment and a deployment name the model lacks', + name: '--no-kc, --profile, --all-profiles and --no-profile each fail before any network call, 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)), + ...refused({kc: false}, err(msg.flag.noKc)), then: [ - refused({profile: 'x', deployment: false}, err(msg.flag.profile)), - refused({allProfiles: true, deployment: false}, - err(msg.flag.allProfiles)), - refused({profile: false, deployment: false}, err(msg.flag.noProfile)), + refused({profile: 'x'}, err(msg.flag.profile)), + refused({allProfiles: true}, err(msg.flag.allProfiles)), + refused({profile: 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)), @@ -82,6 +111,47 @@ scenarios('the two-phase push path', [ ...refused({deployment: ['nope']}, err("--deployment names 'nope', but the scope has no model.")), }, + { + ...underDefault( + 'does not choose the profile of a deployment', ['g'], {}, + [`${CHOOSES_NO_BINDING} Deployment 'g' uses the model file's own ` + + 'bindings.']), + events: ['preflight', 'apply', 'deploy g'], + check: r => { + ownBinding(r); + ownBinding(r, r.deployed('g', 'bq')!); + }, + }, + underDefault( + 'needs no warning when every selected deployment names a profile and one uses it', + [{name: 'g', profile: 'prod'}], {validateOnly: true}, []), + underDefault( + 'names each deployment that names no profile when another deployment uses it', + [{name: 'a', profile: 'prod'}, 'b', 'c'], {validateOnly: true}, + [`${CHOOSES_NO_BINDING} Deployments 'b', 'c' use the model file's ` + + 'own bindings.']), + underDefault( + 'names no deployment under --no-deployment, which selects none', ['g'], + {deployment: false}, [CHOOSES_NO_BINDING]), + { + ...underDefault( + 'does not choose the bindings Knowledge Catalog receives, and gives ' + + 'no warning in a scope with no model', + [], {}, [CHOOSES_NO_BINDING]), + then: [{noModel: true, flags: {deployment: false}, code: 0, warnings: []}], + }, + { + name: 'a model with a sibling profile file and no deployments list publishes the model file as written, with every profile, and deploys nothing, with no flags or profile: true; it refuses --no-kc, and under --no-profile or with only a legacy profile file publishes in one step', + profiles: {prod: boundTo('prod')}, + ...PUBLISHED, + then: [ + {flags: {profile: true}, code: 0, ...PUBLISHED}, + refused({kc: false}, err(msg.flag.noKc)), + {flags: {profile: false}, code: 0, called: ONE_PHASE.called}, + {profiles: {}, legacy: {old: 'semantic_model:\n - name: commerce\n'}, + code: 0, called: ONE_PHASE.called}, + ], + }, { // 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',