Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jbardin opened this issue Mar 28, 2024 · 2 comments · May be fixed by #183
Open

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

jbardin opened this issue Mar 28, 2024 · 2 comments · May be fixed by #183

Comments

@jbardin
Copy link
Contributor

jbardin commented Mar 28, 2024

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
Copy link

t0yv0 commented Jun 12, 2024

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
Copy link

t0yv0 commented Jun 12, 2024

Possible fix in #183

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants