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.

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

rego.v1 enforces if before chained rule bodies, not allowed by parser #6370

Closed
johanfylling opened this issue Oct 31, 2023 · 4 comments · Fixed by #6374
Closed

rego.v1 enforces if before chained rule bodies, not allowed by parser #6370

johanfylling opened this issue Oct 31, 2023 · 4 comments · Fixed by #6374
Assignees
Labels

Comments

@johanfylling
Copy link
Contributor

The policy:

package play

import rego.v1

p if {
	input.x == 1
} {
	input.x == 2
}

will produce error:

1 error occurred: policy.rego:7: rego_parse_error: `if` keyword is required before rule body

However, updating the policy by adding an if to the second body:

package play

import rego.v1

p if {
	input.x == 1
} if {
	input.x == 2
}

will produce error:

1 error occurred: policy.rego:7: rego_parse_error: unexpected if keyword
	} if {
	  ^
@johanfylling
Copy link
Contributor Author

Three possible solutions:

  1. Don't require if on chained bodies despite rego.v1 import
  2. Update parser to allow if on chained bodies
  3. Don't allow chained bodies in 1.0

2 has added benefit of clarity (depending on view, of course):

p if {
	input.x == 1
} if {
	input.x == 2
}

as the added if makes the separation of the bodies somewhat more obvious.

3 might be too obtrusive if this is a commonly used pattern.

@johanfylling johanfylling added this to Backlog in Open Policy Agent via automation Oct 31, 2023
@johanfylling johanfylling moved this from Backlog to Planning - v0.59 in Open Policy Agent Oct 31, 2023
@ashutosh-narkar
Copy link
Member

If we believe it's a commonly used pattern, would option 1 be better? You can make the argument that the first if applies to the chain maybe.

@anderseknert
Copy link
Member

Agreed — since the purpose of chained bodies is likely compactness, I feel like it's fine to only require if in the visible rule head. So I vote for 1 too.

@johanfylling
Copy link
Contributor Author

Alright, so far it looks like no 1 has it 👍.

@johanfylling johanfylling self-assigned this Nov 2, 2023
@johanfylling johanfylling moved this from Planning - v0.59 to In Progress in Open Policy Agent Nov 2, 2023
johanfylling added a commit to johanfylling/opa that referenced this issue Nov 2, 2023
Fixes: open-policy-agent#6370
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Open Policy Agent automation moved this from In Progress to Done Nov 3, 2023
johanfylling added a commit that referenced this issue Nov 3, 2023
Fixes: #6370
Signed-off-by: Johan Fylling <johan.dev@fylling.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging a pull request may close this issue.

3 participants