From 898d010050285320190d9cd13d4940a11f1d5b4d Mon Sep 17 00:00:00 2001 From: Torin Sandall Date: Thu, 1 Apr 2021 16:36:26 -0400 Subject: [PATCH] ast: Refactor SchemaSet to hide ByPath collection This commit does not change any functionality it just refactors the schema implementation a bit: * There is no reason to expose the ByPath collection for now. This change simplifies things for the caller because they can just perform get/put operations on the SchemaSet. * Rename setTypesWithSchema to loadSchema. * Move schema code into a separate file with it's own test cases separate from the compiler. Signed-off-by: Torin Sandall --- ast/check.go | 14 ++-- ast/check_test.go | 8 +-- ast/compile.go | 60 ++++------------ ast/compile_test.go | 154 +--------------------------------------- ast/schema.go | 60 ++++++++++++++++ ast/schema_test.go | 158 ++++++++++++++++++++++++++++++++++++++++++ cmd/eval_test.go | 4 +- loader/loader.go | 4 +- loader/loader_test.go | 4 +- rego/rego_test.go | 2 +- 10 files changed, 255 insertions(+), 213 deletions(-) create mode 100644 ast/schema.go create mode 100644 ast/schema_test.go diff --git a/ast/check.go b/ast/check.go index 91992795a2..8ec51250d5 100644 --- a/ast/check.go +++ b/ast/check.go @@ -1105,25 +1105,29 @@ func getRuleAnnotation(rule *Rule) (sannots []SchemaAnnotation) { // NOTE: Currently, annotations must preceed the rule. In the future, this // restriction could be relaxed with other kinds of annotation scopes. func processAnnotation(annot SchemaAnnotation, env *TypeEnv, rule *Rule) (Ref, types.Type, *Error) { - if env.schemaSet == nil || env.schemaSet.ByPath == nil { + if env.schemaSet == 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, ok := env.schemaSet.ByPath.Get(schemaRef) - if !ok { + + schema := env.schemaSet.Get(schemaRef) + if schema == nil { return nil, nil, NewError(TypeErr, rule.Location, "schema does not exist for given path in annotation: %s", schemaRef.String()) } - newType, err := setTypesWithSchema(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, newType, nil + return ref, tpe, nil } diff --git a/ast/check_test.go b/ast/check_test.go index 288c5d3b79..a6e71b7041 100644 --- a/ast/check_test.go +++ b/ast/check_test.go @@ -1808,9 +1808,9 @@ whocan[user] { }` schemaSet := NewSchemaSet() - schemaSet.ByPath.Put(MustParseRef("schema.input"), ischema) - schemaSet.ByPath.Put(MustParseRef(`schema["whocan-input-schema"]`), ischema2) - schemaSet.ByPath.Put(MustParseRef(`schema["acl-schema"]`), dschema) + schemaSet.Put(MustParseRef("schema.input"), ischema) + schemaSet.Put(MustParseRef(`schema["whocan-input-schema"]`), ischema2) + schemaSet.Put(MustParseRef(`schema["acl-schema"]`), dschema) tests := map[string]struct { module string @@ -1937,7 +1937,7 @@ q = p`, t.Fatal(err) } - ss.ByPath.Put(ref, schema) + ss.Put(ref, schema) } compiler := NewCompiler().WithSchemas(ss) diff --git a/ast/compile.go b/ast/compile.go index 85a61f97d6..513dfa5650 100644 --- a/ast/compile.go +++ b/ast/compile.go @@ -109,25 +109,6 @@ type Compiler struct { schemaSet *SchemaSet // user-supplied schemas for input and data documents } -// SchemaSet holds a map from a path to a schema -type SchemaSet struct { - ByPath *util.HashMap -} - -// NewSchemaSet returns an empty SchemaSet. -func NewSchemaSet() *SchemaSet { - - eqFunc := func(a, b util.T) bool { - return a.(Ref).Equal(b.(Ref)) - } - - hashFunc := func(x util.T) int { return x.(Ref).Hash() } - - return &SchemaSet{ - ByPath: util.NewHashMap(eqFunc, hashFunc), - } -} - // CompilerStage defines the interface for stages in the compiler. type CompilerStage func(*Compiler) *Error @@ -955,20 +936,6 @@ func parseSchema(schema interface{}) (types.Type, error) { return types.A, nil } -func setTypesWithSchema(schema interface{}) (types.Type, error) { - goJSONSchema, err := compileSchema(schema) - if err != nil { - return nil, fmt.Errorf("compile failed: %s", err.Error()) - } - - newtype, err := parseSchema(goJSONSchema.RootSchema) - if err != nil { - return nil, fmt.Errorf("error when type checking %v", err) - } - - return newtype, nil -} - // checkTypes runs the type checker on all rules. The type checker builds a // TypeEnv that is stored on the compiler. func (c *Compiler) checkTypes() { @@ -1058,20 +1025,23 @@ func (c *Compiler) init() { func (c *Compiler) setSchemas() { if c.schemaSet != nil { - if c.schemaSet.ByPath != nil { - // First, set the schemaSet in the type environment - c.TypeEnv.WithSchemas(c.schemaSet) - // Second, set the schema for the input globally - schema, ok := c.schemaSet.ByPath.Get(InputRootRef) - if ok { - newtype, err := setTypesWithSchema(schema) - if err != nil { - c.err(NewError(TypeErr, nil, err.Error())) - } - c.TypeEnv.tree.PutOne(VarTerm("input").Value, newtype) - } + // First, set the schemaSet in the type environment + c.TypeEnv.WithSchemas(c.schemaSet) + + // Second, set the schema for the input globally if it exists + schema := c.schemaSet.Get(InputRootRef) + if schema == nil { + return } + + tpe, err := loadSchema(schema) + if err != nil { + c.err(NewError(TypeErr, nil, err.Error())) + return + } + + c.TypeEnv.tree.Put(InputRootRef, tpe) } } diff --git a/ast/compile_test.go b/ast/compile_test.go index 7cc18adfce..abb072c96f 100644 --- a/ast/compile_test.go +++ b/ast/compile_test.go @@ -869,7 +869,7 @@ func TestCompilerCheckTypesWithSchema(t *testing.T) { t.Fatal("Unexpected error:", err) } schemaSet := NewSchemaSet() - schemaSet.ByPath.Put(InputRootRef, schema) + schemaSet.Put(InputRootRef, schema) c.WithSchemas(schemaSet) compileStages(c, c.checkTypes) assertNotFailed(t, c) @@ -4302,160 +4302,10 @@ func TestCompilerPassesTypeCheckNegative(t *testing.T) { } } -func testParseSchema(t *testing.T, schema string, expectedType types.Type) { - var sch interface{} - err := util.Unmarshal([]byte(schema), &sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - newtype, err := setTypesWithSchema(sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if newtype == nil { - t.Fatalf("parseSchema returned nil type") - } - if types.Compare(newtype, expectedType) != 0 { - t.Fatalf("parseSchema returned an incorrect type: %s, expected: %s", newtype.String(), expectedType.String()) - } -} - -func TestParseSchemaObject(t *testing.T) { - //Expected type is: object, c: any>>, foo: string> - innerObjectStaticProps := []*types.StaticProperty{} - innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "a", Value: types.N}) - innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "b", Value: types.NewArray([]types.Type{types.N}, nil)}) - innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "c", Value: types.A}) - innerObjectType := types.NewObject(innerObjectStaticProps, nil) - - staticProps := []*types.StaticProperty{} - staticProps = append(staticProps, &types.StaticProperty{Key: "b", Value: types.NewArray([]types.Type{innerObjectType}, nil)}) - staticProps = append(staticProps, &types.StaticProperty{Key: "foo", Value: types.S}) - - expectedType := types.NewObject(staticProps, nil) - testParseSchema(t, objectSchema, expectedType) -} - -func TestSetTypesWithSchemaRef(t *testing.T) { - var sch interface{} - err := util.Unmarshal([]byte(refSchema), &sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - newtype, err := setTypesWithSchema(sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if newtype == nil { - t.Fatalf("parseSchema returned nil type") - } - if newtype.String() != "object, generateName: string, generation: number, initializers: object>, result: object>, group: string, kind: string, name: string, retryAfterSeconds: number, uid: string>, kind: string, message: string, metadata: object, reason: string, status: string>>, labels: object[any: any], managedFields: array>, name: string, namespace: string, ownerReferences: array>, resourceVersion: string, selfLink: string, uid: string>>" { - t.Fatalf("parseSchema returned an incorrect type: %s", newtype.String()) - } -} - -func TestSetTypesWithPodSchema(t *testing.T) { - var sch interface{} - err := util.Unmarshal([]byte(podSchema), &sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - newtype, err := setTypesWithSchema(sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - if newtype == nil { - t.Fatalf("parseSchema returned nil type") - } - if newtype.String() == "object" { - t.Fatalf("parseSchema returned an incorrect type: %s", newtype.String()) - } - -} - -func TestParseSchemaUntypedField(t *testing.T) { - //Expected type is: object - staticProps := []*types.StaticProperty{} - staticProps = append(staticProps, &types.StaticProperty{Key: "foo", Value: types.A}) - expectedType := types.NewObject(staticProps, nil) - testParseSchema(t, untypedFieldObjectSchema, expectedType) -} - -func TestParseSchemaNoChildren(t *testing.T) { - //Expected type is: object[any: any] - expectedType := types.NewObject(nil, &types.DynamicProperty{Key: types.A, Value: types.A}) - testParseSchema(t, noChildrenObjectSchema, expectedType) -} - -func TestParseSchemaArrayNoItems(t *testing.T) { - //Expected type is: object - staticProps := []*types.StaticProperty{} - staticProps = append(staticProps, &types.StaticProperty{Key: "b", Value: types.NewArray(nil, types.A)}) - expectedType := types.NewObject(staticProps, nil) - testParseSchema(t, arrayNoItemsSchema, expectedType) -} - -func TestParseSchemaBooleanField(t *testing.T) { - //Expected type is: object - staticProps := []*types.StaticProperty{} - staticProps = append(staticProps, &types.StaticProperty{Key: "a", Value: types.B}) - expectedType := types.NewObject(staticProps, nil) - testParseSchema(t, booleanSchema, expectedType) -} - -func TestParseSchemaBasics(t *testing.T) { - tests := []struct { - note string - schema string - exp types.Type - }{ - { - note: "number", - schema: `{"type": "number"}`, - exp: types.N, - }, - } - - for _, tc := range tests { - t.Run(tc.note, func(t *testing.T) { - testParseSchema(t, tc.schema, tc.exp) - }) - } -} - -func TestCompileSchemaEmptySchema(t *testing.T) { - schema := "" - var sch interface{} - err := util.Unmarshal([]byte(schema), &sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - jsonSchema, _ := compileSchema(sch) - if jsonSchema != nil { - t.Fatalf("Incorrect return from parseSchema with an empty schema") - } -} - -func TestParseSchemaWithSchemaBadSchema(t *testing.T) { - var sch interface{} - err := util.Unmarshal([]byte(objectSchema), &sch) - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - jsonSchema, err := compileSchema(sch) - if err != nil { - t.Fatalf("Unable to compile schema") - } - newtype, err := parseSchema(jsonSchema) // Did not pass the subschema - if newtype != nil { - t.Fatalf("Incorrect return from parseSchema with a bad schema") - } -} - func TestWithSchema(t *testing.T) { c := NewCompiler() schemaSet := NewSchemaSet() - schemaSet.ByPath.Put(InputRootRef, objectSchema) + schemaSet.Put(InputRootRef, objectSchema) c.WithSchemas(schemaSet) if c.schemaSet == nil { t.Fatalf("WithSchema did not set the schema correctly in the compiler") diff --git a/ast/schema.go b/ast/schema.go new file mode 100644 index 0000000000..86a0e7ce26 --- /dev/null +++ b/ast/schema.go @@ -0,0 +1,60 @@ +// Copyright 2021 The OPA Authors. All rights reserved. +// Use of this source code is governed by an Apache2 +// license that can be found in the LICENSE file. + +package ast + +import ( + "fmt" + + "github.com/open-policy-agent/opa/types" + "github.com/open-policy-agent/opa/util" +) + +// SchemaSet holds a map from a path to a schema. +type SchemaSet struct { + m *util.HashMap +} + +// NewSchemaSet returns an empty SchemaSet. +func NewSchemaSet() *SchemaSet { + + eqFunc := func(a, b util.T) bool { + return a.(Ref).Equal(b.(Ref)) + } + + hashFunc := func(x util.T) int { return x.(Ref).Hash() } + + return &SchemaSet{ + m: util.NewHashMap(eqFunc, hashFunc), + } +} + +// Put inserts a raw schema into the set. +func (ss *SchemaSet) Put(path Ref, raw interface{}) { + ss.m.Put(path, raw) +} + +// Get returns the raw schema identified by the path. +func (ss *SchemaSet) Get(path Ref) interface{} { + x, ok := ss.m.Get(path) + if !ok { + return nil + } + return x +} + +func loadSchema(raw interface{}) (types.Type, error) { + + jsonSchema, err := compileSchema(raw) + if err != nil { + return nil, fmt.Errorf("compile failed: %s", err.Error()) + } + + tpe, err := parseSchema(jsonSchema.RootSchema) + if err != nil { + return nil, fmt.Errorf("error when type checking %v", err) + } + + return tpe, nil +} diff --git a/ast/schema_test.go b/ast/schema_test.go new file mode 100644 index 0000000000..1e017db498 --- /dev/null +++ b/ast/schema_test.go @@ -0,0 +1,158 @@ +package ast + +import ( + "testing" + + "github.com/open-policy-agent/opa/types" + "github.com/open-policy-agent/opa/util" +) + +func testParseSchema(t *testing.T, schema string, expectedType types.Type) { + var sch interface{} + err := util.Unmarshal([]byte(schema), &sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + newtype, err := loadSchema(sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if newtype == nil { + t.Fatalf("parseSchema returned nil type") + } + if types.Compare(newtype, expectedType) != 0 { + t.Fatalf("parseSchema returned an incorrect type: %s, expected: %s", newtype.String(), expectedType.String()) + } +} + +func TestParseSchemaObject(t *testing.T) { + //Expected type is: object, c: any>>, foo: string> + innerObjectStaticProps := []*types.StaticProperty{} + innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "a", Value: types.N}) + innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "b", Value: types.NewArray([]types.Type{types.N}, nil)}) + innerObjectStaticProps = append(innerObjectStaticProps, &types.StaticProperty{Key: "c", Value: types.A}) + innerObjectType := types.NewObject(innerObjectStaticProps, nil) + + staticProps := []*types.StaticProperty{} + staticProps = append(staticProps, &types.StaticProperty{Key: "b", Value: types.NewArray([]types.Type{innerObjectType}, nil)}) + staticProps = append(staticProps, &types.StaticProperty{Key: "foo", Value: types.S}) + + expectedType := types.NewObject(staticProps, nil) + testParseSchema(t, objectSchema, expectedType) +} + +func TestSetTypesWithSchemaRef(t *testing.T) { + var sch interface{} + err := util.Unmarshal([]byte(refSchema), &sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + newtype, err := loadSchema(sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if newtype == nil { + t.Fatalf("parseSchema returned nil type") + } + if newtype.String() != "object, generateName: string, generation: number, initializers: object>, result: object>, group: string, kind: string, name: string, retryAfterSeconds: number, uid: string>, kind: string, message: string, metadata: object, reason: string, status: string>>, labels: object[any: any], managedFields: array>, name: string, namespace: string, ownerReferences: array>, resourceVersion: string, selfLink: string, uid: string>>" { + t.Fatalf("parseSchema returned an incorrect type: %s", newtype.String()) + } +} + +func TestSetTypesWithPodSchema(t *testing.T) { + var sch interface{} + err := util.Unmarshal([]byte(podSchema), &sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + newtype, err := loadSchema(sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if newtype == nil { + t.Fatalf("parseSchema returned nil type") + } + if newtype.String() == "object" { + t.Fatalf("parseSchema returned an incorrect type: %s", newtype.String()) + } + +} + +func TestParseSchemaUntypedField(t *testing.T) { + //Expected type is: object + staticProps := []*types.StaticProperty{} + staticProps = append(staticProps, &types.StaticProperty{Key: "foo", Value: types.A}) + expectedType := types.NewObject(staticProps, nil) + testParseSchema(t, untypedFieldObjectSchema, expectedType) +} + +func TestParseSchemaNoChildren(t *testing.T) { + //Expected type is: object[any: any] + expectedType := types.NewObject(nil, &types.DynamicProperty{Key: types.A, Value: types.A}) + testParseSchema(t, noChildrenObjectSchema, expectedType) +} + +func TestParseSchemaArrayNoItems(t *testing.T) { + //Expected type is: object + staticProps := []*types.StaticProperty{} + staticProps = append(staticProps, &types.StaticProperty{Key: "b", Value: types.NewArray(nil, types.A)}) + expectedType := types.NewObject(staticProps, nil) + testParseSchema(t, arrayNoItemsSchema, expectedType) +} + +func TestParseSchemaBooleanField(t *testing.T) { + //Expected type is: object + staticProps := []*types.StaticProperty{} + staticProps = append(staticProps, &types.StaticProperty{Key: "a", Value: types.B}) + expectedType := types.NewObject(staticProps, nil) + testParseSchema(t, booleanSchema, expectedType) +} + +func TestParseSchemaBasics(t *testing.T) { + tests := []struct { + note string + schema string + exp types.Type + }{ + { + note: "number", + schema: `{"type": "number"}`, + exp: types.N, + }, + } + + for _, tc := range tests { + t.Run(tc.note, func(t *testing.T) { + testParseSchema(t, tc.schema, tc.exp) + }) + } +} + +func TestCompileSchemaEmptySchema(t *testing.T) { + schema := "" + var sch interface{} + err := util.Unmarshal([]byte(schema), &sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + jsonSchema, _ := compileSchema(sch) + if jsonSchema != nil { + t.Fatalf("Incorrect return from parseSchema with an empty schema") + } +} + +func TestParseSchemaWithSchemaBadSchema(t *testing.T) { + var sch interface{} + err := util.Unmarshal([]byte(objectSchema), &sch) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + jsonSchema, err := compileSchema(sch) + if err != nil { + t.Fatalf("Unable to compile schema") + } + newtype, err := parseSchema(jsonSchema) // Did not pass the subschema + if newtype != nil { + t.Fatalf("Incorrect return from parseSchema with a bad schema") + } +} diff --git a/cmd/eval_test.go b/cmd/eval_test.go index 36f469948e..80725d151e 100755 --- a/cmd/eval_test.go +++ b/cmd/eval_test.go @@ -299,12 +299,12 @@ func testReadParamWithSchemaDir(t *testing.T, input string, query string, inputS return } - if _, ok := schemaSet.ByPath.Get(ast.MustParseRef("schema.input")); !ok { + if schemaSet.Get(ast.MustParseRef("schema.input")) == nil { err = fmt.Errorf("Expected schema for input in schemaSet but got none") return } - if _, ok := schemaSet.ByPath.Get(ast.MustParseRef(`schema.kubernetes["data-schema"]`)); !ok { + if schemaSet.Get(ast.MustParseRef(`schema.kubernetes["data-schema"]`)) == nil { err = fmt.Errorf("Expected schemas for data in schemaSet but got none") return } diff --git a/loader/loader.go b/loader/loader.go index 414f1126a7..a4c07ced77 100644 --- a/loader/loader.go +++ b/loader/loader.go @@ -282,7 +282,7 @@ func loadSchemas(schemaPath string) (*ast.SchemaSet, error) { if err != nil { return nil, err } - ss.ByPath.Put(ast.InputRootRef, schema) + ss.Put(ast.InputRootRef, schema) return ss, nil } @@ -309,7 +309,7 @@ func loadSchemas(schemaPath string) (*ast.SchemaSet, error) { } key := getSchemaSetByPathKey(relPath) - ss.ByPath.Put(key, schema) + ss.Put(key, schema) return nil }) diff --git a/loader/loader_test.go b/loader/loader_test.go index 5f213ebbdb..9d52fce9f4 100644 --- a/loader/loader_test.go +++ b/loader/loader_test.go @@ -805,8 +805,8 @@ func TestSchemas(t *testing.T) { key := ast.MustParseRef(k) var schema interface{} util.Unmarshal([]byte(v), &schema) - result, ok := ss.ByPath.Get(key) - if !ok { + result := ss.Get(key) + if result == nil { t.Fatalf("expected schema with key %v", key) } if !reflect.DeepEqual(schema, result) { diff --git a/rego/rego_test.go b/rego/rego_test.go index 021592323d..de757c8daa 100644 --- a/rego/rego_test.go +++ b/rego/rego_test.go @@ -1862,7 +1862,7 @@ func TestPrepareAndCompileWithSchema(t *testing.T) { err := util.Unmarshal([]byte(schemaBytes), &schema) schemaSet := ast.NewSchemaSet() - schemaSet.ByPath.Put(ast.InputRootRef, schema) + schemaSet.Put(ast.InputRootRef, schema) r := New( Query("data.test.x"),