Skip to content

Commit

Permalink
cue: fail on required field in various cases
Browse files Browse the repository at this point in the history 
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 <mpvl@gmail.com>
Change-Id: Id028719ec329b8d586822117c7a8c3e75e9c8ee4
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/552170
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
Unity-Result: CUEcueckoo <cueckoo@cuelang.org>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
  • Loading branch information
@mpvl
mpvl committed Apr 6, 2023
1 parent 071c4ab commit e7d7c6a
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 69 deletions.
74 changes: 47 additions & 27 deletions cue/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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
Expand Down Expand Up @@ -2214,17 +2225,26 @@ 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() {
case StructKind:
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:
Expand Down
110 changes: 74 additions & 36 deletions cue/types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
})
}
}

Expand Down Expand Up @@ -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: `""`,
}, {
Expand Down Expand Up @@ -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
Expand Down
10 changes: 10 additions & 0 deletions internal/core/adt/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 1 addition & 6 deletions internal/core/validate/validate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down

0 comments on commit e7d7c6a

Please sign in to comment.