Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate workflow settings fields #1581

Merged
merged 6 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions common/protos/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i hate this, let's please switch

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some digging into this, and the alternatives which can validate (https://www.npmjs.com/package/@protobuf-ts/plugin, https://www.npmjs.com/package/protoc-gen-ts) are significantly slower. More details in b/305915462#comment18.

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
Expand Down
38 changes: 35 additions & 3 deletions core/main_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`, () => {
Expand All @@ -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.
});
});

Expand Down
21 changes: 14 additions & 7 deletions core/workflow_settings.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { verifyObjectMatchesProto } from "df/common/protos";
import { dataform } from "df/protos/ts";

export function readWorkflowSettings(): dataform.ProjectConfig {
Expand All @@ -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;
}
}

Expand Down