Skip to content

Commit 9db87d9

Browse files
Kiryoustalborenkibanamachine
authored andcommitted
fix: yaml variable validation (elastic#231342)
## Summary - fixes [Bug] Connector step should execute when connector-id is missing - fixes bug: kibana crashes if template expression used without quotes - use zod schema for connector params and outputs - enhance completion with path segments type - handle completion, when cursor inside curly brackets ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ... --------- Co-authored-by: Tal <[email protected]> Co-authored-by: kibanamachine <[email protected]>
1 parent 6262ae9 commit 9db87d9

File tree

22 files changed

+1461
-318
lines changed

22 files changed

+1461
-318
lines changed

src/platform/packages/shared/kbn-workflows/spec/lib/generate_yaml_schema.ts

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,36 +20,16 @@ import {
2020

2121
export interface ConnectorContract {
2222
type: string;
23-
params: Array<{
24-
name: string;
25-
type: 'string' | 'number' | 'boolean' | 'object';
26-
}>;
27-
}
28-
29-
function getZodTypeForParam(param: ConnectorContract['params'][number]) {
30-
switch (param.type) {
31-
case 'string':
32-
return z.string();
33-
case 'number':
34-
return z.number();
35-
case 'boolean':
36-
return z.boolean();
37-
case 'object':
38-
return z.record(z.string(), z.any());
39-
default:
40-
return z.string();
41-
}
23+
paramsSchema: z.ZodType;
24+
connectorIdRequired?: boolean;
25+
outputSchema: z.ZodType;
4226
}
4327

4428
function generateStepSchemaForConnector(connector: ConnectorContract) {
45-
const paramSchema = connector.params.reduce((acc, param) => {
46-
acc[param.name] = getZodTypeForParam(param);
47-
return acc;
48-
}, {} as Record<string, z.ZodType>);
49-
5029
return BaseConnectorStepSchema.extend({
5130
type: z.literal(connector.type),
52-
with: z.object(paramSchema),
31+
'connector-id': connector.connectorIdRequired ? z.string() : z.string().optional(),
32+
with: connector.paramsSchema,
5333
});
5434
}
5535

src/platform/packages/shared/kbn-workflows/spec/schema.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,3 +256,24 @@ export const WorkflowSchema = z.object({
256256
});
257257

258258
export type WorkflowYaml = z.infer<typeof WorkflowSchema>;
259+
260+
export const WorkflowContextSchema = z.object({
261+
workflowRunId: z.string(),
262+
event: z.any().optional(),
263+
consts: z.record(z.string(), z.any()).optional(),
264+
steps: z.record(
265+
z.string(),
266+
z.object({
267+
output: z.any().optional(),
268+
error: z.any().optional(),
269+
})
270+
),
271+
foreach: z
272+
.object({
273+
item: z.any(),
274+
})
275+
.optional(),
276+
now: z.date().optional(),
277+
});
278+
279+
export type WorkflowContext = z.infer<typeof WorkflowContextSchema>;

src/platform/plugins/shared/workflows_execution_engine/server/step/connector_step.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -70,8 +70,8 @@ export class ConnectorStepImpl extends StepBase<ConnectorStep> {
7070
tags: ['console', 'log'],
7171
});
7272
// eslint-disable-next-line no-console
73-
console.log(step.with?.message);
74-
return { output: step.with?.message, error: undefined };
73+
console.log(withInputs.message);
74+
return { output: withInputs.message, error: undefined };
7575
} else if (step.type === 'delay') {
7676
const delayTime = step.with?.delay ?? 1000;
7777
// this.contextManager.logDebug(`Delaying for ${delayTime}ms`);
@@ -93,7 +93,13 @@ export class ConnectorStepImpl extends StepBase<ConnectorStep> {
9393
renderedInputs
9494
);
9595

96-
return { output, error: undefined };
96+
const { data, status, message } = output;
97+
98+
if (status === 'ok') {
99+
return { output: data, error: undefined };
100+
} else {
101+
return await this.handleFailure(message);
102+
}
97103
} catch (error) {
98104
return await this.handleFailure(error);
99105
}

src/platform/plugins/shared/workflows_execution_engine/server/workflow_context_manager/workflow_context_manager.ts

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
import type { graphlib } from '@dagrejs/dagre';
1111
import type { ElasticsearchClient, Logger } from '@kbn/core/server';
12-
import type { WorkflowSchema } from '@kbn/workflows';
12+
import type { WorkflowContext, WorkflowSchema } from '@kbn/workflows';
1313
import type { z } from '@kbn/zod';
1414
import type { WorkflowExecutionRuntimeManager } from './workflow_execution_runtime_manager';
1515

