command/hook_ui: Truncate the ID considering multibyte characters

[minamijoyo]

Fixes #18822

The tuncatedId function had been introduced in #12261 and increased the maxIdLen to 80 in #13317.

Since the number of bytes itself seems to be unimportant, the ID should be truncated to 80 characters, not 80 bytes.

Please let me know if we need to truncate it to 80 bytes considering multibyte character boundaries. This will require more complex logic.

[terraform@fix-multibyte-trucate|✔]$ go test -v ./command -run TestTruncateId
=== RUN   TestTruncateId
=== RUN   TestTruncateId/0
=== RUN   TestTruncateId/1
=== RUN   TestTruncateId/2
=== RUN   TestTruncateId/3
=== RUN   TestTruncateId/4
=== RUN   TestTruncateId/5
=== RUN   TestTruncateId/6
=== RUN   TestTruncateId/7
=== RUN   TestTruncateId/8
=== RUN   TestTruncateId/9
=== RUN   TestTruncateId/10
=== RUN   TestTruncateId/11
=== RUN   TestTruncateId/12
=== RUN   TestTruncateId/13
=== RUN   TestTruncateId/14
=== RUN   TestTruncateId/15
=== RUN   TestTruncateId/16
=== RUN   TestTruncateId/17
--- PASS: TestTruncateId (0.00s)
    --- PASS: TestTruncateId/0 (0.00s)
    --- PASS: TestTruncateId/1 (0.00s)
    --- PASS: TestTruncateId/2 (0.00s)
    --- PASS: TestTruncateId/3 (0.00s)
    --- PASS: TestTruncateId/4 (0.00s)
    --- PASS: TestTruncateId/5 (0.00s)
    --- PASS: TestTruncateId/6 (0.00s)
    --- PASS: TestTruncateId/7 (0.00s)
    --- PASS: TestTruncateId/8 (0.00s)
    --- PASS: TestTruncateId/9 (0.00s)
    --- PASS: TestTruncateId/10 (0.00s)
    --- PASS: TestTruncateId/11 (0.00s)
    --- PASS: TestTruncateId/12 (0.00s)
    --- PASS: TestTruncateId/13 (0.00s)
    --- PASS: TestTruncateId/14 (0.00s)
    --- PASS: TestTruncateId/15 (0.00s)
    --- PASS: TestTruncateId/16 (0.00s)
    --- PASS: TestTruncateId/17 (0.00s)
PASS
ok      github.com/hashicorp/terraform/command  0.038s

[mildwonkey]

Hi @minamijoyo,
Thank you for reporting this bug (in #18822) and working on this fix.

This commit touches a section of terraform that is being reworked as part of terraform 0.12, so if it's okay with you I'd prefer to wait and fix this in that release, mainly to avoid accidentally losing the work you've done when we merge in the 0.12 development branch.

Thanks again for investigating and proposing a fix for this, especially with tests!

[minamijoyo]

@mildwonkey No problem. I'm looking forward to v0.12!

[minamijoyo]

@mildwonkey
I noticed that the v0.12-dev branch had been finally merged into the master!
It's a great work 🎉 🎉 🎉

Now, this bug fix branch seems not to conflict with the latest master.

Please review and merge when you can. Thanks!

[minamijoyo]

@mildwonkey I rebased this PR on the master branch and confirmed that the test passed, but when I tried to reproduce the original issue reported in #18822 with terraform plan, I found that it can no longer be reproduced.

With the change of v0.12, it seems that the truncateId function is not called from plan already, but it still has been called from apply.

terraform/command/hook_ui.go

Line 205 in 33e1839

idSuffix = fmt.Sprintf("%s=%s, ", state.IDKey, truncateId(state.IDValue, maxIdLen))

I think there is still meaning to fix this bug at the moment. However, the current code base seems to be a somewhat inconsistent behavior in plan and apply.

[pselle]

@minamijoyo 👋 It does seem that this code is still touched when stillApplying runs, and that is the only case where truncateId is used. I've opened a separate PR #22357 investigating that I'm not seeing the section populated consistently, and am still digging into it, but once I was able to get them displaying (...mostly) I was able to test your fix. Thank you for helping make Terraform more inclusive of other languages!

[minamijoyo]

@pselle Thanks!

[ghost]

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.