zclconf / go-cty

A type system for dynamic values in Go applications
MIT License
338 stars 70 forks source link

Some valid `Path` values cannot be applied to their source `Value` #180

Open jbardin opened 3 months ago

jbardin commented 3 months ago

A Value containing a set cannot use the path argument within a Transform callback for all paths.

Given an object like so:

ObjectVal(map[string]Value{
  "set": SetVal([]Value{
    ObjectVal(map[string]Value{
      "attr": StringVal("val"),
    }),
  }),
})

Using Transform to obtain each step's value from another object will encounter a path which cannot be used.

// using the same value to ensure all paths are valid, and should result in the same value
Transform(val, func(p Path, v Value) (Value, error) {
    return p.Apply(val)
})

This results in at step 1: key value not number or string when the path is for

cty.Path{cty.GetAttrStep{Name:"set"}, cty.IndexStep{Key:cty.ObjectVal(map[string]cty.Value{"attr":cty.StringVal("val")})}, cty.GetAttrStep{Name:"attr"}}

I'm not sure how we would intend to handle this. Sets are not currently index-able, so simply skipping the key value not number or string check doesn't work. Should we make all Path values valid for Apply on a correctly shaped Value, have special handling to detect when a set is encountered in the path, or maybe just document this as a potential hazard within Transform and Walk?

t0yv0 commented 3 weeks ago

Running into this as well. Since transform encodes elements by pushing the entire element into IndexStep, this code may need to be made to handle it:

func (s IndexStep) Apply(val Value) (Value, error) {
    if val == NilVal || val.IsNull() {
        return NilVal, errors.New("cannot index a null value")
    }

    switch s.Key.Type() {
    case Number:
        if !(val.Type().IsListType() || val.Type().IsTupleType()) {
            return NilVal, errors.New("not a list type")
        }
    case String:
        if !val.Type().IsMapType() {
            return NilVal, errors.New("not a map type")
        }
    default:
        return NilVal, errors.New("key value not number or string")
    }

    has := val.HasIndex(s.Key)
    if !has.IsKnown() {
        return UnknownVal(val.Type().ElementType()), nil
    }
    if !has.True() {
        return NilVal, errors.New("value does not have given index key")
    }

    return val.Index(s.Key), nil
}

What's available:

type Set struct {
    vals  map[int][]interface{}
    rules Rules
}
// Rules represents the operations that define membership for a Set.
//
// Each Set has a Rules instance, whose methods must satisfy the interface
// contracts given below for any value that will be added to the set.
type Rules interface {
    // Hash returns an int that somewhat-uniquely identifies the given value.
    //
    // A good hash function will minimize collisions for values that will be
    // added to the set, though collisions *are* permitted. Collisions will
    // simply reduce the efficiency of operations on the set.
    Hash(interface{}) int

    // Equivalent returns true if and only if the two values are considered
    // equivalent for the sake of set membership. Two values that are
    // equivalent cannot exist in the set at the same time, and if two
    // equivalent values are added it is undefined which one will be
    // returned when enumerating all of the set members.
    //
    // Two values that are equivalent *must* result in the same hash value,
    // though it is *not* required that two values with the same hash value
    // be equivalent.
    Equivalent(interface{}, interface{}) bool
}

So IndexStep{v}.Apply(Set{}) should probably try to use the Rules on the set to find an equivalent value to v.

t0yv0 commented 3 weeks ago

Possible fix in https://github.com/zclconf/go-cty/pull/183