Skip to content

Opa format panic caused by comments #6330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
sirpi opened this issue Oct 19, 2023 · 5 comments · Fixed by #7458
Closed

Opa format panic caused by comments #6330

sirpi opened this issue Oct 19, 2023 · 5 comments · Fixed by #7458
Assignees
@sspaink
Labels
bug inactive

Comments

@sirpi
Copy link

sirpi commented Oct 19, 2023

Short description

Wrongly placed comments can cause a panic error message in cli.

OPA version: any (tested even with 0.57.0)
Type: CLI

Steps To Reproduce

opa fmt mypolicy.rego

Shortest rego content:

package a

value := {"a":  #
"b"} #

For that, the following stack trace appears:

panic: overwriting non-nil beforeEnd

goroutine 1 [running]:
[github.com/open-policy-agent/opa/format.(*writer).beforeLineEnd(..](http://github.com/open-policy-agent/opa/format.(*writer).beforeLineEnd(..).)
        /src/format/format.go:1326
[github.com/open-policy-agent/opa/format.(*writer).insertComments(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).insertComments(0xc0003d6af0), {0xc0000a65d0, 0x1, 0x2ba5c80?}, 0xc0004656c0)
        /src/format/format.go:513 +0x2b9
[github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0), 0xa0?, 0xc00049cfc0, {0xc0000a65d0?, 0x1d27373?, 0x1b25340?})
        /src/format/format.go:699 +0x4b
[github.com/open-policy-agent/opa/format.(*writer).writeTerm(0x45540f](http://github.com/open-policy-agent/opa/format.(*writer).writeTerm(0x45540f)?, 0x1d27373?, {0xc0000a65d0?, 0x0?, 0x0?})
        /src/format/format.go:695 +0x26
[github.com/open-policy-agent/opa/format.(*writer).writeObject.(*writer).objectWriter.func2({0x1b58f60](http://github.com/open-policy-agent/opa/format.(*writer).writeObject.(*writer).objectWriter.func2(%7B0x1b58f60)?, 0xc00009edd0?}, {0xc0000a65b8?, 0xc00009ede0?, 0xc0004392e4?})
        /src/format/format.go:1045 +0xa8
[github.com/open-policy-agent/opa/format.(*writer).writeIterableLine(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeIterableLine(0xc0003d6af0), {0xc00009ee30, 0x1, 0xc000465740?}, {0xc0000a65b8?, 0x2ba5c80?, 0xc000465bc0?}, 0xc0005bf188)
        /src/format/format.go:1037 +0x110
[github.com/open-policy-agent/opa/format.(*writer).writeIterable(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeIterable(0xc0003d6af0), {0xc00009ede0?, 0x9e0905?, 0x0?}, 0x0?, 0x0?, {0xc0000a65b8, 0x1, 0x1}, 0xc0005bf188)
        /src/format/format.go:1013 +0x1a9
[github.com/open-policy-agent/opa/format.(*writer).writeObject(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeObject(0xc0003d6af0), {0x21e9d20, 0xc000465800}, 0xc00049d680?, {0xc0000a65b8, 0x1, 0x1})
        /src/format/format.go:864 +0x19e
[github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeTermParens(0xc0003d6af0), 0xae?, 0xc00049cf30, {0xc0000a65b0?, 0x2?, 0x45540f?})
        /src/format/format.go:708 +0x4fb
[github.com/open-policy-agent/opa/format.(*writer).writeTerm(..](http://github.com/open-policy-agent/opa/format.(*writer).writeTerm(..).)
        /src/format/format.go:695
[github.com/open-policy-agent/opa/format.(*writer).writeHead(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeHead(0xc0003d6af0), 0xc0003d6a80, 0x0, 0x1, {0x3d?, 0x0?, 0xc0?}, {0xc0000a65b0, 0x1, 0x1})
        /src/format/format.go:501 +0x59c
[github.com/open-policy-agent/opa/format.(*writer).writeRule(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeRule(0xc0003d6af0), 0xc00007eb90, 0x2?, {0x0?, 0x0?, 0x0?}, {0xc0000a65b0, 0x1, 0x1})
        /src/format/format.go:346 +0x1ec
[github.com/open-policy-agent/opa/format.(*writer).writeRules(0x0](http://github.com/open-policy-agent/opa/format.(*writer).writeRules(0x0)?, {0xc0000a4720, 0x4, 0xc0000b0f80?}, {0x28?, 0xf5?, 0x5b?}, {0xc000465ac0, 0x5, 0x8})
        /src/format/format.go:321 +0x99
[github.com/open-policy-agent/opa/format.(*writer).writeModule(0xc0003d6af0](http://github.com/open-policy-agent/opa/format.(*writer).writeModule(0xc0003d6af0), 0xc0000b1000?, {0x20?, 0xd3?, 0x49?})
        /src/format/format.go:280 +0x4ea
[github.com/open-policy-agent/opa/format.AstWithOpts({0x1c3fbe0](http://github.com/open-policy-agent/opa/format.AstWithOpts(%7B0x1c3fbe0)?, 0xc0000b0f80?}, {0x0?})
        /src/format/format.go:157 +0x5f1
[github.com/open-policy-agent/opa/format.Ast(..](http://github.com/open-policy-agent/opa/format.Ast(..).)
        /src/format/format.go:65
[github.com/open-policy-agent/opa/format.Source({0xc00000a0e8](http://github.com/open-policy-agent/opa/format.Source(%7B0xc00000a0e8), 0x8}, {0xc000022200?, 0x64?, 0x4cf107b8?})
        /src/format/format.go:44 +0x9b
[github.com/open-policy-agent/opa/cmd.formatFile(0x2c069a4](http://github.com/open-policy-agent/opa/cmd.formatFile(0x2c069a4), {0x21d3080, 0xc000074030}, {0xc00000a0e8, 0x8}, {0x21e0b88, 0xc0003d61c0}, {0x0?, 0x0?})
        /src/cmd/fmt.go:111 +0x265
[github.com/open-policy-agent/opa/cmd.opaFmt.func1({0xc00000a0e8](http://github.com/open-policy-agent/opa/cmd.opaFmt.func1(%7B0xc00000a0e8)?, 0xc0003d61c0?}, {0x21e0b88?, 0xc0003d61c0?}, {0x0?, 0x0?})
        /src/cmd/fmt.go:76 +0x59
path/filepath.walk({0xc00000a0e8, 0x8}, {0x21e0b88, 0xc0003d61c0}, 0x2084818)
        /usr/local/go/src/path/filepath/path.go:484 +0xff
path/filepath.Walk({0xc00000a0e8, 0x8}, 0x2084818)
        /usr/local/go/src/path/filepath/path.go:579 +0x66
[github.com/open-policy-agent/opa/cmd.opaFmt({0xc00009e740](http://github.com/open-policy-agent/opa/cmd.opaFmt(%7B0xc00009e740)?, 0x1, 0x0?})
        /src/cmd/fmt.go:75 +0xe5
[github.com/open-policy-agent/opa/cmd.glob..func2(0x1a86760](http://github.com/open-policy-agent/opa/cmd.glob..func2(0x1a86760)?, {0xc00009e740?, 0x4?, 0x1d27ee2?})
        /src/cmd/fmt.go:53 +0x25
[github.com/spf13/cobra.(*Command).execute(0x1a86760](http://github.com/spf13/cobra.(*Command).execute(0x1a86760), {0xc00009e700, 0x1, 0x1})
        /src/vendor/[github.com/spf13/cobra/command.go:944](http://github.com/spf13/cobra/command.go:944) +0x863
[github.com/spf13/cobra.(*Command).ExecuteC(0x1a86480)](http://github.com/spf13/cobra.(*Command).ExecuteC(0x1a86480))
        /src/vendor/[github.com/spf13/cobra/command.go:1068](http://github.com/spf13/cobra/command.go:1068) +0x3a5
[github.com/spf13/cobra.(*Command).Execute(0xc00006a000](http://github.com/spf13/cobra.(*Command).Execute(0xc00006a000)?)
        /src/vendor/[github.com/spf13/cobra/command.go:992](http://github.com/spf13/cobra/command.go:992) +0x13
main.main()
        /src/main.go:14 +0x1a

Expected behavior

A comprehensive error message without a stack trace would be more suitable.

The root cause of this is the clever mechanism which moves comments to the end of a formatted line if that would appear in the middle. Though if there is already a comment there, we get this error.

Additional context
@sirpi sirpi added the bug label Oct 19, 2023
@anderseknert
Copy link
Member

Thanks for reporting this @sirpi 👍 Sounds like you've done some research already. Would you want to try and fix it? 🙂

Is an error message the expected behavior though? Perhaps I'm missing something obvious, but I'd expect the formatter to format anything as long as it's valid Rego.

philipaconrad added a commit to philipaconrad/opa that referenced this issue Oct 24, 2023
@philipaconrad
format: Add error message for comment merge conflict.
8e1adbd 
This commit adds a basic check for comment merge conflicts, and provides
a helpful error message if a conflict happens during formatting.

Fixes open-policy-agent#6330

Signed-off-by: Philip Conrad <[email protected]>
@philipaconrad
Copy link
Contributor

@anderseknert How should the two comments be merged together? Is that a case the formatter can resolve on its own?

@sirpi
Copy link
Author

sirpi commented Oct 25, 2023 •
edited
Loading

Thanks for reporting this @sirpi 👍 Sounds like you've done some research already. Would you want to try and fix it? 🙂

Is an error message the expected behavior though? Perhaps I'm missing something obvious, but I'd expect the formatter to format anything as long as it's valid Rego.

Formatting the rego file without any complaint would be even better :) But I don't see how the comments should be merged automatically. Because a general solution should solve this for any number of comments as well:

value := {"a": # this is
{"b": # my ridiculous
{"c": # way of
"d"}}} # commenting code

just to give an idea.

Thanks for asking to fix this, but I am not familiar enough with Go to confidently touch this code. My observation came from simply running format commands with different inputs.

And I also see there is a fix with proper error handling and I appreciate it!

@anderseknert
Copy link
Member

Yes, sorry if I wasn't clear on that — if the formatter can't prettify some lines, I'd expect it to just skip those and proceed to format the rest of the file.

The formatter should never bail out on valid Rego.

@stale Stale
Copy link

stale bot commented Nov 24, 2023

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

@stale stale bot added the inactive label Nov 24, 2023
@sspaink sspaink self-assigned this Mar 17, 2025
@sspaink sspaink mentioned this issue Mar 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sspaink sspaink

Labels
bug inactive
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: don't panic on format due to unexpected comments sspaink/opa
4 participants
@anderseknert @philipaconrad @sspaink @sirpi