hclwrite: Allow body to set attributes in blocks

[minamijoyo]

The current implementation can set attributes in top-level, but not attributes in blocks. To do this, add the (*Body).GetBlock() method that selects a block by typeName and labels.

Related to #88

---

UPDATE: The original proposal has changed as a result of the suggestion:

• Add (*Block).Type() and (*Block).Labels()
• Add (*Body).FirstMatchingBlock() instead of (*Body).GetBlock()
• Expose parser.decodeStringLit() in the hclsyntax package as ParseStringLiteralToken()

[codecov-io]

Codecov Report

Merging #126 into master will increase coverage by 0.15%. The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   69.02%   69.18%   +0.15%     
==========================================
  Files          98       98              
  Lines       10258    10288      +30     
==========================================
+ Hits         7081     7118      +37     
+ Misses       2858     2852       -6     
+ Partials      319      318       -1

Impacted Files	Coverage Δ
hcl/hclsyntax/parser_template.go	65.99% <100%> (ø)	:arrow_up:
hclwrite/ast_body.go	89.87% <100%> (+9.31%)	:arrow_up:
hcl/hclsyntax/parser.go	67.33% <66.66%> (ø)	:arrow_up:
hclwrite/ast_block.go	91.8% <95.65%> (+2.32%)	:arrow_up:
hclwrite/node.go	72.38% <0%> (+1.9%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 66c59f9...def1c4e. Read the comment docs.

[apparentlymart]

Hi @minamijoyo! Thanks for working on this.

The way I'd intended this to be done is for the caller to call Body.Blocks and then iterate over the list to find the one they want to add to, because blocks within a body are not necessarily unique even by block type and labels, depending on how the application has defined its schema. However, currently there isn't a good way to actually match on the labels because they are represented as opaque nodes.

This GetBlock method is, I suppose, a shorthand for that in the case where the blocks are unique by type and labels, which is a common design choice. Therefore I'd like to merge this, but I'd suggest the following changes:

• If we call the method FirstMatchingBlock instead of GetBlock then perhaps it's clearer that it does not handle the case where there are multiple blocks with the same type and labels.

• I'd like to understand better what the strings.Trim(labelString, ") call is for. I understand that normally labels are given in quotes and so they'd be included in the token stream here, but I'm not sure why spaces would be there :thinking:.

  A more precise way to write this, I think, would be to look at the individual tokens and recognize the only two valid cases for label tokens:

  • If the node is *identifier then there should be only one token and it should have type TokenIdent. The label is the string equivalent of that token's bytes.
  • If the node is *quoted then there should be three tokens and the first one should be TokenOQuote and the last one should be TokenCQuote. The middle token should be a TokenQuotedLit whose value is the label. This case is harder because the TokenQuotedLit may contain escape sequences, and currently the logic for processing those is unexported in the hclsyntax package.

  The function that can turn a string literal token into a raw Go string is parser.decodeStringLit in the hclsyntax package. Although it is currently a method on parser I think it doesn't actually use its parser receiver at all, so we could make it an exported package-level function ParseStringLiteralToken instead and then hclwrite could call it here with the TokenQuotedLit token (converted to a hclsyntax.Token) to get the value.

  • If neither of the previous cases are true (should be impossible) or if parsing the string literal returns error diagnostics then we can just assume the label doesn't match, because it's invalid in some way.

  Adding a method Block.Labels that returns []string of labels would give callers a way to do manual filtering if they need to and also allow FirstMatchingBlock to call it to get the labels to match against.

If you don't have time to respond to these changes please let me know and I'd be happy to do it myself when I have some spare time. And please also let me know if anything above is unclear or seems surprising. Thanks again for working on this!

[minamijoyo]

@apparentlymart Thank you for your comments! I don't know much about the internal implementation, so it might take some time for me to fix it, but I'm interested in the hcl2, so I'll try to fix it.

[minamijoyo]

@apparentlymart I fixed it. How about this? If it looks almost good, I will add more tests.

[minamijoyo]

@apparentlymart I've added tests. Please review this.

If my understanding is correct, I found a bug in the existing implementation. If label is unquoted, we expect to get a TokenIdent as *identifier, but the current implementation seems to wrap it in *quoted. Please let me know if I am wrong.

[minamijoyo]

Looking at the parser implementation, I may have found the cause. https://github.com/hashicorp/hcl2/blob/1aba4ac822ee0b54f729b458191f9f069d68af28/hclwrite/parser.go#L310

(*hclwrite.node)(0xc000118570)({
 content: (*hclwrite.quoted)(0xc0000b3000)({
  leafNode: (hclwrite.leafNode) {
  },
  parent: (*hclwrite.node)(<nil>),
  tokens: (hclwrite.Tokens) (len=1 cap=1) {
   (*hclwrite.Token)(0xc000106690)({
    Type: (hclsyntax.TokenType) TokenIdent,
    Bytes: ([]uint8) (len=6 cap=6) {
     00000000  6c 61 62 65 6c 31                                 |label1|
    },
    SpacesBefore: (int) 1
   })
  }
 }),
 list: (*hclwrite.nodes)(<nil>),
 before: (*hclwrite.node)(<nil>),
 after: (*hclwrite.node)(<nil>)
})

[minamijoyo]

I've opened PR #130 to fix it.

[minamijoyo]

I rebased this branch on the latest master and removed the workaround. All tests are now passed. It's ready to be merged. Please check this again! @apparentlymart

[apparentlymart]

Thanks @minamijoyo!

We're currently working on moving the code from this repository into the hashicorp/hcl repository as major version 2, so I'm going to wait to merge this just to avoid conflicting with that work, and then I'll apply your changeset to the new branch in the other repository in order to merge it.

Thanks again for working on this!

[minamijoyo]

@apparentlymart OK. I'll be waiting for it.

[apparentlymart]

Hi again! Sorry for the delay.

This is now merged over in the hcl2 branch in hashicorp/hcl, and included in the v2.0.0 release there. The commit in the other repository is 9d1235a5b4e8.