Skip to content

Commit fda0dd5

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #131 from jennybuckley/error-message
Fix useless error message 'invalid atom'
2 parents 4c5ecdc + 51173b8 commit fda0dd5

File tree

4 files changed

+268
-10
lines changed

4 files changed

+268
-10
lines changed

‎typed/helpers.go

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -114,17 +114,27 @@ func resolveSchema(s *schema.Schema, tr schema.TypeRef, v *value.Value, ah atomH
114114
return handleAtom(a, tr, ah)
115115
}
116116

117-
func deduceAtom(a schema.Atom, v *value.Value) schema.Atom {
117+
// deduceAtom determines which of the possible types in atom 'atom' applies to value 'val'.
118+
// If val is of a type allowed by atom, return a copy of atom with all other types set to nil.
119+
// if val is nil, or is not of a type allowed by atom, just return the original atom,
120+
// and validation will fail at a later stage. (with a more useful error)
121+
func deduceAtom(atom schema.Atom, val *value.Value) schema.Atom {
118122
switch {
119-
case v == nil:
120-
case v.FloatValue != nil, v.IntValue != nil, v.StringValue != nil, v.BooleanValue != nil:
121-
return schema.Atom{Scalar: a.Scalar}
122-
case v.ListValue != nil:
123-
return schema.Atom{List: a.List}
124-
case v.MapValue != nil:
125-
return schema.Atom{Map: a.Map}
123+
case val == nil:
124+
case val.FloatValue != nil, val.IntValue != nil, val.StringValue != nil, val.BooleanValue != nil:
125+
if atom.Scalar != nil {
126+
return schema.Atom{Scalar: atom.Scalar}
127+
}
128+
case val.ListValue != nil:
129+
if atom.List != nil {
130+
return schema.Atom{List: atom.List}
131+
}
132+
case val.MapValue != nil:
133+
if atom.Map != nil {
134+
return schema.Atom{Map: atom.Map}
135+
}
126136
}
127-
return a
137+
return atom
128138
}
129139

130140
func handleAtom(a schema.Atom, tr schema.TypeRef, ah atomHandler) ValidationErrors {

‎typed/symdiff_test.go

Lines changed: 238 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,244 @@ var symdiffCases = []symdiffTestCase{{
299299
modified: _NS(),
300300
added: _NS(_P("a", "b")),
301301
}},
302+
}, {
303+
name: "untyped deduced",
304+
rootTypeName: "__untyped_deduced_",
305+
schema: `types:
306+
- name: __untyped_atomic_
307+
scalar: untyped
308+
list:
309+
elementType:
310+
namedType: __untyped_atomic_
311+
elementRelationship: atomic
312+
map:
313+
elementType:
314+
namedType: __untyped_atomic_
315+
elementRelationship: atomic
316+
- name: __untyped_deduced_
317+
scalar: untyped
318+
list:
319+
elementType:
320+
namedType: __untyped_atomic_
321+
elementRelationship: atomic
322+
map:
323+
elementType:
324+
namedType: __untyped_deduced_
325+
elementRelationship: separable
326+
`,
327+
quints: []symdiffQuint{{
328+
lhs: `{"a":{}}}`,
329+
rhs: `{"a":{"b":{}}}`,
330+
removed: _NS(),
331+
modified: _NS(),
332+
added: _NS(_P("a", "b")),
333+
}, {
334+
lhs: `{"a":null}`,
335+
rhs: `{"a":{"b":{}}}`,
336+
removed: _NS(),
337+
modified: _NS(),
338+
added: _NS(_P("a", "b")),
339+
}, {
340+
lhs: `{"a":{"b":{}}}`,
341+
rhs: `{"a":{}}}`,
342+
removed: _NS(_P("a", "b")),
343+
modified: _NS(),
344+
added: _NS(),
345+
}, {
346+
lhs: `{"a":{"b":{}}}`,
347+
rhs: `{"a":null}`,
348+
removed: _NS(_P("a", "b")),
349+
modified: _NS(),
350+
added: _NS(),
351+
}, {
352+
lhs: `{"a":[]}`,
353+
rhs: `{"a":["b"]}`,
354+
removed: _NS(),
355+
modified: _NS(_P("a")),
356+
added: _NS(),
357+
}, {
358+
lhs: `{"a":null}`,
359+
rhs: `{"a":["b"]}`,
360+
removed: _NS(),
361+
modified: _NS(_P("a")),
362+
added: _NS(),
363+
}, {
364+
lhs: `{"a":["b"]}`,
365+
rhs: `{"a":[]}`,
366+
removed: _NS(),
367+
modified: _NS(_P("a")),
368+
added: _NS(),
369+
}, {
370+
lhs: `{"a":["b"]}`,
371+
rhs: `{"a":null}`,
372+
removed: _NS(),
373+
modified: _NS(_P("a")),
374+
added: _NS(),
375+
}, {
376+
lhs: `{"a":null}`,
377+
rhs: `{"a":"b"}`,
378+
removed: _NS(),
379+
modified: _NS(_P("a")),
380+
added: _NS(),
381+
}, {
382+
lhs: `{"a":"b"}`,
383+
rhs: `{"a":null}`,
384+
removed: _NS(),
385+
modified: _NS(_P("a")),
386+
added: _NS(),
387+
}, {
388+
lhs: `{"a":{"b":{}}}`,
389+
rhs: `{"a":["b"]}}`,
390+
removed: _NS(_P("a", "b")),
391+
modified: _NS(_P("a")),
392+
added: _NS(),
393+
}, {
394+
lhs: `{"a":["b"]}}`,
395+
rhs: `{"a":{"b":{}}}`,
396+
removed: _NS(),
397+
modified: _NS(_P("a")),
398+
added: _NS(_P("a", "b")),
399+
}, {
400+
lhs: `{"a":{"b":{}}}`,
401+
rhs: `{"a":"b"}`,
402+
removed: _NS(_P("a", "b")),
403+
modified: _NS(_P("a")),
404+
added: _NS(),
405+
}, {
406+
lhs: `{"a":"b"}`,
407+
rhs: `{"a":{"b":{}}}`,
408+
removed: _NS(),
409+
modified: _NS(_P("a")),
410+
added: _NS(_P("a", "b")),
411+
}, {
412+
lhs: `{"a":["b"]}}`,
413+
rhs: `{"a":"b"}`,
414+
removed: _NS(),
415+
modified: _NS(_P("a")),
416+
added: _NS(),
417+
}, {
418+
lhs: `{"a":"b"}`,
419+
rhs: `{"a":["b"]}}`,
420+
removed: _NS(),
421+
modified: _NS(_P("a")),
422+
added: _NS(),
423+
}},
424+
}, {
425+
name: "untyped separable",
426+
rootTypeName: "__untyped_separable_",
427+
schema: `types:
428+
- name: __untyped_separable_
429+
scalar: untyped
430+
list:
431+
elementType:
432+
namedType: __untyped_separable_
433+
elementRelationship: associative
434+
map:
435+
elementType:
436+
namedType: __untyped_separable_
437+
elementRelationship: separable
438+
`,
439+
quints: []symdiffQuint{{
440+
lhs: `{"a":{}}}`,
441+
rhs: `{"a":{"b":{}}}`,
442+
removed: _NS(),
443+
modified: _NS(),
444+
added: _NS(_P("a", "b")),
445+
}, {
446+
lhs: `{"a":null}`,
447+
rhs: `{"a":{"b":{}}}`,
448+
removed: _NS(),
449+
modified: _NS(),
450+
added: _NS(_P("a", "b")),
451+
}, {
452+
lhs: `{"a":{"b":{}}}`,
453+
rhs: `{"a":{}}}`,
454+
removed: _NS(_P("a", "b")),
455+
modified: _NS(),
456+
added: _NS(),
457+
}, {
458+
lhs: `{"a":{"b":{}}}`,
459+
rhs: `{"a":null}`,
460+
removed: _NS(_P("a", "b")),
461+
modified: _NS(),
462+
added: _NS(),
463+
}, {
464+
lhs: `{"a":[]}`,
465+
rhs: `{"a":["b"]}`,
466+
removed: _NS(),
467+
modified: _NS(),
468+
added: _NS(_P("a", _SV("b"))),
469+
}, {
470+
lhs: `{"a":null}`,
471+
rhs: `{"a":["b"]}`,
472+
removed: _NS(),
473+
// TODO: result should be the same as the previous case
474+
// nothing shoule be modified here.
475+
modified: _NS(_P("a")),
476+
added: _NS(_P("a", _SV("b"))),
477+
}, {
478+
lhs: `{"a":["b"]}`,
479+
rhs: `{"a":[]}`,
480+
removed: _NS(_P("a", _SV("b"))),
481+
modified: _NS(),
482+
added: _NS(),
483+
}, {
484+
lhs: `{"a":["b"]}`,
485+
rhs: `{"a":null}`,
486+
removed: _NS(_P("a", _SV("b"))),
487+
// TODO: result should be the same as the previous case
488+
// nothing shoule be modified here.
489+
modified: _NS(_P("a")),
490+
added: _NS(),
491+
}, {
492+
lhs: `{"a":null}`,
493+
rhs: `{"a":"b"}`,
494+
removed: _NS(),
495+
modified: _NS(_P("a")),
496+
added: _NS(),
497+
}, {
498+
lhs: `{"a":"b"}`,
499+
rhs: `{"a":null}`,
500+
removed: _NS(),
501+
modified: _NS(_P("a")),
502+
added: _NS(),
503+
}, {
504+
lhs: `{"a":{"b":{}}}`,
505+
rhs: `{"a":["b"]}}`,
506+
removed: _NS(_P("a", "b")),
507+
modified: _NS(),
508+
added: _NS(_P("a", _SV("b"))),
509+
}, {
510+
lhs: `{"a":["b"]}}`,
511+
rhs: `{"a":{"b":{}}}`,
512+
removed: _NS(_P("a", _SV("b"))),
513+
modified: _NS(),
514+
added: _NS(_P("a", "b")),
515+
}, {
516+
lhs: `{"a":{"b":{}}}`,
517+
rhs: `{"a":"b"}`,
518+
removed: _NS(_P("a", "b")),
519+
modified: _NS(_P("a")),
520+
added: _NS(),
521+
}, {
522+
lhs: `{"a":"b"}`,
523+
rhs: `{"a":{"b":{}}}`,
524+
removed: _NS(),
525+
modified: _NS(_P("a")),
526+
added: _NS(_P("a", "b")),
527+
}, {
528+
lhs: `{"a":["b"]}}`,
529+
rhs: `{"a":"b"}`,
530+
removed: _NS(_P("a", _SV("b"))),
531+
modified: _NS(_P("a")),
532+
added: _NS(),
533+
}, {
534+
lhs: `{"a":"b"}`,
535+
rhs: `{"a":["b"]}}`,
536+
removed: _NS(),
537+
modified: _NS(_P("a")),
538+
added: _NS(_P("a", _SV("b"))),
539+
}},
302540
}, {
303541
name: "struct grab bag",
304542
rootTypeName: "myStruct",

‎typed/validate.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,11 @@ func (v *validatingObjectWalker) visitMapItems(t *schema.Map, m *value.Map) (err
198198
} else {
199199
v2 := v.prepareDescent(pe, t.ElementType)
200200
v2.value = item.Value
201-
errs = append(errs, v2.validate()...)
201+
if (t.ElementType == schema.TypeRef{}) {
202+
errs = append(errs, v2.errorf("field not declared in schema")...)
203+
} else {
204+
errs = append(errs, v2.validate()...)
205+
}
202206
v2.doNode()
203207
v.finishDescent(v2)
204208
}

‎typed/validate_test.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package typed_test
1818

1919
import (
2020
"fmt"
21+
"strings"
2122
"testing"
2223

2324
"sigs.k8s.io/structured-merge-diff/schema"
@@ -216,6 +217,8 @@ var validationCases = []validationTestCase{{
216217
invalidObjects: []typed.YAMLObject{
217218
`{"key":true,"value":1}`,
218219
`{"list":{"key":true,"value":1}}`,
220+
`{"list":true}`,
221+
`true`,
219222
`{"list":[{"key":true,"value":1}]}`,
220223
`{"list":[{"key":[],"value":1}]}`,
221224
`{"list":[{"key":{},"value":1}]}`,
@@ -261,6 +264,9 @@ func (tt validationTestCase) test(t *testing.T) {
261264
if err == nil {
262265
t.Errorf("Object should fail: %v\n%v", err, iv)
263266
}
267+
if strings.Contains(err.Error(), "invalid atom") {
268+
t.Errorf("Error should be useful, but got: %v\n%v", err, iv)
269+
}
264270
})
265271
}
266272
}

0 commit comments

Comments
 (0)