Allow arbitrary JSON in google.protobuf.Struct fields during validation by SkyrimL · Pull Request #2119 · dataform-co/dataform

SkyrimL · 2026-03-19T22:32:18Z

The current verifyObjectMatchesProto helper is very strict. When users provide arbitrary JSON metadata in a Struct field (like extra_properties), the validator throws an "unexpected property" error because those custom keys are not explicitly defined in the protobuf message.

This PR introduces a manual conversion from plain JavaScript objects to the standard google.protobuf.Struct format during the validation phase.

kolina

Let's add compilation test cases

common/protos/index.ts

…ion.ts

kolina

Some tests seem to be failing?

core/main_test.ts

core/actions/table.ts

kolina · 2026-03-21T01:48:07Z

core/actions/table.ts

      return;
    }
-    const config = this.verifyConfig(unverifiedConfig);
+    if (this.unverifiedConfig.metadata?.extraProperties) {


Users of JavaScript API may theoretically pass a protobuf object directly: see here.

I think, there should be a check that if extraProperties is already an instance of protobuf struct object, that this conversion is skipped.

Can we add a test for this case?

ikholopov-omni · 2026-03-22T15:37:11Z

common/protos/index.ts

+export function jsonToValue(value: any): any {
+  if (value === null || typeof value === "undefined") {
+    return { nullValue: 0 };
+  } else if (typeof value === "number") {


Else statement are redundant in these chains.

ikholopov-omni · 2026-03-22T15:37:51Z

common/protos/index.ts

+  return { fields };
+}
+
+export function jsonToValue(value: any): any {


Please don't omit the returned type

ikholopov-omni · 2026-03-22T15:38:14Z

common/protos/index.ts

  return buf;
 }
+
+export function jsonToStruct(obj: any): any {


Please don't omit the return type

ikholopov-omni · 2026-03-22T15:40:23Z

core/session.ts

    }

-    this.jitContextData.fields[key] = unknownToValue(data);
+    this.jitContextData.fields[key] = google.protobuf.Value.create(jsonToValue(data));


We don't require JiT data to be a JSON object - it can be any JS object. Please consider using a better name for a conversion function.

…and session.ts" This reverts commit 6860230.

…he same logic

ikholopov-omni · 2026-03-24T19:13:17Z

common/protos/index.ts

  return buf;
 }
+
+export interface IValue {


Why do we need this interface? google.protobuf.IValue should already cover this

…e new interface.

rahulmadanahalli · 2026-03-26T00:23:54Z

common/protos/index.ts

 const CONFIGS_PROTO_DOCUMENTATION_URL =
  "https://dataform-co.github.io/dataform/docs/configs-reference";
 const REPORT_ISSUE_URL = "https://github.com/dataform-co/dataform/issues";
+const STRUCT_FIELD_NAMES = new Set(["extraProperties", "contexts", "glossary_terms"]);


we don't want to parametrize this set on all the possible keys that we can store inside extraProperties - we'd need to update this open source project every time we want to support another key. Let's generalize it so we don't need to store glossary_terms or contacts here.

Why do we have other fields except extraProperties here?

rahulmadanahalli · 2026-03-26T00:29:41Z

core/actions/table_test.ts

+          glossary_terms: [
+            {
+              column_name: "trip_id",
+              project: "bq-dataworkeragent-test",


let's not use the name of our internal project here - rename this to project_identifier or something else

rahulmadanahalli · 2026-03-26T00:30:30Z

core/actions/table_test.ts

+            }
+          ],
+          generic: {
+            system: "yo",


let's replace these with better values (ie. "my custom system value"

kolina · 2026-03-28T01:21:41Z

common/protos/index.ts

 const CONFIGS_PROTO_DOCUMENTATION_URL =
  "https://dataform-co.github.io/dataform/docs/configs-reference";
 const REPORT_ISSUE_URL = "https://github.com/dataform-co/dataform/issues";
+const STRUCT_FIELD_NAMES = new Set(["extraProperties", "contexts", "glossary_terms"]);


Why do we have other fields except extraProperties here?

kolina · 2026-03-28T01:29:50Z

common/protos/index.ts

+    return {
+      structValue: {
+        fields: Object.fromEntries(
+          Object.entries(raw as object).map(([key, value]) => [key, unknownToValue(value)])


is cast raw as object needed here?

kolina · 2026-03-28T01:32:03Z

common/protos/index.ts

+      if (structFieldNames.has(presentKey)) {
+        return;
+      }


why is this check needed here?

kolina · 2026-03-28T13:00:43Z

common/protos/index.ts

+      if (Array.isArray(value)) {
+        result[key] = {
+          listValue: {
+            values: value.map(item => unknownToValue(item))
+          }
+        };
+      } else if (typeof value === "object" && !value.fields) {
+        const converted = unknownToValue(value);
+        result[key] = converted.structValue;
+      }


This logic is a bit unclear to me, I think you only need a check for top-level field that is always a struct at the moment?

kolina · 2026-03-28T13:17:38Z

core/actions/table.ts

      return;
    }
-    const config = this.verifyConfig(unverifiedConfig);
+    if (this.unverifiedConfig.metadata?.extraProperties) {


Can we add a test for this case?

Allow arbitrary JSON in google.protobuf.Struct fields during validation

6606cea

SkyrimL requested a review from a team as a code owner March 19, 2026 22:32

SkyrimL requested review from andrzej-grudzien, fernst, kolina and rahulmadanahalli and removed request for a team March 19, 2026 22:32

kolina requested changes Mar 20, 2026

View reviewed changes

common/protos/index.ts Outdated Show resolved Hide resolved

Add compilation test cases. Reuse the same logic in index.ts and sess…

6860230

…ion.ts

SkyrimL requested a review from ikholopov-omni March 20, 2026 21:03

kolina requested changes Mar 21, 2026

View reviewed changes

ikholopov-omni requested changes Mar 22, 2026

View reviewed changes

SkyrimL added 3 commits March 23, 2026 22:27

Revert "Add compilation test cases. Reuse the same logic in index.ts …

6660b33

…and session.ts" This reverts commit 6860230.

move the unknownToValue logic from session.ts to index.ts and reuse t…

dc99d86

…he same logic

Add compilation test. Add return type.

09d4ab4

ikholopov-omni requested changes Mar 24, 2026

View reviewed changes

SkyrimL added 7 commits March 25, 2026 00:39

fix the test error. Using google.protobuf.IValue instead of create th…

17fccb4

…e new interface.

change the import format

42207b1

add the compilation test

95670db

add recursive logic

7a5a366

add a complex test

41aa331

fix a lint bug

123b3a0

fix the test fail

c7ab5ce

rahulmadanahalli reviewed Mar 26, 2026

View reviewed changes

kolina reviewed Mar 28, 2026

View reviewed changes

Conversation

SkyrimL commented Mar 19, 2026

Uh oh!

kolina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kolina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants