[go: up one dir, main page]
Skip to content
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

calling Module.String() returns an invalid module for Rego V1 #6973

Closed
nikpivkin opened this issue Aug 29, 2024 · 2 comments · Fixed by #7000
Closed

calling Module.String() returns an invalid module for Rego V1 #6973

nikpivkin opened this issue Aug 29, 2024 · 2 comments · Fixed by #7000
Labels
bug

Comments

@nikpivkin
Copy link
Contributor

Short description

OPA v0.67.1

Steps To Reproduce

package main

import "github.com/open-policy-agent/opa/ast"

func main() {
	mod := `package test
	
import rego.v1

allow if {
	1 < 2
}
`

	module, err := ast.ParseModuleWithOpts("test.rego", mod, ast.ParserOptions{})
	if err != nil {
		panic(err)
	}

	println(module.String())

	_, err = ast.ParseModuleWithOpts("test.rego", module.String(), ast.ParserOptions{})
	if err != nil {
		panic(err)
	}
}

Output:

go run .
package test

import rego.v1

allow = true { lt(1, 2) }
panic: 1 error occurred: test.rego:5: rego_parse_error: `if` keyword is required before rule body

goroutine 1 [running]:
main.main()
        /Users/nikita/projects/opa-test/main.go:24 +0x100
exit status 2

Expected behavior
@nikpivkin nikpivkin added the bug label Aug 29, 2024
@johanfylling
Copy link
Contributor

I believe the intention of ast.Module.String() is to output a human-readable representation of the module, and not to produce idiomatic Rego. For instance, the compiler might apply several modifications to the AST to prepare it for evaluation that won't directly translate to "correct" Rego. E.g.: it will drop directive imports, such as future.keywords and rego.v1 and add local variables, rule bodies, etc. These changes are not guaranteed to represent idiomatic, or even "correct" Rego. There is also nothing stopping a user/developer from programmatically building an AST from scratch. String() is a means to produce a human-readable representation of that state.

To produce idiomatic Rego from AST, OPA provides functions like format.Source() and format.Ast(). I'd suggest using these if the end goal is to produce Rego that is guaranteed to be consumable by OPA.

johanfylling added a commit to johanfylling/opa that referenced this issue Sep 6, 2024
@johanfylling
ast: Make Module.String() include if/contains for modules parse…
c0117b1 
…d as v1

Fixes: open-policy-agent#6973
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
@johanfylling johanfylling mentioned this issue Sep 6, 2024
Merged
@johanfylling
Copy link
Contributor

I think having Module.String() include if and contains where appropriate for v1 modules improves readability, though.

johanfylling added a commit that referenced this issue Sep 6, 2024
@johanfylling
ast: Make Module.String() include if/contains for v1 modules (#…
7cd3fec 
…7000)

Fixes: #6973
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
@BrewTestBot BrewTestBot mentioned this issue Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

ast: Make Module.String() include if/contains for v1 modules johanfylling/opa
2 participants
@johanfylling @nikpivkin