Skip to content

Commit

Permalink
ast: Misc. refactoring on annotations support
Browse files Browse the repository at this point in the history 
This commit combines a bunch of refactoring on annotations to support
future work.

Specifically:

* Annotations are now normal AST nodes/statements. This means that
  annotations store locations and also implement String() and
  Compare(). Annotations are now correctly compared during module
  comparison and annotations are included in the module string
  representation (before annotations would be dropped when the module
  String() function was called.) Also, the visitor and transformer
  functions support annotations now.

* Annotations are no longer hidden behind an interface. Instead, there
  is a single annotation struct that we can evolve over
  time. It was unclear how the Annotations interface was going to work
  in the long-term (e.g., callers would not be able to define their
  own annotation types since the parser needs to be aware of them.)
  With this change, Annotations are just structs now. We can extend
  the struct as needed going forward. Custom data can be stored in a
  dedicated field.

* Annotation parsing has been refactored. We now attach annotations to
  the statement following the annotation. The parser will reject
  METADATA blocks that contain whitespace between the METADATA hint
  and the YAML block. Similarly, we no longer support trailing
  unindented comments that follow the METADATA block. Users can inject
  whitespace after the YAML block if they want to include trailing
  comments.

* The opa parse subcommand now enables annotation processing.

Signed-off-by: Torin Sandall <[email protected]>
  • Loading branch information
@tsandall
tsandall committed Apr 27, 2021
1 parent 947ec26 commit d3adb90
Show file tree
Hide file tree
Showing 15 changed files with 869 additions and 338 deletions.
33 changes: 13 additions & 20 deletions ast/check.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1129,42 +1129,35 @@ func getObjectType(ref Ref, o types.Type, rule *Rule, d *types.DynamicProperty)
return getObjectTypeRec(keys, o, d), nil
}

