diff --git a/core/actions/assertion.ts b/core/actions/assertion.ts index 5f4e2e81c..f9580533f 100644 --- a/core/actions/assertion.ts +++ b/core/actions/assertion.ts @@ -56,7 +56,8 @@ export const IAssertionConfigProperties = strictKeysOf()([ "name", "schema", "tags", - "type" + "type", + "dependOnDependencyAssertions" ]); /** @@ -151,7 +152,9 @@ export class Assertion extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); + const resolvableTarget = resolvableAsTarget(resolvable); + this.session.actionAssertionMap.set(resolvableTarget, this); + this.proto.dependencyTargets.push(resolvableTarget); }); return this; } diff --git a/core/actions/index.ts b/core/actions/index.ts index 9afe2cc6d..b2c542b27 100644 --- a/core/actions/index.ts +++ b/core/actions/index.ts @@ -23,6 +23,7 @@ export type SqlxConfig = ( export abstract class ActionBuilder { public session: Session; + public includeAssertionsForDependency: Map = new Map(); constructor(session?: Session) { this.session = session; diff --git a/core/actions/notebook.ts b/core/actions/notebook.ts index 15ba9f8ac..7cefc2d7a 100644 --- a/core/actions/notebook.ts +++ b/core/actions/notebook.ts @@ -1,9 +1,11 @@ import { verifyObjectMatchesProto } from "df/common/protos"; import { ActionBuilder } from "df/core/actions"; +import { Resolvable } from "df/core/common"; import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, nativeRequire, resolveActionsConfigFilename } from "df/core/utils"; @@ -18,6 +20,9 @@ export class Notebook extends ActionBuilder { // TODO: make this field private, to enforce proto update logic to happen in this class. public proto: dataform.INotebook = dataform.Notebook.create(); + // If true, adds the inline assertions of dependencies as direct dependencies for this action. + public dependOnDependencyAssertions: boolean = false; + constructor( session?: Session, config?: dataform.ActionConfig.NotebookConfig, @@ -35,9 +40,8 @@ export class Notebook extends ActionBuilder { this.proto.target = this.applySessionToTarget(target, config.filename); this.proto.canonicalTarget = this.applySessionCanonicallyToTarget(target); this.proto.tags = config.tags; - this.proto.dependencyTargets = config.dependencyTargets.map(dependencyTarget => - actionConfigToCompiledGraphTarget(dataform.ActionConfig.Target.create(dependencyTarget)) - ); + this.dependOnDependencyAssertions = config.dependOnDependencyAssertions; + this.dependencies(config.dependencyTargets); this.proto.fileName = config.filename; if (config.disabled) { this.proto.disabled = config.disabled; @@ -61,6 +65,15 @@ export class Notebook extends ActionBuilder { return this; } + /** + * @hidden + */ + public dependencies(value: Resolvable | Resolvable[]) { + const newDependencies = Array.isArray(value) ? value : [value]; + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); + return this; + } + /** * @hidden */ diff --git a/core/actions/operation.ts b/core/actions/operation.ts index 65106c7fa..da2507d0c 100644 --- a/core/actions/operation.ts +++ b/core/actions/operation.ts @@ -16,13 +16,14 @@ import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, checkExcessProperties, nativeRequire, resolvableAsTarget, resolveActionsConfigFilename, setNameAndTarget, strictKeysOf, - toResolvable + toResolvable, } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -31,10 +32,10 @@ import { dataform } from "df/protos/ts"; */ export interface IOperationConfig extends IActionConfig, - IDependenciesConfig, - IDocumentableConfig, - INamedConfig, - ITargetableConfig { + IDependenciesConfig, + IDocumentableConfig, + INamedConfig, + ITargetableConfig { /** * Declares that this `operations` action creates a dataset which should be referenceable using the `ref` function. * @@ -59,7 +60,8 @@ export const IIOperationConfigProperties = strictKeysOf()([ "name", "schema", "tags", - "type" + "type", + "dependOnDependencyAssertions" ]); /** @@ -72,6 +74,9 @@ export class Operation extends ActionBuilder { // Hold a reference to the Session instance. public session: Session; + // If true, adds the inline assertions of dependencies as direct dependencies for this action. + public dependOnDependencyAssertions: boolean = false; + // We delay contextification until the final compile step, so hold these here for now. private contextableQueries: Contextable; @@ -105,7 +110,8 @@ export class Operation extends ActionBuilder { tags: config.tags, disabled: config.disabled, hasOutput: config.hasOutput, - description: config.description + description: config.description, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); this.queries(nativeRequire(config.filename).query); @@ -118,6 +124,9 @@ export class Operation extends ActionBuilder { IIOperationConfigProperties, "operation config" ); + if (config.dependOnDependencyAssertions) { + this.setDependOnDependencyAssertions(config.dependOnDependencyAssertions); + } if (config.dependencies) { this.dependencies(config.dependencies); } @@ -155,9 +164,7 @@ export class Operation extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; - newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); - }); + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); return this; } @@ -228,6 +235,11 @@ export class Operation extends ActionBuilder { return this; } + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean) { + this.dependOnDependencyAssertions = dependOnDependencyAssertions; + return this; + } + /** * @hidden */ diff --git a/core/actions/table.ts b/core/actions/table.ts index 537877374..60e2e02c7 100644 --- a/core/actions/table.ts +++ b/core/actions/table.ts @@ -17,6 +17,7 @@ import * as Path from "df/core/path"; import { Session } from "df/core/session"; import { actionConfigToCompiledGraphTarget, + addDependenciesToActionDependencyTargets, checkExcessProperties, nativeRequire, resolvableAsTarget, @@ -25,7 +26,7 @@ import { strictKeysOf, tableTypeStringToEnum, toResolvable, - validateQueryString + validateQueryString, } from "df/core/utils"; import { dataform } from "df/protos/ts"; @@ -157,10 +158,10 @@ const ITableAssertionsProperties = () => */ export interface ITableConfig extends IActionConfig, - IDependenciesConfig, - IDocumentableConfig, - INamedConfig, - ITargetableConfig { + IDependenciesConfig, + IDocumentableConfig, + INamedConfig, + ITargetableConfig { /** * The type of the dataset. For more information on how this setting works, check out some of the [guides](guides) * on publishing different types of datasets with Dataform. @@ -221,7 +222,8 @@ export const ITableConfigProperties = () => "database", "columns", "description", - "materialized" + "materialized", + "dependOnDependencyAssertions" ]); /** @@ -256,6 +258,9 @@ export class Table extends ActionBuilder { // Hold a reference to the Session instance. public session: Session; + // If true, adds the inline assertions of dependencies as direct dependencies for this action. + public dependOnDependencyAssertions: boolean = false; + // We delay contextification until the final compile step, so hold these here for now. public contextableQuery: Contextable; private contextableWhere: Contextable; @@ -340,7 +345,8 @@ export class Table extends ActionBuilder { tags: config.tags, disabled: config.disabled, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } if (tableType === "view") { @@ -370,7 +376,8 @@ export class Table extends ActionBuilder { materialized: config.materialized, tags: config.tags, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } if (tableType === "incremental") { @@ -422,7 +429,8 @@ export class Table extends ActionBuilder { uniqueKey: config.uniqueKey, tags: config.tags, description: config.description, - bigquery: bigqueryOptions + bigquery: bigqueryOptions, + dependOnDependencyAssertions: config.dependOnDependencyAssertions }); } this.query(nativeRequire(tableTypeConfig.filename).query); @@ -444,6 +452,9 @@ export class Table extends ActionBuilder { if (config.type) { this.type(config.type); } + if (config.dependOnDependencyAssertions) { + this.setDependOnDependencyAssertions(config.dependOnDependencyAssertions); + } if (config.dependencies) { this.dependencies(config.dependencies); } @@ -552,10 +563,7 @@ export class Table extends ActionBuilder { public dependencies(value: Resolvable | Resolvable[]) { const newDependencies = Array.isArray(value) ? value : [value]; - newDependencies.forEach(resolvable => { - this.proto.dependencyTargets.push(resolvableAsTarget(resolvable)); - }); - + newDependencies.forEach(resolvable => addDependenciesToActionDependencyTargets(this, resolvable)); return this; } @@ -676,6 +684,11 @@ export class Table extends ActionBuilder { return this; } + public setDependOnDependencyAssertions(dependOnDependencyAssertions: boolean) { + this.dependOnDependencyAssertions = dependOnDependencyAssertions; + return this; + } + /** * @hidden */ @@ -740,7 +753,7 @@ export class Table extends ActionBuilder { * @hidden */ export class TableContext implements ITableContext { - constructor(private table: Table, private isIncremental = false) {} + constructor(private table: Table, private isIncremental = false) { } public config(config: ITableConfig) { this.table.config(config); diff --git a/core/common.ts b/core/common.ts index 106075be9..645cfcff2 100644 --- a/core/common.ts +++ b/core/common.ts @@ -133,6 +133,11 @@ export interface IDependenciesConfig { * dependencies, then it should be set to `true`. */ hermetic?: boolean; + + /** + * If this flag is set to true, assertions depenedent upon any of the dependencies are added as depenedencies as well. + */ + dependOnDependencyAssertions?: boolean; } /** @@ -219,6 +224,8 @@ export interface ITarget { schema?: string; name?: string; + + includeDependentAssertions?: boolean; } /** diff --git a/core/main_test.ts b/core/main_test.ts index 1f5fe7049..3d11c97b2 100644 --- a/core/main_test.ts +++ b/core/main_test.ts @@ -112,7 +112,7 @@ suite("@dataform/core", ({ afterEach }) => { ].forEach(testConfig => { test( `assertions target context functions with project suffix '${testConfig.projectSuffix}', ` + - `dataset suffix '${testConfig.datasetSuffix}', and name prefix '${testConfig.namePrefix}'`, + `dataset suffix '${testConfig.datasetSuffix}', and name prefix '${testConfig.namePrefix}'`, () => { const projectDir = tmpDirFixture.createNewTmpDir(); fs.writeFileSync( @@ -132,8 +132,8 @@ suite("@dataform/core", ({ afterEach }) => { ).deep.equals([]); expect(asPlainObject(result.compile.compiledGraph.assertions[0].query)).deep.equals( `default-database${testConfig.projectSuffix ? `_suffix` : ""}.` + - `schema${testConfig.datasetSuffix ? `_suffix` : ""}.` + - `${testConfig.namePrefix ? `prefix_` : ""}name` + `schema${testConfig.datasetSuffix ? `_suffix` : ""}.` + + `${testConfig.namePrefix ? `prefix_` : ""}name` ); } ); @@ -1260,6 +1260,404 @@ actions: expect(result.compile.compiledGraph.operations[0].fileName).deep.equals("table.sql"); }); }); + + suite("Assertions as dependencies", ({ beforeEach }) => { + [ + TestConfigs.bigquery, + TestConfigs.bigqueryWithDatasetSuffix, + TestConfigs.bigqueryWithNamePrefix + ].forEach(testConfig => { + let projectDir: any; + beforeEach("Create temporary dir and files", () => { + projectDir = tmpDirFixture.createNewTmpDir(); + fs.writeFileSync( + path.join(projectDir, "workflow_settings.yaml"), + dumpYaml(dataform.WorkflowSettings.create(testConfig)) + ); + fs.mkdirSync(path.join(projectDir, "definitions")); + fs.writeFileSync(path.join(projectDir, "definitions/A.sqlx"), + ` +config { + type: "table", + assertions: {rowConditions: ["test > 1"]}} + SELECT 1 as test`); + fs.writeFileSync(path.join(projectDir, "definitions/A_assert.sqlx"), + ` +config { + type: "assertion", +} +select test from \${ref("A")} where test > 3`); + fs.writeFileSync(path.join(projectDir, "definitions/B.sql"), "SELECT 1"); + fs.writeFileSync(path.join(projectDir, "definitions/C.sql"), "SELECT 1"); + fs.writeFileSync( + path.join(projectDir, `definitions/notebook.ipynb`), + EMPTY_NOTEBOOK_CONTENTS + ); + + }); + + test("When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] +} +select 1 as btest +`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + ]) + }) + + test("Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: true}, "C"] +} +select 1 as btest`); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + ` +config { +type: "table", + assertions: { + rowConditions: ["test > 1"] +} +} +SELECT 1 as test +}`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "C"), + ]) + }) + + test("Setting includeDependentAssertions to true in ref, adds assertions from that dependency to dependencyTargets", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: true})} +select 1 as btest`); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "C"), + prefixAdjustedName(testConfig.namePrefix, "schema_C_assertions_rowConditions"), + ]) + }) + + test("When dependOnDependencyAssertions=true and includeDependentAssertions=false, the assertions related to dependency should not be added to dependencyTargets", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependOnDependencyAssertions: true, + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: false})} +select 1 as btest`); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(4) + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "C"), + ]); + }) + + test("When dependOnDependencyAssertions=false and includeDependentAssertions=true, the assertions related to dependency should be added to dependencyTargets", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "operations", + dependOnDependencyAssertions: false, + dependencies: ["A"] +} +select * from \${ref({name: "C", includeDependentAssertions: true})} +select 1 as btest`); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "C"), + prefixAdjustedName(testConfig.namePrefix, "schema_C_assertions_rowConditions"), + ]); + }) + + test("Assertions added through includeDependentAssertions and explicitly listed in dependencies are deduplicated.", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependencies: ["A_assert"] +} +select * from \${ref({name: "A", includeDependentAssertions: true})} +select 1 as btest`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.flatMap(dependencyTarget => dependencyTarget.name))).deep.equals([ + prefixAdjustedName(testConfig.namePrefix, "A_assert"), + prefixAdjustedName(testConfig.namePrefix, "A"), + prefixAdjustedName(testConfig.namePrefix, "schema_A_assertions_rowConditions"), + ]); + }) + + test("When includeDependentAssertions property in config and ref are set differently for the same dependency, compilation error is thrown.", () => { + fs.writeFileSync(path.join(projectDir, "definitions/B.sqlx"), + ` +config { + type: "table", + dependencies: [{name: "A", includeDependentAssertions: false}, {name: "C", includeDependentAssertions: true}] +} +select * from \${ref({name: "A", includeDependentAssertions: true})} +select * from \${ref({name: "C", includeDependentAssertions: false})} +select 1 as btest`); + fs.writeFileSync(path.join(projectDir, "definitions/C.sqlx"), + ` +config { + type: "table", + assertions: { + rowConditions: ["test > 1"] + } +} +SELECT 1 as test +}`); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(2); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" properties are not allowed. Dependency A has different values set for this property.`); + }) + + suite("Action configs", () => { + test(`When dependOnDependencyAssertions property is set to true, assertions from A are added as dependencies`, () => { + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` +actions: +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +` + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`Setting includeDependentAssertions to true in config.dependencies adds assertions from that dependency to dependencyTargets`, () => { + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` +actions: +- view: + filename: B.sql + dependencyTargets: + - name: A + includeDependentAssertions: true +- operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true +- notebook: + filename: notebook.ipynb + dependencyTargets: + - name: A + includeDependentAssertions: true +` + ); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`When dependOnDependencyAssertions=true and includeDependentAssertions=false, the assertions related to dependency should not be added to dependencyTargets`, () => { + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` +actions: +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false +- assertion: + filename: B_assert.sql + dependencyTargets: + - name: B +- operation: + filename: C.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: true + dependencyTargets: + - name: A + includeDependentAssertions: false + - name: B +` + ); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(1); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(3); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`When dependOnDependencyAssertions=false and includeDependentAssertions=true, the assertions related to dependency should be added to dependencyTargets`, () => { + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` +actions: +- view: + filename: B.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true +- assertion: + filename: B_assert.sql + dependencyTargets: + - name: B +- operation: + filename: C.sql + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B +- notebook: + filename: notebook.ipynb + dependOnDependencyAssertions: false + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B +` + ); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(asPlainObject(result.compile.compiledGraph.operations.find(operation => operation.target.name === prefixAdjustedName(testConfig.namePrefix, "C")).dependencyTargets.length)).deep.equals(4); + expect(asPlainObject(result.compile.compiledGraph.tables.find(table => table.target.name === prefixAdjustedName(testConfig.namePrefix, "B")).dependencyTargets.length)).deep.equals(3); + expect(asPlainObject(result.compile.compiledGraph.notebooks.find(notebook => notebook.target.name === prefixAdjustedName(testConfig.namePrefix, "notebook")).dependencyTargets.length)).deep.equals(4); + expect(result.compile.compiledGraph.graphErrors.compilationErrors).deep.equals([]); + }); + + test(`When includeDependentAssertions property in config and ref are set differently for the same dependency, compilation error is thrown.`, () => { + fs.writeFileSync( + path.join(projectDir, "definitions/actions.yaml"), + ` +actions: +- view: + filename: B.sql + dependOnDependencyAssertions: true + dependencyTargets: + - name: A +- operation: + filename: C.sql + dependencyTargets: + - name: A + includeDependentAssertions: true + - name: B + - name: A + includeDependentAssertions: false +` + ); + fs.writeFileSync(path.join(projectDir, "definitions/B_assert.sql"), "SELECT test from B"); + + const result = runMainInVm(coreExecutionRequestFromPath(projectDir)); + + expect(result.compile.compiledGraph.graphErrors.compilationErrors.length).deep.equals(1); + expect(result.compile.compiledGraph.graphErrors.compilationErrors[0].message).deep.equals(`Conflicting "includeDependentAssertions" properties are not allowed. Dependency A has different values set for this property.`); + }); + }); + }) + }); }); function coreExecutionRequestFromPath(projectDir: string): dataform.CoreExecutionRequest { @@ -1321,3 +1719,7 @@ function walkDirectoryForFilenames(projectDir: string, relativePath: string = "" }); return paths.map(filename => path.join(relativePath, filename)); } + +function prefixAdjustedName(prefix: string | undefined, name: string) { + return prefix ? `${prefix}_${name}` : name +} diff --git a/core/session.ts b/core/session.ts index 06d9be58f..96c4d8de8 100644 --- a/core/session.ts +++ b/core/session.ts @@ -9,7 +9,7 @@ import { Notebook } from "df/core/actions/notebook"; import { Operation, OperationContext } from "df/core/actions/operation"; import { ITableConfig, ITableContext, Table, TableContext, TableType } from "df/core/actions/table"; import { Test } from "df/core/actions/test"; -import { Contextable, ICommonContext, Resolvable } from "df/core/common"; +import { Contextable, ICommonContext, ITarget, Resolvable } from "df/core/common"; import { CompilationSql } from "df/core/compilation_sql"; import { targetAsReadableString, targetStringifier } from "df/core/targets"; import * as utils from "df/core/utils"; @@ -47,9 +47,13 @@ export class Session { public canonicalConfig: dataform.IProjectConfig; public actions: Action[]; - public indexedActions: ActionIndex; + public indexedActions: ActionMap; public tests: { [name: string]: Test }; + // This map holds information about what assertions are dependent + // upon a certain action in our actions list. We use this later to resolve dependencies. + public actionAssertionMap = new ActionMap([]); + public graphErrors: dataform.IGraphErrors; constructor( @@ -321,7 +325,7 @@ export class Session { } public compile(): dataform.CompiledGraph { - this.indexedActions = new ActionIndex(this.actions); + this.indexedActions = new ActionMap(this.actions); if ( (this.config.warehouse === "bigquery" || this.config.warehouse === "") && @@ -461,6 +465,12 @@ export class Session { // We found a single matching target, and fully-qualify it if it's a normal dependency. const protoDep = possibleDeps[0].proto; fullyQualifiedDependencies[targetAsReadableString(protoDep.target)] = protoDep.target; + + if (dependency.includeDependentAssertions) { + this.actionAssertionMap.find(dependency).forEach(assertion => + fullyQualifiedDependencies[targetAsReadableString(assertion.proto.target)] = assertion.proto.target + ); + } } else { // Too many targets matched the dependency. this.compileError( @@ -737,53 +747,41 @@ function objectExistsOrIsNonEmpty(prop: any): boolean { ); } -class ActionIndex { - private readonly byName: Map = new Map(); - private readonly bySchemaAndName: Map> = new Map(); - private readonly byDatabaseAndName: Map> = new Map(); - private readonly byDatabaseSchemaAndName: Map< +class ActionMap { + private byName: Map = new Map(); + private bySchemaAndName: Map> = new Map(); + private byDatabaseAndName: Map> = new Map(); + private byDatabaseSchemaAndName: Map< string, Map> > = new Map(); public constructor(actions: Action[]) { for (const action of actions) { - if (!this.byName.has(action.proto.target.name)) { - this.byName.set(action.proto.target.name, []); - } - this.byName.get(action.proto.target.name).push(action); + this.set(action.proto.target, action) + } + } - if (!this.bySchemaAndName.has(action.proto.target.schema)) { - this.bySchemaAndName.set(action.proto.target.schema, new Map()); - } - const forSchema = this.bySchemaAndName.get(action.proto.target.schema); - if (!forSchema.has(action.proto.target.name)) { - forSchema.set(action.proto.target.name, []); - } - forSchema.get(action.proto.target.name).push(action); + public set(actionTarget: ITarget, assertionTarget: Action) { + this.setByNameLevel(this.byName, actionTarget.name, assertionTarget); - if (!!action.proto.target.database) { - if (!this.byDatabaseAndName.has(action.proto.target.database)) { - this.byDatabaseAndName.set(action.proto.target.database, new Map()); - } - const forDatabaseNoSchema = this.byDatabaseAndName.get(action.proto.target.database); - if (!forDatabaseNoSchema.has(action.proto.target.name)) { - forDatabaseNoSchema.set(action.proto.target.name, []); - } - forDatabaseNoSchema.get(action.proto.target.name).push(action); + if (!!actionTarget.schema) { + this.setBySchemaLevel(this.bySchemaAndName, actionTarget, assertionTarget); + } - if (!this.byDatabaseSchemaAndName.has(action.proto.target.database)) { - this.byDatabaseSchemaAndName.set(action.proto.target.database, new Map()); - } - const forDatabase = this.byDatabaseSchemaAndName.get(action.proto.target.database); - if (!forDatabase.has(action.proto.target.schema)) { - forDatabase.set(action.proto.target.schema, new Map()); - } - const forDatabaseAndSchema = forDatabase.get(action.proto.target.schema); - if (!forDatabaseAndSchema.has(action.proto.target.name)) { - forDatabaseAndSchema.set(action.proto.target.name, []); + if (!!actionTarget.database) { + if (!this.byDatabaseAndName.has(actionTarget.database)) { + this.byDatabaseAndName.set(actionTarget.database, new Map()); + } + const forDatabaseNoSchema = this.byDatabaseAndName.get(actionTarget.database); + this.setByNameLevel(forDatabaseNoSchema, actionTarget.name, assertionTarget) + + if (!!actionTarget.schema) { + if (!this.byDatabaseSchemaAndName.has(actionTarget.database)) { + this.byDatabaseSchemaAndName.set(actionTarget.database, new Map()); } - forDatabaseAndSchema.get(action.proto.target.name).push(action); + const forDatabase = this.byDatabaseSchemaAndName.get(actionTarget.database); + this.setBySchemaLevel(forDatabase, actionTarget, assertionTarget); } } } @@ -805,4 +803,19 @@ class ActionIndex { } return this.byName.get(target.name) || []; } + + private setByNameLevel(targetMap: Map, name: string, assertionTarget: Action) { + if (!targetMap.has(name)) { + targetMap.set(name, []); + } + targetMap.get(name).push(assertionTarget); + } + + private setBySchemaLevel(targetMap: Map>, actionTarget: ITarget, assertionTarget: Action) { + if (!targetMap.has(actionTarget.schema)) { + targetMap.set(actionTarget.schema, new Map()); + } + const forSchema = targetMap.get(actionTarget.schema); + this.setByNameLevel(forSchema, actionTarget.name, assertionTarget); + } } diff --git a/core/utils.ts b/core/utils.ts index 18a507043..f8b68616c 100644 --- a/core/utils.ts +++ b/core/utils.ts @@ -1,5 +1,6 @@ import { Action } from "df/core/actions"; import { Assertion } from "df/core/actions/assertion"; +import { Notebook } from "df/core/actions/notebook"; import { Operation } from "df/core/actions/operation"; import { Table } from "df/core/actions/table"; import { Resolvable } from "df/core/common"; @@ -10,6 +11,8 @@ import { dataform } from "df/protos/ts"; declare var __webpack_require__: any; declare var __non_webpack_require__: any; +type actionsWithDependencies = Table | Operation | Notebook + // This side-steps webpack's require in favour of the real require. export const nativeRequire = typeof __webpack_require__ === "function" ? __non_webpack_require__ : require; @@ -134,8 +137,8 @@ export function ambiguousActionNameMsg(act: Resolvable, allActs: Action[] | stri typeof allActs[0] === "string" ? allActs : (allActs as Array).map( - r => `${r.proto.target.schema}.${r.proto.target.name}` - ); + r => `${r.proto.target.schema}.${r.proto.target.name}` + ); return `Ambiguous Action name: ${stringifyResolvable( act )}. Did you mean one of: ${allActNames.join(", ")}.`; @@ -216,8 +219,7 @@ export function checkExcessProperties( if (extraProperties.length > 0) { reportError( new Error( - `Unexpected property "${extraProperties[0]}"${ - !!name ? ` in ${name}` : "" + `Unexpected property "${extraProperties[0]}"${!!name ? ` in ${name}` : "" }. Supported properties are: ${JSON.stringify(supportedProperties)}` ) ); @@ -293,9 +295,36 @@ export function actionConfigToCompiledGraphTarget( if (actionConfigTarget.project) { compiledGraphTarget.database = actionConfigTarget.project; } + if (actionConfigTarget.hasOwnProperty("includeDependentAssertions")) { + compiledGraphTarget.includeDependentAssertions = actionConfigTarget.includeDependentAssertions; + } return dataform.Target.create(compiledGraphTarget); } export function resolveActionsConfigFilename(configFilename: string, configPath: string) { return Path.normalize(Path.join(Path.dirName(configPath), configFilename)); } + +export function addDependenciesToActionDependencyTargets(action: actionsWithDependencies, resolvable: Resolvable) { + const dependencyTarget = resolvableAsTarget(resolvable); + if (!dependencyTarget.hasOwnProperty("includeDependentAssertions")) { + // dependency `includeDependentAssertions` takes precedence over the config's `dependOnDependencyAssertions` + dependencyTarget.includeDependentAssertions = action.dependOnDependencyAssertions; + } + + // check if same dependency already exist in this action but with opposite value for includeDependentAssertions + const dependencyTargetString = action.session.compilationSql().resolveTarget(dependencyTarget) + + if (action.includeAssertionsForDependency.has(dependencyTargetString)) { + if (action.includeAssertionsForDependency.get(dependencyTargetString) !== dependencyTarget.includeDependentAssertions) { + action.session.compileError( + `Conflicting "includeDependentAssertions" properties are not allowed. Dependency ${dependencyTarget.name} has different values set for this property.`, + action.proto.fileName, + action.proto.target + ) + return action; + } + } + action.proto.dependencyTargets.push(dependencyTarget); + action.includeAssertionsForDependency.set(dependencyTargetString, dependencyTarget.includeDependentAssertions) +} diff --git a/protos/configs.proto b/protos/configs.proto index 5b1b870fa..723917a16 100644 --- a/protos/configs.proto +++ b/protos/configs.proto @@ -63,6 +63,9 @@ message ActionConfig { // The name of the action. string name = 4; + + // flag for when we want to add assertions of this dependency in dependency_targets as well + bool include_dependent_assertions = 5; } message ColumnDescriptor { @@ -148,6 +151,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 17; + + // When set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool depend_on_dependency_assertions = 18; } message ViewConfig { @@ -208,6 +215,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 14; + + // When set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool depend_on_dependency_assertions = 15; } message IncrementalTableConfig { @@ -296,6 +307,10 @@ message ActionConfig { // If the option name contains special characters, encapsulate the name in // quotes, for example: additionalOptions: { "option-name": "value" }. map additional_options = 20; + + // When set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool depend_on_dependency_assertions = 21; } message AssertionConfig { @@ -361,6 +376,10 @@ message ActionConfig { // Descriptions of columns within the operation. Can only be set if // hasOutput is true. repeated ColumnDescriptor columns = 10; + + // When set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool depend_on_dependency_assertions = 11; } message DeclarationConfig { @@ -407,6 +426,10 @@ message ActionConfig { // Description of the notebook. string description = 8; + // When set to true, assertions dependent upon any dependency will + // be add as dedpendency to this action + bool depend_on_dependency_assertions = 9; + // TODO(ekrekr): add a notebook runtime field definition. } diff --git a/protos/core.proto b/protos/core.proto index 46ff9cbce..65ed933b9 100644 --- a/protos/core.proto +++ b/protos/core.proto @@ -48,6 +48,7 @@ message Target { string database = 3; string schema = 1; string name = 2; + bool include_dependent_assertions = 4; } message BigQueryOptions { diff --git a/tests/api/projects.spec.ts b/tests/api/projects.spec.ts index 7968a06fa..0ca99ae7e 100644 --- a/tests/api/projects.spec.ts +++ b/tests/api/projects.spec.ts @@ -35,17 +35,17 @@ suite("examples", () => { { fileName: "definitions/has_compile_errors/assertion_with_bigquery.sqlx", message: - 'Unexpected property "bigquery" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "bigquery" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_materialized.sqlx", message: - 'Unexpected property "materialized" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "materialized" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_output.sqlx", message: - 'Unexpected property "hasOutput" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "hasOutput" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/assertion_with_postops.sqlx", @@ -63,7 +63,7 @@ suite("examples", () => { { fileName: "definitions/has_compile_errors/protected_assertion.sqlx", message: - 'Unexpected property "protected" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type"]' + 'Unexpected property "protected" in assertion config. Supported properties are: ["database","dependencies","description","disabled","hermetic","name","schema","tags","type","dependOnDependencyAssertions"]' }, { fileName: "definitions/has_compile_errors/view_with_incremental.sqlx", diff --git a/tests/core/core.spec.ts b/tests/core/core.spec.ts index fc0d5c17f..bc3990774 100644 --- a/tests/core/core.spec.ts +++ b/tests/core/core.spec.ts @@ -901,7 +901,7 @@ suite("@dataform/core", () => { expect( cGraph.graphErrors.compilationErrors.filter(item => item.message.match( - /Ambiguous Action name: {\"name\":\"a\"}. Did you mean one of: foo.a, bar.a./ + /Ambiguous Action name: {\"name\":\"a\",\"includeDependentAssertions\":false}. Did you mean one of: foo.a, bar.a./ ) ).length ).greaterThan(0); diff --git a/version.bzl b/version.bzl index e65a35d23..c6080b9de 100644 --- a/version.bzl +++ b/version.bzl @@ -1,3 +1,3 @@ # NOTE: If you change the format of this line, you must change the bash command # in /scripts/publish to extract the version string correctly. -DF_VERSION = "3.0.0-beta.4" +DF_VERSION = "3.0.0-beta.5"