chore: remove all possible AsString panics #183
Conversation
extract/parameter.go
Outdated
| tyValStr, ok := hclext.AsString(tyVal) | ||
| if !ok { | ||
| // Either the val is unknown or null | ||
| diag := &hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: fmt.Sprintf("Invalid %q attribute for block %s", key, block.Label()), | ||
| Detail: "Expected a string, got an unknown or null value", | ||
| EvalContext: block.Context().Inner(), | ||
| } | ||
|
|
||
| if tyAttr.IsNotNil() { | ||
| diag.Subject = &(tyAttr.HCLAttribute().Range) | ||
| // diag.Context = &(block.HCLBlock().DefRange) | ||
| diag.Expression = tyAttr.HCLAttribute().Expr | ||
| } | ||
| return "", diag | ||
| } | ||
| return tyValStr, nil |
There was a problem hiding this comment.
Returns an error now instead of a panic.
| // TODO: Should this error be captured? | ||
| if key.Type() != cty.String { | ||
| return true | ||
| } | ||
| //nolint:gocritic // string type asserted above | ||
| k := key.AsString() | ||
| k, ok := AsString(key) | ||
| if !ok { | ||
| // TODO: Should this error be captured? | ||
| return stop | ||
| } |
There was a problem hiding this comment.
Invalid values were skipped before, so behavior is unchanged. Just no potential for a panic.
| refParts = append(refParts, fmt.Sprintf("[%s]", part.Key.AsString())) | ||
| stringName, ok := AsString(part.Key) | ||
| if !ok { | ||
| // Nothing we can do, just put a placeholder | ||
| stringName = "??" | ||
| } | ||
| refParts = append(refParts, fmt.Sprintf("[%s]", stringName)) |
There was a problem hiding this comment.
CreateDotReferenceFromTraversal converts a reference to a string for error messages and logs. So the ?? is ok if that value is really unknown.
| path, ok := hclext.AsString(args[0]) | ||
| if !ok { | ||
| return cty.NilVal, xerrors.Errorf("invalid path argument") | ||
| } |
| name, ok := hclext.AsString(nameVal) | ||
| if !ok { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The code above does the same thing as this.
| k := key.AsString() | ||
| k, ok := AsString(key) | ||
| if !ok { | ||
| // TODO: Should this error be captured? |
There was a problem hiding this comment.
Probably no harm to drop a warn diag here, if possible?
There was a problem hiding this comment.
Unfortunately I cannot really. This is all for merging context values. Which happens in a hook that cannot raise errors.
The context is supposed to store variable (reference) values. So this failing just means a reference/var is skipped. Which leaves it's value as unknown when the context tries to load the value.
Which is the correct behavior at a high level. In reality though, I do not think this should happen. Maybe with some complex structs/types? I need to test more.
Any call to
AsStringis unsafe. All places where the error is ignored is used in errors, so a placeholder or empty string is ok.A panic was observed by a customer. I am not sure where it occured. To be safe, I am using the function we have to protect these
AsStringcalls. I added a custom linter to prevent these calls, this PR just removes all calls that were not fixed already.