func getRuleAnnotation(rule *Rule) (sannots []SchemaAnnotation) {
for _, annot := range rule.Module.Annotation {
schemaAnnots, ok := annot.(*SchemaAnnotations)
if ok && schemaAnnots.Scope == ruleScope && schemaAnnots.Rule == rule {
return schemaAnnots.SchemaAnnotation
func getRuleAnnotation(rule *Rule) (result []*SchemaAnnotation) {
for _, a := range rule.Module.Annotations {
other, ok := a.Node.(*Rule)
if !ok {
continue
}
if other == rule {
result = append(result, a.Schemas...)
}
}
return nil
return result
}

// NOTE: Currently, annotations must preceed the rule. In the future, this
// restriction could be relaxed with other kinds of annotation scopes.
func processAnnotation(ss *SchemaSet, annot SchemaAnnotation, env *TypeEnv, rule *Rule) (Ref, types.Type, *Error) {
func processAnnotation(ss *SchemaSet, annot *SchemaAnnotation, env *TypeEnv, rule *Rule) (Ref, types.Type, *Error) {
if ss == nil {
return nil, nil, NewError(TypeErr, rule.Location, "schemas need to be supplied for the annotation: %s", annot.Schema)
}

schemaRef, err := ParseRef(annot.Schema)
if err != nil {
return nil, nil, NewError(TypeErr, rule.Location, "schema is not well formed in annotation: %s", annot.Schema)
}

schema := ss.Get(schemaRef)
schema := ss.Get(annot.Schema)
if schema == nil {
return nil, nil, NewError(TypeErr, rule.Location, "schema does not exist for given path in annotation: %s", schemaRef.String())
return nil, nil, NewError(TypeErr, rule.Location, "schema does not exist for given path in annotation: %s", annot.Schema)
}

tpe, err := loadSchema(schema)
if err != nil {
return nil, nil, NewError(TypeErr, rule.Location, err.Error())
}

ref, err := ParseRef(annot.Path)
if err != nil {
return nil, nil, NewError(TypeErr, rule.Location, err.Error())
}

return ref, tpe, nil
return annot.Path, tpe, nil
}
54 changes: 0 additions & 54 deletions ast/check_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1446,57 +1446,6 @@ default allow = false
# scope: rule
# schemas:
# - input: schema["badpath"]
whocan[user] {
access = acl[user]
access[_] == input.operation
}`

module5 := `
package policy
import data.acl
import input
default allow = false
# METADATA
# scope: rule
# schemas:
# - badref: schema["whocan-input-schema"]
whocan[user] {
access = acl[user]
access[_] == input.operation
}`

module6 := `
package policy
import data.acl
import input
default allow = false
# METADATA
# scope: rule
# schemas:
# - data/acl: schema/acl-schema
whocan[user] {
access = acl[user]
access[_] == input.operation
}`

module7 := `
package policy
import data.acl
import input
default allow = false
# METADATA
# scope: rule
# schemas:
# - input= schema["whocan-input-schema"]
whocan[user] {
access = acl[user]
access[_] == input.operation
Expand Down Expand Up @@ -1821,9 +1770,6 @@ whocan[user] {
"correct data override": {module: module2, schemaSet: schemaSet},
"incorrect data override": {module: module3, schemaSet: schemaSet, err: "undefined ref: input.user"},
"schema not exist in annotation path": {module: module4, schemaSet: schemaSet, err: "schema does not exist for given path in annotation"},
"non ref in annotation": {module: module5, schemaSet: schemaSet, err: "expected ref but got"},
"Ill-structured annotation with bad path": {module: module6, schemaSet: schemaSet, err: "schema is not well formed in annotation"},
"Ill-structured (invalid) annotation": {module: module7, schemaSet: schemaSet, err: "unable to unmarshall the metadata yaml in comment"},
"empty schema set": {module: module1, schemaSet: nil, err: "schemas need to be supplied for the annotation"},
"overriding ref with length greater than one and not existing": {module: module8, schemaSet: schemaSet, err: "undefined ref: input.apple.banana"},
"overriding ref with length greater than one and existing prefix": {module: module9, schemaSet: schemaSet},
Expand Down
24 changes: 24 additions & 0 deletions ast/compare.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,9 @@ func Compare(a, b interface{}) int {
case *Package:
b := b.(*Package)
return a.Compare(b)
case *Annotations:
b := b.(*Annotations)
return a.Compare(b)
case *Module:
b := b.(*Module)
return a.Compare(b)
Expand Down Expand Up @@ -251,6 +254,8 @@ func sortOrder(x interface{}) int {
return 1001
case *Package:
return 1002
case *Annotations:
return 1003
case *Module:
return 10000
}
Expand All @@ -276,6 +281,25 @@ func importsCompare(a, b []*Import) int {
return 0
}

func annotationsCompare(a, b []*Annotations) int {
minLen := len(a)
if len(b) < minLen {
minLen = len(b)
}
for i := 0; i < minLen; i++ {
if cmp := a[i].Compare(b[i]); cmp != 0 {
return cmp
}
}
if len(a) < len(b) {
return -1
}
if len(b) < len(a) {
return 1
}
return 0
}

func rulesCompare(a, b []*Rule) int {
minLen := len(a)
if len(b) < minLen {
Expand Down
214 changes: 213 additions & 1 deletion ast/compare_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@

package ast

import "testing"
import (
"testing"
)

func TestCompare(t *testing.T) {

Expand Down Expand Up @@ -97,4 +99,214 @@ import input.x.z`)
if result != -1 {
t.Errorf("Expected %v to be less than %v but got: %v", a, b, result)
}

var err error

a, err = ParseModuleWithOpts("test.rego", `package a
# METADATA
# scope: rule
# schemas:
# - input: schema.a
p := 7`, ParserOptions{ProcessAnnotation: true})
if err != nil {
t.Fatal(err)
}

b, err = ParseModuleWithOpts("test.rego", `package a
# METADATA
# scope: rule
# schemas:
# - input: schema.b
p := 7`, ParserOptions{ProcessAnnotation: true})
if err != nil {
t.Fatal(err)
}

result = Compare(a, b)

if result != -1 {
t.Errorf("Expected %v to be less than %v but got: %v", a, b, result)
}
}

func TestCompareAnnotations(t *testing.T) {

tests := []struct {
note string
a string
b string
exp int
}{
{
note: "same",
a: `
# METADATA
# scope: a`,
b: `
# METADATA
# scope: a`,
exp: 0,
},
{
note: "unknown scope",
a: `
# METADATA
# scope: rule`,
b: `
# METADATA
# scope: a`,
exp: 1,
},
{
note: "unknown scope - less than",
a: `
# METADATA
# scope: a`,
b: `
# METADATA
# scope: rule`,
exp: -1,
},
{
note: "unknown scope - greater than - lexigraphical",
a: `
# METADATA
# scope: b`,
b: `
# METADATA
# scope: a`,
exp: 1,
},
{
note: "unknown scope - less than - lexigraphical",
a: `
# METADATA
# scope: b`,
b: `
# METADATA
# scope: c`,
exp: -1,
},
{
note: "schema",
a: `
# METADATA
# scope: rule
# schemas:
# - input: schema`,
b: `
# METADATA
# scope: rule
# schemas:
# - input: schema`,
exp: 0,
},
{
note: "schema - less than",
a: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema`,
b: `
# METADATA
# scope: rule
# schemas:
# - input.b: schema`,
exp: -1,
},
{
note: "schema - greater than",
a: `
# METADATA
# scope: rule
# schemas:
# - input.b: schema`,
b: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema`,
exp: 1,
},
{
note: "schema - less than (fewer)",
a: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema`,
b: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema
# - input.b: schema`,
exp: -1,
},
{
note: "schema - greater than (more)",
a: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema
# - input.b: schema`,
b: `
# METADATA
# scope: rule
# schemas:
# - input.a: schema`,
exp: 1,
},
{
note: "schema - less than - lexigraphical",
a: `
# METADATA
# scope: rule
# schemas:
# - input: schema.a`,
b: `
# METADATA
# scope: rule
# schemas:
# - input: schema.b`,
exp: -1,
},
{
note: "schema - greater than - lexigraphical",
a: `
# METADATA
# scope: rule
# schemas:
# - input: schema.c`,
b: `
# METADATA
# scope: rule
# schemas:
# - input: schema.b`,
exp: 1,
},
}

for _, tc := range tests {
t.Run(tc.note, func(t *testing.T) {
stmts, _, err := ParseStatementsWithOpts("test.rego", tc.a, ParserOptions{ProcessAnnotation: true})
if err != nil {
t.Fatal(err)
}
a := stmts[0].(*Annotations)
stmts, _, err = ParseStatementsWithOpts("test.rego", tc.b, ParserOptions{ProcessAnnotation: true})
if err != nil {
t.Fatal(err)
}
b := stmts[0].(*Annotations)
result := a.Compare(b)
if result != tc.exp {
t.Fatalf("Expected %d but got %v for %v and %v", tc.exp, result, a, b)
}
})
}
}
Loading

0 comments on commit d3adb90

Please sign in to comment.