From 8363f0e905c89293b155683d05c67ab26a2c1228 Mon Sep 17 00:00:00 2001 From: kary zheng Date: Thu, 13 Aug 2026 16:26:27 -0700 Subject: [PATCH] feat(gui): take the required marker from the schema's own condition A field required only under a condition cannot say so in the top-level `required` array, which is a fixed list, so the marker never appeared for one and the panel gave no sign that a field had become mandatory until the operator was refused. Aggregate needs such a rule and already declares it correctly, under definitions.AggregationOperation.allOf. The marker was then produced a second time, by operator name, in this component. Two statements of one rule, in two languages, free to drift, and the next operator wanting the same thing would have added a third branch beside the field-specific ones already here. conditionalRequiredRules reads what the schema declares. The walk is recursive because a rule may govern a field inside an array item, where it sits under definitions rather than at the top. Keying by field name is enough: the condition is evaluated against the field's own parent model, the row for an array item and the operator for a top-level field, so the same rule resolves correctly in both. It matches on if.properties..const, which the attributeTypeRules blocks do not have, naming their sibling directly and requiring nothing. Aggregate keeps its behaviour and loses its branch. Sklearn's text column, which its descriptor now declares the same way, gets the marker without a line here. Co-Authored-By: Claude Opus 5 (1M context) --- ...ator-property-edit-frame.component.spec.ts | 50 ++++++++++--- .../operator-property-edit-frame.component.ts | 73 +++++++++++++++---- 2 files changed, 97 insertions(+), 26 deletions(-) diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts index a127d0bc6dc..74ccc4dcea3 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.spec.ts @@ -19,11 +19,7 @@ import { ComponentFixture, discardPeriodicTasks, fakeAsync, TestBed, tick } from "@angular/core/testing"; -import { - AGGREGATE_COUNT, - isAggregateAttributeRequired, - OperatorPropertyEditFrameComponent, -} from "./operator-property-edit-frame.component"; +import { conditionalRequiredRules, OperatorPropertyEditFrameComponent } from "./operator-property-edit-frame.component"; import { WorkflowActionService } from "../../../service/workflow-graph/model/workflow-action.service"; import { WorkflowCompilingService } from "../../../service/compile-workflow/workflow-compiling.service"; import { CustomJSONSchema7 } from "../../../types/custom-json-schema.interface"; @@ -64,14 +60,44 @@ import { UiUdfParametersSyncService } from "../../../service/code-editor/ui-udf- const { marbles } = configure({ run: false }); -describe("Aggregate attribute requirement", () => { - it("makes the attribute optional for count and required for every other function", () => { - // count -> optional (empty attribute means COUNT(*)) - expect(isAggregateAttributeRequired(AGGREGATE_COUNT)).toBe(false); - // every other aggregate function -> attribute required - ["sum", "average", "min", "max", "concat"].forEach(fn => { - expect(isAggregateAttributeRequired(fn)).toBe(true); +describe("conditionalRequiredRules", () => { + it("reads a `then` rule, as Sklearn states it for the text column", () => { + const rules = conditionalRequiredRules({ + allOf: [{ if: { properties: { countVectorizer: { const: true } } }, then: { required: ["text"] } }], + }); + expect(rules.get("text")).toEqual({ sibling: "countVectorizer", value: true, requiredOnMatch: true }); + }); + + it("reads an `else` rule nested in a definition, as Aggregate states it", () => { + const rules = conditionalRequiredRules({ + definitions: { + AggregationOperation: { + allOf: [ + { + if: { properties: { aggFunction: { const: "count" } } }, + then: {}, + else: { required: ["attribute"] }, + }, + ], + }, + }, }); + // count -> optional (an empty attribute means COUNT(*)); every other function -> required + expect(rules.get("attribute")).toEqual({ sibling: "aggFunction", value: "count", requiredOnMatch: false }); + }); + + it("ignores an attributeTypeRules block, which names its sibling without `properties`", () => { + const rules = conditionalRequiredRules({ + attributeTypeRules: { + attribute: { allOf: [{ if: { aggFunction: { valEnum: ["sum"] } }, then: { enum: ["integer"] } }] }, + }, + }); + expect(rules.size).toBe(0); + }); + + it("returns nothing for a schema that states no condition", () => { + expect(conditionalRequiredRules({ properties: { a: { type: "string" } } }).size).toBe(0); + expect(conditionalRequiredRules(undefined).size).toBe(0); }); }); diff --git a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts index beedbabd90f..b6379255192 100644 --- a/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts +++ b/frontend/src/app/workspace/component/property-editor/operator-property-edit-frame/operator-property-edit-frame.component.ts @@ -78,15 +78,56 @@ import { UiUdfParametersSyncService } from "../../../service/code-editor/ui-udf- Quill.register("modules/cursors", QuillCursors); -// The Aggregate "count" function. With an empty attribute it means COUNT(*) (all rows); -// with a column it counts that column's non-null values. It is the only function whose -// attribute is optional. -export const AGGREGATE_COUNT = "count"; - -// The Aggregate attribute is required for every function except `count` (an empty -// attribute on count means COUNT(*), which needs no column). -export function isAggregateAttributeRequired(aggFunction: unknown): boolean { - return aggFunction !== AGGREGATE_COUNT; +/** A field the schema requires only while a sibling holds a particular value. */ +export interface ConditionalRequiredRule { + sibling: string; + value: unknown; + requiredOnMatch: boolean; +} + +/** + * The conditional `required` rules a schema declares, keyed by the field each + * governs. A schema states one as + * + * allOf: [{ if: { properties: { sibling: { const: v } } }, then: { required: [field] } }] + * + * with `else` for the inverted form. Validation already honours these, but a + * field's own config never learns of them, so the required marker would not + * appear. Reading them out lets the marker follow the condition the validator + * applies, and lets an operator declare it in one place rather than here. + * + * The walk covers nested schemas because a rule may govern a field inside an + * array item, where it sits under `definitions`. Keying by field name is enough: + * the marker resolves the sibling against the field's own parent model, which is + * the row for an array item and the operator for a top-level field. + */ +export function conditionalRequiredRules(schema: unknown): Map { + const rules = new Map(); + const visit = (node: any): void => { + if (node === null || typeof node !== "object") { + return; + } + for (const branch of Array.isArray(node.allOf) ? node.allOf : []) { + // `if.properties` distinguishes a real condition from the `attributeTypeRules` + // blocks, which name the sibling directly and require nothing. + const condition = branch?.if?.properties; + const sibling = condition === undefined ? undefined : Object.keys(condition)[0]; + if (sibling === undefined || !("const" in (condition[sibling] ?? {}))) { + continue; + } + for (const [outcome, requiredOnMatch] of [ + ["then", true], + ["else", false], + ] as const) { + for (const field of branch?.[outcome]?.required ?? []) { + rules.set(field, { sibling, value: condition[sibling].const, requiredOnMatch }); + } + } + } + Object.values(node).forEach(visit); + }; + visit(schema); + return rules; } /** @@ -786,6 +827,8 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On document.getElementsByClassName("operator-version")[0].setAttribute("style", boundary.toString()); } } + // Read once: the rules describe the whole schema, not one field. + const conditionalRules = conditionalRequiredRules(this.currentOperatorSchema?.jsonSchema); // intercept JsonSchema -> FormlySchema process, adding custom options // this requires a one-to-one mapping. // for relational custom options, have to do it after FormlySchema is generated. @@ -1067,14 +1110,16 @@ export class OperatorPropertyEditFrameComponent implements OnInit, OnChanges, On mappedField.type = "datasetversionselector"; } - // Aggregate: the attribute is optional for `count` (an empty attribute means COUNT(*), - // counting all rows) and required for every other function. Show the required marker - // (red *) accordingly, based on the sibling aggFunction within the same row. - if (this.currentOperatorSchema?.operatorType === "Aggregate" && mappedField.key === "attribute") { + // Show the required marker for a field the schema requires conditionally, + // e.g. Sklearn's Text Attribute once Count Vectorizer is on, or Aggregate's + // attribute for every function but `count`. + const conditionalRequired = conditionalRules.get(mappedField.key as string); + if (conditionalRequired !== undefined) { mappedField.expressions = { ...mappedField.expressions, "props.required": (field: FormlyFieldConfig) => - isAggregateAttributeRequired(field.parent?.model?.aggFunction), + (field.parent?.model?.[conditionalRequired.sibling] === conditionalRequired.value) === + conditionalRequired.requiredOnMatch, }; }