hashicorp / hcl

HCL is the HashiCorp configuration language.
Mozilla Public License 2.0
5.24k stars 591 forks source link

Slices of structs not supported without a key a some sort.. #164

Open abourget opened 7 years ago

abourget commented 7 years ago

This is a failing test:

func TestDecode_sliceIntoSameObject(t *testing.T) {
    type Bar struct {
        Val  string
        Val2 string
    }
    type Feat struct {
        Bars []Bar `hcl:"bar"`
    }

    var actual Feat

    err := Decode(&actual, `
bar {
  val = "hello"
  val2 = "world"
}
bar {
  val = "hello2"
}
`)
    if err != nil {
        t.Fatalf("err: %s", err)
    }

    expected := Feat{
        Bars: []Bar{
            Bar{
                Val:  "hello",
                Val2: "hello",
            },
            Bar{
                Val: "hello2",
            },
        },
    }

    if !reflect.DeepEqual(actual, expected) {
        fmt.Println("Actual")
        spew.Dump(actual)
        fmt.Println("Expected")
        spew.Dump(expected)
        t.Fatalf("Actual: %#v\n\nExpected: %#v", actual, expected)
    }
}

Outputs:

Actual
(hcl.Feat) {
 Bars: ([]hcl.Bar) (len=3 cap=4) {
  (hcl.Bar) {
   Val: (string) (len=5) "hello",
   Val2: (string) ""
  },
  (hcl.Bar) {
   Val: (string) "",
   Val2: (string) (len=5) "world"
  },
  (hcl.Bar) {
   Val: (string) (len=6) "hello2",
   Val2: (string) ""
  }
 }
}
Expected
(hcl.Feat) {
 Bars: ([]hcl.Bar) (len=2 cap=2) {
  (hcl.Bar) {
   Val: (string) (len=5) "hello",
   Val2: (string) (len=5) "hello"
  },
  (hcl.Bar) {
   Val: (string) (len=6) "hello2",
   Val2: (string) ""
  }
 }
}

Expected behavior

I would have expected each bar statements to be decoded to their own Bar instance.

Actual behavior

Uses of this sort of structure seem to always require a key like:

bar "some" {
  val = "hello"
  val2 = "world"
}
bar "random_key" {
  val = "hello2"
}

in the current design.

Is this intended behavior, and it is something we want to support ? I assumed the lib would do that for me when I only specified a []Bar. I'd still like to use the simpler syntax.

sethvargo commented 7 years ago

Hi there,

This is parsing correctly, and is a common source of confusion. Everything is decoded into an array of objects, since it's completely valid HCL to specify a stanza multiple times.

abourget commented 7 years ago

But shouldn't it respect the nesting?

abourget commented 7 years ago

I'd expect things defined inside a rule block to belong to that rule block. Shouldn't I?

sethvargo commented 7 years ago

It looks like this might be a bug. I deduced it into a smaller repro:

package main

import (
    "encoding/json"
    "fmt"
    "log"

    "github.com/hashicorp/hcl"
)

const str = `
a {
  b {
    c = "d"
  }
}
`

func main() {
    var a A
    if err := hcl.Decode(&a, str); err != nil {
        log.Fatal(err)
    }

    out, _ := json.MarshalIndent(a, "", "  ")
    fmt.Printf("%s\n", out)
}

type A struct {
    B []*B `hcl:"b"`
}

type B struct {
    C []*C `hcl:"c"`
}

type C struct {
    D string `hcl:"d"`
}

It looks like the decoder isn't automatically creating pointers, leaving the result as null. If I explicitly create all the sub-objects in the struct, it works:

a.B = []*B{&B{C: []*C{}}}
abourget commented 7 years ago

Tried with revisions from a few months back.. and it's still the same.

abourget commented 7 years ago

Is it possible that https://github.com/hashicorp/hcl/blob/master/decoder.go#L397 passing an indirected pointer (so a struct), passes by copy ? I'm not sure with these reflect wrappers there if things decoded further will be updating the val in there.

abourget commented 7 years ago

Might be 2 bugs.. or the bug might have 2 separate effects. I removed any pointer from the equation to be sure.

I've narrowed my case to:

func TestDecode_sliceIntoSameObject(t *testing.T) {
    type Bar struct {
        Val  string
        Val2 string
    }
    type Feat struct {
        Bars []Bar `hcl:"bar"`
    }

    var actual Feat

    err := Decode(&actual, `
bar {
  val = "hello"
  val2 = "world"
}
`)
    if err != nil {
        t.Fatalf("err: %s", err)
    }

    expected := Feat{
        Bars: []Bar{
            Bar{
                Val:  "hello",
                Val2: "hello",
            },
        },
    }

    if !reflect.DeepEqual(actual, expected) {
        fmt.Println("Actual")
        spew.Dump(actual)
        fmt.Println("Expected")
        spew.Dump(expected)
        t.Fatalf("Actual: %#v\n\nExpected: %#v", actual, expected)
    }
}

This test case fails, but looks like it should compile as intended.

Outputs:

Actual
(hcl.Feat) {
 Bars: ([]hcl.Bar) (len=2 cap=2) {
  (hcl.Bar) {
   Val2: (string) "",
   Val: (string) (len=5) "hello"
  },
  (hcl.Bar) {
   Val2: (string) (len=5) "world",
   Val: (string) ""
  }
 }
}
Expected
(hcl.Feat) {
 Bars: ([]hcl.Bar) (len=1 cap=1) {
  (hcl.Bar) {
   Val2: (string) (len=5) "hello",
   Val: (string) (len=5) "hello"
  }
 }
}
abourget commented 7 years ago

Code here https://github.com/hashicorp/hcl/blob/master/decoder.go#L422-L428 will read the sub-nodes as a list of hcl.Bar indeed.. is that intended behavior ?

Also, I see in the decoder_test.go the ,expand tag suffix being used, but nowhere in the code is that read or taken into account.

I see usage here: https://gowalker.org/github.com/ChrisMcKenzie/dropship/dropship and in the TestDecode_structureArray test in decoder_test.go, that the supported format is:

rule "key" {
  something = true
}
rule "" {
  something = true
}

but it seems that:

rule {
  something = true
}
rule {
  something = true
}

is where it breaks down.

Before I dive and try to fix this, is it in-line with the design ? Would it be useful to have such support ? I initially assumed this was going to work right away.. and perhaps fail if the underlying struct isn't a slice when it hits the second rule instance.

Can you confirm this behavior, and I'll start the work.

abourget commented 7 years ago

@sethvargo I think you hit a different bug.. perhaps we should open a different issue for that. What do you think ?

abourget commented 7 years ago

Anything you think I can do to help ? This is blocking me quite a bit.

promiseofcake commented 7 years ago

I am hitting the same issues:

rule {
  something = true
}
rule {
  something = true
}

When attempting to build out definitions for Terraform

graph {
    title = "Redis latency (ms)"
    viz   = "timeseries"

    request {
      q    = "avg:redis.info.latency_ms{$host}"
      type = "bars"
    }
  }