@@ -26,42 +26,42 @@ export interface ContextManagerInit {
2626
}
2727

2828
export class WorkflowContextManager {
29-
private context: Record<string, any>; // Make it strongly typed
29+
// 'now' will be added by the templating engine
30+
private context: Omit<WorkflowContext, 'now'>;
3031
private workflowExecutionGraph: graphlib.Graph;
3132
private workflowExecutionRuntime: WorkflowExecutionRuntimeManager;
3233

3334
constructor(init: ContextManagerInit) {
3435
this.context = {
3536
workflowRunId: init.workflowRunId,
36-
workflow: init.workflow,
3737
event: init.event,
3838
consts: init.workflow.consts || {},
39+
steps: {},
3940
};
4041

4142
this.workflowExecutionGraph = init.workflowExecutionGraph;
4243
this.workflowExecutionRuntime = init.workflowExecutionRuntime;
4344
}
4445

45-
public getContext(): Record<string, any> {
46-
const stepContex: Record<string, any> = {
46+
public getContext() {
47+
const stepContext: WorkflowContext = {
4748
...this.context,
48-
steps: {},
4949
};
5050

5151
const visited = new Set<string>();
5252
const collectPredecessors = (nodeId: string) => {
5353
if (visited.has(nodeId)) return;
5454
visited.add(nodeId);
5555

56-
stepContex.steps[nodeId] = {};
56+
stepContext.steps[nodeId] = {};
5757
const stepResult = this.workflowExecutionRuntime.getStepResult(nodeId);
5858
if (stepResult) {
59-
stepContex.steps[nodeId] = stepResult.output;
59+
stepContext.steps[nodeId] = stepResult;
6060
}
6161

6262
const stepState = this.workflowExecutionRuntime.getStepState(nodeId);
6363
if (stepState) {
64-
stepContex.steps[nodeId] = stepState;
64+
stepContext.steps[nodeId] = stepState;
6565
}
6666

6767
const preds = this.workflowExecutionGraph.predecessors(nodeId) || [];
@@ -73,11 +73,11 @@ export class WorkflowContextManager {
7373
const directPredecessors = this.workflowExecutionGraph.predecessors(currentNodeId) || [];
7474
directPredecessors.forEach((nodeId) => collectPredecessors(nodeId));
7575

76-
return stepContex;
76+
return stepContext;
7777
}
7878

7979
public getContextKey(key: string): any {
80-
return this.context[key];
80+
return this.context[key as keyof typeof this.context];
8181
}
8282

8383
public readContextPath(propertyPath: string): { pathExists: boolean; value: any } {
Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the "Elastic License
4+
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
5+
* Public License v 1"; you may not use this file except in compliance with, at
6+
* your election, the "Elastic License 2.0", the "GNU Affero General Public
7+
* License v3.0 only", or the "Server Side Public License, v 1".
8+
*/
9+
10+
import { validateVariablePath, parseVariablePath } from './parse_variable_path';
11+
12+
describe('validateVariablePath', () => {
13+
it('should validate a simple path', () => {
14+
expect(validateVariablePath('foo')).toBe(true);
15+
});
16+
17+
it('should fail if any segment starts with a number', () => {
18+
expect(validateVariablePath('1foo')).toBe(false);
19+
});
20+
21+
it('should fail if any segment contains a space', () => {
22+
expect(validateVariablePath('foo bar')).toBe(false);
23+
});
24+
25+
it('should fail if any segment contains a special character', () => {
26+
expect(validateVariablePath('foo@bar')).toBe(false);
27+
});
28+
29+
it('should validate a path with a dot', () => {
30+
expect(validateVariablePath('steps.snake_case')).toBe(true);
31+
});
32+
33+
it('should fail on kebab-case if accessed with dot', () => {
34+
expect(validateVariablePath('steps.first-step')).toBe(false);
35+
});
36+
37+
it('should validate a path with a brackets', () => {
38+
expect(validateVariablePath('foo["4-bar-a"]')).toBe(true);
39+
expect(validateVariablePath("foo['4-bar-a']")).toBe(true);
40+
});
41+
42+
it('should fail if quotes are not closed or mixed', () => {
43+
expect(validateVariablePath(`steps["first-step']`)).toBe(false);
44+
expect(validateVariablePath('steps["first-step]')).toBe(false);
45+
expect(validateVariablePath(`steps['first-step]`)).toBe(false);
46+
});
47+
48+
it('should validate a path with a dot and a bracket', () => {
49+
expect(validateVariablePath('foo.bar["baz"]')).toBe(true);
50+
});
51+
52+
it('should validate a numeric index', () => {
53+
expect(validateVariablePath('steps[0]')).toBe(true);
54+
});
55+
56+
it('should validate a path with filters', () => {
57+
expect(validateVariablePath('foo | title')).toBe(true);
58+
});
59+
60+
it('should validate a path with multiple filters', () => {
61+
expect(validateVariablePath('foo | replace("foo", "bar") | capitalize')).toBe(true);
62+
});
63+
64+
it('should validate a complex path with filters', () => {
65+
expect(validateVariablePath('steps.data["key"] | join(",") | upper')).toBe(true);
66+
});
67+
});
68+
69+
describe('parseVariablePath', () => {
70+
it('should parse a simple path without filters', () => {
71+
const result = parseVariablePath('foo');
72+
expect(result).toEqual({
73+
propertyPath: 'foo',
74+
filters: [],
75+
});
76+
});
77+
78+
it('should parse a complex path without filters', () => {
79+
const result = parseVariablePath('steps.data["key"][0]');
80+
expect(result).toEqual({
81+
propertyPath: 'steps.data["key"][0]',
82+
filters: [],
83+
});
84+
});
85+
86+
it('should return errors for invalid paths', () => {
87+
const result = parseVariablePath('steps.data.kebab-case[0] | capitalize');
88+
expect(result).toEqual({
89+
errors: ['Invalid property path: steps.data.kebab-case[0]'],
90+
propertyPath: null,
91+
filters: [],
92+
});
93+
});
94+
95+
it('should parse a path with a single filter', () => {
96+
const result = parseVariablePath('foo | title');
97+
expect(result).toEqual({
98+
propertyPath: 'foo',
99+
filters: ['title'],
100+
});
101+
});
102+
103+
it('should parse a path with a filter that has arguments', () => {
104+
const result = parseVariablePath('foo | join(",")');
105+
expect(result).toEqual({
106+
propertyPath: 'foo',
107+
filters: ['join(",")'],
108+
});
109+
});
110+
111+
it('should parse a path with multiple filters', () => {
112+
const result = parseVariablePath('foo | replace("foo", "bar") | capitalize');
113+
expect(result).toEqual({
114+
propertyPath: 'foo',
115+
filters: ['replace("foo", "bar")', 'capitalize'],
116+
});
117+
});
118+
119+
it('should parse a complex path with multiple filters', () => {
120+
const result = parseVariablePath('steps.data["key"] | join(",") | upper | trim');
121+
expect(result).toEqual({
122+
propertyPath: 'steps.data["key"]',
123+
filters: ['join(",")', 'upper', 'trim'],
124+
});
125+
});
126+
127+
it('should handle filters with nested parentheses', () => {
128+
const result = parseVariablePath('foo | replace("(test)", "bar") | title');
129+
expect(result).toEqual({
130+
propertyPath: 'foo',
131+
filters: ['replace("(test)", "bar")', 'title'],
132+
});
133+
});
134+
135+
it('should handle filters with single quotes', () => {
136+
const result = parseVariablePath("foo | replace('old', 'new')");
137+
expect(result).toEqual({
138+
propertyPath: 'foo',
139+
filters: ["replace('old', 'new')"],
140+
});
141+
});
142+
143+
it('should handle filters with named parameters', () => {
144+
const result = parseVariablePath('foo | replace(old="old", new="new")');
145+
expect(result).toEqual({
146+
propertyPath: 'foo',
147+
filters: ['replace(old="old", new="new")'],
148+
});
149+
});
150+
151+
it('should handle spaces around pipes and filter names', () => {
152+
const result = parseVariablePath(' foo | title | upper ');
153+
expect(result).toEqual({
154+
propertyPath: 'foo',
155+
filters: ['title', 'upper'],
156+
});
157+
});
158+
159+
it('should return null for invalid variable paths', () => {
160+
expect(parseVariablePath('1foo | title')).toEqual({
161+
errors: ['Invalid property path: 1foo'],
162+
propertyPath: null,
163+
filters: [],
164+
});
165+
expect(parseVariablePath('foo@bar | title')).toEqual({
166+
errors: ['Invalid property path: foo@bar'],
167+
propertyPath: null,
168+
filters: [],
169+
});
170+
});
171+
172+
it('should return null for invalid filters', () => {
173+
expect(parseVariablePath('foo | 1invalid')).toEqual({
174+
errors: ['Invalid filter name: 1invalid'],
175+
propertyPath: null,
176+
filters: [],
177+
});
178+
179+
expect(parseVariablePath('foo | title | $invalid')).toEqual({
180+
errors: ['Invalid filter name: $invalid'],
181+
propertyPath: null,
182+
filters: [],
183+
});
184+
});
185+
});

0 commit comments

Comments
 (0)