json: ImpliedType should return an error if an object includes the same property name twice

[apparentlymart]

The json.ImpliedType function is a helper to infer a cty.Type that a given JSON payload ought to be able to decode into without loss. This is intended for situations where applications are accepting arbitrary JSON without any fixed type/schema in mind, and an example in this codebase is the function/stdlib implementation of JSONDecode:

go-cty/cty/function/stdlib/json.go

Lines 123 to 124 in 0ed0ebb

	buf := []byte(str.AsString())
	return json.ImpliedType(buf)

The ImpliedType implementation deals with objects on a property-by-property basis, with the intend of finding the full set of attribute names that are included in the object and a suitable type constraint to use for each one:

go-cty/cty/json/type_implied.go

Lines 95 to 137 in 0ed0ebb

	var atys map[string]cty.Type
	for {
	// Read the object key first
	tok, err := dec.Token()
	if err != nil {
	return cty.NilType, err
	}
	if ttok, ok := tok.(json.Delim); ok {
	if rune(ttok) != '}' {
	return cty.NilType, fmt.Errorf("unexpected delimiter %q", ttok)
	}
	break
	}
	key, ok := tok.(string)
	if !ok {
	return cty.NilType, fmt.Errorf("expected string but found %T", tok)
	}
	// Now read the value
	tok, err = dec.Token()
	if err != nil {
	return cty.NilType, err
	}
	aty, err := impliedTypeForTok(tok, dec)
	if err != nil {
	return cty.NilType, err
	}
	if atys == nil {
	atys = make(map[string]cty.Type)
	}
	atys[key] = aty
	}
	if len(atys) == 0 {
	return cty.EmptyObject, nil
	}
	return cty.Object(atys), nil

Currently this loop doesn't check whether atys already contains an element for the given key, which means that a JSON object with two properties that have the same name is considered to imply a type that expects that attribute to be of the type of whatever was the final occurrence of that property name in a given object.

For example, given this input:

{
  "a": "hello",
  "a": true
}

The result would be cty.Object(map[string]cty.Type{"a": cty.Bool}).

A subsequent attempt to translate that input into a cty.Value using json.Unmarshal with that returned type would fail, because it would try to convert "hello" from the first "a" to a cty.Bool, which is impossible.

The ImpliedType function should probably have just immediately rejected duplicate property names, because although they are technically valid in JSON syntax, they have unspecified meaning and for the purposes of json.ImpliedType and json.Unmarshal they can't really have any significant meaning due to the constraints of cty's own definition of object types.

However, it is technically possible today to provide an object with duplicate properties that are all of the same type and have that "work" in some sense:

{
  "a": "hello",
  "a": "world"
}

The implied type is cty.Object(map[string]cty.Type{"a": cty.String}). and the Unmarshal function is also happy to first convert "hello" to string, then convert "world" to string to replace it, thereby returning cty.ObjectVal(map[string]cty.Value{"a": cty.StringVal("world")}) with no errors.

Given that, to preserve the current valid case (even though it's dubious whether that valid result is actually useful) I think the new behavior should be for json.ImpliedType to return an error if it finds two properties of the same name of incompatible types, but to continue to accept duplicates if their types are compatible. That therefore would narrowly introduce a better error message for the case that can't work without breaking the case that does currently work.

(It is technically still "breaking" of an application that was expecting to be able to json.ImpliedType successfully without subsequently calling json.Unmarshal, since the error only appears when both are combined as in our stdlib.JSONDecode function, but that particular scenario seems marginal enough that I think it's worth breaking it to improve the messaging for the more likely situation.)

[apparentlymart]

Fixed in 1c48de3.