This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-6443-998899f02f6aa8ce0cc65a286d36f8ee686c53a2 in repository https://gitbox.apache.org/repos/asf/texera.git
commit a4e03e441d1a50feb51b8a6845da0691bbcfee72 Author: Eugene Gu <[email protected]> AuthorDate: Tue Jul 21 19:38:01 2026 -0700 fix(frontend): reject null payloads in dashboard type predicates (#6443) ### What changes were proposed in this PR? The five type guards in `dashboard/type/type-predicates.ts` checked their nested payload with `typeof value.<field> === "object"`, which also accepts null because `typeof null === "object"` in JavaScript, so an entry like `{workflow: null}` passed `isDashboardWorkflow` and the `DashboardEntry` constructor then crashed with an unrelated `TypeError: Cannot read properties of null` while dereferencing the payload, instead of reaching its intentional `"Unexpected type in DashboardEntry."` error path. The guards also returned the falsy input itself rather than `false` for null/undefined input, violating their declared boolean type-guard signatures. This PR adds an `isNonNullObject` type guard (`typeof x === "object" && x !== null`) to the shared predicate utility (`common/util/predicate.ts`, next to `isDefined`) so the correct non-null object check is discoverable and reusable, uses it in the four object-payload guards, and switches all five guards to `!!value && ...` so they return strict booleans in every path. `isDashboardProject`'s `!value.workflow` exclusion is deliberately unchanged: a null `workflow` field means "no workflow data", so an object with a string `name` still classifies as a project and no valid entry changes classification. ### Any related issues, documentation, discussions? Fixes #6439. The buggy behavior was pinned by the five "(current behavior)" tests added in #6425 (issue #6400), which this PR flips to assert the corrected behavior. ### How was this PR tested? Added `predicate.spec.ts` (8 tests) covering `isNonNullObject` and the previously untested `isDefined`. Updated `type-predicates.spec.ts`: the four null-payload pins now assert `toBe(false)`, the null/undefined input cases are tightened from `toBeFalsy()` to `toBe(false)`, and the `isDashboardProject` null-workflow case keeps asserting `true` with a comment marking it as an intentional decision; the 5x5 cross-classification matrix and all of its 25 expected values are untouched, confirming that classification of realistic entries is unaffected. Ran locally via `yarn ng test --watch=false --include='**/common/util/predicate.spec.ts' --include='**/dashboard/type/type-predicates.spec.ts'` (61/61 pass), plus the full frontend suite whose failure set is identical to the unmodified main baseline (pre-existing, environment-related failures only), and `tsc --noEmit` with zero errors. ### Was this PR authored or co-authored using generative AI tooling? Co-authored using Claude Code(Fable 5). --------- Co-authored-by: Claude Opus 4.8 <[email protected]> --- frontend/src/app/common/util/predicate.spec.ts | 32 +++++++++- frontend/src/app/common/util/predicate.ts | 10 ++++ .../src/app/dashboard/type/type-predicates.spec.ts | 70 ++++++++++------------ frontend/src/app/dashboard/type/type-predicates.ts | 11 ++-- 4 files changed, 77 insertions(+), 46 deletions(-) diff --git a/frontend/src/app/common/util/predicate.spec.ts b/frontend/src/app/common/util/predicate.spec.ts index bd4f681507..65e16fa8fc 100644 --- a/frontend/src/app/common/util/predicate.spec.ts +++ b/frontend/src/app/common/util/predicate.spec.ts @@ -17,7 +17,37 @@ * under the License. */ -import { isDefined } from "./predicate"; +import { isDefined, isNonNullObject } from "./predicate"; + +describe("isNonNullObject", () => { + it("should return true for plain objects", () => { + expect(isNonNullObject({})).toBe(true); + expect(isNonNullObject({ a: 1 })).toBe(true); + }); + + it("should return true for arrays", () => { + // Arrays intentionally pass: the predicate checks "non-null object" in the + // `typeof` sense, and `typeof [] === "object"`. + expect(isNonNullObject([])).toBe(true); + expect(isNonNullObject([1, 2, 3])).toBe(true); + }); + + it("should return false for null and undefined", () => { + expect(isNonNullObject(null)).toBe(false); + expect(isNonNullObject(undefined)).toBe(false); + }); + + it("should return false for primitives", () => { + expect(isNonNullObject(42)).toBe(false); + expect(isNonNullObject("string")).toBe(false); + expect(isNonNullObject(true)).toBe(false); + }); + + it("should return false for functions", () => { + // `typeof fn === "function"`, not "object", so functions are rejected. + expect(isNonNullObject(() => {})).toBe(false); + }); +}); describe("isDefined", () => { it("returns false for undefined", () => { diff --git a/frontend/src/app/common/util/predicate.ts b/frontend/src/app/common/util/predicate.ts index 981a1c7ff6..044417a738 100644 --- a/frontend/src/app/common/util/predicate.ts +++ b/frontend/src/app/common/util/predicate.ts @@ -25,3 +25,13 @@ export function isDefined<T>(val: T | undefined | null): val is T { return val !== undefined && val != null; } + +/** + * checks that the given value is a non-null object in the `typeof` sense, + * guarding against the JavaScript quirk that `typeof null === "object"`. + * @param x + * @returns {boolean} + */ +export function isNonNullObject(x: unknown): x is object { + return typeof x === "object" && x !== null; +} diff --git a/frontend/src/app/dashboard/type/type-predicates.spec.ts b/frontend/src/app/dashboard/type/type-predicates.spec.ts index 1557010023..d98afd8b9f 100644 --- a/frontend/src/app/dashboard/type/type-predicates.spec.ts +++ b/frontend/src/app/dashboard/type/type-predicates.spec.ts @@ -135,11 +135,9 @@ describe("isDashboardWorkflow", () => { expect(isDashboardWorkflow(workflowFixture)).toBe(true); }); - // The guard returns the falsy input itself via the `value && ...` short-circuit, - // so the result is null/undefined rather than the boolean false. - it("should be falsy for null and undefined", () => { - expect(isDashboardWorkflow(null)).toBeFalsy(); - expect(isDashboardWorkflow(undefined)).toBeFalsy(); + it("should return false for null and undefined", () => { + expect(isDashboardWorkflow(null)).toBe(false); + expect(isDashboardWorkflow(undefined)).toBe(false); }); it("should return false for an object without a workflow field", () => { @@ -150,9 +148,9 @@ describe("isDashboardWorkflow", () => { expect(isDashboardWorkflow({ workflow: "not an object" })).toBe(false); }); - it("should return true when workflow is null (current behavior)", () => { - // Documents current behavior: typeof null === "object", so a null field passes the guard. - expect(isDashboardWorkflow({ workflow: null })).toBe(true); + it("should return false when workflow is null", () => { + // A null payload must be rejected even though typeof null === "object". + expect(isDashboardWorkflow({ workflow: null })).toBe(false); }); }); @@ -161,11 +159,9 @@ describe("isDashboardProject", () => { expect(isDashboardProject(projectFixture)).toBe(true); }); - // The guard returns the falsy input itself via the `value && ...` short-circuit, - // so the result is null/undefined rather than the boolean false. - it("should be falsy for null and undefined", () => { - expect(isDashboardProject(null)).toBeFalsy(); - expect(isDashboardProject(undefined)).toBeFalsy(); + it("should return false for null and undefined", () => { + expect(isDashboardProject(null)).toBe(false); + expect(isDashboardProject(undefined)).toBe(false); }); it("should return false for an object without a name field", () => { @@ -180,9 +176,9 @@ describe("isDashboardProject", () => { expect(isDashboardProject({ name: "x", workflow: workflowFixture.workflow })).toBe(false); }); - it("should return true when name is a string and workflow is null (current behavior)", () => { - // Documents current behavior: `!value.workflow` is true for a null workflow, - // so the exclusion branch does not reject it. + it("should return true when name is a string and workflow is null", () => { + // Intentional: a null workflow field is treated as "no workflow", so the + // exclusion branch `!value.workflow` still classifies the object as a project. expect(isDashboardProject({ name: "x", workflow: null })).toBe(true); }); }); @@ -192,11 +188,9 @@ describe("isDashboardFile", () => { expect(isDashboardFile(fileFixture)).toBe(true); }); - // The guard returns the falsy input itself via the `value && ...` short-circuit, - // so the result is null/undefined rather than the boolean false. - it("should be falsy for null and undefined", () => { - expect(isDashboardFile(null)).toBeFalsy(); - expect(isDashboardFile(undefined)).toBeFalsy(); + it("should return false for null and undefined", () => { + expect(isDashboardFile(null)).toBe(false); + expect(isDashboardFile(undefined)).toBe(false); }); it("should return false for an empty object", () => { @@ -215,9 +209,9 @@ describe("isDashboardFile", () => { expect(isDashboardFile({ ownerEmail: 42, file: fileFixture.file })).toBe(false); }); - it("should return true when file is null (current behavior)", () => { - // Documents current behavior: typeof null === "object", so a null field passes the guard. - expect(isDashboardFile({ ownerEmail: "[email protected]", file: null })).toBe(true); + it("should return false when file is null", () => { + // A null payload must be rejected even though typeof null === "object". + expect(isDashboardFile({ ownerEmail: "[email protected]", file: null })).toBe(false); }); }); @@ -226,11 +220,9 @@ describe("isDashboardDataset", () => { expect(isDashboardDataset(datasetFixture)).toBe(true); }); - // The guard returns the falsy input itself via the `value && ...` short-circuit, - // so the result is null/undefined rather than the boolean false. - it("should be falsy for null and undefined", () => { - expect(isDashboardDataset(null)).toBeFalsy(); - expect(isDashboardDataset(undefined)).toBeFalsy(); + it("should return false for null and undefined", () => { + expect(isDashboardDataset(null)).toBe(false); + expect(isDashboardDataset(undefined)).toBe(false); }); it("should return false for an object without a dataset field", () => { @@ -241,9 +233,9 @@ describe("isDashboardDataset", () => { expect(isDashboardDataset({ dataset: "not an object" })).toBe(false); }); - it("should return true when dataset is null (current behavior)", () => { - // Documents current behavior: typeof null === "object", so a null field passes the guard. - expect(isDashboardDataset({ dataset: null })).toBe(true); + it("should return false when dataset is null", () => { + // A null payload must be rejected even though typeof null === "object". + expect(isDashboardDataset({ dataset: null })).toBe(false); }); }); @@ -252,11 +244,9 @@ describe("isDashboardWorkflowComputingUnit", () => { expect(isDashboardWorkflowComputingUnit(computingUnitFixture)).toBe(true); }); - // The guard returns the falsy input itself via the `value && ...` short-circuit, - // so the result is null/undefined rather than the boolean false. - it("should be falsy for null and undefined", () => { - expect(isDashboardWorkflowComputingUnit(null)).toBeFalsy(); - expect(isDashboardWorkflowComputingUnit(undefined)).toBeFalsy(); + it("should return false for null and undefined", () => { + expect(isDashboardWorkflowComputingUnit(null)).toBe(false); + expect(isDashboardWorkflowComputingUnit(undefined)).toBe(false); }); it("should return false for an object without a computingUnit field", () => { @@ -267,9 +257,9 @@ describe("isDashboardWorkflowComputingUnit", () => { expect(isDashboardWorkflowComputingUnit({ computingUnit: "not an object" })).toBe(false); }); - it("should return true when computingUnit is null (current behavior)", () => { - // Documents current behavior: typeof null === "object", so a null field passes the guard. - expect(isDashboardWorkflowComputingUnit({ computingUnit: null })).toBe(true); + it("should return false when computingUnit is null", () => { + // A null payload must be rejected even though typeof null === "object". + expect(isDashboardWorkflowComputingUnit({ computingUnit: null })).toBe(false); }); }); diff --git a/frontend/src/app/dashboard/type/type-predicates.ts b/frontend/src/app/dashboard/type/type-predicates.ts index ae4217d79e..8c128b2c11 100644 --- a/frontend/src/app/dashboard/type/type-predicates.ts +++ b/frontend/src/app/dashboard/type/type-predicates.ts @@ -22,23 +22,24 @@ import { DashboardProject } from "./dashboard-project.interface"; import { DashboardFile } from "./dashboard-file.interface"; import { DashboardDataset } from "./dashboard-dataset.interface"; import { DashboardWorkflowComputingUnit } from "../../common/type/workflow-computing-unit"; +import { isNonNullObject } from "../../common/util/predicate"; export function isDashboardWorkflow(value: any): value is DashboardWorkflow { - return value && typeof value.workflow === "object"; + return !!value && isNonNullObject(value.workflow); } export function isDashboardProject(value: any): value is DashboardProject { - return value && typeof value.name === "string" && !value.workflow; + return !!value && typeof value.name === "string" && !value.workflow; } export function isDashboardFile(value: any): value is DashboardFile { - return value && typeof value.ownerEmail === "string" && typeof value.file === "object"; + return !!value && typeof value.ownerEmail === "string" && isNonNullObject(value.file); } export function isDashboardDataset(value: any): value is DashboardDataset { - return value && typeof value.dataset === "object"; + return !!value && isNonNullObject(value.dataset); } export function isDashboardWorkflowComputingUnit(value: any): value is DashboardWorkflowComputingUnit { - return value && typeof value.computingUnit === "object"; + return !!value && isNonNullObject(value.computingUnit); }
