From e7d7c6a7aa9ba8e2024a874431558c8e5ec4e8d5 Mon Sep 17 00:00:00 2001 From: Marcel van Lohuizen Date: Tue, 4 Apr 2023 12:07:29 +0200 Subject: [PATCH] cue: fail on required field in various cases In the cue package, structValue used to keep a list of features to include. This is no longer necessary, as optional and required fields are now arcs as well. We replace it with a list of arcs instead. This allows replacement arcs to be generated to reflect a required field error when appropriate. Fixes #2309 Signed-off-by: Marcel van Lohuizen Change-Id: Id028719ec329b8d586822117c7a8c3e75e9c8ee4 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/552170 Reviewed-by: Roger Peppe Unity-Result: CUEcueckoo TryBot-Result: CUEcueckoo --- cue/types.go | 74 ++++++++++++------- cue/types_test.go | 110 +++++++++++++++++++---------- internal/core/adt/errors.go | 10 +++ internal/core/validate/validate.go | 7 +- 4 files changed, 132 insertions(+), 69 deletions(-) diff --git a/cue/types.go b/cue/types.go index ea58c65dc8f..0e8066f6523 100644 --- a/cue/types.go +++ b/cue/types.go @@ -88,10 +88,10 @@ const ( // // TODO: remove type structValue struct { - ctx *adt.OpContext - v Value - obj *adt.Vertex - features []adt.Feature + ctx *adt.OpContext + v Value + obj *adt.Vertex + arcs []*adt.Vertex } type hiddenStructValue = structValue @@ -101,18 +101,17 @@ func (o *hiddenStructValue) Len() int { if o.obj == nil { return 0 } - return len(o.features) + return len(o.arcs) } // At reports the key and value of the ith field, i < o.Len(). func (o *hiddenStructValue) At(i int) (key string, v Value) { - f := o.features[i] - return o.v.idx.LabelStr(f), newChildValue(o, i) + arc := o.arcs[i] + return o.v.idx.LabelStr(arc.Label), newChildValue(o, i) } func (o *hiddenStructValue) at(i int) *adt.Vertex { - f := o.features[i] - return o.obj.Lookup(f) + return o.arcs[i] } // Lookup reports the field for the given key. The returned Value is invalid @@ -122,7 +121,7 @@ func (o *hiddenStructValue) Lookup(key string) Value { i := 0 len := o.Len() for ; i < len; i++ { - if o.features[i] == f { + if o.arcs[i].Label == f { break } } @@ -1386,9 +1385,12 @@ func (v Value) structValOpts(ctx *adt.OpContext, o options) (s structValue, err } } + // features are topologically sorted. + // TODO(sort): make sort order part of the evaluator and eliminate this. features := export.VertexFeatures(ctx, obj) - k := 0 + arcs := make([]*adt.Vertex, 0, len(obj.Arcs)) + for _, f := range features { if f.IsLet() { continue @@ -1403,14 +1405,28 @@ func (v Value) structValOpts(ctx *adt.OpContext, o options) (s structValue, err if arc == nil { continue } - if arc.IsConstraint() && o.omitOptional { - continue + switch arc.ArcType { + case adt.ArcOptional: + if o.omitOptional { + continue + } + case adt.ArcRequired: + // We report an error for required fields if the configuration is + // final or concrete. We also do so if omitOptional is true, as + // it avoids hiding errors in required fields. + if o.omitOptional || o.concrete || o.final { + arc = &adt.Vertex{ + Label: arc.Label, + Parent: arc.Parent, + Conjuncts: arc.Conjuncts, + BaseValue: adt.NewRequiredNotPresentError(ctx, arc), + } + arc.ForceDone() + } } - features[k] = f - k++ + arcs = append(arcs, arc) } - features = features[:k] - return structValue{ctx, v, obj, features}, nil + return structValue{ctx, v, obj, arcs}, nil } // Struct returns the underlying struct of a value or an error if the value @@ -1476,8 +1492,8 @@ func (s *hiddenStruct) Field(i int) FieldInfo { // it interprets name as an arbitrary string for a regular field. func (s *hiddenStruct) FieldByName(name string, isIdent bool) (FieldInfo, error) { f := s.v.idx.Label(name, isIdent) - for i, a := range s.features { - if a == f { + for i, a := range s.arcs { + if a.Label == f { return s.Field(i), nil } } @@ -1501,12 +1517,7 @@ func (v Value) Fields(opts ...Option) (*Iterator, error) { return &Iterator{idx: v.idx, ctx: ctx}, v.toErr(err) } - arcs := []*adt.Vertex{} - for i := range obj.features { - arc := obj.at(i) - arcs = append(arcs, arc) - } - return &Iterator{idx: v.idx, ctx: ctx, val: v, arcs: arcs}, nil + return &Iterator{idx: v.idx, ctx: ctx, val: v, arcs: obj.arcs}, nil } // Lookup reports the value at a path starting from v. The empty path returns v @@ -2214,7 +2225,8 @@ func (v Value) Validate(opts ...Option) error { // Walk descends into all values of v, calling f. If f returns false, Walk // will not descent further. It only visits values that are part of the data -// model, so this excludes optional fields, hidden fields, and definitions. +// model, so this excludes definitions and optional, required, and hidden +// fields. func (v Value) Walk(before func(Value) bool, after func(Value)) { ctx := v.ctx() switch v.Kind() { @@ -2222,9 +2234,17 @@ func (v Value) Walk(before func(Value) bool, after func(Value)) { if before != nil && !before(v) { return } - obj, _ := v.structValData(ctx) + obj, _ := v.structValOpts(ctx, options{ + omitHidden: true, + omitDefinitions: true, + }) for i := 0; i < obj.Len(); i++ { _, v := obj.At(i) + // TODO: should we error on required fields, or visit them anyway? + // Walk is not designed to error at this moment, though. + if v.v.ArcType != adt.ArcMember { + continue + } v.Walk(before, after) } case ListKind: diff --git a/cue/types_test.go b/cue/types_test.go index 0f27690d5da..d759b6e25d6 100644 --- a/cue/types_test.go +++ b/cue/types_test.go @@ -798,6 +798,9 @@ func TestFields(t *testing.T) { if step1.value > 100 { }`, err: "undefined field: value", + }, { + value: `{a!: 1, b?: 2, c: 3}`, + err: "a: field is required but not present", }} for _, tc := range testCases { t.Run(tc.value, func(t *testing.T) { @@ -895,6 +898,11 @@ func TestLookup(t *testing.T) { [string]: int64 } & #V v: #X + +a: { + b!: 1 + c: 2 +} `) if err != nil { t.Fatalf("compile: %v", err) @@ -904,48 +912,74 @@ v: #X // log.Fatalf("parseExpr: %v", err) // } // v := inst.Eval(expr) - testCases := []struct { - ref []string - raw string - eval string - }{{ - ref: []string{"v", "x"}, - raw: ">=-9223372036854775808 & <=9223372036854775807 & int", - eval: "int64", + + type testCase struct { + ref []string + result string + syntax string + } + testCases := []testCase{{ + ref: []string{"a"}, + result: `{ + b!: 1 + c: 2 +}`, + syntax: "{b!: 1, c: 2}", + }, { + // Allow descending into structs even if it has a required field error. + ref: []string{"a", "c"}, + result: "2", + syntax: "2", + }, { + ref: []string{"a", "b"}, + result: "_|_ // a.b: field is required but not present", + syntax: "1", + }, { + ref: []string{"v", "x"}, + result: "int64", + syntax: "int64", }} for _, tc := range testCases { - v := inst.Lookup(tc.ref...) - - if got := fmt.Sprintf("%#v", v); got != tc.raw { - t.Errorf("got %v; want %v", got, tc.raw) - } + t.Run("", func(t *testing.T) { + v := inst.Lookup(tc.ref...) - got := fmt.Sprint(astinternal.DebugStr(v.Eval().Syntax())) - if got != tc.eval { - t.Errorf("got %v; want %v", got, tc.eval) - } + if got := fmt.Sprintf("%+v", v); got != tc.result { + t.Errorf("got %v; want %v", got, tc.result) + } - v = inst.Lookup() - for _, ref := range tc.ref { - s, err := v.Struct() - if err != nil { - t.Fatal(err) + got := fmt.Sprint(astinternal.DebugStr(v.Eval().Syntax())) + if got != tc.syntax { + t.Errorf("got %v; want %v", got, tc.syntax) } - fi, err := s.FieldByName(ref, false) - if err != nil { - t.Fatal(err) + + v = inst.Lookup() + for _, ref := range tc.ref { + s, err := v.Struct() + if err != nil { + t.Fatal(err) + } + fi, err := s.FieldByName(ref, false) + if err != nil { + t.Fatal(err) + } + v = fi.Value + + // Struct gets all fields. Skip tests with optional fields, + // as the result will differ. + if v.v.ArcType != adt.ArcMember { + return + } } - v = fi.Value - } - if got := fmt.Sprintf("%#v", v); got != tc.raw { - t.Errorf("got %v; want %v", got, tc.raw) - } + if got := fmt.Sprintf("%+v", v); got != tc.result { + t.Errorf("got %v; want %v", got, tc.result) + } - got = fmt.Sprint(astinternal.DebugStr(v.Eval().Syntax())) - if got != tc.eval { - t.Errorf("got %v; want %v", got, tc.eval) - } + got = fmt.Sprint(astinternal.DebugStr(v.Eval().Syntax())) + if got != tc.syntax { + t.Errorf("got %v; want %v", got, tc.syntax) + } + }) } } @@ -2594,11 +2628,12 @@ func docStr(docs []*ast.CommentGroup) string { // TODO: unwrap marshal error // TODO: improve error messages func TestMarshalJSON(t *testing.T) { - testCases := []struct { + type testCase struct { value string json string err string - }{{ + } + testCases := []testCase{{ value: `""`, json: `""`, }, { @@ -2662,6 +2697,9 @@ func TestMarshalJSON(t *testing.T) { }, { value: `{foo?: 1, bar?: 2, baz: 3}`, json: `{"baz":3}`, + }, { + value: `{foo!: 1, bar: 2}`, + err: "cue: marshal error: foo: field is required but not present", }, { // Has an unresolved cycle, but should not matter as all fields involved // are optional diff --git a/internal/core/adt/errors.go b/internal/core/adt/errors.go index 50ca0db42d3..2565c9d3fd2 100644 --- a/internal/core/adt/errors.go +++ b/internal/core/adt/errors.go @@ -206,6 +206,16 @@ func CombineErrors(src ast.Node, x, y Value) *Bottom { } } +func NewRequiredNotPresentError(ctx *OpContext, v *Vertex) *Bottom { + saved := ctx.PushArc(v) + b := &Bottom{ + Code: IncompleteError, + Err: ctx.Newf("field is required but not present"), + } + ctx.PopArc(saved) + return b +} + // A ValueError is returned as a result of evaluating a value. type ValueError struct { r Runtime diff --git a/internal/core/validate/validate.go b/internal/core/validate/validate.go index 4912ae54652..66cae481b1c 100644 --- a/internal/core/validate/validate.go +++ b/internal/core/validate/validate.go @@ -99,12 +99,7 @@ func (v *validator) validate(x *adt.Vertex) { for _, a := range x.Arcs { if a.ArcType == adt.ArcRequired && v.inDefinition == 0 { - saved := v.ctx.PushArc(a) - v.add(&adt.Bottom{ - Code: adt.IncompleteError, - Err: v.ctx.Newf("field is required but not present"), - }) - v.ctx.PopArc(saved) + v.add(adt.NewRequiredNotPresentError(v.ctx, a)) continue }