Inline "if" causes objects casted to maps, and corrupts data types

mkielar commented 1 year ago

Terraform Version

1.3.7

Terraform Configuration Files

N/A, just open terraform console

Debug Output

In terraforfm console run:

> true ? { x: 1234, y: true, z: "haha", w: [1,2,3] } : null
{
"w" = [
1,
2,
3,
]
"x" = 1234
"y" = true
"z" = "haha"
}

Observe that the returned object has all the attributes correctly typed.

In terraform console run:

> true ? { x: 1234, y: true, z: "haha", w: [1,2,3] } : {}
╷
│ Error: Inconsistent conditional result types
│
│   on <console-input> line 1:
│   (source code not available)
│
│ The true and false result expressions must have consistent types. The 'true' value includes object attribute "w", which is absent in the 'false' value.
╵

It seems terraform is trying to do something weird... Like it would try to cast the first value into a map, with all values having the same type? Or same keys? The error message in this case is completely confusing.

In terraform console, run:
```
> true ? { x: 1234, y: true, z: "haha" } : {}
tomap({
"x" = "1234"
"y" = "true"
"z" = "haha"
})
```
Observe, that the value returned is in fact a map, not an object. But why? Also, all the values in the map have been casted to string!
In terraform console run:
```
> true ? { x: 1234, y: true, z: "haha" } : toobject({})
{
"x" = 1234
"y" = true
"z" = "haha"
}
```
Observe that this time, terraform returned an object. The toobject({}) would cause error if executed (because there's no such function), but in this case, terraform somehow managed to figure out the first value is an object?

Expected Behavior

In each case, terraform should return an object, and never should cast the values to string.

Actual Behavior

Actual behaviour is weird and confusing.

In ex.1, it correctly assumes the return type of that expression is supposed to be the object, and returns it without performing any casting
In ex.2 and ex. 3, however, it assumes the return type should be a map, even though it has the object with multiple types of arguments right there, and it's the object that is being returned (because of the evaluated condition). I'm assuming terraform looks at the second value of the conditional expression to inspect it's type and based on that it tries to cast the object to map, instead of the empty map to object. I believe this is a bug - as a result of this behaviour, terraform is actively corrupting data, by converting values inside of the object to common type (here: string).
Even more confusing is the fact, that terraform fails in ex.2 (not being able to perform the conversion), but not in ex. 4. It's clear from ex. 2 and ex. 3 that terraform performs some kind of evaluation of the second value in the conditional expression to figure out the type of the return value. Why, then, in ex. 4, doesn't terraform fail, not being to evaluate the type of the second value? I would expect terraform to fail in ex. 4 due to toobject() not being defined, but no such thing happens.

Steps to Reproduce

See "Debug output".

Additional Context

I spotted that when building payloads for input argument of aws_cloudwatch_event_target. I have a bunch of EventRule Schedules, and I noticed the JSON documents I pass as Lambda Events in these Rules have all values casted to string, regardless of what I set in terraform. I even tried explicit casts using tonumber and tobool, only to see them all converted into strings.

This was my setup:

locals {
  event_payload = merge(

    # Generic part. This goes into every event.
    {
      type = var.scaler
    },

    # Configuration for ECS Services.
    var.scaler == "ecs_service" ? {
      cluster_arn   = var.scaler_configuration.cluster_arn
      service_arn   = var.scaler_configuration.service_arn
      action        = var.scaler_configuration.action
      desired_count = tonumber(var.scaler_configuration.desired_count)
    } : {},

    # Configuration for ECS Services, when using Min/Max method.
    var.scaler == "ecs_service_min_max" ? {
      cluster_arn             = var.scaler_configuration.cluster_arn
      service_arn             = var.scaler_configuration.service_arn
      min_count               = tonumber(var.scaler_configuration.min_count)
      max_count               = tonumber(var.scaler_configuration.max_count)
      force_scale_up_to_min   = tobool(var.scaler_configuration.force_scale_up_to_min)
      force_scale_down_to_max = tobool(var.scaler_configuration.force_scale_down_to_max)
    } : {},

  )
}

resource "aws_cloudwatch_event_target" "this" {
  input = jsonencode(local.event_payload)

  # ...
}

After I changed the } : {} lines to } : null it all magically started to work as expected. But the fact that it didn't in the first place is terribly confusing. Also, I'm now reviewing, fixing and adding code comments in places like this across my entire codebase, so that noone would accidentally "fix" the ? { ... } : null expressions back into ? { ... } : {}, and break stuff.

References

No response

mkielar commented 1 year ago

I find at least few things problematic with the above:

When terraform decides to fall back to map type, it converts the values into that map. In that case, it converts them all to string which allows converting bools and numbers. So in that case, terraform makes a decision to fall-back to a less restrictive datatype. However, when faced with <expr> ? <object> : {} it somehow identifies the {} as map, and decides to fall-back to map type. But out of the two (map, object) the less restrictive one would be the object, not the map. So why does terraform sometimes fall-back to _less restrictive` datatype, and sometimes to more restrictive one? That's inconsistent and error prone.
I'm struggling to understand why {} is a map, and not an empty object. Especially, when there is a tomap() function - which convers objects to maps - but there's no toobject() function that would do otherwise. Lack of toobject() and existence of tomap() suggests that anything I define using curly braces should initially be of object type, and only explicit conversion using tomap() or implicit conversion when using in contexts like merge() should trigger conversion. Instead, { x: 1, y: "foo"} is an object, and { x: 1 } is an object, and any other combination of attributes is always an object, even type({}) returns object({}). But when {} is used in a inline "if" expression, it is somehow a map, and forces conversion of the other value in the expression? It should not be like that...

jbardin commented 1 year ago

Hi @mkielar,

I think the misunderstanding here is that {} is an empty object, not a map, and that is the cause of your problem. The expression must always result in a single type, and because the value { x: 1234, y: true, z: "haha" } is not the same type as {}, terraform must try convert them to the same type, and the only possible type for unification here is a map(string). If no unification is possible, then you get the Inconsistent conditional result types error.

We have an open enhancement request here to work on clarifying the type unification process in #28337, and #29622 to try and make the errors easier to understand in more complex situations.

Thanks!

mkielar commented 1 year ago

@jbardin can you please elaborate a bit more on how the value { x: 1234, y: true, z: "haha" } is not the same type as {}? When I run type() function on both of them, it returns object in both cases...

jbardin commented 1 year ago

When you use the console type() function the output for these is different:

> type({ x: 1234, y: true, z: "haha" })
object({
    x: number,
    y: bool,
    z: string,
})
> type({})
object({})

object is a kind of type, but object({}) is a different structural type from object({x: number}) and from object({x: number, y: bool}) and so on.

github-actions[bot] commented 1 year ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

hashicorp / terraform