Skip to content

Commit

Permalink
Rule: rule-length (#354)
Browse files Browse the repository at this point in the history 
Made easy by now having the `text` attribute present in the AST.
Thanks for that @charlieegan3!

Fixed existing violations against this rule, which were almost
exlusively in tests. Sprinkled a few ignore directives in the
places where it wasn't possible to make rules any shorter.

Fixes #236

Signed-off-by: Anders Eknert <anders@styra.com>
  • Loading branch information
@anderseknert
anderseknert committed Sep 30, 2023
1 parent 716a574 commit de17845
Show file tree
Hide file tree
Showing 17 changed files with 306 additions and 138 deletions.
1 change: 1 addition & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ The following rules are currently available:
| style | [opa-fmt](https://docs.styra.com/regal/rules/style/opa-fmt) | File should be formatted with `opa fmt` |
| style | [prefer-snake-case](https://docs.styra.com/regal/rules/style/prefer-snake-case) | Prefer snake_case for names |
| style | [prefer-some-in-iteration](https://docs.styra.com/regal/rules/style/prefer-some-in-iteration) | Prefer `some .. in` for iteration |
| style | [rule-length](https://docs.styra.com/regal/rules/style/rule-length) | Max rule length exceeded |
| style | [todo-comment](https://docs.styra.com/regal/rules/style/todo-comment) | Avoid TODO comments |
| style | [unconditional-assignment](https://docs.styra.com/regal/rules/style/unconditional-assignment) | Unconditional assignment in rule body |
| style | [use-assignment-operator](https://docs.styra.com/regal/rules/style/use-assignment-operator) | Prefer := over = for assignment |
Expand Down
14 changes: 7 additions & 7 deletions bundle/regal/ast_test.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import future.keywords.in

import data.regal.ast

# regal ignore:rule-length
test_find_vars if {
policy := `
package p
Expand Down Expand Up @@ -116,7 +117,7 @@ allow := true
blocks := ast.comment_blocks(module.comments)
blocks == [
[
{"Location": {"col": 1, "file": "p.rego", "row": 3}, "Text": "IE1FVEFEQVRB"},
{"Location": {"col": 1, "file": "p.rego", "row": 3, "text": "IyBNRVRBREFUQQ=="}, "Text": "IE1FVEFEQVRB"},
{"Location": {"col": 1, "file": "p.rego", "row": 4}, "Text": "IHRpdGxlOiBmb28="},
{"Location": {"col": 1, "file": "p.rego", "row": 5}, "Text": "IGJhcjogaW52YWxpZA=="},
],
Expand All @@ -128,6 +129,7 @@ allow := true
]
}

# regal ignore:rule-length
test_find_vars_in_local_scope if {
policy := `
package p
Expand Down Expand Up @@ -181,10 +183,10 @@ test_find_vars_in_local_scope_complex_comprehension_term if {
allow_rule := module.rules[0]

ast.find_vars_in_local_scope(allow_rule, {"col": 10, "row": 10}) == [
{"location": {"col": 3, "file": "p.rego", "row": 7}, "type": "var", "value": "a"},
{"location": {"col": 15, "file": "p.rego", "row": 7}, "type": "var", "value": "b"},
{"location": {"col": 20, "file": "p.rego", "row": 7}, "type": "var", "value": "c"},
{"location": {"col": 31, "file": "p.rego", "row": 7}, "type": "var", "value": "b"},
{"location": {"col": 3, "file": "p.rego", "row": 7, "text": "YQ=="}, "type": "var", "value": "a"},
{"location": {"col": 15, "file": "p.rego", "row": 7, "text": "Yg=="}, "type": "var", "value": "b"},
{"location": {"col": 20, "file": "p.rego", "row": 7, "text": "Yw=="}, "type": "var", "value": "c"},
{"location": {"col": 31, "file": "p.rego", "row": 7, "text": "Yg=="}, "type": "var", "value": "b"},
]
}

Expand Down Expand Up @@ -212,11 +214,9 @@ test_find_names_in_scope if {
}`

module := regal.parse_module("p.rego", policy)

allow_rule := module.rules[3]

in_scope := ast.find_names_in_scope(allow_rule, {"col": 1, "row": 30}) with input as module

in_scope == {"bar", "global", "comp", "allow", "a", "b", "c", "d", "e"}
}

Expand Down
4 changes: 4 additions & 0 deletions bundle/regal/config/provided/data.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ rules:
prefer-some-in-iteration:
level: error
ignore-nesting-level: 2
rule-length:
except-empty-body: true
level: error
max-rule-length: 30
todo-comment:
level: error
unconditional-assignment:
Expand Down
4 changes: 1 addition & 3 deletions bundle/regal/result_test.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ test_related_resources_generated_by_result_fail_for_builtin_rule if {
}
}

# regal ignore:rule-length
test_aggregate_function_builtin_rule if {
chain := [
{"path": ["regal", "rules", "testing", "aggregation", "report"]},
Expand All @@ -99,7 +100,6 @@ test_aggregate_function_builtin_rule if {
"regal": {"file": {"name": "policy.rego"}},
"package": {"path": [{"value": "data"}, {"value": "a"}, {"value": "b"}, {"value": "c"}]},
}

r == {
"rule": {
"category": "testing",
Expand Down Expand Up @@ -127,12 +127,10 @@ test_aggregate_function_custom_rule if {
"path": ["custom", "regal", "rules", "testing", "aggregation"],
},
]

r := result.aggregate(chain, {"foo": "bar", "baz": [1, 2, 3]}) with input as {
"regal": {"file": {"name": "policy.rego"}},
"package": {"path": [{"value": "data"}, {"value": "a"}, {"value": "b"}, {"value": "c"}]},
}

r == {
"rule": {
"category": "testing",
Expand Down
8 changes: 4 additions & 4 deletions bundle/regal/rules/custom/naming_convention.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ report contains violation if {
violation := with_description(
result.fail(rego.metadata.chain(), result.location(input["package"])),
sprintf(
"Naming convention violation: package name %q does not match pattern %q",
"Naming convention violation: package name %q does not match pattern '%s'",
[ast.package_name, convention.pattern],
),
)
Expand All @@ -45,7 +45,7 @@ report contains violation if {
violation := with_description(
result.fail(rego.metadata.chain(), result.location(rule.head)),
sprintf(
"Naming convention violation: rule name %q does not match pattern %q",
"Naming convention violation: rule name %q does not match pattern '%s'",
[ast.name(rule), convention.pattern],
),
)
Expand All @@ -66,7 +66,7 @@ report contains violation if {
violation := with_description(
result.fail(rego.metadata.chain(), result.location(rule.head)),
sprintf(
"Naming convention violation: function name %q does not match pattern %q",
"Naming convention violation: function name %q does not match pattern '%s'",
[ast.name(rule), convention.pattern],
),
)
Expand All @@ -86,7 +86,7 @@ report contains violation if {
violation := with_description(
result.fail(rego.metadata.chain(), result.location(var)),
sprintf(
"Naming convention violation: variable name %q does not match pattern %q",
"Naming convention violation: variable name %q does not match pattern '%s'",
[var.value, convention.pattern],
),
)
Expand Down
130 changes: 44 additions & 86 deletions bundle/regal/rules/custom/naming_convention_test.rego
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,18 +12,10 @@ test_fail_package_name_does_not_match_pattern if {
"conventions": [{"targets": ["package"], "pattern": `^foo\.bar\..+$`}],
}
r := rule.report with input as regal.parse_module("policy.rego", "package foo.bar") with config.for_rule as cfg
r == {{
"category": "custom",
# regal ignore:line-length
"description": "Naming convention violation: package name \"foo.bar\" does not match pattern \"^foo\\\\.bar\\\\..+$\"",
"level": "error",
"location": {"col": 1, "file": "policy.rego", "row": 1, "text": "package foo.bar"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
}}
r == {expected(
`Naming convention violation: package name "foo.bar" does not match pattern '^foo\.bar\..+$'`,
{"col": 1, "file": "policy.rego", "row": 1, "text": "package foo.bar"},
)}
}

test_success_package_name_matches_pattern if {
Expand All @@ -41,17 +33,10 @@ test_fail_rule_name_does_not_match_pattern if {
"conventions": [{"targets": ["rule"], "pattern": "^[a-z]+$"}],
}
r := rule.report with input as ast.policy(`FOO := true`) with config.for_rule as cfg
r == {{
"category": "custom",
"description": "Naming convention violation: rule name \"FOO\" does not match pattern \"^[a-z]+$\"",
"level": "error",
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "FOO := true"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
}}
r == {expected(
`Naming convention violation: rule name "FOO" does not match pattern '^[a-z]+$'`,
{"col": 1, "file": "policy.rego", "row": 3, "text": "FOO := true"},
)}
}

test_success_rule_name_matches_pattern if {
Expand All @@ -69,17 +54,10 @@ test_fail_function_name_does_not_match_pattern if {
"conventions": [{"targets": ["function"], "pattern": "^[a-z]+$"}],
}
r := rule.report with input as ast.policy(`fooBar(_) := true`) with config.for_rule as cfg
r == {{
"category": "custom",
"description": "Naming convention violation: function name \"fooBar\" does not match pattern \"^[a-z]+$\"",
"level": "error",
"location": {"col": 1, "file": "policy.rego", "row": 3, "text": "fooBar(_) := true"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
}}
r == {expected(
`Naming convention violation: function name "fooBar" does not match pattern '^[a-z]+$'`,
{"col": 1, "file": "policy.rego", "row": 3, "text": "fooBar(_) := true"},
)}
}

test_success_function_name_matches_pattern if {
Expand All @@ -103,17 +81,10 @@ test_fail_var_name_does_not_match_pattern if {
"conventions": [{"targets": ["variable"], "pattern": "^[a-z_]+$"}],
}
r := rule.report with input as policy with config.for_rule as cfg
r == {{
"category": "custom",
"description": "Naming convention violation: variable name \"fooBar\" does not match pattern \"^[a-z_]+$\"",
"level": "error",
"location": {"col": 3, "file": "policy.rego", "row": 5, "text": "\t\tfooBar := true"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
}}
r == {expected(
`Naming convention violation: variable name "fooBar" does not match pattern '^[a-z_]+$'`,
{"col": 3, "file": "policy.rego", "row": 5, "text": "\t\tfooBar := true"},
)}
}

test_success_var_name_matches_pattern if {
Expand Down Expand Up @@ -142,48 +113,35 @@ test_fail_multiple_conventions if {
fooBar == true
}
`)
cfg := {
"level": "error",
"conventions": [
{"targets": ["package"], "pattern": `^acmecorp\.[a-z_\.]+$`},
{"targets": ["rule", "variable"], "pattern": "^bar$|^foo_bar$"},
],
}
cfg := {"level": "error", "conventions": [
{"targets": ["package"], "pattern": `^acmecorp\.[a-z_\.]+$`},
{"targets": ["rule", "variable"], "pattern": "^bar$|^foo_bar$"},
]}
r := rule.report with input as policy with config.for_rule as cfg
r == {
{
"category": "custom",
# regal ignore:line-length
"description": "Naming convention violation: package name \"foo.bar\" does not match pattern \"^acmecorp\\\\.[a-z_\\\\.]+$\"",
"level": "error",
"location": {"col": 1, "file": "policy.rego", "row": 1, "text": "package foo.bar"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
},
{
"category": "custom",
"description": "Naming convention violation: rule name \"foo\" does not match pattern \"^bar$|^foo_bar$\"",
"level": "error",
"location": {"col": 2, "file": "policy.rego", "row": 3, "text": "\tfoo := true"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
},
{
"category": "custom",
"description": "Naming convention violation: variable name \"fooBar\" does not match pattern \"^bar$|^foo_bar$\"",
"level": "error",
"location": {"col": 3, "file": "policy.rego", "row": 6, "text": "\t\tfooBar := true"},
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
},
expected(
`Naming convention violation: package name "foo.bar" does not match pattern '^acmecorp\.[a-z_\.]+$'`,
{"col": 1, "file": "policy.rego", "row": 1, "text": "package foo.bar"},
),
expected(
`Naming convention violation: rule name "foo" does not match pattern '^bar$|^foo_bar$'`,
{"col": 2, "file": "policy.rego", "row": 3, "text": "\tfoo := true"},
),
expected(
`Naming convention violation: variable name "fooBar" does not match pattern '^bar$|^foo_bar$'`,
{"col": 3, "file": "policy.rego", "row": 6, "text": "\t\tfooBar := true"},
),
}
}

expected(description, location) := {
"category": "custom",
"description": description,
"level": "error",
"location": location,
"related_resources": [{
"description": "documentation",
"ref": config.docs.resolve_url("$baseUrl/$category/naming-convention", "custom"),
}],
"title": "naming-convention",
}
31 changes: 31 additions & 0 deletions bundle/regal/rules/style/rule_length.rego
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
# METADATA
# description: Max rule length exceeded
package regal.rules.style["rule-length"]

import future.keywords.contains
import future.keywords.if
import future.keywords.in

import data.regal.ast
import data.regal.config
import data.regal.result

cfg := config.for_rule("style", "rule-length")

report contains violation if {
some rule in input.rules
lines := split(base64.decode(rule.location.text), "\n")

count(lines) > cfg["max-rule-length"]

not empty_body_exception(cfg, rule)

violation := result.fail(rego.metadata.chain(), result.location(rule.head))
}

empty_body_exception(conf, rule) if {
conf["except-empty-body"] == true
count(rule.body) == 1
rule.body[0].terms.type == "boolean"
rule.body[0].terms.value == true
}
Loading

0 comments on commit de17845

Please sign in to comment.