Validate workflow settings fields (#1581)

* Add workflow settings validation * Tidy * Remove return from recursive field check * Fix object lint error * Remove braking run cache option from CLI * Remove console log
dataform-co · Nov 22, 2023 · 2a8f584 · 2a8f584
1 parent 14f6562
commit 2a8f584
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 14 deletions.
diff --git a/cli/index.ts b/cli/index.ts
@@ -302,8 +302,7 @@ export function runCli() {
             {
               warehouse: argv[warehouseOption.name],
               defaultDatabase: argv[defaultDatabaseOptionName],
-              defaultLocation: argv[defaultLocationOptionName],
-              useRunCache: false
+              defaultLocation: argv[defaultLocationOptionName]
             },
             {
               skipInstall: argv[skipInstallOptionName]

diff --git a/common/protos/index.ts b/common/protos/index.ts
@@ -14,6 +14,33 @@ export interface IProtoClass<IProto, Proto> {
   fromObject(obj: { [k: string]: any }): Proto;
 }
 
+// ProtobufJS's native verify method does not check that only defined fields are present.
+// TODO(ekrekr): swap to Typescript protobuf library rather than having to do this.
+export function verifyObjectMatchesProto<Proto>(
+  protoType: IProtoClass<any, Proto>,
+  object: object
+) {
+  // Create a version of the object that only contains the valid proto fields.
+  // ProtobufJS doesn't care about the types of the values of fields.
+  const protoCastObject = protoType.toObject(protoType.create(object));
+
+  function checkFields(present: { [k: string]: any }, desired: { [k: string]: any }) {
+    // Present is guaranteed to be a strict subset of desired.
+    // Present fields cannot have empty values.
+    Object.entries(present).forEach(([presentKey, presentValue]) => {
+      const desiredValue = desired[presentKey];
+      if (!desiredValue) {
+        throw ReferenceError(`unexpected key '${presentKey}'`);
+      }
+      if (typeof presentValue === "object") {
+        checkFields(presentValue, desiredValue);
+      }
+    });
+  }
+
+  checkFields(object, protoCastObject);
+}
+
 export function encode64<IProto, Proto>(
   protoType: IProtoClass<IProto, Proto>,
   value: IProto | Proto = {} as IProto

diff --git a/core/main_test.ts b/core/main_test.ts
@@ -103,9 +103,7 @@ suite("@dataform/core", ({ afterEach }) => {
         compile: { compileConfig: { projectDir } }
       });
 
-      expect(() => runMainInVm(coreExecutionRequest)).to.throw(
-        "workflow_settings.yaml contains invalid fields"
-      );
+      expect(() => runMainInVm(coreExecutionRequest)).to.throw("workflow_settings.yaml is invalid");
     });
 
     test(`main fails when dataform.json is an invalid json file`, () => {
@@ -120,6 +118,40 @@ suite("@dataform/core", ({ afterEach }) => {
         "Unexpected token k in JSON at position 1"
       );
     });
+
+    test(`main fails when a valid workflow_settings.yaml contains unknown fields`, () => {
+      const projectDir = tmpDirFixture.createNewTmpDir();
+      // tslint:disable-next-line: tsr-detect-non-literal-fs-filename
+      fs.writeFileSync(
+        path.join(projectDir, "workflow_settings.yaml"),
+        "notAProjectConfigField: value"
+      );
+      const coreExecutionRequest = dataform.CoreExecutionRequest.create({
+        compile: { compileConfig: { projectDir } }
+      });
+
+      expect(() => runMainInVm(coreExecutionRequest)).to.throw(
+        "Workflow settings error: unexpected key 'notAProjectConfigField'"
+      );
+    });
+
+    test(`main fails when a valid dataform.json contains unknown fields`, () => {
+      const projectDir = tmpDirFixture.createNewTmpDir();
+      // tslint:disable-next-line: tsr-detect-non-literal-fs-filename
+      fs.writeFileSync(
+        path.join(projectDir, "dataform.json"),
+        `{"notAProjectConfigField": "value"}`
+      );
+      const coreExecutionRequest = dataform.CoreExecutionRequest.create({
+        compile: { compileConfig: { projectDir } }
+      });
+
+      expect(() => runMainInVm(coreExecutionRequest)).to.throw(
+        "Workflow settings error: unexpected key 'notAProjectConfigField'"
+      );
+    });
+
+    // TODO(ekrekr): add a test for nested fields, once they exist.
   });
 });
 

diff --git a/core/workflow_settings.ts b/core/workflow_settings.ts
@@ -1,3 +1,4 @@
+import { verifyObjectMatchesProto } from "df/common/protos";
 import { dataform } from "df/protos/ts";
 
 export function readWorkflowSettings(): dataform.ProjectConfig {
@@ -13,23 +14,29 @@ export function readWorkflowSettings(): dataform.ProjectConfig {
 
   if (workflowSettingsYaml) {
     const workflowSettingsAsJson = workflowSettingsYaml.asJson();
+    if (!workflowSettingsAsJson) {
+      throw Error("workflow_settings.yaml is invalid");
+    }
     verifyWorkflowSettingsAsJson(workflowSettingsAsJson);
-    return dataform.ProjectConfig.create(workflowSettingsAsJson);
+    return dataform.ProjectConfig.fromObject(workflowSettingsAsJson);
   }
 
   if (dataformJson) {
     verifyWorkflowSettingsAsJson(dataformJson);
-    return dataform.ProjectConfig.create(dataformJson);
+    return dataform.ProjectConfig.fromObject(dataformJson);
   }
 
   throw Error("Failed to resolve workflow_settings.yaml");
 }
 
-function verifyWorkflowSettingsAsJson(workflowSettingsAsJson?: object) {
-  // TODO(ekrekr): Implement a protobuf field validator. Protobufjs's verify method is not fit for
-  // purpose.
-  if (!workflowSettingsAsJson) {
-    throw Error("workflow_settings.yaml contains invalid fields");
+function verifyWorkflowSettingsAsJson(workflowSettingsAsJson: object) {
+  try {
+    verifyObjectMatchesProto(dataform.ProjectConfig, workflowSettingsAsJson);
+  } catch (e) {
+    if (e instanceof ReferenceError) {
+      throw ReferenceError(`Workflow settings error: ${e.message}`);
+    }
+    throw e;
   }
 }
 

diff --git a/tests/cli/cli.spec.ts b/tests/cli/cli.spec.ts
@@ -95,7 +95,6 @@ select 1 as \${dataform.projectConfig.vars.testVar2}
         assertionSchema: "dataform_assertions",
         defaultDatabase: "dataform-integration-tests",
         defaultLocation: "US",
-        useRunCache: false,
         vars: {
           testVar1: "testValue1",
           testVar2: "testValue2"
@@ -154,7 +153,6 @@ select 1 as \${dataform.projectConfig.vars.testVar2}
         defaultDatabase: "dataform-integration-tests",
         defaultLocation: "US",
         defaultSchema: "dataform",
-        useRunCache: false,
         warehouse: "bigquery",
         vars: {
           testVar1: "testValue1",