From 037101cd7c2d40e2fce163298f742c3ae89da0cf Mon Sep 17 00:00:00 2001 From: Anders Eknert Date: Fri, 6 Mar 2026 23:07:35 +0100 Subject: [PATCH] Linter configuration cleanup (#8397) And enable more staticcheck linters. I saw staticcheck failures mentioned in another PR, so thought I'd check it out. - `WriteString(fmt.Sprintf)` -> `fmt.Fprintf` - Rewrite calls to deprecated `*Rule.Path()` - Don't use `==` to compare `time.Time` - Use inline ignores over config exclusions of paths - Remove 'varcheck' ignores as no longer used - Remove v0 topdown/graphql.go (!) Signed-off-by: Anders Eknert --- .golangci.yaml | 175 ++------- ast/builtins.go | 2 +- ast/visit.go | 4 +- internal/compiler/wasm/wasm.go | 1 - internal/gojsonschema/utils.go | 2 +- internal/uuid/uuid.go | 2 +- server/types/types.go | 2 +- topdown/graphql.go | 481 ------------------------- topdown/trace.go | 2 +- v1/ast/parser_test.go | 2 +- v1/ast/policy_appenders.go | 18 + v1/capabilities/capabilities.go | 2 +- v1/compile/compile.go | 4 +- v1/cover/cover.go | 17 +- v1/cover/cover_bench_test.go | 6 +- v1/dependencies/deps.go | 14 +- v1/dependencies/deps_bench_test.go | 25 +- v1/plugins/bundle/plugin.go | 5 +- v1/plugins/bundle/plugin_test.go | 12 +- v1/plugins/discovery/discovery.go | 4 +- v1/plugins/discovery/discovery_test.go | 12 +- v1/profiler/profiler_bench_test.go | 6 +- v1/rego/rego.go | 8 +- v1/repl/repl.go | 7 +- v1/runtime/runtime.go | 2 +- v1/server/server.go | 10 +- v1/tester/runner.go | 44 ++- v1/topdown/eval.go | 29 +- v1/topdown/graphql.go | 10 +- v1/topdown/trace.go | 2 +- 30 files changed, 165 insertions(+), 745 deletions(-) delete mode 100644 topdown/graphql.go diff --git a/.golangci.yaml b/.golangci.yaml index b2001541ea..274f2303aa 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -67,7 +67,24 @@ linters: disabled: true - name: unused-receiver disabled: false + staticcheck: + checks: + # https://staticcheck.dev/docs/checks/# for more details on these rules + [ + "all", + "-ST1000", + "-ST1003", + "-ST1016", + "-ST1020", + "-ST1021", + "-ST1022", + "-QF1001", + "-QF1002", + "-QF1003", + "-QF1008", + ] exclusions: + warn-unused: true generated: lax presets: - comments @@ -77,159 +94,14 @@ linters: rules: - linters: - staticcheck - path: ast/ - text: SA1019 + text: This package is intended for older projects transitioning from OPA v0.x + path-except: v1/ - linters: - staticcheck - path: bundle/ - text: SA1019 - - linters: - - staticcheck - path: capabilities/ - text: SA1019 - - linters: - - staticcheck - path: compile/ - text: SA1019 - - linters: - - staticcheck - path: config/ - text: SA1019 - - linters: - - staticcheck - path: cover/ - text: SA1019 - - linters: - - staticcheck - path: debug/ - text: SA1019 - - linters: - - staticcheck - path: dependencies/ - text: SA1019 - - linters: - - staticcheck - path: download/ - text: SA1019 - - linters: - - staticcheck - path: format/ - text: SA1019 - - linters: - - staticcheck - path: hooks/ - text: SA1019 - - linters: - - staticcheck - path: ir/ - text: SA1019 - - linters: - - staticcheck - path: keys/ - text: SA1019 - - linters: - - staticcheck - path: loader/ - text: SA1019 - - linters: - - staticcheck - path: logging/ - text: SA1019 - - linters: - - staticcheck - path: metrics/ - text: SA1019 - - linters: - - staticcheck - path: plugins/ - text: SA1019 - - linters: - - staticcheck - path: profiler/ - text: SA1019 - - linters: - - staticcheck - path: refactor/ - text: SA1019 - - linters: - - staticcheck - path: repl/ - text: SA1019 - - linters: - - staticcheck - path: rego/ - text: SA1019 - - linters: - - staticcheck - path: resolver/ - text: SA1019 - - linters: - - staticcheck - path: runtime/ - text: SA1019 - - linters: - - staticcheck - path: schemas/ - text: SA1019 - - linters: - - staticcheck - path: sdk/ - text: SA1019 - - linters: - - staticcheck - path: server/ - text: SA1019 - - linters: - - staticcheck - path: storage/ - text: SA1019 - - linters: - - staticcheck - path: tester/ - text: SA1019 - - linters: - - staticcheck - path: topdown/ - text: SA1019 - - linters: - - staticcheck - path: tracing/ - text: SA1019 - - linters: - - staticcheck - path: types/ - text: SA1019 - - linters: - - staticcheck - path: util/ - text: SA1019 - - linters: - - staticcheck - path: version/ - text: SA1019 - - linters: - - staticcheck - text: QF1001 - - linters: - - staticcheck - text: QF1002 - - linters: - - staticcheck - text: QF1003 - - linters: - - staticcheck - text: QF1008 - - linters: - - staticcheck - text: QF1009 - - linters: - - staticcheck - text: QF1012 + text: SA1019 # Using a deprecated function, variable, constant or field + path: _test\.go paths: - internal/gojsonschema - - third_party$ - - builtin$ - - examples$ issues: # don't hide issues in CI runs because they are the same type max-same-issues: 0 @@ -239,8 +111,3 @@ formatters: - goimports exclusions: generated: lax - paths: - - internal/gojsonschema - - third_party$ - - builtin$ - - examples$ diff --git a/ast/builtins.go b/ast/builtins.go index d0ab69a163..65de5abef2 100644 --- a/ast/builtins.go +++ b/ast/builtins.go @@ -29,7 +29,7 @@ var BuiltinMap = v1.BuiltinMap // Deprecated: Builtins can now be directly annotated with the // Nondeterministic property, and when set to true, will be ignored // for partial evaluation. -var IgnoreDuringPartialEval = v1.IgnoreDuringPartialEval +var IgnoreDuringPartialEval = v1.IgnoreDuringPartialEval //nolint:staticcheck /** * Unification diff --git a/ast/visit.go b/ast/visit.go index f785b8c104..50028269d2 100644 --- a/ast/visit.go +++ b/ast/visit.go @@ -12,13 +12,13 @@ import v1 "github.com/open-policy-agent/opa/v1/ast" // visited. // // Deprecated: use GenericVisitor or another visitor implementation -type Visitor = v1.Visitor +type Visitor = v1.Visitor //nolint:staticcheck // BeforeAndAfterVisitor wraps Visitor to provide hooks for being called before // and after the AST has been visited. // // Deprecated: use GenericVisitor or another visitor implementation -type BeforeAndAfterVisitor = v1.BeforeAndAfterVisitor +type BeforeAndAfterVisitor = v1.BeforeAndAfterVisitor //nolint:staticcheck // Walk iterates the AST by calling the Visit function on the Visitor // v for x before recursing. diff --git a/internal/compiler/wasm/wasm.go b/internal/compiler/wasm/wasm.go index e531a9b9b9..b7f1a27812 100644 --- a/internal/compiler/wasm/wasm.go +++ b/internal/compiler/wasm/wasm.go @@ -32,7 +32,6 @@ const ( opaWasmABIMinorVersionVar = "opa_wasm_abi_minor_version" ) -// nolint: varcheck const ( opaTypeNull int32 = iota + 1 opaTypeBoolean diff --git a/internal/gojsonschema/utils.go b/internal/gojsonschema/utils.go index 95754fab7f..a8639d4d9a 100644 --- a/internal/gojsonschema/utils.go +++ b/internal/gojsonschema/utils.go @@ -23,7 +23,7 @@ // // created 26-02-2013 -// nolint:unused,varcheck // Package in development (2021). +// nolint:unused // Package in development (2021). package gojsonschema import ( diff --git a/internal/uuid/uuid.go b/internal/uuid/uuid.go index a18f024a25..63e1a5b071 100644 --- a/internal/uuid/uuid.go +++ b/internal/uuid/uuid.go @@ -86,7 +86,7 @@ func byteDecimalToHexMAC(bytes []byte, sep string) string { hexs.Grow((l * 3) - 1) // 1 byte -> 2 hexes + 1 separator (if one char) for i, b := range bytes { - hexs.WriteString(fmt.Sprintf("%02x", b)) + fmt.Fprintf(&hexs, "%02x", b) if i < l-1 { hexs.WriteString(sep) } diff --git a/server/types/types.go b/server/types/types.go index 0fa30beaf9..c602d60c75 100644 --- a/server/types/types.go +++ b/server/types/types.go @@ -203,7 +203,7 @@ const ( // of the health API. // // Deprecated: Use ParamBundlesActivationV1 instead. - ParamBundleActivationV1 = v1.ParamBundleActivationV1 + ParamBundleActivationV1 = v1.ParamBundleActivationV1 //nolint:staticcheck // ParamBundlesActivationV1 defines the name of the HTTP URL parameter that // indicates the client wants to include bundle activation in the results diff --git a/topdown/graphql.go b/topdown/graphql.go deleted file mode 100644 index bfe3a450aa..0000000000 --- a/topdown/graphql.go +++ /dev/null @@ -1,481 +0,0 @@ -// Copyright 2022 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 topdown - -import ( - "encoding/json" - "fmt" - "strings" - - gqlast "github.com/vektah/gqlparser/v2/ast" - gqlparser "github.com/vektah/gqlparser/v2/parser" - gqlvalidator "github.com/vektah/gqlparser/v2/validator" - - // Side-effecting import. Triggers GraphQL library's validation rule init() functions. - _ "github.com/vektah/gqlparser/v2/validator/rules" - - "github.com/open-policy-agent/opa/v1/ast" - "github.com/open-policy-agent/opa/v1/topdown/builtins" -) - -var unverified = ast.ArrayTerm(ast.InternedTerm(false), ast.InternedEmptyObject, ast.InternedEmptyObject) - -// Parses a GraphQL schema, and returns the GraphQL AST for the schema. -func parseSchema(schema string) (*gqlast.SchemaDocument, error) { - // NOTE(philipc): We don't include the "built-in schema defs" from the - // underlying graphql parsing library here, because those definitions - // generate enormous AST blobs. In the future, if there is demand for - // a "full-spec" version of schema ASTs, we may need to provide a - // version of this function that includes the built-in schema - // definitions. - schemaAST, err := gqlparser.ParseSchema(&gqlast.Source{Input: schema}) - if err != nil { - errorParts := strings.SplitN(err.Error(), ":", 4) - msg := strings.TrimLeft(errorParts[3], " ") - return nil, fmt.Errorf("%s in GraphQL string at location %s:%s", msg, errorParts[1], errorParts[2]) - } - return schemaAST, nil -} - -// Parses a GraphQL query, and returns the GraphQL AST for the query. -func parseQuery(query string) (*gqlast.QueryDocument, error) { - queryAST, err := gqlparser.ParseQuery(&gqlast.Source{Input: query}) - if err != nil { - errorParts := strings.SplitN(err.Error(), ":", 4) - msg := strings.TrimLeft(errorParts[3], " ") - return nil, fmt.Errorf("%s in GraphQL string at location %s:%s", msg, errorParts[1], errorParts[2]) - } - return queryAST, nil -} - -// Validates a GraphQL query against a schema, and returns an error. -// In this case, we get a wrappered error list type, and pluck out -// just the first error message in the list. -func validateQuery(schema *gqlast.Schema, query *gqlast.QueryDocument) error { - // Validate the query against the schema, erroring if there's an issue. - err := gqlvalidator.Validate(schema, query) - if err != nil { - // We use strings.TrimSuffix to remove the '.' characters that the library - // authors include on most of their validation errors. This should be safe, - // since variable names in their error messages are usually quoted, and - // this affects only the last character(s) in the string. - // NOTE(philipc): We know the error location will be in the query string, - // because schema validation always happens before this function is called. - errorParts := strings.SplitN(err.Error(), ":", 4) - msg := strings.TrimSuffix(strings.TrimLeft(errorParts[3], " "), ".\n") - return fmt.Errorf("%s in GraphQL query string at location %s:%s", msg, errorParts[1], errorParts[2]) - } - return nil -} - -func getBuiltinSchema() *gqlast.SchemaDocument { - schema, err := gqlparser.ParseSchema(gqlvalidator.Prelude) - if err != nil { - panic(fmt.Errorf("Error in gqlparser Prelude (should be impossible): %w", err)) - } - return schema -} - -// NOTE(philipc): This function expects *validated* schema documents, and will break -// if it is fed arbitrary structures. -func mergeSchemaDocuments(docA *gqlast.SchemaDocument, docB *gqlast.SchemaDocument) *gqlast.SchemaDocument { - ast := &gqlast.SchemaDocument{} - ast.Merge(docA) - ast.Merge(docB) - return ast -} - -// Converts a SchemaDocument into a gqlast.Schema object that can be used for validation. -// It merges in the builtin schema typedefs exactly as gqltop.LoadSchema did internally. -func convertSchema(schemaDoc *gqlast.SchemaDocument) (*gqlast.Schema, error) { - // Merge builtin schema + schema we were provided. - builtinsSchemaDoc := getBuiltinSchema() - mergedSchemaDoc := mergeSchemaDocuments(builtinsSchemaDoc, schemaDoc) - schema, err := gqlvalidator.ValidateSchemaDocument(mergedSchemaDoc) - if err != nil { - return nil, fmt.Errorf("Error in gqlparser SchemaDocument to Schema conversion: %w", err) - } - return schema, nil -} - -// Converts an ast.Object into a gqlast.QueryDocument object. -func objectToQueryDocument(value ast.Object) (*gqlast.QueryDocument, error) { - // Convert ast.Term to any for JSON encoding below. - asJSON, err := ast.JSON(value) - if err != nil { - return nil, err - } - // Marshal to JSON. - bs, err := json.Marshal(asJSON) - if err != nil { - return nil, err - } - // Unmarshal from JSON -> gqlast.QueryDocument. - var result gqlast.QueryDocument - err = json.Unmarshal(bs, &result) - if err != nil { - return nil, err - } - return &result, nil -} - -// Converts an ast.Object into a gqlast.SchemaDocument object. -func objectToSchemaDocument(value ast.Object) (*gqlast.SchemaDocument, error) { - // Convert ast.Term to any for JSON encoding below. - asJSON, err := ast.JSON(value) - if err != nil { - return nil, err - } - // Marshal to JSON. - bs, err := json.Marshal(asJSON) - if err != nil { - return nil, err - } - // Unmarshal from JSON -> gqlast.SchemaDocument. - var result gqlast.SchemaDocument - err = json.Unmarshal(bs, &result) - if err != nil { - return nil, err - } - return &result, nil -} - -// Recursively traverses an AST that has been run through InterfaceToValue, -// and prunes away the fields with null or empty values, and all `Position` -// structs. -// NOTE(philipc): We currently prune away null values to reduce the level -// of clutter in the returned AST objects. In the future, if there is demand -// for ASTs that have a more regular/fixed structure, we may need to provide -// a "raw" version of the AST, where we still prune away the `Position` -// structs, but leave in the null fields. -func pruneIrrelevantGraphQLASTNodes(value ast.Value) ast.Value { - // We iterate over the Value we've been provided, and recurse down - // in the case of complex types, such as Arrays/Objects. - // We are guaranteed to only have to deal with standard JSON types, - // so this is much less ugly than what we'd need for supporting every - // extant ast type! - switch x := value.(type) { - case *ast.Array: - result := ast.NewArray() - // Iterate over the array's elements, and do the following: - // - Drop any Nulls - // - Drop any any empty object/array value (after running the pruner) - for i := range x.Len() { - vTerm := x.Elem(i) - switch v := vTerm.Value.(type) { - case ast.Null: - continue - case *ast.Array: - // Safe, because we knew the type before going to prune it. - va := pruneIrrelevantGraphQLASTNodes(v).(*ast.Array) - if va.Len() > 0 { - result = result.Append(ast.NewTerm(va)) - } - case ast.Object: - // Safe, because we knew the type before going to prune it. - vo := pruneIrrelevantGraphQLASTNodes(v).(ast.Object) - if len(vo.Keys()) > 0 { - result = result.Append(ast.NewTerm(vo)) - } - default: - result = result.Append(vTerm) - } - } - return result - case ast.Object: - result := ast.NewObject() - // Iterate over our object's keys, and do the following: - // - Drop "Position". - // - Drop any key with a Null value. - // - Drop any key with an empty object/array value (after running the pruner) - keys := x.Keys() - for _, k := range keys { - // We drop the "Position" objects because we don't need the - // source-backref/location info they provide for policy rules. - // Note that keys are ast.Strings. - if ast.String("Position").Equal(k.Value) { - continue - } - vTerm := x.Get(k) - switch v := vTerm.Value.(type) { - case ast.Null: - continue - case *ast.Array: - // Safe, because we knew the type before going to prune it. - va := pruneIrrelevantGraphQLASTNodes(v).(*ast.Array) - if va.Len() > 0 { - result.Insert(k, ast.NewTerm(va)) - } - case ast.Object: - // Safe, because we knew the type before going to prune it. - vo := pruneIrrelevantGraphQLASTNodes(v).(ast.Object) - if len(vo.Keys()) > 0 { - result.Insert(k, ast.NewTerm(vo)) - } - default: - result.Insert(k, vTerm) - } - } - return result - default: - return x - } -} - -// Reports errors from parsing/validation. -func builtinGraphQLParse(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - var queryDoc *gqlast.QueryDocument - var schemaDoc *gqlast.SchemaDocument - var err error - - // Parse/translate query if it's a string/object. - switch x := operands[0].Value.(type) { - case ast.String: - queryDoc, err = parseQuery(string(x)) - case ast.Object: - queryDoc, err = objectToQueryDocument(x) - default: - // Error if wrong type. - return builtins.NewOperandTypeErr(0, x, "string", "object") - } - if err != nil { - return err - } - - // Parse/translate schema if it's a string/object. - switch x := operands[1].Value.(type) { - case ast.String: - schemaDoc, err = parseSchema(string(x)) - case ast.Object: - schemaDoc, err = objectToSchemaDocument(x) - default: - // Error if wrong type. - return builtins.NewOperandTypeErr(1, x, "string", "object") - } - if err != nil { - return err - } - - // Transform the ASTs into Objects. - queryASTValue, err := ast.InterfaceToValue(queryDoc) - if err != nil { - return err - } - schemaASTValue, err := ast.InterfaceToValue(schemaDoc) - if err != nil { - return err - } - - // Validate the query against the schema, erroring if there's an issue. - schema, err := convertSchema(schemaDoc) - if err != nil { - return err - } - if err := validateQuery(schema, queryDoc); err != nil { - return err - } - - // Recursively remove irrelevant AST structures. - queryResult := pruneIrrelevantGraphQLASTNodes(queryASTValue.(ast.Object)) - querySchema := pruneIrrelevantGraphQLASTNodes(schemaASTValue.(ast.Object)) - - // Construct return value. - verified := ast.ArrayTerm( - ast.NewTerm(queryResult), - ast.NewTerm(querySchema), - ) - - return iter(verified) -} - -// Returns default value when errors occur. -func builtinGraphQLParseAndVerify(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - var queryDoc *gqlast.QueryDocument - var schemaDoc *gqlast.SchemaDocument - var err error - - // Parse/translate query if it's a string/object. - switch x := operands[0].Value.(type) { - case ast.String: - queryDoc, err = parseQuery(string(x)) - case ast.Object: - queryDoc, err = objectToQueryDocument(x) - default: - // Error if wrong type. - return iter(unverified) - } - if err != nil { - return iter(unverified) - } - - // Parse/translate schema if it's a string/object. - switch x := operands[1].Value.(type) { - case ast.String: - schemaDoc, err = parseSchema(string(x)) - case ast.Object: - schemaDoc, err = objectToSchemaDocument(x) - default: - // Error if wrong type. - return iter(unverified) - } - if err != nil { - return iter(unverified) - } - - // Transform the ASTs into Objects. - queryASTValue, err := ast.InterfaceToValue(queryDoc) - if err != nil { - return iter(unverified) - } - schemaASTValue, err := ast.InterfaceToValue(schemaDoc) - if err != nil { - return iter(unverified) - } - - // Validate the query against the schema, erroring if there's an issue. - schema, err := convertSchema(schemaDoc) - if err != nil { - return iter(unverified) - } - if err := validateQuery(schema, queryDoc); err != nil { - return iter(unverified) - } - - // Recursively remove irrelevant AST structures. - queryResult := pruneIrrelevantGraphQLASTNodes(queryASTValue.(ast.Object)) - querySchema := pruneIrrelevantGraphQLASTNodes(schemaASTValue.(ast.Object)) - - // Construct return value. - verified := ast.ArrayTerm( - ast.InternedTerm(true), - ast.NewTerm(queryResult), - ast.NewTerm(querySchema), - ) - - return iter(verified) -} - -func builtinGraphQLParseQuery(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - raw, err := builtins.StringOperand(operands[0].Value, 1) - if err != nil { - return err - } - - // Get the highly-nested AST struct, along with any errors generated. - query, err := parseQuery(string(raw)) - if err != nil { - return err - } - - // Transform the AST into an Object. - value, err := ast.InterfaceToValue(query) - if err != nil { - return err - } - - // Recursively remove irrelevant AST structures. - result := pruneIrrelevantGraphQLASTNodes(value.(ast.Object)) - - return iter(ast.NewTerm(result)) -} - -func builtinGraphQLParseSchema(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - raw, err := builtins.StringOperand(operands[0].Value, 1) - if err != nil { - return err - } - - // Get the highly-nested AST struct, along with any errors generated. - schema, err := parseSchema(string(raw)) - if err != nil { - return err - } - - // Transform the AST into an Object. - value, err := ast.InterfaceToValue(schema) - if err != nil { - return err - } - - // Recursively remove irrelevant AST structures. - result := pruneIrrelevantGraphQLASTNodes(value.(ast.Object)) - - return iter(ast.NewTerm(result)) -} - -func builtinGraphQLIsValid(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - var queryDoc *gqlast.QueryDocument - var schemaDoc *gqlast.SchemaDocument - var err error - - switch x := operands[0].Value.(type) { - case ast.String: - queryDoc, err = parseQuery(string(x)) - case ast.Object: - queryDoc, err = objectToQueryDocument(x) - default: - // Error if wrong type. - return iter(ast.InternedTerm(false)) - } - if err != nil { - return iter(ast.InternedTerm(false)) - } - - switch x := operands[1].Value.(type) { - case ast.String: - schemaDoc, err = parseSchema(string(x)) - case ast.Object: - schemaDoc, err = objectToSchemaDocument(x) - default: - // Error if wrong type. - return iter(ast.InternedTerm(false)) - } - if err != nil { - return iter(ast.InternedTerm(false)) - } - - // Validate the query against the schema, erroring if there's an issue. - schema, err := convertSchema(schemaDoc) - if err != nil { - return iter(ast.InternedTerm(false)) - } - if err := validateQuery(schema, queryDoc); err != nil { - return iter(ast.InternedTerm(false)) - } - - // If we got this far, the GraphQL query passed validation. - return iter(ast.InternedTerm(true)) -} - -func builtinGraphQLSchemaIsValid(_ BuiltinContext, operands []*ast.Term, iter func(*ast.Term) error) error { - var schemaDoc *gqlast.SchemaDocument - var err error - - switch x := operands[0].Value.(type) { - case ast.String: - schemaDoc, err = parseSchema(string(x)) - case ast.Object: - schemaDoc, err = objectToSchemaDocument(x) - default: - // Error if wrong type. - return iter(ast.InternedTerm(false)) - } - if err != nil { - return iter(ast.InternedTerm(false)) - } - - // Validate the schema, this determines the result - _, err = convertSchema(schemaDoc) - return iter(ast.InternedTerm(err == nil)) -} - -func init() { - RegisterBuiltinFunc(ast.GraphQLParse.Name, builtinGraphQLParse) - RegisterBuiltinFunc(ast.GraphQLParseAndVerify.Name, builtinGraphQLParseAndVerify) - RegisterBuiltinFunc(ast.GraphQLParseQuery.Name, builtinGraphQLParseQuery) - RegisterBuiltinFunc(ast.GraphQLParseSchema.Name, builtinGraphQLParseSchema) - RegisterBuiltinFunc(ast.GraphQLIsValid.Name, builtinGraphQLIsValid) - RegisterBuiltinFunc(ast.GraphQLSchemaIsValid.Name, builtinGraphQLSchemaIsValid) -} diff --git a/topdown/trace.go b/topdown/trace.go index fcd351d7ce..0153e4f090 100644 --- a/topdown/trace.go +++ b/topdown/trace.go @@ -65,7 +65,7 @@ type Event = v1.Event // Tracer defines the interface for tracing in the top-down evaluation engine. // // Deprecated: Use QueryTracer instead. -type Tracer = v1.Tracer +type Tracer = v1.Tracer //nolint:staticcheck // QueryTracer defines the interface for tracing in the top-down evaluation engine. // The implementation can provide additional configuration to modify the tracing diff --git a/v1/ast/parser_test.go b/v1/ast/parser_test.go index 7065970390..b325e30db0 100644 --- a/v1/ast/parser_test.go +++ b/v1/ast/parser_test.go @@ -8250,7 +8250,7 @@ func generateDeeplyNestedArray(depth int) string { func generateDeeplyNestedObject(depth int) string { var sb strings.Builder for i := range depth { - sb.WriteString(fmt.Sprintf(`{"key%d": `, i)) + fmt.Fprintf(&sb, `{"key%d": `, i) } sb.WriteString("1") for range depth { diff --git a/v1/ast/policy_appenders.go b/v1/ast/policy_appenders.go index c7e50cf3c3..1c9813aa97 100644 --- a/v1/ast/policy_appenders.go +++ b/v1/ast/policy_appenders.go @@ -3,6 +3,8 @@ package ast import ( "encoding" "fmt" + + "github.com/open-policy-agent/opa/v1/util" ) func (m *Module) AppendText(buf []byte) ([]byte, error) { @@ -322,3 +324,19 @@ func (d *SomeDecl) AppendText(buf []byte) ([]byte, error) { func (c *Comment) AppendText(buf []byte) ([]byte, error) { return append(append(buf, '#'), c.Text...), nil } + +// RulePath returns the string representation of the rule's path, i.e. its package path followed by the rule head ref. +func RulePath(r *Rule) string { + if r == nil { + return "" + } + if r.Module == nil { + return "" + } + buf := make([]byte, 0, r.Module.Package.Path.StringLength()+r.Head.Ref().StringLength()+1) + buf, _ = r.Module.Package.Path.AppendText(buf) + buf = append(buf, '.') + buf, _ = r.Head.Ref().AppendText(buf) + + return util.ByteSliceToString(buf) +} diff --git a/v1/capabilities/capabilities.go b/v1/capabilities/capabilities.go index ff2c9aa2c7..69ac718ebd 100644 --- a/v1/capabilities/capabilities.go +++ b/v1/capabilities/capabilities.go @@ -5,7 +5,7 @@ package capabilities import ( - v0 "github.com/open-policy-agent/opa/capabilities" + v0 "github.com/open-policy-agent/opa/capabilities" //nolint:staticcheck ) // FS contains the embedded capabilities/ directory of the built version, diff --git a/v1/compile/compile.go b/v1/compile/compile.go index 603daeaad7..e51c1f091f 100644 --- a/v1/compile/compile.go +++ b/v1/compile/compile.go @@ -607,7 +607,7 @@ func (c *Compiler) compilePlan(context.Context) error { extras := ast.NewSet() for rule := range deps { - extras.Add(ast.NewTerm(rule.Path())) + extras.Add(ast.NewTerm(rule.Module.Package.Path.Extend(rule.Head.Ref().GroundPrefix()))) } sorted := extras.Sorted() @@ -796,7 +796,7 @@ func pruneBundleEntrypoints(b *bundle.Bundle, entrypointrefs []*ast.Term) error // Drop any rules that match the entrypoint path. var rules []*ast.Rule for _, rule := range mf.Parsed.Rules { - rulePath := rule.Path() + rulePath := rule.Module.Package.Path.Extend(rule.Head.Ref().GroundPrefix()) if !rulePath.Equal(entrypoint.Value) { rules = append(rules, rule) } else { diff --git a/v1/cover/cover.go b/v1/cover/cover.go index a4447afc8b..0dfb1ee114 100644 --- a/v1/cover/cover.go +++ b/v1/cover/cover.go @@ -6,9 +6,9 @@ package cover import ( - "bytes" "fmt" "slices" + "strings" "sync" "github.com/open-policy-agent/opa/v1/ast" @@ -254,28 +254,29 @@ type CoverageThresholdError struct { } func (e *CoverageThresholdError) Error() string { - var buffer bytes.Buffer - buffer.WriteString(fmt.Sprintf( + sb := &strings.Builder{} + fmt.Fprintf(sb, "Code coverage threshold not met: got %.2f instead of %.2f", e.Coverage, - e.Threshold)) + e.Threshold, + ) if e.Report != nil && len(e.Report.Files) > 0 { - buffer.WriteString("\nLines not covered:") + sb.WriteString("\nLines not covered:") for _, file := range util.KeysSorted(e.Report.Files) { report := e.Report.Files[file] for _, r := range report.NotCovered { if r.Start.Row == r.End.Row { - buffer.WriteString(fmt.Sprintf("\n\t%s:%d", file, r.Start.Row)) + fmt.Fprintf(sb, "\n\t%s:%d", file, r.Start.Row) } else { - buffer.WriteString(fmt.Sprintf("\n\t%s:%d-%d", file, r.Start.Row, r.End.Row)) + fmt.Fprintf(sb, "\n\t%s:%d-%d", file, r.Start.Row, r.End.Row) } } } } - return buffer.String() + return sb.String() } func sortedPositionSliceToRangeSlice(sorted []Position) (result []Range) { diff --git a/v1/cover/cover_bench_test.go b/v1/cover/cover_bench_test.go index eb6376f894..8674a90efc 100644 --- a/v1/cover/cover_bench_test.go +++ b/v1/cover/cover_bench_test.go @@ -53,7 +53,7 @@ func BenchmarkCoverBigLocalVar(b *testing.B) { } func generateModule(numVars int, dataSize int) string { - sb := strings.Builder{} + sb := &strings.Builder{} sb.WriteString(`package test p if { @@ -61,12 +61,12 @@ p if { v := x[i] `) for i := range numVars { - sb.WriteString(fmt.Sprintf("\tv%d := x[i+%d]\n", i, i)) + fmt.Fprintf(sb, "\tv%d := x[i+%d]\n", i, i) } sb.WriteString("\tfalse\n}\n") sb.WriteString("\na := [\n") for i := range dataSize { - sb.WriteString(fmt.Sprintf("\t%d,\n", i)) + fmt.Fprintf(sb, "\t%d,\n", i) } sb.WriteString("]\n") return sb.String() diff --git a/v1/dependencies/deps.go b/v1/dependencies/deps.go index 1635e41545..60ab632cc8 100644 --- a/v1/dependencies/deps.go +++ b/v1/dependencies/deps.go @@ -147,19 +147,13 @@ func virtual(compiler *ast.Compiler, x any, virtualRefs *dependencies) error { } for _, r := range refs { - r = r.ConstantPrefix() - if rules := compiler.GetRules(r); len(rules) > 0 { - for _, rule := range rules { - if virtualRefs.visited(rule) { - continue - } + for _, rule := range compiler.GetRules(r.ConstantPrefix()) { + if !virtualRefs.visited(rule) { virtualRefs.visit(rule) - err := virtual(compiler, rule, virtualRefs) - if err != nil { + if err := virtual(compiler, rule, virtualRefs); err != nil { panic("not reached") } - - virtualRefs.add(rule.Path()) + virtualRefs.add(rule.Module.Package.Path.Extend(rule.Head.Ref().GroundPrefix())) } } } diff --git a/v1/dependencies/deps_bench_test.go b/v1/dependencies/deps_bench_test.go index 47015d001d..a351b2912d 100644 --- a/v1/dependencies/deps_bench_test.go +++ b/v1/dependencies/deps_bench_test.go @@ -59,24 +59,25 @@ func BenchmarkVirtual(b *testing.B) { // makePolicy constructs a policy with ruleCount number of rules. // Each rule will depend on as many other rules as possible without creating circular dependencies. func makePolicy(ruleCount int) string { - var b strings.Builder - b.WriteString("package test\n\n") + sb := &strings.Builder{} - b.WriteString("main if {\n") + sb.WriteString("package test\n\n") + + sb.WriteString("main if {\n") for i := range ruleCount { - b.WriteString(fmt.Sprintf(" p_%d\n", i)) + fmt.Fprintf(sb, " p_%d\n", i) } - b.WriteString("}\n\n") + sb.WriteString("}\n\n") for i := range ruleCount { - b.WriteString(fmt.Sprintf("p_%d if {\n", i)) + fmt.Fprintf(sb, "p_%d if {\n", i) for j := i + 1; j < ruleCount; j++ { - b.WriteString(fmt.Sprintf(" p_%d\n", j)) + fmt.Fprintf(sb, " p_%d\n", j) } - b.WriteString(" input.x == 1\n") - b.WriteString(" input.y == 2\n") - b.WriteString(" input.z == 3\n") - b.WriteString("}\n") + sb.WriteString(" input.x == 1\n") + sb.WriteString(" input.y == 2\n") + sb.WriteString(" input.z == 3\n") + sb.WriteString("}\n") } - return b.String() + return sb.String() } diff --git a/v1/plugins/bundle/plugin.go b/v1/plugins/bundle/plugin.go index ab2e0a193f..88da1b1eb2 100644 --- a/v1/plugins/bundle/plugin.go +++ b/v1/plugins/bundle/plugin.go @@ -18,7 +18,6 @@ import ( "runtime" "strings" "sync" - "time" bundleUtils "github.com/open-policy-agent/opa/internal/bundle" "github.com/open-policy-agent/opa/internal/ref" @@ -591,7 +590,7 @@ func (p *Plugin) checkPluginReadiness() { if !p.ready { readyNow := true // optimistically for _, status := range p.status { - if len(status.Errors) > 0 || (status.LastSuccessfulActivation == time.Time{}) { + if len(status.Errors) > 0 || status.LastSuccessfulActivation.IsZero() { readyNow = false // Not ready yet, check again on next bundle activation. break } @@ -664,7 +663,7 @@ func (p *Plugin) activate(ctx context.Context, name string, b *bundle.Bundle, is if isMultiBundle { activateErr = bundle.Activate(opts) } else { - activateErr = bundle.ActivateLegacy(opts) + activateErr = bundle.ActivateLegacy(opts) //nolint:staticcheck } plugins.SetCompilerOnContext(params.Context, compiler) diff --git a/v1/plugins/bundle/plugin_test.go b/v1/plugins/bundle/plugin_test.go index b7c30a57fa..5b7897c15f 100644 --- a/v1/plugins/bundle/plugin_test.go +++ b/v1/plugins/bundle/plugin_test.go @@ -4311,7 +4311,8 @@ func TestPluginRequestVsDownloadTimestamp(t *testing.T) { // simulate HTTP 200 response from downloader _ = plugin.oneShot(ctx, bundleName, download.Update{Bundle: b}) - if plugin.status[bundleName].LastSuccessfulDownload != plugin.status[bundleName].LastSuccessfulRequest || plugin.status[bundleName].LastSuccessfulDownload != plugin.status[bundleName].LastRequest { + if !plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastSuccessfulRequest) || + !plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastRequest) { t.Fatal("expected last successful request to be same as download and request") } @@ -4321,21 +4322,24 @@ func TestPluginRequestVsDownloadTimestamp(t *testing.T) { // simulate HTTP 304 response from downloader. _ = plugin.oneShot(ctx, bundleName, download.Update{Bundle: nil}) - if plugin.status[bundleName].LastSuccessfulDownload == plugin.status[bundleName].LastSuccessfulRequest || plugin.status[bundleName].LastSuccessfulDownload == plugin.status[bundleName].LastRequest { + if plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastSuccessfulRequest) || + plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastRequest) { t.Fatal("expected last successful request to differ from download and request") } // simulate HTTP 200 response from downloader _ = plugin.oneShot(ctx, bundleName, download.Update{Bundle: b}) - if plugin.status[bundleName].LastSuccessfulDownload != plugin.status[bundleName].LastSuccessfulRequest || plugin.status[bundleName].LastSuccessfulDownload != plugin.status[bundleName].LastRequest { + if !plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastSuccessfulRequest) || + !plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastRequest) { t.Fatal("expected last successful request to be same as download and request") } // simulate error response from downloader _ = plugin.oneShot(ctx, bundleName, download.Update{Error: errors.New("xxx")}) - if plugin.status[bundleName].LastSuccessfulDownload != plugin.status[bundleName].LastSuccessfulRequest || plugin.status[bundleName].LastSuccessfulDownload == plugin.status[bundleName].LastRequest { + if !plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastSuccessfulRequest) || + plugin.status[bundleName].LastSuccessfulDownload.Equal(plugin.status[bundleName].LastRequest) { t.Fatal("expected last successful request to be same as download but different from request") } } diff --git a/v1/plugins/discovery/discovery.go b/v1/plugins/discovery/discovery.go index 67e72833cf..e4e76c3101 100644 --- a/v1/plugins/discovery/discovery.go +++ b/v1/plugins/discovery/discovery.go @@ -639,7 +639,7 @@ func getPluginSet( // Parse and validate bundle/logs/status configurations. // If `bundle` was configured use that, otherwise try the new `bundles` option - bundleConfig, err := bundle.ParseConfig(config.Bundle, serviceNames) + bundleConfig, err := bundle.ParseConfig(config.Bundle, serviceNames) //nolint:staticcheck if err != nil { return nil, err } @@ -760,7 +760,7 @@ func registerBundleStatusUpdates(m *plugins.Manager) { // Depending on how the plugin was configured we will want to use different listeners // for backwards compatibility. if !bp.Config().IsMultiBundle() { - bp.Register(pluginlistener(status.Name), sp.UpdateBundleStatus) + bp.Register(pluginlistener(status.Name), sp.UpdateBundleStatus) //nolint:staticcheck } else { bp.RegisterBulkListener(pluginlistener(status.Name), sp.BulkUpdateBundleStatus) } diff --git a/v1/plugins/discovery/discovery_test.go b/v1/plugins/discovery/discovery_test.go index 798767d49a..80e4220344 100644 --- a/v1/plugins/discovery/discovery_test.go +++ b/v1/plugins/discovery/discovery_test.go @@ -3143,7 +3143,8 @@ func TestStatusUpdatesTimestamp(t *testing.T) { t.Fatal(err) } - if disco.status.LastSuccessfulDownload != disco.status.LastSuccessfulRequest || disco.status.LastSuccessfulDownload != disco.status.LastRequest { + if !disco.status.LastSuccessfulDownload.Equal(disco.status.LastSuccessfulRequest) || + !disco.status.LastSuccessfulDownload.Equal(disco.status.LastRequest) { t.Fatal("expected last successful request to be same as download and request") } @@ -3158,7 +3159,8 @@ func TestStatusUpdatesTimestamp(t *testing.T) { if err != nil { t.Fatal(err) } - if disco.status.LastSuccessfulDownload == disco.status.LastSuccessfulRequest || disco.status.LastSuccessfulDownload == disco.status.LastRequest { + if disco.status.LastSuccessfulDownload.Equal(disco.status.LastSuccessfulRequest) || + disco.status.LastSuccessfulDownload.Equal(disco.status.LastRequest) { t.Fatal("expected last successful download to differ from request and last request") } @@ -3172,7 +3174,8 @@ func TestStatusUpdatesTimestamp(t *testing.T) { t.Fatal(err) } - if disco.status.LastSuccessfulDownload != disco.status.LastSuccessfulRequest || disco.status.LastSuccessfulDownload != disco.status.LastRequest { + if !disco.status.LastSuccessfulDownload.Equal(disco.status.LastSuccessfulRequest) || + !disco.status.LastSuccessfulDownload.Equal(disco.status.LastRequest) { t.Fatal("expected last successful request to be same as download and request") } @@ -3186,7 +3189,8 @@ func TestStatusUpdatesTimestamp(t *testing.T) { t.Fatal(err) } - if disco.status.LastSuccessfulDownload != disco.status.LastSuccessfulRequest || disco.status.LastSuccessfulDownload == disco.status.LastRequest { + if !disco.status.LastSuccessfulDownload.Equal(disco.status.LastSuccessfulRequest) || + disco.status.LastSuccessfulDownload.Equal(disco.status.LastRequest) { t.Fatal("expected last successful request to be same as download but different from request") } } diff --git a/v1/profiler/profiler_bench_test.go b/v1/profiler/profiler_bench_test.go index ac4e01dd20..cf9be6f42b 100644 --- a/v1/profiler/profiler_bench_test.go +++ b/v1/profiler/profiler_bench_test.go @@ -52,7 +52,7 @@ func BenchmarkProfilerBigLocalVar(b *testing.B) { } func generateModule(numVars int, dataSize int) string { - sb := strings.Builder{} + sb := &strings.Builder{} sb.WriteString(`package test p if { @@ -60,12 +60,12 @@ p if { v := x[i] `) for i := range numVars { - sb.WriteString(fmt.Sprintf("\tv%d := x[i+%d]\n", i, i)) + fmt.Fprintf(sb, "\tv%d := x[i+%d]\n", i, i) } sb.WriteString("\tfalse\n}\n") sb.WriteString("\na := [\n") for i := range dataSize { - sb.WriteString(fmt.Sprintf("\t%d,\n", i)) + fmt.Fprintf(sb, "\t%d,\n", i) } sb.WriteString("]\n") return sb.String() diff --git a/v1/rego/rego.go b/v1/rego/rego.go index 042d8014ea..a69dba1bb8 100644 --- a/v1/rego/rego.go +++ b/v1/rego/rego.go @@ -42,10 +42,7 @@ import ( const ( defaultPartialNamespace = "partial" wasmVarPrefix = "^" -) -// nolint:varcheck -const ( targetWasm = "wasm" targetRego = "rego" ) @@ -1371,6 +1368,7 @@ func New(options ...func(r *Rego)) *Rego { callHook := r.compiler == nil // call hook only if we created the compiler here if r.compiler == nil { + //nolint:staticcheck r.compiler = ast.NewCompiler(). WithUnsafeBuiltins(r.unsafeBuiltins). WithBuiltins(r.builtinDecls). @@ -2496,7 +2494,7 @@ func (r *Rego) partialResult(ctx context.Context, pCfg *PrepareConfig) (PartialR Module: module, } module.Rules[i] = rule - if checkPartialResultForRecursiveRefs(body, rule.Path()) { + if checkPartialResultForRecursiveRefs(body, module.Package.Path.Extend(rule.Head.Reference.GroundPrefix())) { return PartialResult{}, Errors{errPartialEvaluationNotEffective} } } @@ -2687,7 +2685,7 @@ func (r *Rego) rewriteQueryToCaptureValue(_ ast.QueryCompiler, query ast.Body) ( expr.Terms = ast.Equality.Expr(terms, capture).Terms r.capture[expr] = capture.Value.(ast.Var) case []*ast.Term: - tpe := r.compiler.TypeEnv.Get(terms[0]) + tpe := r.compiler.TypeEnv.GetByValue(terms[0].Value) if !types.Void(tpe) && types.Arity(tpe) == len(terms)-1 { capture = r.generateTermVar() expr.Terms = append(terms, capture) diff --git a/v1/repl/repl.go b/v1/repl/repl.go index 4b1af65faf..87a7ddd7db 100644 --- a/v1/repl/repl.go +++ b/v1/repl/repl.go @@ -405,7 +405,7 @@ func (r *REPL) complete(line string) []string { // add virtual docs defined in repl for _, mod := range r.modules { for _, rule := range mod.Rules { - path := rule.Path().String() + path := ast.RulePath(rule) if strings.HasPrefix(path, line) { set[path] = struct{}{} } @@ -421,8 +421,7 @@ func (r *REPL) complete(line string) []string { // add virtual docs defined by policies for _, mod := range mods { for _, rule := range mod.Rules { - path := rule.Path().String() - if strings.HasPrefix(path, line) { + if path := ast.RulePath(rule); strings.HasPrefix(path, line) { set[path] = struct{}{} } } @@ -1305,7 +1304,7 @@ func (r *REPL) printTypes(_ context.Context, typeEnv *ast.TypeEnv, body ast.Body vis.Walk(body) for v := range vis.Vars() { - fmt.Fprintf(r.output, "# %v: %v\n", v, typeEnv.Get(v)) + fmt.Fprintf(r.output, "# %v: %v\n", v, typeEnv.GetByValue(v)) } } diff --git a/v1/runtime/runtime.go b/v1/runtime/runtime.go index 859396a583..72b0275847 100644 --- a/v1/runtime/runtime.go +++ b/v1/runtime/runtime.go @@ -511,7 +511,7 @@ func NewRuntime(ctx context.Context, params Params) (*Runtime, error) { plugins.WithRouter(params.Router), plugins.WithPrometheusRegister(metrics), plugins.WithTracerProvider(tracerProvider), - plugins.WithEnableTelemetry(params.EnableVersionCheck), + plugins.WithEnableVersionCheck(params.EnableVersionCheck), plugins.WithParserOptions(params.parserOptions()), plugins.WithDistributedTracingOpts(params.DistributedTracingOpts), plugins.WithBundleActivatorPlugin(params.BundleActivatorPlugin), diff --git a/v1/server/server.go b/v1/server/server.go index 45d68929ed..6de543abac 100644 --- a/v1/server/server.go +++ b/v1/server/server.go @@ -1033,7 +1033,7 @@ func getRevisions(ctx context.Context, store storage.Store, txn storage.Transact br.Revisions = map[string]string{} // Check if we still have a legacy bundle manifest in the store - br.LegacyRevision, err = bundle.LegacyReadRevisionFromStore(ctx, store, txn) + br.LegacyRevision, err = bundle.LegacyReadRevisionFromStore(ctx, store, txn) //nolint:staticcheck if err != nil && !storage.IsNotFound(err) { return br, err } @@ -1269,7 +1269,7 @@ func (*Server) bundlesReady(pluginStatuses map[string]*plugins.Status) bool { func (s *Server) unversionedGetHealth(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - includeBundleStatus := getBoolParam(r.URL, types.ParamBundleActivationV1, true) || + includeBundleStatus := getBoolParam(r.URL, types.ParamBundleActivationV1, true) || //nolint:staticcheck getBoolParam(r.URL, types.ParamBundlesActivationV1, true) includePluginStatus := getBoolParam(r.URL, types.ParamPluginsV1, true) excludePlugin := getStringSliceParam(r.URL, types.ParamExcludePluginV1) @@ -1388,7 +1388,11 @@ func (s *Server) unversionedGetHealthWithPolicy(w http.ResponseWriter, r *http.R func writeHealthResponse(w http.ResponseWriter, err error) { if err != nil { - writer.JSON(w, http.StatusInternalServerError, types.HealthResponseV1{Error: err.Error()}, false) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + if err := json.NewEncoder(w).Encode(types.HealthResponseV1{Error: err.Error()}); err != nil { + writer.ErrorAuto(w, err) + } return } diff --git a/v1/tester/runner.go b/v1/tester/runner.go index 2b45721c77..4897a4c7f1 100644 --- a/v1/tester/runner.go +++ b/v1/tester/runner.go @@ -170,15 +170,24 @@ func (r *Result) String() string { } func (r *Result) string(subResults bool) string { - if r.Skip { - return fmt.Sprintf("%v.%v: %v", r.Package, r.Name, r.outcome()) - } var buf bytes.Buffer - buf.WriteString(fmt.Sprintf("%v.%v: %v (%v)", r.Package, r.Name, r.outcome(), r.Duration)) + buf.WriteString(r.Package) + buf.WriteByte('.') + buf.WriteString(r.Name) + buf.WriteString(": ") + buf.WriteString(r.outcome()) + + if r.Skip { + return buf.String() + } + + buf.WriteString(" (") + buf.WriteString(r.Duration.String()) + buf.WriteByte(')') if subResults { - buf.WriteString("\n") + buf.WriteByte('\n') buf.WriteString(r.SubResults.String()) } @@ -199,7 +208,7 @@ func (r *Result) outcome() string { } func (sr *SubResult) String() string { - return fmt.Sprintf("%v: %v", sr.Name, sr.outcome()) + return sr.Name + ": " + sr.outcome() } func (sr *SubResult) outcome() string { @@ -236,10 +245,9 @@ func (srm SubResultMap) String() string { func (srm SubResultMap) string(indent string) string { var buf bytes.Buffer for fullName, sr := range srm.Iter { - buf.WriteString(fmt.Sprintf("%s%s\n", - strings.Repeat(indent, len(fullName)-1), - sr.String(), - )) + buf.WriteString(strings.Repeat(indent, len(fullName)-1)) + buf.WriteString(sr.String()) + buf.WriteByte('\n') } return buf.String() } @@ -868,11 +876,12 @@ func moveExpr(body ast.Body, from int, to int) (ast.Body, bool) { // use rule.Head.Ref() func ruleName(h *ast.Head) (string, ast.Ref) { var n string - var ref ast.Ref - for _, term := range h.Ref().GroundPrefix() { - ref = ref.Append(term) - switch v := term.Value.(type) { + rgp := h.Ref().GroundPrefix() + i := 0 + + for i = range rgp { + switch v := rgp[i].Value.(type) { case ast.Var: n = string(v) case ast.String: @@ -880,13 +889,12 @@ func ruleName(h *ast.Head) (string, ast.Ref) { default: n = "" } - if strings.HasPrefix(n, TestPrefix) || strings.HasPrefix(n, SkipTestPrefix) { break } } - return n, ref + return n, rgp[:i+1] } func (r *Runner) runTest(ctx context.Context, txn storage.Transaction, mod *ast.Module, rule *ast.Rule) (*Result, bool) { @@ -1067,7 +1075,6 @@ func subResult(n string, v any) *SubResult { func (r *Runner) runBenchmark(ctx context.Context, txn storage.Transaction, mod *ast.Module, rule *ast.Rule, options BenchmarkOptions) (*Result, bool) { _, rf := ruleName(rule.Head) - tr := &Result{ Location: rule.Loc(), Package: mod.Package.Path.String(), @@ -1079,12 +1086,11 @@ func (r *Runner) runBenchmark(ctx context.Context, txn storage.Transaction, mod t0 := time.Now() br := testing.Benchmark(func(b *testing.B) { - pq, err := rego.New( rego.Store(r.store), rego.Transaction(txn), rego.Compiler(r.compiler), - rego.Query(rule.Path().String()), + rego.Query(rule.Module.Package.Path.Extend(rule.Head.Ref().GroundPrefix()).String()), rego.Runtime(r.runtime), rego.Target(r.target), ).PrepareForEval(ctx) diff --git a/v1/topdown/eval.go b/v1/topdown/eval.go index 0d4c899635..6f93ba530e 100644 --- a/v1/topdown/eval.go +++ b/v1/topdown/eval.go @@ -318,7 +318,6 @@ func (e *eval) traceUnify(a, b *ast.Term) { } func (e *eval) traceEvent(op Op, x ast.Node, msg string, target *ast.Ref) { - if !e.traceEnabled { return } @@ -3374,15 +3373,26 @@ func (vcKeyScope) IsGround() bool { } func (q vcKeyScope) String() string { - buf := make([]string, 0, len(q.Ref)) + buf, _ := q.AppendText(make([]byte, 0, 2+q.StringLength())) + return util.ByteSliceToString(buf) +} + +func (q vcKeyScope) AppendText(buf []byte) ([]byte, error) { + buf = append(buf, '<') for _, t := range q.Ref { if _, ok := t.Value.(ast.Var); ok { - buf = append(buf, "_") + buf = append(buf, '_') } else { - buf = append(buf, t.String()) + var err error + if buf, err = t.AppendText(buf); err != nil { + return nil, err + } } + buf = append(buf, ',') } - return fmt.Sprintf("<%s>", strings.Join(buf, ",")) + buf[len(buf)-1] = '>' + + return buf, nil } // reduce removes vars from the tail of the ref. @@ -3633,6 +3643,7 @@ func (e evalVirtualComplete) evalValueRule(iter unifyIterator, rule *ast.Rule, p e.e.childWithBindingSizeHint(rule.Body, child, ast.EstimateBodyBindingCount(rule.Body)) child.findOne = findOne child.traceEnter(rule) + var result *ast.Term err := child.eval(func(child *eval) error { child.traceExit(rule) @@ -3651,8 +3662,7 @@ func (e evalVirtualComplete) evalValueRule(iter unifyIterator, rule *ast.Rule, p e.e.virtualCache.Put(e.plugged[:e.pos+1], result) term, termbindings := child.bindings.apply(rule.Head.Value) - err := e.evalTerm(iter, term, termbindings) - if err != nil { + if err := e.evalTerm(iter, term, termbindings); err != nil { return err } @@ -3676,8 +3686,7 @@ func (e evalVirtualComplete) partialEval(iter unifyIterator) error { child.traceExit(rule) term, termbindings := child.bindings.apply(rule.Head.Value) - err := e.evalTerm(iter, term, termbindings) - if err != nil { + if err := e.evalTerm(iter, term, termbindings); err != nil { return err } @@ -4329,7 +4338,7 @@ func isFunction(env *ast.TypeEnv, ref any) bool { default: panic("expected ast.Value or *ast.Term") } - _, ok := env.Get(r).(*types.Function) + _, ok := env.GetByRef(r).(*types.Function) return ok } diff --git a/v1/topdown/graphql.go b/v1/topdown/graphql.go index ba65e973f2..c47f7dc4e6 100644 --- a/v1/topdown/graphql.go +++ b/v1/topdown/graphql.go @@ -13,15 +13,15 @@ import ( gqlast "github.com/vektah/gqlparser/v2/ast" gqlparser "github.com/vektah/gqlparser/v2/parser" gqlvalidator "github.com/vektah/gqlparser/v2/validator" - - // Side-effecting import. Triggers GraphQL library's validation rule init() functions. - _ "github.com/vektah/gqlparser/v2/validator/rules" + "github.com/vektah/gqlparser/v2/validator/rules" "github.com/open-policy-agent/opa/v1/ast" "github.com/open-policy-agent/opa/v1/topdown/builtins" "github.com/open-policy-agent/opa/v1/topdown/cache" ) +var defaultRules = rules.NewDefaultRules() + // Parses a GraphQL schema, and returns the GraphQL AST for the schema. func parseSchema(schema string) (*gqlast.SchemaDocument, error) { // NOTE(philipc): We don't include the "built-in schema defs" from the @@ -51,8 +51,7 @@ func parseQuery(query string) (*gqlast.QueryDocument, error) { // just the first error message in the list. func validateQuery(schema *gqlast.Schema, query *gqlast.QueryDocument) error { // Validate the query against the schema, erroring if there's an issue. - err := gqlvalidator.Validate(schema, query) - if err != nil { + if err := gqlvalidator.ValidateWithRules(schema, query, defaultRules); err != nil { return formatGqlParserError(err) } return nil @@ -674,7 +673,6 @@ func cacheKeyWithPrefix(bctx BuiltinContext, t *ast.Term, prefix string) (string const gqlCacheName = "graphql" func init() { - var defaultCacheEntries = 10 var graphqlCacheConfig = cache.NamedValueCacheConfig{ MaxNumEntries: &defaultCacheEntries, diff --git a/v1/topdown/trace.go b/v1/topdown/trace.go index 49748dcace..52451dc6bd 100644 --- a/v1/topdown/trace.go +++ b/v1/topdown/trace.go @@ -411,7 +411,7 @@ func formatEvent(event *Event, depth int) string { var details any if node, ok := event.Node.(*ast.Rule); ok { - details = node.Path() + details = ast.RulePath(node) } else if event.Ref != nil { details = event.Ref } else {