Azure / Azure/mapotf

reorder_attributes emits attribute pairs fused on one line for certain source layouts

Open
#107 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
60
Forks
19
Avg merge
2h 2m
Merged PRs (30d)
4

Description

## Summary

`reorder_attributes` emits attributes via `body.AppendUnstructuredTokens(el.attr.BuildTokens(nil))` (`pkg/transform_reorder_attributes.go:299`). For certain attribute pairs the resulting tokens fuse onto one line — e.g. `source = "./submod" location = "westeurope"` — producing output that requires `terraform fmt` to clean up. `terraform fmt` does fix it, so this is **cosmetic only**; the output is not structurally invalid HCL.

Surfaced during governance-side verification of [PR #106](https://github.com/Azure/mapotf/pull/106) by the `Azure/avm-terraform-governance` team. Not a regression introduced by #106 — pre-existing behaviour in `reorder_attributes` made visible by their G4 module-input ordering experiment.

## Reproduction

Before:
```hcl
module "scrambled" {
tags = { env = "test" }
enable_telemetry = false
name = "rg-example"
location = "westeurope"
source = "./submod"
}
```

Transform:
```hcl
transform "reorder_attributes" "module_inputs" {
target_block_address = "module.scrambled"
head_attributes = ["source", "version", "for_each", "count", "providers"]
body_attributes = ["location", "name", "enable_telemetry", "tags"]
foot_attributes = ["depends_on"]
}
```

Actual output (note the fused first line):
```hcl
module "scrambled" {
source = "./submod" location = "westeurope"
name = "rg-example"
enable_telemetry = false
tags = { env = "test" }
}
```

Expected output:
```hcl
module "scrambled" {
source = "./submod"

location = "westeurope"
name = "rg-example"
enable_telemetry = false
tags = { env = "test" }
}
```

## Likely cause

`pkg/transform_reorder_attributes.go:275-302` `emitReorderElements`:

```go
if el.isNested {
body.AppendBlock(el.block)
} else {
body.AppendUnstructuredTokens(el.attr.BuildTokens(nil))
}
```

`hclwrite.Attribute.BuildTokens` returns the attribute's complete token stream including leading whitespace/indent that was captured at parse time. `AppendUnstructuredTokens` is a raw token splice — it doesn't insert newlines on attribute boundaries the way `SetAttributeRaw` does. When an attribute's parse-time leading whitespace was a single space (because it followed a `{` opener, say) it gets appended after the previous attribute's trailing tokens without a newline in between.

The `body.AppendNewline()` calls at `emitReorderElements:293` and the initial `body.AppendNewline()` at `Apply:92` mitigate the section-boundary case for blocks (`AppendBlock` does its own newline management), but the attribute-only path relies on `BuildTokens` always returning a trailing newline. For attributes that originally lacked one (e.g. last attribute of the source block before the closing `}`), it doesn't.

## Suggested fix candidates

**Option A (smallest)** — switch to `SetAttributeRaw` for attributes:

```go
if el.isNested {
body.AppendBlock(el.block)
} else {
body.SetAttributeRaw(el.attr.Name(), el.attr.Expr().BuildTokens(nil))
}
```

`SetAttributeRaw` handles newline insertion correctly. Caveat: drops any leading comments/whitespace originally attached to the attribute (`BuildTokens` preserves these, `SetAttributeRaw` doesn't). Need a test to confirm whether the comment-preservation requirement was real.

**Option B (preserves comments)** — append an explicit `Newline` token after each attribute when the last token isn't a newline:

```go
if el.isNested {
body.AppendBlock(el.block)
} else {
tokens := el.attr.BuildTokens(nil)
body.AppendUnstructuredTokens(tokens)
if len(tokens) > 0 && tokens[len(tokens)-1].Type != hclsyntax.TokenNewline {
body.AppendNewline()
}
}
```

Imports `hclsyntax`. More defensive.

## Severity

Cosmetic. `terraform fmt -recursive` cleans the file on next run, so pipelines that run `mapotf transform` + `terraform fmt` together (which is the typical AVM `make pre-commit` pattern) are unaffected. Standalone `mapotf transform` users without a follow-up `terraform fmt` see ugly output.

**Not v0.1.4 blocking.** Suggest target v0.1.5.

## Acceptance criteria

- Add a regression test in `pkg/transform_reorder_attributes_test.go` with the reproduction fixture above (or similar — last-source-attribute-promoted-to-head) asserting line-by-line output equality with the expected layout
- All existing `reorder_attributes` tests still pass
- Output remains `terraform fmt`-stable (running `terraform fmt` on the output produces zero diff)

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.