Skip to content

Commit 2e5d9ee

Browse files
ArtemHoruzhenkoArtemHoruzhenko
authored
RI-7186 validate job name separately (#4804)
* RI-7186 validate job name separately * RI-7186 add tests
1 parent a3a28b5 commit 2e5d9ee

File tree

11 files changed

+677
-20
lines changed

11 files changed

+677
-20
lines changed

‎redisinsight/ui/src/components/yaml-validator/validatePipeline.test.ts‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { get } from 'lodash'
22
import { validatePipeline } from './validatePipeline'
3-
import { validateYamlSchema } from './validateYamlSchema'
3+
import { validateYamlSchema, validateSchema } from './validateYamlSchema'
44

55
jest.mock('./validateYamlSchema')
66

@@ -31,10 +31,16 @@ describe('validatePipeline', () => {
3131
valid: true,
3232
errors: [],
3333
}))
34+
;(validateSchema as jest.Mock).mockImplementation(() => ({
35+
valid: true,
36+
errors: [],
37+
}))
3438

3539
const result = validatePipeline({
3640
config: 'name: valid-config',
3741
schema: mockSchema,
42+
monacoJobsSchema: null,
43+
jobNameSchema: null,
3844
jobs: [
3945
{ name: 'Job1', value: 'task: job1' },
4046
{ name: 'Job2', value: 'task: job2' },
@@ -57,10 +63,16 @@ describe('validatePipeline', () => {
5763
? { valid: false, errors: ["Missing required property 'name'"] }
5864
: { valid: true, errors: [] },
5965
)
66+
;(validateSchema as jest.Mock).mockImplementation(() => ({
67+
valid: true,
68+
errors: [],
69+
}))
6070

6171
const result = validatePipeline({
6272
config: 'invalid-config-content',
6373
schema: mockSchema,
74+
monacoJobsSchema: null,
75+
jobNameSchema: null,
6476
jobs: [{ name: 'Job1', value: 'task: job1' }],
6577
})
6678

@@ -75,14 +87,20 @@ describe('validatePipeline', () => {
7587

7688
it('should return invalid result when jobs are invalid', () => {
7789
;(validateYamlSchema as jest.Mock).mockImplementation((_, schema) =>
78-
schema === get(mockSchema, 'jobs', null)
90+
schema === null
7991
? { valid: false, errors: ["Missing required property 'task'"] }
8092
: { valid: true, errors: [] },
8193
)
94+
;(validateSchema as jest.Mock).mockImplementation(() => ({
95+
valid: true,
96+
errors: [],
97+
}))
8298

8399
const result = validatePipeline({
84100
config: 'name: valid-config',
85101
schema: mockSchema,
102+
monacoJobsSchema: null,
103+
jobNameSchema: null,
86104
jobs: [{ name: 'Job1', value: 'invalid-job-content' }],
87105
})
88106

@@ -100,15 +118,21 @@ describe('validatePipeline', () => {
100118
if (schema === get(mockSchema, 'config', null)) {
101119
return { valid: false, errors: ["Missing required property 'name'"] }
102120
}
103-
if (schema === get(mockSchema, 'jobs', null)) {
121+
if (schema === null) {
104122
return { valid: false, errors: ["Missing required property 'task'"] }
105123
}
106124
return { valid: true, errors: [] }
107125
})
126+
;(validateSchema as jest.Mock).mockImplementation(() => ({
127+
valid: true,
128+
errors: [],
129+
}))
108130

109131
const result = validatePipeline({
110132
config: 'invalid-config-content',
111133
schema: mockSchema,
134+
monacoJobsSchema: null,
135+
jobNameSchema: null,
112136
jobs: [{ name: 'Job1', value: 'invalid-job-content' }],
113137
})
114138

@@ -126,10 +150,16 @@ describe('validatePipeline', () => {
126150
valid: false,
127151
errors: ['Duplicate error', 'Duplicate error'], // all the jobs get these errors
128152
}))
153+
;(validateSchema as jest.Mock).mockImplementation(() => ({
154+
valid: true,
155+
errors: [],
156+
}))
129157

130158
const result = validatePipeline({
131159
config: 'invalid-config-content',
132160
schema: mockSchema,
161+
monacoJobsSchema: null,
162+
jobNameSchema: null,
133163
jobs: [
134164
{ name: 'Job1', value: 'invalid-job-content' },
135165
{ name: 'Job2', value: 'invalid-job-content' },
@@ -145,4 +175,31 @@ describe('validatePipeline', () => {
145175
},
146176
})
147177
})
178+
179+
it('should return invalid result when job name validation fails', () => {
180+
;(validateYamlSchema as jest.Mock).mockImplementation(() => ({
181+
valid: true,
182+
errors: [],
183+
}))
184+
;(validateSchema as jest.Mock).mockImplementation(() => ({
185+
valid: false,
186+
errors: ['Job name: Invalid job name'],
187+
}))
188+
189+
const result = validatePipeline({
190+
config: 'name: valid-config',
191+
schema: mockSchema,
192+
monacoJobsSchema: null,
193+
jobNameSchema: { type: 'string', pattern: '^[a-zA-Z]+$' },
194+
jobs: [{ name: 'Job-1', value: 'task: job1' }],
195+
})
196+
197+
expect(result).toEqual({
198+
result: false,
199+
configValidationErrors: [],
200+
jobsValidationErrors: {
201+
'Job-1': ['Job name: Invalid job name'],
202+
},
203+
})
204+
})
148205
})

‎redisinsight/ui/src/components/yaml-validator/validatePipeline.ts‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,20 @@
11
import { get } from 'lodash'
2-
import { validateYamlSchema } from './validateYamlSchema'
2+
import { Nullable } from 'uiSrc/utils'
3+
import { validateSchema, validateYamlSchema } from './validateYamlSchema'
34

45
interface PipelineValidationProps {
56
config: string
67
schema: any
8+
monacoJobsSchema: Nullable<object>
9+
jobNameSchema: Nullable<object>
710
jobs: { name: string; value: string }[]
811
}
912

1013
export const validatePipeline = ({
1114
config,
1215
schema,
16+
monacoJobsSchema,
17+
jobNameSchema,
1318
jobs,
1419
}: PipelineValidationProps) => {
1520
const { valid: isConfigValid, errors: configErrors } = validateYamlSchema(
@@ -22,7 +27,11 @@ export const validatePipeline = ({
2227
jobsErrors: Record<string, Set<string>>
2328
}>(
2429
(acc, j) => {
25-
const validation = validateYamlSchema(j.value, get(schema, 'jobs', null))
30+
const validation = validateYamlSchema(j.value, monacoJobsSchema)
31+
const jobNameValidation = validateSchema(j.name, jobNameSchema, {
32+
errorMessagePrefix: 'Job name',
33+
includePathIntoErrorMessage: false,
34+
})
2635

2736
if (!acc.jobsErrors[j.name]) {
2837
acc.jobsErrors[j.name] = new Set()
@@ -32,7 +41,14 @@ export const validatePipeline = ({
3241
validation.errors.forEach((error) => acc.jobsErrors[j.name].add(error))
3342
}
3443

35-
acc.areJobsValid = acc.areJobsValid && validation.valid
44+
if (!jobNameValidation.valid) {
45+
jobNameValidation.errors.forEach((error) =>
46+
acc.jobsErrors[j.name].add(error),
47+
)
48+
}
49+
50+
acc.areJobsValid =
51+
acc.areJobsValid && validation.valid && jobNameValidation.valid
3652
return acc
3753
},
3854
{ areJobsValid: true, jobsErrors: {} },

‎redisinsight/ui/src/components/yaml-validator/validateYamlSchema.test.ts‎

Lines changed: 166 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import yaml from 'js-yaml'
2-
import { validateYamlSchema } from './validateYamlSchema'
2+
import { validateYamlSchema, validateSchema } from './validateYamlSchema'
33

44
const schema = {
55
type: 'object',
@@ -97,3 +97,168 @@ describe('validateYamlSchema', () => {
9797
jest.restoreAllMocks()
9898
})
9999
})
100+
101+
describe('validateSchema with ValidationConfig', () => {
102+
const testSchema = {
103+
type: 'object',
104+
properties: {
105+
name: { type: 'string' },
106+
nested: {
107+
type: 'object',
108+
properties: {
109+
value: { type: 'number' }
110+
},
111+
required: ['value']
112+
}
113+
},
114+
required: ['name']
115+
}
116+
117+
const invalidData = {
118+
nested: {
119+
value: 'not-a-number'
120+
}
121+
// missing required 'name' field
122+
}
123+
124+
describe('default ValidationConfig', () => {
125+
it('should use default error message prefix "Error:"', () => {
126+
const result = validateSchema(invalidData, testSchema)
127+
128+
expect(result.valid).toBe(false)
129+
expect(result.errors).toEqual(
130+
expect.arrayContaining([
131+
expect.stringContaining('Error:')
132+
])
133+
)
134+
})
135+
136+
it('should include path information by default', () => {
137+
const result = validateSchema(invalidData, testSchema)
138+
139+
expect(result.valid).toBe(false)
140+
expect(result.errors).toEqual(
141+
expect.arrayContaining([
142+
expect.stringContaining('(at root)'),
143+
expect.stringContaining('(at /nested/value)')
144+
])
145+
)
146+
})
147+
})
148+
149+
describe('custom ValidationConfig', () => {
150+
it('should use custom error message prefix', () => {
151+
const config = { errorMessagePrefix: 'Custom Prefix:' }
152+
const result = validateSchema(invalidData, testSchema, config)
153+
154+
expect(result.valid).toBe(false)
155+
expect(result.errors).toEqual(
156+
expect.arrayContaining([
157+
expect.stringContaining('Custom Prefix:')
158+
])
159+
)
160+
expect(result.errors).not.toEqual(
161+
expect.arrayContaining([
162+
expect.stringContaining('Error:')
163+
])
164+
)
165+
})
166+
167+
it('should exclude path information when includePathIntoErrorMessage is false', () => {
168+
const config = { includePathIntoErrorMessage: false }
169+
const result = validateSchema(invalidData, testSchema, config)
170+
171+
expect(result.valid).toBe(false)
172+
expect(result.errors).not.toEqual(
173+
expect.arrayContaining([
174+
expect.stringContaining('(at ')
175+
])
176+
)
177+
})
178+
179+
it('should use both custom prefix and exclude path information', () => {
180+
const config = {
181+
errorMessagePrefix: 'Custom Error:',
182+
includePathIntoErrorMessage: false
183+
}
184+
const result = validateSchema(invalidData, testSchema, config)
185+
186+
expect(result.valid).toBe(false)
187+
expect(result.errors).toEqual(
188+
expect.arrayContaining([
189+
expect.stringContaining('Custom Error:')
190+
])
191+
)
192+
expect(result.errors).not.toEqual(
193+
expect.arrayContaining([
194+
expect.stringContaining('(at ')
195+
])
196+
)
197+
})
198+
199+
it('should handle empty string as error message prefix', () => {
200+
const config = { errorMessagePrefix: '' }
201+
const result = validateSchema(invalidData, testSchema, config)
202+
203+
expect(result.valid).toBe(false)
204+
expect(result.errors.length).toBeGreaterThan(0)
205+
// Should not start with "Error:" but with the actual error message
206+
expect(result.errors[0]).not.toMatch(/^Error:/)
207+
})
208+
})
209+
210+
describe('ValidationConfig with exceptions', () => {
211+
it('should use custom error prefix for unknown errors', () => {
212+
const mockSchema = null // This will cause an error in AJV
213+
const config = { errorMessagePrefix: 'Schema Error:' }
214+
215+
const result = validateSchema({}, mockSchema, config)
216+
217+
expect(result.valid).toBe(false)
218+
expect(result.errors).toEqual(['Schema Error: unknown error'])
219+
})
220+
221+
it('should use default error prefix for unknown errors when no config provided', () => {
222+
const mockSchema = null // This will cause an error in AJV
223+
224+
const result = validateSchema({}, mockSchema)
225+
226+
expect(result.valid).toBe(false)
227+
expect(result.errors).toEqual(['Error: unknown error'])
228+
})
229+
})
230+
231+
describe('edge cases', () => {
232+
it('should handle valid data with custom config', () => {
233+
const validData = { name: 'test', nested: { value: 42 } }
234+
const config = {
235+
errorMessagePrefix: 'Custom Error:',
236+
includePathIntoErrorMessage: false
237+
}
238+
239+
const result = validateSchema(validData, testSchema, config)
240+
241+
expect(result).toEqual({
242+
valid: true,
243+
errors: []
244+
})
245+
})
246+
247+
it('should handle undefined config properties gracefully', () => {
248+
const config = {
249+
errorMessagePrefix: undefined,
250+
includePathIntoErrorMessage: undefined
251+
}
252+
const result = validateSchema(invalidData, testSchema, config)
253+
254+
expect(result.valid).toBe(false)
255+
// Should use defaults when undefined
256+
expect(result.errors).toEqual(
257+
expect.arrayContaining([
258+
expect.stringContaining('Error:'),
259+
expect.stringContaining('(at ')
260+
])
261+
)
262+
})
263+
})
264+
})

0 commit comments

Comments
 (0)