martinohmann / hcl-rs

HCL parsing and encoding libraries for rust with serde support
Apache License 2.0
122 stars 14 forks source link

Expression serialization is broken #344

Closed miller-time closed 4 months ago

miller-time commented 4 months ago

I was using hcl-rs to de-serialize an HCL file, then re-serialize an Expression assigned to an attribute. The output was simply handle = 0, so I built this small program to see if I could reproduce it. It appears that Expression always serializes as just handle = $someNumber

use hcl::Expression;

fn main() {
    let expr = Expression::from_iter([
        ("foo", Expression::from(vec![1, 2])),
        ("bar", Expression::Bool(true)),
    ]);
    serialize_expr(&expr);

    let expr = Expression::Bool(true);
    serialize_expr(&expr);

    let expr = Expression::from(vec![1, 2]);
    serialize_expr(&expr);
}

fn serialize_expr(expr: &Expression) {
    let s = hcl::to_string(expr).unwrap();
    println!("{s}");
}

output:

handle = 0

handle = 1

handle = 2

expected:

[foo = [1, 2], bar = true]           # just a guess, not sure if this is the correct serialization for this expression
true
[1, 2]

I tried to figure out what is causing this, and discovered a lot of "internal handle" code for the (de)serialization. But I couldn't quite figure out what specifically isn't working right.

miller-time commented 4 months ago

I believe it's because of this:

https://github.com/martinohmann/hcl-rs/blob/870df8335eb97df5b5a08edbd2a29102d7684dd3/crates/hcl-rs/src/expr/ser/mod.rs#L56

miller-time commented 4 months ago

While debugging I came across this comment on hcl::to_string

/// If you want to serialize the data structures provided by this crate (e.g. [`Body`]) consider
/// using [`hcl::format::to_string`](crate::format::to_string) instead because it is more
/// efficient.

The above example, when using hcl::format::to_string works!

output:

{
  "foo" = [
    1,
    2
  ]
  "bar" = true
}
true
[
  1,
  2
]

So I'm not sure why hcl::to_string and hcl::format::to_string would behave so differently, but I at least have a very simple workaround.

martinohmann commented 4 months ago

Hi there, can you tell me which version of hcl-rs you're using? This certainly looks like a bug in the internal serialization code.

martinohmann commented 4 months ago

Using hcl::to_string to serialize types that do not resemble an object is invalid because it would produce invalid HCL (at the top-level HCL expects blocks and attributes, bare expressions are not allowed). For example, serializing a bare boolean, string, number or vec will already produce an error.

For Expression, there was some error handling missing which caused internal handles to leak out instead. I prepared a PR which fixes this and makes hcl::to_string return errors for these invalid inputs.

hcl::format::to_string does not have this issue because it's meant for formatting any kind of HCL data type in a standalone way, so the resulting output here does not necessarily need to resemble a valid HCL document.

miller-time commented 4 months ago

@martinohmann Thanks for the quick reply and fix!

Hi there, can you tell me which version of hcl-rs you're using? This certainly looks like a bug in the internal serialization code.

I checked out the repo and wrote that simple program in the examples directory.

I've pulled your fix and run that program again:

foo = [
  1,
  2
]
bar = true

thread 'main' panicked at crates/hcl-rs/examples/serialize-expr.rs:18:34:
called `Result::unwrap()` on an `Err` value: Message("non-object HCL expressions are not permitted in this context")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

so this was valid (for use with hcl::to_string):

Expression::from_iter([
    ("foo", Expression::from(vec![1, 2])),
    ("bar", Expression::Bool(true)),
])

but this was not:

Expression::Bool(true)

and neither is this:

Expression::from(vec![1, 2])

what do you think about suggesting hcl::format::to_string in that error message?

miller-time commented 4 months ago

by the way, the crate where I'm using hcl-rs is hq

(think jq for HCL)

thanks for providing hcl-rs!