diff --git a/.changeset/temporary-validation-workaround.md b/.changeset/temporary-validation-workaround.md new file mode 100644 index 00000000..58357b96 --- /dev/null +++ b/.changeset/temporary-validation-workaround.md @@ -0,0 +1,5 @@ +--- +"@openworkflowspec/diagram-editor": minor +--- + +Temporary validation workaround before updated specification is published diff --git a/packages/open-workflow-diagram-editor/src/core/index.ts b/packages/open-workflow-diagram-editor/src/core/index.ts index 64fd1bb2..3f8efe3b 100644 --- a/packages/open-workflow-diagram-editor/src/core/index.ts +++ b/packages/open-workflow-diagram-editor/src/core/index.ts @@ -16,6 +16,8 @@ export * from "./workflowSdk"; export * from "./validationErrors"; +/* TEMPORARY — remove with the workaround; see the revert checklist in workflowSdk.ts. */ +export * from "./specWorkarounds"; export * from "./graph"; export * from "./taskDetails"; export * from "./taskSubType"; diff --git a/packages/open-workflow-diagram-editor/src/core/specWorkarounds.ts b/packages/open-workflow-diagram-editor/src/core/specWorkarounds.ts new file mode 100644 index 00000000..9429f3d1 --- /dev/null +++ b/packages/open-workflow-diagram-editor/src/core/specWorkarounds.ts @@ -0,0 +1,60 @@ +/* + * Copyright 2021-Present The Open Workflow Specification Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import type { ValidationError } from "./workflowSdk"; + +/* TEMPORARY — DELETE FILE WHEN THE SDK IS BUMPED https://github.com/open-workflow-specification/editor/issues/267 + The specification has merged validation changes (spec#1169, spec#1175) that are not yet published in `@openworkflowspec/sdk`. Once the spec publishes, the bundled schema will reject workflows that are valid against it, and the editor would show validation errors for them. This module temporarily strips those false errors. + */ + +const URI_TEMPLATE_DEF = "/$defs/uriTemplate"; +const EMIT_WITH_REQUIRED = "/properties/emit/properties/event/properties/with/required"; + +/* https://github.com/open-workflow-specification/specification/pull/1169 */ +function findRelaxedUriPaths(errors: ValidationError[]): Set { + const paths = new Set(); + for (const error of errors) { + if (error.path !== undefined && error.errorType?.includes(URI_TEMPLATE_DEF)) { + paths.add(error.path); + } + } + return paths; +} + +/* https://github.com/open-workflow-specification/specification/pull/1175 */ +function isEmitSourceRequiredError(error: ValidationError): boolean { + return ( + error.errorType?.includes(EMIT_WITH_REQUIRED) === true && + error.object?.["missingProperty"] === "source" + ); +} + +/* + Removes validation errors the bundled SDK schema raises for workflows that are + valid against the current specification. + Done during `validateWorkflow` so the errors never reach the store. + */ +export function stripSpecAheadOfSdkErrors(errors: ValidationError[]): ValidationError[] { + const relaxedUriPaths = findRelaxedUriPaths(errors); + + return errors.filter((error) => { + if (error.path !== undefined && relaxedUriPaths.has(error.path)) { + return false; + } + + return !isEmitSourceRequiredError(error); + }); +} diff --git a/packages/open-workflow-diagram-editor/src/core/workflowSdk.ts b/packages/open-workflow-diagram-editor/src/core/workflowSdk.ts index c5a4e496..4765f032 100644 --- a/packages/open-workflow-diagram-editor/src/core/workflowSdk.ts +++ b/packages/open-workflow-diagram-editor/src/core/workflowSdk.ts @@ -17,6 +17,7 @@ import { load } from "js-yaml"; import * as sdk from "@openworkflowspec/sdk"; import { fixNodesConnections } from "./graph"; +import { stripSpecAheadOfSdkErrors } from "./specWorkarounds"; /** * Sanitizes an object by removing dangerous prototype pollution keys @@ -238,9 +239,14 @@ export function validateWorkflow(model: sdk.Specification.Workflow): SdkError[] const message = err instanceof Error ? err.message : String(err); const parsedErrors = dedupeValidationErrors(parseValidationErrorMessage(message)); - // If parsing succeeded and returned errors, use them + /* If parsing succeeded and returned errors, use them. + * + * TEMPORARY — TO REVERT WHEN THE SDK IS BUMPED (see core/specWorkarounds.ts): + * Replace `stripSpecAheadOfSdkErrors(...)` with `return parsedErrors;` again. + */ if (parsedErrors.length > 0) { - return parsedErrors; + //return parsedErrors; + return stripSpecAheadOfSdkErrors(parsedErrors); } // Otherwise, return the original error as-is diff --git a/packages/open-workflow-diagram-editor/stories/features/DiagramEditor.stories.tsx b/packages/open-workflow-diagram-editor/stories/features/DiagramEditor.stories.tsx index 19fa9fd8..82986458 100644 --- a/packages/open-workflow-diagram-editor/stories/features/DiagramEditor.stories.tsx +++ b/packages/open-workflow-diagram-editor/stories/features/DiagramEditor.stories.tsx @@ -63,7 +63,7 @@ do: call: openapi with: document: - endpoint: http://myorg.io/ordersservices.json + endpoint: openapi/ordersservices.json operationId: produceReport - emitEvent: emit: @@ -78,6 +78,13 @@ do: items: - breed: dalmatian quantity: 101 + - emitCompletion: + emit: + event: + with: + type: com.petstore.readings.completed.v1 + data: + roomId: \${ .roomid } timeout: after: hours: 1`; @@ -107,3 +114,52 @@ export const Component: Story = { content: workflowExample, }, }; + +/* The two stories below each isolate ONE piece of spec syntax the editor must accept, + * and both must render clean (no error badge). + */ + +const relativeUriEndpointExample = `document: + dsl: '1.0.3' + namespace: examples + name: relative-uri-endpoint + version: '0.1.0' +do: + - generateReport: + call: openapi + with: + document: + endpoint: openapi/ordersservices.json + operationId: produceReport`; + +const emitWithoutSourceExample = `document: + dsl: '1.0.3' + namespace: examples + name: emit-without-source + version: '0.1.0' +do: + - emitCompletion: + emit: + event: + with: + type: com.petstore.readings.completed.v1 + data: + roomId: \${ .roomid }`; + +/* A URI is an RFC 3986 URI-reference, so a relative one is valid and must not error. */ +export const RelativeUriEndpoint: Story = { + args: { + isReadOnly: true, + locale: "en", + content: relativeUriEndpointExample, + }, +}; + +/* `source` is optional when emitting (runtimes generate it from the workflow) — so omitting it must not error. */ +export const EmitWithoutSource: Story = { + args: { + isReadOnly: true, + locale: "en", + content: emitWithoutSourceExample, + }, +}; diff --git a/packages/open-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts b/packages/open-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts index 100b3b6a..fdbad5ba 100644 --- a/packages/open-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts +++ b/packages/open-workflow-diagram-editor/tests-e2e/diagram-editor.spec.ts @@ -27,9 +27,9 @@ test("diagram editor renders correctly", async ({ page }) => { // Check total nodes const nodes = page.locator('[data-testid^="rf__node-"]'); - await expect(nodes).toHaveCount(9); + await expect(nodes).toHaveCount(10); // Check total edge const edges = page.locator('[data-testid^="rf__edge-"]'); - await expect(edges).toHaveCount(7); + await expect(edges).toHaveCount(8); }); diff --git a/packages/open-workflow-diagram-editor/tests/core/specWorkarounds.test.ts b/packages/open-workflow-diagram-editor/tests/core/specWorkarounds.test.ts new file mode 100644 index 00000000..441af6ce --- /dev/null +++ b/packages/open-workflow-diagram-editor/tests/core/specWorkarounds.test.ts @@ -0,0 +1,241 @@ +/* + * Copyright 2021-Present The Open Workflow Specification Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { describe, it, expect } from "vitest"; +import { + parseWorkflow, + buildFlatGraph, + getTaskReferences, + getErrorTaskReferences, + getGeneralErrors, + stripSpecAheadOfSdkErrors, +} from "../../src/core"; +import type { ValidationError } from "../../src/core"; + +/* TEMPORARY — delete this whole file with src/core/specWorkarounds.ts. */ + +const vErr = (partial: Partial): ValidationError => ({ + message: "boom", + ...partial, +}); + +/* What the UI actually surfaces: the tasks that get an error badge plus the + * workflow-level errors behind the side-panel count. Asserting here rather than on + * the raw `errors` array is deliberate — the raw array still carries the SDK's + * `oneOf` discrimination cascade, which `isNoiseError` strips at the query layer. + * The workaround only has to remove the root-cause errors; the cascade siblings are + * already-handled noise. */ +function surfacedErrors(yaml: string): { badgedTasks: string[]; general: string[] } { + const { model, errors } = parseWorkflow(yaml); + const taskReferences = getTaskReferences(buildFlatGraph(model!)); + + return { + badgedTasks: [...getErrorTaskReferences(errors, taskReferences)], + general: getGeneralErrors(errors, taskReferences).map((error) => error.message), + }; +} + +const DOCUMENT = `document: + dsl: '1.0.0' + namespace: test + name: spec-workarounds + version: '1.0.0' +`; + +/* End-to-end against the real bundled SDK schema. Each workflow is valid against the + * current specification, so the editor must surface no errors for it. */ +describe("workflows valid per current spec produce no editor errors", () => { + it.each([ + { + pr: "spec#1169 — relative URI in a call endpoint", + yaml: `${DOCUMENT}do: + - getPets: + call: openapi + with: + document: + endpoint: openapi/petstore.json + operationId: findPets +`, + }, + { + pr: "spec#1169 — relative URI in an emit event source", + yaml: `${DOCUMENT}do: + - emitIt: + emit: + event: + with: + source: /orders/service + type: com.example.order +`, + }, + { + pr: "spec#1175 — emit event with no source", + yaml: `${DOCUMENT}do: + - emitIt: + emit: + event: + with: + type: com.example.order +`, + }, + ])("$pr", ({ yaml }) => { + expect(surfacedErrors(yaml)).toEqual({ badgedTasks: [], general: [] }); + }); +}); + +describe("errors the workaround must not swallow", () => { + it("still badges a task with a genuinely missing required property", () => { + const { badgedTasks } = surfacedErrors(`${DOCUMENT}do: + - getPets: + call: http + with: + method: get +`); + + expect(badgedTasks).toEqual(["/do/0/getPets"]); + }); + + it("still reports a workflow missing its document", () => { + expect( + surfacedErrors("do:\n - noop:\n set:\n x: 1\n").general.length, + ).toBeGreaterThan(0); + }); +}); + +/* TRIPWIRES. These three spec changes add new syntax rather than relaxing existing + * validation, so no already-written workflow can contain them and a false error is + * only reachable by an author using a feature the spec has not published yet — not + * worth suppressing. spec#1173 additionally MUST stay flagged: the SDK's graph + * builder ignores `catch.then` too and never draws the edge to the `then` target, so + * hiding the error would present a wrong diagram as valid. + * + * When the SDK bump makes these fail, that is the signal to delete + * src/core/specWorkarounds.ts along with this whole file. */ +describe("merged spec changes intentionally left flagged", () => { + it.each([ + { + pr: "spec#1170 — inline array in for.in", + yaml: `${DOCUMENT}do: + - loopIt: + for: + each: item + in: + - name: a + - name: b + do: + - noop: + set: + x: 1 +`, + }, + { + pr: "spec#1178 — read on schedule", + yaml: `${DOCUMENT}schedule: + on: + one: + with: + type: com.example.order-placed + read: envelope +do: + - noop: + set: + x: 1 +`, + }, + { + pr: "spec#1173 — then on catch, which the SDK cannot yet render", + yaml: `${DOCUMENT}do: + - tryIt: + try: + - noop: + set: + x: 1 + catch: + errors: + with: + type: https://example.com/err + then: recordFailure + - recordFailure: + set: + y: 2 +`, + }, + ])("$pr", ({ yaml }) => { + const { badgedTasks, general } = surfacedErrors(yaml); + + expect(badgedTasks.length + general.length).toBeGreaterThan(0); + }); +}); + +describe("stripSpecAheadOfSdkErrors", () => { + it("drops the whole sibling cluster at a path where the old URI pattern fired", () => { + const errors = [ + vErr({ + path: "/do/0/emitIt/emit/event/with/source", + errorType: "#/$defs/uriTemplate/anyOf/0/pattern", + }), + vErr({ + path: "/do/0/emitIt/emit/event/with/source", + errorType: "#/$defs/runtimeExpression/pattern", + }), + vErr({ + path: "/do/0/emitIt/emit/event/with/source", + errorType: "#/properties/source/oneOf", + }), + ]; + + expect(stripSpecAheadOfSdkErrors(errors)).toEqual([]); + }); + + it("keeps a runtimeExpression error at a path with no URI error", () => { + const ifError = vErr({ path: "/do/0/noop/if", errorType: "#/$defs/runtimeExpression/pattern" }); + + expect(stripSpecAheadOfSdkErrors([ifError])).toEqual([ifError]); + }); + + it("keeps errors for the merged spec changes it does not cover", () => { + const errors = [ + vErr({ + path: "/do/0/loopIt/for/in", + errorType: "#/allOf/1/properties/for/properties/in/type", + }), + vErr({ + path: "/schedule", + errorType: "#/properties/schedule/unevaluatedProperties", + object: { unevaluatedProperty: "read" }, + }), + vErr({ + path: "/do/0/tryIt/catch", + errorType: "#/allOf/1/properties/catch/unevaluatedProperties", + object: { unevaluatedProperty: "then" }, + }), + ]; + + expect(stripSpecAheadOfSdkErrors(errors)).toEqual(errors); + }); + + it("keeps a required-property error for a property other than emit's source", () => { + const errors = [ + vErr({ + path: "/do/0/emitIt/emit/event/with", + errorType: "#/allOf/1/properties/emit/properties/event/properties/with/required", + object: { missingProperty: "type" }, + }), + ]; + + expect(stripSpecAheadOfSdkErrors(errors)).toEqual(errors); + }); +});